diff --git a/libcef/browser/browser_host_impl.cc b/libcef/browser/browser_host_impl.cc index f581945b4..8ecc5fcac 100644 --- a/libcef/browser/browser_host_impl.cc +++ b/libcef/browser/browser_host_impl.cc @@ -68,7 +68,6 @@ #include "content/public/browser/render_view_host.h" #include "content/public/browser/render_widget_host.h" #include "content/public/browser/resource_request_info.h" -#include "content/public/common/browser_side_navigation_policy.h" #include "content/public/common/favicon_url.h" #include "extensions/browser/process_manager.h" #include "net/base/net_errors.h" @@ -2729,14 +2728,10 @@ void CefBrowserHostImpl::DidFinishNavigation( content::NavigationHandle* navigation_handle) { const net::Error error_code = navigation_handle->GetNetErrorCode(); - // With PlzNavigate the RenderFrameHost will only be nullptr if the - // provisional load fails, in which case |error_code| will be ERR_ABORTED. - // Without PlzNavigate the RenderFrameHost may be nullptr and |error_code| - // may be OK when a pending navigation is canceled (e.g. by calling LoadURL - // from OnCertificateError). - DCHECK(navigation_handle->GetRenderFrameHost() || - error_code == net::ERR_ABORTED || - !content::IsBrowserSideNavigationEnabled()); + // Skip calls where the navigation has not yet committed and there is no + // error code. For example, when creating a browser without loading a URL. + if (!navigation_handle->HasCommitted() && error_code == net::OK) + return; const int64 frame_id = navigation_handle->GetRenderFrameHost() @@ -2752,7 +2747,9 @@ void CefBrowserHostImpl::DidFinishNavigation( base::string16(), url); if (error_code == net::OK) { - // The navigation has been committed. + // The navigation has been committed and there is no error. + DCHECK(navigation_handle->HasCommitted()); + // Don't call OnLoadStart for same page navigations (fragments, // history state). if (!navigation_handle->IsSameDocument()) @@ -2761,21 +2758,21 @@ void CefBrowserHostImpl::DidFinishNavigation( if (is_main_frame) OnAddressChange(frame, url); } else { - // The navigation failed before commit. Originates from + // The navigation failed with an error. This may happen before commit + // (e.g. network error) or after commit (e.g. response filter error). + // If the error happened before commit then this call will originate from // RenderFrameHostImpl::OnDidFailProvisionalLoadWithError. // OnLoadStart/OnLoadEnd will not be called. OnLoadError(frame, navigation_handle->GetURL(), error_code); } - if (!web_contents()) - return; - - CefBrowserContext* context = - static_cast(web_contents()->GetBrowserContext()); - if (!context) - return; - - context->AddVisitedURLs(navigation_handle->GetRedirectChain()); + if (web_contents()) { + CefBrowserContext* context = + static_cast(web_contents()->GetBrowserContext()); + if (context) { + context->AddVisitedURLs(navigation_handle->GetRedirectChain()); + } + } } void CefBrowserHostImpl::DocumentAvailableInMainFrame() { diff --git a/tests/ceftests/navigation_unittest.cc b/tests/ceftests/navigation_unittest.cc index 0ce17195d..dfa53f075 100644 --- a/tests/ceftests/navigation_unittest.cc +++ b/tests/ceftests/navigation_unittest.cc @@ -2387,7 +2387,6 @@ const char kPopupJSOpenMainUrl[] = "http://www.tests-pjso.com/main.html"; const char kPopupJSOpenPopupUrl[] = "http://www.tests-pjso.com/popup.html"; // Test a popup where the URL is a JavaScript URI that opens another popup. -// See comments on CefBrowserInfoManager::FilterPendingPopupURL. class PopupJSWindowOpenTestHandler : public TestHandler { public: PopupJSWindowOpenTestHandler() @@ -2533,6 +2532,109 @@ TEST(NavigationTest, PopupJSWindowOpen) { namespace { +const char kPopupJSEmptyMainUrl[] = "http://www.tests-pjse.com/main.html"; + +// Test creation of a popup where the URL is empty. +class PopupJSWindowEmptyTestHandler : public TestHandler { + public: + PopupJSWindowEmptyTestHandler() {} + + void RunTest() override { + AddResource(kPopupJSEmptyMainUrl, "Main", "text/html"); + + // Create the browser. + CreateBrowser(kPopupJSEmptyMainUrl); + + // Time out the test after a reasonable period of time. + SetTestTimeout(); + } + + bool OnBeforePopup(CefRefPtr browser, + CefRefPtr frame, + const CefString& target_url, + const CefString& target_frame_name, + cef_window_open_disposition_t target_disposition, + bool user_gesture, + const CefPopupFeatures& popupFeatures, + CefWindowInfo& windowInfo, + CefRefPtr& client, + CefBrowserSettings& settings, + bool* no_javascript_access) override { + got_before_popup_.yes(); + return false; + } + + void OnAfterCreated(CefRefPtr browser) override { + TestHandler::OnAfterCreated(browser); + + if (browser->IsPopup()) { + got_after_created_popup_.yes(); + } + } + + void OnLoadingStateChange(CefRefPtr browser, + bool isLoading, + bool canGoBack, + bool canGoForward) override { + if (isLoading) + return; + + if (browser->IsPopup()) { + got_load_end_popup_.yes(); + CloseBrowser(browser, true); + } else { + browser->GetMainFrame()->LoadURL("javascript:window.open('')"); + } + } + + void OnLoadError(CefRefPtr browser, + CefRefPtr frame, + ErrorCode errorCode, + const CefString& errorText, + const CefString& failedUrl) override { + ADD_FAILURE() << "OnLoadError url: " << failedUrl.ToString() + << " error: " << errorCode; + } + + void OnBeforeClose(CefRefPtr browser) override { + TestHandler::OnBeforeClose(browser); + + if (browser->IsPopup()) { + got_before_close_popup_.yes(); + DestroyTest(); + } + } + + private: + void DestroyTest() override { + EXPECT_TRUE(got_before_popup_); + EXPECT_TRUE(got_after_created_popup_); + EXPECT_TRUE(got_load_end_popup_); + EXPECT_TRUE(got_before_close_popup_); + + TestHandler::DestroyTest(); + } + + TrackCallback got_before_popup_; + TrackCallback got_after_created_popup_; + TrackCallback got_load_end_popup_; + TrackCallback got_before_close_popup_; + + IMPLEMENT_REFCOUNTING(PopupJSWindowEmptyTestHandler); +}; + +} // namespace + +// Test creation of a popup where the URL is empty. +TEST(NavigationTest, PopupJSWindowEmpty) { + CefRefPtr handler = + new PopupJSWindowEmptyTestHandler(); + handler->ExecuteTest(); + ReleaseAndWaitForDestructor(handler); +} + +namespace { + const char kBrowseNavPageUrl[] = "http://tests-browsenav/nav.html"; // Browser side.