Skip to content

[windows] fix memory leak in WebView#23540

Merged
rmarinho merged 1 commit into
dotnet:mainfrom
jonathanpeppers:WindowsWebViewLeaks
Jul 11, 2024
Merged

[windows] fix memory leak in WebView#23540
rmarinho merged 1 commit into
dotnet:mainfrom
jonathanpeppers:WindowsWebViewLeaks

Conversation

@jonathanpeppers

@jonathanpeppers jonathanpeppers commented Jul 10, 2024

Copy link
Copy Markdown
Member

Fixes: #20283
Fixes: #22972
Context: https://github.com/davide-cavallini/webview-winUI-maui-leak

The above sample on Windows was leaking because it does the following:

  1. Your app has a single Window

  2. Navigate to a page with a WebView

  3. Navigate away

Because the Window remains alive, WebViewHandler subscribes to Window.Closed which keeps a reference to the WebView and keeps it alive indefinitely.

I was able to reproduce this in a test, that keeps the Window alive before calling AssertionExtensions.WaitForGC().

The solution was to move the Window.Closed subscription to the WebView2Proxy nested class. This makes sure that the WebView, its handler, etc. can be collected before the Window is closed.

I also found a secondary issue while debugging, if you call:

webView.Close(); // MauiWebView or WebView2

If CoreWebView2 is not initialized, it will throw a C++ exception.

We can instead do:

if (webView.CoreWebView2 is not null)
{
    webView.Close();
}

Fixes: dotnet#20283
Fixes: dotnet#22972
Context: https://github.com/davide-cavallini/webview-winUI-maui-leak

The above sample on Windows was leaking because it does the following:

1. Your app has a single `Window`

2. Navigate to a page with a `WebView`

3. Navigate away

Because the `Window` remains alive, `WebViewHandler` subscribes
to `Window.Closed` which keeps a reference to the `WebView` and
keeps it alive indefinitely.

I was able to reproduce this in a test, that keeps the `Window`
alive before calling `AssertionExtensions.WaitForGC()`.

The solution was to move the `Window.Closed` subscription to the
`WebView2Proxy` nested class. This makes sure that the `WebView`,
its handler, etc. can be collected *before* the `Window` is closed.

I also found a secondary issue while debugging, if you call:

    webView.Close(); // MauiWebView or WebView2

If `CoreWebView2` is not initialized, it will throw a C++ exception.

We can instead do:

    if (webView.CoreWebView2 is not null)
    {
        webView.Close();
    }
@jonathanpeppers jonathanpeppers added perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/windows labels Jul 10, 2024
@jonathanpeppers jonathanpeppers marked this pull request as ready for review July 10, 2024 18:28
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner July 10, 2024 18:28
@jonathanpeppers

Copy link
Copy Markdown
Member Author

There is a Windows test failure, but not sure if it's related to my changes. I reran it again.

@rmarinho

Copy link
Copy Markdown
Member

Failing tests not related.

@rmarinho rmarinho merged commit f19f9cf into dotnet:main Jul 11, 2024
@jonathanpeppers jonathanpeppers deleted the WindowsWebViewLeaks branch July 11, 2024 13:22
@bronteq

bronteq commented Jul 24, 2024

Copy link
Copy Markdown

Hi @jonathanpeppers, I tried the repro project in the following way

  • Release and Debug mode
  • Set Microsoft.Maui.Controls 8.0.80-ci.net8.24374.2
  • Set Microsoft.Maui.Controls.Compatibility 8.0.80-ci.net8.24374.2

For me the bug does not seem solved, in fact I don't see in Task Manager the memory free up, the situation seems the same as reported in the bug.

image

If the following row is added in OnDisappearing(), the memory is freed also in Task Manager
webView.Handler?.DisconnectHandler();

Am I doing something wrong? 8.0.80-ci.net8.24374.2 seems the latest nightly version available for the moment

@jonathanpeppers

Copy link
Copy Markdown
Member Author

@bronteq the fix here prevents every Window from keeping the WebView and MauiWebView (platform view) from staying alive until the Window is closed.

If there are msedgewebview2.exe staying alive, this is probably a Windows App SDK (or WebView2) issue. I know some of this is intentional, as they keep the WebView2 process alive so it can be reused:

If calling DisconnectHandler() fixes something for you, I would keep using it. In .NET 9, they are going to make this be called more frequently, so it will likely fix this issue long-term:

@bronteq

bronteq commented Jul 24, 2024

Copy link
Copy Markdown

Sorry @jonathanpeppers I misunderstood your PR.

My initial issue was #21194
This issue was considered a duplicate of #20283 that should be solved with this PR.

If the issues are different, probably my issue should be reopened, because it is not solved yet and the amount of ram that is not freed up can be very high (think of an industrial pc with limited specs, the program becomes unusable).

Calling DisconnectHandler() solves my issue, good to hear of #23738

@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! fixed-in-8.0.80 and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Aug 2, 2024
@samhouts samhouts added fixed-in-9.0.0-preview.7.24407.4 and removed fixed-in-net9.0-nightly This may be available in a nightly release! labels Aug 27, 2024
@RuddyOne

Copy link
Copy Markdown

I am still seeing the issue on 8.0.80. Am I missing something?

@github-actions github-actions Bot locked and limited conversation to collaborators Oct 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Win platform WebView cannot be release after its parent window get close WebView MAUI WinUI Memory Leak

6 participants