diff --git a/chrome/browser/apps/platform_apps/app_window_browsertest.cc b/chrome/browser/apps/platform_apps/app_window_browsertest.cc index 1393919680bb2..7ae68422b7f8c 100644 --- a/chrome/browser/apps/platform_apps/app_window_browsertest.cc +++ b/chrome/browser/apps/platform_apps/app_window_browsertest.cc @@ -10,6 +10,7 @@ #include "chrome/browser/apps/platform_apps/app_browsertest_util.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/views/apps/chrome_native_app_window_views.h" #include "components/services/app_service/public/cpp/app_launch_params.h" #include "components/services/app_service/public/cpp/app_launch_util.h" #include "content/public/test/browser_test.h" @@ -273,3 +274,87 @@ IN_PROC_BROWSER_TEST_F(AppWindowAPITest, TestVisibleOnAllWorkspaces) { RunAppWindowAPITestAndWaitForRoundTrip("testVisibleOnAllWorkspaces")) << message_; } + +// Regression test for a browser-process use-after-free in +// extensions::AppWindow::SetNativeWindowFullscreen(): +// +// void AppWindow::SetNativeWindowFullscreen() { +// native_app_window_->SetFullscreen(fullscreen_types_); // [1] +// RestoreAlwaysOnTop(); // [2] +// } +// +// On Windows/macOS, [1] reaches Widget::SetFullscreen() -> +// HWNDMessageHandler::SetFullscreen() -> ::SetWindowPos(SWP_FRAMECHANGED) (or +// [NSWindow toggleFullScreen:] on Mac), which can spin a nested UI-thread +// message loop. Chromium has hardened every other layer of this call stack +// with WeakPtr guards (Widget::SetFullscreen, +// DesktopWindowTreeHostWin::SetFullscreen, HWNDMessageHandler::SetFullscreen, +// FullscreenHandler::ProcessFullscreen, WebContentsImpl::EnterFullscreenMode, +// RenderFrameHostImpl::EnterFullscreen) for crbug.com/1506535 / +// crbug.com/498752242 -- but extensions::AppWindow was missed. +// +// If a renderer-queued window.close() (-> AppWindow::CloseContents -> +// NativeAppWindowViews::Close -> Widget::Close -> posted CloseNow) is +// processed inside that nested loop, Widget::HandleWidgetDestroyed() runs the +// NativeAppWindowViews DeleteDelegate callback, which calls +// AppWindow::OnNativeClose() -> `delete this`. Control then unwinds through +// the WeakPtr-guarded layers back to [2], which dereferences the freed +// AppWindow: +// +// void AppWindow::RestoreAlwaysOnTop() { +// if (cached_always_on_top_) // read of freed bool member +// UpdateNativeAlwaysOnTop(); // virtual calls through freed +// // unique_ptr +// } +// +// This test simulates the platform-specific nested-loop destruction via +// ChromeNativeAppWindowViews::g_simulate_destroy_in_set_fullscreen_for_testing +// (since the Linux/X11 SetFullscreen path does not pump UI-thread tasks) and +// verifies, under ASan, that AppWindow does not touch `this` after +// native_app_window_->SetFullscreen() returns. +// +// A Chrome App renderer can drive the entire sequence via standard APIs: +// chrome.app.window.current().setAlwaysOnTop(true); +// setTimeout(() => window.close(), 0); +// chrome.app.window.current().fullscreen(); +using AppWindowFullscreenUAFTest = extensions::PlatformAppBrowserTest; + +IN_PROC_BROWSER_TEST_F(AppWindowFullscreenUAFTest, + SetNativeWindowFullscreenUseAfterFree) { + // 1. Launch a platform-app window (renderer-reachable surface). + LoadAndLaunchPlatformApp("minimal", "Launched"); + extensions::AppWindow* app_window = GetFirstAppWindow(); + ASSERT_TRUE(app_window); + + // 2. Arm cached_always_on_top_ so the freed-`this` read in + // RestoreAlwaysOnTop() falls through to UpdateNativeAlwaysOnTop()'s + // virtual dispatches on native_app_window_. Renderer equivalent: + // chrome.app.window.current().setAlwaysOnTop(true). + app_window->SetAlwaysOnTop(true); + + // 3. Simulate the Windows/macOS nested-message-loop destruction inside + // NativeAppWindow::SetFullscreen(). + ChromeNativeAppWindowViews:: + g_simulate_destroy_in_set_fullscreen_for_testing = true; + + // 4. Renderer equivalent: chrome.app.window.current().fullscreen() + // -> AppCurrentWindowInternalFullscreenFunction::Run + // -> AppWindow::Fullscreen() + // -> AppWindow::SetFullscreen(FULLSCREEN_TYPE_WINDOW_API, true) + // -> AppWindow::SetNativeWindowFullscreen() + // native_app_window_->SetFullscreen(...) + // -> (simulated nested loop) widget()->CloseNow() + // -> Widget::HandleWidgetDestroyed + // -> WidgetDelegate::DeleteDelegate + // -> NativeAppWindowViews delete-callback + // -> AppWindow::OnNativeClose() -> `delete this` + // RestoreAlwaysOnTop() <-- heap-use-after-free on `this` + // + // Under ASan this aborts with heap-use-after-free in + // extensions::AppWindow::RestoreAlwaysOnTop / + // extensions::AppWindow::UpdateNativeAlwaysOnTop. + app_window->Fullscreen(); + + // If the bug is fixed (WeakPtr guard added in SetNativeWindowFullscreen), + // execution reaches here cleanly with the window destroyed. +} diff --git a/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc b/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc index d0548e99c7a90..64e5d9e55b60a 100644 --- a/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc +++ b/chrome/browser/ui/views/apps/chrome_native_app_window_views.cc @@ -361,7 +361,27 @@ bool ChromeNativeAppWindowViews::AcceleratorPressed( // NativeAppWindow implementation. +// static +bool ChromeNativeAppWindowViews:: + g_simulate_destroy_in_set_fullscreen_for_testing = false; + void ChromeNativeAppWindowViews::SetFullscreen(int fullscreen_types) { + if (g_simulate_destroy_in_set_fullscreen_for_testing) { + g_simulate_destroy_in_set_fullscreen_for_testing = false; + // On Windows/macOS, widget()->SetFullscreen() may spin a nested message + // loop (SetWindowPos / [NSWindow toggleFullScreen:]) that processes a + // queued CloseNow() task. Widget::SetFullscreen, DesktopWindowTreeHostWin + // ::SetFullscreen, HWNDMessageHandler::SetFullscreen and FullscreenHandler + // ::ProcessFullscreen all carry WeakPtr guards for exactly this case. + // When that happens, by the time this function returns, the Widget, this + // NativeAppWindowViews, and the owning extensions::AppWindow have all been + // freed. Simulate that here for test coverage on platforms whose + // fullscreen transition does not pump tasks. + widget()->CloseNow(); + // `this` is freed; do not touch any members. Return into + // AppWindow::SetNativeWindowFullscreen() — which has no WeakPtr guard. + return; + } widget()->SetFullscreen(fullscreen_types != AppWindow::FULLSCREEN_TYPE_NONE); } diff --git a/chrome/browser/ui/views/apps/chrome_native_app_window_views.h b/chrome/browser/ui/views/apps/chrome_native_app_window_views.h index fea7f2b2d3507..7c8d78dd76705 100644 --- a/chrome/browser/ui/views/apps/chrome_native_app_window_views.h +++ b/chrome/browser/ui/views/apps/chrome_native_app_window_views.h @@ -25,6 +25,16 @@ class ChromeNativeAppWindowViews ~ChromeNativeAppWindowViews() override; + // Test-only: when true, SetFullscreen() will synchronously CloseNow() the + // widget instead of (or before returning from) the platform fullscreen + // transition. This simulates the Windows/macOS behavior documented at + // crbug.com/1506535 / crbug.com/498752242 where the platform fullscreen + // call (SetWindowPos / toggleFullScreen) spins a nested message loop that + // can process a queued window-close, destroying the Widget — and via the + // DeleteDelegate callback, the owning extensions::AppWindow — before + // SetFullscreen() returns. + static bool g_simulate_destroy_in_set_fullscreen_for_testing; + SkRegion* shape() { return shape_.get(); } ShapeRects* shape_rects() { return shape_rects_.get(); }