[6] Range boundary-point lock discipline follow-up

MediumWebCore DOMRace

621e3bf

Medium, and only because the lock it tightens already exists — this is review follow-up on the Range locking work, closing a window where a concurrent reader could observe a start/end pair drawn from two different states. Whether that window has real teeth depends on whether the GC-side reader retains bare node pointers past the synchronized region.

A live DOM Range keeps two endpoints — a start and an end — that must satisfy one relationship: start comes before or equals end in document order. When script sets one endpoint past the other, the specification requires collapsing the range onto the newly set endpoint. That repair is a second write, and the two writes together form one logical update; a reader that lands between them sees an inverted range, a configuration the code itself treats as invalid. The endpoints are guarded by m_boundaryPointLock, and the seven static boundary* helpers that mutation notifications call all assume — but until this commit did not declare — that their caller holds it.

The angle: for defensive value — this closes the intermediate-state window a marking thread could be scheduled into, and turns an unstated lock precondition into something a future refactor cannot silently drop.

Unreviewed. Addressing the review comments in the original PR.

Source/WebCore/dom/Range.cpp

Patch Details

Two distinct changes, both in Range.cpp.

First, Range::setStart and Range::setEnd no longer split a single logical update across two separate Locker { m_boundaryPointLock } scopes. Previously the code took the lock, wrote m_start (resp. m_end), released the lock, evaluated treeOrder(makeBoundaryPoint(m_start), makeBoundaryPoint(m_end)), and — if the range had become inverted — re-acquired the lock to collapse the other endpoint. Now the ordering comparison is hoisted above the critical section and computed from the incoming arguments (BoundaryPoint(container.copyRef(), offset)) against the current opposite endpoint, storing the result in a bool shouldAlsoSetEnd / bool shouldAlsoSetStart; both m_start.set(...) and the conditional m_end = m_start (resp. m_start = m_end) then happen inside one critical section. Note the hoisted predicate still reads the opposite endpoint outside the lock — the patch folds the write side into one critical section, it does not add locking to that read.

Second, seven static boundary-mutation helpers — boundaryNodeChildrenChanged, boundaryNodeChildrenWillBeRemoved, boundaryNodeWillBeRemoved, boundaryTextInserted, boundaryTextRemoved, boundaryTextNodesMerged, boundaryTextNodesSplit — gain an unnamed leading Locker<Lock>& parameter, and their callers (Range::nodeChildrenChanged, nodeChildrenWillBeRemoved, nodeWillBeRemoved, textInserted, textRemoved, textNodesMerged, textNodeSplit) pass their local locker. This is lock-discipline plumbing: it makes "caller must hold m_boundaryPointLock" part of each helper's signature rather than an unstated convention. No functional change to the helper bodies.

A multi-field update split across two separate critical sections, letting a concurrent reader observe an intermediate state that no single-threaded observer can reach.

Background

Live Range and its endpoints. A DOM Range obtained from document.createRange() stores its endpoints as two RangeBoundaryPoint members, m_start and m_end. RangeBoundaryPoint holds Ref<Node> m_container, unsigned m_offset, and RefPtr<Node> m_childBefore; because it holds a strong Ref, the container node stays alive as long as the boundary point references it.

Mutation notifications. When the DOM tree changes, Document notifies every attached live range through Range::nodeChildrenChanged, nodeChildrenWillBeRemoved, nodeWillBeRemoved, textInserted, textRemoved, textNodesMerged, and textNodeSplit, each of which fixes up both boundary points via a static boundary* helper.

NODELETE. An annotation applied to some of these helpers in Range.cpp, marking code paths that must not trigger object destruction. The helpers that carry it are exactly those whose visible bodies only adjust offsets, while the unannotated ones reassign a boundary's container.

Non-thread-safe refcounting. WebCore Node reference counts are not atomic, so in general only the main thread may increment or decrement them.

GC-thread visiting. JSC's garbage collector traces the heap on dedicated marking threads. DOM wrappers participate through visitAdditionalChildrenInGCThread; for Range this is JSRange::visitAdditionalChildrenInGCThread, which calls Range::visitNodesInGCThread(visitor). Opaque roots are the mechanism by which a wrapper tells the collector "keep this native subgraph's wrappers alive."

Lock / Locker. WTF's mutex and its RAII scoped-acquisition helper. Range declares mutable Lock m_boundaryPointLock. A Locker releases the lock when its enclosing scope exits.

treeOrder. treeOrder(BoundaryPoint, BoundaryPoint) returns a std::partial_ordering describing which of two boundary points comes first in document order; is_lteq tests for "before or equal." The DOM specification requires setStart/setEnd to collapse the range onto the newly set endpoint when the resulting range would be inverted.

Analysis

The root cause is a lock-granularity defect: m_boundaryPointLock already existed and already guarded the individual writes, but a single logical two-endpoint update was performed as two critical sections with an unlocked gap between them.

  Main thread (pre-patch setStart)      Marking thread
  ────────────────────────────────      ──────────────
  Locker { lock }
    m_start.set(new)   ← drops
      m_start's OLD container ref
  } release
                                        ── scheduled here ──
                                        reads m_start (new)
                                        reads m_end   (stale)
                                        => inverted pair: a state
                                           no single-threaded
                                           caller can observe
  treeOrder(m_start, m_end) -> inverted
  Locker { lock }
    m_end = m_start
  }

Be precise about which reference is released where. In pre-patch setStart, the first critical section's m_start.set(...) drops m_start's own previous container reference; m_end keeps its Ref<Node> intact throughout the gap and is only overwritten — dropping its old container reference — by the second block. So the intermediate state exposed in the gap is (a) the inverted start/end pair and (b) the release of m_start's previous container, not a dangling m_end. Post-patch, both the write to m_start and the conditional collapse of m_end occur under a single Locker, so no reader can be scheduled between them.

The reader in question is JSRange::visitAdditionalChildrenInGCThread calling wrapped().visitNodesInGCThread(visitor) — a JSC marking thread reading a live Range's boundary points off the main thread. Whether the GC-side reader takes m_boundaryPointLock itself is what decides how much teeth this window has, and this patch does not touch Range::visitNodesInGCThread — so whether a genuinely unsynchronized concurrent reader remains is an open question rather than something the change settles.

The patch closes the write-side tear only. The predicate that decides whether to collapse still reads the opposite endpoint outside the lock: post-patch setStart evaluates makeBoundaryPoint(m_end) before acquiring the lock, and setEnd evaluates makeBoundaryPoint(m_start) before acquiring it. The residual shape is "unlocked read of one endpoint, then locked atomic write of both," not a fully serialized read-modify-write. Whether that residual read matters depends on which other threads may write those fields; the mutation-notification paths in this file all write under the lock on the main thread.

Where the race would have teeth is worth mapping precisely. Four of the seven boundary helpers — boundaryNodeChildrenWillBeRemoved, boundaryNodeWillBeRemoved, boundaryTextNodesMerged, boundaryTextNodesSplit — are not annotated NODELETE, unlike boundaryNodeChildrenChanged, boundaryTextInserted, and boundaryTextRemoved, and those four are exactly the ones whose visible bodies reassign a boundary's container and can therefore release a node reference. That partition is the map of where a reference release, and therefore a stale-pointer read, could occur.

Discovery is most likely code review. The change has the shape of review follow-up on an earlier locking patch: a reviewer noticing that setStart/setEnd performed one logical update across two critical sections, and that the boundary* helpers carried their lock precondition only by convention. The underlying race that motivated the original locking work was plausibly found either by auditing the surface reachable from JSC's marking threads — enumerating visitAdditionalChildrenInGCThread implementors and checking which read mutable main-thread state — or from a crash-report / ThreadSanitizer signal on a marking thread; which of the two is not determined here.

This defect weakens the consistency guarantee at the boundary between the main (mutator) thread and JSC's marking threads inside the WebContent process. The assumption at stake is that DOM state read from a GC thread is either immutable for the duration of the read or serialized as a whole transaction by a lock — the assumption m_boundaryPointLock exists to uphold. Before the fix, a marking thread scheduled in the unlocked gap could read a start/end pair composed of one updated and one stale endpoint, and the node whose reference the mutator released in the first critical section could be one the reader had already sampled as a bare pointer. If a reader that holds only unpinned node pointers exists, an attacker who reliably won such a race would hold a cross-thread stale-pointer read on DOM nodes in the renderer; that would remain confined to the WebContent sandbox and would still require a separate escape.

Insight: the restructuring is only possible because the predicate can be evaluated against the incoming arguments (BoundaryPoint(container.copyRef(), offset)) rather than against the post-write value of m_start — recognising that equivalence is what let the author collapse two critical sections into one instead of widening the lock to cover the comparison. The second half — threading an unnamed Locker<Lock>& through every helper — is a cheap type-level way to encode a lock precondition in a codebase where the mutating helpers are free functions a future refactor could easily call from an unlocked path. It does not prove the right lock is held, which is what WTF_GUARDED_BY_LOCK / assertIsHeld would add, but it makes the requirement impossible to overlook at the call site.

Audit directions

