Bug 1663616 - Use the helper thread lock for off-thread promise state r=jandem

This changes the off-thread promise helper code from using its own mutex to
using the helper thread lock for synchronisation.

For promise tasks executed on helper threads there's little benefit from using
a separate mutex as we already have to wait on the helper thread lock to start
the task. The changes ensure we don't delete the OffThreadPromiseTask while the
helper thread system thinks it's still running.

Differential Revision: https://phabricator.services.mozilla.com/D89472

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,

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<PromiseObject*> 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; }
 };
 

js/src/vm/MutexIDs.h

@@ -46,7 +46,6 @@
   _(TraceLoggerThreadState, 500)      \
   _(DateTimeInfoMutex, 500)           \
   _(ProcessExecutableRegion, 500)     \
-  _(OffThreadPromiseState, 500)       \
   _(BufferStreamState, 500)           \
   _(SharedArrayGrow, 500)             \
   _(RuntimeScriptData, 500)           \

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<Mutex> 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<Mutex> 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<Mutex>(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<Mutex> 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<OffThreadPromiseRuntimeState*>(closure);
   MOZ_ASSERT(state.usingInternalDispatchQueue());
-
-  LockGuard<Mutex> 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<Mutex> 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<Mutex> 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<Mutex> 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<Mutex> 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

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<OffThreadPromiseTaskSet> 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<ConditionVariable> allCanceled_;
+  HelperThreadLockData<size_t> numCanceled_;
 
   // The queue of JS::Dispatchables used by the DispatchToEventLoopCallback that
   // calling js::UseInternalJobQueues installs.
-  DispatchableFifo internalDispatchQueue_;
-  ConditionVariable internalDispatchQueueAppended_;
-  bool internalDispatchQueueClosed_;
+  HelperThreadLockData<DispatchableFifo> internalDispatchQueue_;
+  HelperThreadLockData<ConditionVariable> internalDispatchQueueAppended_;
+  HelperThreadLockData<bool> 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;