From b0e64746e6f197c2f3b93932d9a6ea56e6230760 Mon Sep 17 00:00:00 2001 From: Daniel Holbert Date: Thu, 11 Sep 2008 12:02:09 -0700 Subject: [PATCH] Backed out changeset 38140c8a8327 to see if it fixes Bug 454896 (sporadic RLk) --- .../protocol/http/src/nsHttpConnection.cpp | 2 +- .../boot/src/nsSecureBrowserUIImpl.cpp | 13 +- security/manager/ssl/src/nsNSSIOLayer.cpp | 295 +++++++----------- security/manager/ssl/src/nsNSSIOLayer.h | 11 +- uriloader/base/nsDocLoader.cpp | 4 +- 5 files changed, 120 insertions(+), 205 deletions(-) diff --git a/netwerk/protocol/http/src/nsHttpConnection.cpp b/netwerk/protocol/http/src/nsHttpConnection.cpp index af9457fc66a..47f0e1cd943 100644 --- a/netwerk/protocol/http/src/nsHttpConnection.cpp +++ b/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(NS_IsMainThread(), "wrong thread"); + NS_ASSERTION(PR_GetCurrentThread() != gSocketThread, "wrong thread"); if (mTransaction) { nsCOMPtr callbacks; diff --git a/security/manager/boot/src/nsSecureBrowserUIImpl.cpp b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp index 8711c0d3157..020d7796396 100644 --- a/security/manager/boot/src/nsSecureBrowserUIImpl.cpp +++ b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp @@ -190,12 +190,13 @@ nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl() PR_DestroyMonitor(mMonitor); } -NS_IMPL_ISUPPORTS6(nsSecureBrowserUIImpl, nsISecureBrowserUI, - nsIWebProgressListener, - nsIFormSubmitObserver, - nsIObserver, - nsISupportsWeakReference, - nsISSLStatusProvider) +NS_IMPL_THREADSAFE_ISUPPORTS6(nsSecureBrowserUIImpl, + nsISecureBrowserUI, + nsIWebProgressListener, + nsIFormSubmitObserver, + nsIObserver, + nsISupportsWeakReference, + nsISSLStatusProvider) NS_IMETHODIMP nsSecureBrowserUIImpl::Init(nsIDOMWindow *aWindow) diff --git a/security/manager/ssl/src/nsNSSIOLayer.cpp b/security/manager/ssl/src/nsNSSIOLayer.cpp index 165240cec14..e3135f1788f 100644 --- a/security/manager/ssl/src/nsNSSIOLayer.cpp +++ b/security/manager/ssl/src/nsNSSIOLayer.cpp @@ -222,15 +222,12 @@ nsNSSSocketInfo::nsNSSSocketInfo() mPort(0) { mThreadData = new nsSSLSocketThreadData; - mCallbacksLock = nsAutoLock::NewLock("nsNSSSocketInfo::mCallbacksLock"); } nsNSSSocketInfo::~nsNSSSocketInfo() { delete mThreadData; - nsAutoLock::DestroyLock(mCallbacksLock); - nsNSSShutDownPreventionLock locker; if (isAlreadyShutDown()) return; @@ -331,192 +328,119 @@ PRBool nsNSSSocketInfo::GetHasCleartextPhase() NS_IMETHODIMP nsNSSSocketInfo::GetNotificationCallbacks(nsIInterfaceRequestor** aCallbacks) { - nsCOMPtr supports; - { - nsAutoLock lock(mCallbacksLock); - supports = mCallbacks; - } - nsCOMPtr callbacks(do_QueryInterface(supports)); - callbacks.forget(aCallbacks); + *aCallbacks = mCallbacks; + NS_IF_ADDREF(*aCallbacks); return NS_OK; } NS_IMETHODIMP nsNSSSocketInfo::SetNotificationCallbacks(nsIInterfaceRequestor* aCallbacks) { - nsCOMPtr callbacks(do_QueryInterface(aCallbacks)); + if (!aCallbacks) { + mCallbacks = nsnull; + return NS_OK; + } - nsAutoLock lock(mCallbacksLock); + mCallbacks = aCallbacks; + mDocShellDependentStuffKnown = PR_FALSE; - callbacks.swap(mCallbacks); - if (mCallbacks) { - mDocShellDependentStuffKnown = PR_FALSE; + return NS_OK; +} + +nsresult +nsNSSSocketInfo::EnsureDocShellDependentStuffKnown() +{ + if (mDocShellDependentStuffKnown) + return NS_OK; + + if (!mCallbacks || nsSSLThread::exitRequested()) + return NS_ERROR_FAILURE; + + mDocShellDependentStuffKnown = PR_TRUE; + + nsCOMPtr proxiedCallbacks; + NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, + NS_GET_IID(nsIInterfaceRequestor), + static_cast(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 docshell; + + nsCOMPtr item(do_GetInterface(proxiedCallbacks)); + if (item) + { + nsCOMPtr proxiedItem; + nsCOMPtr 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) + { + nsCOMPtr proxiedDocShell; + 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 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 + // 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 statprov = do_QueryInterface(secureUI); + if (statprov) { + nsCOMPtr isup_stat; + statprov->GetSSLStatus(getter_AddRefs(isup_stat)); + if (isup_stat) { + nsCOMPtr sslstat = do_QueryInterface(isup_stat); + if (sslstat) { + sslstat->GetServerCert(getter_AddRefs(mPreviousCert)); + } + } + } + } } return NS_OK; } -class nsGatherDocshellInfoForPSMRunnable : public nsRunnable -{ -public: - nsGatherDocshellInfoForPSMRunnable(nsIInterfaceRequestor* aCallbacks, - PRBool* aExternalReporting, - nsIX509Cert** aPreviousCert) - : mCallbacks(aCallbacks), mExternalReporting(aExternalReporting), - mPreviousCert(aPreviousCert) - { - NS_ASSERTION(aCallbacks, "Null pointer!"); - } - - NS_IMETHOD Run() - { - NS_ASSERTION(NS_IsMainThread(), "Must run only on the main thread!"); - - *mExternalReporting = PR_FALSE; - *mPreviousCert = nsnull; - - // 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 item(do_GetInterface(mCallbacks)); - if (!item) - return NS_OK; - - nsCOMPtr rootItem; - item->GetSameTypeRootTreeItem(getter_AddRefs(rootItem)); - - nsCOMPtr docshell(do_QueryInterface(rootItem)); - NS_ASSERTION(docshell, "rootItem do_QI is null"); - if (!docshell) - return NS_OK; - - nsCOMPtr 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 statprov(do_QueryInterface(secureUI)); - if (!statprov) - return NS_OK; - - nsCOMPtr isup_stat; - statprov->GetSSLStatus(getter_AddRefs(isup_stat)); - - nsCOMPtr 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 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 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 mainThread(do_GetMainThread()); - NS_ENSURE_TRUE(mainThread, NS_ERROR_FAILURE); - - PRBool externalReporting = PR_FALSE; - nsCOMPtr previousCert; - - nsCOMPtr 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; - } - } - - // 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) { - NS_ENSURE_ARG_POINTER(state); - nsresult rv = EnsureDocShellDependentStuffKnown(state); + nsresult rv = EnsureDocShellDependentStuffKnown(); NS_ENSURE_SUCCESS(rv, rv); - + *state = mExternalErrorReporting; return NS_OK; } nsresult nsNSSSocketInfo::SetExternalErrorReporting(PRBool aState) { - nsAutoLock lock(mCallbacksLock); mExternalErrorReporting = aState; return NS_OK; } @@ -620,33 +544,27 @@ 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) { - nsCOMPtr callbacks; + nsresult rv; + if (!mCallbacks) { + nsCOMPtr ir = new PipUIContext(); + if (!ir) + return NS_ERROR_OUT_OF_MEMORY; - { - nsAutoLock lock(mCallbacksLock); - callbacks = mCallbacks; - } - - if (callbacks) { - // XXX Shouldn't we check this even if callbacks is null? + rv = ir->GetInterface(uuid, result); + } else { if (nsSSLThread::exitRequested()) return NS_ERROR_FAILURE; nsCOMPtr proxiedCallbacks; - nsresult rv = NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, - NS_GET_IID(nsIInterfaceRequestor), - callbacks, - NS_PROXY_SYNC, - getter_AddRefs(proxiedCallbacks)); - NS_ENSURE_SUCCESS(rv, rv); + NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, + NS_GET_IID(nsIInterfaceRequestor), + mCallbacks, + NS_PROXY_SYNC, + getter_AddRefs(proxiedCallbacks)); - return proxiedCallbacks->GetInterface(uuid, result); + rv = proxiedCallbacks->GetInterface(uuid, result); } - - nsCOMPtr ir = new PipUIContext(); - NS_ENSURE_TRUE(ir, NS_ERROR_OUT_OF_MEMORY); - - return ir->GetInterface(uuid, result); + return rv; } nsresult @@ -795,9 +713,12 @@ nsresult nsNSSSocketInfo::SetFileDescPtr(PRFileDesc* aFilePtr) nsresult nsNSSSocketInfo::GetPreviousCert(nsIX509Cert** _result) { NS_ENSURE_ARG_POINTER(_result); - nsresult rv = EnsureDocShellDependentStuffKnown(nsnull, _result); + nsresult rv = EnsureDocShellDependentStuffKnown(); NS_ENSURE_SUCCESS(rv, rv); + *_result = mPreviousCert; + NS_IF_ADDREF(*_result); + return NS_OK; } diff --git a/security/manager/ssl/src/nsNSSIOLayer.h b/security/manager/ssl/src/nsNSSIOLayer.h index e6210240c6f..1087e7fbe25 100644 --- a/security/manager/ssl/src/nsNSSIOLayer.h +++ b/security/manager/ssl/src/nsNSSIOLayer.h @@ -43,7 +43,6 @@ #include "prtypes.h" #include "prio.h" -#include "prlock.h" #include "certt.h" #include "nsString.h" #include "nsIInterfaceRequestor.h" @@ -202,7 +201,7 @@ public: PRStatus CloseSocketAndDestroy(); protected: - nsCOMPtr mCallbacks; + nsCOMPtr mCallbacks; PRFileDesc* mFd; nsCOMPtr mCert; nsCOMPtr mPreviousCert; // DocShellDependent @@ -236,13 +235,7 @@ protected: nsSSLSocketThreadData *mThreadData; - // This lock will protect mCallbacks, mDocShellDependentStuffKnown, - // mExternalErrorReporting, and mPreviousCert from concurrent changes. - PRLock* mCallbacksLock; - - nsresult - EnsureDocShellDependentStuffKnown(PRBool* aExternalReporting = nsnull, - nsIX509Cert** aPreviousCert = nsnull); + nsresult EnsureDocShellDependentStuffKnown(); private: virtual void virtualDestroyNSSReference(); diff --git a/uriloader/base/nsDocLoader.cpp b/uriloader/base/nsDocLoader.cpp index 19fca31731b..6ceccc2108e 100644 --- a/uriloader/base/nsDocLoader.cpp +++ b/uriloader/base/nsDocLoader.cpp @@ -219,8 +219,8 @@ nsDocLoader::~nsDocLoader() /* * Implementation of ISupports methods... */ -NS_IMPL_ADDREF(nsDocLoader) -NS_IMPL_RELEASE(nsDocLoader) +NS_IMPL_THREADSAFE_ADDREF(nsDocLoader) +NS_IMPL_THREADSAFE_RELEASE(nsDocLoader) NS_INTERFACE_MAP_BEGIN(nsDocLoader) NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIRequestObserver)