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.

Sign up for GitHub

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

crash report fixes #15147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
41aa117
crash report fixes
pinzart90 Apr 17, 2024
63e1b5d
update
pinzart90 Apr 23, 2024
f474446
update
pinzart90 Apr 23, 2024
3c37ceb
Update DynamoModel.cs
pinzart90 Apr 23, 2024
068adc2
Update PackageManagerViewExtensionTests.cs
pinzart90 Apr 23, 2024
1bcce09
update
pinzart90 Apr 24, 2024
928e049
update
pinzart90 Apr 25, 2024
1c92594
Merge branch 'master' into crash_report_fixes
pinzart90 Apr 25, 2024
b61e60d
Update PackageManagerViewExtensionTests.cs
pinzart90 Apr 26, 2024
8dfb8be
update
pinzart90 Apr 26, 2024
1e2b79c
update
pinzart90 Apr 26, 2024
cb74175
Update PackageManagerViewExtensionTests.cs
pinzart90 Apr 26, 2024
a15b2d3
more logs
pinzart90 Apr 26, 2024
570c1e9
Update DynamoViewModel.cs
pinzart90 Apr 26, 2024
0272a11
Update DynamoViewModel.cs
pinzart90 Apr 26, 2024
1fd5bbf
update
pinzart90 Apr 26, 2024
243787d
Update DynamoViewModel.cs
pinzart90 Apr 26, 2024
0659d76
update
pinzart90 Apr 29, 2024
890984f
Update DynamoViewModel.cs
pinzart90 Apr 29, 2024
cf9aff5
Create TestCrash.dll
pinzart90 Apr 29, 2024
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
9 changes: 8 additions & 1 deletion src/DynamoCoreWpf/UI/Prompts/CrashPrompt.xaml.cs
View file
Open in desktop
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,8 +48,14 @@ 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)
{
DynamoViewModel dynamoViewModel = sender as DynamoViewModel;
Copy link
Member

@mjkkirschner mjkkirschner Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit pick - use var
also pattern match is would be nice with an if statement for the second line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean like so ?

            DynamoModel model = null;
            var dynamoViewModel = sender as DynamoViewModel;
            if (dynamoViewModel != null)
            {
                model = dynamoViewModel.Model;
            }
            else if (sender is DynamoModel dm)
            {
                model = dm;
            }

DynamoModel model = dynamoViewModel != null ? dynamoViewModel.Model : sender as DynamoModel;

InitializeComponent();

var packageLoader = dynamoViewModel?.Model?.GetPackageManagerExtension()?.PackageLoader;
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
11 changes: 5 additions & 6 deletions src/DynamoCoreWpf/Utilities/CrashReportTool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,14 @@ 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();
}

Expand All @@ -183,14 +181,15 @@ internal static void ShowCrashWindow(object sender, CrashPromptArgs args)
/// <param name="viewModel">The Dynamo view model</param>
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
/// <param name="args"></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)
{
return false;
}

DynamoModel model = viewModel?.Model;
DynamoViewModel viewModel = sender as DynamoViewModel;
DynamoModel model = viewModel != null ? viewModel.Model : sender as DynamoModel;

string cerToolDir = !string.IsNullOrEmpty(model?.CERLocation) ?
model?.CERLocation : FindCERToolInInstallLocations();
Expand Down
12 changes: 12 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,18 @@ private void CrashGracefully(Exception ex, bool fatal = false)
{
try
{
if (!fatal)
{
var localPkg = Model.GetPackageManagerExtension()?.PackageLoader?.LocalPackages?.FirstOrDefault(p => p.LoadedAssemblies.FirstOrDefault(a => a.Name == ex.Source) != null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

might happen that one of these package assemblies is already loaded by dynamo (as a dependency), so we could potentially continue running even though the exception is coming from somewhere else. (not the pacakge we identified)
But I think that might be an edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment this code will always continue the session and only log the exception.
As an alternative, we could show a warning message that exception ex was thrown from package X and give the user a choice to continue the dynamo seasion or crash and send CER report
@Amoursol any preferences?

Copy link
Member

Choose a reason for hiding this comment

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

If the source property is not set explicitly, the runtime automatically sets it to the name of the assembly in which the exception originated.

hmm, so in most cases this will work.

Have you verified that this property is set to the short name or full name of the assembly? Does it include the version number and culture?

Copy link
Member

Choose a reason for hiding this comment

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

what about the edge case where a user has shipped system binaries (Microsoft) or Dynamo binaries?

Copy link
Member

@mjkkirschner mjkkirschner Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

what about also logging something to ADP so we have analytics on how often these crashes occur without actually crashing?
whoops missed your call to track!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you verified that this property is set to the short name or full name of the assembly
It is just the short name. Obviously this can be overridden by custom exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about the edge case where a user has shipped system binaries (Microsoft) or Dynamo binaries?
Not sure at the moment how I can identify these sort of cases...Are the package LoadedAssemblies actually loaded or they can just be on disk without actually being the ones to be loaded...I can check this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, LoadedAssemblies could be already loaded assemblies. So if a user supplies a dotnet system dll, then all the exceptions from that dll (even if it was loaded by dotnet previously) will not cause a hard crash anymore.

I can verify if the assemblies have been loaded already, probably at a small performance penalty

Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Minor: does it still make sense to have this check (code path) in the same function (CrashGracefully), as it isn't crashing in this case?

Copy link
Contributor Author

@pinzart90 pinzart90 Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

CrashGracefully is called from multiple places.
Another way I could go is to not subscribe to the unhandled_exception_handlers at all during testing. But I think the logging is still valuable for debugging. So I suggest we leave the rethrow in the CrashGracefully method for now

if (localPkg != null)
{
var crashDetails = new CrashErrorReportArgs(ex);
Model?.Logger?.LogError($"Unhandled exception coming from package {localPkg.Name} was handled: {crashDetails.Details}");
Analytics.TrackException(ex, true);
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
return;
}
}

DynamoModel.IsCrashing = true;
var crashData = new CrashErrorReportArgs(ex);
Model?.Logger?.LogError($"Unhandled exception: {crashData.Details} ");
Expand Down
Loading