+++ This bug was initially created as a clone of Bug #679036 +++ Currently, we use many XPCOM proxies in the processing of SSL errors (e.g. reporting errors). Instead of replacing each XPCOM proxy usage with a separate runnable that gets sync-dispatched to the main thread, let's move all of this processing to a single runnable that gets dispatched to the main thread.

You probably have to apply the patch for bug 679144 before applying this one, as the removal of EnsureDocShellDependentStuff depends on the removal of GetPreviousCert() in that bug.

This patch removes the XPCOM proxies that are using during the processing of SSL errors, including certificate errors. You need to apply this patch first: https://bugzilla.mozilla.org/attachment.cgi?id=562083&action=edit

Comment on attachment 562086 [details] [diff] [review] Handle all SSL errors (including cert errors) on the main thread Review of attachment 562086 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with following comments. ::: security/manager/ssl/src/nsCertOverrideService.cpp @@ +154,5 @@ > > + nsCOMPtr<nsIObserverService> observerService = > + mozilla::services::GetObserverService(); > + if (!observerService) > + return NS_ERROR_FAILURE; Too hard failure. Use the previous conditional code please. @@ +158,5 @@ > + return NS_ERROR_FAILURE; > + > + observerService->AddObserver(this, "profile-before-change", PR_TRUE); > + observerService->AddObserver(this, "profile-do-change", PR_TRUE); > + observerService->AddObserver(this, "shutdown-cleanse", PR_TRUE); Registering for "shutdown-cleanse" topic is non-sense. It's an arg to profile-before-change notification, correctly handled in Observe(). @@ +164,4 @@ > // cache mSettingsFile > NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mSettingsFile)); > + if (!mSettingsFile) > + return NS_ERROR_FAILURE; I think this service should work even w/o a profile. You should not hard fail Init() on failure of getting the profile dir. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ -1700,5 @@ > - if (NS_FAILED(rv)) { > - nsPSMInitPanic::SetPanic(); > - return rv; > - } > - } Hmm... added in bug 619201. I'd a bit more prefer to leave this code here, but up to you. It has no affect on the start-up time anyway, I think. I think we should have a mechanism to ensure nss component be initialized on the main thread even it may get called from a background thread (e.g. in a worker using DOMCrypto, if it's accessible to it). ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1383,1 @@ > if (socketInfo->GetCanceled()) { Value of this getter may get changed on either SSL thread or network thread. On the network thread only when the socket associated with the info is not "busy" (i.e. the socket is the only one currently processed on the SSL thread). It means it is safe to read it here on the main thread because the socket is now busy and no code can modify it. Some comment like this (feel free to rephrase my poor english) would be useful here. @@ +1388,3 @@ > } > > if (nsSSLThread::stoppedOrStopping()) { This is read on some places w/o a lock anyway, so it is probably ok to read it here as well. @@ +1854,5 @@ > > // This is the common place where we trigger an error message on a SSL socket. > // This might be reached at any time of the connection. > if (!wantRetry && (IS_SSL_ERROR(err) || IS_SEC_ERROR(err))) { > + nsCOMPtr<SyncRunnableBase> runnable = new SSLErrorRunnable(socketInfo, err); nsRefPtr please. @@ +3202,5 @@ > + PRBool const mHasCertNameMismatch; > + SECStatus mRv; > + PRErrorCode mErrorCodeToReport; > + nsXPIDLCString mHostname; > +}; Might be nice to group and comment members (read: event arguments) that are in, in/out (if any) and out only. @@ +3313,5 @@ > + defaultErrorCodeToReport); > + > + // now grab the host name to pass to the STS Service > + nsrv = infoObject->GetHostName(getter_Copies(runnable->mHostname)); > + if (!NS_SUCCEEDED(nsrv)) { NS_FAILED ? @@ +3355,5 @@ > + return; > + > + PRErrorCode errorCodeMismatch = 0; > + PRErrorCode errorCodeTrust = 0; > + PRErrorCode errorCodeExpired = 0; Use SECSuccess instead of 0 ? @@ +3524,5 @@ > NS_ConvertASCIItoUTF16 hostU(hostString); > NS_ConvertASCIItoUTF16 hostWithPortU(hostWithPortString); > > + nsCOMPtr<nsINSSComponent> inss = do_GetService(kNSSComponentCID, &nsrv); > + if (NS_SUCCEEDED(nsrv)) { Can you just add a comment why this is needed ? I think nss component is already up when we get here.

(In reply to Honza Bambas (:mayhemer) from comment #4) > ::: security/manager/ssl/src/nsCertOverrideService.cpp > @@ +154,5 @@ > > > > + nsCOMPtr<nsIObserverService> observerService = > > + mozilla::services::GetObserverService(); > > + if (!observerService) > > + return NS_ERROR_FAILURE; > > Too hard failure. Use the previous conditional code please. We really need the observer service to operate correctly and securely. Otherwise, we may end up reading/writing to the wrong override files after a profile switch. Really, mozilla::services::GetObserverService() must never fail; otherwise, I am sure many, many things break. > > + observerService->AddObserver(this, "profile-before-change", PR_TRUE); > > + observerService->AddObserver(this, "profile-do-change", PR_TRUE); > > + observerService->AddObserver(this, "shutdown-cleanse", PR_TRUE); > > Registering for "shutdown-cleanse" topic is non-sense. It's an arg to > profile-before-change notification, correctly handled in Observe(). Agreed. I will make this change. > > @@ +164,4 @@ > > // cache mSettingsFile > > NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mSettingsFile)); > > + if (!mSettingsFile) > > + return NS_ERROR_FAILURE; > > I think this service should work even w/o a profile. You should not hard > fail Init() on failure of getting the profile dir. OK. I will try to make the service operate correctly without a profile. > ::: security/manager/ssl/src/nsNSSComponent.cpp > @@ -1700,5 @@ > > - if (NS_FAILED(rv)) { > > - nsPSMInitPanic::SetPanic(); > > - return rv; > > - } > > - } > > Hmm... added in bug 619201. > > I'd a bit more prefer to leave this code here, but up to you. It has no > affect on the start-up time anyway, I think. This patch is basically an alternative solution to bug 619201, and that patch caused bug 650858. This patch will fix both of those bugs. > > + PRErrorCode errorCodeMismatch = 0; > > + PRErrorCode errorCodeTrust = 0; > > + PRErrorCode errorCodeExpired = 0; > > Use SECSuccess instead of 0 ? SECSuccess is of type SECStatus, not PRErrorCode. That is why I changed it to 0. > > + nsCOMPtr<nsINSSComponent> inss = do_GetService(kNSSComponentCID, &nsrv); > > + if (NS_SUCCEEDED(nsrv)) { > > Can you just add a comment why this is needed ? I think nss component is > already up when we get here. Yes, the NSS component is already up. Simply, we need a reference to it to pass to a function a few lines afterward. Thanks for r+ing the patch. I am going to upload another version for you to review because of the small disagreements/misunderstandings above.

Review comments addressed. I changed the error handling in the cert override service so that it will work without a profile, and in such a way that it will not ever miss a profile change event. I also changed the external error reporting to behave like it did before.

The patch queue for this patch should look like this: psm-remove-default-ssl-error-UI.patch (bug 682329) psm-remove-certerror-test.patch (bug 682329) psm-runnable.patch (bug 675221) psm-handle-ssl-and-cert-errors-on-main-thread.patch This patch will not work without the preceeding patches.

Note that we never change profiles in a process: we start out with no profile, we start a profile, and then we shut down. It is not necessary to support switching profile directories.

Ben, I would be very happy to be able to assume that. However, I was told Seamonkey does implement same-process profile switching. Also, Dan Mills and Ben Adida mentioned that they were considering profile switching for "Sign in to the Browser." Regardless, PSM has always tried to support it, and it wasn't hard to fix the patch to do what we need.

We absolutely don't support it in any application nowadays and haven't since seamonkey switched to the new addons manager and toolkit/xre startup sequence, so it's really dead/untested code.

Comment on attachment 564046 [details] [diff] [review] Do all SSL error processing on the main thread [v2] Review of attachment 564046 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Looks good. r=honzab with one question answered. ::: security/manager/ssl/src/nsCertOverrideService.cpp @@ +159,5 @@ > + // attempt to read/write any settings file. Otherwise, we would end up > + // reading/writing the wrong settings file after a profile change. > + if (observerService) { > + observerService->AddObserver(this, "profile-before-change", true); > + observerService->AddObserver(this, "profile-do-change", true); Maybe add a new line here to make direct call to Observe more visible. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +552,5 @@ > { > + if (!NS_IsMainThread()) { > + NS_NOTREACHED("nsNSSSocketInfo::GetInterface called off the main thread"); > + return NS_ERROR_NOT_SAME_THREAD; > + } The question: Can you explain what changes in the patch ensure this will never happen or is it actually necessary to ensure this? @@ +1792,5 @@ > > return false; > } > > +class SSLErrorRunnable : public SyncRunnableBase What about to call this e.g. SyncHandleSSLError to reflect that it calls nsHandleSSLError function? @@ +3242,4 @@ > return SECFailure; > > + if (defaultErrorCodeToReport == 0) { > + NS_NOTREACHED("No error code set during certificate validation failure."); Please use here rather NS_ERROR or NS_WARNING ; i'd prefer the letter because you recover from this state in the following code. However, this may indicate an error (critical? - you might know better then me) in the cert validation code, so it should not be loosen to warning but rather be an error then.. @@ +3480,5 @@ > nsCOMPtr<nsIStrictTransportSecurityService> stss > = do_GetService(NS_STSSERVICE_CONTRACTID); > + if (stss) { > + nsrv = stss->IsStsHost(mHostname, &strictTransportSecurityEnabled); > + if (!NS_SUCCEEDED(nsrv)) NS_FAILED ? @@ +3482,5 @@ > + if (stss) { > + nsrv = stss->IsStsHost(mHostname, &strictTransportSecurityEnabled); > + if (!NS_SUCCEEDED(nsrv)) > + return; // use default rv and errorCodeToReport > + } Looks like when we don't have STS service (in that case creation of the proxy object fails) the old code fails (result is SECFailure). But yours passes. Sorry I didn't spot this in the previous version of the patch.

I moved the changes to nsNSSSocketInfo::GetInterface to the relevant patch in bug 675221, where they belong. I did not rename the SSLErrorRunnable class in order to keep its name analogous to CertErrorRunnable. I also rebased the patch on current mozilla-central (without requiring any PR_TRUE/PR_FALSE -> true/false patches), because this will need to be checked into mozilla-aurora to fix bug 650858.

Comment on attachment 565145 [details] [diff] [review] Do all SSL error processing on the main thread [v3] Carrying over r+.

Comment on attachment 565145 [details] [diff] [review] Do all SSL error processing on the main thread [v3] https://hg.mozilla.org/mozilla-central/rev/eb25f2b8c126

Tracking Firefox 8 to fix bug 650858.

Tracking Firefox 8 and 9 to fix bug 650858. A simpler alternative to taking this is to back out the patch for bug 619201 like we did for Firefox 6 and 7.

---------------------------------[ Triage Comment ]--------------------------------- We decided to instead back out the patch for bug 619201 like we did for Firefox 6 and 7. The backouts are in bug 650858 and we shouldn't need them on mozilla-central / Firefox 10 because this landed there. There is nothing to do here and no need to track.