crash handlers for Dynamo #14826
crash handlers for Dynamo #14826
Conversation
pinzart90
commented
Jan 10, 2024
•
pinzart90
commented
- Introduced the following exception handlers (all in DynamoViewModel.cs):
- Dispatcher.UnhandledException - catches unhandled exceptions from the UI thread. Can mark exceptions as handled (and close Dynamo) so that host apps can continue running normally even though Dynamo crashed
- AppDomain.UnhandledException - catches unhandled exceptions that are fatal to the current process. These exceptions cannot be handled and process termination is guaranteed.
- TaskScheduler.UnobservedTaskException - catches unobserved Task exceptions from all threads. Does not crash DYnamo, we only log the exceptions and do not call CER or close Dynamo.
- DynamoModel.IsCrashing is reset (set to false) on every DynamoModel constructor call.
- SplashScreen::WebView_NavigationCompleted is wrapped in a try catch.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
UI Smoke TestsTest: success. 2 passed, 0 failed. |
|
@pinzart90 it looks like none of the code from this line onwards will execute after these changes, i.e., if the unhandled exceptions are handled by your new handlers. Neither will this crash prompt dialog (sad-face) be displayed: While this might be a good change since now we're capturing these exceptions in CER, I wonder if any of the steps that we were doing earlier (other than showing the sad-face dialog), such as saving the recorded commands, etc. still be needed? |
|
@pinzart90 in the case of Sandbox, I do see that unhandled exceptions are eventually caught here and then handled to show the CER dialog before Dynamo is shutdown gracefully. So I'm not sure if having these new unhandled exception handlers that more or less do the same thing is even necessary. Can you clarify? |
|
@pinzart90 You have a conflict. |
| if (!DynamoModel.IsCrashing && !IsClosing) | ||
| { | ||
| CrashReportTool.ShowCrashWindow(viewModel, new CrashErrorReportArgs(ex)); | ||
| Close();// Close the SpashScreen |
There was a problem hiding this comment.
added Try Catch in the WebView_NavigationCompleted body.
Looks like all exceptions thrown in WebView_NavigationCompleted are silenced by webview2.
SplashScreen.WebView_NavigationCompleted is particularly sensitive because Dynamo views are initialzied inside this method and if an exceptions is thrown, SplashScreen will seem like it hangs.
| @@ -162,6 +162,21 @@ private static string FindCERToolInInstallLocations() | |||
| } | |||
| } | |||
|
|
|||
| internal static void ShowCrashWindow(object sender, CrashPromptArgs args) | |||
| { | |||
| var viewModel = sender as DynamoViewModel; | |||
There was a problem hiding this comment.
can you add a null check here?
There was a problem hiding this comment.
Actually can be null. ShowCrashWindow could be called like ShowCrashWindow(null, new CrashPromptArgs());
|
| @@ -690,14 +691,20 @@ public static DynamoViewModel Start(StartConfiguration startConfiguration = new | |||
|
|
|||
| protected DynamoViewModel(StartConfiguration startConfiguration) | |||
| { | |||
| // CurrentDomain_UnhandledException - catches unhandled exceptions that are fatal to the current process. These exceptions cannot be handled and process termination is guaranteed | |||
| AppDomain.CurrentDomain.UnhandledException += CurrentDomain_UnhandledException; | |||
There was a problem hiding this comment.
I thought this was initially added to DynamoModel. Can you explain why it has been added here instead of DynamoModel and the same for TaskScheduler.UnobservedException? Since DynamoModel is instantiated before the view model, there could be code that's uncaught by any of these handlers.
There was a problem hiding this comment.
That would require more refactor. CrashTool is in the DynamoCoreWPF project. It (along with other CrashPromptArgs) uses DynamoViewModel ..which I cannot access from DynamoCore and also may not exist yet depending on when the crash happens. Also if DynamoViewModel is created...then we would want all exception handlers to start using the DynamoViewModel. and stop the DynamoModel handler ?
I would leave that for another PR...
There was a problem hiding this comment.
Ok. Maybe we should decouple the CER crash prompt from DynamoViewModel. Currently, it needs the view model to show the dialog. Would be good to remove the dependency on the view model for this dialog if possible.
There was a problem hiding this comment.
I am trying that now, but there are too many changes for this PR
I will create a followup
