Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: b982ebd3147d60c781c65bff005b603999b57100
https://github.com/WebKit/WebKit/commit/b982ebd3147d60c781c65bff005b603999b57100
Author: Chris Dumez <[email protected]>
Date: 2026-09-07 (Mon, 07 Sep 2026)
Changed paths:
M Source/WTF/wtf/ThreadAssertions.h
M Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp
M Source/WebCore/css/CSSStyleSheet.cpp
M Source/WebCore/css/CSSStyleSheet.h
M Source/WebCore/dom/Attr.cpp
M Source/WebCore/dom/Attr.h
M Source/WebCore/dom/TreeWalker.cpp
M Source/WebCore/dom/TreeWalker.h
M Source/WebCore/html/HTMLCollection.cpp
M Source/WebCore/html/HTMLCollection.h
M Source/WebCore/html/HTMLCollectionInlines.h
M Source/WebCore/html/track/TextTrackCue.cpp
M Source/WebCore/html/track/TextTrackCue.h
M Source/WebCore/xml/XSLStyleSheet.h
Log 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
To unsubscribe from these emails, change your notification settings at
https://github.com/WebKit/WebKit/settings/notifications
