diff --git a/security/manager/boot/src/nsSecureBrowserUIImpl.cpp b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp index 98049c5c4681..971b81985fc9 100644 --- a/security/manager/boot/src/nsSecureBrowserUIImpl.cpp +++ b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp @@ -162,6 +162,7 @@ nsSecureBrowserUIImpl::nsSecureBrowserUIImpl() mMonitor = PR_NewMonitor(); mOnStateLocationChangeReentranceDetection = 0; mTransferringRequests.ops = nsnull; + mInconsistency = PR_FALSE; mNewToplevelSecurityState = STATE_IS_INSECURE; mNewToplevelIsEV = PR_FALSE; mNewToplevelSecurityStateKnown = PR_TRUE; @@ -596,10 +597,6 @@ nsSecureBrowserUIImpl::OnStateChange(nsIWebProgress* aWebProgress, PRUint32 aProgressStateFlags, nsresult aStatus) { - nsAutoAtomic atomic(mOnStateLocationChangeReentranceDetection); - NS_ASSERTION(mOnStateLocationChangeReentranceDetection == 1, - "unexpected parallel nsIWebProgress OnStateChange and/or OnLocationChange notification"); - /* All discussion, unless otherwise mentioned, only refers to http, https, file or wyciwig requests. @@ -690,19 +687,43 @@ nsSecureBrowserUIImpl::OnStateChange(nsIWebProgress* aWebProgress, regardless of whether the load flags indicate a top level document. */ - nsCOMPtr windowForProgress; - aWebProgress->GetDOMWindow(getter_AddRefs(windowForProgress)); + nsAutoAtomic atomic(mOnStateLocationChangeReentranceDetection); + if (mOnStateLocationChangeReentranceDetection > 1) + { + nsAutoMonitor lock(mMonitor); + mInconsistency = PR_TRUE; + // we ignore all events until the reentrance is gone + return NS_ERROR_FAILURE; + } + PRBool mustResetAfterInconsistency = PR_FALSE; + + nsCOMPtr windowForProgress; nsCOMPtr window; PRBool isViewSource; { nsAutoMonitor lock(mMonitor); + + if (mInconsistency) { + mInconsistency = PR_FALSE; + mustResetAfterInconsistency = PR_TRUE; + } + window = do_QueryReferent(mWindow); NS_ASSERTION(window, "Window has gone away?!"); isViewSource = mIsViewSource; } + if (mustResetAfterInconsistency) { + mNewToplevelSecurityState = STATE_IS_INSECURE; + mNewToplevelIsEV = PR_FALSE; + mNewToplevelSecurityStateKnown = PR_TRUE; + mSSLStatus = nsnull; + ResetStateTracking(); + } + + aWebProgress->GetDOMWindow(getter_AddRefs(windowForProgress)); const PRBool isToplevelProgress = (windowForProgress.get() == window.get()); #ifdef PR_LOGGING @@ -1251,8 +1272,8 @@ void nsSecureBrowserUIImpl::UpdateMyFlags(PRBool &showWarning, lockIconState &wa mNotifiedToplevelIsEV = mNewToplevelIsEV; } -nsresult nsSecureBrowserUIImpl::TellTheWorld(PRBool &showWarning, - lockIconState &warnSecurityState, +nsresult nsSecureBrowserUIImpl::TellTheWorld(PRBool showWarning, + lockIconState warnSecurityState, nsIRequest* aRequest) { nsCOMPtr temp_ToplevelEventSink; @@ -1319,8 +1340,20 @@ nsSecureBrowserUIImpl::OnLocationChange(nsIWebProgress* aWebProgress, nsIURI* aLocation) { nsAutoAtomic atomic(mOnStateLocationChangeReentranceDetection); - NS_ASSERTION(mOnStateLocationChangeReentranceDetection == 1, - "unexpected parallel nsIWebProgress OnStateChange and/or OnLocationChange notification"); + if (mOnStateLocationChangeReentranceDetection > 1) + { + nsAutoMonitor lock(mMonitor); + mInconsistency = PR_TRUE; + // We ignore all events until the reentrance is gone + // and has been reset by ::OnStateChange. + return NS_ERROR_FAILURE; + } + + // We could test for mInconsistency right here and exit, + // but let's avoid another lock to mMonitor. + // We'll do the preparation work based on the parameters, + // and once we are ready to lock the monitor, we'll do + // the inconsistency check. PRBool updateIsViewSource = PR_FALSE; PRBool temp_IsViewSource = PR_FALSE; @@ -1344,6 +1377,9 @@ nsSecureBrowserUIImpl::OnLocationChange(nsIWebProgress* aWebProgress, { nsAutoMonitor lock(mMonitor); + if (mInconsistency) { + return NS_ERROR_FAILURE; + } if (updateIsViewSource) { mIsViewSource = temp_IsViewSource; } diff --git a/security/manager/boot/src/nsSecureBrowserUIImpl.h b/security/manager/boot/src/nsSecureBrowserUIImpl.h index 7e7baf510b25..93779619b729 100644 --- a/security/manager/boot/src/nsSecureBrowserUIImpl.h +++ b/security/manager/boot/src/nsSecureBrowserUIImpl.h @@ -114,6 +114,7 @@ protected: void ResetStateTracking(); PRUint32 mNewToplevelSecurityState; + PRPackedBool mInconsistency; PRPackedBool mNewToplevelIsEV; PRPackedBool mNewToplevelSecurityStateKnown; PRPackedBool mIsViewSource; @@ -129,8 +130,8 @@ protected: static nsresult MapInternalToExternalState(PRUint32* aState, lockIconState lock, PRBool ev); nsresult UpdateSecurityState(nsIRequest* aRequest); void UpdateMyFlags(PRBool &showWarning, lockIconState &warnSecurityState); - nsresult TellTheWorld(PRBool &showWarning, - lockIconState &warnSecurityState, + nsresult TellTheWorld(PRBool showWarning, + lockIconState warnSecurityState, nsIRequest* aRequest); nsresult EvaluateAndUpdateSecurityState(nsIRequest *aRequest); diff --git a/security/manager/ssl/src/nsNSSCallbacks.cpp b/security/manager/ssl/src/nsNSSCallbacks.cpp index 30db376147f2..a94e9bfae458 100644 --- a/security/manager/ssl/src/nsNSSCallbacks.cpp +++ b/security/manager/ssl/src/nsNSSCallbacks.cpp @@ -80,20 +80,35 @@ NSSCleanupAutoPtrClass(CERTCertificate, CERT_DestroyCertificate) extern PRLogModuleInfo* gPIPNSSLog; #endif -struct nsHTTPDownloadEvent : nsRunnable { +class nsHTTPDownloadEvent : public nsRunnable { +public: nsHTTPDownloadEvent(); ~nsHTTPDownloadEvent(); NS_IMETHOD Run(); - + void Cancel(); + nsNSSHttpRequestSession *mRequestSession; // no ownership nsCOMPtr mListener; PRBool mResponsibleForDoneSignal; + +protected: + PRLock *mLock; + + // no nsCOMPtr. When I use it, I get assertions about + // loadgroup not being thread safe. + // So, let's use a raw pointer and ensure we only create and destroy + // it on the network thread ourselves. + nsILoadGroup *mLoadGroup; + PRThread *mLoadGroupOwnerThread; }; nsHTTPDownloadEvent::nsHTTPDownloadEvent() :mResponsibleForDoneSignal(PR_TRUE) +,mLock(nsnull) +,mLoadGroup(nsnull) +,mLoadGroupOwnerThread(nsnull) { } @@ -101,11 +116,17 @@ nsHTTPDownloadEvent::~nsHTTPDownloadEvent() { if (mResponsibleForDoneSignal && mListener) mListener->send_done_signal(); + if (mLock) + PR_DestroyLock(mLock); } NS_IMETHODIMP nsHTTPDownloadEvent::Run() { + mLock = PR_NewLock(); + if (!mLock) + return NS_ERROR_OUT_OF_MEMORY; + if (!mListener) return NS_OK; @@ -120,9 +141,16 @@ nsHTTPDownloadEvent::Run() // Create a loadgroup for this new channel. This way if the channel // is redirected, we'll have a way to cancel the resulting channel. - nsCOMPtr loadGroup = - do_CreateInstance(NS_LOADGROUP_CONTRACTID); - chan->SetLoadGroup(loadGroup); + nsCOMPtr lg; + { + nsAutoLock locker(mLock); + lg = do_CreateInstance(NS_LOADGROUP_CONTRACTID); + mLoadGroup = lg.get(); + NS_ADDREF(mLoadGroup); + mLoadGroupOwnerThread = PR_GetCurrentThread(); + } + chan->SetLoadGroup(lg); + lg = nsnull; if (mRequestSession->mHasPostData) { @@ -148,8 +176,6 @@ nsHTTPDownloadEvent::Run() rv = hchan->SetRequestMethod(mRequestSession->mRequestMethod); NS_ENSURE_SUCCESS(rv, rv); - nsSSLThread::rememberPendingHTTPRequest(loadGroup); - mResponsibleForDoneSignal = PR_FALSE; mListener->mResponsibleForDoneSignal = PR_TRUE; @@ -162,20 +188,47 @@ nsHTTPDownloadEvent::Run() if (NS_FAILED(rv)) { mListener->mResponsibleForDoneSignal = PR_FALSE; mResponsibleForDoneSignal = PR_TRUE; - - nsSSLThread::rememberPendingHTTPRequest(nsnull); } return NS_OK; } struct nsCancelHTTPDownloadEvent : nsRunnable { + nsRefPtr mEvent; + NS_IMETHOD Run() { - nsSSLThread::cancelPendingHTTPRequest(); + mEvent->Cancel(); + mEvent = nsnull; return NS_OK; } }; +void +nsHTTPDownloadEvent::Cancel() +{ + nsILoadGroup *lg = nsnull; + + if (mLock) { + nsAutoLock locker(mLock); + + if (mLoadGroup) { + if (mLoadGroupOwnerThread != PR_GetCurrentThread()) { + NS_ASSERTION(PR_FALSE, + "attempt to access nsHTTPDownloadEvent::mLoadGroup on multiple threads"); + } + else { + lg = mLoadGroup; + mLoadGroup = nsnull; + } + } + } + + if (lg) { + lg->Cancel(NS_ERROR_ABORT); + NS_RELEASE(lg); + } +} + SECStatus nsNSSHttpServerSession::createSessionFcn(const char *host, PRUint16 portnum, SEC_HTTP_SERVER_SESSION *pSession) @@ -367,7 +420,6 @@ nsNSSHttpRequestSession::internal_send_receive_attempt(PRBool &retryable_error, } PRBool request_canceled = PR_FALSE; - PRBool aborted_wait = PR_FALSE; { nsAutoLock locker(waitLock); @@ -410,30 +462,26 @@ nsNSSHttpRequestSession::internal_send_receive_attempt(PRBool &retryable_error, if (!request_canceled) { - if ((PRIntervalTime)(PR_IntervalNow() - start_time) > mTimeoutInterval) + PRBool wantExit = nsSSLThread::exitRequested(); + PRBool timeout = + (PRIntervalTime)(PR_IntervalNow() - start_time) > mTimeoutInterval; + + if (wantExit || timeout) { request_canceled = PR_TRUE; - // but we'll to continue to wait for waitFlag - - nsCOMPtr cancelevent = new nsCancelHTTPDownloadEvent; + + nsRefPtr cancelevent = new nsCancelHTTPDownloadEvent; + cancelevent->mEvent = event; rv = NS_DispatchToMainThread(cancelevent); - if (NS_FAILED(rv)) - { + if (NS_FAILED(rv)) { NS_WARNING("cannot post cancel event"); - aborted_wait = PR_TRUE; - break; } + break; } } } } - if (aborted_wait) - { - // we couldn't cancel it, let's no longer reference it - nsSSLThread::rememberPendingHTTPRequest(nsnull); - } - if (request_canceled) return SECFailure; @@ -628,8 +676,6 @@ nsHTTPListener::OnStreamComplete(nsIStreamLoader* aLoader, void nsHTTPListener::send_done_signal() { - nsSSLThread::rememberPendingHTTPRequest(nsnull); - mResponsibleForDoneSignal = PR_FALSE; { diff --git a/security/manager/ssl/src/nsNSSComponent.cpp b/security/manager/ssl/src/nsNSSComponent.cpp index 38d4813e79f7..a48faada70e3 100644 --- a/security/manager/ssl/src/nsNSSComponent.cpp +++ b/security/manager/ssl/src/nsNSSComponent.cpp @@ -1722,6 +1722,19 @@ nsNSSComponent::Init() return rv; } + // Access our string bundles now, this prevents assertions from I/O + // - nsStandardURL not thread-safe + // - wrong thread: 'NS_IsMainThread()' in nsIOService.cpp + // when loading error strings on the SSL threads. + { + NS_NAMED_LITERAL_STRING(dummy_name, "dummy"); + nsXPIDLString result; + mPIPNSSBundle->GetStringFromName(dummy_name.get(), + getter_Copies(result)); + mNSSErrorsBundle->GetStringFromName(dummy_name.get(), + getter_Copies(result)); + } + if (!mPrefBranch) { mPrefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID); NS_ASSERTION(mPrefBranch, "Unable to get pref service"); diff --git a/security/manager/ssl/src/nsNSSIOLayer.cpp b/security/manager/ssl/src/nsNSSIOLayer.cpp index 91a63a94404d..4718ba2be8b8 100644 --- a/security/manager/ssl/src/nsNSSIOLayer.cpp +++ b/security/manager/ssl/src/nsNSSIOLayer.cpp @@ -204,6 +204,7 @@ nsNSSSocketInfo::nsNSSSocketInfo() : mFd(nsnull), mBlockingState(blocking_state_unknown), mSecurityState(nsIWebProgressListener::STATE_IS_INSECURE), + mDocShellDependentStuffKnown(PR_FALSE), mExternalErrorReporting(PR_FALSE), mForSTARTTLS(PR_FALSE), mHandshakePending(PR_TRUE), @@ -319,15 +320,30 @@ nsNSSSocketInfo::SetNotificationCallbacks(nsIInterfaceRequestor* aCallbacks) return NS_OK; } + mCallbacks = aCallbacks; + mDocShellDependentStuffKnown = PR_FALSE; + + return NS_OK; +} + +NS_IMETHODIMP +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(aCallbacks), + static_cast(mCallbacks), NS_PROXY_SYNC, getter_AddRefs(proxiedCallbacks)); - mCallbacks = 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. @@ -339,7 +355,7 @@ nsNSSSocketInfo::SetNotificationCallbacks(nsIInterfaceRequestor* aCallbacks) nsCOMPtr docshell; - nsCOMPtr item(do_GetInterface(mCallbacks)); + nsCOMPtr item(do_GetInterface(proxiedCallbacks)); if (item) { nsCOMPtr proxiedItem; @@ -395,6 +411,8 @@ nsNSSSocketInfo::SetNotificationCallbacks(nsIInterfaceRequestor* aCallbacks) nsresult nsNSSSocketInfo::GetExternalErrorReporting(PRBool* state) { + nsresult rv = EnsureDocShellDependentStuffKnown(); + NS_ENSURE_SUCCESS(rv, rv); *state = mExternalErrorReporting; return NS_OK; } @@ -465,10 +483,17 @@ NS_IMETHODIMP nsNSSSocketInfo::GetInterface(const nsIID & uuid, void * *result) rv = ir->GetInterface(uuid, result); } else { - // Proxy of the channel callbacks should probably go here, rather - // than in the password callback code + if (nsSSLThread::exitRequested()) + return NS_ERROR_FAILURE; - rv = mCallbacks->GetInterface(uuid, result); + nsCOMPtr proxiedCallbacks; + NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, + NS_GET_IID(nsIInterfaceRequestor), + mCallbacks, + NS_PROXY_SYNC, + getter_AddRefs(proxiedCallbacks)); + + rv = proxiedCallbacks->GetInterface(uuid, result); } return rv; } @@ -619,6 +644,8 @@ nsresult nsNSSSocketInfo::SetFileDescPtr(PRFileDesc* aFilePtr) nsresult nsNSSSocketInfo::GetPreviousCert(nsIX509Cert** _result) { NS_ENSURE_ARG_POINTER(_result); + nsresult rv = EnsureDocShellDependentStuffKnown(); + NS_ENSURE_SUCCESS(rv, rv); *_result = mPreviousCert; NS_IF_ADDREF(*_result); @@ -1139,6 +1166,9 @@ displayAlert(nsAFlatString &formattedString, nsNSSSocketInfo *infoObject) // The interface requestor object may not be safe, so proxy the call to get // the nsIPrompt. + if (nsSSLThread::exitRequested()) + return NS_ERROR_FAILURE; + nsCOMPtr proxiedCallbacks; NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, NS_GET_IID(nsIInterfaceRequestor), @@ -1173,6 +1203,10 @@ nsHandleSSLError(nsNSSSocketInfo *socketInfo, PRInt32 err) return NS_OK; } + if (nsSSLThread::exitRequested()) { + return NS_ERROR_FAILURE; + } + nsresult rv; NS_DEFINE_CID(nssComponentCID, NS_NSSCOMPONENT_CID); nsCOMPtr nssComponent(do_GetService(nssComponentCID, &rv)); @@ -1187,9 +1221,16 @@ nsHandleSSLError(nsNSSSocketInfo *socketInfo, PRInt32 err) socketInfo->GetPort(&port); // Try to get a nsISSLErrorListener implementation from the socket consumer. - nsCOMPtr callbacks; - socketInfo->GetNotificationCallbacks(getter_AddRefs(callbacks)); - if (callbacks) { + nsCOMPtr cb; + socketInfo->GetNotificationCallbacks(getter_AddRefs(cb)); + if (cb) { + nsCOMPtr callbacks; + NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, + NS_GET_IID(nsIInterfaceRequestor), + cb, + NS_PROXY_SYNC, + getter_AddRefs(callbacks)); + nsCOMPtr sel = do_GetInterface(callbacks); if (sel) { nsISSLErrorListener *proxy_sel = nsnull; @@ -2743,6 +2784,9 @@ nsNSSBadCertHandler(void *arg, PRFileDesc *sslSocket) if (!infoObject) return SECFailure; + if (nsSSLThread::exitRequested()) + return cancel_and_failure(infoObject); + CERTCertificate *peerCert = nsnull; CERTCertificateCleaner peerCertCleaner(peerCert); peerCert = SSL_PeerCertificate(sslSocket); @@ -2911,9 +2955,16 @@ nsNSSBadCertHandler(void *arg, PRFileDesc *sslSocket) nsresult rv; // Try to get a nsIBadCertListener2 implementation from the socket consumer. - nsCOMPtr callbacks; - infoObject->GetNotificationCallbacks(getter_AddRefs(callbacks)); - if (callbacks) { + nsCOMPtr cb; + infoObject->GetNotificationCallbacks(getter_AddRefs(cb)); + if (cb) { + nsCOMPtr callbacks; + NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, + NS_GET_IID(nsIInterfaceRequestor), + cb, + NS_PROXY_SYNC, + getter_AddRefs(callbacks)); + nsCOMPtr bcl = do_GetInterface(callbacks); if (bcl) { nsIBadCertListener2 *proxy_bcl = nsnull; diff --git a/security/manager/ssl/src/nsNSSIOLayer.h b/security/manager/ssl/src/nsNSSIOLayer.h index 54ea1be056d9..b5fddb03a270 100644 --- a/security/manager/ssl/src/nsNSSIOLayer.h +++ b/security/manager/ssl/src/nsNSSIOLayer.h @@ -198,14 +198,15 @@ protected: nsCOMPtr mCallbacks; PRFileDesc* mFd; nsCOMPtr mCert; - nsCOMPtr mPreviousCert; + nsCOMPtr mPreviousCert; // DocShellDependent enum { blocking_state_unknown, is_nonblocking_socket, is_blocking_socket } mBlockingState; PRUint32 mSecurityState; nsString mShortDesc; nsString mErrorMessage; - PRPackedBool mExternalErrorReporting; + PRPackedBool mDocShellDependentStuffKnown; + PRPackedBool mExternalErrorReporting; // DocShellDependent PRPackedBool mForSTARTTLS; PRPackedBool mHandshakePending; PRPackedBool mCanceled; @@ -223,6 +224,8 @@ protected: nsSSLSocketThreadData *mThreadData; + nsresult EnsureDocShellDependentStuffKnown(); + private: virtual void virtualDestroyNSSReference(); void destructorSafeDestroyNSSReference(); diff --git a/security/manager/ssl/src/nsSSLThread.cpp b/security/manager/ssl/src/nsSSLThread.cpp index 1304f695f664..8c5ccc3aa890 100644 --- a/security/manager/ssl/src/nsSSLThread.cpp +++ b/security/manager/ssl/src/nsSSLThread.cpp @@ -1120,28 +1120,14 @@ void nsSSLThread::Run(void) } } -void nsSSLThread::rememberPendingHTTPRequest(nsIRequest *aRequest) +PRBool nsSSLThread::exitRequested() { if (!ssl_thread_singleton) - return; + return PR_FALSE; - nsAutoLock threadLock(ssl_thread_singleton->mMutex); + // no lock - ssl_thread_singleton->mPendingHTTPRequest = aRequest; -} - -void nsSSLThread::cancelPendingHTTPRequest() -{ - if (!ssl_thread_singleton) - return; - - nsAutoLock threadLock(ssl_thread_singleton->mMutex); - - if (ssl_thread_singleton->mPendingHTTPRequest) - { - ssl_thread_singleton->mPendingHTTPRequest->Cancel(NS_ERROR_ABORT); - ssl_thread_singleton->mPendingHTTPRequest = nsnull; - } + return ssl_thread_singleton->mExitRequested; } nsSSLThread *nsSSLThread::ssl_thread_singleton = nsnull; diff --git a/security/manager/ssl/src/nsSSLThread.h b/security/manager/ssl/src/nsSSLThread.h index 5e60f11a52a3..d91b2ee77278 100644 --- a/security/manager/ssl/src/nsSSLThread.h +++ b/security/manager/ssl/src/nsSSLThread.h @@ -152,9 +152,7 @@ public: static nsresult requestActivateSSL(nsNSSSocketInfo *si); - // Called from either Necko or SSL thread. - static void rememberPendingHTTPRequest(nsIRequest *aRequest); - static void cancelPendingHTTPRequest(); + static PRBool exitRequested(); }; #endif //_NSSSLTHREAD_H_