Bug 1387211 Avoid potential deadlock during worker shutdown. r=billm
authorBen Kelly <ben@wanderview.com>
Mon, 07 Aug 2017 09:24:22 -0700
changeset 642168 2024c4bb1c766f5c42f428abe03b7e500e364666
parent 642167 eb96d03a53a2af0ebe762ce6ff4a37cc9035073c
child 642169 551898614c2c8e44fd249cf7f924c9e43d3eff01
push id72668
push userbmo:tchiovoloni@mozilla.com
push dateMon, 07 Aug 2017 20:01:43 +0000
reviewersbillm
bugs1387211
milestone57.0a1
Bug 1387211 Avoid potential deadlock during worker shutdown. r=billm
dom/workers/WorkerPrivate.cpp
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -6216,30 +6216,44 @@ WorkerPrivate::NotifyInternal(JSContext*
   Status previousStatus;
   {
     MutexAutoLock lock(mMutex);
 
     if (mStatus >= aStatus) {
       return true;
     }
 
+    // Make sure the hybrid event target stops dispatching runnables
+    // once we reaching the killing state.
+    if (aStatus == Killing) {
+      // To avoid deadlock we always acquire the event target mutex before the
+      // worker private mutex.  (We do it in this order because this is what
+      // workers best for event dispatching.)  To enforce that order here we
+      // need to unlock the worker private mutex before we lock the event target
+      // mutex in ForgetWorkerPrivate.
+      {
+        MutexAutoUnlock unlock(mMutex);
+        mWorkerHybridEventTarget->ForgetWorkerPrivate(this);
+      }
+
+      // Check the status code again in case another NotifyInternal came in
+      // while we were unlocked above.
+      if (mStatus >= aStatus) {
+        return true;
+      }
+    }
+
     previousStatus = mStatus;
     mStatus = aStatus;
 
     // Mark parent status as closing immediately to avoid new events being
     // dispatched after we clear the queue below.
     if (aStatus == Closing) {
       Close();
     }
-
-    // Make sure the hybrid event target stops dispatching runnables
-    // once we reaching the killing state.
-    if (aStatus == Killing) {
-      mWorkerHybridEventTarget->ForgetWorkerPrivate(this);
-    }
   }
 
   if (mCrossThreadDispatcher) {
     // Since we'll no longer process events, make sure we no longer allow
     // anyone to post them. We have to do this without mMutex held, since our
     // mutex must be acquired *after* mCrossThreadDispatcher's mutex when
     // they're both held.
     mCrossThreadDispatcher->Forget();