bug 1498686 - avoid acquiring TransportSecurityInfo::mMutex in hot code r=jesup,jcj
authorDana Keeler <dkeeler@mozilla.com>
Thu, 18 Oct 2018 20:08:02 +0000
changeset 500455 7b439431eb95b33e5619ac6c4cd07de948afe7b6
parent 500454 9a05c09af3770321c9d3acd89204ac62f3fb1878
child 500456 9e0a42364ef5fd6fd57d06a5df8113d34cb4b7f0
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjesup, jcj
bugs1498686
milestone64.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 1498686 - avoid acquiring TransportSecurityInfo::mMutex in hot code r=jesup,jcj Before this patch, Necko functions polling the state of TLS sockets (essentially, TransportSecurityInfo) would cause a considerable amount of locking on TransportSecurityInfo::mMutex instances via GetErrorCode(). Most of this code only cared if an error had been set via SetCanceled(), so this patch adds an atomic boolean mCanceled (and associated accessor GetCanceled()) that can be used to the same effect but without acquiring the lock. Differential Revision: https://phabricator.services.mozilla.com/D8754
security/manager/ssl/TransportSecurityInfo.cpp
security/manager/ssl/TransportSecurityInfo.h
security/manager/ssl/nsNSSIOLayer.cpp
--- a/security/manager/ssl/TransportSecurityInfo.cpp
+++ b/security/manager/ssl/TransportSecurityInfo.cpp
@@ -47,16 +47,17 @@ TransportSecurityInfo::TransportSecurity
   , mSignatureSchemeName()
   , mIsDomainMismatch(false)
   , mIsNotValidAtThisTime(false)
   , mIsUntrusted(false)
   , mIsEV(false)
   , mHasIsEVStatus(false)
   , mHaveCipherSuiteAndProtocol(false)
   , mHaveCertErrorBits(false)
