bug 1433409 - avoid acquiring nsNSSComponent.mMutex when we don't have to r=franziskus
authorDavid Keeler <dkeeler@mozilla.com>
Wed, 01 Aug 2018 20:56:28 +0000
changeset 487400 da4d091b0d2fa9bb8d728771b075e0813c62d201
parent 487399 6148cbf8e14c49951bc1da44cb705369e9f5ae24
child 487401 c66ba2794d94ccced815676c52c8dfab959dfb68
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfranziskus
bugs1433409
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1433409 - avoid acquiring nsNSSComponent.mMutex when we don't have to r=franziskus In some cases, nsNSSComponent functions were acquiring nsNSSComponent's mMutex to check mNSSInitialized to see if it had been initialized. It turns out this is unnecessary in some cases because those functions are only callable if nsNSSComponent has been initialized. This fixes those instances and renames 'mNSSInitialized' to 'mNonIdempotentCleanupMustHappen' to make it clear exactly what that boolean represents. Differential Revision: https://phabricator.services.mozilla.com/D2577
security/manager/ssl/nsNSSComponent.cpp
security/manager/ssl/nsNSSComponent.h
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -197,18 +197,18 @@ GetRevocationBehaviorFromPrefs(/*out*/ C
   SSL_ClearSessionCache();
 }
 
 nsNSSComponent::nsNSSComponent()
   : mLoadableRootsLoadedMonitor("nsNSSComponent.mLoadableRootsLoadedMonitor")
   , mLoadableRootsLoaded(false)
   , mLoadableRootsLoadedResult(NS_ERROR_FAILURE)
   , mMutex("nsNSSComponent.mMutex")
-  , mNSSInitialized(false)
   , mMitmDetecionEnabled(false)
+  , mNonIdempotentCleanupMustHappen(false)
 {
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsNSSComponent::ctor\n"));
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
   MOZ_ASSERT(mInstanceCount == 0,
              "nsNSSComponent is a singleton, but instantiated multiple times!");
   ++mInstanceCount;
 }
@@ -892,22 +892,16 @@ NS_IMETHODIMP
 nsNSSComponent::HasUserCertsInstalled(bool* result)
 {
   NS_ENSURE_ARG_POINTER(result);
   MOZ_ASSERT(NS_IsMainThread(), "Main thread only");
   if (!NS_IsMainThread()) {
     return NS_ERROR_NOT_SAME_THREAD;
   }
 
-  MutexAutoLock nsNSSComponentLock(mMutex);
-
-  if (!mNSSInitialized) {
-    return NS_ERROR_NOT_INITIALIZED;
-  }
-
   *result = false;
   UniqueCERTCertList certList(
     CERT_FindUserCertsByUsage(CERT_GetDefaultCertDB(), certUsageSSLClient,
                               false, true, nullptr));
   if (!certList) {
     return NS_OK;
   }
 
@@ -932,22 +926,16 @@ nsNSSComponent::BlockUntilLoadableRootsL
 
   return mLoadableRootsLoadedResult;
 }
 
 nsresult
 nsNSSComponent::CheckForSmartCardChanges()
 {
 #ifndef MOZ_NO_SMART_CARDS
-  MutexAutoLock nsNSSComponentLock(mMutex);
-
-  if (!mNSSInitialized) {
-    return NS_ERROR_NOT_INITIALIZED;
-  }
-
   // SECMOD_UpdateSlotList attempts to acquire the list lock as well,
   // so we have to do this in two steps. The lock protects the list itself, so
   // if we get our own owned references to the modules we're interested in,
   // there's no thread safety concern here.
   Vector<UniqueSECMODModule> modulesWithRemovableSlots;
   {
     AutoSECMODListReadLock secmodLock;
     SECMODModuleList* list = SECMOD_GetDefaultModuleList();
@@ -1974,17 +1962,17 @@ nsNSSComponent::InitializeNSS()
 
     RefPtr<LoadLoadableRootsTask> loadLoadableRootsTask(
       new LoadLoadableRootsTask(this));
     rv = loadLoadableRootsTask->Dispatch();
     if (NS_FAILED(rv)) {
       return rv;
     }
 
-    mNSSInitialized = true;
+    mNonIdempotentCleanupMustHappen = true;
     return NS_OK;
   }
 }
 
 void
 nsNSSComponent::ShutdownNSS()
 {
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsNSSComponent::ShutdownNSS\n"));
@@ -1993,25 +1981,26 @@ nsNSSComponent::ShutdownNSS()
   MutexAutoLock lock(mMutex);
 
   // We have to block until the load loadable roots task has completed, because
   // otherwise we might try to unload the loadable roots while the loadable
   // roots loading thread is setting up EV information, which can cause
   // it to fail to find the roots it is expecting. However, if initialization
   // failed, we won't have dispatched the load loadable roots background task.
   // In that case, we don't want to block on an event that will never happen.
-  if (mNSSInitialized) {
+  if (mNonIdempotentCleanupMustHappen) {
     Unused << BlockUntilLoadableRootsLoaded();
 
     // We can only run SSL_ShutdownServerSessionIDCache once (the rest of
     // these operations are idempotent).
     SSL_ClearSessionCache();
     // TLSServerSocket may be run with the session cache enabled. This ensures
     // those resources are cleaned up.
     Unused << SSL_ShutdownServerSessionIDCache();
+    mNonIdempotentCleanupMustHappen = false;
   }
 
   ::mozilla::psm::UnloadLoadableRoots();
 
 #ifdef XP_WIN
   mFamilySafetyRoot = nullptr;
   mEnterpriseRoots = nullptr;
 #endif
@@ -2021,18 +2010,16 @@ nsNSSComponent::ShutdownNSS()
   Preferences::RemoveObserver(this, "security.");
 
   // Release the default CertVerifier. This will cause any held NSS resources
   // to be released.
   mDefaultCertVerifier = nullptr;
   // We don't actually shut down NSS - XPCOM does, after all threads have been
   // joined and the component manager has been shut down (and so there shouldn't
   // be any XPCOM objects holding NSS resources).
-
-  mNSSInitialized = false;
 }
 
 nsresult
 nsNSSComponent::Init()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   if (!NS_IsMainThread()) {
     return NS_ERROR_NOT_SAME_THREAD;
@@ -2238,17 +2225,16 @@ nsNSSComponent::IsCertTestBuiltInRoot(CE
   }
   nsAutoString certHash;
   nsresult rv = nsc->GetSha256Fingerprint(certHash);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   MutexAutoLock lock(mMutex);
-  MOZ_ASSERT(mNSSInitialized);
   if (mTestBuiltInRootHash.IsEmpty()) {
     return NS_OK;
   }
 
   *result = mTestBuiltInRootHash.Equals(certHash);
 #endif // DEBUG
 
   return NS_OK;
@@ -2268,17 +2254,16 @@ nsNSSComponent::IsCertContentSigningRoot
   nsAutoString certHash;
   nsresult rv = nsc->GetSha256Fingerprint(certHash);
   if (NS_FAILED(rv)) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("getting cert fingerprint failed"));
     return rv;
   }
 
   MutexAutoLock lock(mMutex);
