cefsharp / CefSharp.Wpf.HwndHost

Designed as a drop in replacement for CefSharp.Wpf for those who want the native Win32 based implementation (For IME support and better performance).
Other
34 stars 10 forks source link

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

Closed kkwpsv closed 2 years ago

kkwpsv commented 3 years ago

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 commented 3 years ago

And maybe a finalizer?

~ChromiumWebBrowser()
{
    Dispose(false);
}
kkwpsv commented 3 years ago

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 commented 3 years ago

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

kkwpsv commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

kkwpsv commented 3 years ago

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

amaitland commented 3 years ago

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 commented 3 years ago

For reference the shutdown code now matches the WPF implementation and will close all ChromiumWebBrowser instances when the Dispatcher is shutdown. See https://github.com/cefsharp/CefSharp.Wpf.HwndHost/commit/c5c6d486a1748162cf63f5920f1aedad34ea5033

amaitland commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

kkwpsv commented 3 years ago

OK, I'll submit it later.

amaitland commented 2 years ago

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

amaitland commented 2 years ago

CleanupElement will now be set when the browser is added/moved to a Window. Commit https://github.com/cefsharp/CefSharp.Wpf.HwndHost/commit/54a6b507529e34ce99af01dfb0e05e4fa2e098d8