Skip to content
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 #23540

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jul 10, 2024

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 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
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
Member

Failing tests not related.

@rmarinho rmarinho merged commit f19f9cf into dotnet:main Jul 11, 2024
95 of 97 checks passed
@jonathanpeppers jonathanpeppers deleted the WindowsWebViewLeaks branch July 11, 2024 13:22
@bronteq
Copy link

bronteq commented Jul 24, 2024

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

bronteq commented Jul 24, 2024

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

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