crash report fixes #15147

pinzart90 · 2024-04-17T04:37:39Z

DYN-4849 - Handle exceptions coming from 3d party packages.

Fix sender object on Crash handlers
Only handle exceptions coming from assemblies that were loaded from a 3rd party package
Skip crash window and rethrow exceptions during tests
Removed feature flag for CER (will always be on)

github-actions · 2024-04-17T04:53:10Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

pinzart90 · 2024-04-17T04:53:27Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

@@ -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);


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.

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?

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?

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

~~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!

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.

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

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

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?

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

pinzart90 · 2024-04-18T04:05:53Z

Only the removePII fails (as in master) https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/15886/

mjkkirschner · 2024-04-22T14:09:36Z

src/DynamoCoreWpf/UI/Prompts/CrashPrompt.xaml.cs

        {
+            DynamoViewModel dynamoViewModel = sender as DynamoViewModel;


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

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; }

mjkkirschner · 2024-04-22T14:15:33Z

can you add a test for this by raising a mocked exception and setting source explicitly to the name of an assembly in one of the packages we load by default during tests?

pinzart90 · 2024-04-22T14:16:23Z

can you add a test for this by raising a mocked exception and setting source explicitly to the name of an assembly in one of the packages we load by default during tests?

Sure can. Totally forgot about tests..

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

src/DynamoCoreWpf/UI/Prompts/CrashPrompt.xaml.cs

src/DynamoCoreWpf/Utilities/CrashReportTool.cs

aparajit-pratap

Can you give some context to why was the crash happening in the first place?

pinzart90 · 2024-04-22T15:45:21Z

Can you give some context to why was the crash happening in the first place?

The exception is thrown somewhere in the juggnernaut assemblies. Not sure what.
The request was to dismiss/ignore exceptions (if possible) that are coming from 3d party package assemblies. Basically don't crash dynamo if a pacackage does not work properly

aparajit-pratap · 2024-04-22T15:46:22Z

Can you give some context to why was the crash happening in the first place?

Is the crash because an assembly that's already loaded is trying to be loaded again because it's a dependency of a package?

pinzart90 · 2024-04-22T17:18:39Z

because an assembly that's already loaded is trying to be loaded again because it's a dependency of a

No, it is just a null reference exceptions or something similar

src/NodeServices/MessageEvents.cs

pinzart90 · 2024-04-23T05:39:26Z

src/DynamoPackages/Package.cs

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


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

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)

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

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

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

src/DynamoCore/Models/DynamoModel.cs

aparajit-pratap · 2024-04-29T16:09:30Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

+            if (DynamoModel.IsTestMode)
+            {
+                // Do not handle the exception in test mode.
+                // Let the test host handle it.


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?

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

aparajit-pratap · 2024-04-29T16:28:46Z

@pinzart90 your new test is failing. Can you check? You might already be on top of it but I'm monitoring the builds this week ;)

crash report fixes

41aa117

pinzart90 commented Apr 17, 2024

View reviewed changes

pinzart90 requested review from mjkkirschner, aparajit-pratap and sm6srw April 18, 2024 01:00

QilongTang added this to the 3.2 milestone Apr 18, 2024

mjkkirschner reviewed Apr 22, 2024

View reviewed changes

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 22, 2024

View reviewed changes

src/DynamoCoreWpf/UI/Prompts/CrashPrompt.xaml.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 22, 2024

View reviewed changes

src/DynamoCoreWpf/Utilities/CrashReportTool.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 22, 2024

View reviewed changes

update

63e1b5d

pinzart90 commented Apr 23, 2024

View reviewed changes

src/NodeServices/MessageEvents.cs Show resolved Hide resolved

pinzart90 commented Apr 23, 2024

View reviewed changes

pinzart90 requested review from aparajit-pratap and mjkkirschner April 23, 2024 14:53

update

f474446

mjkkirschner reviewed Apr 23, 2024

View reviewed changes

src/DynamoCore/Models/DynamoModel.cs Show resolved Hide resolved

Update DynamoModel.cs

3c37ceb

aparajit-pratap approved these changes Apr 23, 2024

View reviewed changes

Update PackageManagerViewExtensionTests.cs

068adc2

sm6srw approved these changes Apr 24, 2024

View reviewed changes

update

1bcce09

mjkkirschner approved these changes Apr 24, 2024

View reviewed changes

pinzart90 added 13 commits April 25, 2024 15:27

update

928e049

Merge branch 'master' into crash_report_fixes

1c92594

Update PackageManagerViewExtensionTests.cs

b61e60d

update

8dfb8be

update

1e2b79c

Update PackageManagerViewExtensionTests.cs

cb74175

more logs

a15b2d3

Update DynamoViewModel.cs

570c1e9

Update DynamoViewModel.cs

0272a11

update

1fd5bbf

Update DynamoViewModel.cs

243787d

update

0659d76

Update DynamoViewModel.cs

890984f

aparajit-pratap reviewed Apr 29, 2024

View reviewed changes

Create TestCrash.dll

cf9aff5

pinzart90 merged commit 59e22d8 into master Apr 29, 2024
23 checks passed

pinzart90 deleted the crash_report_fixes branch April 29, 2024 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crash report fixes #15147

crash report fixes #15147

pinzart90 commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading

pinzart90 Apr 17, 2024

pinzart90 Apr 18, 2024

mjkkirschner Apr 22, 2024

mjkkirschner Apr 22, 2024

mjkkirschner Apr 22, 2024 •

edited

Loading

pinzart90 Apr 22, 2024

pinzart90 Apr 22, 2024

pinzart90 Apr 22, 2024

aparajit-pratap Apr 22, 2024 •

edited

Loading

pinzart90 Apr 23, 2024 •

edited

Loading

pinzart90 commented Apr 18, 2024

mjkkirschner Apr 22, 2024 •

edited

Loading

pinzart90 Apr 22, 2024

mjkkirschner commented Apr 22, 2024

pinzart90 commented Apr 22, 2024

aparajit-pratap left a comment

pinzart90 commented Apr 22, 2024

aparajit-pratap commented Apr 22, 2024

pinzart90 commented Apr 22, 2024

pinzart90 Apr 23, 2024

pinzart90 Apr 23, 2024

mjkkirschner Apr 24, 2024

mjkkirschner Apr 24, 2024

pinzart90 Apr 24, 2024

aparajit-pratap Apr 29, 2024

pinzart90 Apr 29, 2024

aparajit-pratap commented Apr 29, 2024

		{
		DynamoViewModel dynamoViewModel = sender as DynamoViewModel;

crash report fixes #15147

crash report fixes #15147

Conversation

pinzart90 commented Apr 17, 2024 • edited Loading

github-actions bot commented Apr 17, 2024 • edited Loading

UI Smoke Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

pinzart90 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

pinzart90 commented Apr 18, 2024

mjkkirschner Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Apr 22, 2024

pinzart90 commented Apr 22, 2024

aparajit-pratap left a comment

Choose a reason for hiding this comment

pinzart90 commented Apr 22, 2024

aparajit-pratap commented Apr 22, 2024

pinzart90 commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap commented Apr 29, 2024

pinzart90 commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading

mjkkirschner Apr 22, 2024 •

edited

Loading

aparajit-pratap Apr 22, 2024 •

edited

Loading

pinzart90 Apr 23, 2024 •

edited

Loading

mjkkirschner Apr 22, 2024 •

edited

Loading