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

@@ -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<CefBrowserViewDelegate>

libcef/browser/chrome/views/browser_platform_delegate_chrome_views.cc

@@ -24,7 +24,8 @@ void CefBrowserPlatformDelegateChromeViews::SetBrowserView(
     CefRefPtr<CefBrowserView> browser_view) {
   DCHECK(!browser_view_);
   DCHECK(browser_view);
-  browser_view_ = static_cast<CefBrowserViewImpl*>(browser_view.get());
+  browser_view_ =
+      static_cast<CefBrowserViewImpl*>(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_| 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_| 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;

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<CefBrowserView> browser_view) override;
   bool IsViewsHosted() const override;
 
-  CefRefPtr<CefBrowserViewImpl> browser_view() const { return browser_view_; }
+  CefBrowserViewImpl* browser_view() const { return browser_view_.get(); }
 
  private:
   CefWindowImpl* GetWindowImpl() const;
 
-  CefRefPtr<CefBrowserViewImpl> 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<CefBrowserViewImpl> browser_view_;
 };
 
 #endif  // CEF_LIBCEF_BROWSER_CHROME_VIEWS_BROWSER_PLATFORM_DELEGATE_CHROME_VIEWS_H_

libcef/browser/views/browser_platform_delegate_views.cc

@@ -27,7 +27,8 @@ void CefBrowserPlatformDelegateViews::SetBrowserView(
     CefRefPtr<CefBrowserView> browser_view) {
   DCHECK(!browser_view_);
   DCHECK(browser_view);
-  browser_view_ = static_cast<CefBrowserViewImpl*>(browser_view.get());
+  browser_view_ =
+      static_cast<CefBrowserViewImpl*>(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_| may be destroyed before this callback arrives.
+  if (browser_view_) {
     browser_view_->BrowserDestroyed(browser);
+  }
   browser_view_ = nullptr;
   native_delegate_->BrowserDestroyed(browser);
 }

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<CefBrowserPlatformDelegateNative> native_delegate_;
-  CefRefPtr<CefBrowserViewImpl> 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<CefBrowserViewImpl> browser_view_;
 };
 
 #endif  // CEF_LIBCEF_BROWSER_VIEWS_BROWSER_PLATFORM_DELEGATE_VIEWS_H_

libcef/browser/views/browser_view_impl.cc

@@ -144,6 +144,27 @@ CefRefPtr<CefBrowserViewImpl> 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<CefBrowserHostBase> browser = browser_;
+
+    // Force the browser to be destroyed.
+    browser->WindowDestroyed();
+  }
+}
+
 void CefBrowserViewImpl::WebContentsCreated(
     content::WebContents* web_contents) {
   if (web_view()) {

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<CefBrowserViewImpl> Create(
@@ -99,6 +101,10 @@ class CefBrowserViewImpl
   bool IsAlloyStyle() const { return is_alloy_style_; }
   bool IsChromeStyle() const { return !is_alloy_style_; }
 
+  base::WeakPtr<CefBrowserViewImpl> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
  private:
   // Create a new implementation object.
   // Always call Initialize() after creation.

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);

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<CefOverlayController> controller() const { return cef_controller_; }

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) {