Bug 712363, Part 4: Split the creation of CertErrorRunnable from the dispatching of it, r=honzab,a=Standard8 for checkin to comm specific relbranch COMM110_2012030905_RELBRANCH
authorBrian Smith <bsmith@mozilla.com>
Tue, 31 Jan 2012 08:05:06 -0800
branchCOMM110_2012030905_RELBRANCH
changeset 85563 528955b0770d5e9ed5b07400eb152b89b67baea8
parent 85562 eb52b2ada33ca7e0f8761bdc9ad92505f149536a
child 85564 7eb04a2551588593a7d59cad0ac972628e8a572b
push id1
push usersledru@mozilla.com
push dateThu, 04 Dec 2014 17:57:20 +0000
reviewershonzab, Standard8
bugs712363
milestone11.0
Bug 712363, Part 4: Split the creation of CertErrorRunnable from the dispatching of it, r=honzab,a=Standard8 for checkin to comm specific relbranch
security/manager/ssl/src/SSLServerCertVerification.cpp
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -134,16 +134,17 @@
 #include "nsIBadCertListener2.h"
 #include "nsICertOverrideService.h"
 #include "nsIStrictTransportSecurityService.h"
 #include "nsNSSComponent.h"
 #include "nsNSSCleaner.h"
 #include "nsRecentBadCerts.h"
 #include "nsNSSIOLayer.h"
 
+#include "mozilla/Assertions.h"
 #include "nsIThreadPool.h"
 #include "nsXPCOMCIDInternal.h"
 #include "nsComponentManagerUtils.h"
 #include "nsServiceManagerUtils.h"
 #include "PSMRunnable.h"
 
 #include "ssl.h"
 #include "secerr.h"
@@ -252,17 +253,16 @@ class CertErrorRunnable : public SyncRun
       mDefaultErrorCodeToReport(defaultErrorCodeToReport),
       mCollectedErrors(collectedErrors),
       mErrorCodeTrust(errorCodeTrust),
       mErrorCodeMismatch(errorCodeMismatch),
       mErrorCodeExpired(errorCodeExpired)
   {
   }
 
-  NS_DECL_NSIRUNNABLE
   virtual void RunOnTargetThread();
   nsCOMPtr<nsIRunnable> mResult; // out
 private:
   SSLServerCertVerificationResult* CheckCertOverrides();
   
   const void * const mFdForLogging; // may become an invalid pointer; do not dereference
   const nsCOMPtr<nsIX509Cert> mCert;
   const nsRefPtr<nsNSSSocketInfo> mInfoObject;
@@ -271,17 +271,17 @@ private:
   const PRErrorCode mErrorCodeTrust;
   const PRErrorCode mErrorCodeMismatch;
   const PRErrorCode mErrorCodeExpired;
 };
 
 SSLServerCertVerificationResult *
 CertErrorRunnable::CheckCertOverrides()
 {
-  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p][%p] top of CertErrorRunnable::Run\n",
+  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p][%p] top of CheckCertOverrides\n",
                                     mFdForLogging, this));
 
   if (!NS_IsMainThread()) {
     NS_ERROR("CertErrorRunnable::CheckCertOverrides called off main thread");
     return new SSLServerCertVerificationResult(*mInfoObject,
                                                mDefaultErrorCodeToReport);
   }
 
@@ -383,136 +383,103 @@ CertErrorRunnable::CheckCertOverrides()
                                 : mErrorCodeMismatch ? mErrorCodeMismatch
                                 : mErrorCodeExpired  ? mErrorCodeExpired
                                 : mDefaultErrorCodeToReport;
 
   return new SSLServerCertVerificationResult(*mInfoObject, errorCodeToReport,
                                              OverridableCertErrorMessage);
 }
 
