perfview icon indicating copy to clipboard operation
perfview copied to clipboard

Fix WebView2 crash on close by implementing proper disposal pattern

Open Copilot opened this issue 6 months ago • 5 comments

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 avatar Jun 10 '25 21:06 Copilot

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 avatar Jun 10 '25 21:06 brianrob

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 avatar Jun 10 '25 22:06 Copilot

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

cincuranet avatar Jun 11 '25 12:06 cincuranet

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

Good call @cincuranet.

brianrob avatar Jun 11 '25 23:06 brianrob

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

cincuranet avatar Jun 16 '25 07:06 cincuranet

Additional Copilot Post-Mortem:

It looks like once a maintainer makes a comment that causes copilot to respond with a code change, that maintainer's approval no longer counts. That looks like it's going to be a bit too strong of a limitation, given that we've now had two additional maintainers, in addition to the originator of the PR approve, and that's still not enough. It feels like there is a sweet spot here where the originator of the PR needs to unleash copilot to do the work so that the additional maintainers' approval still counts (or something along those lines).

brianrob avatar Jun 19 '25 22:06 brianrob

It seems like the current behavior for approvals is this:

  • The originator of the PR (the person that assigns the issue to copilot) can never approve.
  • If another maintainer makes a comment and copilot makes a change as a result, that maintainer cannot approve.
  • However, if yet another maintainer makes a comment that copilot reacts to, then the first maintainer (not the originator) can approve and it will count. However, old approvals won't count because they are invalidated when copilot responds to a comment that they made.

We'll see if this guidance sticks, but for now, that seems to be the case. I'm merging this before copilot makes another change. :)

brianrob avatar Jun 19 '25 23:06 brianrob