From 995c9093715d5526e1c1567604be0d449242ac14 Mon Sep 17 00:00:00 2001
From: mark a. foltz <mfoltz@chromium.org>
Date: Fri, 28 Aug 2026 13:35:37 -0700
Subject: [PATCH] [actor] Handle frame detach during tool initialization.

ToolBase::EnsureTargetInView() can synchronously dispatch DOM events
(beforematch) when revealing hidden=until-found ancestors of the target.
The page may use this to detach the owning frame, destroying the
ToolExecutor and ToolBase while they are still on the stack.

Guard the affected paths with weak_this checks:
- ToolBase::EnsureTargetInView() bails if destroyed.
- ToolExecutor::InitializeToolImpl() and InvokeTool() bail after
  EnsureTargetInView and return kFrameWentAway.
- Journal::PendingAsyncEntry now holds Journal as a WeakPtr so
  on-stack entries survive Journal destruction.
- PageTool::Validate() wraps the InitializeTool callback with
  mojo::WrapCallbackWithDefaultInvokeIfNotRun so it resolves with
  kFrameWentAway if the renderer connection drops.

Add a regression browser test to verify synchronous frame detachment
during tool initialization is handled cleanly.

Fixed: 516404074
Change-Id: Ib2ef0b668b1fb8610ed38f26c8ecd1dedb3eca75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8291278
Commit-Queue: Mark Foltz <mfoltz@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1688282}
---
diff --git a/chrome/browser/actor/tools/page_tool.cc b/chrome/browser/actor/tools/page_tool.cc
index 93092da..8113d4bb 100644
--- a/chrome/browser/actor/tools/page_tool.cc
+++ b/chrome/browser/actor/tools/page_tool.cc
@@ -310,10 +310,18 @@
   std::string text = request_->GetTextContentSentToRenderer();
   if (text.empty() || !scanning_enabled) {
     if (validation_supported) {
+      // If the renderer connection drops (e.g. frame detached during
+      // tool initialization), ensure the callback is invoked with
+      // kFrameWentAway.
+      ToolCallback wrapped_callback =
+          mojo::WrapCallbackWithDefaultInvokeIfNotRun(
+              std::move(callback),
+              MakeResult(mojom::ActionResultCode::kFrameWentAway));
       chrome_render_frame_->InitializeTool(
           std::move(invocation),
           base::BindOnce(&PageTool::OnInitializeToolComplete,
-                         weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+                         weak_ptr_factory_.GetWeakPtr(),
+                         std::move(wrapped_callback)));
     } else {
       std::move(callback).Run(MakeOkResult());
     }
diff --git a/chrome/renderer/actor/journal.cc b/chrome/renderer/actor/journal.cc
index b731ded8..c2ac62b0 100644
--- a/chrome/renderer/actor/journal.cc
+++ b/chrome/renderer/actor/journal.cc
@@ -18,11 +18,11 @@
 }  // namespace
 
 Journal::PendingAsyncEntry::PendingAsyncEntry(base::PassKey<Journal> pass_key,
-                                              base::SafeRef<Journal> journal,
+                                              base::WeakPtr<Journal> journal,
                                               TaskId task_id,
                                               std::string_view event_name)
     : pass_key_(pass_key),
-      journal_(journal),
+      journal_(std::move(journal)),
       task_id_(task_id),
       event_name_(event_name) {}
 
@@ -37,17 +37,23 @@
   CHECK(!terminated_);
   terminated_ = true;
   ACTOR_LOG() << "End " << event_name_ << ": " << details;
-  journal_->AddEndEvent(pass_key_, task_id_, event_name_, std::move(details));
+  if (journal_) {
+    journal_->AddEndEvent(pass_key_, task_id_, event_name_, std::move(details));
+  }
 }
 
 void Journal::PendingAsyncEntry::Log(std::string_view event_name) {
-  journal_->Log(task_id_, event_name, {});
+  if (journal_) {
+    journal_->Log(task_id_, event_name, {});
+  }
 }
 
 void Journal::PendingAsyncEntry::Log(
     std::string_view event_name,
     std::vector<mojom::JournalDetailsPtr> details) {
-  journal_->Log(task_id_, event_name, std::move(details));
+  if (journal_) {
+    journal_->Log(task_id_, event_name, std::move(details));
+  }
 }
 
 Journal::Journal() = default;
@@ -89,7 +95,7 @@
       std::string(event_name), MakeRendererTrackUUID(task_id),
       std::move(details)));
   return base::WrapUnique(new PendingAsyncEntry(base::PassKey<Journal>(),
-                                                weak_factory_.GetSafeRef(),
+                                                weak_factory_.GetWeakPtr(),
                                                 task_id, event_name));
 }
 
