bug 1555854 - avoid creating transient threads in PSM (particularly CryptoTask) r=KevinJacobs

CryptoTask is a helper class that makes it easier to implement code that runs on
a background thread and then notifies completion on the main thread (this is
useful for not blocking the main thread with long-running cryptography or I/O).
Before this patch, each CryptoTask would create a new thread each time it ran,
which was inefficient. This patch updates CryptoTask to use the stream transport
service (which is essentially a pool of threads for doing exactly these kinds of
things and notably is not to be confused with the socket transport service) to
run each task. Additionally, there were a few places in PSM where we
unnecessarily created new threads to perform similar tasks. These now use the
stream transport service as well.

Differential Revision: https://phabricator.services.mozilla.com/D33534

--HG--
extra : moz-landing-system : lando

security/apps/AppSignatureVerification.cpp

@@ -1368,5 +1368,5 @@ nsNSSCertificateDB::OpenSignedAppFileAsync(
   SignaturePolicy policy(policyInt);
   RefPtr<OpenSignedAppFileTask> task(
       new OpenSignedAppFileTask(aTrustedRoot, aJarFile, policy, aCallback));
-  return task->Dispatch("SignedJAR");
+  return task->Dispatch();
 }

security/manager/ssl/ContentSignatureVerifier.cpp

@@ -17,6 +17,7 @@
 #include "mozilla/dom/Promise.h"
 #include "nsCOMPtr.h"
 #include "nsPromiseFlatString.h"
+#include "nsProxyRelease.h"
 #include "nsSecurityHeaderParser.h"
 #include "nsWhitespaceTokenizer.h"
 #include "mozpkix/pkix.h"
