From 884b582789dbcce245f182b112853d40b0ea2ea8 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Sat, 12 Oct 2024 14:52:17 -0400 Subject: [PATCH] views: Trigger CefBrowser destruction on CefBrowserView release (see #3790) Avoid a circular ownership dependency between CefBrowserViewImpl and CefBrowserPlatformDelegate[Chrome]Views (owned by CefBrowserHostBase). Trigger CefBrowserHostBase destruction when the last CefBrowserViewImpl reference is released. This fixes a number of shutdown crashes related to overlay CefBrowsers still existing at CefShutdown. --- libcef/browser/browser_platform_delegate.cc | 6 +++++ .../browser_platform_delegate_chrome_views.cc | 23 +++++++++++++------ .../browser_platform_delegate_chrome_views.h | 10 ++++++-- .../views/browser_platform_delegate_views.cc | 22 ++++++++++++------ .../views/browser_platform_delegate_views.h | 9 +++++++- libcef/browser/views/browser_view_impl.cc | 21 +++++++++++++++++ libcef/browser/views/browser_view_impl.h | 6 +++++ libcef/browser/views/overlay_view_host.cc | 5 ++++ libcef/browser/views/overlay_view_host.h | 1 + .../browser/views_overlay_browser.cc | 8 ++++++- 10 files changed, 93 insertions(+), 18 deletions(-) diff --git a/libcef/browser/browser_platform_delegate.cc b/libcef/browser/browser_platform_delegate.cc index 9da6ba9f1..690e4cb8b 100644 --- a/libcef/browser/browser_platform_delegate.cc +++ b/libcef/browser/browser_platform_delegate.cc @@ -226,6 +226,9 @@ void CefBrowserPlatformDelegate::PopupWebContentsCreated( // Associate the PlatformDelegate with the new BrowserView. new_platform_delegate->SetBrowserView(new_browser_view); + + // Keep the BrowserView alive until PopupBrowserCreated() is called. + new_browser_view->AddRef(); } void CefBrowserPlatformDelegate::PopupBrowserCreated( @@ -263,6 +266,9 @@ void CefBrowserPlatformDelegate::PopupBrowserCreated( CefWindow::CreateTopLevelWindow( new PopupWindowDelegate(new_browser_view.get())); } + + // Release the reference added in PopupWebContentsCreated(). + new_browser_view->Release(); } CefRefPtr diff --git a/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.cc b/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.cc index 444492787..5696b9ba4 100644 --- a/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.cc +++ b/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.cc @@ -24,7 +24,8 @@ void CefBrowserPlatformDelegateChromeViews::SetBrowserView( CefRefPtr browser_view) { DCHECK(!browser_view_); DCHECK(browser_view); - browser_view_ = static_cast(browser_view.get()); + browser_view_ = + static_cast(browser_view.get())->GetWeakPtr(); } void CefBrowserPlatformDelegateChromeViews::WebContentsCreated( @@ -37,7 +38,10 @@ void CefBrowserPlatformDelegateChromeViews::WebContentsCreated( void CefBrowserPlatformDelegateChromeViews::WebContentsDestroyed( content::WebContents* web_contents) { CefBrowserPlatformDelegateChrome::WebContentsDestroyed(web_contents); - browser_view_->WebContentsDestroyed(web_contents); + // |browser_view_| may be destroyed before this callback arrives. + if (browser_view_) { + browser_view_->WebContentsDestroyed(web_contents); + } } void CefBrowserPlatformDelegateChromeViews::BrowserCreated( @@ -48,7 +52,7 @@ void CefBrowserPlatformDelegateChromeViews::BrowserCreated( void CefBrowserPlatformDelegateChromeViews::NotifyBrowserCreated() { if (auto delegate = browser_view_->delegate()) { - delegate->OnBrowserCreated(browser_view_, browser_.get()); + delegate->OnBrowserCreated(browser_view_.get(), browser_.get()); // DevTools windows hide the notification bubble by default. However, we // don't currently have the ability to intercept WebContents creation via @@ -75,8 +79,9 @@ void CefBrowserPlatformDelegateChromeViews::NotifyBrowserCreated() { } void CefBrowserPlatformDelegateChromeViews::NotifyBrowserDestroyed() { - if (browser_view_->delegate()) { - browser_view_->delegate()->OnBrowserDestroyed(browser_view_, + // |browser_view_| may be destroyed before this callback arrives. + if (browser_view_ && browser_view_->delegate()) { + browser_view_->delegate()->OnBrowserDestroyed(browser_view_.get(), browser_.get()); } } @@ -84,7 +89,11 @@ void CefBrowserPlatformDelegateChromeViews::NotifyBrowserDestroyed() { void CefBrowserPlatformDelegateChromeViews::BrowserDestroyed( CefBrowserHostBase* browser) { CefBrowserPlatformDelegateChrome::BrowserDestroyed(browser); - browser_view_->BrowserDestroyed(browser); + // |browser_view_| may be destroyed before this callback arrives. + if (browser_view_) { + browser_view_->BrowserDestroyed(browser); + } + browser_view_ = nullptr; } void CefBrowserPlatformDelegateChromeViews::CloseHostWindow() { @@ -100,7 +109,7 @@ CefWindowHandle CefBrowserPlatformDelegateChromeViews::GetHostWindowHandle() } views::Widget* CefBrowserPlatformDelegateChromeViews::GetWindowWidget() const { - if (browser_view_->root_view()) { + if (browser_view_ && browser_view_->root_view()) { return browser_view_->root_view()->GetWidget(); } return nullptr; diff --git a/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.h b/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.h index 094ac0e9d..efadab2d8 100644 --- a/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.h +++ b/libcef/browser/chrome/views/browser_platform_delegate_chrome_views.h @@ -5,6 +5,7 @@ #ifndef CEF_LIBCEF_BROWSER_CHROME_VIEWS_BROWSER_PLATFORM_DELEGATE_CHROME_VIEWS_H_ #define CEF_LIBCEF_BROWSER_CHROME_VIEWS_BROWSER_PLATFORM_DELEGATE_CHROME_VIEWS_H_ +#include "base/memory/weak_ptr.h" #include "cef/libcef/browser/chrome/browser_platform_delegate_chrome.h" #include "cef/libcef/browser/views/browser_view_impl.h" @@ -33,12 +34,17 @@ class CefBrowserPlatformDelegateChromeViews void SetBrowserView(CefRefPtr browser_view) override; bool IsViewsHosted() const override; - CefRefPtr browser_view() const { return browser_view_; } + CefBrowserViewImpl* browser_view() const { return browser_view_.get(); } private: CefWindowImpl* GetWindowImpl() const; - CefRefPtr browser_view_; + // Holding a weak reference here because we want the CefBrowserViewImpl to be + // destroyed first if all references are released by the client. + // CefBrowserViewImpl destruction will then trigger destruction of any + // associated CefBrowserHostBase (which owns this CefBrowserPlatformDelegate + // object). + base::WeakPtr browser_view_; }; #endif // CEF_LIBCEF_BROWSER_CHROME_VIEWS_BROWSER_PLATFORM_DELEGATE_CHROME_VIEWS_H_ diff --git a/libcef/browser/views/browser_platform_delegate_views.cc b/libcef/browser/views/browser_platform_delegate_views.cc index 250ebb251..2597945f6 100644 --- a/libcef/browser/views/browser_platform_delegate_views.cc +++ b/libcef/browser/views/browser_platform_delegate_views.cc @@ -27,7 +27,8 @@ void CefBrowserPlatformDelegateViews::SetBrowserView( CefRefPtr browser_view) { DCHECK(!browser_view_); DCHECK(browser_view); - browser_view_ = static_cast(browser_view.get()); + browser_view_ = + static_cast(browser_view.get())->GetWeakPtr(); } void CefBrowserPlatformDelegateViews::WebContentsCreated( @@ -41,7 +42,10 @@ void CefBrowserPlatformDelegateViews::WebContentsCreated( void CefBrowserPlatformDelegateViews::WebContentsDestroyed( content::WebContents* web_contents) { CefBrowserPlatformDelegateAlloy::WebContentsDestroyed(web_contents); - browser_view_->WebContentsCreated(web_contents); + // |browser_view_| may be destroyed before this callback arrives. + if (browser_view_) { + browser_view_->WebContentsDestroyed(web_contents); + } native_delegate_->WebContentsDestroyed(web_contents); } @@ -57,15 +61,16 @@ void CefBrowserPlatformDelegateViews::NotifyBrowserCreated() { DCHECK(browser_view_); DCHECK(browser_); if (browser_view_->delegate()) { - browser_view_->delegate()->OnBrowserCreated(browser_view_, browser_.get()); + browser_view_->delegate()->OnBrowserCreated(browser_view_.get(), + browser_.get()); } } void CefBrowserPlatformDelegateViews::NotifyBrowserDestroyed() { - DCHECK(browser_view_); DCHECK(browser_); - if (browser_view_->delegate()) { - browser_view_->delegate()->OnBrowserDestroyed(browser_view_, + // |browser_view_| may be destroyed before this callback arrives. + if (browser_view_ && browser_view_->delegate()) { + browser_view_->delegate()->OnBrowserDestroyed(browser_view_.get(), browser_.get()); } } @@ -74,7 +79,10 @@ void CefBrowserPlatformDelegateViews::BrowserDestroyed( CefBrowserHostBase* browser) { CefBrowserPlatformDelegateAlloy::BrowserDestroyed(browser); - browser_view_->BrowserDestroyed(browser); + // |browser_view_| may be destroyed before this callback arrives. + if (browser_view_) { + browser_view_->BrowserDestroyed(browser); + } browser_view_ = nullptr; native_delegate_->BrowserDestroyed(browser); } diff --git a/libcef/browser/views/browser_platform_delegate_views.h b/libcef/browser/views/browser_platform_delegate_views.h index 8b055b892..8a940a779 100644 --- a/libcef/browser/views/browser_platform_delegate_views.h +++ b/libcef/browser/views/browser_platform_delegate_views.h @@ -5,6 +5,7 @@ #ifndef CEF_LIBCEF_BROWSER_VIEWS_BROWSER_PLATFORM_DELEGATE_VIEWS_H_ #define CEF_LIBCEF_BROWSER_VIEWS_BROWSER_PLATFORM_DELEGATE_VIEWS_H_ +#include "base/memory/weak_ptr.h" #include "cef/libcef/browser/alloy/browser_platform_delegate_alloy.h" #include "cef/libcef/browser/native/browser_platform_delegate_native.h" #include "cef/libcef/browser/views/browser_view_impl.h" @@ -65,7 +66,13 @@ class CefBrowserPlatformDelegateViews private: std::unique_ptr native_delegate_; - CefRefPtr browser_view_; + + // Holding a weak reference here because we want the CefBrowserViewImpl to be + // destroyed first if all references are released by the client. + // CefBrowserViewImpl destruction will then trigger destruction of any + // associated CefBrowserHostBase (which owns this CefBrowserPlatformDelegate + // object). + base::WeakPtr browser_view_; }; #endif // CEF_LIBCEF_BROWSER_VIEWS_BROWSER_PLATFORM_DELEGATE_VIEWS_H_ diff --git a/libcef/browser/views/browser_view_impl.cc b/libcef/browser/views/browser_view_impl.cc index e545a3d68..73e6f7fac 100644 --- a/libcef/browser/views/browser_view_impl.cc +++ b/libcef/browser/views/browser_view_impl.cc @@ -144,6 +144,27 @@ CefRefPtr CefBrowserViewImpl::CreateForPopup( return browser_view; } +CefBrowserViewImpl::~CefBrowserViewImpl() { + // We want no further callbacks to this object. + weak_ptr_factory_.InvalidateWeakPtrs(); + + // |browser_| may exist here if the BrowserView was removed from the Views + // hierarchy prior to tear-down and the last BrowserView reference was + // released. In that case DisassociateFromWidget() will be called when the + // BrowserView is removed from the Window but Detach() will not be called + // because the BrowserView was not destroyed via the Views hierarchy + // tear-down. + DCHECK(!cef_widget_); + if (browser_ && !browser_->WillBeDestroyed()) { + // With Alloy style |browser_| will disappear when WindowDestroyed() + // indirectly calls BrowserDestroyed() so keep a reference. + CefRefPtr browser = browser_; + + // Force the browser to be destroyed. + browser->WindowDestroyed(); + } +} + void CefBrowserViewImpl::WebContentsCreated( content::WebContents* web_contents) { if (web_view()) { diff --git a/libcef/browser/views/browser_view_impl.h b/libcef/browser/views/browser_view_impl.h index 565c242eb..3dc890c13 100644 --- a/libcef/browser/views/browser_view_impl.h +++ b/libcef/browser/views/browser_view_impl.h @@ -35,6 +35,8 @@ class CefBrowserViewImpl CefBrowserViewImpl(const CefBrowserViewImpl&) = delete; CefBrowserViewImpl& operator=(const CefBrowserViewImpl&) = delete; + ~CefBrowserViewImpl() override; + // Create a new CefBrowserView instance. |delegate| may be nullptr. // |window_info| will only be used when creating a Chrome child window. static CefRefPtr Create( @@ -99,6 +101,10 @@ class CefBrowserViewImpl bool IsAlloyStyle() const { return is_alloy_style_; } bool IsChromeStyle() const { return !is_alloy_style_; } + base::WeakPtr GetWeakPtr() { + return weak_ptr_factory_.GetWeakPtr(); + } + private: // Create a new implementation object. // Always call Initialize() after creation. diff --git a/libcef/browser/views/overlay_view_host.cc b/libcef/browser/views/overlay_view_host.cc index 702e974e1..a22cca00f 100644 --- a/libcef/browser/views/overlay_view_host.cc +++ b/libcef/browser/views/overlay_view_host.cc @@ -312,6 +312,11 @@ void CefOverlayViewHost::OnViewBoundsChanged(views::View* observed_view) { MoveIfNecessary(); } +void CefOverlayViewHost::OnViewIsDeleting(views::View* observed_view) { + view_ = nullptr; + Cleanup(); +} + gfx::Rect CefOverlayViewHost::ComputeBounds() const { // This method is only used with corner docking. DCHECK_NE(docking_mode_, CEF_DOCKING_MODE_CUSTOM); diff --git a/libcef/browser/views/overlay_view_host.h b/libcef/browser/views/overlay_view_host.h index f59989006..e483757ee 100644 --- a/libcef/browser/views/overlay_view_host.h +++ b/libcef/browser/views/overlay_view_host.h @@ -44,6 +44,7 @@ class CefOverlayViewHost : public views::WidgetDelegate, // views::ViewObserver methods: void OnViewBoundsChanged(views::View* observed_view) override; + void OnViewIsDeleting(views::View* observed_view) override; cef_docking_mode_t docking_mode() const { return docking_mode_; } CefRefPtr controller() const { return cef_controller_; } diff --git a/tests/cefclient/browser/views_overlay_browser.cc b/tests/cefclient/browser/views_overlay_browser.cc index 3e38a3953..6b9f20fdf 100644 --- a/tests/cefclient/browser/views_overlay_browser.cc +++ b/tests/cefclient/browser/views_overlay_browser.cc @@ -33,9 +33,15 @@ void ViewsOverlayBrowser::Initialize( void ViewsOverlayBrowser::Destroy() { window_ = nullptr; - browser_view_ = nullptr; controller_->Destroy(); controller_ = nullptr; + + // We hold the last reference to the BrowserView, and releasing it will + // trigger overlay Browser destruction. OnBeforeClose for that Browser may be + // called synchronously or asynchronously depending on whether beforeunload + // needs to be dispatched. + DCHECK(browser_view_->HasOneRef()); + browser_view_ = nullptr; } void ViewsOverlayBrowser::UpdateBounds(CefInsets insets) {