bug 1375709 - avoid deadlock when shutting down NSS r?Cykesiopka,ttaubert draft
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 10 Jul 2017 16:25:51 -0700
changeset 607754 6c350819e3c2ba9c965c457b6eeaf3155bc22e15
parent 607669 03bcd6d65af62c5e60a0ab9247ccce43885e707b
child 637138 e73b390ef38b73b1da58720a7a6ec150535f10da
push id68100
push userbmo:dkeeler@mozilla.com
push dateWed, 12 Jul 2017 20:41:48 +0000
reviewersCykesiopka, ttaubert
bugs1375709, 1273475
milestone56.0a1
bug 1375709 - avoid deadlock when shutting down NSS r?Cykesiopka,ttaubert The deadlock fix attempted in bug 1273475 was incomplete. This should prevent the issue by preventing nsNSSShutDownPreventionLocks from attempting to increment the NSS activity state count when shutdown is in progress (this is acceptible because when code that creates any nsNSSShutDownPreventionLocks then checks isAlreadyShutDown(), it will return true because sInShutdown is true, thus preventing that code from unsafely using NSS resources and functions). MozReview-Commit-ID: 4o5DGbU2TCq
security/manager/ssl/nsNSSShutDown.cpp
security/manager/ssl/nsNSSShutDown.h
--- a/security/manager/ssl/nsNSSShutDown.cpp
+++ b/security/manager/ssl/nsNSSShutDown.cpp
@@ -170,52 +170,54 @@ nsresult nsNSSShutDownList::evaporateAll
   // us and remove themselves while we are iterating over the list,
   // and the behaviour of changing the list while iterating is undefined.
   while (singleton) {
     auto iter = singleton->mObjects.Iter();
     if (iter.Done()) {
       break;
     }
     auto entry = static_cast<ObjectHashEntry*>(iter.Get());
-    {
-      StaticMutexAutoUnlock unlock(sListLock);
-      entry->obj->shutdown(nsNSSShutDownObject::ShutdownCalledFrom::List);
-    }
+    entry->obj->shutdown(nsNSSShutDownObject::ShutdownCalledFrom::List);
     iter.Remove();
   }
 
   if (!singleton) {
     return NS_ERROR_FAILURE;
   }
 
   singleton->mActivityState.releaseCurrentThreadActivityRestriction();
   delete singleton;
 
   return NS_OK;
 }
 
-void nsNSSShutDownList::enterActivityState()
+void nsNSSShutDownList::enterActivityState(/*out*/ bool& enteredActivityState)
 {
   StaticMutexAutoLock lock(sListLock);
   if (nsNSSShutDownList::construct(lock)) {
     singleton->mActivityState.enter();
+    enteredActivityState = true;
   }
 }
 
 void nsNSSShutDownList::leaveActivityState()
 {
   StaticMutexAutoLock lock(sListLock);
   if (singleton) {
     singleton->mActivityState.leave();
   }
 }
 
 bool nsNSSShutDownList::construct(const StaticMutexAutoLock& /*proofOfLock*/)
 {
-  if (!singleton && !sInShutdown && XRE_IsParentProcess()) {
+  if (sInShutdown) {
+    return false;
+  }
+
+  if (!singleton && XRE_IsParentProcess()) {
     singleton = new nsNSSShutDownList();
   }
 
   return !!singleton;
 }
 
 nsNSSActivityState::nsNSSActivityState()
 :mNSSActivityStateLock("nsNSSActivityState.mNSSActivityStateLock"),
@@ -268,22 +270,25 @@ void nsNSSActivityState::releaseCurrentT
   MutexAutoLock lock(mNSSActivityStateLock);
 
   mNSSRestrictedThread = nullptr;
 
   mNSSActivityChanged.NotifyAll();
 }
 
 nsNSSShutDownPreventionLock::nsNSSShutDownPreventionLock()
+  : mEnteredActivityState(false)
 {
-  nsNSSShutDownList::enterActivityState();
+  nsNSSShutDownList::enterActivityState(mEnteredActivityState);
 }
 
 nsNSSShutDownPreventionLock::~nsNSSShutDownPreventionLock()
 {
-  nsNSSShutDownList::leaveActivityState();
+  if (mEnteredActivityState) {
+    nsNSSShutDownList::leaveActivityState();
+  }
 }
 
 bool
 nsNSSShutDownObject::isAlreadyShutDown() const
 {
   return mAlreadyShutDown || sInShutdown;
 }
--- a/security/manager/ssl/nsNSSShutDown.h
+++ b/security/manager/ssl/nsNSSShutDown.h
@@ -52,16 +52,22 @@ private:
 };
 
 // Helper class that automatically enters/leaves the global activity state
 class nsNSSShutDownPreventionLock
 {
 public:
   nsNSSShutDownPreventionLock();
   ~nsNSSShutDownPreventionLock();
+private:
+  // Keeps track of whether or not we actually managed to enter the NSS activity
+  // state. This is important because if we're attempting to shut down and we
+  // can't enter the NSS activity state, we need to not attempt to leave it when
+  // our destructor runs.
+  bool mEnteredActivityState;
 };
 
 // Singleton, used by nsNSSComponent to track the list of PSM objects,
 // which hold NSS resources and support the "early cleanup mechanism".
 class nsNSSShutDownList
 {
 public:
   // track instances that support early cleanup
@@ -77,17 +83,19 @@ public:
   // using NSS functions.
   static nsresult evaporateAllNSSResourcesAndShutDown();
 
   // PSM has been asked to log out of a token.
   // Notify all registered instances that want to react to that event.
   static nsresult doPK11Logout();
 
   // Signal entering/leaving a scope where shutting down NSS is prohibited.
-  static void enterActivityState();
+  // enteredActivityState will be set to true if we actually managed to enter
+  // the NSS activity state.
+  static void enterActivityState(/*out*/ bool& enteredActivityState);
   static void leaveActivityState();
 
 private:
   static bool construct(const mozilla::StaticMutexAutoLock& /*proofOfLock*/);
 
   nsNSSShutDownList();
   ~nsNSSShutDownList();