From 39758987b5e955b4d75fe1f53d5c703dcda9f425 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Mon, 6 Jul 2026 17:44:01 -0400 Subject: [PATCH] chrome: Fix dangling pointers during child window teardown (fixes #4147) Fixes two teardown-ordering hazards for Chrome style browsers created with a native parent window (CefWindowInfo::SetAsChild): - ChromeBrowserHostImpl::DestroyBrowser() may destroy the platform delegate before Widget teardown destroys the ChildWindowDelegate, leaving its raw_ptr dangling (SIGTRAP with PartitionAllocDanglingPtr). The pointer was only cleared in OnWindowDestroyed on Windows, and only if that ran first. Store a base::WeakPtr from the existing CefBrowserPlatformDelegateNativeAura factory instead, which auto-invalidates on delegate destruction on all Aura platforms. - In the reverse order on Windows, the client may destroy the parent window (and thereby the browser's Widget) while the browser is still alive, leaving the Widget pointer cached by set_widget() dangling. SetFocus, NotifyMoveOrResizeStarted and root window bounds queries could then dereference the deleted Widget. Reuse the existing WidgetDeleted mechanism (already wired up for the non-Views-hosted code path in CreateHostWindow) by executing the delete callback from ChildWindowDelegate::OnWindowDestroyed. The callback is bound with a WeakPtr, so it no-ops safely when the delegate is destroyed first. Adds ceftests coverage for both teardown orders (Windows and Linux X11). Dangling pointer regressions are detected when running with --enable-features=PartitionAllocBackupRefPtr,PartitionAllocDanglingPtr or in an ASAN build. Chrome style is not supported with a native parent on MacOS (see issue #3294). --- cef_paths2.gypi | 1 + .../chrome/views/chrome_child_window.cc | 26 +- .../browser_platform_delegate_native_aura.h | 10 +- .../ceftests/chrome_child_window_unittest.cc | 323 ++++++++++++++++++ 4 files changed, 354 insertions(+), 6 deletions(-) create mode 100644 tests/ceftests/chrome_child_window_unittest.cc diff --git a/cef_paths2.gypi b/cef_paths2.gypi index 4211cfcd2..f519b1b17 100644 --- a/cef_paths2.gypi +++ b/cef_paths2.gypi @@ -565,6 +565,7 @@ 'tests/ceftests/browser_info_map_unittest.cc', 'tests/ceftests/browser_settings_unittest.cc', 'tests/ceftests/certificate_error_unittest.cc', + 'tests/ceftests/chrome_child_window_unittest.cc', 'tests/ceftests/command_line_unittest.cc', 'tests/ceftests/cookie_unittest.cc', 'tests/ceftests/cors_unittest.cc', diff --git a/libcef/browser/chrome/views/chrome_child_window.cc b/libcef/browser/chrome/views/chrome_child_window.cc index dee8c9894..b13ab413b 100644 --- a/libcef/browser/chrome/views/chrome_child_window.cc +++ b/libcef/browser/chrome/views/chrome_child_window.cc @@ -10,11 +10,13 @@ #include "ui/gfx/native_ui_types.h" #if BUILDFLAG(IS_WIN) +#include "base/functional/callback.h" #include "cef/libcef/browser/native/browser_platform_delegate_native_win.h" #include "ui/views/win/hwnd_util.h" #endif #if defined(USE_AURA) +#include "base/memory/weak_ptr.h" #include "cef/libcef/browser/native/browser_platform_delegate_native_aura.h" #endif @@ -66,7 +68,11 @@ class ChildWindowDelegate : public CefWindowDelegate { browser_view_ = nullptr; window_ = nullptr; #if BUILDFLAG(IS_WIN) - native_delegate_ = nullptr; + // Clear the native delegate's Widget state, as the Widget is being + // destroyed. The delegate may outlive the Widget. + if (widget_delete_callback_) { + std::move(widget_delete_callback_).Run(); + } #endif } @@ -96,8 +102,11 @@ class ChildWindowDelegate : public CefWindowDelegate { DCHECK(platform_delegate->IsViewsHosted()); auto chrome_delegate = static_cast(platform_delegate); - native_delegate_ = static_cast( + auto* native_delegate = static_cast( chrome_delegate->native_delegate()); + // The native delegate may be destroyed before this object, so keep a weak + // reference. + native_delegate_ = native_delegate->GetWeakPtr(); #if BUILDFLAG(IS_WIN) auto widget = static_cast(window_.get())->widget(); @@ -107,9 +116,13 @@ class ChildWindowDelegate : public CefWindowDelegate { // The Windows delegate needs state to perform some actions. auto* delegate_win = - static_cast(native_delegate_); + static_cast(native_delegate); delegate_win->set_widget(widget, widget_hwnd); + // Executed from OnWindowDestroyed() to clear the above state if the + // delegate is still alive at that time. + widget_delete_callback_ = delegate_win->GetWidgetDeleteCallback(); + if (window_info_.ex_style & WS_EX_NOACTIVATE) { const DWORD widget_ex_styles = GetWindowLongPtr(widget_hwnd, GWL_EXSTYLE); @@ -145,8 +158,11 @@ class ChildWindowDelegate : public CefWindowDelegate { CefRefPtr window_; #if defined(USE_AURA) - base::raw_ptr native_delegate_ = - nullptr; + base::WeakPtr native_delegate_; +#endif + +#if BUILDFLAG(IS_WIN) + base::OnceClosure widget_delete_callback_; #endif IMPLEMENT_REFCOUNTING(ChildWindowDelegate); diff --git a/libcef/browser/native/browser_platform_delegate_native_aura.h b/libcef/browser/native/browser_platform_delegate_native_aura.h index af6074ac9..1c7eb8bd5 100644 --- a/libcef/browser/native/browser_platform_delegate_native_aura.h +++ b/libcef/browser/native/browser_platform_delegate_native_aura.h @@ -7,6 +7,7 @@ #include +#include "base/functional/callback.h" #include "base/memory/raw_ptr.h" #include "base/memory/weak_ptr.h" #include "cef/libcef/browser/native/browser_platform_delegate_native.h" @@ -79,9 +80,16 @@ class CefBrowserPlatformDelegateNativeAura return std::nullopt; } - protected: + base::WeakPtr GetWeakPtr() { + return weak_ptr_factory_.GetWeakPtr(); + } + + // Returns a callback that clears |window_widget_| state. Safe to execute in + // any order relative to destruction of this object; it will be a no-op if + // this object is destroyed first. base::OnceClosure GetWidgetDeleteCallback(); + protected: std::optional RootWindowBoundsCallback(); static base::TimeTicks GetEventTimeStamp(); diff --git a/tests/ceftests/chrome_child_window_unittest.cc b/tests/ceftests/chrome_child_window_unittest.cc new file mode 100644 index 000000000..3ff5df54c --- /dev/null +++ b/tests/ceftests/chrome_child_window_unittest.cc @@ -0,0 +1,323 @@ +// Copyright 2026 The Chromium Embedded Framework Authors. All rights +// reserved. Use of this source code is governed by a BSD-style license that +// can be found in the LICENSE file. + +#include "include/base/cef_build.h" + +// Chrome style browsers created with a native parent window +// (CefWindowInfo::SetAsChild) are implemented via ChildWindowDelegate in +// libcef/browser/chrome/views/chrome_child_window.cc. Not supported on MacOS. +// On Linux a native parent window can only be created when running with X11. +#if defined(OS_WIN) || (defined(OS_LINUX) && defined(CEF_X11)) + +// These tests verify teardown of the browser and the native parent window in +// both orders: +// - CloseBrowserFirst: CloseBrowser() destroys the browser (and its platform +// delegate) before the Widget/window teardown completes. The internal +// window delegate must not retain a dangling reference to the platform +// delegate. +// - DestroyParentFirst (Windows only): the client destroys the native parent +// window (and thereby the browser's Widget) while the browser is still +// alive. Browser host methods that use cached Widget state must safely +// no-op afterwards. +// +// Dangling raw_ptr regressions in these scenarios are detected when running +// with --enable-features=PartitionAllocBackupRefPtr,PartitionAllocDanglingPtr +// or in an ASAN build. + +#include "include/base/cef_callback.h" +#include "include/cef_browser.h" +#include "include/wrapper/cef_closure_task.h" +#include "tests/ceftests/test_handler.h" +#include "tests/ceftests/thread_helper.h" +#include "tests/ceftests/track_callback.h" +#include "tests/gtest/include/gtest/gtest.h" + +#if defined(OS_WIN) +#include +#endif + +#if defined(OS_LINUX) +#include +#endif + +namespace { + +const char kTestUrl[] = "https://tests.test/chrome-child-window.html"; +const char kTestContent[] = + "Chrome Child Window Test"; + +constexpr int kParentWidth = 800; +constexpr int kParentHeight = 600; + +// Number of UI thread task round trips used to allow previously posted +// teardown tasks (browser destruction, Widget destruction) to complete +// before continuing. +constexpr size_t kTeardownRoundTrips = 3; + +class ChromeChildWindowTestHandler : public TestHandler { + public: + enum class TeardownOrder { + // Close the browser first. The native parent window is destroyed after + // the browser and Widget teardown completes. + kCloseBrowserFirst, + // Destroy the native parent window (and thereby the browser's Widget) + // first, while the browser is still alive. + kDestroyParentFirst, + }; + + explicit ChromeChildWindowTestHandler(TeardownOrder order) : order_(order) {} + + ChromeChildWindowTestHandler(const ChromeChildWindowTestHandler&) = delete; + ChromeChildWindowTestHandler& operator=(const ChromeChildWindowTestHandler&) = + delete; + + void RunTest() override { + AddResource(kTestUrl, kTestContent, "text/html"); + + // Browser lifetime is managed by this test (see OnAfterCreated), so test + // completion must be signaled explicitly. + SetSignalTestCompletionCount(1); + + CefPostTask(TID_UI, + base::BindOnce( + &ChromeChildWindowTestHandler::CreateChildBrowser, this)); + SetTestTimeout(); + } + + // The browser is Views-hosted internally (the CefWindow is created by + // ChildWindowDelegate), but the window is not test-managed. Skip the + // TestHandler implementations, which expect a TestWindowDelegate-managed + // window for Views-hosted browsers, and track the browser directly. + void OnAfterCreated(CefRefPtr browser) override { + EXPECT_UI_THREAD(); + EXPECT_FALSE(browser_); + browser_ = browser; + } + + void OnBeforeClose(CefRefPtr browser) override { + EXPECT_UI_THREAD(); + EXPECT_TRUE(browser_); + EXPECT_TRUE(browser->IsSame(browser_)); + browser_ = nullptr; + got_before_close_.yes(); + + // Allow the posted browser and Widget teardown tasks to complete before + // finishing the test. + RunAfterUITaskRoundTrips( + kTeardownRoundTrips, + base::BindOnce(&ChromeChildWindowTestHandler::FinishTest, this)); + } + + void OnLoadEnd(CefRefPtr browser, + CefRefPtr frame, + int httpStatusCode) override { + EXPECT_UI_THREAD(); + if (!frame->IsMain()) { + return; + } + EXPECT_FALSE(got_load_end_); + got_load_end_.yes(); + EXPECT_EQ(200, httpStatusCode); + + if (order_ == TeardownOrder::kCloseBrowserFirst) { + // Destroys the browser (and its platform delegate) first. The Widget + // and window teardown will follow. + DestroyTest(); + } else { + CefPostTask(TID_UI, + base::BindOnce( + &ChromeChildWindowTestHandler::DestroyParentFirst, this)); + } + } + + void DestroyTest() override { + EXPECT_UI_THREAD(); + + if (browser_) { + CloseBrowser(browser_, /*force_close=*/true); + } + + TestHandler::DestroyTest(); + } + + void OnTestTimeout(int timeout_ms, bool treat_as_error) override { + timed_out_ = true; + TestHandler::OnTestTimeout(timeout_ms, treat_as_error); + } + + private: + void CreateChildBrowser() { + EXPECT_UI_THREAD(); + + parent_window_ = CreateParentWindow(); + if (parent_window_ == kNullWindowHandle) { + ADD_FAILURE() << "Failed to create native parent window"; + DestroyTest(); + SignalTestCompletion(); + return; + } + + CefWindowInfo window_info; + window_info.SetAsChild(parent_window_, + CefRect(0, 0, kParentWidth, kParentHeight)); + + // ChildWindowDelegate is only used with Chrome style browsers. + window_info.runtime_style = CEF_RUNTIME_STYLE_CHROME; + + CefBrowserSettings settings; + CefBrowserHost::CreateBrowser(window_info, this, kTestUrl, settings, + /*extra_info=*/nullptr, + /*request_context=*/nullptr); + } + + // kDestroyParentFirst only: destroy the native parent window while the + // browser is still alive. This also destroys the browser's Widget. + void DestroyParentFirst() { + EXPECT_UI_THREAD(); + EXPECT_TRUE(browser_); + + DestroyParentWindow(); + + // Allow the Widget teardown to complete, then exercise the browser before + // closing it. + RunAfterUITaskRoundTrips( + kTeardownRoundTrips, + base::BindOnce(&ChromeChildWindowTestHandler::PokeBrowserAndClose, + this)); + } + + // kDestroyParentFirst only: called after the Widget has been destroyed. + void PokeBrowserAndClose() { + EXPECT_UI_THREAD(); + + // The browser may have started closing as a result of the Widget + // destruction. + if (browser_) { + // These calls use Widget state that was cached by the browser's + // platform delegate and must safely no-op after the Widget has been + // destroyed. + browser_->GetHost()->SetFocus(true); + browser_->GetHost()->NotifyMoveOrResizeStarted(); + } + + // Close the browser (if still open) and complete the test. + DestroyTest(); + } + + // Last step for both teardown orders; called after the browser has closed + // and teardown tasks have completed. + void FinishTest() { + EXPECT_UI_THREAD(); + EXPECT_TRUE(got_load_end_); + EXPECT_TRUE(got_before_close_); + + DestroyParentWindow(); + + if (!timed_out_) { + SignalTestCompletion(); + } + } + + // Executes |callback| on the UI thread after |count| task round trips. + void RunAfterUITaskRoundTrips(size_t count, base::OnceClosure callback) { + EXPECT_UI_THREAD(); + if (count == 0) { + std::move(callback).Run(); + return; + } + CefPostTask( + TID_UI, + base::BindOnce(&ChromeChildWindowTestHandler::RunAfterUITaskRoundTrips, + this, count - 1, std::move(callback))); + } + + CefWindowHandle CreateParentWindow() { + EXPECT_UI_THREAD(); +#if defined(OS_WIN) + const wchar_t kClassName[] = L"ceftests_ChildWindowParent"; + const HINSTANCE instance = GetModuleHandle(nullptr); + + WNDCLASSEXW wc = {}; + wc.cbSize = sizeof(wc); + wc.lpfnWndProc = DefWindowProcW; + wc.hInstance = instance; + wc.lpszClassName = kClassName; + // May fail if the class is already registered; ignored. + RegisterClassExW(&wc); + + return CreateWindowExW(0, kClassName, L"ChromeChildWindowTest", + WS_OVERLAPPEDWINDOW | WS_CLIPCHILDREN | WS_VISIBLE, + CW_USEDEFAULT, CW_USEDEFAULT, kParentWidth, + kParentHeight, nullptr, nullptr, instance, nullptr); +#elif defined(OS_LINUX) + xdisplay_ = XOpenDisplay(nullptr); + if (!xdisplay_) { + return kNullWindowHandle; + } + const ::Window window = XCreateSimpleWindow( + xdisplay_, DefaultRootWindow(xdisplay_), 0, 0, kParentWidth, + kParentHeight, /*border_width=*/0, /*border=*/0, /*background=*/0); + XMapWindow(xdisplay_, window); + XFlush(xdisplay_); + return static_cast(window); +#endif + } + + // Safe to call multiple times. + void DestroyParentWindow() { + EXPECT_UI_THREAD(); + if (parent_window_ == kNullWindowHandle) { + return; + } +#if defined(OS_WIN) + ::DestroyWindow(parent_window_); +#elif defined(OS_LINUX) + XDestroyWindow(xdisplay_, parent_window_); + XFlush(xdisplay_); + XCloseDisplay(xdisplay_); + xdisplay_ = nullptr; +#endif + parent_window_ = kNullWindowHandle; + } + + const TeardownOrder order_; + + CefRefPtr browser_; + CefWindowHandle parent_window_ = kNullWindowHandle; +#if defined(OS_LINUX) + ::Display* xdisplay_ = nullptr; +#endif + bool timed_out_ = false; + + TrackCallback got_load_end_; + TrackCallback got_before_close_; + + IMPLEMENT_REFCOUNTING(ChromeChildWindowTestHandler); +}; + +} // namespace + +// Close the browser first; the platform delegate is destroyed before the +// Widget/window teardown completes. +TEST(ChromeChildWindowTest, CloseBrowserFirst) { + CefRefPtr handler = + new ChromeChildWindowTestHandler( + ChromeChildWindowTestHandler::TeardownOrder::kCloseBrowserFirst); + handler->ExecuteTest(); + ReleaseAndWaitForDestructor(handler); +} + +#if defined(OS_WIN) +// Destroy the native parent window (and thereby the browser's Widget) while +// the browser is still alive. +TEST(ChromeChildWindowTest, DestroyParentFirst) { + CefRefPtr handler = + new ChromeChildWindowTestHandler( + ChromeChildWindowTestHandler::TeardownOrder::kDestroyParentFirst); + handler->ExecuteTest(); + ReleaseAndWaitForDestructor(handler); +} +#endif // defined(OS_WIN) + +#endif // defined(OS_WIN) || (defined(OS_LINUX) && defined(CEF_X11))