diff --git a/js/src/vm/HelperThreads.cpp b/js/src/vm/HelperThreads.cpp index ab4097c7dfc4..ca87c2edb358 100644 --- a/js/src/vm/HelperThreads.cpp +++ b/js/src/vm/HelperThreads.cpp @@ -2172,10 +2172,7 @@ void HelperThread::handlePromiseHelperTaskWorkload( HelperThreadState().promiseHelperTasks(locked).popCopy(); currentTask.emplace(task); - { - AutoUnlockHelperThreadState unlock(locked); - task->runTask(); - } + task->runTaskLocked(locked); currentTask.reset(); @@ -2387,9 +2384,17 @@ void PromiseHelperTask::executeAndResolveAndDestroy(JSContext* cx) { run(cx, JS::Dispatchable::NotShuttingDown); } -void PromiseHelperTask::runTask() { - execute(); - dispatchResolveAndDestroy(); +void PromiseHelperTask::runTaskLocked(AutoLockHelperThreadState& lock) { + { + AutoUnlockHelperThreadState unlock(lock); + execute(); + } + + // Don't release the lock between dispatching the resolve and destroy + // operation (which may start immediately on another thread) and returning + // from this method. + + dispatchResolveAndDestroy(lock); } bool js::StartOffThreadPromiseHelperTask(JSContext* cx, diff --git a/js/src/vm/HelperThreads.h b/js/src/vm/HelperThreads.h index 2e00773236fc..854b578d3c7c 100644 --- a/js/src/vm/HelperThreads.h +++ b/js/src/vm/HelperThreads.h @@ -889,7 +889,7 @@ class SourceCompressionTask : public RunnableTask { // The helper thread will call execute() to do the main work. Then, the thread // of the JSContext used to create the PromiseHelperTask will call resolve() to // resolve promise according to those results. -struct PromiseHelperTask : OffThreadPromiseTask, public RunnableTask { +struct PromiseHelperTask : OffThreadPromiseTask, public HelperThreadTask { PromiseHelperTask(JSContext* cx, Handle promise) : OffThreadPromiseTask(cx, promise) {} @@ -902,7 +902,8 @@ struct PromiseHelperTask : OffThreadPromiseTask, public RunnableTask { // Warning: After this function returns, 'this' can be deleted at any time, so // the caller must immediately return from the stream callback. void executeAndResolveAndDestroy(JSContext* cx); - void runTask() override; + + void runTaskLocked(AutoLockHelperThreadState& lock) override; ThreadType threadType() override { return THREAD_TYPE_PROMISE_TASK; } }; diff --git a/js/src/vm/MutexIDs.h b/js/src/vm/MutexIDs.h index 2d46180d0b0b..30c6f5811748 100644 --- a/js/src/vm/MutexIDs.h +++ b/js/src/vm/MutexIDs.h @@ -46,7 +46,6 @@ _(TraceLoggerThreadState, 500) \ _(DateTimeInfoMutex, 500) \ _(ProcessExecutableRegion, 500) \ - _(OffThreadPromiseState, 500) \ _(BufferStreamState, 500) \ _(SharedArrayGrow, 500) \ _(RuntimeScriptData, 500) \ diff --git a/js/src/vm/OffThreadPromiseRuntimeState.cpp b/js/src/vm/OffThreadPromiseRuntimeState.cpp index 3ed50e8b131b..24acba8c9f8e 100644 --- a/js/src/vm/OffThreadPromiseRuntimeState.cpp +++ b/js/src/vm/OffThreadPromiseRuntimeState.cpp @@ -16,11 +16,8 @@ #include "js/HeapAPI.h" // JS::shadow::Zone #include "js/Promise.h" // JS::Dispatchable, JS::DispatchToEventLoopCallback #include "js/Utility.h" // js_delete, js::AutoEnterOOMUnsafeRegion -#include "threading/LockGuard.h" // js::LockGuard -#include "threading/Mutex.h" // js::Mutex #include "threading/ProtectedData.h" // js::UnprotectedData #include "vm/JSContext.h" // JSContext -#include "vm/MutexIDs.h" // js::mutexid::OffThreadPromiseState #include "vm/PromiseObject.h" // js::PromiseObject #include "vm/Realm.h" // js::AutoRealm #include "vm/Runtime.h" // JSRuntime @@ -58,9 +55,9 @@ bool OffThreadPromiseTask::init(JSContext* cx) { OffThreadPromiseRuntimeState& state = runtime_->offThreadPromiseState.ref(); MOZ_ASSERT(state.initialized()); - LockGuard lock(state.mutex_); + AutoLockHelperThreadState lock; - if (!state.live_.putNew(this)) { + if (!state.live().putNew(this)) { ReportOutOfMemory(cx); return false; } @@ -71,8 +68,8 @@ bool OffThreadPromiseTask::init(JSContext* cx) { void OffThreadPromiseTask::unregister(OffThreadPromiseRuntimeState& state) { MOZ_ASSERT(registered_); - LockGuard lock(state.mutex_); - state.live_.remove(this); + AutoLockHelperThreadState lock; + state.live().remove(this); registered_ = false; } @@ -85,6 +82,9 @@ void OffThreadPromiseTask::run(JSContext* cx, // Remove this task from live_ before calling `resolve`, so that if `resolve` // itself drains the queue reentrantly, the queue will not think this task is // yet to be queued and block waiting for it. + // + // The unregister method synchronizes on the helper thread lock and ensures + // that we don't delete the task while the helper thread is still running. OffThreadPromiseRuntimeState& state = runtime_->offThreadPromiseState.ref(); MOZ_ASSERT(state.initialized()); unregister(state); @@ -103,11 +103,17 @@ void OffThreadPromiseTask::run(JSContext* cx, } void OffThreadPromiseTask::dispatchResolveAndDestroy() { + AutoLockHelperThreadState lock; + dispatchResolveAndDestroy(lock); +} + +void OffThreadPromiseTask::dispatchResolveAndDestroy( + const AutoLockHelperThreadState& lock) { MOZ_ASSERT(registered_); OffThreadPromiseRuntimeState& state = runtime_->offThreadPromiseState.ref(); MOZ_ASSERT(state.initialized()); - MOZ_ASSERT((LockGuard(state.mutex_), state.live_.has(this))); + MOZ_ASSERT(state.live().has(this)); // If the dispatch succeeds, then we are guaranteed that run() will be // called on an active JSContext of runtime_. @@ -121,24 +127,22 @@ void OffThreadPromiseTask::dispatchResolveAndDestroy() { // dispatchResolveAndDestroy, and when they account for the entire contents of // live_, notify OffThreadPromiseRuntimeState::shutdown that it is safe to // destruct them. - LockGuard lock(state.mutex_); state.numCanceled_++; - if (state.numCanceled_ == state.live_.count()) { - state.allCanceled_.notify_one(); + if (state.numCanceled_ == state.live().count()) { + state.allCanceled().notify_one(); } } OffThreadPromiseRuntimeState::OffThreadPromiseRuntimeState() : dispatchToEventLoopCallback_(nullptr), dispatchToEventLoopClosure_(nullptr), - mutex_(mutexid::OffThreadPromiseState), numCanceled_(0), internalDispatchQueueClosed_(false) {} OffThreadPromiseRuntimeState::~OffThreadPromiseRuntimeState() { - MOZ_ASSERT(live_.empty()); + MOZ_ASSERT(live_.refNoCheck().empty()); MOZ_ASSERT(numCanceled_ == 0); - MOZ_ASSERT(internalDispatchQueue_.empty()); + MOZ_ASSERT(internalDispatchQueue_.refNoCheck().empty()); MOZ_ASSERT(!initialized()); } @@ -158,8 +162,7 @@ bool OffThreadPromiseRuntimeState::internalDispatchToEventLoop( OffThreadPromiseRuntimeState& state = *reinterpret_cast(closure); MOZ_ASSERT(state.usingInternalDispatchQueue()); - - LockGuard lock(state.mutex_); + MOZ_ASSERT(HelperThreadState().isLockedByCurrentThread()); if (state.internalDispatchQueueClosed_) { return false; @@ -168,12 +171,12 @@ bool OffThreadPromiseRuntimeState::internalDispatchToEventLoop( // The JS API contract is that 'false' means shutdown, so be infallible // here (like Gecko). AutoEnterOOMUnsafeRegion noOOM; - if (!state.internalDispatchQueue_.pushBack(d)) { + if (!state.internalDispatchQueue().pushBack(d)) { noOOM.crash("internalDispatchToEventLoop"); } // Wake up internalDrain() if it is waiting for a job to finish. - state.internalDispatchQueueAppended_.notify_one(); + state.internalDispatchQueueAppended().notify_one(); return true; } @@ -192,39 +195,39 @@ bool OffThreadPromiseRuntimeState::initialized() const { void OffThreadPromiseRuntimeState::internalDrain(JSContext* cx) { MOZ_ASSERT(usingInternalDispatchQueue()); - MOZ_ASSERT(!internalDispatchQueueClosed_); for (;;) { JS::Dispatchable* d; { - LockGuard lock(mutex_); + AutoLockHelperThreadState lock; - MOZ_ASSERT_IF(!internalDispatchQueue_.empty(), !live_.empty()); - if (live_.empty()) { + MOZ_ASSERT(!internalDispatchQueueClosed_); + MOZ_ASSERT_IF(!internalDispatchQueue().empty(), !live().empty()); + if (live().empty()) { return; } // There are extant live OffThreadPromiseTasks. If none are in the queue, // block until one of them finishes and enqueues a dispatchable. - while (internalDispatchQueue_.empty()) { - internalDispatchQueueAppended_.wait(lock); + while (internalDispatchQueue().empty()) { + internalDispatchQueueAppended().wait(lock); } - d = internalDispatchQueue_.popCopyFront(); + d = internalDispatchQueue().popCopyFront(); } - // Don't call run() with mutex_ held to avoid deadlock. + // Don't call run() with lock held to avoid deadlock. d->run(cx, JS::Dispatchable::NotShuttingDown); } } bool OffThreadPromiseRuntimeState::internalHasPending() { MOZ_ASSERT(usingInternalDispatchQueue()); - MOZ_ASSERT(!internalDispatchQueueClosed_); - LockGuard lock(mutex_); - MOZ_ASSERT_IF(!internalDispatchQueue_.empty(), !live_.empty()); - return !live_.empty(); + AutoLockHelperThreadState lock; + MOZ_ASSERT(!internalDispatchQueueClosed_); + MOZ_ASSERT_IF(!internalDispatchQueue().empty(), !live().empty()); + return !live().empty(); } void OffThreadPromiseRuntimeState::shutdown(JSContext* cx) { @@ -232,51 +235,50 @@ void OffThreadPromiseRuntimeState::shutdown(JSContext* cx) { return; } + AutoLockHelperThreadState lock; + // When the shell is using the internal event loop, we must simulate our // requirement of the embedding that, before shutdown, all successfully- // dispatched-to-event-loop tasks have been run. if (usingInternalDispatchQueue()) { DispatchableFifo dispatchQueue; { - LockGuard lock(mutex_); - std::swap(dispatchQueue, internalDispatchQueue_); - MOZ_ASSERT(internalDispatchQueue_.empty()); + std::swap(dispatchQueue, internalDispatchQueue()); + MOZ_ASSERT(internalDispatchQueue().empty()); internalDispatchQueueClosed_ = true; } - // Don't call run() with mutex_ held to avoid deadlock. + // Don't call run() with lock held to avoid deadlock. + AutoUnlockHelperThreadState unlock(lock); for (JS::Dispatchable* d : dispatchQueue) { d->run(cx, JS::Dispatchable::ShuttingDown); } } - { - // An OffThreadPromiseTask may only be safely deleted on its JSContext's - // thread (since it contains a PersistentRooted holding its promise), and - // only after it has called dispatchResolveAndDestroy (since that is our - // only indication that its owner is done writing into it). - // - // OffThreadPromiseTasks accepted by the DispatchToEventLoopCallback are - // deleted by their 'run' methods. Only dispatchResolveAndDestroy invokes - // the callback, and the point of the callback is to call 'run' on the - // JSContext's thread, so the conditions above are met. - // - // But although the embedding's DispatchToEventLoopCallback promises to run - // every task it accepts before shutdown, when shutdown does begin it starts - // rejecting tasks; we cannot count on 'run' to clean those up for us. - // Instead, dispatchResolveAndDestroy keeps a count of rejected ('canceled') - // tasks; once that count covers everything in live_, this function itself - // runs only on the JSContext's thread, so we can delete them all here. - LockGuard lock(mutex_); - while (live_.count() != numCanceled_) { - MOZ_ASSERT(numCanceled_ < live_.count()); - allCanceled_.wait(lock); - } + // An OffThreadPromiseTask may only be safely deleted on its JSContext's + // thread (since it contains a PersistentRooted holding its promise), and + // only after it has called dispatchResolveAndDestroy (since that is our + // only indication that its owner is done writing into it). + // + // OffThreadPromiseTasks accepted by the DispatchToEventLoopCallback are + // deleted by their 'run' methods. Only dispatchResolveAndDestroy invokes + // the callback, and the point of the callback is to call 'run' on the + // JSContext's thread, so the conditions above are met. + // + // But although the embedding's DispatchToEventLoopCallback promises to run + // every task it accepts before shutdown, when shutdown does begin it starts + // rejecting tasks; we cannot count on 'run' to clean those up for us. + // Instead, dispatchResolveAndDestroy keeps a count of rejected ('canceled') + // tasks; once that count covers everything in live_, this function itself + // runs only on the JSContext's thread, so we can delete them all here. + while (live().count() != numCanceled_) { + MOZ_ASSERT(numCanceled_ < live().count()); + allCanceled().wait(lock); } // Now that live_ contains only cancelled tasks, we can just delete // everything. - for (OffThreadPromiseTaskSet::Range r = live_.all(); !r.empty(); + for (OffThreadPromiseTaskSet::Range r = live().all(); !r.empty(); r.popFront()) { OffThreadPromiseTask* task = r.front(); @@ -286,7 +288,7 @@ void OffThreadPromiseRuntimeState::shutdown(JSContext* cx) { task->registered_ = false; js_delete(task); } - live_.clear(); + live().clear(); numCanceled_ = 0; // After shutdown, there should be no OffThreadPromiseTask activity in this diff --git a/js/src/vm/OffThreadPromiseRuntimeState.h b/js/src/vm/OffThreadPromiseRuntimeState.h index 2d1d700763b8..f2d24a600b6d 100644 --- a/js/src/vm/OffThreadPromiseRuntimeState.h +++ b/js/src/vm/OffThreadPromiseRuntimeState.h @@ -17,7 +17,6 @@ #include "js/Promise.h" // JS::Dispatchable, JS::Dispatchable::MaybeShuttingDown, JS::DispatchToEventLoopCallback #include "js/RootingAPI.h" // JS::Handle, JS::PersistentRooted #include "threading/ConditionVariable.h" // js::ConditionVariable -#include "threading/Mutex.h" // js::Mutex #include "vm/PromiseObject.h" // js::PromiseObject struct JS_PUBLIC_API JSContext; @@ -136,6 +135,7 @@ class OffThreadPromiseTask : public JS::Dispatchable { // However, if shutdown interrupts, resolve() may not be called, though the // OffThreadPromiseTask will be destroyed on a JSContext thread. void dispatchResolveAndDestroy(); + void dispatchResolveAndDestroy(const AutoLockHelperThreadState& lock); }; using OffThreadPromiseTaskSet = @@ -152,26 +152,33 @@ class OffThreadPromiseRuntimeState { JS::DispatchToEventLoopCallback dispatchToEventLoopCallback_; void* dispatchToEventLoopClosure_; - // All following fields are mutated by any thread and are guarded by mutex_. - Mutex mutex_; - // A set of all OffThreadPromiseTasks that have successfully called 'init'. // OffThreadPromiseTask's destructor removes them from the set. - OffThreadPromiseTaskSet live_; + HelperThreadLockData live_; - // The allCancelled_ condition is waited on and notified during engine + // The allCanceled_ condition is waited on and notified during engine // shutdown, communicating when all off-thread tasks in live_ are safe to be // destroyed from the (shutting down) main thread. This condition is met when // live_.count() == numCanceled_ where "canceled" means "the // DispatchToEventLoopCallback failed after this task finished execution". - ConditionVariable allCanceled_; - size_t numCanceled_; + HelperThreadLockData allCanceled_; + HelperThreadLockData numCanceled_; // The queue of JS::Dispatchables used by the DispatchToEventLoopCallback that // calling js::UseInternalJobQueues installs. - DispatchableFifo internalDispatchQueue_; - ConditionVariable internalDispatchQueueAppended_; - bool internalDispatchQueueClosed_; + HelperThreadLockData internalDispatchQueue_; + HelperThreadLockData internalDispatchQueueAppended_; + HelperThreadLockData internalDispatchQueueClosed_; + + OffThreadPromiseTaskSet& live() { return live_.ref(); } + ConditionVariable& allCanceled() { return allCanceled_.ref(); } + + DispatchableFifo& internalDispatchQueue() { + return internalDispatchQueue_.ref(); + } + ConditionVariable& internalDispatchQueueAppended() { + return internalDispatchQueueAppended_.ref(); + } static bool internalDispatchToEventLoop(void*, JS::Dispatchable*); bool usingInternalDispatchQueue() const;