From ccc0ccf66880d0306d268da01f2d88a1f0aa1f00 Mon Sep 17 00:00:00 2001
From: Alex Moshchuk <[email protected]>
Date: Fri, 17 Jul 2026 09:12:18 -0700
Subject: [PATCH] Clear inner-delegate placeholder children at attach time

When attaching an inner WebContents/GuestView to a placeholder
RenderFrameHost, SwapOuterDelegateFrame() sends an Unload IPC but defers
the placeholder's child-frame cleanup to OnUnloadACK(), which only runs
once the renderer sends DidUnloadRenderFrame. Until then the placeholder
can hold both inner_tree_main_frame_tree_node_id_ and non-empty
children_, which FrameTree::NodeIterator does not handle (the children
are skipped by cross-tree iteration).

Make the cleanup browser-side:
- SwapOuterDelegateFrame() now calls ResetChildren() before
  sending the Unload IPC, so existing children are cleared at
  attach time without waiting for the renderer.
- OnCreateChildFrame() now drops the message if
  inner_tree_main_frame_tree_node_id_ is set, so the placeholder
  cannot acquire new children once it has become a delegate node
  for an inner frame tree.

The existing ResetChildren() in OnUnloadACK() is retained as a no-op
safety net.

This is a followup to a similar issue in crbug.com/517241992.

Most of this CL has been adapted from the AI-generated fix at
https://crbug.com/518078552#comment4.

Bug: 518078552, 517241992
Change-Id: I66871ede1d5b8312c68393d09d7ff4ac2e6c0d37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8112019
Commit-Queue: Alex Moshchuk <[email protected]>
Reviewed-by: Kevin McNee <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1663944}
---

diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index 27e1428..cf8354a 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -5188,6 +5188,16 @@
     }
   }
 
+  // The placeholder frame for an inner frame tree (e.g., an attached
+  // GuestView, fenced frame) must never have its own local children, since
+  // the inner tree's main frame is treated as this frame's only child during
+  // FrameTree iteration. This message could only have been sent before the
+  // renderer processed the corresponding Unload IPC, so just drop it. See
+  // https://crbug.com/518078552.
+  if (inner_tree_main_frame_tree_node_id_) {
+    return;
+  }
+
   // `new_routing_id`, `frame_token`, `devtools_frame_token` and
   // `document_token` were generated on the browser's IO thread and not taken
   // from the renderer process.
@@ -6879,6 +6889,12 @@
 void RenderFrameHostImpl::SwapOuterDelegateFrame(
     RenderFrameProxyHost* proxy,
     const base::UnguessableToken& devtools_frame_token) {
+  // The placeholder frame for an inner frame tree must never have its own
+  // local children. Clear them here, before sending the Unload IPC, so that
+  // the cleanup does not depend on the renderer sending a DidUnloadRenderFrame
+  // ACK. See https://crbug.com/518078552.
+  ResetChildren();
+
   // Note: At this point the placeholder iframe for embedding the guest has
   // been initialized with a devtools_frame_token that is different from the
   // guest's main frame (that is about to be attached to it). When we swap
@@ -7182,7 +7198,8 @@
     // stay around but it will no longer be associated with a RenderFrame.
     // Ensure there are no lingering child frames before marking the frame
     // deleted - this matters for compromised renderers (see
-    // https://crbug.com/517241992).
+    // https://crbug.com/517241992). This is a defense-in-depth call that
+    // mirrors a similar call in SwapOuterDelegateFrame().
     ResetChildren();
     RenderFrameDeleted();
     return;
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 7a4372a9..1cafdb7 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -80,6 +80,8 @@
 #include "device/gamepad/public/mojom/gamepad.mojom.h"
 #include "ipc/constants.mojom.h"
 #include "mojo/core/embedder/embedder.h"
+#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -2963,6 +2965,82 @@
   EXPECT_EQ(0U, subframe->child_count());
 }
 