-NS_IMETHODIMP
-CertErrorRunnable::Run()
-{
-  // This code is confusing: First, Run() is called on the socket transport
-  // thread. Then we re-dispatch it to the main thread synchronously (step 1).
-  // On the main thread, we call CheckCertOverrides (step 2). Then we return
-  // from the main thread and are back on the socket transport thread. There,
-  // we run the result runnable directly (step 3).
-  if (!NS_IsMainThread()) {
-    // We are running on the socket transport thread. We need to re-dispatch
-    // ourselves synchronously to the main thread.
-    DispatchToMainThreadAndWait(); // step 1
-
-    // step 3
-    if (!mResult) {
-      // Either the dispatch failed or CheckCertOverrides wrongly returned null
-      NS_ERROR("Did not create a SSLServerCertVerificationResult");
-      mResult = new SSLServerCertVerificationResult(*mInfoObject,
-                                                    PR_INVALID_STATE_ERROR);
-    }
-    return mResult->Run(); 
-  } else {
-    // block this thread (the socket transport thread) until RunOnTargetThread
-    // is complete.
-    return SyncRunnableBase::Run(); // step 2
-  }
-}
-
 void 
 CertErrorRunnable::RunOnTargetThread()
 {
-  // Now we are running on the main thread, blocking the socket tranposrt
-  // thread. This is exactly the state we need to be in to call
-  // CheckCertOverrides; CheckCertOverrides must block events on both of
-  // these threads because it calls nsNSSSocketInfo::GetInterface(),
-  // which calls nsHttpConnection::GetInterface() through
-  // nsNSSSocketInfo::mCallbacks. nsHttpConnection::GetInterface must always
-  // execute on the main thread, with the socket transport thread blocked.
-  mResult = CheckCertOverrides(); 
+  MOZ_ASSERT(NS_IsMainThread());
+
+  mResult = CheckCertOverrides();
+  
+  MOZ_ASSERT(mResult);
 }
 
-// Returns SECSuccess if it dispatched the CertErrorRunnable. In that case,
-// the caller should NOT dispatch its own SSLServerCertVerificationResult;
-// the CertErrorRunnable will do it instead.
-//
-// Returns SECFailure with the error code set if it does not dispatch the
-// CertErrorRunnable. In that case, the caller should dispatch its own 
-// SSLServerCertVerificationResult with the error code from PR_GetError().
-SECStatus
-HandleBadCertificate(PRErrorCode defaultErrorCodeToReport,
-                    nsNSSSocketInfo * socketInfo, CERTCertificate & cert,
-                    const void * fdForLogging,
-                    const nsNSSShutDownPreventionLock & /*proofOfLock*/)
+// Returns null with the error code (PR_GetError()) set if it does not create
+// the CertErrorRunnable.
+CertErrorRunnable *
+CreateCertErrorRunnable(PRErrorCode defaultErrorCodeToReport,
+                        nsNSSSocketInfo * socketInfo,
+                        CERTCertificate * cert,
+                        const void * fdForLogging)
 {
+  MOZ_ASSERT(socketInfo);
+  MOZ_ASSERT(cert);
+  
   // cert was revoked, don't do anything else
   if (defaultErrorCodeToReport == SEC_ERROR_REVOKED_CERTIFICATE) {
     PR_SetError(SEC_ERROR_REVOKED_CERTIFICATE, 0);
-    return SECFailure;
+    return nsnull;
   }
 
   if (defaultErrorCodeToReport == 0) {
     NS_ERROR("No error code set during certificate validation failure.");
     PR_SetError(PR_INVALID_STATE_ERROR, 0);
-    return SECFailure;
+    return nsnull;
   }
 
   nsRefPtr<nsNSSCertificate> nssCert;
-  nssCert = nsNSSCertificate::Create(&cert);
+  nssCert = nsNSSCertificate::Create(cert);
   if (!nssCert) {
-    NS_ERROR("nsNSSCertificate::Create failed in DispatchCertErrorRunnable");
+    NS_ERROR("nsNSSCertificate::Create failed");
     PR_SetError(SEC_ERROR_NO_MEMORY, 0);
-    return SECFailure;
+    return nsnull;
   }
 
   SECStatus srv;
   nsresult nsrv;
 
   nsCOMPtr<nsINSSComponent> inss = do_GetService(kNSSComponentCID, &nsrv);
   if (!inss) {
-    NS_ERROR("do_GetService(kNSSComponentCID) failed in DispatchCertErrorRunnable");
+    NS_ERROR("do_GetService(kNSSComponentCID) failed");
     PR_SetError(defaultErrorCodeToReport, 0);
-    return SECFailure;
+    return nsnull;
   }
 
   nsRefPtr<nsCERTValInParamWrapper> survivingParams;
   nsrv = inss->GetDefaultCERTValInParam(survivingParams);
   if (NS_FAILED(nsrv)) {
-    NS_ERROR("GetDefaultCERTValInParam failed in DispatchCertErrorRunnable");
+    NS_ERROR("GetDefaultCERTValInParam failed");
     PR_SetError(defaultErrorCodeToReport, 0);
-    return SECFailure;
+    return nsnull;
   }
   
   PRArenaPool *log_arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
   PRArenaPoolCleanerFalseParam log_arena_cleaner(log_arena);
   if (!log_arena) {
-    NS_ERROR("PORT_NewArena failed in DispatchCertErrorRunnable");
-    return SECFailure; // PORT_NewArena set error code
+    NS_ERROR("PORT_NewArena failed");
+    return nsnull; // PORT_NewArena set error code
   }
 
   CERTVerifyLog *verify_log = PORT_ArenaZNew(log_arena, CERTVerifyLog);
   if (!verify_log) {
-    NS_ERROR("PORT_ArenaZNew failed in DispatchCertErrorRunnable");
-    return SECFailure; // PORT_ArenaZNew set error code
+    NS_ERROR("PORT_ArenaZNew failed");
+    return nsnull; // PORT_ArenaZNew set error code
   }
   CERTVerifyLogContentsCleaner verify_log_cleaner(verify_log);
   verify_log->arena = log_arena;
 
   if (!nsNSSComponent::globalConstFlagUsePKIXVerification) {
-    srv = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), &cert,
+    srv = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), cert,
                                 true, certificateUsageSSLServer,
                                 PR_Now(), static_cast<void*>(socketInfo),
                                 verify_log, NULL);
   }
   else {
     CERTValOutParam cvout[2];
     cvout[0].type = cert_po_errorLog;
     cvout[0].value.pointer.log = verify_log;
     cvout[1].type = cert_po_end;
 
-    srv = CERT_PKIXVerifyCert(&cert, certificateUsageSSLServer,
+    srv = CERT_PKIXVerifyCert(cert, certificateUsageSSLServer,
                               survivingParams->GetRawPointerForNSS(),
                               cvout, static_cast<void*>(socketInfo));
   }
 
   // We ignore the result code of the cert verification.
   // Either it is a failure, which is expected, and we'll process the
   //                         verify log below.
   // Or it is a success, then a domain mismatch is the only 
@@ -525,17 +492,17 @@ HandleBadCertificate(PRErrorCode default
   PRUint32 collected_errors = 0;
 
   if (socketInfo->IsCertIssuerBlacklisted()) {
     collected_errors |= nsICertOverrideService::ERROR_UNTRUSTED;
     errorCodeTrust = defaultErrorCodeToReport;
   }
 
   // Check the name field against the desired hostname.
-  if (CERT_VerifyCertName(&cert, socketInfo->GetHostName()) != SECSuccess) {
+  if (CERT_VerifyCertName(cert, socketInfo->GetHostName()) != SECSuccess) {
     collected_errors |= nsICertOverrideService::ERROR_MISMATCH;
     errorCodeMismatch = SSL_ERROR_BAD_CERT_DOMAIN;
   }
 
   CERTVerifyLogNode *i_node;
   for (i_node = verify_log->head; i_node; i_node = i_node->next)
   {
     switch (i_node->error)
@@ -561,57 +528,69 @@ HandleBadCertificate(PRErrorCode default
       case SEC_ERROR_EXPIRED_CERTIFICATE:
         collected_errors |= nsICertOverrideService::ERROR_TIME;
         if (errorCodeExpired == SECSuccess) {
           errorCodeExpired = i_node->error;
         }
         break;
       default:
         PR_SetError(i_node->error, 0);
-        return SECFailure;
+        return nsnull;
     }
   }
 
   if (!collected_errors)
   {
     // This will happen when CERT_*Verify* only returned error(s) that are
     // not on our whitelist of overridable certificate errors.
     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p] !collected_errors: %d\n",
            fdForLogging, static_cast<int>(defaultErrorCodeToReport)));
     PR_SetError(defaultErrorCodeToReport, 0);
-    return SECFailure;
+    return nsnull;
   }
 
   socketInfo->SetStatusErrorBits(*nssCert, collected_errors);
 
-  nsRefPtr<CertErrorRunnable> runnable =
-    new CertErrorRunnable(fdForLogging, 
-                          static_cast<nsIX509Cert*>(nssCert.get()),
-                          socketInfo, defaultErrorCodeToReport, 
-                          collected_errors, errorCodeTrust, 
-                          errorCodeMismatch, errorCodeExpired);
-
-  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
-          ("[%p][%p] Before dispatching CertErrorRunnable\n",
-          fdForLogging, runnable.get()));
+  return new CertErrorRunnable(fdForLogging, 
+                               static_cast<nsIX509Cert*>(nssCert.get()),
+                               socketInfo, defaultErrorCodeToReport, 
+                               collected_errors, errorCodeTrust, 
+                               errorCodeMismatch, errorCodeExpired);
+}
 
-  nsresult nrv;
-  nsCOMPtr<nsIEventTarget> stsTarget
-    = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &nrv);
-  if (NS_SUCCEEDED(nrv)) {
-    nrv = stsTarget->Dispatch(runnable, NS_DISPATCH_NORMAL);
+// When doing async cert processing, we dispatch one of these runnables to the
+// socket transport service thread, which blocks the socket transport
+// service thread while it waits for the inner CertErrorRunnable to execute
+// CheckCertOverrides on the main thread. CheckCertOverrides must block events
+// on both of these threads because it calls nsNSSSocketInfo::GetInterface(), 
+// which may call nsHttpConnection::GetInterface() through
+// nsNSSSocketInfo::mCallbacks. nsHttpConnection::GetInterface must always
+// execute on the main thread, with the socket transport service thread
+// blocked.
+class CertErrorRunnableRunnable : public nsRunnable
+{
+public:
+  CertErrorRunnableRunnable(CertErrorRunnable * certErrorRunnable)
+    : mCertErrorRunnable(certErrorRunnable)
+  {
   }
-  if (NS_FAILED(nrv)) {
-    NS_ERROR("Failed to dispatch CertErrorRunnable");
-    PR_SetError(defaultErrorCodeToReport, 0);
-    return SECFailure;
+private:
+  NS_IMETHOD Run()
+  {
+    nsresult rv = mCertErrorRunnable->DispatchToMainThreadAndWait();
+    // The result must run on the socket transport thread, which we are already
+    // on, so we can just run it directly, instead of dispatching it.
+    if (NS_SUCCEEDED(rv)) {
+      rv = mCertErrorRunnable->mResult ? mCertErrorRunnable->mResult->Run()
+                                       : NS_ERROR_UNEXPECTED;
+    }
+    return rv;
   }
-
-  return SECSuccess;
-}
+  nsRefPtr<CertErrorRunnable> mCertErrorRunnable;
+};
 
 class SSLServerCertVerificationJob : public nsRunnable
 {
 public:
   // Must be called only on the socket transport thread
   static SECStatus Dispatch(const void * fdForLogging,
                             nsNSSSocketInfo * infoObject,
                             CERTCertificate * serverCert);
@@ -1065,27 +1044,45 @@ SSLServerCertVerificationJob::Run()
       nsRefPtr<SSLServerCertVerificationResult> restart 
         = new SSLServerCertVerificationResult(*mSocketInfo, 0);
       restart->Dispatch();
       return NS_OK;
     }
 
     error = PR_GetError();
     if (error != 0) {
-      rv = HandleBadCertificate(error, mSocketInfo, *mCert, mFdForLogging,
-                                nssShutdownPrevention);
-      if (rv == SECSuccess) {
-        // The CertErrorRunnable will run on the main thread and it will dispatch
-        // the cert verification result to the socket transport thread, so we
-        // don't have to. This way, this verification thread doesn't need to
-        // wait for the CertErrorRunnable to complete.
-        return NS_OK; 
+      nsRefPtr<CertErrorRunnable> runnable = CreateCertErrorRunnable(
+              error, mSocketInfo, mCert, mFdForLogging);
+      if (!runnable) {
+        // CreateCertErrorRunnable set a new error code
+        error = PR_GetError(); 
+      } else {
+        // We must block the the socket transport service thread while the
+        // main thread executes the CertErrorRunnable. The CertErrorRunnable
+        // will dispatch the result asynchronously, so we don't have to block
+        // this thread waiting for it.
+
+        PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
+                ("[%p][%p] Before dispatching CertErrorRunnable\n",
+                mFdForLogging, runnable.get()));
+
+        nsresult nrv;
+        nsCOMPtr<nsIEventTarget> stsTarget
+          = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &nrv);
+        if (NS_SUCCEEDED(nrv)) {
+          nrv = stsTarget->Dispatch(new CertErrorRunnableRunnable(runnable),
+                                    NS_DISPATCH_NORMAL);
+        }
+        if (NS_SUCCEEDED(nrv)) {
+          return NS_OK;
+        }
+
+        NS_ERROR("Failed to dispatch CertErrorRunnable");
+        error = PR_INVALID_STATE_ERROR;
       }
-      // DispatchCertErrorRunnable set a new error code.
-      error = PR_GetError(); 
     }
   }
 
   if (error == 0) {
     NS_NOTREACHED("no error set during certificate validation failure");
     error = PR_INVALID_STATE_ERROR;
   }