bug 957368 - standardize and simplify nsNSSShutDownObject implementations r=cviecco r=briansmith
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 14 Jan 2014 09:28:43 -0800
changeset 163405 221049daa8dd83f01829727a508b6d5f66ccdc35
parent 163404 3be73d80464dec037f8dad11d182435df35305f5
child 163406 e2c5499e4333bb534dd8f68265bee194d3edc227
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewerscviecco, briansmith
bugs957368
milestone29.0a1
bug 957368 - standardize and simplify nsNSSShutDownObject implementations r=cviecco r=briansmith
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
netwerk/base/src/BackgroundFileSaver.cpp
security/manager/ssl/src/nsCMS.cpp
security/manager/ssl/src/nsCryptoHash.cpp
security/manager/ssl/src/nsNSSCertCache.cpp
security/manager/ssl/src/nsNSSCertificate.cpp
security/manager/ssl/src/nsNSSShutDown.h
security/manager/ssl/src/nsPK11TokenDB.cpp
security/manager/ssl/src/nsPKCS11Slot.cpp
toolkit/identity/IdentityCryptoService.cpp
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -497,18 +497,24 @@ PeerConnectionImpl::~PeerConnectionImpl(
     CSFLogError(logTag, "PeerConnectionCtx is already gone. Ignoring...");
   }
 
   CSFLogInfo(logTag, "%s: PeerConnectionImpl destructor invoked for %s",
              __FUNCTION__, mHandle.c_str());
   CloseInt();
 
 #ifdef MOZILLA_INTERNAL_API
-  // Deregister as an NSS Shutdown Object
-  shutdown(calledFromObject);
+  {
+    // Deregister as an NSS Shutdown Object
+    nsNSSShutDownPreventionLock locker;
+    if (!isAlreadyShutDown()) {
+      destructorSafeDestroyNSSReference();
+      shutdown(calledFromObject);
+    }
+  }
 #endif
 
   // Since this and Initialize() occur on MainThread, they can't both be
   // running at once
 
   // Right now, we delete PeerConnectionCtx at XPCOM shutdown only, but we
   // probably want to shut it down more aggressively to save memory.  We
   // could shut down here when there are no uses.  It might be more optimal
@@ -1607,16 +1613,22 @@ PeerConnectionImpl::ShutdownMedia()
 
 #ifdef MOZILLA_INTERNAL_API
 // If NSS is shutting down, then we need to get rid of the DTLS
 // identity right now; otherwise, we'll cause wreckage when we do
 // finally deallocate it in our destructor.
 void
 PeerConnectionImpl::virtualDestroyNSSReference()
 {
+  destructorSafeDestroyNSSReference();
+}
+
+void
+PeerConnectionImpl::destructorSafeDestroyNSSReference()
+{
   MOZ_ASSERT(NS_IsMainThread());
   CSFLogDebug(logTag, "%s: NSS shutting down; freeing our DtlsIdentity.", __FUNCTION__);
   mIdentity = nullptr;
 }
 #endif
 
 void
 PeerConnectionImpl::onCallEvent(const OnCallEventArgs& args)
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -518,16 +518,17 @@ private:
     NS_ENSURE_SUCCESS(mThread->IsOnCurrentThread(&on), false);
     NS_ENSURE_TRUE(on, false);
 #endif
     return true;
   }
 
 #ifdef MOZILLA_INTERNAL_API
   void virtualDestroyNSSReference() MOZ_FINAL;