+  , mCanceled(false)
   , mMutex("TransportSecurityInfo::mMutex")
   , mSecurityState(nsIWebProgressListener::STATE_IS_INSECURE)
   , mErrorCode(0)
   , mPort(0)
 {
 }
 
 NS_IMPL_ISUPPORTS(TransportSecurityInfo,
@@ -79,31 +80,53 @@ TransportSecurityInfo::SetPort(int32_t a
 
 void
 TransportSecurityInfo::SetOriginAttributes(
   const OriginAttributes& aOriginAttributes)
 {
   mOriginAttributes = aOriginAttributes;
 }
 
+// NB: GetErrorCode may be called before an error code is set (if ever). In that
+// case, this returns (by pointer) 0, which is treated as a successful value.
 NS_IMETHODIMP
 TransportSecurityInfo::GetErrorCode(int32_t* state)
 {
   MutexAutoLock lock(mMutex);
 
+  // We're in an inconsistent state if we think we've been canceled but no error
+  // code was set or we haven't been canceled but an error code was set.
+  MOZ_ASSERT(!((mCanceled && mErrorCode == 0) ||
+              (!mCanceled && mErrorCode != 0)));
+  if ((mCanceled && mErrorCode == 0) || (!mCanceled && mErrorCode != 0)) {
+    mCanceled = true;
+    mErrorCode = SEC_ERROR_LIBRARY_FAILURE;
+  }
+
   *state = mErrorCode;
   return NS_OK;
 }
 
 void
 TransportSecurityInfo::SetCanceled(PRErrorCode errorCode)
 {
-  MutexAutoLock lock(mMutex);
+  MOZ_ASSERT(errorCode != 0);
+  if (errorCode == 0) {
+    errorCode = SEC_ERROR_LIBRARY_FAILURE;
+  }
 
+  MutexAutoLock lock(mMutex);
   mErrorCode = errorCode;
+  mCanceled = true;
+}
+
+bool
+TransportSecurityInfo::IsCanceled()
+{
+  return mCanceled;
 }
 
 NS_IMETHODIMP
 TransportSecurityInfo::GetSecurityState(uint32_t* state)
 {
   *state = mSecurityState;
   return NS_OK;
 }
@@ -386,16 +409,21 @@ TransportSecurityInfo::Read(nsIObjectInp
   }
   uint32_t errorCode;
   rv = aStream->Read32(&errorCode);
   if (NS_FAILED(rv)) {
     return rv;
   }
   // PRErrorCode will be a negative value
   mErrorCode = static_cast<PRErrorCode>(errorCode);
+  // If mErrorCode is non-zero, SetCanceled was called on the
+  // TransportSecurityInfo that was serialized.
+  if (mErrorCode != 0) {
+    mCanceled = true;
+  }
 
   // Re-purpose mErrorMessageCached to represent serialization version
   // If string doesn't match exact version it will be treated as older
   // serialization.
   nsAutoString serVersion;
   rv = aStream->ReadString(serVersion);
   if (NS_FAILED(rv)) {
     return rv;
--- a/security/manager/ssl/TransportSecurityInfo.h
+++ b/security/manager/ssl/TransportSecurityInfo.h
@@ -63,16 +63,17 @@ public:
   void SetPort(int32_t aPort);
 
   const OriginAttributes& GetOriginAttributes() const {
     return mOriginAttributes;
   }
   void SetOriginAttributes(const OriginAttributes& aOriginAttributes);
 
   void SetCanceled(PRErrorCode errorCode);
+  bool IsCanceled();
 
   void SetStatusErrorBits(nsNSSCertificate* cert, uint32_t collected_errors);
 
   nsresult SetFailedCertChain(UniqueCERTCertList certList);
 
   void SetServerCert(nsNSSCertificate* aServerCert, EVStatus aEVStatus);
 
   nsresult SetSucceededCertChain(mozilla::UniqueCERTCertList certList);
@@ -98,16 +99,21 @@ public:
   bool mHasIsEVStatus;
   bool mHaveCipherSuiteAndProtocol;
 
   /* mHaveCertErrrorBits is relied on to determine whether or not a SPDY
      connection is eligible for joining in nsNSSSocketInfo::JoinConnection() */
   bool mHaveCertErrorBits;
 
 private:
+  // True if SetCanceled has been called (or if this was deserialized with a
+  // non-zero mErrorCode, which can only be the case if SetCanceled was called
+  // on the original TransportSecurityInfo).
+  Atomic<bool> mCanceled;
+
   mutable ::mozilla::Mutex mMutex;
 
 protected:
   nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
 
 private:
   uint32_t mSecurityState;
 
--- a/security/manager/ssl/nsNSSIOLayer.cpp
+++ b/security/manager/ssl/nsNSSIOLayer.cpp
@@ -421,25 +421,25 @@ nsNSSSocketInfo::SetDenyClientCert(bool 
 }
 
 NS_IMETHODIMP
 nsNSSSocketInfo::DriveHandshake()
 {
   if (!mFd) {
     return NS_ERROR_FAILURE;
   }
-  PRErrorCode errorCode = GetErrorCode();
-  if (errorCode) {
+  if (IsCanceled()) {
+    PRErrorCode errorCode = GetErrorCode();
     return GetXPCOMFromNSSError(errorCode);
   }
 
   SECStatus rv = SSL_ForceHandshake(mFd);
 
   if (rv != SECSuccess) {
-    errorCode = PR_GetError();
+    PRErrorCode errorCode = PR_GetError();
     if (errorCode == PR_WOULD_BLOCK_ERROR) {
       return NS_BASE_STREAM_WOULD_BLOCK;
     }
 
     SetCanceled(errorCode);
     return GetXPCOMFromNSSError(errorCode);
   }
   return NS_OK;
@@ -714,39 +714,16 @@ nsNSSSocketInfo::SetSharedOwningReferenc
 
 void nsSSLIOLayerHelpers::Cleanup()
 {
   MutexAutoLock lock(mutex);
   mTLSIntoleranceInfo.Clear();
   mInsecureFallbackSites.Clear();
 }
 
-static void
-nsHandleSSLError(nsNSSSocketInfo* socketInfo,
-                 PRErrorCode err)
-{
-  if (!NS_IsMainThread()) {
-    NS_ERROR("nsHandleSSLError called off the main thread");
-    return;
-  }
-
-  // SetCanceled is only called by the main thread or the socket transport
-  // thread. Whenever this function is called on the main thread, the SSL
-  // thread is blocked on it. So, no mutex is necessary for
-  // SetCanceled()/GetError*().
-  if (socketInfo->GetErrorCode()) {
-    // If the socket has been flagged as canceled,
-    // the code who did was responsible for setting the error code.
-    return;
-  }
-
-  // We must cancel, which sets the error code.
-  socketInfo->SetCanceled(err);
-}
-
 namespace {
 
 enum Operation { reading, writing, not_reading_or_writing };
 
 int32_t checkHandshake(int32_t bytesTransfered, bool wasReading,
                        PRFileDesc* ssl_layer_fd,
                        nsNSSSocketInfo* socketInfo);
 
@@ -757,17 +734,17 @@ getSocketInfoIfRunning(PRFileDesc* fd, O
       fd->identity != nsSSLIOLayerHelpers::nsSSLIOLayerIdentity) {
     NS_ERROR("bad file descriptor passed to getSocketInfoIfRunning");
     PR_SetError(PR_BAD_DESCRIPTOR_ERROR, 0);
     return nullptr;
   }
 
   nsNSSSocketInfo* socketInfo = (nsNSSSocketInfo*) fd->secret;
 
-  if (socketInfo->GetErrorCode()) {
+  if (socketInfo->IsCanceled()) {
     PRErrorCode err = socketInfo->GetErrorCode();
     PR_SetError(err, 0);
     if (op == reading || op == writing) {
       // We must do TLS intolerance checks for reads and writes, for timeouts
       // in particular.
       (void) checkHandshake(-1, op == reading, fd, socketInfo);
     }
 
@@ -1122,35 +1099,16 @@ nsDumpBuffer(unsigned char* buf, int len
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("%s%s\n", hexbuf, chrbuf));
 }
 
 #define DEBUG_DUMP_BUFFER(buf,len) nsDumpBuffer(buf,len)
 #else
 #define DEBUG_DUMP_BUFFER(buf,len)
 #endif
 
-class SSLErrorRunnable : public SyncRunnableBase
-{
- public:
-  SSLErrorRunnable(nsNSSSocketInfo* infoObject,
-                   PRErrorCode errorCode)
-    : mInfoObject(infoObject)
-    , mErrorCode(errorCode)
-  {
-  }
-
-  virtual void RunOnTargetThread() override
-  {
-    nsHandleSSLError(mInfoObject, mErrorCode);
-  }
-
-  RefPtr<nsNSSSocketInfo> mInfoObject;
-  const PRErrorCode mErrorCode;
-};
-
 namespace {
 
 uint32_t tlsIntoleranceTelemetryBucket(PRErrorCode err)
 {
   // returns a numeric code for where we track various errors in telemetry
   // only errors that cause version fallback are tracked,
   // so this is also used to determine which errors can cause version fallback
   switch (err) {
@@ -1342,27 +1300,23 @@ checkHandshake(int32_t bytesTransfered, 
       }
 
       wantRetry = retryDueToTLSIntolerance(err, socketInfo);
     }
 
     // This is the common place where we trigger non-cert-errors on a SSL
     // socket. This might be reached at any time of the connection.
     //
-    // The socketInfo->GetErrorCode() check is here to ensure we don't try to
-    // do the synchronous dispatch to the main thread unnecessarily after we've
-    // already handled a certificate error. (SSLErrorRunnable calls
-    // nsHandleSSLError, which has logic to avoid replacing the error message,
-    // so without the !socketInfo->GetErrorCode(), it would just be an
-    // expensive no-op.)
+    // IsCanceled() is backed by an atomic boolean. It will only ever go from
+    // false to true, so we will never erroneously not call SetCanceled here. We
+    // could in theory overwrite a previously-set error code, but we'll always
+    // have some sort of error.
     if (!wantRetry && mozilla::psm::IsNSSErrorCode(err) &&
-        !socketInfo->GetErrorCode()) {
-      RefPtr<SyncRunnableBase> runnable(
-        new SSLErrorRunnable(socketInfo, err));
-      (void) runnable->DispatchToMainThreadAndWait();
+        !socketInfo->IsCanceled()) {
+      socketInfo->SetCanceled(err);
     }
   } else if (wasReading && 0 == bytesTransfered) {
     // zero bytes on reading, socket closed
     if (handleHandshakeResultNow) {
       wantRetry = retryDueToTLSIntolerance(PR_END_OF_FILE_ERROR, socketInfo);
     }
   }
 
@@ -1388,17 +1342,17 @@ checkHandshake(int32_t bytesTransfered, 
   }
 
   if (bytesTransfered < 0) {
     // Remember that we encountered an error so that getSocketInfoIfRunning
     // will correctly cause us to fail if another part of Gecko
     // (erroneously) calls an I/O function (PR_Send/PR_Recv/etc.) again on
     // this socket. Note that we use the original error because if we use
     // PR_CONNECT_RESET_ERROR, we'll repeated try to reconnect.
-    if (originalError != PR_WOULD_BLOCK_ERROR && !socketInfo->GetErrorCode()) {
+    if (originalError != PR_WOULD_BLOCK_ERROR && !socketInfo->IsCanceled()) {
       socketInfo->SetCanceled(originalError);
     }
     PR_SetError(err, 0);
   }
 
   return bytesTransfered;
 }