Fix crash when parent is destroyed during popup creation (issue #2041)

include/capi/cef_life_span_handler_capi.h

@@ -76,7 +76,9 @@ typedef struct _cef_life_span_handler_t {
   // is set to false (0) the new browser will not be scriptable and may not be
   // hosted in the same renderer process as the source browser. Any
   // modifications to |windowInfo| will be ignored if the parent browser is
-  // wrapped in a cef_browser_view_t.
+  // wrapped in a cef_browser_view_t. Popup browser creation will be canceled if
+  // the parent browser is destroyed before the popup browser creation completes
+  // (indicated by a call to OnAfterCreated for the popup browser).
   ///
   int (CEF_CALLBACK *on_before_popup)(struct _cef_life_span_handler_t* self,
       struct _cef_browser_t* browser, struct _cef_frame_t* frame,

include/cef_life_span_handler.h

@@ -71,7 +71,9 @@ class CefLifeSpanHandler : public virtual CefBaseRefCounted {
   // false the new browser will not be scriptable and may not be hosted in the
   // same renderer process as the source browser. Any modifications to
   // |windowInfo| will be ignored if the parent browser is wrapped in a
-  // CefBrowserView.
+  // CefBrowserView. Popup browser creation will be canceled if the parent
+  // browser is destroyed before the popup browser creation completes (indicated
+  // by a call to OnAfterCreated for the popup browser).
   ///
   /*--cef(optional_param=target_url,optional_param=target_frame_name)--*/
   virtual bool OnBeforePopup(CefRefPtr<CefBrowser> browser,

libcef/browser/browser_host_impl.cc

@@ -355,6 +355,11 @@ CefRefPtr<CefBrowserHostImpl> CefBrowserHostImpl::CreateInternal(
   DCHECK(!opener.get() || browser_info->is_popup());
 
   if (opener) {
+    if (!opener->platform_delegate_) {
+      // The opener window is being destroyed. Cancel the popup.
+      return nullptr;
+    }
+
     // Give the opener browser's platform delegate an opportunity to modify the
     // new browser's platform delegate.
     opener->platform_delegate_->PopupWebContentsCreated(
@@ -386,7 +391,7 @@ CefRefPtr<CefBrowserHostImpl> CefBrowserHostImpl::CreateInternal(
   // CefBrowserViewDelegate::OnBrowserCreated().
   browser->platform_delegate_->NotifyBrowserCreated();
 
-  if (opener) {
+  if (opener && opener->platform_delegate_) {
     // 3. Notify the opener browser's platform delegate. With Views this will
     // result in a call to CefBrowserViewDelegate::OnPopupBrowserViewCreated().
     opener->platform_delegate_->PopupBrowserCreated(browser.get(),
@@ -2292,7 +2297,8 @@ void CefBrowserHostImpl::WebContentsCreated(
   DCHECK(info->is_popup());
 
   CefRefPtr<CefBrowserHostImpl> opener = GetBrowserForContents(source_contents);
-  DCHECK(opener.get());
+  if (!opener.get())
+    return;
 
   // Popups must share the same BrowserContext as the parent.
   CefBrowserContext* browser_context =

libcef/browser/browser_info_manager.cc

@@ -409,34 +409,55 @@ void CefBrowserInfoManager::GetBrowserInfoList(BrowserInfoList& list) {
 
 void CefBrowserInfoManager::RenderProcessHostDestroyed(
     content::RenderProcessHost* host) {
-  base::AutoLock lock_scope(browser_info_lock_);
-
   const int render_process_id = host->GetID();
+  DCHECK_GT(render_process_id, 0);
 
   // Remove all pending requests that reference the destroyed host.
-  PendingNewBrowserInfoList::iterator it =
+  {
-      pending_new_browser_info_list_.begin();
+    base::AutoLock lock_scope(browser_info_lock_);
-  while (it != pending_new_browser_info_list_.end()) {
+
-    PendingNewBrowserInfo* info = *it;
+    PendingNewBrowserInfoList::iterator it =
-    if (info->render_process_id == render_process_id)
+        pending_new_browser_info_list_.begin();
-      it = pending_new_browser_info_list_.erase(it);
+    while (it != pending_new_browser_info_list_.end()) {
-    else
+      PendingNewBrowserInfo* info = *it;
-      ++it;
+      if (info->render_process_id == render_process_id)
+        it = pending_new_browser_info_list_.erase(it);
+      else
+        ++it;
+    }
+  }
+
+  // Remove all pending popups that reference the destroyed host as the opener.
+  {
+    base::AutoLock lock_scope(pending_popup_lock_);
+
+    PendingPopupList::iterator it = pending_popup_list_.begin();
+    while (it != pending_popup_list_.end()) {
+      PendingPopup* popup = *it;
+      if (popup->opener_process_id == render_process_id) {
+        it = pending_popup_list_.erase(it);
+      } else {
+        ++it;
+      }
+    }
   }
 }
 
 void CefBrowserInfoManager::FilterPendingPopupURL(
     int opener_process_id,
     std::unique_ptr<CefBrowserInfoManager::PendingPopup> pending_popup) {
+  // |host| may be nullptr if the parent browser is destroyed while the popup is
+  // pending.
   content::RenderProcessHost* host =
       content::RenderProcessHost::FromID(opener_process_id);
-  DCHECK(host);
+  if (host) {
-  host->FilterURL(false, &pending_popup->target_url);
+    host->FilterURL(false, &pending_popup->target_url);
-
+    GetInstance()->PushPendingPopup(std::move(pending_popup));
-  GetInstance()->PushPendingPopup(std::move(pending_popup));
+  }
 }
 
-void CefBrowserInfoManager::PushPendingPopup(std::unique_ptr<PendingPopup> popup) {
+void CefBrowserInfoManager::PushPendingPopup(
+    std::unique_ptr<PendingPopup> popup) {
   base::AutoLock lock_scope(pending_popup_lock_);
   pending_popup_list_.push_back(std::move(popup));
 }

libcef/renderer/content_renderer_client.cc

@@ -848,9 +848,8 @@ void CefContentRendererClient::BrowserCreated(
           render_view_routing_id,
           render_frame_routing_id,
           &params));
-  DCHECK_GT(params.browser_id, 0);
   if (params.browser_id == 0) {
-    // The request failed for some reason.
+    // The popup may have been canceled during creation.
     return;
   }
 

tests/ceftests/request_context_unittest.cc

@@ -631,6 +631,12 @@ class PopupNavTestHandler : public TestHandler {
     ALLOW_CLOSE_POPUP_LAST,
     DENY,
     NAVIGATE_AFTER_CREATION,
+    DESTROY_PARENT_BEFORE_CREATION,
+    DESTROY_PARENT_BEFORE_CREATION_FORCE,
+    DESTROY_PARENT_DURING_CREATION,
+    DESTROY_PARENT_DURING_CREATION_FORCE,
+    DESTROY_PARENT_AFTER_CREATION,
+    DESTROY_PARENT_AFTER_CREATION_FORCE,
   };
   enum RCMode {
     RC_MODE_NONE,
@@ -700,12 +706,26 @@ class PopupNavTestHandler : public TestHandler {
     EXPECT_FALSE(user_gesture);
     EXPECT_FALSE(*no_javascript_access);
 
+    if (mode_ == DESTROY_PARENT_DURING_CREATION ||
+        mode_ == DESTROY_PARENT_DURING_CREATION_FORCE) {
+      // Destroy the main (parent) browser while popup creation is pending.
+      CloseBrowser(browser, mode_ == DESTROY_PARENT_DURING_CREATION_FORCE);
+    }
+
     return (mode_ == DENY);  // Return true to cancel the popup.
   }
 
   void OnAfterCreated(CefRefPtr<CefBrowser> browser) override {
     TestHandler::OnAfterCreated(browser);
 
+    if (browser->IsPopup() &&
+        (mode_ == DESTROY_PARENT_AFTER_CREATION ||
+        mode_ == DESTROY_PARENT_AFTER_CREATION_FORCE)) {
+      // Destroy the main (parent) browser immediately after the popup is
+      // created.
+      CloseBrowser(GetBrowser(), mode_ == DESTROY_PARENT_AFTER_CREATION_FORCE);
+    }
+
     if (mode_ == NAVIGATE_AFTER_CREATION && browser->IsPopup()) {
       // Navigate to the 2nd popup URL instead of the 1st popup URL.
       browser->GetMainFrame()->LoadURL(kPopupNavPopupUrl2);
@@ -755,6 +775,13 @@ class PopupNavTestHandler : public TestHandler {
 
       frame->ExecuteJavaScript("doPopup()", kPopupNavPageUrl, 0);
 
+      if (mode_ == DESTROY_PARENT_BEFORE_CREATION ||
+          mode_ == DESTROY_PARENT_BEFORE_CREATION_FORCE) {
+        // Destroy the main (parent) browser immediately before the popup is
+        // created.
+        CloseBrowser(browser, mode_ == DESTROY_PARENT_BEFORE_CREATION_FORCE);
+      }
+
       if (mode_ == DENY) {
         // Wait a bit to make sure the popup window isn't created.
         CefPostDelayedTask(TID_UI,
@@ -796,7 +823,13 @@ class PopupNavTestHandler : public TestHandler {
       // Destroy the test after the popup browser closes.
       if (browser->IsPopup())
         destroy_test = true;
-    } else if (mode_ == ALLOW_CLOSE_POPUP_LAST) {
+    } else if (mode_ == ALLOW_CLOSE_POPUP_LAST ||
+               mode_ == DESTROY_PARENT_BEFORE_CREATION ||
+               mode_ == DESTROY_PARENT_BEFORE_CREATION_FORCE ||
+               mode_ == DESTROY_PARENT_DURING_CREATION ||
+               mode_ == DESTROY_PARENT_DURING_CREATION_FORCE ||
+               mode_ == DESTROY_PARENT_AFTER_CREATION ||
+               mode_ == DESTROY_PARENT_AFTER_CREATION_FORCE) {
       // Destroy the test after the main browser closes.
       if (!browser->IsPopup())
         destroy_test = true;
@@ -812,7 +845,14 @@ class PopupNavTestHandler : public TestHandler {
     EXPECT_TRUE(got_load_start_);
     EXPECT_FALSE(got_load_error_);
     EXPECT_TRUE(got_load_end_);
-    EXPECT_TRUE(got_on_before_popup_);
+
+    // OnBeforePopup may come before or after browser destruction with the
+    // DESTROY_PARENT_BEFORE_CREATION* tests.
+    if (mode_ != DESTROY_PARENT_BEFORE_CREATION &&
+        mode_ != DESTROY_PARENT_BEFORE_CREATION_FORCE) {
+      EXPECT_TRUE(got_on_before_popup_);
+    }
+
     if (mode_ == ALLOW_CLOSE_POPUP_FIRST || mode_ == ALLOW_CLOSE_POPUP_LAST) {
       EXPECT_TRUE(got_popup_load_start_);
       EXPECT_FALSE(got_popup_load_error_);
@@ -820,7 +860,13 @@ class PopupNavTestHandler : public TestHandler {
       EXPECT_FALSE(got_popup_load_start2_);
       EXPECT_FALSE(got_popup_load_error2_);
       EXPECT_FALSE(got_popup_load_end2_);
-    } else if (mode_ == DENY) {
+    } else if (mode_ == DENY ||
+               mode_ == DESTROY_PARENT_BEFORE_CREATION ||
+               mode_ == DESTROY_PARENT_BEFORE_CREATION_FORCE ||
+               mode_ == DESTROY_PARENT_DURING_CREATION ||
+               mode_ == DESTROY_PARENT_DURING_CREATION_FORCE ||
+               mode_ == DESTROY_PARENT_AFTER_CREATION ||
+               mode_ == DESTROY_PARENT_AFTER_CREATION_FORCE) {
       EXPECT_FALSE(got_popup_load_start_);
       EXPECT_FALSE(got_popup_load_error_);
       EXPECT_FALSE(got_popup_load_end_);
@@ -887,6 +933,18 @@ POPUP_TEST_GROUP(Deny, DENY);
 // internal objects are tracked correctly (see issue #1392).
 POPUP_TEST_GROUP(NavigateAfterCreation, NAVIGATE_AFTER_CREATION);
 
+// Test destroying the parent browser during or immediately after popup creation
+// to verify that internal objects are tracked correctly (see issue #2041).
+POPUP_TEST_GROUP(DestroyParentBeforeCreation, DESTROY_PARENT_BEFORE_CREATION);
+POPUP_TEST_GROUP(DestroyParentBeforeCreationForce,
+                 DESTROY_PARENT_BEFORE_CREATION_FORCE);
+POPUP_TEST_GROUP(DestroyParentDuringCreation, DESTROY_PARENT_DURING_CREATION);
+POPUP_TEST_GROUP(DestroyParentDuringCreationForce,
+                 DESTROY_PARENT_DURING_CREATION_FORCE);
+POPUP_TEST_GROUP(DestroyParentAfterCreation, DESTROY_PARENT_AFTER_CREATION);
+POPUP_TEST_GROUP(DestroyParentAfterCreationForce,
+                 DESTROY_PARENT_AFTER_CREATION_FORCE);
+
 
 namespace {