@@ -50,7 +51,8 @@ class VerifyContentSignatureTask : public CryptoTask {
         mCertChain(aCertChain),
         mHostname(aHostname),
         mSignatureVerified(false),
-        mPromise(aPromise) {}
+        mPromise(new nsMainThreadPtrHolder<Promise>(
+            "VerifyContentSignatureTask::mPromise", aPromise)) {}
 
  private:
   virtual nsresult CalculateResult() override;
@@ -61,7 +63,7 @@ class VerifyContentSignatureTask : public CryptoTask {
   nsCString mCertChain;
   nsCString mHostname;
   bool mSignatureVerified;
-  RefPtr<Promise> mPromise;
+  nsMainThreadPtrHandle<Promise> mPromise;
 };
 
 NS_IMETHODIMP
@@ -84,7 +86,7 @@ ContentSignatureVerifier::AsyncVerifyContentSignature(
 
   RefPtr<VerifyContentSignatureTask> task(new VerifyContentSignatureTask(
       aData, aCSHeader, aCertChain, aHostname, promise));
-  nsresult rv = task->Dispatch("ContentSig");
+  nsresult rv = task->Dispatch();
   if (NS_FAILED(rv)) {
     return rv;
   }

security/manager/ssl/CryptoTask.cpp

@@ -6,28 +6,26 @@
 
 #include "CryptoTask.h"
 #include "nsNSSComponent.h"
+#include "nsNetCID.h"
 
 namespace mozilla {
 
-nsresult CryptoTask::Dispatch(const nsACString& taskThreadName) {
+nsresult CryptoTask::Dispatch() {
-  MOZ_ASSERT(taskThreadName.Length() <= 15);
-
   // Ensure that NSS is initialized, since presumably CalculateResult
   // will use NSS functions
   if (!EnsureNSSInitializedChromeOrContent()) {
     return NS_ERROR_FAILURE;
   }
 
-  // Can't add 'this' as the event to run, since mThread may not be set yet
+  // The stream transport service (note: not the socket transport service) can
-  nsresult rv =
+  // be used to perform background tasks or I/O that would otherwise block the
-      NS_NewNamedThread(taskThreadName, getter_AddRefs(mThread), nullptr,
+  // main thread.
-                        nsIThreadManager::DEFAULT_STACK_SIZE);
+  nsCOMPtr<nsIEventTarget> target(
-  if (NS_FAILED(rv)) {
+      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID));
-    return rv;
+  if (!target) {
+    return NS_ERROR_FAILURE;
   }
-
+  return target->Dispatch(this, NS_DISPATCH_NORMAL);
-  // Note: event must not null out mThread!
-  return mThread->Dispatch(this, NS_DISPATCH_NORMAL);
 }
 
 NS_IMETHODIMP
@@ -37,21 +35,8 @@ CryptoTask::Run() {
     NS_DispatchToMainThread(this);
   } else {
     // back on the main thread
-
     CallCallback(mRv);
-
-    // Not all uses of CryptoTask use a transient thread
-    if (mThread) {
-      // Don't leak threads!
-      mThread->Shutdown();  // can't Shutdown from the thread itself, darn
-      // Don't null out mThread!
-      // See bug 999104.  We must hold a ref to the thread across Dispatch()
-      // since the internal mThread ref could be released while processing
-      // the Dispatch(), and Dispatch/PutEvent itself doesn't hold a ref; it
-      // assumes the caller does.
   }
-  }
-
   return NS_OK;
 }
 

security/manager/ssl/CryptoTask.h

@@ -23,13 +23,7 @@ namespace mozilla {
  */
 class CryptoTask : public Runnable {
  public:
-  template <size_t LEN>
+  nsresult Dispatch();
-  nsresult Dispatch(const char (&taskThreadName)[LEN]) {
-    static_assert(LEN <= 15, "Thread name must be no more than 15 characters");
-    return Dispatch(nsDependentCString(taskThreadName, LEN - 1));
-  }
-
-  nsresult Dispatch(const nsACString& taskThreadName);
 
  protected:
   CryptoTask() : Runnable("CryptoTask"), mRv(NS_ERROR_NOT_INITIALIZED) {}
@@ -51,7 +45,6 @@ class CryptoTask : public Runnable {
   NS_IMETHOD Run() final;
 
   nsresult mRv;
-  nsCOMPtr<nsIThread> mThread;
 };
 
 }  // namespace mozilla

security/manager/ssl/LocalCertService.cpp

@@ -402,7 +402,7 @@ LocalCertService::GetOrCreateCert(const nsACString& aNickname,
   }
 
   RefPtr<LocalCertGetTask> task(new LocalCertGetTask(aNickname, aCallback));
-  return task->Dispatch("LocalCertGet");
+  return task->Dispatch();
 }
 
 NS_IMETHODIMP
@@ -424,7 +424,7 @@ LocalCertService::RemoveCert(const nsACString& aNickname,
 
   RefPtr<LocalCertRemoveTask> task(
       new LocalCertRemoveTask(aNickname, aCallback));
-  return task->Dispatch("LocalCertRm");
+  return task->Dispatch();
 }
 
 NS_IMETHODIMP

security/manager/ssl/OSReauthenticator.cpp

@@ -7,6 +7,7 @@
 #include "OSReauthenticator.h"
 
 #include "OSKeyStore.h"
+#include "nsNetCID.h"
 
 NS_IMPL_ISUPPORTS(OSReauthenticator, nsIOSReauthenticator)
 
@@ -240,9 +241,12 @@ OSReauthenticator::AsyncReauthenticateUser(const nsACString& aPrompt,
         BackgroundReauthenticateUser(promiseHandle, aPrompt);
       }));
 
-  nsCOMPtr<nsIThread> thread;
+  nsCOMPtr<nsIEventTarget> target(
-  rv = NS_NewNamedThread(NS_LITERAL_CSTRING("ReauthenticateUserThread"),
+      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID));
-                         getter_AddRefs(thread), runnable);
+  if (!target) {
+    return NS_ERROR_FAILURE;
+  }
+  rv = target->Dispatch(runnable, NS_DISPATCH_NORMAL);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }

security/manager/ssl/SecretDecoderRing.cpp

@@ -20,6 +20,7 @@
 #include "nsITokenPasswordDialogs.h"
 #include "nsNSSComponent.h"
 #include "nsNSSHelper.h"
+#include "nsNetCID.h"
 #include "nsPK11TokenDB.h"
 #include "pk11func.h"
 #include "pk11sdr.h"  // For PK11SDR_Encrypt, PK11SDR_Decrypt
@@ -170,9 +171,12 @@ SecretDecoderRing::AsyncEncryptStrings(uint32_t plaintextsCount,
         BackgroundSdrEncryptStrings(plaintextsUtf8, promise);
       }));
 
