diff --git a/libcef/browser/chrome/chrome_browser_host_impl.cc b/libcef/browser/chrome/chrome_browser_host_impl.cc index 47d4bceb3..d05d5a862 100644 --- a/libcef/browser/chrome/chrome_browser_host_impl.cc +++ b/libcef/browser/chrome/chrome_browser_host_impl.cc @@ -124,8 +124,17 @@ void ChromeBrowserHostImpl::AddNewContents( void ChromeBrowserHostImpl::OnWebContentsDestroyed( content::WebContents* web_contents) { - platform_delegate_->WebContentsDestroyed(web_contents); - DestroyBrowser(); + // GetWebContents() should return nullptr at this point. + DCHECK(!GetWebContents()); + + // In most cases WebContents destruction will trigger browser destruction. + // The exception is if the browser still exists at CefShutdown, in which + // case DestroyBrowser() will be called first via + // CefBrowserInfoManager::DestroyAllBrowsers(). + if (platform_delegate_) { + platform_delegate_->WebContentsDestroyed(web_contents); + DestroyBrowser(); + } } void ChromeBrowserHostImpl::OnSetFocus(cef_focus_source_t source) { @@ -525,15 +534,34 @@ bool ChromeBrowserHostImpl::WillBeDestroyed() const { void ChromeBrowserHostImpl::DestroyBrowser() { CEF_REQUIRE_UIT(); - browser_ = nullptr; - weak_ptr_factory_.InvalidateWeakPtrs(); + // Notify that this browser has been destroyed. These must be delivered in + // the expected order. + + // 1. Notify the platform delegate. With Views this will result in a call to + // CefBrowserViewDelegate::OnBrowserDestroyed(). + platform_delegate_->NotifyBrowserDestroyed(); + + // 2. Notify the browser's LifeSpanHandler. This must always be the last + // notification for this browser. OnBeforeClose(); + + // Notify any observers that may have state associated with this browser. OnBrowserDestroyed(); + // If the WebContents still exists at this point, signal destruction before + // browser destruction. + if (auto web_contents = GetWebContents()) { + platform_delegate_->WebContentsDestroyed(web_contents); + } + // Disassociate the platform delegate from this browser. platform_delegate_->BrowserDestroyed(this); + // Clean up UI thread state. + browser_ = nullptr; + weak_ptr_factory_.InvalidateWeakPtrs(); + CefBrowserHostBase::DestroyBrowser(); } diff --git a/patch/patches/chrome_runtime_views.patch b/patch/patches/chrome_runtime_views.patch index 219c07a34..fbb66f77c 100644 --- a/patch/patches/chrome_runtime_views.patch +++ b/patch/patches/chrome_runtime_views.patch @@ -355,7 +355,7 @@ index 0c231b6ac5b01..6b5af98e18e42 100644 BrowserFrame(const BrowserFrame&) = delete; BrowserFrame& operator=(const BrowserFrame&) = delete; diff --git chrome/browser/ui/views/frame/browser_view.cc chrome/browser/ui/views/frame/browser_view.cc -index 2c4cb9a1e892c..b7336650fc418 100644 +index 2c4cb9a1e892c..2e38fc9d32d98 100644 --- chrome/browser/ui/views/frame/browser_view.cc +++ chrome/browser/ui/views/frame/browser_view.cc @@ -338,11 +338,10 @@ using content::NativeWebKeyboardEvent; @@ -423,7 +423,42 @@ index 2c4cb9a1e892c..b7336650fc418 100644 contents_separator_ = top_container_->AddChildView(std::make_unique()); -@@ -1147,12 +1163,14 @@ gfx::Size BrowserView::GetWebAppFrameToolbarPreferredSize() const { +@@ -1019,7 +1035,9 @@ BrowserView::~BrowserView() { + + // All the tabs should have been destroyed already. If we were closed by the + // OS with some tabs than the NativeBrowserFrame should have destroyed them. ++ if (browser_) { + DCHECK_EQ(0, browser_->tab_strip_model()->count()); ++ } + + // Stop the animation timer explicitly here to avoid running it in a nested + // message loop, which may run by Browser destructor. +@@ -1033,12 +1051,14 @@ BrowserView::~BrowserView() { + // child views and it is an observer for avatar toolbar button if any. + autofill_bubble_handler_.reset(); + ++ if (browser_) { + auto* global_registry = + extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile()); + if (global_registry->registry_for_active_window() == + extension_keybinding_registry_.get()) { + global_registry->set_registry_for_active_window(nullptr); + } ++ } + + // The TabStrip attaches a listener to the model. Make sure we shut down the + // TabStrip first so that it can cleanly remove the listener. +@@ -1056,7 +1076,9 @@ BrowserView::~BrowserView() { + + // `SidePanelUI::RemoveSidePanelUIForBrowser()` deletes the + // SidePanelCoordinator. ++ if (browser()) { + SidePanelUI::RemoveSidePanelUIForBrowser(browser()); ++ } + } + + // static +@@ -1147,12 +1169,14 @@ gfx::Size BrowserView::GetWebAppFrameToolbarPreferredSize() const { #if BUILDFLAG(IS_MAC) bool BrowserView::UsesImmersiveFullscreenMode() const { @@ -440,7 +475,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 } bool BrowserView::UsesImmersiveFullscreenTabbedMode() const { -@@ -1914,6 +1932,8 @@ bool BrowserView::ShouldHideUIForFullscreen() const { +@@ -1914,6 +1938,8 @@ bool BrowserView::ShouldHideUIForFullscreen() const { if (immersive_mode_controller_->IsEnabled()) return false; @@ -449,7 +484,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 return frame_->GetFrameView()->ShouldHideTopUIForFullscreen(); } -@@ -2921,7 +2941,8 @@ DownloadShelf* BrowserView::GetDownloadShelf() { +@@ -2921,7 +2947,8 @@ DownloadShelf* BrowserView::GetDownloadShelf() { } DownloadBubbleUIController* BrowserView::GetDownloadBubbleUIController() { @@ -459,7 +494,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 if (auto* download_button = toolbar_button_provider_->GetDownloadButton()) return download_button->bubble_controller(); return nullptr; -@@ -3454,7 +3475,8 @@ void BrowserView::ReparentTopContainerForEndOfImmersive() { +@@ -3454,7 +3481,8 @@ void BrowserView::ReparentTopContainerForEndOfImmersive() { if (top_container()->parent() == this) return; @@ -469,7 +504,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 top_container()->DestroyLayer(); AddChildViewAt(top_container(), 0); EnsureFocusOrder(); -@@ -3904,11 +3926,38 @@ void BrowserView::GetAccessiblePanes(std::vector* panes) { +@@ -3904,11 +3932,38 @@ void BrowserView::GetAccessiblePanes(std::vector* panes) { bool BrowserView::ShouldDescendIntoChildForEventHandling( gfx::NativeView child, const gfx::Point& location) { @@ -510,7 +545,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 // Draggable regions are defined relative to the web contents. gfx::Point point_in_contents_web_view_coords(location); views::View::ConvertPointToTarget(GetWidget()->GetRootView(), -@@ -3917,7 +3966,7 @@ bool BrowserView::ShouldDescendIntoChildForEventHandling( +@@ -3917,7 +3972,7 @@ bool BrowserView::ShouldDescendIntoChildForEventHandling( // Draggable regions should be ignored for clicks into any browser view's // owned widgets, for example alerts, permission prompts or find bar. @@ -519,7 +554,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 point_in_contents_web_view_coords.x(), point_in_contents_web_view_coords.y()) || WidgetOwnedByAnchorContainsPoint(point_in_contents_web_view_coords); -@@ -4025,8 +4074,10 @@ void BrowserView::Layout() { +@@ -4025,8 +4080,10 @@ void BrowserView::Layout() { // TODO(jamescook): Why was this in the middle of layout code? toolbar_->location_bar()->omnibox_view()->SetFocusBehavior( @@ -532,7 +567,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 #if BUILDFLAG(IS_CHROMEOS_ASH) // In chromeOS ash we round the bottom two corners of the browser frame by -@@ -4104,6 +4155,11 @@ void BrowserView::AddedToWidget() { +@@ -4104,6 +4161,11 @@ void BrowserView::AddedToWidget() { SetThemeProfileForWindow(GetNativeWindow(), browser_->profile()); #endif @@ -544,7 +579,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 toolbar_->Init(); // TODO(pbos): Investigate whether the side panels should be creatable when -@@ -4152,13 +4208,9 @@ void BrowserView::AddedToWidget() { +@@ -4152,13 +4214,9 @@ void BrowserView::AddedToWidget() { EnsureFocusOrder(); @@ -560,7 +595,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 using_native_frame_ = frame_->ShouldUseNativeFrame(); MaybeInitializeWebUITabStrip(); -@@ -4571,7 +4623,8 @@ void BrowserView::ProcessFullscreen(bool fullscreen, +@@ -4571,7 +4629,8 @@ void BrowserView::ProcessFullscreen(bool fullscreen, // Undo our anti-jankiness hacks and force a re-layout. in_process_fullscreen_ = false; ToolbarSizeChanged(false); @@ -570,7 +605,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 } bool BrowserView::ShouldUseImmersiveFullscreenForUrl(const GURL& url) const { -@@ -4942,6 +4995,8 @@ Profile* BrowserView::GetProfile() { +@@ -4942,6 +5001,8 @@ Profile* BrowserView::GetProfile() { } void BrowserView::UpdateUIForTabFullscreen() { @@ -579,7 +614,7 @@ index 2c4cb9a1e892c..b7336650fc418 100644 frame()->GetFrameView()->UpdateFullscreenTopUI(); } -@@ -4964,6 +5019,8 @@ void BrowserView::HideDownloadShelf() { +@@ -4964,6 +5025,8 @@ void BrowserView::HideDownloadShelf() { } bool BrowserView::CanUserExitFullscreen() const { diff --git a/tests/ceftests/test_handler.cc b/tests/ceftests/test_handler.cc index 5d4d6e363..b681b19e8 100644 --- a/tests/ceftests/test_handler.cc +++ b/tests/ceftests/test_handler.cc @@ -4,6 +4,8 @@ #include "tests/ceftests/test_handler.h" +#include + #include "include/base/cef_callback.h" #include "include/base/cef_logging.h" #include "include/cef_command_line.h" @@ -16,17 +18,29 @@ #include "tests/ceftests/test_util.h" #include "tests/shared/common/client_switches.h" +// Set to 1 to enable verbose debugging info logging. +#define VERBOSE_DEBUGGING 0 + namespace { +bool UseViews() { + static bool use_views = []() { + return CefCommandLine::GetGlobalCommandLine()->HasSwitch( + client::switches::kUseViews); + }(); + return use_views; +} + // Delegate implementation for the CefWindow that will host the Views-based // browser. class TestWindowDelegate : public CefWindowDelegate { public: // Create a new top-level Window hosting |browser_view|. - static void CreateBrowserWindow(CefRefPtr browser_view, + static void CreateBrowserWindow(TestHandler* handler, + CefRefPtr browser_view, const std::string& title) { - CefWindow::CreateTopLevelWindow( - new TestWindowDelegate(browser_view, "CefUnitTestViews " + title)); + CefWindow::CreateTopLevelWindow(new TestWindowDelegate( + handler, browser_view, "CefUnitTestViews " + title)); } // CefWindowDelegate methods: @@ -37,10 +51,17 @@ class TestWindowDelegate : public CefWindowDelegate { window->SetTitle(title_); window->AddChildView(browser_view_); window->Show(); + + // With Chrome runtime, the Browser is not created until after the + // BrowserView is assigned to the Window. + browser_id_ = browser_view_->GetBrowser()->GetIdentifier(); + handler_->OnWindowCreated(browser_id_); } void OnWindowDestroyed(CefRefPtr window) override { + auto browser = browser_view_->GetBrowser(); browser_view_ = nullptr; + handler_->OnWindowDestroyed(browser_id_); } bool CanClose(CefRefPtr window) override { @@ -53,11 +74,14 @@ class TestWindowDelegate : public CefWindowDelegate { } private: - TestWindowDelegate(CefRefPtr browser_view, + TestWindowDelegate(TestHandler* handler, + CefRefPtr browser_view, const CefString& title) - : browser_view_(browser_view), title_(title) {} + : handler_(handler), browser_view_(browser_view), title_(title) {} + TestHandler* const handler_; CefRefPtr browser_view_; + int browser_id_ = 0; CefString title_; IMPLEMENT_REFCOUNTING(TestWindowDelegate); @@ -67,20 +91,57 @@ class TestWindowDelegate : public CefWindowDelegate { // Delegate implementation for the CefBrowserView. class TestBrowserViewDelegate : public CefBrowserViewDelegate { public: - TestBrowserViewDelegate() {} + explicit TestBrowserViewDelegate(TestHandler* handler) : handler_(handler) {} // CefBrowserViewDelegate methods: + void OnBrowserDestroyed(CefRefPtr browser_view, + CefRefPtr browser) override { +#if VERBOSE_DEBUGGING + LOG(INFO) << handler_->debug_string_prefix() << browser->GetIdentifier() + << ": OnBrowserDestroyed"; +#endif + // Always close the containing Window when the browser is destroyed. + if (auto window = browser_view->GetWindow()) { +#if VERBOSE_DEBUGGING + LOG(INFO) << handler_->debug_string_prefix() << browser->GetIdentifier() + << ": OnBrowserDestroyed Close"; +#endif + window->Close(); + } + } + + CefRefPtr GetDelegateForPopupBrowserView( + CefRefPtr browser_view, + const CefBrowserSettings& settings, + CefRefPtr client, + bool is_devtools) override { + if (client.get() == handler_) { + // Use the same Delegate when using the same TestHandler instance. + return this; + } + + // Return a new Delegate when using a different TestHandler instance. + auto* handler = static_cast(client.get()); + return new TestBrowserViewDelegate(handler); + } + bool OnPopupBrowserViewCreated(CefRefPtr browser_view, CefRefPtr popup_browser_view, bool is_devtools) override { + // The popup may use a different TestHandler instance. + auto* handler = static_cast( + popup_browser_view->GetBrowser()->GetHost()->GetClient().get()); + // Create our own Window for popups. It will show itself after creation. - TestWindowDelegate::CreateBrowserWindow(popup_browser_view, + TestWindowDelegate::CreateBrowserWindow(handler, popup_browser_view, is_devtools ? "DevTools" : "Popup"); return true; } private: + TestHandler* const handler_; + IMPLEMENT_REFCOUNTING(TestBrowserViewDelegate); DISALLOW_COPY_AND_ASSIGN(TestBrowserViewDelegate); }; @@ -175,11 +236,7 @@ void TestHandler::UIThreadHelper::TaskHelper(base::OnceClosure task) { int TestHandler::browser_count_ = 0; TestHandler::TestHandler(CompletionState* completion_state) - : first_browser_id_(0), - signal_completion_when_all_browsers_close_(true), - destroy_event_(nullptr), - destroy_test_expected_(true), - destroy_test_called_(false) { + : debug_string_prefix_(MakeDebugStringPrefix()) { if (completion_state) { completion_state_ = completion_state; completion_state_owned_ = false; @@ -197,6 +254,8 @@ TestHandler::~TestHandler() { EXPECT_FALSE(destroy_test_called_); } EXPECT_TRUE(browser_map_.empty()); + EXPECT_EQ(0U, window_count_); + EXPECT_TRUE(browser_status_map_.empty()); if (completion_state_owned_) { delete completion_state_; @@ -210,8 +269,6 @@ TestHandler::~TestHandler() { void TestHandler::OnAfterCreated(CefRefPtr browser) { EXPECT_UI_THREAD(); - browser_count_++; - const int browser_id = browser->GetIdentifier(); EXPECT_EQ(browser_map_.find(browser_id), browser_map_.end()); if (browser_map_.empty()) { @@ -219,6 +276,8 @@ void TestHandler::OnAfterCreated(CefRefPtr browser) { first_browser_ = browser; } browser_map_.insert(std::make_pair(browser_id, browser)); + + OnCreated(browser_id, NT_BROWSER); } void TestHandler::OnBeforeClose(CefRefPtr browser) { @@ -235,12 +294,102 @@ void TestHandler::OnBeforeClose(CefRefPtr browser) { first_browser_ = nullptr; } - if (browser_map_.empty() && signal_completion_when_all_browsers_close_) { - // Signal that the test is now complete. + OnClosed(browser_id, NT_BROWSER); +} + +void TestHandler::OnWindowCreated(int browser_id) { + CHECK(UseViews()); + EXPECT_UI_THREAD(); + window_count_++; + OnCreated(browser_id, NT_WINDOW); +} + +void TestHandler::OnWindowDestroyed(int browser_id) { + CHECK(UseViews()); + EXPECT_UI_THREAD(); + window_count_--; + OnClosed(browser_id, NT_WINDOW); +} + +void TestHandler::OnCreated(int browser_id, NotifyType type) { + bool creation_complete = false; + + auto& browser_status = browser_status_map_[browser_id]; + EXPECT_FALSE(browser_status.got_created[type]) + << "Duplicate call to OnCreated(" << browser_id << ", " + << (type == NT_BROWSER ? "BROWSER" : "WINDOW") << ")"; + browser_status.got_created[type].yes(); + + // When using Views, wait for both Browser and Window notifications. + if (UseViews()) { + creation_complete = browser_status.got_created[NT_BROWSER] && + browser_status.got_created[NT_WINDOW]; + } else { + creation_complete = browser_status.got_created[NT_BROWSER]; + } + +#if VERBOSE_DEBUGGING + LOG(INFO) << debug_string_prefix_ << browser_id << ": OnCreated type=" + << (type == NT_BROWSER ? "BROWSER" : "WINDOW") + << " creation_complete=" << creation_complete; +#endif + + if (creation_complete) { + browser_count_++; + } +} + +void TestHandler::OnClosed(int browser_id, NotifyType type) { + bool close_complete = false; + + auto& browser_status = browser_status_map_[browser_id]; + EXPECT_FALSE(browser_status.got_closed[type]) + << "Duplicate call to OnClosed(" << browser_id << ", " + << (type == NT_BROWSER ? "BROWSER" : "WINDOW") << ")"; + browser_status.got_closed[type].yes(); + + // When using Views, wait for both Browser and Window notifications. + if (UseViews()) { + close_complete = browser_status.got_closed[NT_BROWSER] && + browser_status.got_closed[NT_WINDOW]; + } else { + close_complete = browser_status.got_closed[NT_BROWSER]; + } + + // Test is complete if no Browsers/Windows are remaining. + const bool test_complete = + close_complete && + (UseViews() ? window_count_ == 0 : browser_map_.empty()); + +#if VERBOSE_DEBUGGING + LOG(INFO) << debug_string_prefix_ << browser_id + << ": OnClosed type=" << (type == NT_BROWSER ? "BROWSER" : "WINDOW") + << " close_complete=" << close_complete + << " test_complete=" << test_complete; +#endif + + if (close_complete) { + browser_status_map_.erase(browser_id); + } + + if (test_complete && signal_completion_when_all_browsers_close_) { + // Signal that the test is now complete. May result in |this| being deleted. TestComplete(); } - browser_count_--; + if (close_complete) { + browser_count_--; + } +} + +std::string TestHandler::MakeDebugStringPrefix() const { +#if VERBOSE_DEBUGGING + std::stringstream ss; + ss << "TestHandler [0x" << std::hex << this << "]: "; + return ss.str(); +#else + return std::string(); +#endif } namespace { @@ -359,9 +508,7 @@ void TestHandler::OnTestTimeout(int timeout_ms, bool treat_as_error) { void TestHandler::CreateBrowser(const CefString& url, CefRefPtr request_context, CefRefPtr extra_info) { - const bool use_views = CefCommandLine::GetGlobalCommandLine()->HasSwitch( - client::switches::kUseViews); - if (use_views && !CefCurrentlyOn(TID_UI)) { + if (UseViews() && !CefCurrentlyOn(TID_UI)) { // Views classes must be accessed on the UI thread. CefPostTask(TID_UI, base::BindOnce(&TestHandler::CreateBrowser, this, url, request_context, extra_info)); @@ -371,14 +518,14 @@ void TestHandler::CreateBrowser(const CefString& url, CefWindowInfo windowInfo; CefBrowserSettings settings; - if (use_views) { + if (UseViews()) { // Create the BrowserView. CefRefPtr browser_view = CefBrowserView::CreateBrowserView( this, url, settings, extra_info, request_context, - new TestBrowserViewDelegate()); + new TestBrowserViewDelegate(this)); // Create the Window. It will show itself after creation. - TestWindowDelegate::CreateBrowserWindow(browser_view, std::string()); + TestWindowDelegate::CreateBrowserWindow(this, browser_view, std::string()); } else { #if defined(OS_WIN) windowInfo.SetAsPopup(nullptr, "CefUnitTest"); @@ -392,6 +539,10 @@ void TestHandler::CreateBrowser(const CefString& url, // static void TestHandler::CloseBrowser(CefRefPtr browser, bool force_close) { +#if VERBOSE_DEBUGGING + LOG(INFO) << "TestHandler: " << browser->GetIdentifier() + << ": CloseBrowser force_close=" << force_close; +#endif browser->GetHost()->CloseBrowser(force_close); } @@ -461,6 +612,10 @@ void TestHandler::TestComplete() { return; } +#if VERBOSE_DEBUGGING + LOG(INFO) << debug_string_prefix_ << "TestComplete"; +#endif + EXPECT_TRUE(browser_map_.empty()); completion_state_->TestComplete(); } diff --git a/tests/ceftests/test_handler.h b/tests/ceftests/test_handler.h index bffd5f5f8..9613176f4 100644 --- a/tests/ceftests/test_handler.h +++ b/tests/ceftests/test_handler.h @@ -206,9 +206,15 @@ class TestHandler : public CefClient, destroy_test_expected_ = expected; } + // Called from TestWindowDelegate when Views is enabled. + void OnWindowCreated(int browser_id); + void OnWindowDestroyed(int browser_id); + // Returns true if a browser currently exists. static bool HasBrowser() { return browser_count_ > 0; } + std::string debug_string_prefix() const { return debug_string_prefix_; } + protected: // Indicate that test setup is complete. Only used in combination with a // Collection. @@ -260,17 +266,38 @@ class TestHandler : public CefClient, UIThreadHelper* GetUIThreadHelper(); private: + enum NotifyType { + NT_BROWSER, + NT_WINDOW, + }; + void OnCreated(int browser_id, NotifyType type); + void OnClosed(int browser_id, NotifyType type); + + std::string MakeDebugStringPrefix() const; + const std::string debug_string_prefix_; + // Used to notify when the test is complete. Can be accessed on any thread. CompletionState* completion_state_; bool completion_state_owned_; - // Map browser ID to browser object for non-popup browsers. Only accessed on - // the UI thread. + // Map of browser ID to browser object. Only accessed on the UI thread. BrowserMap browser_map_; + // Count of window objects. Only accessed on the UI thread. + size_t window_count_ = 0; + + struct NotifyStatus { + // Keyed by NotifyType. + TrackCallback got_created[2]; + TrackCallback got_closed[2]; + }; + + // Map of browser ID to current status. Only accessed on the UI thread. + std::map browser_status_map_; + // Values for the first created browser. Modified on the UI thread but can be // accessed on any thread. - int first_browser_id_; + int first_browser_id_ = 0; CefRefPtr first_browser_; // Map of resources that can be automatically loaded. Only accessed on the @@ -279,13 +306,13 @@ class TestHandler : public CefClient, ResourceMap resource_map_; // If true test completion will be signaled when all browsers have closed. - bool signal_completion_when_all_browsers_close_; + bool signal_completion_when_all_browsers_close_ = true; CefRefPtr destroy_event_; // Tracks whether DestroyTest() is expected or has been called. - bool destroy_test_expected_; - bool destroy_test_called_; + bool destroy_test_expected_ = true; + bool destroy_test_called_ = false; std::unique_ptr ui_thread_helper_;