From d5186c8dddab74ef4874b27aba84f942c16efd16 Mon Sep 17 00:00:00 2001
From: Alex Rudenko <[email protected]>
Date: Mon, 03 Aug 2026 03:00:48 -0700
Subject: [PATCH] DevTools: avoid reentrant erase during TargetHandler::Disable

TargetHandler::Disable() bulk-clears `attached_sessions_`, but
destroying a Session detaches its child DevTools session from the
target's agent host. The embedder side of that child session may own
hidden targets which get closed at this point; if their renderers are
gone the close path runs synchronously and the hidden targets' agent
hosts call AgentHostClosed() on sibling Sessions still owned by the map
being cleared, which then call `attached_sessions_.erase(id_)` on it.

Move the map into a local before releasing it so the reentrant erase
operates on the now-empty member instead of the container being walked.

Fixed: 521620916
Change-Id: Iaa54071504b73d54ac1f18cfc67514f42e1c3d31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8182568
Commit-Queue: Alex Rudenko <[email protected]>
Reviewed-by: Andrey Kosyakov <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1672509}
---

diff --git a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
index 1174671f..382d5c3c 100644
--- a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
+++ b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
@@ -5374,4 +5374,108 @@
   EXPECT_EQ("Internal error", *error_message);
 }
 
+// Regression test for crbug.com/521620916: detaching the browser client when
+// hidden targets with crashed renderers are attached should not cause
+// reentrancy UAF when sessions are cleared.
+IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, DetachWithCrashedHiddenTargets) {
+  content::ScopedAllowRendererCrashes scoped_allow_renderer_crashes;
+  set_agent_host_can_close();
+  AttachToBrowserTarget();
+
+  base::DictValue create_page_params;
+  create_page_params.Set("url", "about:blank");
+  const base::DictValue* result =
+      SendCommandSync("Target.createTarget", std::move(create_page_params));
+  ASSERT_TRUE(result);
+  const std::string* page_target_id_ptr = result->FindString("targetId");
+  ASSERT_TRUE(page_target_id_ptr);
+  std::string page_target_id = *page_target_id_ptr;
+
+  scoped_refptr<DevToolsAgentHost> page_agent_host =
+      DevToolsAgentHost::GetForId(page_target_id);
+  ASSERT_TRUE(page_agent_host);
+  EXPECT_TRUE(WaitForLoadStop(page_agent_host->GetWebContents()));
+
+  base::DictValue attach_params;
+  attach_params.Set("targetId", page_target_id);
+  attach_params.Set("flatten", true);
+  result = SendCommandSync("Target.attachToTarget", std::move(attach_params));
+  ASSERT_TRUE(result);
+  const std::string* page_session_id_ptr = result->FindString("sessionId");
+  ASSERT_TRUE(page_session_id_ptr);
+  std::string page_session_id = *page_session_id_ptr;
+
+  // Use 12 targets to ensure sufficient depth and branching in libc++'s
+  // std::map Red-Black tree so that reentrant erase() calls during clearance
+  // reliably collide with post-order traversal regardless of key ordering.
+  constexpr size_t kHiddenTargetCount = 12;
+  std::vector<std::string> hidden_target_ids;
+  for (size_t i = 0; i < kHiddenTargetCount; ++i) {
+    base::DictValue params;
+    params.Set("url", "about:blank");
+    params.Set("hidden", true);
+    const base::DictValue* create_result = SendSessionCommand(
+        "Target.createTarget", std::move(params), page_session_id, true);
+    ASSERT_TRUE(create_result);
+    const std::string* target_id = create_result->FindString("targetId");
+    ASSERT_TRUE(target_id);
+    hidden_target_ids.push_back(*target_id);
+  }
+
+  for (const std::string& target_id : hidden_target_ids) {
+    scoped_refptr<DevToolsAgentHost> agent_host =
+        DevToolsAgentHost::GetForId(target_id);
+    ASSERT_TRUE(agent_host);
+    WebContents* web_contents = agent_host->GetWebContents();
+    ASSERT_TRUE(web_contents);
+    EXPECT_TRUE(WaitForLoadStop(web_contents));
+  }
+
+  for (const std::string& target_id : hidden_target_ids) {
+    base::DictValue params;
+    params.Set("targetId", target_id);
+    params.Set("flatten", true);
+    SendCommandSync("Target.attachToTarget", std::move(params));
+  }
+
+  // Terminate each hidden target's renderer so that closing the page on
+  // detach takes the synchronous path. The browser session is then detached
+  // during fixture tear down, which closes the hidden targets while the
+  // owning child session is being released.
+  std::set<RenderProcessHost*> rphs;
+  for (const std::string& target_id : hidden_target_ids) {
+    scoped_refptr<DevToolsAgentHost> agent_host =
+        DevToolsAgentHost::GetForId(target_id);
+    ASSERT_TRUE(agent_host);
+    WebContents* web_contents = agent_host->GetWebContents();
+    ASSERT_TRUE(web_contents);
+    RenderProcessHost* rph = web_contents->GetPrimaryMainFrame()->GetProcess();
+    if (rph) {
+      rphs.insert(rph);
+    }
+  }
+
+  for (RenderProcessHost* rph : rphs) {
+    if (rph && rph->IsInitializedAndNotDead()) {
+      RenderProcessHostWatcher watcher(
+          rph, RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+      rph->Shutdown(RESULT_CODE_KILLED);
+      watcher.Wait();
+    }
+  }
+
+  for (const std::string& target_id : hidden_target_ids) {
+    scoped_refptr<DevToolsAgentHost> agent_host =
+        DevToolsAgentHost::GetForId(target_id);
+    ASSERT_TRUE(agent_host);
+    WebContents* web_contents = agent_host->GetWebContents();
+    ASSERT_TRUE(web_contents);
+    ASSERT_FALSE(web_contents->GetPrimaryMainFrame()->IsRenderFrameLive());
+  }
+
+  // Detaching the browser client closes the page session and hidden targets
+  // synchronously; should not crash or trigger reentrancy UAF.
+  Detach();
+}
+
 }  // namespace content
