Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 10, 2025

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:

Microsoft_Web_WebView2_Wpf!Microsoft.Web.WebView2.Wpf.WebView2.Dispose+0x34
PresentationFramework_ni!System.Windows.Interop.HwndHost.Finalize+0x14
clr!FinalizerThread::DoOneFinalization+0x8f

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:

  1. Made WebBrowserWindow implement IDisposable with standard dispose pattern
  2. Added explicit disposal in Window_Closing when the window is actually closing (not just hiding)
  3. Added safety checks throughout the class to prevent operations on disposed WebView2 controls
  4. Protected async initialization from disposal race conditions

Key Changes

  • WebBrowserWindow now implements IDisposable with proper dispose pattern
  • Window_Closing event handler calls Dispose() to clean up WebView2 before window destruction
  • All WebView2 operations check _disposed flag to prevent post-disposal usage
  • Async Browser_Loaded method includes disposal checks to avoid unnecessary work

Testing

Created and validated a mock test that confirms:

  • WebView2 disposal is called correctly
  • Post-disposal operations are safely handled
  • Double disposal is safe
  • Async operations respect disposal state

The implementation follows PerfView coding standards and uses minimal, surgical changes to fix the crash without affecting existing functionality.

Fixes #2229.

Copilot AI and others added 2 commits June 10, 2025 21:42
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Crash When Using HTML Views Fix WebView2 crash on close by implementing proper disposal pattern Jun 10, 2025
@Copilot Copilot AI requested a review from brianrob June 10, 2025 21:45
Copilot finished work on behalf of brianrob June 10, 2025 21:45
@brianrob
Copy link
Member

Copilot Post-Mortem:

  • Great example of an issue that requires minimal coaching.
  • The stack trace was sufficient for copilot to know what to do. All I had to do was test locally.

@brianrob brianrob marked this pull request as ready for review June 10, 2025 21:52
marklio
marklio previously approved these changes Jun 10, 2025
Copy link
Collaborator

@marklio marklio left a 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
Copy link
Collaborator

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

Copy link
Contributor Author

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>
Copy link
Contributor Author

Copilot AI commented Jun 10, 2025

I approve, but consider the feedback on going to the trouble of implementing the full verbose IDisposable pattern.

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)

Copilot finished work on behalf of marklio June 10, 2025 22:15
marklio
marklio previously approved these changes Jun 10, 2025
private void BackClick(object sender, RoutedEventArgs e)
{
if (Browser.CanGoBack)
if (!_disposed && Browser.CanGoBack)
Copy link
Collaborator

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.

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'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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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>
Copilot finished work on behalf of cincuranet June 11, 2025 12:43
@cincuranet
Copy link
Collaborator

I asked for a change (already done by Copilot). PTAL @marklio.

@brianrob
Copy link
Member

I asked for a change (already done by Copilot). PTAL @marklio.

Good call @cincuranet.

@cincuranet
Copy link
Collaborator

@leculver @mconnew PTAL. The rest of the people are not good enough reviewers in current state. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash When Using HTML Views
4 participants