-  MOZ_ASSERT(mNSSInitialized);
 
   if (mContentSigningRootHash.IsEmpty()) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("mContentSigningRootHash is empty"));
     return NS_ERROR_FAILURE;
   }
 
   *result = mContentSigningRootHash.Equals(certHash);
   return NS_OK;
@@ -2299,17 +2284,16 @@ nsNSSComponent::IssuerMatchesMitmCanary(
 }
 
 SharedCertVerifier::~SharedCertVerifier() { }
 
 NS_IMETHODIMP
 nsNSSComponent::GetDefaultCertVerifier(SharedCertVerifier** result)
 {
   MutexAutoLock lock(mMutex);
-  MOZ_ASSERT(mNSSInitialized);
   NS_ENSURE_ARG_POINTER(result);
   RefPtr<SharedCertVerifier> certVerifier(mDefaultCertVerifier);
   certVerifier.forget(result);
   return NS_OK;
 }
 
 namespace mozilla { namespace psm {
 
--- a/security/manager/ssl/nsNSSComponent.h
+++ b/security/manager/ssl/nsNSSComponent.h
@@ -100,29 +100,44 @@ private:
   mozilla::Monitor mLoadableRootsLoadedMonitor;
   bool mLoadableRootsLoaded;
   nsresult mLoadableRootsLoadedResult;
 
   // mMutex protects all members that are accessed from more than one thread.
   mozilla::Mutex mMutex;
 
   // The following members are accessed from more than one thread:
-  bool mNSSInitialized;
+
 #ifdef DEBUG
   nsString mTestBuiltInRootHash;
 #endif
   nsString mContentSigningRootHash;
   RefPtr<mozilla::psm::SharedCertVerifier> mDefaultCertVerifier;
   nsString mMitmCanaryIssuer;
   bool mMitmDetecionEnabled;
   mozilla::UniqueCERTCertList mEnterpriseRoots;
   mozilla::UniqueCERTCertificate mFamilySafetyRoot;
 
   // The following members are accessed only on the main thread:
   static int mInstanceCount;
+  // If initialization (i.e. InitializeNSS) succeeds, we have called
+  // SSL_ConfigServerSessionIDCache. To clean this up, we must call
+  // SSL_ClearSessionCache and SSL_ShutdownServerSessionIDCache exactly once
+  // each (and if we haven't called SSL_ConfigServerSessionIDCache - for example
+  // if initialization failed - then we mustn't call the cleanup functions
+  // ever). There are multiple events that can cause us to enter our cleanup
+  // function (ShutdownNSS) and so we keep track of if we need to call these
+  // non-idempotent functions in the following boolean.
+  // Similarly, if InitializeNSS succeeds, then we have dispatched an event to
+  // load the loadable roots module on a background thread. We must wait for it
+  // to complete before attempting to unload the module again in ShutdownNSS. If
+  // we never dispatched the event, then we can't wait for it to complete
+  // (because it will never complete) so we again use this boolean to keep track
+  // of if we should wait.
+  bool mNonIdempotentCleanupMustHappen;
 };
 
 inline nsresult
 BlockUntilLoadableRootsLoaded()
 {
   nsCOMPtr<nsINSSComponent> component(do_GetService(PSM_COMPONENT_CONTRACTID));
   if (!component) {
     return NS_ERROR_FAILURE;