diff --git a/content/browser/devtools/protocol/target_handler.cc b/content/browser/devtools/protocol/target_handler.cc
index f051aaf1..f20a1a8 100644
--- a/content/browser/devtools/protocol/target_handler.cc
+++ b/content/browser/devtools/protocol/target_handler.cc
@@ -792,7 +792,10 @@
   SetDiscoverTargets(false, {});
   hidden_target_manager_.Clear();
   auto_attached_sessions_.clear();
-  attached_sessions_.clear();
+  // Destroying a session may synchronously close attached targets which can
+  // try to detach sibling sessions, so move the map aside before releasing.
+  auto attached_sessions = std::move(attached_sessions_);
+  attached_sessions.clear();
 
   DevToolsManagerDelegate* delegate =
       DevToolsManager::GetInstance()->delegate();

diff --git a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
index 1174671f..382d5c3c 100644
--- a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
+++ b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
@@ -5374,4 +5374,108 @@
   EXPECT_EQ("Internal error", *error_message);
 }
 
+// Regression test for crbug.com/521620916: detaching the browser client when
+// hidden targets with crashed renderers are attached should not cause
+// reentrancy UAF when sessions are cleared.
+IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, DetachWithCrashedHiddenTargets) {
+  content::ScopedAllowRendererCrashes scoped_allow_renderer_crashes;
+  set_agent_host_can_close();
+  AttachToBrowserTarget();
+
+  base::DictValue create_page_params;
+  create_page_params.Set("url", "about:blank");
+  const base::DictValue* result =
+      SendCommandSync("Target.createTarget", std::move(create_page_params));
+  ASSERT_TRUE(result);
+  const std::string* page_target_id_ptr = result->FindString("targetId");
+  ASSERT_TRUE(page_target_id_ptr);
+  std::string page_target_id = *page_target_id_ptr;
+
+  scoped_refptr<DevToolsAgentHost> page_agent_host =
+      DevToolsAgentHost::GetForId(page_target_id);
+  ASSERT_TRUE(page_agent_host);
+  EXPECT_TRUE(WaitForLoadStop(page_agent_host->GetWebContents()));
+
+  base::DictValue attach_params;
+  attach_params.Set("targetId", page_target_id);
+  attach_params.Set("flatten", true);
+  result = SendCommandSync("Target.attachToTarget", std::move(attach_params));
+  ASSERT_TRUE(result);
+  const std::string* page_session_id_ptr = result->FindString("sessionId");
+  ASSERT_TRUE(page_session_id_ptr);
+  std::string page_session_id = *page_session_id_ptr;
+
+  // Use 12 targets to ensure sufficient depth and branching in libc++'s
+  // std::map Red-Black tree so that reentrant erase() calls during clearance
+  // reliably collide with post-order traversal regardless of key ordering.
+  constexpr size_t kHiddenTargetCount = 12;
+  std::vector<std::string> hidden_target_ids;
+  for (size_t i = 0; i < kHiddenTargetCount; ++i) {
+    base::DictValue params;
+    params.Set("url", "about:blank");
+    params.Set("hidden", true);
+    const base::DictValue* create_result = SendSessionCommand(
+        "Target.createTarget", std::move(params), page_session_id, true);
+    ASSERT_TRUE(create_result);
+    const std::string* target_id = create_result->FindString("targetId");
+    ASSERT_TRUE(target_id);
+    hidden_target_ids.push_back(*target_id);
+  }
+
+  for (const std::string& target_id : hidden_target_ids) {
+    scoped_refptr<DevToolsAgentHost> agent_host =
+        DevToolsAgentHost::GetForId(target_id);
+    ASSERT_TRUE(agent_host);
+    WebContents* web_contents = agent_host->GetWebContents();
+    ASSERT_TRUE(web_contents);
+    EXPECT_TRUE(WaitForLoadStop(web_contents));
+  }
+
+  for (const std::string& target_id : hidden_target_ids) {
+    base::DictValue params;
+    params.Set("targetId", target_id);
+    params.Set("flatten", true);
+    SendCommandSync("Target.attachToTarget", std::move(params));
+  }
+
+  // Terminate each hidden target's renderer so that closing the page on
+  // detach takes the synchronous path. The browser session is then detached
+  // during fixture tear down, which closes the hidden targets while the
+  // owning child session is being released.
+  std::set<RenderProcessHost*> rphs;
+  for (const std::string& target_id : hidden_target_ids) {
+    scoped_refptr<DevToolsAgentHost> agent_host =
+        DevToolsAgentHost::GetForId(target_id);
+    ASSERT_TRUE(agent_host);
+    WebContents* web_contents = agent_host->GetWebContents();
+    ASSERT_TRUE(web_contents);
+    RenderProcessHost* rph = web_contents->GetPrimaryMainFrame()->GetProcess();
+    if (rph) {
+      rphs.insert(rph);
+    }
+  }
+
+  for (RenderProcessHost* rph : rphs) {
+    if (rph && rph->IsInitializedAndNotDead()) {
+      RenderProcessHostWatcher watcher(
+          rph, RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+      rph->Shutdown(RESULT_CODE_KILLED);
+      watcher.Wait();
+    }
+  }
+
+  for (const std::string& target_id : hidden_target_ids) {
+    scoped_refptr<DevToolsAgentHost> agent_host =
+        DevToolsAgentHost::GetForId(target_id);
+    ASSERT_TRUE(agent_host);
+    WebContents* web_contents = agent_host->GetWebContents();
+    ASSERT_TRUE(web_contents);
+    ASSERT_FALSE(web_contents->GetPrimaryMainFrame()->IsRenderFrameLive());
+  }
+
+  // Detaching the browser client closes the page session and hidden targets
+  // synchronously; should not crash or trigger reentrancy UAF.
+  Detach();
+}
+
 }  // namespace content