Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: 9ea05fa06f20960616a47235f58d08bb1fbdea55
https://github.com/WebKit/WebKit/commit/9ea05fa06f20960616a47235f58d08bb1fbdea55
Author: Mark Lam <[email protected]>
Date: 2025-09-16 (Tue, 16 Sep 2025)
Changed paths:
M Source/JavaScriptCore/runtime/StackManager.cpp
M Source/JavaScriptCore/runtime/StackManager.h
M Source/JavaScriptCore/runtime/VM.h
M Source/JavaScriptCore/runtime/VMTraps.cpp
M Source/JavaScriptCore/runtime/VMTraps.h
M Source/WTF/wtf/Compiler.h
M Source/WTF/wtf/Forward.h
M Source/WTF/wtf/OptionSet.h
M Source/WTF/wtf/OptionSetHash.h
M Tools/TestWebKitAPI/Tests/WTF/OptionSet.cpp
Log Message:
-----------
Make VM::setHasTerminationRequest() thread-safe.
https://bugs.webkit.org/show_bug.cgi?id=298798
rdar://160494627
Reviewed by Yijia Huang.
VM::setHasTerminationRequest() should be thread-safe but is not because it sets
a bit on the
VM::m_entryScopeServices OptionSet, and the bit set operation is not atomic..
For the record, this is how the concurrent access works:
1. VM::m_entryScopeServices is normally set by the mutator thread running that
VM.
2. However, the Worker.terminate() API can be used by the main thread to
terminate worker threads.
Worker.terminate() calls ...
WorkerMessagingProxy::terminateWorkerGlobalScope(), which calls ...
WorkerOrWorkletThread::stop(), which calls ...
WorkerOrWorkletScriptController::scheduleExecutionTermination(), which calls ...
VM::notifyNeedTermination(), which calls ...
VM::setHasTerminationRequest(), which calls ...
VM::requestEntryScopeService(), and ...
VM::requestEntryScopeService() does bitwise OR of the
EntryScopeService::ResetTerminationRequest bit
into the worker thread's VM::m_entryScopeServices. This is not thread safe
because it may collide with
the worker thread writing values into its VM::m_entryScopeServices.
We can fix this by moving the EntryScopeService::ResetTerminationRequest bit
into a separate byte that
is safe to write to concurrently. However, will go one step further and make a
concurrent-safe
OptionSet and use that for storing the
EntryScopeService::ResetTerminationRequest bit. This way, we
can add more concurrent bits if needed.
In practice, the bug never manifests because it is extremely rare for
VM::m_entryScopeServices to ever
be written to, and the impact is benign. Regardless, we are fixing this.
Changes:
1. #define a CONCURRENT_SAFE macro in Compiler.h, and use it to annotate
functions that we intend to
be safe to call concurrently from other threads. Currently, CONCURRENT_SAFE
is just a documentation
mechanism, and does not correspond to any compiler artifact. In the future,
if desired, we can
add a verifier pass that ensures that CONCURRENT_SAFE functions can only
call other CONCURRENT_SAFE
functions, and such a verifier would have caught the bug in this commit.
2. Move EntryScopeService::ResetTerminationRequest into a separate enum class
ConcurrentEntryScopeService.
While EntryScopeService bits continue to be stored in
VM::m_entryScopeServices,
ConcurrentEntryScopeService bits will now be stored in a new
VM::m_concurrentEntryScopeServices.
Both VM::m_entryScopeServices and VM::m_concurrentEntryScopeServices are
backed by storage in
VM::m_entryScopeServicesRawBits. This allows us to do a single null check on
VM::m_entryScopeServicesRawBits to quickly determine if we have any
"services" that we need to
service (concurrent or otherwise).
3. Enhance WTF::OptionSet to be able to accept a ConcurrencyTag template
parameter. By default,
if not explicitly specified, ConcurrencyTag::None will be used. This
results in OptionSet
behaving exactly as it is today.
However, we can now use OptionSet with ConcurrencyTag::Atomic, which will
switch its operations
to operate on the its m_storage in an atomic way. This is done for add()
and remove() where we
add / remove bits to / from it m_storage word.
We also enhance hasExactlyOneBitSet() and toSingleValue() so that they are
not subject to TOCTOU
issues which may cause their result to be internally inconsistent. These
functions need to read
m_storage more than once. So, the ConcurrencyTag::Atomic version needs to
make a copy of
m_storage and operate on that single copy that is not subject to concurrent
edits.
VM::m_concurrentEntryScopeServices is implemented using the
ConcurrencyTag::Atomic variant of
OptionSet.
TestWebKitAPI OptionSet tests has been updated to test the
ConcurrencyTag::Atomic variant as well.
The VM entryScopeServices mechanism is covered by existing tests e.g. JSC's
testapi.
* Source/JavaScriptCore/runtime/StackManager.cpp:
(JSC::StackManager::requestStop):
(JSC::StackManager::cancelStop):
* Source/JavaScriptCore/runtime/StackManager.h:
* Source/JavaScriptCore/runtime/VM.h:
(JSC::VM::clearHasTerminationRequest):
(JSC::VM::setHasTerminationRequest):
(JSC::VM::hasAnyEntryScopeServiceRequest):
(JSC::VM::requestEntryScopeService):
(JSC::VM::entryScopeServices):
(JSC::VM::concurrentEntryScopeServices):
(JSC::VM::hasEntryScopeServiceRequest):
(JSC::VM::clearEntryScopeService):
(JSC::VM::notifyNeedDebuggerBreak):
(JSC::VM::notifyNeedShellTimeoutCheck):
(JSC::VM::notifyNeedTermination):
(JSC::VM::notifyNeedWatchdogCheck):
* Source/JavaScriptCore/runtime/VMTraps.cpp:
(JSC::VMTraps::cancelThreadStopIfNeeded):
(JSC::VMTraps::requestThreadStopIfNeeded):
* Source/JavaScriptCore/runtime/VMTraps.h:
(JSC::VMTraps::clearTrap):
(JSC::VMTraps::fireTrap):
* Source/WTF/wtf/Compiler.h:
* Source/WTF/wtf/Forward.h:
* Source/WTF/wtf/OptionSet.h:
(WTF::OptionSet::add):
(WTF::OptionSet::remove):
(WTF::OptionSet::hasExactlyOneBitSet const):
(WTF::OptionSet::toSingleValue const):
(WTF::isValidOptionSet):
(WTF::ConstexprOptionSet::ConstexprOptionSet):
(WTF::ConstexprOptionSet::operator* const):
* Source/WTF/wtf/OptionSetHash.h:
(WTF::DefaultHash<OptionSet<T>>::hash): Deleted.
(WTF::DefaultHash<OptionSet<T>>::equal): Deleted.
(WTF::HashTraits<OptionSet<T>>::emptyValue): Deleted.
(WTF::HashTraits<OptionSet<T>>::constructDeletedValue): Deleted.
(WTF::HashTraits<OptionSet<T>>::isDeletedValue): Deleted.
* Tools/TestWebKitAPI/Tests/WTF/OptionSet.cpp:
(TestWebKitAPI::TEST(WTF_OptionSet, EmptySet)):
(TestWebKitAPI::TEST(WTF_OptionSet, ContainsOneFlag)):
(TestWebKitAPI::TEST(WTF_OptionSet, Equal)):
(TestWebKitAPI::TEST(WTF_OptionSet, NotEqual)):
(TestWebKitAPI::TEST(WTF_OptionSet, Or)):
(TestWebKitAPI::TEST(WTF_OptionSet, OrAssignment)):
(TestWebKitAPI::TEST(WTF_OptionSet, Minus)):
(TestWebKitAPI::TEST(WTF_OptionSet, AddAndRemove)):
(TestWebKitAPI::TEST(WTF_OptionSet, Set)):
(TestWebKitAPI::TEST(WTF_OptionSet, ContainsTwoFlags)):
(TestWebKitAPI::TEST(WTF_OptionSet, ContainsTwoFlags2)):
(TestWebKitAPI::TEST(WTF_OptionSet, ContainsTwoFlags3)):
(TestWebKitAPI::TEST(WTF_OptionSet, EmptyOptionSetToRawValueToOptionSet)):
(TestWebKitAPI::TEST(WTF_OptionSet,
OptionSetThatContainsOneFlagToRawValueToOptionSet)):
(TestWebKitAPI::TEST(WTF_OptionSet,
OptionSetThatContainsOneFlagToRawValueToOptionSet2)):
(TestWebKitAPI::TEST(WTF_OptionSet,
OptionSetThatContainsTwoFlagsToRawValueToOptionSet)):
(TestWebKitAPI::TEST(WTF_OptionSet,
OptionSetThatContainsTwoFlagsToRawValueToOptionSet2)):
(TestWebKitAPI::TEST(WTF_OptionSet, TwoIteratorsIntoSameOptionSet)):
(TestWebKitAPI::TEST(WTF_OptionSet, IterateOverOptionSetThatContainsTwoFlags)):
(TestWebKitAPI::TEST(WTF_OptionSet, IterateOverOptionSetThatContainsFlags2)):
(TestWebKitAPI::TEST(WTF_OptionSet, NextItemAfterLargestIn32BitFlagSet)):
(TestWebKitAPI::TEST(WTF_OptionSet, NextItemAfterLargestIn64BitFlagSet)):
(TestWebKitAPI::TEST(WTF_OptionSet,
IterationOrderTheSameRegardlessOfInsertionOrder)):
(TestWebKitAPI::TEST(WTF_OptionSet, OperatorAnd)):
(TestWebKitAPI::TEST(WTF_OptionSet, OperatorXor)):
(TestWebKitAPI::TEST(WTF_OptionSet, ContainsAny)):
(TestWebKitAPI::TEST(WTF_OptionSet, ContainsAll)):
(TestWebKitAPI::TEST(WTF_OptionSet, HashSet)):
Canonical link: https://commits.webkit.org/300049@main
To unsubscribe from these emails, change your notification settings at
https://github.com/WebKit/WebKit/settings/notifications
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
