Bug 1895515 - Correct RunLoopNeverRan() and ResetWorkerPrivateInWorkerThread() sequence for Worker initialization fails. r=dom-worker-reviewers,asuth,smaug

Once WorkerPrivate::ResetWorkerPrivateInWorkerThread() is called, the connection between the WorkerPrivate and the WorkerThread is decoupled. This means the WorkerPrivate is not valid anymore for any WorkerThreadRunnables. So, before decoupling the WorkerPrivate and WorkerThread, we should ensure all pending WorkerThreadRunnables are executed, and the Worker should be in the "Dead" status.

It means the logic in the WorkerPrivate::RunLoopNeverRan() except WorkerPrivate::ScheduleDeletion() should be executed before WorkerPrivate::ResetWorkerPrivateInWorkerThread(). However, currently, these logic are executed after ResetWorkerPrivateInWorkerThread().

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

dom/workers/RuntimeService.cpp

@@ -2066,21 +2066,24 @@ WorkerThreadPrimaryRunnable::Run() {
                                    url.get());
 
   using mozilla::ipc::BackgroundChild;
-
   {
+    bool runLoopRan = false;
     auto failureCleanup = MakeScopeExit([&]() {
-      // The creation of threadHelper above is the point at which a worker is
+      // If Worker initialization fails, call WorkerPrivate::ScheduleDeletion()
-      // considered to have run, because the `mPreStartRunnables` are all
+      // to release the WorkerPrivate in the parent thread.
-      // re-dispatched after `mThread` is set.  We need to let the WorkerPrivate
+      mWorkerPrivate->ScheduleDeletion(WorkerPrivate::WorkerRan);
-      // know so it can clean up the various event loops and delete the worker.
-      mWorkerPrivate->RunLoopNeverRan();
     });
 
     mWorkerPrivate->SetWorkerPrivateInWorkerThread(mThread.unsafeGetRawPtr());
 
     const auto threadCleanup = MakeScopeExit([&] {
-      // This must be called before ScheduleDeletion, which is either called
+      // If Worker initialization fails, such as creating a BackgroundChild or
-      // from failureCleanup leaving scope, or from the outer scope.
+      // the worker's JSContext initialization failing, call
+      // WorkerPrivate::RunLoopNeverRan() to set the Worker to the correct
+      // status, which means "Dead," to forbid WorkerThreadRunnable dispatching.
+      if (!runLoopRan) {
+        mWorkerPrivate->RunLoopNeverRan();
+      }
       mWorkerPrivate->ResetWorkerPrivateInWorkerThread();
     });
 
@@ -2117,6 +2120,7 @@ WorkerThreadPrimaryRunnable::Run() {
       }
 
       failureCleanup.release();
+      runLoopRan = true;
 
       {
         PROFILER_SET_JS_CONTEXT(cx);

dom/workers/WorkerPrivate.cpp

@@ -3247,8 +3247,6 @@ void WorkerPrivate::RunLoopNeverRan() {
   // PerformanceStorageWorker which holds a WeakWorkerRef.
   // Notify WeakWorkerRefs with Dead status.
   NotifyWorkerRefs(Dead);
-
-  ScheduleDeletion(WorkerPrivate::WorkerRan);
 }
 
 void WorkerPrivate::UnrootGlobalScopes() {
@@ -5854,10 +5852,11 @@ void WorkerPrivate::ResetWorkerPrivateInWorkerThread() {
 
   // Release the mutex before doomedThread.
   MutexAutoLock lock(mMutex);
+  MOZ_ASSERT(mStatus == Dead);
 
   MOZ_ASSERT(mThread);
 
-  mThread->SetWorker(WorkerThreadFriendKey{}, nullptr);
+  mThread->ClearEventQueueAndWorker(WorkerThreadFriendKey{});
   mThread.swap(doomedThread);
 }
 

dom/workers/WorkerThread.cpp

@@ -117,50 +117,54 @@ SafeRefPtr<WorkerThread> WorkerThread::Create(
 void WorkerThread::SetWorker(const WorkerThreadFriendKey& /* aKey */,
                              WorkerPrivate* aWorkerPrivate) {
   MOZ_ASSERT(PR_GetCurrentThread() == mThread);
+  MOZ_ASSERT(aWorkerPrivate);
 
-  if (aWorkerPrivate) {
+  {
-    {
+    MutexAutoLock lock(mLock);
-      MutexAutoLock lock(mLock);
 
-      MOZ_ASSERT(!mWorkerPrivate);
+    MOZ_ASSERT(!mWorkerPrivate);
-      MOZ_ASSERT(mAcceptingNonWorkerRunnables);
+    MOZ_ASSERT(mAcceptingNonWorkerRunnables);
 
-      mWorkerPrivate = aWorkerPrivate;
+    mWorkerPrivate = aWorkerPrivate;
 #ifdef DEBUG
-      mAcceptingNonWorkerRunnables = false;
+    mAcceptingNonWorkerRunnables = false;
 #endif
+  }
+
+  mObserver = new Observer(aWorkerPrivate);
+  MOZ_ALWAYS_SUCCEEDS(AddObserver(mObserver));
+}
+
+void WorkerThread::ClearEventQueueAndWorker(
+    const WorkerThreadFriendKey& /* aKey */) {
+  MOZ_ASSERT(PR_GetCurrentThread() == mThread);
+
+  MOZ_ALWAYS_SUCCEEDS(RemoveObserver(mObserver));
+  mObserver = nullptr;
+
+  {
+    MutexAutoLock lock(mLock);
+
+    MOZ_ASSERT(mWorkerPrivate);
+    MOZ_ASSERT(!mAcceptingNonWorkerRunnables);
+    // mOtherThreadsDispatchingViaEventTarget can still be non-zero here
+    // because WorkerThread::Dispatch isn't atomic so a thread initiating
+    // dispatch can have dispatched a runnable at this thread allowing us to
+    // begin shutdown before that thread gets a chance to decrement
+    // mOtherThreadsDispatchingViaEventTarget back to 0.  So we need to wait
+    // for that.
+    while (mOtherThreadsDispatchingViaEventTarget) {
+      mWorkerPrivateCondVar.Wait();
+    }
+    // Need to clean up the dispatched runnables if
+    // mOtherThreadsDispatchingViaEventTarget was non-zero.
+    if (NS_HasPendingEvents(nullptr)) {
+      NS_ProcessPendingEvents(nullptr);
     }
-
-    mObserver = new Observer(aWorkerPrivate);
-    MOZ_ALWAYS_SUCCEEDS(AddObserver(mObserver));
-  } else {
-    MOZ_ALWAYS_SUCCEEDS(RemoveObserver(mObserver));
-    mObserver = nullptr;
-
-    {
-      MutexAutoLock lock(mLock);
-
-      MOZ_ASSERT(mWorkerPrivate);
-      MOZ_ASSERT(!mAcceptingNonWorkerRunnables);
-      // mOtherThreadsDispatchingViaEventTarget can still be non-zero here
-      // because WorkerThread::Dispatch isn't atomic so a thread initiating
-      // dispatch can have dispatched a runnable at this thread allowing us to
-      // begin shutdown before that thread gets a chance to decrement
-      // mOtherThreadsDispatchingViaEventTarget back to 0.  So we need to wait
-      // for that.
-      while (mOtherThreadsDispatchingViaEventTarget) {
-        mWorkerPrivateCondVar.Wait();
-      }
-      // Need to clean up the dispatched runnables if
-      // mOtherThreadsDispatchingViaEventTarget was non-zero.
-      if (NS_HasPendingEvents(nullptr)) {
-        NS_ProcessPendingEvents(nullptr);
-      }
 #ifdef DEBUG
-      mAcceptingNonWorkerRunnables = true;
+    mAcceptingNonWorkerRunnables = true;
 #endif
-      mWorkerPrivate = nullptr;
+    mWorkerPrivate = nullptr;
-    }
   }
 }
 

dom/workers/WorkerThread.h

@@ -72,6 +72,13 @@ class WorkerThread final : public nsThread {
   void SetWorker(const WorkerThreadFriendKey& aKey,
                  WorkerPrivate* aWorkerPrivate);
 
+  // This method is used to decouple the connection with the WorkerPrivate which
+  // is set in SetWorker(). And it also clears all pending runnables on this
+  // WorkerThread.
+  // After decoupling, WorkerThreadRunnable can not run on this WorkerThread
+  // anymore, since WorkerPrivate is invalid.
+  void ClearEventQueueAndWorker(const WorkerThreadFriendKey& aKey);
+
   nsresult DispatchPrimaryRunnable(const WorkerThreadFriendKey& aKey,
                                    already_AddRefed<nsIRunnable> aRunnable);