4c43686c30  Fix ContainerNode::replaceAll not removing existing children when inserting an Element

The bug at a glance

Medium. This is a DOM tree-consistency logic error in ContainerNode::replaceAll, not a directly reachable memory-safety bug. OBSERVED: the commit message itself states the bug is currently unreachable because every existing caller of replaceAll passes either a Text node or nullptr, never an Element, so the short-circuit that skips child removal is never taken in shipping code. The severity is bounded by that latency: no current attacker path. INFERRED: the fix is defensive against a near-future regression — an existing FIXME in HTMLElement::setInnerText() proposes routing Element inserts through replaceAll(), which would immediately activate the latent bug and leave stale, still-parented children in the tree while the insertion logic believes all children were replaced, a classic setup for later use-after-free or double-parenting confusion.

A refactor (288944@main) inlined a side-effecting call into the right-hand side of a boolean || expression, so C++ short-circuit evaluation silently suppressed the child-removal side effect whenever the left operand (is<Element>(*node)) was already true.

Root cause

ContainerNode::replaceAll() is responsible for atomically replacing all of a node’s children with a single new node — the primitive underlying operations like setting textContent. Correct behavior requires two things to happen unconditionally: (1) every existing child must be removed via removeAllChildrenWithScriptAssertionMaybeAsync(), which runs the full script-assertion / mutation-observer machinery and returns whether any removed nodes were Elements; and (2) the new node must be inserted, tagged with a ReplacedAllChildren enum recording whether the resulting subtree includes Elements (which affects downstream style/layout invalidation).

The regressing refactor collapsed both concerns into one expression: auto replacedAllChildren = is<Element>(*node) || removeAllChildrenWithScriptAssertionMaybeAsync(…).didRemoveElements == DidRemoveElements::Yes ? ReplacedAllChildren::YesIncludingElements : ReplacedAllChildren::YesNotIncludingElements. The author’s intent was purely to compute the enum. But removeAllChildrenWithScriptAssertionMaybeAsync() is not a pure predicate — calling it is the mechanism that actually detaches the existing children. Under C++ short-circuit rules, when is<Element>(*node) evaluates true, the right operand of || is never evaluated, so removeAllChildrenWithScriptAssertionMaybeAsync() is never called and the old children are never removed.

The result: replaceAll() would insert the new Element alongside the pre-existing children rather than in place of them, while still reporting YesIncludingElements to executeNodeInsertionWithScriptAssertion(). The tree ends up in a state the caller’s bookkeeping does not expect — stale children remain parented under a node the engine considers freshly replaced. If replaceAll were reachable with an Element argument, that divergence between believed and actual tree shape is the seed of subsequent lifetime confusion.

The fix hoists the call into its own unconditional statement, auto removeResult = removeAllChildrenWithScriptAssertionMaybeAsync(…), then consumes only its result in the enum computation, guaranteeing the removal side effect always executes regardless of the inserted node’s type.

Key code

ContainerNode::replaceAll — removal split out of the || so it always runs

-    auto replacedAllChildren = is<Element>(*node) || removeAllChildrenWithScriptAssertionMaybeAsync(ChildChange::Source::API, removedChildren, DeferChildrenChanged::No).didRemoveElements == DidRemoveElements::Yes
+    auto removeResult = removeAllChildrenWithScriptAssertionMaybeAsync(ChildChange::Source::API, removedChildren, DeferChildrenChanged::No);
+    auto replacedAllChildren = is<Element>(*node) || removeResult.didRemoveElements == DidRemoveElements::Yes
         ? ReplacedAllChildren::YesIncludingElements : ReplacedAllChildren::YesNotIncludingElements;

Patch walkthrough

• Source/WebCore/dom/ContainerNode.cpp — In ContainerNode::replaceAll(), the single combined statement is split into two. removeAllChildrenWithScriptAssertionMaybeAsync(ChildChange::Source::API, removedChildren, DeferChildrenChanged::No) is called first and stored in removeResult, so its child-removal side effect is now unconditional. The subsequent ternary reads is<Element>(*node) || removeResult.didRemoveElements == DidRemoveElements::Yes only to choose the ReplacedAllChildren enum value, which is pure. The observable behavior is unchanged for today’s Text/nullptr callers but becomes correct for Element inserts.

Background

ContainerNode::replaceAll — Internal DOM helper that replaces all children of a node with a single new node in one mutation, used as the backend for textContent-style bulk replacement.

Short-circuit evaluation — In C++, the right operand of || is not evaluated when the left operand is true. Placing a side-effecting call there makes the side effect conditional on the left operand being false.

removeAllChildrenWithScriptAssertionMaybeAsync — Detaches all existing children while honoring script-run assertions and mutation observers; returns a struct whose didRemoveElements field reports whether any removed node was an Element.

ReplacedAllChildren enum — Passed to executeNodeInsertionWithScriptAssertion to signal whether the post-replacement subtree contains Elements, driving downstream style and layout invalidation decisions.

288944@main — The prior WebKit revision that introduced the regression by inlining the removal call into the boolean expression.

Vulnerability window

1. Baseline — replaceAll() removes existing children in a standalone statement, then inserts the new node.
2. Regression (288944@main) — A refactor inlines removeAllChildrenWithScriptAssertionMaybeAsync() into the RHS of an || used to compute the ReplacedAllChildren enum.
3. Latent — All shipping callers pass Text or nullptr, so is<Element>(*node) is false and the RHS still runs — the bug is dormant, never triggered in practice.
4. Discovery — Code review notices the short-circuit would skip removal for Element inserts, and that HTMLElement::setInnerText() has a FIXME proposing exactly such a caller.
5. Fix (313912@main) — The removal call is hoisted to its own unconditional statement; the enum computation keeps only the pure comparison.

Triggering

No test added; the commit explicitly states this fixes nothing observable today because no caller passes an Element. A conceptual trigger would require calling ContainerNode::replaceAll with an Element node while the container already has children (e.g. via the proposed HTMLElement::setInnerText() rework); the old children would remain in the tree after the call instead of being removed.

Exploitation

1. Not currently exploitable — Unreachable in shipping code: no caller passes an Element, so the skipped-removal branch never executes.
2. Hypothetical activation — Were setInnerText() (or any new API) to route Element insertion through replaceAll, the container would retain its old children while the engine records a full replacement.
3. Downstream corruption — The divergence between the believed replaced tree and the actual over-populated tree could drive stale-node references in style/layout, a foundation for use-after-free if those children are later freed under the assumption they were detached.

Detection & hunting

For defenders and SOC / detection engineers:

• Callers of ContainerNode::replaceAll with non-Text arguments —
• Post-replaceAll child count —

Audit directions

• Side-effecting calls inside boolean expressions —
• HTMLElement::setInnerText FIXME —
• ReplacedAllChildren consumers —