Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crash report fixes #15147

Merged
merged 20 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/DynamoCore/Models/DynamoModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,7 @@ protected DynamoModel(IStartConfiguration config)
LogWarningMessageEvents.LogWarningMessage += LogWarningMessage;
LogWarningMessageEvents.LogInfoMessage += LogInfoMessage;
DynamoConsoleLogger.LogMessageToDynamoConsole += LogMessageWrapper;
DynamoConsoleLogger.LogErrorToDynamoConsole += LogErrorMessageWrapper;
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
StartBackupFilesTimer();

TraceReconciliationProcessor = this;
Expand Down Expand Up @@ -1425,6 +1426,7 @@ public void Dispose()
LogWarningMessageEvents.LogWarningMessage -= LogWarningMessage;
LogWarningMessageEvents.LogInfoMessage -= LogInfoMessage;
DynamoConsoleLogger.LogMessageToDynamoConsole -= LogMessageWrapper;
DynamoConsoleLogger.LogErrorToDynamoConsole -= LogErrorMessageWrapper;
foreach (var ws in _workspaces)
{
ws.Dispose();
Expand Down Expand Up @@ -3296,11 +3298,17 @@ private void LogMessage(ILogMessage obj)
{
Logger.Log(obj);
}

private void LogMessageWrapper(string m)
{
LogMessage(Dynamo.Logging.LogMessage.Info(m));
}

private void LogErrorMessageWrapper(string m)
{
LogMessage(Dynamo.Logging.LogMessage.Error(m));
}

#if DEBUG_LIBRARY
private void DumpLibrarySnapshot(IEnumerable<Engine.FunctionGroup> functionGroups)
{
Expand Down
19 changes: 17 additions & 2 deletions src/DynamoCoreWpf/UI/Prompts/CrashPrompt.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Windows.Markup;
using System.Xml;
using Dynamo.Core;
using Dynamo.Models;
using Dynamo.PackageManager;
using Dynamo.ViewModels;

Expand Down Expand Up @@ -47,11 +48,25 @@ public CrashPrompt(DynamoViewModel dynamoViewModel)
txtOverridingText.Text = string.Format(Wpf.Properties.Resources.CrashPromptDialogCrashMessage, productName);
}

public CrashPrompt(CrashPromptArgs args, DynamoViewModel dynamoViewModel)
public CrashPrompt(CrashPromptArgs args, DynamoViewModel dynamoViewModel) : this(dynamoViewModel, args)
{}

internal CrashPrompt(object sender, CrashPromptArgs args)
{
DynamoModel model = null;
var dynamoViewModel = sender as DynamoViewModel;
if (dynamoViewModel != null)
{
model = dynamoViewModel.Model;
}
else if (sender is DynamoModel dm)
{
model = dm;
}

InitializeComponent();

var packageLoader = dynamoViewModel?.Model?.GetPackageManagerExtension()?.PackageLoader;
var packageLoader = model?.GetPackageManagerExtension()?.PackageLoader;
markdownPackages = Wpf.Utilities.CrashUtilities.PackagesToMakrdown(packageLoader);

productName = dynamoViewModel?.BrandingResourceProvider.ProductName ?? Process.GetCurrentProcess().ProcessName;
Expand Down
24 changes: 13 additions & 11 deletions src/DynamoCoreWpf/Utilities/CrashReportTool.cs
Original file line number Diff line number Diff line change
Expand Up @@ -164,33 +164,35 @@ private static string FindCERToolInInstallLocations()

internal static void ShowCrashWindow(object sender, CrashPromptArgs args)
{
var viewModel = sender as DynamoViewModel;

if (CrashReportTool.ShowCrashErrorReportWindow(viewModel,
if (CrashReportTool.ShowCrashErrorReportWindow(sender,
(args is CrashErrorReportArgs cerArgs) ? cerArgs :
new CrashErrorReportArgs(args.Details)))
{
return;
}
// Backup crash reporting dialog (in case ADSK CER is not found)
var prompt = new Nodes.Prompts.CrashPrompt(args, viewModel);
var prompt = new Nodes.Prompts.CrashPrompt(sender, args);
prompt.ShowDialog();
}

/// <summary>
/// Calls external CER tool (with UI)
/// </summary>
/// <param name="viewModel">The Dynamo view model</param>
/// <param name="args"></param>
/// <param name="sender">Either a DynamoViewModel or a DynamoModel object</param>
/// <param name="args">Arguments consumed by the Crash report UI</param>
/// <returns>True if the CER tool process was successfully started. False otherwise</returns>
internal static bool ShowCrashErrorReportWindow(DynamoViewModel viewModel, CrashErrorReportArgs args)
private static bool ShowCrashErrorReportWindow(object sender, CrashErrorReportArgs args)
{
if (DynamoModel.FeatureFlags?.CheckFeatureFlag("CER_v2", false) == false)
DynamoModel model = null;
var viewModel = sender as DynamoViewModel;
if (viewModel != null)
{
return false;
model = viewModel.Model;
}
else if (sender is DynamoModel dm)
{
model = dm;
}

DynamoModel model = viewModel?.Model;

string cerToolDir = !string.IsNullOrEmpty(model?.CERLocation) ?
model?.CERLocation : FindCERToolInInstallLocations();
Expand Down
54 changes: 48 additions & 6 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.Loader;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
Expand Down Expand Up @@ -46,8 +47,10 @@
using Dynamo.Wpf.ViewModels.FileTrust;
using Dynamo.Wpf.ViewModels.Watch3D;
using DynamoMLDataPipeline;
using DynamoServices;
using DynamoUtilities;
using ICSharpCode.AvalonEdit;
using J2N.Text;
using Newtonsoft.Json;
using PythonNodeModels;
using ISelectable = Dynamo.Selection.ISelectable;
Expand Down Expand Up @@ -848,10 +851,18 @@ private void CurrentDispatcher_UnhandledException(object sender, DispatcherUnhan
return;
}

// Try to handle the exception so that the host app can continue (in most cases).
// In some cases Dynamo code might still crash after this handler kicks in. In these edge cases
// we might see 2 CER windows (the extra one from the host app) - CER tool might handle this in the future.
e.Handled = true;
if (DynamoModel.IsTestMode)
{
// Do not handle the exception in test mode.
// Let the test host handle it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might have covered this at standup but can you remind me again what you see in tests if the exception is marked as handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If (during tests) the exception is marked as handled inside the ex_handler, then the test that threw the exception might pass (as if nothing was thrown)
Our tests have a single entry point for async tasks (we run these with DispatcherUtil.DoLoop). Whenever an async task throws an exception (and if unhandled) it will propagate until the DIspatcherUtil.DoLoop calls and eventually be caught by the nunit test runner

}
else
{
// Try to handle the exception so that the host app can continue (in most cases).
// In some cases Dynamo code might still crash after this handler kicks in. In these edge cases
// we might see 2 CER windows (the extra one from the host app) - CER tool might handle this in the future.
e.Handled = true;
}

CrashGracefully(e.Exception);
}
Expand All @@ -864,14 +875,45 @@ private void CurrentDomain_UnhandledException(object sender, UnhandledExceptionE
}
}

private void CrashGracefully(Exception ex, bool fatal = false)
// CrashGracefully should only be used in the DynamoViewModel class or within tests.
internal void CrashGracefully(Exception ex, bool fatal = false)
{
try
{
var exceptionAssembly = ex.TargetSite?.Module?.Assembly;
// TargetInvocationException is the exception that is thrown by methods invoked through reflection
// The inner exception contains the originating exception.
// https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/7.0/reflection-invoke-exceptions
if (ex is TargetInvocationException && ex.InnerException != null)
{
exceptionAssembly = ex.InnerException?.TargetSite?.Module?.Assembly;
}

// Do not crash if the exception is coming from a 3d party package;
if (!fatal && exceptionAssembly != null)
{
// Check if the exception might be coming from a loaded package assembly.
var faultyPkg = Model.GetPackageManagerExtension()?.PackageLoader?.LocalPackages?.FirstOrDefault(p => exceptionAssembly.Location.StartsWith(p.RootDirectory, StringComparison.OrdinalIgnoreCase));
if (faultyPkg != null)
{
var crashDetails = new CrashErrorReportArgs(ex);
DynamoConsoleLogger.OnLogErrorToDynamoConsole($"Unhandled exception coming from package {faultyPkg.Name} was handled: {crashDetails.Details}");
Analytics.TrackException(ex, false);
return;
}
}

DynamoModel.IsCrashing = true;
var crashData = new CrashErrorReportArgs(ex);
Model?.Logger?.LogError($"Unhandled exception: {crashData.Details} ");
DynamoConsoleLogger.OnLogErrorToDynamoConsole($"Unhandled exception: {crashData.Details} ");
Analytics.TrackException(ex, true);

if (DynamoModel.IsTestMode)
{
// Do not show the crash UI during tests.
return;
}

Model?.OnRequestsCrashPrompt(crashData);

if (fatal)
Expand Down
6 changes: 6 additions & 0 deletions src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@ private void WebView_NavigationCompleted(object sender, CoreWebView2NavigationCo
}
catch (Exception ex)
{
if (DynamoModel.IsTestMode)
{
// Rethrow exception during testing.
throw;
}

if (!DynamoModel.IsCrashing && !IsClosing)
{
CrashReportTool.ShowCrashWindow(viewModel, new CrashErrorReportArgs(ex));
Expand Down
3 changes: 1 addition & 2 deletions src/DynamoPackages/Package.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class PackageAssembly
{
public bool IsNodeLibrary { get; set; }
public Assembly Assembly { get; set; }
public string LocalFilePath {get;set;}
public string LocalFilePath {get;set;}

public string Name
{
Expand Down Expand Up @@ -358,7 +358,6 @@ internal IEnumerable<PackageAssembly> EnumerateAndLoadAssembliesInBinDirectory()
// Use the pkg header to determine which assemblies to load and prevent multiple enumeration
// In earlier packages, this field could be null, which is correctly handled by IsNodeLibrary
var nodeLibraries = Header.node_libraries;

foreach (var assemFile in new DirectoryInfo(BinaryDirectory).EnumerateFiles("*.dll"))
{
Assembly assem;
Expand Down
7 changes: 7 additions & 0 deletions src/NodeServices/MessageEvents.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,16 @@ internal static class DynamoConsoleLogger
{
public static event Action<string> LogMessageToDynamoConsole;

public static event Action<string> LogErrorToDynamoConsole;
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved

public static void OnLogMessageToDynamoConsole(string message)
{
LogMessageToDynamoConsole?.Invoke(message);
}

public static void OnLogErrorToDynamoConsole(string message)
{
LogErrorToDynamoConsole?.Invoke(message);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
using Dynamo.PackageManager.UI;
using Dynamo.Scheduler;
using Dynamo.Wpf.Extensions;
using DynamoCoreWpfTests.Utility;
using DynamoServices;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Moq;
Expand Down Expand Up @@ -314,7 +316,49 @@ void PackageManagerViewExtensionTests_NotificationLogged(NotificationMessage obj
}
}

[Test]
public void TestCrashInPackage()
{
var pkgDir = Path.Combine(PackagesDirectory, "SampleViewExtension_Crash");
var currentDynamoModel = ViewModel.Model;
var loader = currentDynamoModel.GetPackageManagerExtension().PackageLoader;

var pkg = loader.ScanPackageDirectory(pkgDir);
Assert.IsNotNull(pkg);

loader.LoadPackages(new List<Package>() { pkg });

Exception expectedEx = null;
var assem = AppDomain.CurrentDomain.GetAssemblies().Where(x => x.GetName().Name == "TestCrash").FirstOrDefault();
Assert.IsNotNull(assem);

try
{
assem.GetType("TestCrash.Class1").GetMethod("TestCrash").Invoke(null, new object[] { });
}
catch (Exception ex)
{
expectedEx = ex;
}

int count = 0;
void DynamoConsoleLogger_LogErrorToDynamoConsole(string obj)
{
if (obj.Contains($"Unhandled exception coming from package {pkg.Name} was handled"))
{
count++;
}
}

DynamoConsoleLogger.LogErrorToDynamoConsole += DynamoConsoleLogger_LogErrorToDynamoConsole;

ViewModel.CrashGracefully(expectedEx);

DynamoConsoleLogger.LogErrorToDynamoConsole -= DynamoConsoleLogger_LogErrorToDynamoConsole;

ViewModel.CrashGracefully(expectedEx);
Assert.AreEqual(1, count);
}

[OneTimeTearDown]
/// <summary>
Expand Down
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<ViewExtensionDefinition>
<AssemblyPath>..\bin\SampleViewExtensionCrash.dll</AssemblyPath>
<TypeName>SampleViewExtension.SampleViewExtension</TypeName>
</ViewExtensionDefinition>
17 changes: 17 additions & 0 deletions test/pkgs/SampleViewExtension_Crash/pkg.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"file_hash":null,
"name":"Sample View Extension",
"version":"2.0.1",
"description":"Dynamo sample view extension.",
"group":"",
"keywords":["dynamo","samples, view, extension, nodes"],
"dependencies":[],
"license":"MIT",
"contents":"",
"engine_version":"2.0.1",
"engine_metadata":"",
"engine":"dynamo",
"site_url": "http://dynamobim.org/",
"repository_url": "https://github.com/DynamoDS/DynamoSamples",
"node_libraries": [ ]
}
Loading