b7bbb825266afcbe86fb36e734e21b6f630945c7  [Site Isolation] Same-site grandchild frame is not detached when its cross-site parent frame is detached

The bug at a glance

RemoteFrame::frameDetached() had no re-entrancy guard, yet it runs client/window/page teardown that can execute web-content unload handlers which re-enter detach on the same frame; the patch adds an m_hasStartedDetaching std::exchange guard and reorders descendant teardown. The proven defect is a re-entrant double-teardown of a site-isolation frame-tree node in the WebContent process (a frame-lifetime ordering error); a use-after-free is the natural consequence of re-running detachFromPage()/detach on an already-dismantled frame and its children, and triage severity high is defensible. Because Frame is ref-counted (RefCountedAndCanMakeWeakPtr) and the diff does not show the exact freed access, the UAF/RCE escalation is marked (inferred).

The tell is the one-line comment ‘Unload handlers can re-enter.’ guarding the detach entry, plus a bespoke detachLocalDescendants() that deliberately only destroys provisional frames for remote children ’to not unload a same-site grandchild before its parent’ — classic frame-lifetime re-entrancy and teardown-ordering bugs.

Root cause

The violated invariant is that RemoteFrame::frameDetached() performs its teardown sequence exactly once and in an order where a parent is torn down before (not after) its descendants’ unload side effects run. Pre-patch frameDetached() immediately called m_client->frameDetached(); m_window->frameDetached(); detachFromPage(). Each of these can synchronously run script: detaching/unloading descendant LocalFrames fires unload/pagehide handlers, and WebRemoteFrameClient::frameDetached() calls checkCompleted() on the owner document, any of which can mutate the frame tree.

The concrete pre-patch re-entrancy chain is: 1. frameDetached() enters on RemoteFrame R. 2. it calls into m_client/m_window/detachFromPage, which dispatches an unload handler in a descendant local frame. 3. that handler removes a frame or navigates, driving the removal path (WebPageProxy::didDestroyFrame -> FrameWasRemovedInAnotherProcess -> WebFrame::removeFromTree, or an ancestor teardown) that re-enters RemoteFrame::frameDetached() on R. 4. the second entry re-runs detachFromPage() and the child teardown on R whose page/tree links are already being cleared, operating on half-dismantled or released frame-tree nodes. Because Frame lifetime is Ref/RefPtr (Frame : RefCountedAndCanMakeWeakPtr), dropping the last reference during the first teardown can free a child, and the second pass then touches it.

The fix makes the entry idempotent with ‘if (std::exchange(m_hasStartedDetaching, true)) return;’ and factors descendant teardown into detachLocalDescendants(), which snapshots children into a Vector<Ref> (the Ref keeps each child alive across the loop), iterates lastChild->previousSibling, calls loader().detachFromParent() on LocalFrame children, and for RemoteFrame children calls client().destroyProvisionalFrame() then recurses detachLocalDescendants() rather than fully detaching — precisely so a same-site grandchild is not unloaded before its parent. A new RemoteFrameClient::destroyProvisionalFrame() virtual (implemented by WebRemoteFrameClient via WebFrame::destroyProvisionalFrame) supports this. Separately WebFrame::removeFromTree() now calls coreFrame->disconnectOwnerElement() for RemoteFrames on the remote-ancestor removal path that skips detachedFromParent2().

Key code

Re-entrancy guard and ordered local-descendant teardown

void RemoteFrame::frameDetached()
{
    // Unload handlers can re-enter.
    if (std::exchange(m_hasStartedDetaching, true))
        return;
    detachLocalDescendants();
    m_client->frameDetached();
    m_window->frameDetached();
    detachFromPage();
}

void RemoteFrame::detachLocalDescendants()
{
    Vector<Ref<Frame>> children;
    children.reserveInitialCapacity(tree().childCount());
    for (RefPtr child = tree().lastChild(); child; child = child->tree().previousSibling())
        children.append(*child);
    for (auto& child : children) {
        if (RefPtr localChild = dynamicDowncast<LocalFrame>(child.get()))
            localChild->loader().detachFromParent();
        else {
            // Detaching it would unload a same-site grandchild before its parent.
            Ref remoteChild = downcast<RemoteFrame>(child.get());
            remoteChild->client().destroyProvisionalFrame();
            remoteChild->detachLocalDescendants();
        }
    }
}

Fix approach

A single bool member m_hasStartedDetaching, flipped with std::exchange at the top of frameDetached(), makes the function a no-op on every re-entry so the client/window/page teardown runs exactly once. The new detachLocalDescendants() explicitly tears down descendants before the frame’s own teardown and in a safe order: local children are detached via their FrameLoader, while remote children only have their provisional frame destroyed and are recursed into, so a same-site grandchild is never unloaded ahead of its parent. A Vector<Ref> snapshot pins every child with a strong ref for the duration of the loop, so mutations driven by unload handlers cannot free a child mid-iteration. WebFrame::removeFromTree() additionally disconnects a RemoteFrame’s owner element on the remote-ancestor removal path (which skips detachedFromParent2) to keep instrumentation/owner teardown idempotent. After the fix a re-entrant detach collapses to an immediate return.

Patch walkthrough

• Source/WebCore/page/RemoteFrame.cpp — Adds the std::exchange(m_hasStartedDetaching) re-entrancy guard and the new detachLocalDescendants() that orders local-child and remote-child teardown; includes FrameLoader.h for detachFromParent().
• Source/WebCore/page/RemoteFrame.h — Declares detachLocalDescendants() and adds the bool m_hasStartedDetaching { false } member.
• Source/WebCore/page/RemoteFrameClient.h — Adds the pure-virtual destroyProvisionalFrame() so RemoteFrame can ask the client to tear down a remote child’s provisional frame.
• Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.{cpp,h} — Implements destroyProvisionalFrame() by forwarding to m_frame->destroyProvisionalFrame().
• Source/WebKit/WebProcess/WebPage/WebFrame.cpp — In removeFromTree(), for a RemoteFrame on the remote-ancestor removal path (which skips detachedFromParent2), calls disconnectOwnerElement() before removing from the parent tree, keeping the two removal paths idempotent.

Background

RemoteFrame — In site isolation, the in-process stand-in for a frame whose document lives in another WebContent process; it still participates in the local frame tree and must be detached cleanly.

frameDetached lifecycle — The teardown that notifies the client and window and calls detachFromPage(); it can synchronously run script (unload handlers, checkCompleted) that mutates the tree.

Unload handler re-entrancy — beforeunload/unload/pagehide handlers execute during teardown and can remove or navigate frames, re-entering detach on a frame already mid-teardown.

Ref vs RefPtr (Frame lifetime) — Frame derives from RefCountedAndCanMakeWeakPtr; the Vector<Ref> snapshot holds strong references so children survive the loop even as handlers drop other references.

Provisional frame — A not-yet-committed frame for a cross-process navigation; remote children are torn down via destroyProvisionalFrame() plus recursion rather than a full detach to preserve ordering.

detachedFromParent2 / removeFromTree — Two frame-removal paths; the remote-ancestor path (frameWasRemovedInAnotherProcess -> removeFromTree) skips detachedFromParent2, so owner-element/instrumentation cleanup must be duplicated there idempotently.

Vulnerability window

1. Setup — Attacker page builds a site-isolated frame tree: a RemoteFrame with local and/or remote descendants, some carrying unload/pagehide handlers.
2. Trigger — The frame is detached (iframe removed, ancestor navigated/removed cross-process), invoking RemoteFrame::frameDetached().
3. Re-enter — Teardown dispatches a descendant unload handler that removes/navigates a frame, re-entering frameDetached() on the same RemoteFrame before the first pass completes.
4. Corruption (inferred) — The second pass re-runs detachFromPage()/descendant teardown on a half-dismantled frame and its possibly-freed children, i.e. a double-detach / use-after-free of frame-tree state.
5. Fix — m_hasStartedDetaching short-circuits the re-entry and detachLocalDescendants() enforces parent-before-grandchild ordering.

Proof of concept

No regression test is included. Conceptual trigger (web path inferred): with site isolation enabled, create a cross-process iframe hierarchy where a descendant local frame installs a pagehide/unload handler that, when fired, synchronously removes an ancestor iframe (or navigates it cross-process). Then remove/navigate the top of that subtree to start RemoteFrame::frameDetached(); the descendant handler re-enters detach on the same RemoteFrame. The crash/corruption manifests on the second, now-removed teardown pass.

Exploitation

1. Reach — Reachable from ordinary web content that can create nested cross-origin iframes with unload/pagehide handlers under site isolation.
2. Trigger — Drive a frame detach whose teardown runs a descendant unload handler that re-enters detach on the same RemoteFrame.
3. Primitive — Primary: re-entrant double-teardown of a RemoteFrame; secondary: operating on descendant frames whose last Ref may have been dropped in the first pass (dangling frame-tree node).
4. Escalation (inferred) — Open questions gating anything past a crash: can the unload storm be shaped to free a specific child/owner object and then reclaim its allocation before the second pass dereferences it, and can the re-entrant path be repeated to widen the window; the diff does not demonstrate the freed access or a controllable reclaim.

Detection & hunting

For defenders and detection engineers:

• Re-entrant frameDetached — Instrument RemoteFrame::frameDetached() to assert/log when entered while m_hasStartedDetaching is already set; pre-patch builds will show re-entry under unload handlers.
• Crashes in detachFromPage / frame-tree teardown — Triage ASAN/ to-be-freed reports in RemoteFrame detach, FrameTree removeChild, or RemoteDOMWindow::frameDetached during iframe removal with active unload handlers.
• Owner-element double cleanup — Watch for owner-element/instrumentation operations on an already-disconnected RemoteFrame on the removeFromTree path.

Remaining risk in the patch

• Guard is per-frame only — m_hasStartedDetaching protects a single RemoteFrame; an unload storm that re-enters detach across sibling or ancestor frames is not serialized by this one bit, so ordering across multiple frames still depends on detachLocalDescendants() being correct.
• Remote children are not fully detached here — For remote children the fix only calls destroyProvisionalFrame() and recurses; a committed (non-provisional) remote child’s own state is relied upon to be torn down elsewhere, which is an ordering assumption worth verifying.
• Unverified callee bodies — FrameLoader::detachFromParent(), WebFrame::destroyProvisionalFrame(), detachFromPage(), and disconnectOwnerElement() are not in the supplied diff, so the actual free/ownership behavior they trigger during the single pass is unverified.

Audit directions

• Other detach entry points — Audit LocalFrame/FrameLoader detach and DOMWindow frameDetached paths for the same missing re-entrancy guard against unload-driven re-entry.
• Teardown that runs script — Find teardown functions that call client/window callbacks or checkCompleted() before clearing their own state, where re-entry could double-process.
• Remote-child recursion — Verify destroyProvisionalFrame() + detachLocalDescendants() recursion fully handles non-provisional remote children and deep same-site grandchild chains.
• removeFromTree idempotency — Confirm both the detachedFromParent2 and remote-ancestor removeFromTree paths disconnect owner element, view, and instrumentation exactly once.