Fix WebView2 crash on close by implementing proper disposal pattern
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:
- Made WebBrowserWindow implement IDisposable with standard dispose pattern
- Added explicit disposal in Window_Closing when the window is actually closing (not just hiding)
- Added safety checks throughout the class to prevent operations on disposed WebView2 controls
- Protected async initialization from disposal race conditions
Key Changes
WebBrowserWindownow implementsIDisposablewith proper dispose patternWindow_Closingevent handler callsDispose()to clean up WebView2 before window destruction- All WebView2 operations check
_disposedflag to prevent post-disposal usage - Async
Browser_Loadedmethod 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 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.
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)
I asked for a change (already done by Copilot). PTAL @marklio.
I asked for a change (already done by Copilot). PTAL @marklio.
Good call @cincuranet.
@leculver @mconnew PTAL. The rest of the people are not good enough reviewers in current state. :D
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).
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. :)