-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[windows] fix memory leak in WebView
#18810
Conversation
Context: dotnet#18365 Context: https://github.com/heyThorsten/GCTest Testing the sample app, I could see a leak in `WebView` on Windows. A memory snapshot showed: Microsoft.Maui.Handlers.WebViewHandler Windows.Foundation.TypedEventHandler<Microsoft.UI.Xaml.Controls.WebView2, Microsoft.UI.Xaml.Controls.CoreWebView2InitializedEventArgs> WinRT._EventSource_global__Windows_Foundation_TypedEventHandler_global__Microsoft_UI_Xaml_Controls_WebView2__global__Microsoft_UI_Xaml_Controls_CoreWebView2InitializedEventArgs_+EventState Windows.Foundation.TypedEventHandler<Microsoft.UI.Xaml.Controls.WebView2, Microsoft.UI.Xaml.Controls.CoreWebView2InitializedEventArgs> [Dependent Handle, Microsoft.Maui.Platform.LayoutPanel <0x2079E7A8D00>] Microsoft.UI.Xaml.RoutedEventHandler WinRT._EventSource_global__Microsoft_UI_Xaml_RoutedEventHandler+EventState Microsoft.UI.Xaml.RoutedEventHandler [Dependent Handle, Microsoft.Maui.Platform.LayoutPanel <0x2079E7A8D00>] I could this same leak in `WebView` by updating `MemoryTests` to do: if (view is WebView webView) { webView.Source = new HtmlWebViewSource { Html = "<p>hi</p>" }; await Task.Delay(1000); } After a bit of debugging, I found that events on `WebView` seem to suffer from the same "circular reference" problem we see on iOS. This makes sense, as it is an unmanaged/native control. (Might be COM?) So we have the cycle: * `WebViewHandler` -> * `WebView2` -> * Various `WebView2` events: * `CoreWebView2Initialized` * `HistoryChanged` * `NavigationStarting` * `NavigationCompleted` * `WebViewHandler` Using the same pattern we've been using for iOS: creating a `WebView2Proxy` type solves the issue. Note that I didn't have to do this for all events, just the ones subscribing to `WebView2`-related events. I also don't see the `WebView2Proxy` object living forever, so the issue does appear to be a cycle.
@@ -25,7 +26,7 @@ internal WebNavigationEvent CurrentNavigationEvent | |||
|
|||
protected override void ConnectHandler(WebView2 platformView) | |||
{ | |||
platformView.CoreWebView2Initialized += OnCoreWebView2Initialized; | |||
_proxy.Connect(this, platformView); |
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.
Hum shouldn't we just connect the proxy when WebView2 is initialized? I think we need this I remember other bug related with this where there was an exception related with this. @Eilon do you remember ?
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.
The connect method should be the same as what it was doing before:
public void Connect(WebViewHandler handler, WebView2 platformView)
{
_handler = new(handler);
platformView.CoreWebView2Initialized += OnCoreWebView2Initialized;
}
I don't think I changed any events -- just moved methods to a new class.
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.
@rmarinho hmm sorry I don't specifically remember a bug around this, but maybe there was.
Context: #18365
Context: https://github.com/heyThorsten/GCTest
Testing the sample app, I could see a leak in
WebView
on Windows. A memory snapshot showed:I could this same leak in
WebView
by updatingMemoryTests
to do:After a bit of debugging, I found that events on
WebView
seem to suffer from the same "circular reference" problem we see on iOS. This makes sense, as it is an unmanaged/native control. (Might be COM?)So we have the cycle:
WebViewHandler
->WebView2
->WebView2
events:CoreWebView2Initialized
HistoryChanged
NavigationStarting
NavigationCompleted
WebViewHandler
Using the same pattern we've been using for iOS: creating a
WebView2Proxy
type solves the issue. Note that I didn't have to do this for all events, just the ones subscribing toWebView2
-related events. I also don't see theWebView2Proxy
object living forever, so the issue does appear to be a cycle.