bug 1239609 - audit nsNSSShutDownObject destructors for correctness r=Cykesiopka,sworkman

dom/wifi/WifiCertService.cpp

@@ -439,6 +439,12 @@ WifiCertService::WifiCertService()
 WifiCertService::~WifiCertService()
 {
   MOZ_ASSERT(!gWifiCertService);
+
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return;
+  }
+  shutdown(calledFromObject);
 }
 
 already_AddRefed<WifiCertService>

netwerk/base/BackgroundFileSaver.cpp

@@ -1212,6 +1212,10 @@ DigestOutputStream::DigestOutputStream(nsIOutputStream* aStream,
 
 DigestOutputStream::~DigestOutputStream()
 {
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return;
+  }
   shutdown(calledFromObject);
 }
 

security/manager/ssl/nsKeyModule.cpp

@@ -99,6 +99,15 @@ nsKeyObjectFactory::nsKeyObjectFactory()
 {
 }
 
+nsKeyObjectFactory::~nsKeyObjectFactory()
+{
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return;
+  }
+  shutdown(calledFromObject);
+}
+
 NS_IMETHODIMP
 nsKeyObjectFactory::KeyFromString(int16_t aAlgorithm, const nsACString& aKey,
                                   nsIKeyObject** _retval)

security/manager/ssl/nsKeyModule.h

@@ -51,7 +51,7 @@ public:
   NS_DECL_NSIKEYOBJECTFACTORY
 
 private:
-  ~nsKeyObjectFactory() {}
+  ~nsKeyObjectFactory();
 
   // Disallow copy constructor
   nsKeyObjectFactory(nsKeyObjectFactory&);

security/manager/ssl/nsNSSShutDown.h

@@ -129,6 +129,22 @@ protected:
   the tracking list, to ensure no additional attempt to free the resources
   will be made.
 
+  ----------------------------------------------------------------------------
+  IMPORTANT NOTE REGARDING CLASSES THAT IMPLEMENT nsNSSShutDownObject BUT DO
+  NOT DIRECTLY HOLD NSS RESOURCES:
+  ----------------------------------------------------------------------------
+  Currently, classes that do not hold NSS resources but do call NSS functions
+  inherit from nsNSSShutDownObject (and use the lock/isAlreadyShutDown
+  mechanism) as a way of ensuring it is safe to call those functions. Because
+  these classes do not hold any resources, however, it is tempting to skip the
+  destructor component of this interface. This MUST NOT be done, because
+  if an object of such a class is destructed before the nsNSSShutDownList
+  processes all of its entries, this essentially causes a use-after-free when
+  nsNSSShutDownList reaches the entry that has been destroyed. The safe way to
+  do this is to implement the destructor as usual but omit the call to
+  destructorSafeDestroyNSSReference() as it is unnecessary and probably isn't
+  defined for that class.
+
   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