+  void destructorSafeDestroyNSSReference();
   nsresult GetTimeSinceEpoch(DOMHighResTimeStamp *result);
 #endif
 
   // Shut down media - called on main thread only
   void ShutdownMedia();
 
   // ICE callbacks run on the right thread.
   nsresult IceConnectionStateChange_m(
--- a/netwerk/base/src/BackgroundFileSaver.cpp
+++ b/netwerk/base/src/BackgroundFileSaver.cpp
@@ -91,27 +91,27 @@ BackgroundFileSaver::BackgroundFileSaver
 , mActualTarget(nullptr)
 , mActualTargetKeepPartial(false)
 , mDigestContext(nullptr)
 {
 }
 
 BackgroundFileSaver::~BackgroundFileSaver()
 {
+  nsNSSShutDownPreventionLock lock;
+  if (isAlreadyShutDown()) {
+    return;
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void
 BackgroundFileSaver::destructorSafeDestroyNSSReference()
 {
-  nsNSSShutDownPreventionLock lock;
-  if (isAlreadyShutDown()) {
-    return;
-  }
   if (mDigestContext) {
     mozilla::psm::PK11_DestroyContext_true(mDigestContext.forget());
     mDigestContext = nullptr;
   }
 }
 
 void
 BackgroundFileSaver::virtualDestroyNSSReference()
--- a/security/manager/ssl/src/nsCMS.cpp
+++ b/security/manager/ssl/src/nsCMS.cpp
@@ -36,33 +36,30 @@ nsCMSMessage::nsCMSMessage()
 nsCMSMessage::nsCMSMessage(NSSCMSMessage *aCMSMsg)
 {
   m_cmsMsg = aCMSMsg;
 }
 
 nsCMSMessage::~nsCMSMessage()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsCMSMessage::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsCMSMessage::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (m_cmsMsg) {
     NSS_CMSMessage_Destroy(m_cmsMsg);
   }
 }
 
 NS_IMETHODIMP nsCMSMessage::VerifySignature()
 {
   return CommonVerifySignature(nullptr, 0);
@@ -379,33 +376,30 @@ public:
   nsZeroTerminatedCertArray()
   :mCerts(nullptr), mPoolp(nullptr), mSize(0)
   {
   }
   
   ~nsZeroTerminatedCertArray()
   {
     nsNSSShutDownPreventionLock locker;
-    if (isAlreadyShutDown())
+    if (isAlreadyShutDown()) {
       return;
-
+    }
     destructorSafeDestroyNSSReference();
     shutdown(calledFromObject);
   }
 
   void virtualDestroyNSSReference()
   {
     destructorSafeDestroyNSSReference();
   }
 
   void destructorSafeDestroyNSSReference()
   {
-    if (isAlreadyShutDown())
-      return;
-
     if (mCerts)
     {
       for (uint32_t i=0; i < mSize; i++) {
         if (mCerts[i]) {
           CERT_DestroyCertificate(mCerts[i]);
         }
       }
     }
@@ -728,33 +722,30 @@ NS_IMPL_ISUPPORTS1(nsCMSDecoder, nsICMSD
 nsCMSDecoder::nsCMSDecoder()
 : m_dcx(nullptr)
 {
 }
 
 nsCMSDecoder::~nsCMSDecoder()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsCMSDecoder::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsCMSDecoder::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (m_dcx) {
     NSS_CMSDecoder_Cancel(m_dcx);
     m_dcx = nullptr;
   }
 }
 
 /* void start (in NSSCMSContentCallback cb, in voidPtr arg); */
 NS_IMETHODIMP nsCMSDecoder::Start(NSSCMSContentCallback cb, void * arg)
@@ -814,34 +805,30 @@ NS_IMPL_ISUPPORTS1(nsCMSEncoder, nsICMSE
 nsCMSEncoder::nsCMSEncoder()
 : m_ecx(nullptr)
 {
 }
 
 nsCMSEncoder::~nsCMSEncoder()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsCMSEncoder::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsCMSEncoder::destructorSafeDestroyNSSReference()
 {
-  nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
-    return;
-
   if (m_ecx)
     NSS_CMSEncoder_Cancel(m_ecx);
 }
 
 /* void start (); */
 NS_IMETHODIMP nsCMSEncoder::Start(nsICMSMessage *aMsg, NSSCMSContentCallback cb, void * arg)
 {
   nsNSSShutDownPreventionLock locker;
--- a/security/manager/ssl/src/nsCryptoHash.cpp
+++ b/security/manager/ssl/src/nsCryptoHash.cpp
@@ -31,34 +31,30 @@ nsCryptoHash::nsCryptoHash()
   : mHashContext(nullptr)
   , mInitialized(false)
 {
 }
 
 nsCryptoHash::~nsCryptoHash()
 {
   nsNSSShutDownPreventionLock locker;
-
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsCryptoHash::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsCryptoHash::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (mHashContext)
     HASH_Destroy(mHashContext);
   mHashContext = nullptr;
 }
 
 NS_IMPL_ISUPPORTS1(nsCryptoHash, nsICryptoHash)
 
 NS_IMETHODIMP 
@@ -217,34 +213,30 @@ NS_IMPL_ISUPPORTS1(nsCryptoHMAC, nsICryp
 nsCryptoHMAC::nsCryptoHMAC()
 {
   mHMACContext = nullptr;
 }
 
 nsCryptoHMAC::~nsCryptoHMAC()
 {
   nsNSSShutDownPreventionLock locker;
-
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsCryptoHMAC::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsCryptoHMAC::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (mHMACContext)
     PK11_DestroyContext(mHMACContext, true);
   mHMACContext = nullptr;
 }
 
 /* void init (in unsigned long aAlgorithm, in nsIKeyObject aKeyObject); */
 NS_IMETHODIMP nsCryptoHMAC::Init(uint32_t aAlgorithm, nsIKeyObject *aKeyObject)
 {
--- a/security/manager/ssl/src/nsNSSCertCache.cpp
+++ b/security/manager/ssl/src/nsNSSCertCache.cpp
@@ -16,32 +16,31 @@ NS_IMPL_ISUPPORTS1(nsNSSCertCache, nsINS
 nsNSSCertCache::nsNSSCertCache()
 :mutex("nsNSSCertCache.mutex"), mCertList(nullptr)
 {
 }
 
 nsNSSCertCache::~nsNSSCertCache()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsNSSCertCache::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsNSSCertCache::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
+  mCertList = nullptr;
 }
 
 NS_IMETHODIMP
 nsNSSCertCache::CacheAllCerts()
 {
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown())
     return NS_ERROR_NOT_AVAILABLE;
--- a/security/manager/ssl/src/nsNSSCertificate.cpp
+++ b/security/manager/ssl/src/nsNSSCertificate.cpp
@@ -167,33 +167,30 @@ nsNSSCertificate::nsNSSCertificate() :
 {
   if (GeckoProcessType_Default != XRE_GetProcessType())
     NS_ERROR("Trying to initialize nsNSSCertificate in a non-chrome process!");
 }
 
 nsNSSCertificate::~nsNSSCertificate()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsNSSCertificate::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsNSSCertificate::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (mPermDelete) {
     if (mCertType == nsNSSCertificate::USER_CERT) {
       nsCOMPtr<nsIInterfaceRequestor> cxt = new PipUIContext();
       PK11_DeleteTokenCertAndKey(mCert, cxt);
     } else if (!PK11_IsReadOnly(mCert->slot)) {
       // If the list of built-ins does contain a non-removable
       // copy of this certificate, our call will not remove
       // the certificate permanently, but rather remove all trust.
@@ -1507,30 +1504,30 @@ nsNSSCertList::nsNSSCertList(CERTCertLis
   } else {
     mCertList = CERT_NewCertList();
   }
 }
 
 nsNSSCertList::~nsNSSCertList()
 {
   nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return;
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsNSSCertList::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsNSSCertList::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown()) {
-    return;
-  }
   if (mCertList) {
     mCertList = nullptr;
   }
 }
 
 /* void addCert (in nsIX509Cert cert); */
 NS_IMETHODIMP
 nsNSSCertList::AddCert(nsIX509Cert *aCert) 
@@ -1644,30 +1641,30 @@ nsNSSCertListEnumerator::nsNSSCertListEn
                                                  const nsNSSShutDownPreventionLock &proofOfLock)
 {
   mCertList = nsNSSCertList::DupCertList(certList, proofOfLock);
 }
 
 nsNSSCertListEnumerator::~nsNSSCertListEnumerator()
 {
   nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return;
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsNSSCertListEnumerator::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsNSSCertListEnumerator::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown()) {
-    return;
-  }
   if (mCertList) {
     mCertList = nullptr;
   }
 }
 
 /* boolean hasMoreElements (); */
 NS_IMETHODIMP
 nsNSSCertListEnumerator::HasMoreElements(bool *_retval)
--- a/security/manager/ssl/src/nsNSSShutDown.h
+++ b/security/manager/ssl/src/nsNSSShutDown.h
@@ -180,49 +180,51 @@ protected:
   also be called from the destructor of the deriving class,
   which is the standard cleanup (not called from the tracking list).
 
   Because of that duplication, it is suggested to implement a
   function destructorSafeDestroyNSSReference() in the deriving
   class, and make the implementation of virtualDestroyNSSReference()
   call destructorSafeDestroyNSSReference().
 
-  The destructor of the derived class should call 
-  destructorSafeDestroyNSSReference() and afterwards call
-  shutdown(calledFromObject), in order to deregister with the
-  tracking list, to ensure no additional attempt to free the resources
+  The destructor of the derived class must prevent NSS shutdown on
+  another thread by acquiring an nsNSSShutDownPreventionLock. It must
+  then check to see if NSS has already been shut down by calling
+  isAlreadyShutDown(). If NSS has not been shut down, the destructor
+  must then call destructorSafeDestroyNSSReference() and then
+  shutdown(calledFromObject). The second call will deregister with
+  the tracking list, to ensure no additional attempt to free the resources
   will be made.
-  
-  Function destructorSafeDestroyNSSReference() must
-  also ensure, that NSS resources have not been freed already.
-  To achieve this, the deriving class should call 
-  isAlreadyShutDown() to check.
-  
-  It is important that you make your implementation
-  failsafe, and check whether the resources have already been freed,
-  in each function that requires the resources.
-  
+
+  destructorSafeDestroyNSSReference() does not need to acquire an
+  nsNSSShutDownPreventionLock or check isAlreadyShutDown() as long as it
+  is only called by the destructor that has already acquired the lock and
+  checked for shutdown or by the NSS shutdown code itself (which acquires
+  the same lock and checks if objects it cleans up have already cleaned
+  up themselves).
+
   class derivedClass : public nsISomeInterface,
                        public nsNSSShutDownObject
   {
     virtual void virtualDestroyNSSReference()
     {
       destructorSafeDestroyNSSReference();
     }
     
     void destructorSafeDestroyNSSReference()
     {
-      if (isAlreadyShutDown())
-        return;
-      
       // clean up all NSS resources here
     }
 
     virtual ~derivedClass()
     {
+      nsNSSShutDownPreventionLock locker;
+      if (isAlreadyShutDown()) {
+        return;
+      }
       destructorSafeDestroyNSSReference();
       shutdown(calledFromObject);
     }
     
     NS_IMETHODIMP doSomething()
     {
       if (isAlreadyShutDown())
         return NS_ERROR_NOT_AVAILABLE;
--- a/security/manager/ssl/src/nsPK11TokenDB.cpp
+++ b/security/manager/ssl/src/nsPK11TokenDB.cpp
@@ -78,33 +78,30 @@ nsPK11Token::refreshTokenInfo()
     mTokenSerialNum.Trim(" ", false, true);
   }
 
 }
 
 nsPK11Token::~nsPK11Token()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsPK11Token::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsPK11Token::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (mSlot) {
     PK11_FreeSlot(mSlot);
     mSlot = nullptr;
   }
 }
 
 /* readonly attribute wstring tokenName; */
 NS_IMETHODIMP nsPK11Token::GetTokenName(char16_t * *aTokenName)
--- a/security/manager/ssl/src/nsPKCS11Slot.cpp
+++ b/security/manager/ssl/src/nsPKCS11Slot.cpp
@@ -62,33 +62,30 @@ nsPKCS11Slot::refreshSlotInfo()
     mSlotFWVersion.AppendInt(slot_info.firmwareVersion.minor);
   }
 
 }
 
 nsPKCS11Slot::~nsPKCS11Slot()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsPKCS11Slot::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsPKCS11Slot::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (mSlot) {
     PK11_FreeSlot(mSlot);
     mSlot = nullptr;
   }
 }
 
 /* readonly attribute wstring name; */
 NS_IMETHODIMP 
@@ -236,33 +233,30 @@ nsPKCS11Module::nsPKCS11Module(SECMODMod
 
   SECMOD_ReferenceModule(module);
   mModule = module;
 }
 
 nsPKCS11Module::~nsPKCS11Module()
 {
   nsNSSShutDownPreventionLock locker;
-  if (isAlreadyShutDown())
+  if (isAlreadyShutDown()) {
     return;
-
+  }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
 void nsPKCS11Module::virtualDestroyNSSReference()
 {
   destructorSafeDestroyNSSReference();
 }
 
 void nsPKCS11Module::destructorSafeDestroyNSSReference()
 {
-  if (isAlreadyShutDown())
-    return;
-
   if (mModule) {
     SECMOD_DestroyModule(mModule);
     mModule = nullptr;
   }
 }
 
 /* readonly attribute wstring name; */
 NS_IMETHODIMP 
--- a/toolkit/identity/IdentityCryptoService.cpp
+++ b/toolkit/identity/IdentityCryptoService.cpp
@@ -96,31 +96,31 @@ public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIIDENTITYKEYPAIR
 
   KeyPair(SECKEYPrivateKey* aPrivateKey, SECKEYPublicKey* aPublicKey);
 
 private:
   ~KeyPair()
   {
+    nsNSSShutDownPreventionLock locker;
+    if (isAlreadyShutDown()) {
+      return;
+    }
     destructorSafeDestroyNSSReference();
     shutdown(calledFromObject);
   }
 
   void virtualDestroyNSSReference() MOZ_OVERRIDE
   {
     destructorSafeDestroyNSSReference();
   }
 
   void destructorSafeDestroyNSSReference()
   {
-    nsNSSShutDownPreventionLock locker;
-    if (isAlreadyShutDown())
-      return;
-
     SECKEY_DestroyPrivateKey(mPrivateKey);
     mPrivateKey = nullptr;
     SECKEY_DestroyPublicKey(mPublicKey);
     mPublicKey = nullptr;
   }
 
   SECKEYPrivateKey * mPrivateKey;
   SECKEYPublicKey * mPublicKey;
@@ -136,32 +136,32 @@ class KeyGenRunnable : public nsRunnable
 public:
   NS_DECL_NSIRUNNABLE
 
   KeyGenRunnable(KeyType keyType, nsIIdentityKeyGenCallback * aCallback);
 
 private:
   ~KeyGenRunnable()
   {
+    nsNSSShutDownPreventionLock locker;
+    if (isAlreadyShutDown()) {
+      return;
+    }
     destructorSafeDestroyNSSReference();
     shutdown(calledFromObject);
   }
 
   virtual void virtualDestroyNSSReference() MOZ_OVERRIDE
   {
     destructorSafeDestroyNSSReference();
   }
 
   void destructorSafeDestroyNSSReference()
   {
-    nsNSSShutDownPreventionLock locker;
-    if (isAlreadyShutDown())
-      return;
-
-     mKeyPair = nullptr;
+    mKeyPair = nullptr;
   }
 
   const KeyType mKeyType; // in
   nsMainThreadPtrHandle<nsIIdentityKeyGenCallback> mCallback; // in
   nsresult mRv; // out
   nsCOMPtr<KeyPair> mKeyPair; // out
 
   KeyGenRunnable(const KeyGenRunnable &) MOZ_DELETE;
@@ -174,31 +174,31 @@ public:
   NS_DECL_NSIRUNNABLE
 
   SignRunnable(const nsACString & textToSign, SECKEYPrivateKey * privateKey,
                nsIIdentitySignCallback * aCallback);
 
 private:
   ~SignRunnable()
   {
+    nsNSSShutDownPreventionLock locker;
+    if (isAlreadyShutDown()) {
+      return;
+    }
     destructorSafeDestroyNSSReference();
     shutdown(calledFromObject);
   }
 
   void virtualDestroyNSSReference() MOZ_OVERRIDE
   {
     destructorSafeDestroyNSSReference();
   }
 
   void destructorSafeDestroyNSSReference()
   {
-    nsNSSShutDownPreventionLock locker;
-    if (isAlreadyShutDown())
-      return;
-
     SECKEY_DestroyPrivateKey(mPrivateKey);
     mPrivateKey = nullptr;
   }
 
   const nsCString mTextToSign; // in
   SECKEYPrivateKey* mPrivateKey; // in
   nsMainThreadPtrHandle<nsIIdentitySignCallback> mCallback; // in
   nsresult mRv; // out