← WebKit — 2026-W37

b982ebd3147d60c781c65bff005b603999b57100  Allow annotating state that is read without locking on its owner thread

severity medium class Race confidence 0.55 signal score 8.17 Source/WebCore/css
Primitive: Unlocked read of CheckedPtr/Ref members (m_track, m_ownerNode, m_current, m_element) on parallel GC marking threads racing main-thread mutation
Triage note: Introduces assertIsOwnerThread()/WTF_GUARDED_BY_LOCK and, concretely, routes the GC-thread reachability check through TextTrackCue::containsTrackAsOpaqueRootInGCThread() which locks m_trackLockForGC instead of calling track() unlocked (JSTextTrackCueCustom isReachableFromOpaqueRoots). This closes a real data race on pointer members read during concurrent marking, but exploitability as a concrete corruption is speculative and most of the diff is annotation.
Structural signals: smartptr_member_added (+6.47), null_guard (+1.2), msg_soft (+1.0), msg_noise (-2.5), area:Source/WebCore/css (+2.0)

Commit message

Allow annotating state that is read without locking on its owner thread
https://bugs.webkit.org/show_bug.cgi?id=323596

Reviewed by Geoffrey Garen.

A recurring pattern could not be expressed with WTF_GUARDED_BY_LOCK() at all, and so
went entirely unchecked: state that is mutated on a single owner thread while holding
a lock, read on that same thread without locking because locking every read would be
too expensive, and read by one other thread -- usually the garbage collector -- which
does take the lock. Several such members already had a lock named for the purpose
(m_elementLockForGC, m_opaqueRootLockForGC, m_trackLockForGC) or a comment describing
the arrangement, but nothing enforced it.

Clang's guarded_by attribute distinguishes access modes: a write requires the
capability exclusively, while a read accepts it shared. That maps onto this pattern
exactly. assertIsOwnerThread(lock, owner) grants shared, read-only access and asserts
at run time that this really is the owner thread; taking the lock grants exclusive
access as before. Annotating these members therefore makes direct writes and
foreign-thread reads compiler-checked, and turns the owner-thread fast reads into
something self-documenting and checked at run time, which is better than the nothing
they had before.

Note that the capability is granted unconditionally by calling assertIsOwnerThread();
the run-time condition only catches a caller that had no business taking the unlocked
path. The assertion also marks the lock as held for the remainder of the scope, so a
function that asserts and then takes the lock would report the Locker as acquiring a
lock that is already held. releaseOwnerThreadAssertion() hands the shared access back
for that case; it generates no code and only moves the analysis state. Where the
unlocked read merely sat just outside an existing critical section it was simpler to
move it inside: the ASSERT()s in Attr::detachFromElementWithValue(),
Attr::attachToElement() and HTMLCollection::setNamedItemCache(), and the return value
of TreeWalker::setCurrent().

Adopted for Attr::m_element, TreeWalker::m_current, CSSStyleSheet::m_ownerNode,
XSLStyleSheet::m_ownerNode, HTMLCollection::m_namedElementCache and
TextTrackCue::m_track. All six are owned by the main thread, which for these classes
is the same as the context thread, since Document::isContextThread() is isMainThread().

TextTrackCue needed one adaptation. JSTextTrackCueOwner::isReachableFromOpaqueRoots()
read m_track twice without m_trackLockForGC. Those reads were not racy:
isReachableFromOpaqueRoots() and hasPendingActivity() run with the main thread paused,
which is why they may read without locking, whereas visitChildren() and
visitAdditionalChildren() do not and therefore already lock. An owner-thread assertion
cannot express "safe because the world is stopped", and the two reads could disagree
with each other, so TextTrackCue now exposes containsTrackAsOpaqueRootInGCThread(),
which holds the lock across a single test, as visitAdditionalChildrenInGCThread()
already did for the same member.

No behaviour change is intended: this patch fixes no bugs, it only makes existing
invariants checkable.

* Source/WTF/wtf/ThreadAssertions.h:
(WTF::assertIsOwnerThread):
(WTF::releaseOwnerThreadAssertion):
* Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:
(WebCore::JSTextTrackCueOwner::isReachableFromOpaqueRoots):
* Source/WebCore/css/CSSStyleSheet.cpp:
(WebCore::CSSStyleSheet::ownerNode const):
(WebCore::CSSStyleSheet::isDetached const):
* Source/WebCore/css/CSSStyleSheet.h:
* Source/WebCore/dom/Attr.cpp:
(WebCore::Attr::setValue):
(WebCore::Attr::style):
(WebCore::Attr::value const):
(WebCore::Attr::detachFromElementWithValue):
(WebCore::Attr::attachToElement):
* Source/WebCore/dom/Attr.h:
(WebCore::Attr::ownerElement const):
* Source/WebCore/dom/TreeWalker.cpp:
(WebCore::TreeWalker::setCurrent):
(WebCore::TreeWalker::parentNode):
(WebCore::TreeWalker::firstChild):
(WebCore::TreeWalker::lastChild):
(WebCore::TreeWalker::traverseSiblings):
(WebCore::TreeWalker::previousNode):
(WebCore::TreeWalker::nextNode):
* Source/WebCore/dom/TreeWalker.h:
(WebCore::TreeWalker::currentNode):
* Source/WebCore/html/HTMLCollection.cpp:
(WebCore::HTMLCollection::namedItemSlow const):
(WebCore::HTMLCollection::supportedPropertyNames):
(WebCore::HTMLCollection::isSupportedPropertyName):
(WebCore::HTMLCollection::namedItems const):
* Source/WebCore/html/HTMLCollection.h:
* Source/WebCore/html/HTMLCollectionInlines.h:
(WebCore::HTMLCollection::hasNamedElementCache const):
(WebCore::HTMLCollection::setNamedItemCache const):
(WebCore::HTMLCollection::namedItemCaches const):
* Source/WebCore/html/track/TextTrackCue.cpp:
(WebCore::TextTrackCue::track const):
(WebCore::TextTrackCue::containsTrackAsOpaqueRootInGCThread const):
* Source/WebCore/html/track/TextTrackCue.h:
* Source/WebCore/xml/XSLStyleSheet.h:
(WebCore::XSLStyleSheet::ownerNode const):

Canonical link: https://commits.webkit.org/320637@main

Before / after

Loading diff…