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 4 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
33 changes: 32 additions & 1 deletion src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
using Dynamo.Wpf.ViewModels.FileTrust;
using Dynamo.Wpf.ViewModels.Watch3D;
using DynamoMLDataPipeline;
using DynamoServices;
using DynamoUtilities;
using ICSharpCode.AvalonEdit;
using Newtonsoft.Json;
Expand Down Expand Up @@ -839,10 +840,40 @@ private void CrashGracefully(Exception ex, bool fatal = false)
{
try
{
if (!fatal)
{
// Check if the exception might be coming from a loaded package assembly.
var faultyPkg = Model.GetPackageManagerExtension()?.PackageLoader?.LocalPackages?.FirstOrDefault(p =>
{
// Find the first assembly that was loaded fromm the package folder and has the same name as the exception source.
return p.LoadedAssemblies.FirstOrDefault(a => a.WasLoadedFromPackage && (a.Name == ex.Source)) != null;
});
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);

if (DynamoModel.IsTestMode)
{
// Rethrow the exception during testing.
throw ex;
}
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)
{
// Rethrow the exception during testing.
throw ex;
}

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
18 changes: 15 additions & 3 deletions src/DynamoPackages/Package.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ public class PackageAssembly
{
public bool IsNodeLibrary { get; set; }
public Assembly Assembly { get; set; }
public string LocalFilePath {get;set;}
public string LocalFilePath {get;set;}
// WasLoadedFromPackage is a flag that represents if the Assembly was actually loaded from the package folder (value = true)
// or if the assembly was already loaded in the appdomain (value = false)
internal bool WasLoadedFromPackage { get; set; }

public string Name
{
Expand Down Expand Up @@ -358,8 +361,14 @@ 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;

List<string> appDomainAssemblies = null;
var binFiles = new DirectoryInfo(BinaryDirectory).EnumerateFiles("*.dll");
if (binFiles.Any()) {
appDomainAssemblies = AppDomain.CurrentDomain.GetAssemblies().Select(x => x.GetName().Name).ToList();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hopefully not too much of a performance hit here.
This will run for every loaded pkg that has at least one binary on disk.

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 I try to move this a level up, the assembly list might change (because we iterate all the types and dependencies might get loaded)

Copy link
Member

Choose a reason for hiding this comment

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

have you considered how ALCs and packages containing extensions or binaries in other folders might affect this check?

Copy link
Member

Choose a reason for hiding this comment

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

Also I am still curious about the name property of the assembly, does that contain the version number or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not consider those. THanks !
binaries in other folders might affect this check? - CHanged the code to check if the faulty assembly is inside the package root folder.
ALCs - Changed the code to use TargetSite.Module.Assembly - Gets the method(and assembly) where the exception was thrown - https://learn.microsoft.com/en-us/dotnet/api/system.exception.targetsite?view=net-8.0 (not sure why I did not see this before)
curious about the name property of the assembly - I am now using the equality operator on assemblies which should consider version and culture and ALCs

}

foreach (var assemFile in new DirectoryInfo(BinaryDirectory).EnumerateFiles("*.dll"))
foreach (var assemFile in binFiles)
{
Assembly assem;
//TODO when can we make this false. 3.0?
Expand All @@ -375,13 +384,16 @@ internal IEnumerable<PackageAssembly> EnumerateAndLoadAssembliesInBinDirectory()
var result = PackageLoader.TryLoadFrom(assemFile.FullName, out assem);
if (result)
{
bool alreadyLoaded = appDomainAssemblies?.Contains(assemFile.Name) ?? false;

// IsNodeLibrary may fail, we store the warnings here and then show
IList<ILogMessage> warnings = new List<ILogMessage>();

assemblies.Add(new PackageAssembly()
{
Assembly = assem,
IsNodeLibrary = IsNodeLibrary(nodeLibraries, assem.GetName(), ref warnings)
IsNodeLibrary = IsNodeLibrary(nodeLibraries, assem.GetName(), ref warnings),
WasLoadedFromPackage = !alreadyLoaded,
});

warnings.ToList().ForEach(this.Log);
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,39 @@ 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);

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

DynamoConsoleLogger.LogErrorToDynamoConsole += DynamoConsoleLogger_LogErrorToDynamoConsole;

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

DispatcherUtil.DoEventsLoop(() => caughtExceptionFromPkg);

Assert.AreEqual(1, count);
Assert.IsTrue(caughtExceptionFromPkg);

DynamoConsoleLogger.LogErrorToDynamoConsole -= DynamoConsoleLogger_LogErrorToDynamoConsole;
}

[OneTimeTearDown]
/// <summary>
Expand Down
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