diff --git a/chrome/renderer/actor/journal.h b/chrome/renderer/actor/journal.h
index a9cc604..5e87ba0c 100644
--- a/chrome/renderer/actor/journal.h
+++ b/chrome/renderer/actor/journal.h
@@ -34,8 +34,10 @@
    public:
     // Creation of the event is only from the Journal itself. Use
     // `Journal::CreatePendingAsyncEntry` to create this object.
+    // Holds a WeakPtr<Journal> so on-stack entries survive frame/journal
+    // destruction during synchronous DOM events without crashing.
     PendingAsyncEntry(base::PassKey<Journal>,
-                      base::SafeRef<Journal> journal,
+                      base::WeakPtr<Journal> journal,
                       TaskId task_id,
                       std::string_view event_name);
     ~PendingAsyncEntry();
@@ -53,7 +55,7 @@
    private:
     base::PassKey<Journal> pass_key_;
     bool terminated_ = false;
-    base::SafeRef<Journal> journal_;
+    base::WeakPtr<Journal> journal_;
     TaskId task_id_;
     std::string event_name_;
   };
diff --git a/chrome/renderer/actor/tool_base.cc b/chrome/renderer/actor/tool_base.cc
index d12bfb19..e961f64 100644
--- a/chrome/renderer/actor/tool_base.cc
+++ b/chrome/renderer/actor/tool_base.cc
@@ -574,25 +574,52 @@
   int32_t dom_node_id = target_->get_dom_node_id();
   WebElement node = GetNodeFromIdIncludingPopup(frame_.get(), dom_node_id)
                         .DynamicTo<WebElement>();
-  if (node && node.VisibleBoundsInWidget().IsEmpty()) {
-    gfx::Rect bounds_before = node.BoundsInWidget();
-    node.RevealAutoExpandableAncestors();
-    node.ScrollIntoViewIfNeeded();
+  if (!node) {
+    return false;
+  }
 
-    journal_->Log(task_id_, "ScrollIntoView",
-                  JournalDetailsBuilder()
-                      .Add("target", node)
-                      .Add("CurBounds", bounds_before)
-                      .Add("NewBounds", node.BoundsInWidget())
-                      .Add("NewBoundsVisible", node.VisibleBoundsInWidget())
-                      .Add("ViewportBounds",
-                           node.GetDocument().GetFrame()->VisibleContentRect())
-                      .Build());
-
+  // Bounds queries force lifecycle/layout updates in Blink. Guard against the
+  // possibility of layout side effects detaching the owning frame and
+  // destroying this tool while executing on the stack.
+  base::WeakPtr<ToolBase> weak_this = weak_ptr_factory_.GetWeakPtr();
+  if (!node.VisibleBoundsInWidget().IsEmpty()) {
+    return false;
+  }
+  if (!weak_this) {
     return true;
   }
 
-  return false;
+  gfx::Rect bounds_before = node.BoundsInWidget();
+  if (!weak_this) {
+    return true;
+  }
+
+  // Revealing auto-expandable ancestors synchronously dispatches DOM events
+  // (`beforematch`) that might run script detaching the owning frame and
+  // destroying this tool.
+  node.RevealAutoExpandableAncestors();
+  if (!weak_this) {
+    return true;
+  }
+  node.ScrollIntoViewIfNeeded();
+  if (!weak_this) {
+    return true;
+  }
+
+  blink::WebLocalFrame* local_frame = node.GetDocument().GetFrame();
+  gfx::Rect viewport_bounds =
+      local_frame ? local_frame->VisibleContentRect() : gfx::Rect();
+
+  journal_->Log(task_id_, "ScrollIntoView",
+                JournalDetailsBuilder()
+                    .Add("target", node)
+                    .Add("CurBounds", bounds_before)
+                    .Add("NewBounds", node.BoundsInWidget())
+                    .Add("NewBoundsVisible", node.VisibleBoundsInWidget())
+                    .Add("ViewportBounds", viewport_bounds)
+                    .Build());
+
+  return true;
 }
 
 mojom::ActionResultPtr ToolBase::ValidateTimeOfUse(
diff --git a/chrome/renderer/actor/tool_base.h b/chrome/renderer/actor/tool_base.h
index 9f8790a..d44d3cd 100644
--- a/chrome/renderer/actor/tool_base.h
+++ b/chrome/renderer/actor/tool_base.h
@@ -11,6 +11,7 @@
 
 #include "base/functional/callback_forward.h"
 #include "base/memory/raw_ref.h"
+#include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "base/types/expected.h"
 #include "chrome/common/actor.mojom-forward.h"
@@ -179,6 +180,11 @@
                                  bool check_aria) const;
 
   bool is_revalidation_ = false;
+
+  // Used to verify this object is still alive across synchronous DOM event
+  // dispatches (such as `beforematch`) that might detach the frame and destroy
+  // this tool while executing on the stack.
+  base::WeakPtrFactory<ToolBase> weak_ptr_factory_{this};
 };
 }  // namespace actor
 
