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

--HG--
extra : rebase_source : 4a611a3ad615277f9b2191919109108dd1dfde46

security/manager/ssl/nsNSSShutDown.cpp

@@ -135,12 +135,17 @@ nsresult nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown()
     return NS_ERROR_NOT_SAME_THREAD;
   }
 
+  // This makes this function idempotent and protects against reentering it
+  // (which really shouldn't happen anyway, but just in case).
+  if (sInShutdown) {
+    return NS_OK;
+  }
+
   StaticMutexAutoLock lock(sListLock);
   // Other threads can acquire an nsNSSShutDownPreventionLock and cause this
   // thread to block when it calls restructActivityToCurrentThread, below. If
-  // those other threads then attempt to create an object that must be
-  // remembered by the shut down list, they will call
-  // nsNSSShutDownList::remember, which attempts to acquire sListLock.
+  // those other threads then attempt to create an nsNSSShutDownObject, they
+  // will call nsNSSShutDownList::remember, which attempts to acquire sListLock.
   // Consequently, holding sListLock while we're in
   // restrictActivityToCurrentThread would result in deadlock. sListLock
   // protects the singleton, so if we enforce that the singleton only be created
@@ -152,6 +157,11 @@ nsresult nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown()
     return NS_OK;
   }
 
+  // Setting sInShutdown here means that threads calling
+  // nsNSSShutDownList::remember will return early (because
+  // nsNSSShutDownList::construct will return false) and not attempt to remember
+  // new objects. If they properly check isAlreadyShutDown(), they will also not
+  // attempt to call NSS functions or use NSS resources.
   sInShutdown = true;
 
   {
@@ -165,38 +175,24 @@ nsresult nsNSSShutDownList::evaporateAllNSSResourcesAndShutDown()
   }
 
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("now evaporating NSS resources"));
-
-  // Never free more than one entry, because other threads might be calling
-  // 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;
-    }
+  for (auto iter = singleton->mObjects.Iter(); !iter.Done(); iter.Next()) {
     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;
   }
 }
 
@@ -210,7 +206,11 @@ void nsNSSShutDownList::leaveActivityState()
 
 bool nsNSSShutDownList::construct(const StaticMutexAutoLock& /*proofOfLock*/)
 {
-  if (!singleton && !sInShutdown && XRE_IsParentProcess()) {
+  if (sInShutdown) {
+    return false;
+  }
+
+  if (!singleton && XRE_IsParentProcess()) {
     singleton = new nsNSSShutDownList();
   }
 
@@ -273,13 +273,16 @@ void nsNSSActivityState::releaseCurrentThreadActivityRestriction()
 }
 
 nsNSSShutDownPreventionLock::nsNSSShutDownPreventionLock()
+  : mEnteredActivityState(false)
 {
-  nsNSSShutDownList::enterActivityState();
+  nsNSSShutDownList::enterActivityState(mEnteredActivityState);
 }
 
 nsNSSShutDownPreventionLock::~nsNSSShutDownPreventionLock()
 {
-  nsNSSShutDownList::leaveActivityState();
+  if (mEnteredActivityState) {
+    nsNSSShutDownList::leaveActivityState();
+  }
 }
 
 bool

security/manager/ssl/nsNSSShutDown.h

@@ -57,6 +57,12 @@ 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,
@@ -82,7 +88,9 @@ public:
   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: