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

ChromiumWebBrowser won't be disposed unless calling Dispose explicitly. #13

Closed
kkwpsv opened this issue Aug 30, 2021 · 18 comments
Closed

Comments

@kkwpsv
Copy link
Contributor

kkwpsv commented Aug 30, 2021

HwndHost will call Dispose in ~HwndHost(). So it could be disposed implicitly.
But in ChromiumWebBrowser, the instance has be added to a static HashSet by AddDisposable, so it'll never be finalized.

Maybe we should add CleanupElement for it?

@campersau
Copy link
Contributor

campersau commented Aug 30, 2021

And maybe a finalizer?

~ChromiumWebBrowser()
{
    Dispose(false);
}

@kkwpsv
Copy link
Contributor Author

kkwpsv commented Aug 30, 2021

And maybe a finalizer?

~ChromiumWebBrowser()
{
    Dispose(false);
}

There's already a finalizer (in base class HwndHost). But the class is referenced by the static HashSet. It won't be gc , and won't be finalized.

@amaitland
Copy link
Member

You can add a class that implements Idisposable, holds a weak reference to the ChromiumWebBrowser instance, pass that to Cef.AddDisposable.

@kkwpsv
Copy link
Contributor Author

kkwpsv commented Sep 1, 2021

You can add a class that implements Idisposable, holds a weak reference to the ChromiumWebBrowser instance, pass that to Cef.AddDisposable.

I didn't understand why pass that to Cef.AddDisposable again? What you want to say is Cef.RemoveDisposable?

@amaitland
Copy link
Member

I'm suggesting instead of passing a direct reference to Cef.AddDisposable passing what is effectively a weak reference. The remove call would also Need to be updated.

https://github.com/cefsharp/CefSharp.Wpf.HwndHost/blob/master/CefSharp.Wpf.HwndHost/ChromiumWebBrowser.cs#L536

@kkwpsv
Copy link
Contributor Author

kkwpsv commented Sep 2, 2021

I'm suggesting instead of passing a direct reference to Cef.AddDisposable passing what is effectively a weak reference. The remove call would also Need to be updated.

https://github.com/cefsharp/CefSharp.Wpf.HwndHost/blob/master/CefSharp.Wpf.HwndHost/ChromiumWebBrowser.cs#L536

Sorry, I misunderstood your meaning earlier. This may also be a solution.
But it also make the behavior is different from CefSharp.Wpf. For example, in my project, there's something static associated with the browser. And, I'll clean them when IsBrowserInitializedChanged is false. It'll work fine in CefSharp.Wpf. But with this solution, this may not work.
Why not use same solution to clean for them? Are there other considerations?

@amaitland
Copy link
Member

HwndHost will call Dispose in ~HwndHost(). So it could be disposed implicitly.
But in ChromiumWebBrowser, the instance has be added to a static HashSet by AddDisposable, so it'll never be finalized.

This is the problem you've stated, and that's the problem I'm suggesting a solution for.

@amaitland amaitland changed the title CefSharp.Wpf.HwndHost.ChromiumWebBrowser won't be disposed unless calling Dispose explicitly. ChromiumWebBrowser won't be disposed unless calling Dispose explicitly. Sep 2, 2021
@kkwpsv
Copy link
Contributor Author

kkwpsv commented Sep 3, 2021

I'd like to introduce CleanupElement like CefSharp.Wpf. Is it acceptable to merge? If so, I'll make a pull request later.

@amaitland
Copy link
Member

I'd like to introduce CleanupElement like CefSharp.Wpf. Is it acceptable to merge? If so, I'll make a pull request later.

Is it actually required? Have you tried the default HwndHost behaviour?

@amaitland
Copy link
Member

For reference the shutdown code now matches the WPF implementation and will close all ChromiumWebBrowser instances when the Dispatcher is shutdown. See c5c6d48

@amaitland
Copy link
Member

Is it actually required? Have you tried the default HwndHost behaviour?

To be clear, comment out the Cef.AddDisposable call and see how the HwndHost finalizer behaves, the control will still be Disposed when the Dispatcher.ShutdownFinished

https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/Interop/HwndHost.cs,1423

@kkwpsv
Copy link
Contributor Author

kkwpsv commented Sep 3, 2021

Is it actually required? Have you tried the default HwndHost behaviour?

To be clear, comment out the Cef.AddDisposable call and see how the HwndHost finalizer behaves, the control will still be Disposed when the Dispatcher.ShutdownFinished

https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/Interop/HwndHost.cs,1423

Hwndhost is not only be disposed when Dispatcher.ShutdownFinished. It's also be disposed when gc in it's finalizer. You can see this behavior from the following sample.

I create a new Window, and put a HwndHost in it. Close the window, wait and call GC. You can see that the finalizer of Hwndhost is called while main thread is waiting for finalizer. And the Dispatcher is still alive. It hasn't shutdown.

image
image
image

@amaitland
Copy link
Member

Hwndhost is not only be disposed when Dispatcher.ShutdownFinished. It's also be disposed when gc in it's finalizer

I'm aware of this.

I was simply making you aware of the change. I'm not suggesting this is a fix.

I create a new Window, and put a HwndHost in it. Close the window, wait and call GC. You can see that the finalizer of Hwndhost is called while main thread is waiting for finalizer. And the Dispatcher is still alive. It hasn't shutdown.

Please no images of code.

@kkwpsv
Copy link
Contributor Author

kkwpsv commented Sep 4, 2021

I misunderstood that you missed the finalizer could be called by gc. So I just use images to show the callstack and calling sequence.

I think CleanupElement shuold be useful. In projects, it's easy to write some code in handler or other that reference to ChromiumWebBrowser, which cause it not be disposed by gc, and cause serious resource leak. With CleanupElement, most of unmanaged resource could be disposed. And disposing when Dispatcher.ShutdownFinished may has less impact, because the event usually be raised when the program is about to exit. Even without disposed, resource will be released by os.

Just removing the reference by Cef.AddDisposable should be useful for this issues. But in larger project, it's not as robust as CleanupElement. I prefer to use CleanupElement.

@amaitland
Copy link
Member

Submit a PR for cleanup element. Making sure all methods are documented and changing of parent windows is handled correctly.

@kkwpsv
Copy link
Contributor Author

kkwpsv commented Sep 5, 2021

OK, I'll submit it later.

@amaitland
Copy link
Member

Version 93.1.140 includes ChromiumWebBrowser.CleanupElement though you have to assign it manually at the moment.

@amaitland
Copy link
Member

CleanupElement will now be set when the browser is added/moved to a Window. Commit 54a6b50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants