From fc4613ff876d53687b1b248723cca7b962534fe7 Mon Sep 17 00:00:00 2001
From: Tom Anderson <[email protected]>
Date: Tue, 21 Jul 2026 11:10:49 -0700
Subject: [PATCH] [Ozone/X11] Revert workaround UAF guards for GeometryCache synchronous dispatch

Now that GeometryCache::GetBoundsPx() peeks at X11 server responses
instead of calling DispatchNow(), event processing and bounds-changed
callbacks no longer execute synchronously during bounds calculation.

This change reverts the temporary WeakPtr liveness guards and unit tests
that were added to handle re-entrancy during GetBoundsPx() /
GetBoundsInPixels():
- Revert "Harden X11DesktopWindowMoveClient against synchronous destruction"
- Revert "Fix use-after-free in DesktopWindowTreeHostPlatform::CenterWindow"
- Revert "ozone/x11: Fix potential UAF in X11Window::DispatchEvent"
- Revert "Ozone: Fix UAF in X11Window::DispatchUiEvent via weak_ptr tracking"
- Revert "Fix UAF in X11Window::SetOverrideRedirect and BrowserDesktopWindowTreeHostLinux"
- Revert "[Ozone/X11] Guard against synchronous deletion in OnWindowMapped"
- Revert "ozone/x11: Fix UAF inside X11Window::GetBoundsInPixels()"

Bug: 524439798, 524824288, 532966375, 524552123, 524395469, 523690961, 524825209, 518245882, 517050585, 518043597
Change-Id: I8a3a311e8ea209e16e06d1179a1fb00b915c00fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8129120
Reviewed-by: Lei Zhang <[email protected]>
Commit-Queue: Thomas Anderson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1665631}
---

diff --git a/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.cc b/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.cc
index bc29ed10..4480eb3 100644
--- a/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.cc
+++ b/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.cc
@@ -138,11 +138,7 @@
         browser_widget_->tab_drag_kind() == TabDragKind::kAllTabs;
     bool is_dragging_window = tab_drag_kind == TabDragKind::kAllTabs;
     if (is_dragging_window != was_dragging_window) {
-      auto weak_this = weak_factory_.GetWeakPtr();
       x11_extension->SetOverrideRedirect(is_dragging_window);
-      if (!weak_this) {
-        return;
-      }
     }
   }
 
diff --git a/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.h b/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.h
index 97b931d..b4a7275b 100644
--- a/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.h
+++ b/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_linux.h
@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_DESKTOP_WINDOW_TREE_HOST_LINUX_H_
 
 #include "base/memory/raw_ptr.h"
-#include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
 #include "build/config/linux/dbus/buildflags.h"
 #include "chrome/browser/ui/views/frame/browser_desktop_window_tree_host.h"
@@ -110,8 +109,6 @@
 
   base::ScopedObservation<ui::LinuxUi, ui::DeviceScaleFactorObserver>
       scale_observation_{this};
-
-  base::WeakPtrFactory<BrowserDesktopWindowTreeHostLinux> weak_factory_{this};
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_FRAME_BROWSER_DESKTOP_WINDOW_TREE_HOST_LINUX_H_
diff --git a/ui/base/x/BUILD.gn b/ui/base/x/BUILD.gn
index dac89b38..eefb196a 100644
--- a/ui/base/x/BUILD.gn
+++ b/ui/base/x/BUILD.gn
@@ -165,7 +165,6 @@
     "selection_utils_unittest.cc",
     "x11_cursor_factory_unittest.cc",
     "x11_cursor_loader_unittest.cc",
-    "x11_desktop_window_move_client_unittest.cc",
     "x11_display_util_unittest.cc",
     "x11_whole_screen_move_loop_unittest.cc",
   ]
diff --git a/ui/base/x/x11_desktop_window_move_client.cc b/ui/base/x/x11_desktop_window_move_client.cc
index 21f93a1..e17db07 100644
--- a/ui/base/x/x11_desktop_window_move_client.cc
+++ b/ui/base/x/x11_desktop_window_move_client.cc
@@ -4,6 +4,7 @@
 
 #include "ui/base/x/x11_desktop_window_move_client.h"
 
+#include "base/check.h"
 #include "base/functional/callback_helpers.h"
 #include "ui/base/x/x11_util.h"
 #include "ui/events/event.h"
@@ -14,23 +15,17 @@
 X11DesktopWindowMoveClient::Delegate::~Delegate() = default;
 
 X11DesktopWindowMoveClient::X11DesktopWindowMoveClient(Delegate* window)
-    : window_(window ? window->AsWeakPtr() : nullptr) {}
+    : window_(window) {
+  CHECK(window_);
+}
 
 X11DesktopWindowMoveClient::~X11DesktopWindowMoveClient() = default;
 
 void X11DesktopWindowMoveClient::OnMouseMovement(const gfx::Point& screen_point,
                                                  int flags,
                                                  base::TimeTicks event_time) {
-  if (!window_) {
-    return;
-  }
   gfx::Point system_loc = screen_point - window_offset_;
-  base::WeakPtr<X11DesktopWindowMoveClient> alive(weak_factory_.GetWeakPtr());
-  const gfx::Size size = window_->GetSize();
-  if (!alive || !window_) {
-    return;
-  }
-  window_->SetBoundsOnMove(gfx::Rect(system_loc, size));
+  window_->SetBoundsOnMove(gfx::Rect(system_loc, window_->GetSize()));
 }
 
 void X11DesktopWindowMoveClient::OnMouseReleased() {
diff --git a/ui/base/x/x11_desktop_window_move_client.h b/ui/base/x/x11_desktop_window_move_client.h
index 8b50668..f95cf2c 100644
--- a/ui/base/x/x11_desktop_window_move_client.h
+++ b/ui/base/x/x11_desktop_window_move_client.h
@@ -6,7 +6,7 @@
 #define UI_BASE_X_X11_DESKTOP_WINDOW_MOVE_CLIENT_H_
 
 #include "base/component_export.h"
-#include "base/memory/weak_ptr.h"
+#include "base/memory/raw_ptr.h"
 #include "ui/base/x/x11_move_loop_delegate.h"
 #include "ui/base/x/x11_whole_screen_move_loop.h"
 #include "ui/gfx/geometry/point.h"
@@ -30,8 +30,6 @@
     virtual scoped_refptr<X11Cursor> GetLastCursor() = 0;
     // Returns the size part of the window bounds.
     virtual gfx::Size GetSize() = 0;
-    // Returns a weak pointer to the delegate.
-    virtual base::WeakPtr<Delegate> AsWeakPtr() = 0;
 
    protected:
     virtual ~Delegate();
@@ -55,14 +53,12 @@
 
   // We need to keep track of this so we can actually move it when reacting to
   // mouse events.
-  base::WeakPtr<Delegate> window_;
+  const raw_ptr<Delegate> window_;
 
   // Our cursor offset from the top left window origin when the drag
   // started. Used to calculate the window's new bounds relative to the current
   // location of the cursor.
   gfx::Vector2d window_offset_;
-
-  base::WeakPtrFactory<X11DesktopWindowMoveClient> weak_factory_{this};
 };
 
 }  // namespace ui
diff --git a/ui/base/x/x11_desktop_window_move_client_unittest.cc b/ui/base/x/x11_desktop_window_move_client_unittest.cc
deleted file mode 100644
index c4c5a11..0000000
--- a/ui/base/x/x11_desktop_window_move_client_unittest.cc
+++ /dev/null
@@ -1,97 +0,0 @@
-// Copyright 2026 The Chromium Authors
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "ui/base/x/x11_desktop_window_move_client.h"
-
-#include <memory>
-
-#include "base/memory/raw_ptr.h"
-#include "base/memory/weak_ptr.h"
-#include "base/time/time.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "ui/gfx/geometry/point.h"
-#include "ui/gfx/geometry/rect.h"
-#include "ui/gfx/geometry/size.h"
-
-namespace ui {
-
-namespace {
-
-class TestDelegate : public X11DesktopWindowMoveClient::Delegate {
- public:
-  TestDelegate() = default;
-  ~TestDelegate() override = default;
-
-  void SetBoundsOnMove(const gfx::Rect& requested_bounds) override {
-    set_bounds_on_move_called_ = true;
-  }
-
-  scoped_refptr<X11Cursor> GetLastCursor() override {
-    return nullptr;
-  }
-
-  gfx::Size GetSize() override {
-    get_size_called_ = true;
-    if (destroy_on_get_size_) {
-      // Synchronously destroy the client (which models the destruction flow in
-      // production when X11Window is destroyed and in turn deletes its move client).
-      move_client_holder_->reset();

diff --git a/ui/base/x/x11_desktop_window_move_client_unittest.cc b/ui/base/x/x11_desktop_window_move_client_unittest.cc
deleted file mode 100644
index c4c5a11..0000000
--- a/ui/base/x/x11_desktop_window_move_client_unittest.cc
+++ /dev/null
@@ -1,97 +0,0 @@
-// Copyright 2026 The Chromium Authors
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "ui/base/x/x11_desktop_window_move_client.h"
-
-#include <memory>
-
-#include "base/memory/raw_ptr.h"
-#include "base/memory/weak_ptr.h"
-#include "base/time/time.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "ui/gfx/geometry/point.h"
-#include "ui/gfx/geometry/rect.h"
-#include "ui/gfx/geometry/size.h"
-
-namespace ui {
-
-namespace {
-
-class TestDelegate : public X11DesktopWindowMoveClient::Delegate {
- public:
-  TestDelegate() = default;
-  ~TestDelegate() override = default;
-
-  void SetBoundsOnMove(const gfx::Rect& requested_bounds) override {
-    set_bounds_on_move_called_ = true;
-  }
-
-  scoped_refptr<X11Cursor> GetLastCursor() override {
-    return nullptr;
-  }
-
-  gfx::Size GetSize() override {
-    get_size_called_ = true;
-    if (destroy_on_get_size_) {
-      // Synchronously destroy the client (which models the destruction flow in
-      // production when X11Window is destroyed and in turn deletes its move client).
-      move_client_holder_->reset();
-    }
-    return gfx::Size(100, 100);
-  }
-
-  base::WeakPtr<X11DesktopWindowMoveClient::Delegate> AsWeakPtr() override {
-    return weak_ptr_factory_.GetWeakPtr();
-  }
-
-  void Init(std::unique_ptr<X11DesktopWindowMoveClient>* holder,
-            bool destroy_on_get_size) {
-    move_client_holder_ = holder;
-    destroy_on_get_size_ = destroy_on_get_size;
-  }
-
-  bool get_size_called() const { return get_size_called_; }
-  bool set_bounds_on_move_called() const { return set_bounds_on_move_called_; }
-
- private:
-  raw_ptr<std::unique_ptr<X11DesktopWindowMoveClient>> move_client_holder_ = nullptr;
-  bool destroy_on_get_size_ = false;
-  bool get_size_called_ = false;
-  bool set_bounds_on_move_called_ = false;
-
-  base::WeakPtrFactory<TestDelegate> weak_ptr_factory_{this};
-};
-
-}  // namespace
-
-TEST(X11DesktopWindowMoveClientTest, SafeOnMouseMovementWithDestruction) {
-  TestDelegate delegate;
-  auto move_client = std::make_unique<X11DesktopWindowMoveClient>(&delegate);
-  delegate.Init(&move_client, /*destroy_on_get_size=*/true);
-
-  // Trigger OnMouseMovement. Inside GetSize(), the delegate will destroy
-  // `move_client` synchronously.
-  move_client->OnMouseMovement(gfx::Point(10, 10), 0, base::TimeTicks());
-
-  // Confirm that the move client was indeed destroyed.
-  EXPECT_FALSE(move_client);
-  EXPECT_TRUE(delegate.get_size_called());
-  // SetBoundsOnMove should NOT have been called, as the delegate and the client
-  // are destroyed/invalidated.
-  EXPECT_FALSE(delegate.set_bounds_on_move_called());
-}
-
-TEST(X11DesktopWindowMoveClientTest, OnMouseMovementWithoutDestruction) {
-  TestDelegate delegate;
-  auto move_client = std::make_unique<X11DesktopWindowMoveClient>(&delegate);
-  delegate.Init(&move_client, /*destroy_on_get_size=*/false);
-
-  move_client->OnMouseMovement(gfx::Point(10, 10), 0, base::TimeTicks());
-
-  EXPECT_TRUE(move_client);
-  EXPECT_TRUE(delegate.get_size_called());
-  EXPECT_TRUE(delegate.set_bounds_on_move_called());
-}
-
-}  // namespace ui
diff --git a/ui/ozone/platform/x11/x11_window_ozone_unittest.cc b/ui/ozone/platform/x11/x11_window_ozone_unittest.cc
index 2a5b43ef..76b1ab5 100644
--- a/ui/ozone/platform/x11/x11_window_ozone_unittest.cc
+++ b/ui/ozone/platform/x11/x11_window_ozone_unittest.cc
@@ -286,90 +286,6 @@
   gfx::Rect guessed_size_px_;
 };
 
-// Verifies that SetBoundsInPixels() is safe against the X11Window being
-// synchronously destroyed during the GetBoundsInPixels() call. This can happen
-// if GeometryCache::GetBoundsPx() processes synchronous X server replies and
-// dispatches an OnBoundsChanged event that closes the widget (e.g., as in
-// crbug.com/1068755).
-TEST_F(X11WindowOzoneTest, SetBoundsInPixelsUseAfterFreeViaGeometryCache) {
-  testing::NiceMock<MockPlatformWindowDelegate> delegate;
-  gfx::AcceleratedWidget widget;
-  constexpr gfx::Rect bounds(30, 80, 800, 600);
-  std::unique_ptr<PlatformWindow> window =
-      CreatePlatformWindow(&delegate, bounds, &widget, nullptr);
-
-  auto* connection = x11::Connection::Get();
-  auto xwindow = static_cast<x11::Window>(widget);
-
-  // Step 1: Force the X11Window's GeometryCache (and its parent chain) to
-  // become Ready and record last_notified_geometry_. This goes through the
-  // synchronous DispatchNow() path. The resulting X11Window::OnBoundsChanged
-  // sees a size change and only posts a delayed-resize task, so the delegate
-  // is not called synchronously here.
-  window->GetBoundsInPixels();
-
-  // Step 2: Create a real parent window at a non-zero offset so that after
-  // reparenting, only the absolute origin of |xwindow| changes (size stays
-  // 800x600). override_redirect avoids any WM interference.
-  x11::Window new_parent = connection->GenerateId<x11::Window>();
-  connection->CreateWindow({
-      .wid = new_parent,
-      .parent = connection->default_root(),
-      .x = 200,
-      .y = 200,
-      .width = 1000,
-      .height = 1000,
-      .c_class = x11::WindowClass::InputOnly,
-      .override_redirect = x11::Bool32(true),
-  });
-
-  // Step 3: Synthesize the ReparentNotify the X server would send for a WM
-  // reparent. GeometryCache::OnEvent replaces parent_ with a fresh un-Ready
-  // GeometryCache for |new_parent|, leaving the leaf cache's chain not Ready.
-  x11::ReparentNotifyEvent reparent{};
-  reparent.event = xwindow;
-  reparent.window = xwindow;
-  reparent.parent = new_parent;
-  reparent.x = 0;
-  reparent.y = 0;
-  x11::Event reparent_event(/*send_event=*/false, std::move(reparent));
-  connection->DispatchEvent(reparent_event);
-
-  // Step 4: Arm the delegate so that the *next* synchronous OnBoundsChanged
-  // (which will fire from inside SetBoundsInPixels → GetBoundsInPixels →
-  // GetBoundsPx → DispatchNow → OnBoundsChanged → NotifyBoundsChanged) frees
-  // the X11Window — modelling Widget::CloseNow → SetPlatformWindow(nullptr).
-  bool armed = true;
-  bool freed = false;
-  EXPECT_CALL(delegate, OnBoundsChanged(_))
-      .WillRepeatedly([&](const PlatformWindowDelegate::BoundsChange&) {
-        if (armed) {
-          armed = false;
-          freed = true;
-          // ~X11Window → PrepareForShutdown → Close → CloseXWindow →
-          // geometry_cache_.reset(). All GeometryCache weak_ptrs are
-          // invalidated, so every GetBoundsPx() frame on the stack will
-          // return {}, and SetBoundsInPixels() should guard against this
-          // deletion.
-          window.reset();
-        }
-      });
-
-  // Step 5: Call SetBoundsInPixels(). Inside, GetBoundsInPixels() recurses
-  // into the un-Ready parent cache, which processes replies synchronously via
-  // DispatchNow(). The resulting OnBoundsChanged event triggers the observer,
-  // which destroys the window. X11Window must safely return early instead
-  // of accessing freed members or the destroyed delegate.
-  PlatformWindow* raw = window.get();
-  raw->SetBoundsInPixels(gfx::Rect(40, 90, 800, 600));
-
-  // If we got here without ASAN tripping, the synchronous-free path was not
-  // exercised; surface that as a test failure rather than a silent pass.
-  EXPECT_TRUE(freed) << "delegate was never invoked synchronously";
-
-  connection->DestroyWindow({new_parent});
-}
-
 // Verifies X11Window sets fullscreen bounds in pixels when going to fullscreen.
 TEST_F(X11WindowOzoneTest, SetFullscreen) {
   constexpr gfx::Rect screen_bounds_in_px(640, 480, 1280, 720);
@@ -388,339 +304,6 @@
   window->SetFullscreen(true, display::kInvalidDisplayId);
 }
 
-// Verifies that X11Window::OnWindowMapped() does not cause a use-after-free
-// if the window is synchronously deleted during Maximize().
-//
-// The MapNotify dispatch path (Connection::DispatchEvent ->
-// EventObserver::OnEvent -> HandleEvent -> OnWindowMapped) is not covered by
-// the weak_this guard in DispatchUiEvent because MapNotify is not a
-// translatable event.
-//
-// This test sets `should_maximize_after_map_` to true, and configures the
-// delegate to synchronously delete the window upon receiving the bounds change
-// from Maximize(). It then dispatches a synthetic MapNotify event and ensures
-// the deletion is handled safely without a use-after-free.
-TEST_F(X11WindowOzoneTest, OnWindowMappedUseAfterFreeAfterMaximize) {
-  testing::NiceMock<MockPlatformWindowDelegate> delegate;
-  gfx::AcceleratedWidget widget;
-  constexpr gfx::Rect bounds(30, 80, 800, 600);
-  std::unique_ptr<PlatformWindow> window =
-      CreatePlatformWindow(&delegate, bounds, &widget, nullptr);
-
-  auto* connection = x11::Connection::Get();
-  auto xwindow = static_cast<x11::Window>(widget);
-
-  // Prime the X11Window's `GeometryCache` so the first `Maximize()` below does
-  // not itself fire a synchronous `OnBoundsChanged()`.
-  window->GetBoundsInPixels();
-
-  // Step 1: Maximize() while `!window_mapped_in_client_` -> sets
-  // `should_maximize_after_map_` = true. (The window has never been shown.)
-  window->Maximize();
-
-  // Step 2: Create a real parent window at a non-zero offset so that after
-  // a synthetic reparent, only the absolute origin of `xwindow` changes.
-  // `override_redirect` avoids any WM interference.
-  x11::Window new_parent = connection->GenerateId<x11::Window>();
-  connection->CreateWindow({
-      .wid = new_parent,
-      .parent = connection->default_root(),
-      .x = 200,
-      .y = 200,
-      .width = 1000,
-      .height = 1000,
-      .c_class = x11::WindowClass::InputOnly,
-      .override_redirect = x11::Bool32(true),
-  });
-
-  // Step 3: Synthesize a `ReparentNotify` so the leaf `GeometryCache` becomes
-  // un-ready. The next `GetBoundsInPixels()` (inside the `Maximize()` called
-  // from `OnWindowMapped()`) will block on the new parent's geometry, observe
-  // an origin change, and fire `OnBoundsChanged()` synchronously.
-  x11::ReparentNotifyEvent reparent{};
-  reparent.event = xwindow;
-  reparent.window = xwindow;
-  reparent.parent = new_parent;
-  reparent.x = 0;
-  reparent.y = 0;
-  x11::Event reparent_event(/*send_event=*/false, std::move(reparent));
-  connection->DispatchEvent(reparent_event);
-
-  // Step 4: Arm the delegate so the next synchronous `OnBoundsChanged()` frees
-  // the `X11Window` — modelling `views::Widget::CloseNow()` ->
-  // `SetPlatformWindow(nullptr)`, which is the deletion sink explicitly
-  // acknowledged by the `weak_this` guards already present in
-  // `X11Window::Maximize()`, `SetFullscreen()`, and
-  // `WindowTreeHostPlatform::OnBoundsChanged()`.
-  bool armed = true;
-  bool freed = false;
-  EXPECT_CALL(delegate, OnBoundsChanged(_))
-      .WillRepeatedly([&](const PlatformWindowDelegate::BoundsChange&) {
-        if (armed) {
-          armed = false;
-          freed = true;
-          window.reset();
-        }
-      });
-
-  // Step 5: Dispatch a synthetic `MapNotify` for this window. There is no
-  // send_event filtering on the `OnEvent` path; `IsTargetedBy()` only checks
-  // `map->window == xwindow_`.
-  //
-  // `OnEvent` -> `HandleEvent` -> `OnWindowMapped()`:
-  //   `should_maximize_after_map_` is true -> `Maximize()`
-  //     -> `GetBoundsInPixels()` -> `GeometryCache` resolves new parent
-  //     -> origin changed -> `NotifyBoundsChanged()` -> delegate frees
-  //        X11Window`
-  //     -> `Maximize()` `weak_this` guard returns early.
-  //   `OnWindowMapped()` then writes `should_maximize_after_map_ = false` and
-  //   reads `should_grab_pointer_after_map_` on the freed `this`.
-  x11::MapNotifyEvent map_notify{};
-  map_notify.event = xwindow;
-  map_notify.window = xwindow;
-  x11::Event map_event(/*send_event=*/false, std::move(map_notify));
-  connection->DispatchEvent(map_event);
-
-  EXPECT_TRUE(base::test::RunUntil([&]() {
-    connection->DispatchAll();
-    return freed;
-  })) << "delegate->OnBoundsChanged() was not invoked from "
-         "OnWindowMapped/Maximize";
... (truncated)