bug 1502841 - fix a deadlock by not holding the DataStorageSharedThread lock while shutting the thread down r=jcj a=jcristau
authorDana Keeler <dkeeler@mozilla.com>
Wed, 14 Nov 2018 00:28:52 +0000
changeset 501414 ce690bc5055827ce9a768a24472ed270b21231c3
parent 501413 a5dfb5153b78bc8fd4838c885c92318fe45a3116
child 501415 140ce47ccf2bb401dfddc6776de3ff3bfdadcc92
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj, jcristau
bugs1502841
milestone64.0
bug 1502841 - fix a deadlock by not holding the DataStorageSharedThread lock while shutting the thread down r=jcj a=jcristau Judging by some stack traces we've received in crash reports, while shutting down the DataStorageSharedThread, it is possible to process an event on that thread that causes an attempt to re-initialize DataStorage. This wouldn't be a problem because we have a shutdown sentinel boolean and we exit early if it is true. However, checking the boolean involves acquiring the static lock for the thread, which means we can't be holding the lock while we're shutting down the thread. Differential Revision: https://phabricator.services.mozilla.com/D11708
security/manager/ssl/DataStorage.cpp
--- a/security/manager/ssl/DataStorage.cpp
+++ b/security/manager/ssl/DataStorage.cpp
@@ -114,31 +114,43 @@ DataStorageSharedThread::Shutdown()
     return NS_OK;
   }
 
   MOZ_ASSERT(gDataStorageSharedThread->mThread);
   if (!gDataStorageSharedThread->mThread) {
     return NS_ERROR_FAILURE;
   }
 
-  nsresult rv = gDataStorageSharedThread->mThread->Shutdown();
+  // We can't hold sDataStorageSharedThreadMutex while shutting down the thread,
+  // because we might process events that try to acquire it (e.g. via
+  // DataStorage::GetAllChildProcessData). So, we set our shutdown sentinel
+  // boolean to true, get a handle on the thread, release the lock, shut down
+  // the thread (thus processing all pending events on it), and re-acquire the
+  // lock.
+  gDataStorageSharedThreadShutDown = true;
+  nsCOMPtr<nsIThread> threadHandle = gDataStorageSharedThread->mThread;
+  nsresult rv;
+  {
+    StaticMutexAutoUnlock unlock(sDataStorageSharedThreadMutex);
+    rv = threadHandle->Shutdown();
+  }
   gDataStorageSharedThread->mThread = nullptr;
-  gDataStorageSharedThreadShutDown = true;
   delete gDataStorageSharedThread;
   gDataStorageSharedThread = nullptr;
 
   return rv;
 }
 
 nsresult
 DataStorageSharedThread::Dispatch(nsIRunnable* event)
 {
   MOZ_ASSERT(XRE_IsParentProcess());
   StaticMutexAutoLock lock(sDataStorageSharedThreadMutex);
-  if (!gDataStorageSharedThread || !gDataStorageSharedThread->mThread) {
+  if (gDataStorageSharedThreadShutDown || !gDataStorageSharedThread ||
+      !gDataStorageSharedThread->mThread) {
     return NS_ERROR_FAILURE;
   }
   return gDataStorageSharedThread->mThread->Dispatch(event, NS_DISPATCH_NORMAL);
 }
 
 } // unnamed namespace
 
 namespace mozilla {