-
Notifications
You must be signed in to change notification settings - Fork 738
Fix WebView2 crash on close by implementing proper disposal pattern #2230
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
Copilot Post-Mortem:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve, but consider the feedback on going to the trouble of implementing the full verbose IDisposable pattern.
@@ -13,7 +13,7 @@ namespace PerfView.GuiUtilities | |||
/// <summary> | |||
/// Interaction logic for WebBrowserWindow.xaml | |||
/// </summary> | |||
public partial class WebBrowserWindow : WindowBase | |||
public partial class WebBrowserWindow : WindowBase, IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of a NIT: Since you're not asking consumers to manage the lifetime of this window, I wouldn't implement IDisposable. The pattern makes sense as an implementation, but I wouldn't expose it (the comment in the Closing() event handler is evidence of this weirdness. The window manages its disposable resources within the "control lifecycle". I'd get rid of the extra Dispose pattern goo and just have the _disposed bool to control operating on _Browser and call _Browser.Dispose conditionally in the Window_Closing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I've simplified the implementation by removing the IDisposable interface and formal dispose pattern. The solution now just uses an internal _disposed bool and calls _Browser.Dispose() conditionally in Window_Closing when the window is actually being closed. This is much cleaner since the window manages its own lifecycle. (8e3e67e)
Co-authored-by: marklio <529657+marklio@users.noreply.github.com>
Thanks for the feedback! I've addressed the suggestion by removing the verbose IDisposable pattern and implementing a simpler internal disposal approach that still fixes the crash. (8e3e67e) |
private void BackClick(object sender, RoutedEventArgs e) | ||
{ | ||
if (Browser.CanGoBack) | ||
if (!_disposed && Browser.CanGoBack) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically what CanGoBack
property above is doing and hence it can be used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! Changed to use the CanGoBack property which already encapsulates the disposal check logic. (d692454)
{ | ||
Browser.GoBack(); | ||
} | ||
} | ||
|
||
private void ForwardClick(object sender, RoutedEventArgs e) | ||
{ | ||
if (Browser.CanGoForward) | ||
if (!_disposed && Browser.CanGoForward) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically what CanGoForward
property above is doing and hence it can be used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct! Updated to use the CanGoForward property to avoid duplicating the disposal check logic. (d692454)
…osal checks Co-authored-by: cincuranet <4540597+cincuranet@users.noreply.github.com>
I asked for a change (already done by Copilot). PTAL @marklio. |
Good call @cincuranet. |
This PR fixes a crash that occurs when closing HTML views in PerfView. The crash was happening during finalization of the WebView2 control, which involves COM interop cleanup that can fail when done through the finalizer.
Problem
The crash stack trace shows:
This indicates the WebView2 control was being cleaned up by the finalizer instead of being explicitly disposed, leading to COM interop crashes.
Solution
Implemented proper IDisposable pattern for
WebBrowserWindow
:Key Changes
WebBrowserWindow
now implementsIDisposable
with proper dispose patternWindow_Closing
event handler callsDispose()
to clean up WebView2 before window destruction_disposed
flag to prevent post-disposal usageBrowser_Loaded
method includes disposal checks to avoid unnecessary workTesting
Created and validated a mock test that confirms:
The implementation follows PerfView coding standards and uses minimal, surgical changes to fix the crash without affecting existing functionality.
Fixes #2229.