From b36c618599df1c588a7485ace1f89e82a8cf2ee5 Mon Sep 17 00:00:00 2001
From: Andrew Paseltiner <[email protected]>
Date: Mon, 21 Sep 2026 15:01:37 -0700
Subject: [PATCH] [Actor] Prevent UAF during ClickDispatcher initialization

ClickDispatcher previously dispatched synchronous mousemove events
directly from its constructor. When constructed in-place inside
std::optional (in ClickTool and TypeTool), reentrant destruction of the
owning tool caused by synchronous frame detachment occurred before the
optional was engaged. As a result, ~ClickDispatcher() was not invoked,
its WeakPtrFactory was not invalidated, and the constructor proceeded to
access freed memory upon returning from event dispatch.

This CL resolves the issue by:

1. Keeping ClickDispatcher's constructor side-effect-free.
2. Introducing an explicit Start() method to begin event dispatch
   once the dispatcher is already engaged in its owner.
3. Adding regression tests in ActorUafRegressionBrowserTest for
   mousemove synchronous detachment in both ClickTool and TypeTool.

Fixed: 558301759
Change-Id: Icc6209a9183fade8c773da5b421d96def84a5469
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8435515
Reviewed-by: Kevin McNee <[email protected]>
Commit-Queue: Andrew Paseltiner <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1702088}
---
diff --git a/chrome/renderer/actor/click_dispatcher.cc b/chrome/renderer/actor/click_dispatcher.cc
index 843c9434..6a52886d 100644
--- a/chrome/renderer/actor/click_dispatcher.cc
+++ b/chrome/renderer/actor/click_dispatcher.cc
@@ -42,8 +42,16 @@
     const ResolvedTarget& target,
     const ToolBase& tool,
     base::OnceCallback<void(mojom::ActionResultPtr)> on_complete)
-    : target_(target), tool_(tool), on_complete_(std::move(on_complete)) {
-  WebWidget* widget = target.GetWidget(*tool_);
+    : button_(button),
+      count_(count),
+      target_(target),
+      tool_(tool),
+      on_complete_(std::move(on_complete)) {}
+
+ClickDispatcher::~ClickDispatcher() = default;
+
+void ClickDispatcher::Start() {
+  WebWidget* widget = target_.GetWidget(*tool_);
   CHECK(widget);
   if (base::FeatureList::IsEnabled(features::kGlicActorMoveBeforeClick)) {
     WebMouseEvent mouse_move(WebInputEvent::Type::kMouseMove,
@@ -51,9 +59,9 @@
                              ui::EventTimeForNow());
     // No button for move
     mouse_move.button = WebMouseEvent::Button::kNoButton;
-    mouse_move.SetPositionInWidget(target.widget_point);
+    mouse_move.SetPositionInWidget(target_.widget_point);
     mouse_move.SetPositionInScreen(
-        target.widget_point +
+        target_.widget_point +
         gfx::Vector2dF(widget->ViewRect().OffsetFromOrigin()));
 
     // Mouse move is considered optional, so we don't check this result.
@@ -69,15 +77,13 @@
     base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&ClickDispatcher::DoMouseDown,
-                       weak_ptr_factory_.GetWeakPtr(), button, count, target),
+                       weak_ptr_factory_.GetWeakPtr()),
         delay);
   } else {
-    DoMouseDown(button, count, target);
+    DoMouseDown();
   }
 }
 
-ClickDispatcher::~ClickDispatcher() = default;
-
 void ClickDispatcher::Cancel() {
   weak_ptr_factory_.InvalidateWeakPtrs();
   on_complete_.Reset();
@@ -88,10 +94,8 @@
   }
 }
 
-void ClickDispatcher::DoMouseDown(WebMouseEvent::Button button,
-                                  int count,
-                                  const ResolvedTarget& target) {
-  WebWidget* widget = target.GetWidget(*tool_);
+void ClickDispatcher::DoMouseDown() {
+  WebWidget* widget = target_.GetWidget(*tool_);
   if (!widget) {
     Finish(MakeResult(mojom::ActionResultCode::kFrameWentAway,
                       /*requires_page_stabilization=*/true,
@@ -101,12 +105,12 @@
 
   WebMouseEvent mouse_down(WebInputEvent::Type::kMouseDown,
                            WebInputEvent::kNoModifiers, ui::EventTimeForNow());
-  mouse_down.button = button;
-  mouse_down.click_count = count;
-  mouse_down.SetPositionInWidget(target.widget_point);
+  mouse_down.button = button_;
+  mouse_down.click_count = count_;
+  mouse_down.SetPositionInWidget(target_.widget_point);
 
   gfx::PointF screen_point =
-      target.widget_point +
+      target_.widget_point +
       gfx::Vector2dF(widget->ViewRect().OffsetFromOrigin());
   mouse_down.SetPositionInScreen(screen_point);
   mouse_down.UpdateEventModifiersToMatchButton();
diff --git a/chrome/renderer/actor/click_dispatcher.h b/chrome/renderer/actor/click_dispatcher.h
index 06aa46fd..8a51b6e 100644
--- a/chrome/renderer/actor/click_dispatcher.h
+++ b/chrome/renderer/actor/click_dispatcher.h
@@ -25,8 +25,6 @@
 // The `on_complete` caller may safely destroy this ClickDispatcher.
 class ClickDispatcher {
  public:
-  // Starts the sequence of mouse events upon creation.
-  //
   // The passed in `tool` must own this instance.
   //
   // `on_complete` is always called asynchronously, but isn't invoked if
@@ -38,15 +36,16 @@
                   base::OnceCallback<void(mojom::ActionResultPtr)> on_complete);
   ~ClickDispatcher();
 
+  // Starts the sequence of mouse events.
+  void Start();
+
   // Cancels future dispatching, with the exception that if a mouse button is in
   // a down state, mouse up will be synchronously dispatched.
   void Cancel();
 
  private:
   // Clicks the mouse down, and calls DoMouseUp() asynchronously after a delay.
-  void DoMouseDown(blink::WebMouseEvent::Button button,
-                   int count,
-                   const ResolvedTarget& target);
+  void DoMouseDown();
 
   // Calls DoMouseUpImpl(), then calls Finish(). Used by Cancel() so it can
   // Finish() with a different result.
@@ -59,6 +58,9 @@
   // called, or if Cancel() has been called.
   void Finish(mojom::ActionResultPtr result);
 
+  const blink::WebMouseEvent::Button button_;
+  const int count_;
+
   // The target to click on.
   const ResolvedTarget target_;
 
diff --git a/chrome/renderer/actor/click_tool.cc b/chrome/renderer/actor/click_tool.cc
index c33d856..c346c39 100644
--- a/chrome/renderer/actor/click_tool.cc
+++ b/chrome/renderer/actor/click_tool.cc
@@ -149,6 +149,7 @@
   CHECK(!click_dispatcher_);
   click_dispatcher_.emplace(button, click_count, target, *this,
                             std::move(callback));
+  click_dispatcher_->Start();
 }
 
 std::string ClickTool::DebugString() const {
diff --git a/chrome/renderer/actor/type_tool.cc b/chrome/renderer/actor/type_tool.cc
index 310d749..55d1bb77 100644
--- a/chrome/renderer/actor/type_tool.cc
+++ b/chrome/renderer/actor/type_tool.cc
@@ -487,6 +487,7 @@
       blink::WebMouseEvent::Button::kLeft, 1, *resolved_target_, *this,
       base::BindOnce(&TypeTool::OnFocusingClickComplete,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  click_dispatcher_->Start();
 }
 
 void TypeTool::Cancel() {
diff --git a/chrome/browser/actor/tools/uaf_regression_browsertest.cc b/chrome/browser/actor/tools/uaf_regression_browsertest.cc
index c45d5cf..75607165 100644
--- a/chrome/browser/actor/tools/uaf_regression_browsertest.cc
+++ b/chrome/browser/actor/tools/uaf_regression_browsertest.cc
@@ -86,6 +86,28 @@
 
   // This should not crash the renderer.
   ASSERT_TRUE(result.Wait());
+  ExpectErrorResult(result, mojom::ActionResultCode::kFrameWentAway);
+  EXPECT_TRUE(main_frame()->IsRenderFrameLive());
+}
+
+// Regression test for UAF in Glic actor tools.
+// See crbug.com/558301759.
+IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
+                       ClickTool_HandlesSynchronousFrameDetachmentOnMouseMove) {
+  SetupAbaFrames("/actor/click_tool_uaf_mousemove_inner.html");
+  RenderFrameHost* inner_rfh = GetInnerRfh();
+  ASSERT_TRUE(inner_rfh);
+
+  const int32_t target_id = GetDOMNodeId(*inner_rfh, "#target").value();
+
+  std::unique_ptr<ToolRequest> action = MakeClickRequest(*inner_rfh, target_id);
+  ActResultFuture result;
+  actor_task().Act(ToRequestList(action), result.GetCallback());
+
+  // This should not crash the renderer.
+  ASSERT_TRUE(result.Wait());
+  ExpectErrorResult(result, mojom::ActionResultCode::kFrameWentAway);
+  EXPECT_TRUE(main_frame()->IsRenderFrameLive());
 }
 
 // Regression test for UAF in Glic actor tools.
@@ -108,6 +130,25 @@
 }
 
 // Regression test for UAF in Glic actor tools.
+// See crbug.com/558301759.
+IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
+                       TypeTool_HandlesSynchronousFrameDetachmentOnMouseMove) {
+  SetupAbaFrames("/actor/type_tool_uaf_mousemove_inner.html");
+  RenderFrameHost* inner_rfh = GetInnerRfh();
+  ASSERT_TRUE(inner_rfh);
+
+  const int32_t target_id = GetDOMNodeId(*inner_rfh, "#target").value();
+
+  std::unique_ptr<ToolRequest> action = MakeTypeRequest(
+      *inner_rfh, target_id, "hello", /* follow_by_enter= */ false);
+  ActResultFuture result;
+  actor_task().Act(ToRequestList(action), result.GetCallback());
+
+  // This should not crash the renderer.
+  ASSERT_TRUE(result.Wait());
+}
+
+// Regression test for UAF in Glic actor tools.
 // See crbug.com/506150628.
 IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
                        DragAndReleaseTool_HandlesSynchronousFrameDetachment) {
diff --git a/components/test/data/actor/click_tool_uaf_mousemove_inner.html b/components/test/data/actor/click_tool_uaf_mousemove_inner.html
new file mode 100644
index 0000000..307160f3
--- /dev/null
+++ b/components/test/data/actor/click_tool_uaf_mousemove_inner.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<body>
+  <div id="target" style="width: 100px; height: 100px; background: red;"></div>
+  <script>
+    const target = document.getElementById('target');
+    target.addEventListener('mousemove', () => {
+      // Remove the mid iframe from the top window.
+      // Since mid is cross-origin, removing it synchronously detaches this frame.
+      parent.parent.document.getElementById('mid').remove();
+    });
+  </script>
+</body>
+</html>
diff --git a/components/test/data/actor/type_tool_uaf_mousemove_inner.html b/components/test/data/actor/type_tool_uaf_mousemove_inner.html
new file mode 100644
index 0000000..b9e645de7
--- /dev/null
+++ b/components/test/data/actor/type_tool_uaf_mousemove_inner.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<body>
+  <input id="target" type="text">
+  <script>
+    const target = document.getElementById('target');
+    target.addEventListener('mousemove', () => {
+      // Remove the mid iframe from the top window.
+      // Since mid is cross-origin, removing it synchronously detaches this frame.
+      parent.parent.document.getElementById('mid').remove();
+    });
+  </script>
+</body>
+</html>

diff --git a/chrome/browser/actor/tools/uaf_regression_browsertest.cc b/chrome/browser/actor/tools/uaf_regression_browsertest.cc
index c45d5cf..75607165 100644
--- a/chrome/browser/actor/tools/uaf_regression_browsertest.cc
+++ b/chrome/browser/actor/tools/uaf_regression_browsertest.cc
@@ -86,6 +86,28 @@
 
   // This should not crash the renderer.
   ASSERT_TRUE(result.Wait());
+  ExpectErrorResult(result, mojom::ActionResultCode::kFrameWentAway);
+  EXPECT_TRUE(main_frame()->IsRenderFrameLive());
+}
+
+// Regression test for UAF in Glic actor tools.
+// See crbug.com/558301759.
+IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
+                       ClickTool_HandlesSynchronousFrameDetachmentOnMouseMove) {
+  SetupAbaFrames("/actor/click_tool_uaf_mousemove_inner.html");
+  RenderFrameHost* inner_rfh = GetInnerRfh();
+  ASSERT_TRUE(inner_rfh);
+
+  const int32_t target_id = GetDOMNodeId(*inner_rfh, "#target").value();
+
+  std::unique_ptr<ToolRequest> action = MakeClickRequest(*inner_rfh, target_id);
+  ActResultFuture result;
+  actor_task().Act(ToRequestList(action), result.GetCallback());
+
+  // This should not crash the renderer.
+  ASSERT_TRUE(result.Wait());
+  ExpectErrorResult(result, mojom::ActionResultCode::kFrameWentAway);
+  EXPECT_TRUE(main_frame()->IsRenderFrameLive());
 }
 
 // Regression test for UAF in Glic actor tools.
@@ -108,6 +130,25 @@
 }
 
 // Regression test for UAF in Glic actor tools.
+// See crbug.com/558301759.
+IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
+                       TypeTool_HandlesSynchronousFrameDetachmentOnMouseMove) {
+  SetupAbaFrames("/actor/type_tool_uaf_mousemove_inner.html");
+  RenderFrameHost* inner_rfh = GetInnerRfh();
+  ASSERT_TRUE(inner_rfh);
+
+  const int32_t target_id = GetDOMNodeId(*inner_rfh, "#target").value();
+
+  std::unique_ptr<ToolRequest> action = MakeTypeRequest(
+      *inner_rfh, target_id, "hello", /* follow_by_enter= */ false);
+  ActResultFuture result;
+  actor_task().Act(ToRequestList(action), result.GetCallback());
+
+  // This should not crash the renderer.
+  ASSERT_TRUE(result.Wait());
+}
+
+// Regression test for UAF in Glic actor tools.
 // See crbug.com/506150628.
 IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
                        DragAndReleaseTool_HandlesSynchronousFrameDetachment) {
diff --git a/components/test/data/actor/click_tool_uaf_mousemove_inner.html b/components/test/data/actor/click_tool_uaf_mousemove_inner.html
new file mode 100644
index 0000000..307160f3
--- /dev/null
+++ b/components/test/data/actor/click_tool_uaf_mousemove_inner.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<body>
+  <div id="target" style="width: 100px; height: 100px; background: red;"></div>
+  <script>
+    const target = document.getElementById('target');
+    target.addEventListener('mousemove', () => {
+      // Remove the mid iframe from the top window.
+      // Since mid is cross-origin, removing it synchronously detaches this frame.
+      parent.parent.document.getElementById('mid').remove();
+    });
+  </script>
+</body>
+</html>
diff --git a/components/test/data/actor/type_tool_uaf_mousemove_inner.html b/components/test/data/actor/type_tool_uaf_mousemove_inner.html
new file mode 100644
index 0000000..b9e645de7
--- /dev/null
+++ b/components/test/data/actor/type_tool_uaf_mousemove_inner.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<body>
+  <input id="target" type="text">
+  <script>
+    const target = document.getElementById('target');
+    target.addEventListener('mousemove', () => {
+      // Remove the mid iframe from the top window.
+      // Since mid is cross-origin, removing it synchronously detaches this frame.
+      parent.parent.document.getElementById('mid').remove();
+    });
+  </script>
+</body>
+</html>