From 7a02419cacd4212b66b5d99aba52859edaabadb0 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Tue, 9 May 2017 16:45:57 -0400 Subject: [PATCH] Fix crash when parent is destroyed during popup creation (issue #2041) --- include/capi/cef_life_span_handler_capi.h | 4 +- include/cef_life_span_handler.h | 4 +- libcef/browser/browser_host_impl.cc | 10 +++- libcef/browser/browser_info_manager.cc | 51 ++++++++++++----- libcef/renderer/content_renderer_client.cc | 3 +- tests/ceftests/request_context_unittest.cc | 64 +++++++++++++++++++++- 6 files changed, 112 insertions(+), 24 deletions(-) diff --git a/include/capi/cef_life_span_handler_capi.h b/include/capi/cef_life_span_handler_capi.h index 77885da09..0db15b839 100644 --- a/include/capi/cef_life_span_handler_capi.h +++ b/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, diff --git a/include/cef_life_span_handler.h b/include/cef_life_span_handler.h index b1acc4d7c..327c8b408 100644 --- a/include/cef_life_span_handler.h +++ b/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 browser, diff --git a/libcef/browser/browser_host_impl.cc b/libcef/browser/browser_host_impl.cc index 096ec108a..ef2a7f520 100644 --- a/libcef/browser/browser_host_impl.cc +++ b/libcef/browser/browser_host_impl.cc @@ -355,6 +355,11 @@ CefRefPtr 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::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 opener = GetBrowserForContents(source_contents); - DCHECK(opener.get()); + if (!opener.get()) + return; // Popups must share the same BrowserContext as the parent. CefBrowserContext* browser_context = diff --git a/libcef/browser/browser_info_manager.cc b/libcef/browser/browser_info_manager.cc index f0c8ba086..43a59d146 100644 --- a/libcef/browser/browser_info_manager.cc +++ b/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(); - while (it != pending_new_browser_info_list_.end()) { - PendingNewBrowserInfo* info = *it; - if (info->render_process_id == render_process_id) - it = pending_new_browser_info_list_.erase(it); - else - ++it; + { + base::AutoLock lock_scope(browser_info_lock_); + + PendingNewBrowserInfoList::iterator it = + pending_new_browser_info_list_.begin(); + while (it != pending_new_browser_info_list_.end()) { + PendingNewBrowserInfo* info = *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 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); - host->FilterURL(false, &pending_popup->target_url); - - GetInstance()->PushPendingPopup(std::move(pending_popup)); + if (host) { + host->FilterURL(false, &pending_popup->target_url); + GetInstance()->PushPendingPopup(std::move(pending_popup)); + } } -void CefBrowserInfoManager::PushPendingPopup(std::unique_ptr popup) { +void CefBrowserInfoManager::PushPendingPopup( + std::unique_ptr popup) { base::AutoLock lock_scope(pending_popup_lock_); pending_popup_list_.push_back(std::move(popup)); } diff --git a/libcef/renderer/content_renderer_client.cc b/libcef/renderer/content_renderer_client.cc index a46137973..aa5b4e482 100644 --- a/libcef/renderer/content_renderer_client.cc +++ b/libcef/renderer/content_renderer_client.cc @@ -848,9 +848,8 @@ void CefContentRendererClient::BrowserCreated( render_view_routing_id, render_frame_routing_id, ¶ms)); - 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; } diff --git a/tests/ceftests/request_context_unittest.cc b/tests/ceftests/request_context_unittest.cc index e842a28b9..c393239eb 100644 --- a/tests/ceftests/request_context_unittest.cc +++ b/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 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 {