-  nsCOMPtr<nsIThread> encryptionThread;
+  nsCOMPtr<nsIEventTarget> target(
-  nsresult rv = NS_NewNamedThread("AsyncSDRThread",
+      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID));
-                                  getter_AddRefs(encryptionThread), runnable);
+  if (!target) {
+    return NS_ERROR_FAILURE;
+  }
+  nsresult rv = target->Dispatch(runnable, NS_DISPATCH_NORMAL);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }

security/manager/ssl/nsNSSCertificateDB.cpp

@@ -1223,7 +1223,7 @@ nsNSSCertificateDB::AsyncVerifyCertAtTime(
     nsICertVerificationCallback* aCallback) {
   RefPtr<VerifyCertAtTimeTask> task(new VerifyCertAtTimeTask(
       aCert, aUsage, aFlags, aHostname, aTime, aCallback));
-  return task->Dispatch("VerifyCert");
+  return task->Dispatch();
 }
 
 NS_IMETHODIMP

security/manager/ssl/nsNSSComponent.cpp

@@ -44,6 +44,7 @@
 #include "nsLiteralString.h"
 #include "nsNSSCertificateDB.h"
 #include "nsNSSHelper.h"
+#include "nsNetCID.h"
 #include "nsPK11TokenDB.h"
 #include "nsPrintfCString.h"
 #include "nsServiceManagerUtils.h"
@@ -529,7 +530,7 @@ void nsNSSComponent::MaybeImportEnterpriseRoots() {
   if (importEnterpriseRoots) {
     RefPtr<BackgroundImportEnterpriseCertsTask> task =
         new BackgroundImportEnterpriseCertsTask(this);
-    Unused << task->Dispatch("EnterpriseCrts");
+    Unused << task->Dispatch();
   }
 }
 
@@ -611,40 +612,22 @@ class LoadLoadableRootsTask final : public Runnable {
   bool mImportEnterpriseRoots;
   uint32_t mFamilySafetyMode;
   Vector<nsCString> mPossibleLoadableRootsLocations;
-  nsCOMPtr<nsIThread> mThread;
 };
 
 nsresult LoadLoadableRootsTask::Dispatch() {
-  // Can't add 'this' as the event to run, since mThread may not be set yet
+  // The stream transport service (note: not the socket transport service) can
-  nsresult rv = NS_NewNamedThread("LoadRoots", getter_AddRefs(mThread), nullptr,
+  // be used to perform background tasks or I/O that would otherwise block the
-                                  nsIThreadManager::DEFAULT_STACK_SIZE);
+  // main thread.
-  if (NS_FAILED(rv)) {
+  nsCOMPtr<nsIEventTarget> target(
-    return rv;
+      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID));
+  if (!target) {
+    return NS_ERROR_FAILURE;
   }
-
+  return target->Dispatch(this, NS_DISPATCH_NORMAL);
-  // Note: event must not null out mThread!
-  return mThread->Dispatch(this, NS_DISPATCH_NORMAL);
 }
 
 NS_IMETHODIMP
 LoadLoadableRootsTask::Run() {
-  // First we Run() on the "LoadRoots" thread, do our work, and then we Run()
-  // again on the main thread so we can shut down the thread (since we don't
-  // need it any more). We can't shut down the thread while we're *on* the
-  // thread, which is why we do the dispatch to the main thread. CryptoTask.cpp
-  // (which informs this code) says that we can't null out mThread. This appears
-  // to be because its refcount could be decreased while this dispatch is being
-  // processed, so it might get prematurely destroyed. I'm not sure this makes
-  // sense but it'll get cleaned up in our destructor anyway, so it's fine to
-  // not null it out here (as long as we don't run twice on the main thread,
-  // which shouldn't be possible).
-  if (NS_IsMainThread()) {
-    if (mThread) {
-      mThread->Shutdown();
-    }
-    return NS_OK;
-  }
-
   nsresult loadLoadableRootsResult = LoadLoadableRoots();
   if (NS_WARN_IF(NS_FAILED(loadLoadableRootsResult))) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Error, ("LoadLoadableRoots failed"));
@@ -686,9 +669,7 @@ LoadLoadableRootsTask::Run() {
               ("failed to notify loadable roots loaded monitor"));
     }
   }
-
+  return NS_OK;
-  // Go back to the main thread to clean up this worker thread.
-  return NS_DispatchToMainThread(this);
 }
 
 NS_IMETHODIMP