+// Test that the placeholder frame for an inner delegate has its children
+// cleared synchronously at attach time, without depending on the renderer's
+// DidUnloadRenderFrame ACK (which a misbehaving renderer may withhold), and
+// that the placeholder cannot acquire new children after the inner delegate is
+// attached. See https://crbug.com/518078552.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       NoChildrenOnInnerDelegatePlaceholderAfterAttach) {
+  // Start on a page with a blank iframe, simulating the normal starting point
+  // of attaching an inner delegate (e.g., for MimeHandlerView) to a placeholder
+  // subframe.
+  GURL main_url(
+      embedded_test_server()->GetURL("a.com", "/page_with_blank_iframe.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  RenderFrameHostImpl* main_frame = web_contents->GetPrimaryMainFrame();
+  RenderFrameHostImpl* subframe = main_frame->child_at(0)->current_frame_host();
+
+  // Add a child frame to the subframe so that we can test if it gets properly
+  // detached. A compromised renderer could create this child.
+  EXPECT_TRUE(ExecJs(subframe,
+                     "let f = document.createElement('iframe'); "
+                     "document.body.appendChild(f);"));
+  ASSERT_EQ(1U, subframe->child_count());
+  RenderFrameHostImpl* grandchild = subframe->child_at(0)->current_frame_host();
+  RenderFrameHostWrapper grandchild_observer(grandchild);
+
+  // Swallow the unload ACK so that any cleanup observed below is solely due to
+  // browser-side attach logic and not the renderer's DidUnloadRenderFrame ACK.
+  subframe->SetUnloadACKCallbackForTesting(
+      base::BindRepeating([]() { return true; }));
+
+  // Attach an inner delegate. This synchronously calls SwapOuterDelegateFrame()
+  // which sends the Unload IPC, and it sets is_inner_delegate_attached().
+  EXPECT_TRUE(CreateAndAttachInnerContents(subframe));
+  EXPECT_TRUE(subframe->frame_tree_node()
+                  ->render_manager()
+                  ->is_inner_delegate_attached());
+  EXPECT_TRUE(subframe->inner_tree_main_frame_tree_node_id());
+
+  // The placeholder frame must have no children immediately after attach,
+  // independently of whether the renderer ACKs the unload.
+  EXPECT_TRUE(grandchild_observer.IsRenderFrameDeleted());
+  EXPECT_EQ(0U, subframe->child_count());
+
+  // Simulate the renderer attempting to create a new child on the placeholder
+  // frame after the inner delegate has been attached. The placeholder's render
+  // frame is still considered created in the browser (since the unload ACK was
+  // swallowed above), so OnCreateChildFrame()'s lifecycle checks alone would
+  // not reject this; the dedicated inner-delegate check should drop it.
+  ASSERT_TRUE(subframe->IsRenderFrameLive());
+  mojo::AssociatedRemote<mojom::Frame> frame_remote;
+  std::ignore = frame_remote.BindNewEndpointAndPassDedicatedReceiver();
+  mojo::PendingRemote<blink::mojom::BrowserInterfaceBroker> bib_remote;
+  mojo::AssociatedReceiver<blink::mojom::AssociatedInterfaceProvider>
+      associated_interface_provider_receiver(nullptr);
+  std::ignore = associated_interface_provider_receiver
+                    .BindNewEndpointAndPassDedicatedRemote();
+  subframe->OnCreateChildFrame(
+      subframe->GetProcess()->GetNextRoutingID(), frame_remote.Unbind(),
+      bib_remote.InitWithNewPipeAndPassReceiver(),
+      blink::mojom::PolicyContainerBindParams::New(
+          mojo::PendingAssociatedRemote<blink::mojom::PolicyContainerHost>()
+              .InitWithNewEndpointAndPassReceiver()),
+      associated_interface_provider_receiver.Unbind(),
+      blink::mojom::TreeScopeType::kDocument, "", "uniqueName1",
+      /*is_created_by_script=*/false, blink::LocalFrameToken(),
+      base::UnguessableToken::Create(), blink::DocumentToken(),
+      blink::FramePolicy(), blink::mojom::FrameOwnerProperties(),
+      blink::FrameOwnerElementType::kIframe, ukm::kInvalidSourceId);
+
+  // The placeholder frame must still have no children.
+  EXPECT_EQ(0U, subframe->child_count());
+}
+
 class BeginNavigationTransitionReplacer : public FrameHostInterceptor {
  public:
   BeginNavigationTransitionReplacer(WebContents* web_contents,

diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 7a4372a9..1cafdb7 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -80,6 +80,8 @@
 #include "device/gamepad/public/mojom/gamepad.mojom.h"
 #include "ipc/constants.mojom.h"
 #include "mojo/core/embedder/embedder.h"
+#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -2963,6 +2965,82 @@
   EXPECT_EQ(0U, subframe->child_count());
 }
 
+// Test that the placeholder frame for an inner delegate has its children
+// cleared synchronously at attach time, without depending on the renderer's
+// DidUnloadRenderFrame ACK (which a misbehaving renderer may withhold), and
+// that the placeholder cannot acquire new children after the inner delegate is
+// attached. See https://crbug.com/518078552.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       NoChildrenOnInnerDelegatePlaceholderAfterAttach) {
+  // Start on a page with a blank iframe, simulating the normal starting point
+  // of attaching an inner delegate (e.g., for MimeHandlerView) to a placeholder
+  // subframe.
+  GURL main_url(
+      embedded_test_server()->GetURL("a.com", "/page_with_blank_iframe.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  RenderFrameHostImpl* main_frame = web_contents->GetPrimaryMainFrame();
+  RenderFrameHostImpl* subframe = main_frame->child_at(0)->current_frame_host();
+
+  // Add a child frame to the subframe so that we can test if it gets properly
+  // detached. A compromised renderer could create this child.
+  EXPECT_TRUE(ExecJs(subframe,
+                     "let f = document.createElement('iframe'); "
+                     "document.body.appendChild(f);"));
+  ASSERT_EQ(1U, subframe->child_count());
+  RenderFrameHostImpl* grandchild = subframe->child_at(0)->current_frame_host();
+  RenderFrameHostWrapper grandchild_observer(grandchild);
+
+  // Swallow the unload ACK so that any cleanup observed below is solely due to
+  // browser-side attach logic and not the renderer's DidUnloadRenderFrame ACK.
+  subframe->SetUnloadACKCallbackForTesting(
+      base::BindRepeating([]() { return true; }));
+
+  // Attach an inner delegate. This synchronously calls SwapOuterDelegateFrame()
+  // which sends the Unload IPC, and it sets is_inner_delegate_attached().
+  EXPECT_TRUE(CreateAndAttachInnerContents(subframe));
+  EXPECT_TRUE(subframe->frame_tree_node()
+                  ->render_manager()
+                  ->is_inner_delegate_attached());
+  EXPECT_TRUE(subframe->inner_tree_main_frame_tree_node_id());
+
+  // The placeholder frame must have no children immediately after attach,
+  // independently of whether the renderer ACKs the unload.
+  EXPECT_TRUE(grandchild_observer.IsRenderFrameDeleted());
+  EXPECT_EQ(0U, subframe->child_count());
+
+  // Simulate the renderer attempting to create a new child on the placeholder
+  // frame after the inner delegate has been attached. The placeholder's render
+  // frame is still considered created in the browser (since the unload ACK was
+  // swallowed above), so OnCreateChildFrame()'s lifecycle checks alone would
+  // not reject this; the dedicated inner-delegate check should drop it.
+  ASSERT_TRUE(subframe->IsRenderFrameLive());
+  mojo::AssociatedRemote<mojom::Frame> frame_remote;
+  std::ignore = frame_remote.BindNewEndpointAndPassDedicatedReceiver();
+  mojo::PendingRemote<blink::mojom::BrowserInterfaceBroker> bib_remote;
+  mojo::AssociatedReceiver<blink::mojom::AssociatedInterfaceProvider>
+      associated_interface_provider_receiver(nullptr);
+  std::ignore = associated_interface_provider_receiver
+                    .BindNewEndpointAndPassDedicatedRemote();
+  subframe->OnCreateChildFrame(
+      subframe->GetProcess()->GetNextRoutingID(), frame_remote.Unbind(),
+      bib_remote.InitWithNewPipeAndPassReceiver(),
+      blink::mojom::PolicyContainerBindParams::New(
+          mojo::PendingAssociatedRemote<blink::mojom::PolicyContainerHost>()
+              .InitWithNewEndpointAndPassReceiver()),
+      associated_interface_provider_receiver.Unbind(),
+      blink::mojom::TreeScopeType::kDocument, "", "uniqueName1",
+      /*is_created_by_script=*/false, blink::LocalFrameToken(),
+      base::UnguessableToken::Create(), blink::DocumentToken(),
+      blink::FramePolicy(), blink::mojom::FrameOwnerProperties(),
+      blink::FrameOwnerElementType::kIframe, ukm::kInvalidSourceId);
+
+  // The placeholder frame must still have no children.
+  EXPECT_EQ(0U, subframe->child_count());
+}
+
 class BeginNavigationTransitionReplacer : public FrameHostInterceptor {
  public:
   BeginNavigationTransitionReplacer(WebContents* web_contents,