Bug 436379 - "Make docshell and securebrowserui only accessible on the main thread". r=kaie.

netwerk/protocol/http/src/nsHttpConnection.cpp

@@ -807,7 +807,7 @@ nsHttpConnection::GetInterface(const nsIID &iid, void **result)
     //       the socket transport thread.  If that weren't the case, then we'd
     //       have to worry about the possibility of mTransaction going away
     //       part-way through this function call.  See CloseTransaction.
-    NS_ASSERTION(PR_GetCurrentThread() != gSocketThread, "wrong thread");
+    NS_ASSERTION(NS_IsMainThread(), "wrong thread");
  
     if (mTransaction) {
         nsCOMPtr<nsIInterfaceRequestor> callbacks;

security/manager/boot/src/nsSecureBrowserUIImpl.cpp

@@ -190,13 +190,12 @@ nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl()
     PR_DestroyMonitor(mMonitor);
 }
 
-NS_IMPL_THREADSAFE_ISUPPORTS6(nsSecureBrowserUIImpl,
+NS_IMPL_ISUPPORTS6(nsSecureBrowserUIImpl, nsISecureBrowserUI,
-                              nsISecureBrowserUI,
+                                          nsIWebProgressListener,
-                              nsIWebProgressListener,
+                                          nsIFormSubmitObserver,
-                              nsIFormSubmitObserver,
+                                          nsIObserver,
-                              nsIObserver,
+                                          nsISupportsWeakReference,
-                              nsISupportsWeakReference,
+                                          nsISSLStatusProvider)
-                              nsISSLStatusProvider)
 
 NS_IMETHODIMP
 nsSecureBrowserUIImpl::Init(nsIDOMWindow *aWindow)

security/manager/ssl/src/nsNSSIOLayer.cpp

@@ -222,12 +222,15 @@ nsNSSSocketInfo::nsNSSSocketInfo()
     mPort(0)
 {
   mThreadData = new nsSSLSocketThreadData;
+  mCallbacksLock = nsAutoLock::NewLock("nsNSSSocketInfo::mCallbacksLock");
 }
 
 nsNSSSocketInfo::~nsNSSSocketInfo()
 {
   delete mThreadData;
 
+  nsAutoLock::DestroyLock(mCallbacksLock);
+
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown())
     return;
@@ -328,119 +331,192 @@ PRBool nsNSSSocketInfo::GetHasCleartextPhase()
 NS_IMETHODIMP
 nsNSSSocketInfo::GetNotificationCallbacks(nsIInterfaceRequestor** aCallbacks)
 {
-  *aCallbacks = mCallbacks;
+  nsCOMPtr<nsISupports> supports;
-  NS_IF_ADDREF(*aCallbacks);
+  {
+    nsAutoLock lock(mCallbacksLock);
+    supports = mCallbacks;
+  }
+  nsCOMPtr<nsIInterfaceRequestor> callbacks(do_QueryInterface(supports));
+  callbacks.forget(aCallbacks);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSSocketInfo::SetNotificationCallbacks(nsIInterfaceRequestor* aCallbacks)
 {
-  if (!aCallbacks) {
+  nsCOMPtr<nsISupports> callbacks(do_QueryInterface(aCallbacks));
-    mCallbacks = nsnull;
-    return NS_OK;
-  }
 
-  mCallbacks = aCallbacks;
+  nsAutoLock lock(mCallbacksLock);
-  mDocShellDependentStuffKnown = PR_FALSE;
+
+  callbacks.swap(mCallbacks);
+  if (mCallbacks) {
+    mDocShellDependentStuffKnown = PR_FALSE;
+  }
 
   return NS_OK;
 }
 
-nsresult
+class nsGatherDocshellInfoForPSMRunnable : public nsRunnable
-nsNSSSocketInfo::EnsureDocShellDependentStuffKnown()
 {
-  if (mDocShellDependentStuffKnown)
+public:
-    return NS_OK;
+  nsGatherDocshellInfoForPSMRunnable(nsIInterfaceRequestor* aCallbacks,
-
+                                     PRBool* aExternalReporting,
-  if (!mCallbacks || nsSSLThread::exitRequested())
+                                     nsIX509Cert** aPreviousCert)
-    return NS_ERROR_FAILURE;
+  : mCallbacks(aCallbacks), mExternalReporting(aExternalReporting),
-
+    mPreviousCert(aPreviousCert)
-  mDocShellDependentStuffKnown = PR_TRUE;
-
-  nsCOMPtr<nsIInterfaceRequestor> proxiedCallbacks;
-  NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
-                       NS_GET_IID(nsIInterfaceRequestor),
-                       static_cast<nsIInterfaceRequestor*>(mCallbacks),
-                       NS_PROXY_SYNC,
-                       getter_AddRefs(proxiedCallbacks));
-
-  // Are we running within a context that wants external SSL error reporting?
-  // We'll look at the presence of a security UI object inside docshell.
-  // If the docshell wants the lock icon, you'll get the ssl error pages, too.
-  // This is helpful to distinguish from all other contexts, like mail windows,
-  // or any other SSL connections running in the background.
-  // We must query it now and remember, because fatal SSL errors will come 
-  // with a socket close, and the socket transport might detach the callbacks 
-  // instance prior to our error reporting.
-
-  nsCOMPtr<nsIDocShell> docshell;
-
-  nsCOMPtr<nsIDocShellTreeItem> item(do_GetInterface(proxiedCallbacks));
-  if (item)
   {
-    nsCOMPtr<nsIDocShellTreeItem> proxiedItem;
+    NS_ASSERTION(aCallbacks, "Null pointer!");
-    nsCOMPtr<nsIDocShellTreeItem> rootItem;
-    NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
-                         NS_GET_IID(nsIDocShellTreeItem),
-                         item.get(),
-                         NS_PROXY_SYNC,
-                         getter_AddRefs(proxiedItem));
-
-    proxiedItem->GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
-    docshell = do_QueryInterface(rootItem);
-    NS_ASSERTION(docshell, "rootItem do_QI is null");
   }
 
-  if (docshell)
+  NS_IMETHOD Run()
   {
-    nsCOMPtr<nsIDocShell> proxiedDocShell;
+    NS_ASSERTION(NS_IsMainThread(), "Must run only on the main thread!");
-    NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
-                         NS_GET_IID(nsIDocShell),
-                         docshell.get(),
-                         NS_PROXY_SYNC,
-                         getter_AddRefs(proxiedDocShell));
-    nsISecureBrowserUI* secureUI;
-    proxiedDocShell->GetSecurityUI(&secureUI);
-    if (secureUI)
-    {
-      nsCOMPtr<nsIThread> mainThread(do_GetMainThread());
-      NS_ProxyRelease(mainThread, secureUI, PR_FALSE);
-      mExternalErrorReporting = PR_TRUE;
 
-      // If this socket is associated to a docshell, let's try to remember
+    *mExternalReporting = PR_FALSE;
-      // the currently used cert. If this socket gets a notification from NSS
+    *mPreviousCert = nsnull;
-      // having the same raw socket, we can keep the PSM wrapper object
+
-      // and all the data it has cached (like verification results).
+    // Are we running within a context that wants external SSL error reporting?
-      nsCOMPtr<nsISSLStatusProvider> statprov = do_QueryInterface(secureUI);
+    // We'll look at the presence of a security UI object inside docshell.
-      if (statprov) {
+    // If the docshell wants the lock icon, you'll get the ssl error pages, too.
-        nsCOMPtr<nsISupports> isup_stat;
+    // This is helpful to distinguish from all other contexts, like mail
-        statprov->GetSSLStatus(getter_AddRefs(isup_stat));
+    // windows, or any other SSL connections running in the background.
-        if (isup_stat) {
+    // We must query it now and remember, because fatal SSL errors will come 
-          nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(isup_stat);
+    // with a socket close, and the socket transport might detach the callbacks 
-          if (sslstat) {
+    // instance prior to our error reporting.
-            sslstat->GetServerCert(getter_AddRefs(mPreviousCert));
+
-          }
+    nsCOMPtr<nsIDocShellTreeItem> item(do_GetInterface(mCallbacks));
+    if (!item)
+      return NS_OK;
+
+    nsCOMPtr<nsIDocShellTreeItem> rootItem;
+    item->GetSameTypeRootTreeItem(getter_AddRefs(rootItem));
+
+    nsCOMPtr<nsIDocShell> docshell(do_QueryInterface(rootItem));
+    NS_ASSERTION(docshell, "rootItem do_QI is null");
+    if (!docshell)
+      return NS_OK;
+
+    nsCOMPtr<nsISecureBrowserUI> secureUI;
+    docshell->GetSecurityUI(getter_AddRefs(secureUI));
+    if (!secureUI)
+      return NS_OK;
+
+    *mExternalReporting = PR_TRUE;
+
+    // If this socket is associated to a docshell, let's try to remember
+    // the currently used cert. If this socket gets a notification from NSS
+    // having the same raw socket, we can keep the PSM wrapper object
+    // and all the data it has cached (like verification results).
+    nsCOMPtr<nsISSLStatusProvider> statprov(do_QueryInterface(secureUI));
+    if (!statprov)
+      return NS_OK;
+
+    nsCOMPtr<nsISupports> isup_stat;
+    statprov->GetSSLStatus(getter_AddRefs(isup_stat));
+
+    nsCOMPtr<nsISSLStatus> sslstat(do_QueryInterface(isup_stat));
+    if (!sslstat)
+      return NS_OK;
+
+    sslstat->GetServerCert(mPreviousCert);
+    return NS_OK;
+  }
+
+private:
+  nsIInterfaceRequestor* mCallbacks;
+  PRBool* mExternalReporting;
+  nsIX509Cert** mPreviousCert;
+};
+
+nsresult
+nsNSSSocketInfo::EnsureDocShellDependentStuffKnown(PRBool* aExternalReporting,
+                                                   nsIX509Cert** aPreviousCert)
+{
+  do {
+    nsCOMPtr<nsISupports> origCallbacks;
+    {
+      nsAutoLock lock(mCallbacksLock);
+
+      if (mDocShellDependentStuffKnown) {
+        if (aExternalReporting) {
+          *aExternalReporting = mExternalErrorReporting;
         }
+        if (aPreviousCert) {
+          NS_IF_ADDREF(*aPreviousCert = mPreviousCert);
+        }
+        return NS_OK;
+      }
+
+      origCallbacks = mCallbacks;
+    }
+
+    if (nsSSLThread::exitRequested() || !origCallbacks) {
+      return NS_ERROR_FAILURE;
+    }
+
+    nsCOMPtr<nsIInterfaceRequestor> callbacks(do_QueryInterface(origCallbacks));
+    NS_ASSERTION(callbacks, "How does this not QI to nsIInterfaceRequestor?!");
+
+    // We're about to touch the docshell which we know to be main-thread-only.
+    nsCOMPtr<nsIThread> mainThread(do_GetMainThread());
+    NS_ENSURE_TRUE(mainThread, NS_ERROR_FAILURE);
+
+    PRBool externalReporting = PR_FALSE;
+    nsCOMPtr<nsIX509Cert> previousCert;
+
+    nsCOMPtr<nsIRunnable> runnable =
+      new nsGatherDocshellInfoForPSMRunnable(callbacks, &externalReporting,
+                                             getter_AddRefs(previousCert));
+    NS_ENSURE_TRUE(runnable, NS_ERROR_OUT_OF_MEMORY);
+
+    nsresult rv = mainThread->Dispatch(runnable, NS_DISPATCH_SYNC);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    {
+      nsAutoLock lock(mCallbacksLock);
+
+      // Check to see if anyone replaced mCallbacks while the runnable was
+      // queued. This is why we store nsISupports in mCallbacks - otherwise we'd
+      // have to QI inside this lock.
+      if (mCallbacks == origCallbacks) {
+        // No one reset this out from under us, so go ahead and update our data.
+        mDocShellDependentStuffKnown = PR_TRUE;
+        mExternalErrorReporting = externalReporting;
+        previousCert.swap(mPreviousCert);
+
+        if (aExternalReporting) {
+          *aExternalReporting = mExternalErrorReporting;
+        }
+        if (aPreviousCert) {
+          NS_IF_ADDREF(*aPreviousCert = mPreviousCert);
+        }
+        return NS_OK;
       }
     }
-  }
 
-  return NS_OK;
+    // Someone replaced mCallbacks while we were running, try again.
+    NS_WARNING("Contention for mCallbacks, trying again");
+
+  } while(1); // Loop until we don't have contention.
+
+  NS_NOTREACHED("Should never get here");
+  return NS_ERROR_UNEXPECTED;
 }
 
 nsresult
 nsNSSSocketInfo::GetExternalErrorReporting(PRBool* state)
 {
-  nsresult rv = EnsureDocShellDependentStuffKnown();
+  NS_ENSURE_ARG_POINTER(state);
+  nsresult rv = EnsureDocShellDependentStuffKnown(state);
   NS_ENSURE_SUCCESS(rv, rv);
-  *state = mExternalErrorReporting;
+
   return NS_OK;
 }
 
 nsresult
 nsNSSSocketInfo::SetExternalErrorReporting(PRBool aState)
 {
+  nsAutoLock lock(mCallbacksLock);
   mExternalErrorReporting = aState;
   return NS_OK;
 }