diff --git a/chrome/renderer/actor/tool_executor.cc b/chrome/renderer/actor/tool_executor.cc
index 96938e28..b4867ca2a 100644
--- a/chrome/renderer/actor/tool_executor.cc
+++ b/chrome/renderer/actor/tool_executor.cc
@@ -156,7 +156,16 @@
       NOTREACHED();
   }
 
-  if (tool_->EnsureTargetInView()) {
+  // EnsureTargetInView() may synchronously dispatch DOM events (`beforematch`)
+  // that can run page script that detaches the owning frame and destroys
+  // this executor.
+  base::WeakPtr<ToolExecutor> weak_this = weak_ptr_factory_.GetWeakPtr();
+  const bool performed_scroll = tool_->EnsureTargetInView();
+  if (!weak_this) {
+    return mojom::InitializeToolResult::NewErrorResult(
+        MakeResult(mojom::ActionResultCode::kFrameWentAway));
+  }
+  if (performed_scroll) {
     performed_scroll_into_view_ = true;
   }
 
@@ -220,8 +229,14 @@
   // helps when debugging unresponsive renderers.
   journal_->SendLogBuffer();
   actor::TaskId task_id = invocation->task_id;
+  // InitializeToolImpl() may synchronously dispatch DOM events which can
+  // destroy the owning frame and this executor.
+  base::WeakPtr<ToolExecutor> weak_this = weak_ptr_factory_.GetWeakPtr();
   mojom::InitializeToolResultPtr result =
       InitializeToolImpl(std::move(invocation));
+  if (!weak_this) {
+    return;
+  }
   if (result->is_error_result()) {
     // The tool failed to initialize because another tool is active. Abort this
     // invocation immediately without disturbing the running tool.
diff --git a/chrome/browser/actor/tools/uaf_regression_browsertest.cc b/chrome/browser/actor/tools/uaf_regression_browsertest.cc
index 34086a4..879cc476 100644
--- a/chrome/browser/actor/tools/uaf_regression_browsertest.cc
+++ b/chrome/browser/actor/tools/uaf_regression_browsertest.cc
@@ -148,5 +148,23 @@
   ASSERT_TRUE(result.Wait());
 }
 
+IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
+                       ScrollToTool_HandlesSynchronousFrameDetachment) {
+  SetupAbaFrames("/actor/scroll_to_tool_detached_frame_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 =
+      MakeScrollToRequest(*inner_rfh, target_id);
+  ActResultFuture result;
+  actor_task().Act(ToRequestList(action), result.GetCallback());
+
+  // Frame detachment during tool initialization should be handled cleanly
+  // without crashing the renderer.
+  ASSERT_TRUE(result.Wait());
+}
+
 }  // namespace
 }  // namespace actor
diff --git a/components/test/data/actor/scroll_to_tool_detached_frame_inner.html b/components/test/data/actor/scroll_to_tool_detached_frame_inner.html
new file mode 100644
index 0000000..c8e38366
--- /dev/null
+++ b/components/test/data/actor/scroll_to_tool_detached_frame_inner.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<html>
+<body>
+  <div id="wrap" hidden="until-found">
+    <button id="target">Target</button>
+  </div>
+  <script>
+    const wrap = document.getElementById('wrap');
+    wrap.addEventListener('beforematch', () => {
+      // 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 34086a4..879cc476 100644
--- a/chrome/browser/actor/tools/uaf_regression_browsertest.cc
+++ b/chrome/browser/actor/tools/uaf_regression_browsertest.cc
@@ -148,5 +148,23 @@
   ASSERT_TRUE(result.Wait());
 }
 
+IN_PROC_BROWSER_TEST_F(ActorUafRegressionBrowserTest,
+                       ScrollToTool_HandlesSynchronousFrameDetachment) {
+  SetupAbaFrames("/actor/scroll_to_tool_detached_frame_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 =
+      MakeScrollToRequest(*inner_rfh, target_id);
+  ActResultFuture result;
+  actor_task().Act(ToRequestList(action), result.GetCallback());
+
+  // Frame detachment during tool initialization should be handled cleanly
+  // without crashing the renderer.
+  ASSERT_TRUE(result.Wait());
+}
+
 }  // namespace
 }  // namespace actor
diff --git a/components/test/data/actor/scroll_to_tool_detached_frame_inner.html b/components/test/data/actor/scroll_to_tool_detached_frame_inner.html
new file mode 100644
index 0000000..c8e38366
--- /dev/null
+++ b/components/test/data/actor/scroll_to_tool_detached_frame_inner.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<html>
+<body>
+  <div id="wrap" hidden="until-found">
+    <button id="target">Target</button>
+  </div>
+  <script>
+    const wrap = document.getElementById('wrap');
+    wrap.addEventListener('beforematch', () => {
+      // 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>