• A logically atomic multi-field update split across two or more critical sections, so a concurrent reader can observe a state no single-threaded caller can reach. This is dangerous because each individual write looks correctly synchronized under review and under a race detector's per-access model — only the composition is broken, and the intermediate state is often one the code explicitly considers invalid. Narrow: grep Source/WebCore/dom and Source/WebCore/editing for functions containing two or more Locker declarations over the same lock, or a Locker scope followed by an unlocked read of the same members — Range::collapse and FrameSelection's live-range synchronization are the immediate neighbours of the code changed here. In code review, two lock-acquire statements guarding writes to members that a single invariant relates — a start/end pair, a pointer/length pair, a buffer/capacity pair — is the tell. Wider: the same shape appears with any release-then-reacquire idiom — a helper that takes the lock internally and is called twice in a row, unlockEarly()-style manual releases, or a scope guard released before the second write; look for any "compute predicate, then repair state" pattern where the predicate is evaluated between the writes. Widest: a concurrent tracer or reader must observe multi-field state only at transaction boundaries, holding in V8's concurrent marking write barriers, Go's GC write barriers, Java's ZGC/Shenandoah load barriers, and Linux-kernel RCU readers — carry the question "is there a moment where a tracer sees field A from state N+1 and field B from state N, and does anything downstream assume they agree?"

• A second thread reading a data structure whose lifetime is managed by non-thread-safe reference counting, where the pinning primitive that would normally make the read safe is unavailable — so the only thing keeping the pointee alive is a lock held by the owning thread, and every unlocked mutation on the owning side becomes a candidate cross-thread stale-pointer read rather than a mere visibility issue. Narrow: enumerate the WebCore classes that implement visitAdditionalChildrenInGCThread (grep for DEFINE_VISIT_ADDITIONAL_CHILDREN_IN_GC_THREAD across Source/WebCore/bindings/js) and, for each, check that every mutation path touching the members that visitor reads is serialized against it — JSRangeCustom.cpp is the instance touched here, and the first thing to establish for it is whether Range::visitNodesInGCThread itself acquires m_boundaryPointLock. Wider: any WebCore state read off the main thread through a different channel — opaque-root computation, WebCoreOpaqueRoot helpers, and any main-thread object sampled by a background thread while holding only a Node*/Element*; the tell is a non-main-thread function that dereferences a type whose ref()/deref() are non-atomic. Widest: if a reader thread cannot take a strong reference to what it reads, the writer's reference release must be inside the same critical section as the reader's dereference — transferring to Rust's Rc vs Arc split, to COM single-threaded apartments, and to any C++ codebase mixing boost::intrusive_ptr with non-atomic counters.

• A locking precondition expressed only as convention — a free function or private helper that mutates lock-guarded state but whose signature says nothing about the lock. New call sites added later have no signal that they must acquire anything, so the discipline erodes silently across refactors. Narrow: grep Source/WebCore/dom for static inline void boundary-style free helpers that take a reference to a member of a lock-holding class and mutate it without a Locker& parameter or an assertIsHeld call; the seven helpers converted in this commit are the template for the compliant shape. Wider: audit WTF Lock users for guarded members lacking WTF_GUARDED_BY_LOCK annotations — the annotation and the Locker&-parameter idiom are alternative encodings of the same contract, and classes using neither are where the next unlocked path will appear; Range.h's m_start/m_end are a concrete follow-up. Widest: a lock precondition not encoded in a type or checked by an analyzer will eventually be violated by a caller who never read the comment — applying to any codebase with Clang thread-safety analysis available but unapplied, to Java's @GuardedBy, and inversely to Rust's Mutex<T>, where encapsulation makes the unlocked path unrepresentable. In code review, a helper mutating m_-prefixed state it does not own with no lock evidence in its signature is the tell; the widest question is "could a new caller of this function compile without acquiring anything?"

• A partial synchronization fix that atomicizes the write side while leaving a companion read unsynchronized. Verify, for each lock introduced into an existing class, whether every access to the guarded members went under the lock or only the writes — here the hoisted treeOrder(..., makeBoundaryPoint(m_end)) in setStart and treeOrder(makeBoundaryPoint(m_start), ...) in setEnd still execute before the Locker is constructed. Narrow: re-read Range.cpp's remaining m_start/m_end uses and classify each as locked or unlocked; the unlocked ones are only sound if no other thread ever writes those fields. Wider: whenever a lock is retrofitted to a class rather than designed in, grep for reads of the guarded members that predate the lock's introduction — retrofits typically cover the writes the crash report pointed at and miss the reads. Widest: introducing a lock to fix a reported race covers the reported access, not the field — carry "which accesses to this field are still outside the lock, and what would it take for a second writer to appear?" into any codebase where synchronization was added after the fact.