@@ -544,27 +620,33 @@ nsNSSSocketInfo::SetErrorMessage(const PRUnichar* aText) {
 /* void getInterface (in nsIIDRef uuid, [iid_is (uuid), retval] out nsQIResult result); */
 NS_IMETHODIMP nsNSSSocketInfo::GetInterface(const nsIID & uuid, void * *result)
 {
-  nsresult rv;
+  nsCOMPtr<nsISupports> callbacks;
-  if (!mCallbacks) {
-    nsCOMPtr<nsIInterfaceRequestor> ir = new PipUIContext();
-    if (!ir)
-      return NS_ERROR_OUT_OF_MEMORY;
 
-    rv = ir->GetInterface(uuid, result);
+  {
-  } else {
+    nsAutoLock lock(mCallbacksLock);
+    callbacks = mCallbacks;
+  }
+
+  if (callbacks) {
+    // XXX Shouldn't we check this even if callbacks is null?
     if (nsSSLThread::exitRequested())
       return NS_ERROR_FAILURE;
 
     nsCOMPtr<nsIInterfaceRequestor> proxiedCallbacks;
-    NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
+    nsresult rv = NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
-                         NS_GET_IID(nsIInterfaceRequestor),
+                                       NS_GET_IID(nsIInterfaceRequestor),
-                         mCallbacks,
+                                       callbacks,
-                         NS_PROXY_SYNC,
+                                       NS_PROXY_SYNC,
-                         getter_AddRefs(proxiedCallbacks));
+                                       getter_AddRefs(proxiedCallbacks));
+    NS_ENSURE_SUCCESS(rv, rv);
 
-    rv = proxiedCallbacks->GetInterface(uuid, result);
+    return proxiedCallbacks->GetInterface(uuid, result);
   }
-  return rv;
+
+  nsCOMPtr<nsIInterfaceRequestor> ir = new PipUIContext();
+  NS_ENSURE_TRUE(ir, NS_ERROR_OUT_OF_MEMORY);
+
+  return ir->GetInterface(uuid, result);
 }
 
 nsresult
@@ -713,12 +795,9 @@ nsresult nsNSSSocketInfo::SetFileDescPtr(PRFileDesc* aFilePtr)
 nsresult nsNSSSocketInfo::GetPreviousCert(nsIX509Cert** _result)
 {
   NS_ENSURE_ARG_POINTER(_result);
-  nsresult rv = EnsureDocShellDependentStuffKnown();
+  nsresult rv = EnsureDocShellDependentStuffKnown(nsnull, _result);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  *_result = mPreviousCert;
-  NS_IF_ADDREF(*_result);
-
   return NS_OK;
 }
 

security/manager/ssl/src/nsNSSIOLayer.h

@@ -43,6 +43,7 @@
 
 #include "prtypes.h"
 #include "prio.h"
+#include "prlock.h"
 #include "certt.h"
 #include "nsString.h"
 #include "nsIInterfaceRequestor.h"
@@ -201,7 +202,7 @@ public:
   PRStatus CloseSocketAndDestroy();
   
 protected:
-  nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
+  nsCOMPtr<nsISupports> mCallbacks;
   PRFileDesc* mFd;
   nsCOMPtr<nsIX509Cert> mCert;
   nsCOMPtr<nsIX509Cert> mPreviousCert; // DocShellDependent
@@ -235,7 +236,13 @@ protected:
 
   nsSSLSocketThreadData *mThreadData;
 
-  nsresult EnsureDocShellDependentStuffKnown();
+  // This lock will protect mCallbacks, mDocShellDependentStuffKnown,
+  // mExternalErrorReporting, and mPreviousCert from concurrent changes.
+  PRLock* mCallbacksLock;
+
+  nsresult
+  EnsureDocShellDependentStuffKnown(PRBool* aExternalReporting = nsnull,
+                                    nsIX509Cert** aPreviousCert = nsnull);
 
 private:
   virtual void virtualDestroyNSSReference();

uriloader/base/nsDocLoader.cpp

@@ -219,8 +219,8 @@ nsDocLoader::~nsDocLoader()
 /*
  * Implementation of ISupports methods...
  */
-NS_IMPL_THREADSAFE_ADDREF(nsDocLoader)
+NS_IMPL_ADDREF(nsDocLoader)
-NS_IMPL_THREADSAFE_RELEASE(nsDocLoader)
+NS_IMPL_RELEASE(nsDocLoader)
 
 NS_INTERFACE_MAP_BEGIN(nsDocLoader)
    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIRequestObserver)