Currently the PB service calls logoutAndTeardown on the private-browsing notification. We need to support a way to use the SDR for both private and public connections, and do something like logoutAndTeardown on last-pb-context-exited.

1. Why do we call logoutAndTeardown for private browsing? 2. What do we use SDR for during private browsing? My understanding is that SDR is used for encrypting data that we store on disk. AFAICT, we don't need to encrypt any data that we store only in memory. So, this indicates to me that we shouldn't be using SDR at all for private connections. But, I don't know all the uses of SDR so I may be way off base here.

(In reply to Brian Smith (:bsmith) from comment #1) > 1. Why do we call logoutAndTeardown for private browsing? To make sure that all of the authenticated sessions are terminated, so that websites cannot associate your private and non-private sessions. > 2. What do we use SDR for during private browsing? The private browsing code doesn't use SDR except for calling logoutAndTeardown on it. > My understanding is that SDR is used for encrypting data that we store on > disk. AFAICT, we don't need to encrypt any data that we store only in > memory. So, this indicates to me that we shouldn't be using SDR at all for > private connections. > > But, I don't know all the uses of SDR so I may be way off base here. Hmm, this code dates back to 2008 when I was working on the initial implementation. I'm sure that somebody told me that this is the right way to do this, but I don't remember who, and I know next to nothing about SDR myself.

@Ehsan,Josh Can you refer https://bugzilla.mozilla.org/show_bug.cgi?id=769283#c1. Since this is on the same pattern as that other one, I'd like to take up this too, if Chris hasn't yet started working on it. Thanks.

As far as I can tell, we only use the SDR for providing encrypt and decrypt in the password storage (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-mozStorage.js), meaning that it isn't used in PB windows, and in one place in Sync code to unlock the master password. This suggests to me that we can forgo requiring concurrent stuff; however, Brian, could you take a look at LogoutAndTeardown (http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsSDR.cpp#261) and let us know if that is cleaning up anything else we should be worrying about?

Formalizing the request in comment 4.

AFAICT, it is a bug if LogoutAndTeardown during private browsing transitions when we support concurrent non-private and private browsing windows. Instead, private browsing windows need to make sure that they (a) shut down all connections created in private browsing mode, including SSL connections, (b) use a separate "client certificate remember" cache from non-private browsing connections, and (c) stop writing to the intermediate certificate cache in AuthCertificate (in SSLServerCertVerification.cpp).

I think we'll get a) for free by nature of all windows being closed (and downloads cancelled), causing their channels to be cancelled, but I wouldn't mind somebody like mcmanus backing me up here.

c) appears to be made very easy by my changes in bug 722979.

Comment on attachment 678480 [details] [diff] [review] Make PSM more amenable to storing concurrent private and non-private data. This is what I've been working on, based on our IRC conversation. This does eliminate one of the cases that I have determined we are definitely cleaning up on leaving global PB mode (based on nsNSSComponent::LogoutAuthenticatedPK11), but I have no been able to make heads or tails of PK11_LogoutAll or SSL_ClearSessionCache. Do you have any thoughts about those?

Comment on attachment 678488 [details] [diff] [review] Part 2: Avoid storing intermediate cert data for private contexts. This patch implements the c) in comment 6, but it's a very chemistry dog moment: http://i.qkme.me/3rngcw.jpg

This is the easier way of dealing with cert overrides which gives us parity with the existing implementation. I have another half-completed patch that modifies the nsICertOverrideService API to allow concurrent public and private overrides, but it's more churn and complicated by nsCertTree.

Here's the rundown on the effects of these patches versus the existing call to LogoutAndTearDown: * we don't call PK11_LogoutAll; I discussed its effects with Kai and it didn't sound like it would be a requirement for this situation. * we don't trigger net:prune-dead-connections - this may be necessary, but won't be hard to do. Input required. * we don't trigger the PK11 logout for the NSS shutdown list - I require further input here; there are a number of classes that are mysterious to me and I can't guarantee any kind of sensible evaluation of their uses. I would appreciate feedback soon, since the goal is to finish this work before the next merge.

Comment on attachment 678480 [details] [diff] [review] Make PSM more amenable to storing concurrent private and non-private data. Review of attachment 678480 [details] [diff] [review]: ----------------------------------------------------------------- In general, looks pretty good. I overlooked that the idea we discussed would not work for exceptionDialog.js. I suggest a fix for that below. ::: security/manager/pki/resources/content/exceptionDialog.js @@ +87,5 @@ > > // returns true if found and global status could be set > function findRecentBadCert(uri) { > try { > + //XXXjdm I suggest you add: nsIRecentBadCerts getRecentBadCertsService(bool isPrivate); to nsIX509CertDB. nsNSSCertificateDB can maintain mPrivateRecentBadCerts and mPublicRecentBadCerts (lazily initialized), and you can remove the mRecentBadCertsService member from SharedSSLState; in nsNSSIOLayer, just use the new nsIX509CertDB.getRecentBadCertsService() method. Also, we should remove the XPCOM service registration for the nsIRecentBadCertsService interface. ::: security/manager/ssl/src/SharedSSLState.cpp @@ +12,5 @@ > +#include "nsIObserverService.h" > +#include "mozilla/Services.h" > +#include "nsCRT.h" > + > +using namespace mozilla::psm; class SharedSSLState should be within the mozilla::psm private namespace. @@ +15,5 @@ > + > +using namespace mozilla::psm; > + > +static SharedSSLState* gPublicState; > +static SharedSSLState* gPrivateState; These should be within the namespace { } so they don't need to be marked static. @@ +74,5 @@ > + mIOLayerHelpers.clearStoredData(); > +} > + > +void > +SharedSSLState::GlobalInit() It is helpful to add MOZ_ASSERT(NS_IsMainThread()); to document that this occurs on the main thread since most things don't. @@ +82,5 @@ > + gPrivateState->NotePrivateBrowsingStatus(); > +} > + > +void > +SharedSSLState::GlobalCleanup() MOZ_ASSERT(NS_IsMainThread()) ::: security/manager/ssl/src/nsNSSCallbacks.cpp @@ +822,5 @@ > PRBool siteSupportsSafeRenego; > if (SSL_HandshakeNegotiatedExtension(fd, ssl_renegotiation_info_xtn, &siteSupportsSafeRenego) != SECSuccess > || !siteSupportsSafeRenego) { > > + bool wantWarning = (ioLayerHelpers->getWarnLevelMissingRFC5746() > 0); We do not need to store multiple copies of the prefs, so the static members of nsNSSIOLayerHelpers that contain prefs could remain static, and most of your pref-related changes can be reverted. (In fact, it might be simpler to just move the things you made non-static--the TLS intolerant site list and the client auth remember list--directly into SharedSSLState. Up to you. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +437,5 @@ > { > mAllowTLSIntoleranceTimeout = aAllow; > } > > +void nsNSSSocketInfo::SetSharedState(SharedSSLState* aState) aState should just be passed to the nsNSSSocketInfo constructor to avoid needing this setter. That would make it clearer that mSharedState will never change throughout the lifetime of the instance. (Also, we can make mSharedState an "SharedSSLState &" to emphasize this, and the fact that the shared state is never null.) @@ +442,5 @@ > +{ > + mSharedState = aState; > +} > + > +SharedSSLState* nsNSSSocketInfo::SharedState() IMO, it is better to return SharedSSLState & since it can't ever be null.

Comment on attachment 679160 [details] [diff] [review] Clear all temporary cert overrides upon leaving private browsing. Review of attachment 679160 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +2317,5 @@ > else if (nsCRT::strcmp(aTopic, PROFILE_CHANGE_NET_RESTORE_TOPIC) == 0) { > PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("receiving network restore topic\n")); > DoProfileChangeNetRestore(); > } > + else if (nsCRT::strcmp(aTopic, LEAVE_PRIVATE_BROWSING_TOPIC) == 0) { This can be done in nsCertOverrideService.cpp, not here.

Comment on attachment 679161 [details] [diff] [review] Part 3: Clear SSL session cache upon leaving private browsing. Review of attachment 679161 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +2327,5 @@ > 0); > + } > + > + { > + nsNSSShutDownPreventionLock locker; We don't need this because this happens on the main thread and NSS shutdown happens on the main thread. @@ +2328,5 @@ > + } > + > + { > + nsNSSShutDownPreventionLock locker; > + SSL_ClearSessionCache(); I need to investigate if there is a better way of doing this (having separate private browsing and non-private-browsing caches). I'd rather not reset the whole session cache if we can avoid it.

(In reply to Josh Matthews [:jdm] from comment #15) > * we don't trigger net:prune-dead-connections - this may be necessary, but > won't be hard to do. Input required. Kai or Honza may know better why this was done. I think it may only be needed to prevent some bad things happening during actual shutdown, but I am not sure. > * we don't trigger the PK11 logout for the NSS shutdown list - I require > further input here; there are a number of classes that are mysterious to me > and I can't guarantee any kind of sensible evaluation of their uses. The shutdown list exists to tell objects that they must never call any more NSS functions because NSS has become unavailable. So, it is OK to skip doing this.

>::: security/manager/ssl/src/nsNSSCallbacks.cpp >@@ +822,5 @@ >> PRBool siteSupportsSafeRenego; >> if (SSL_HandshakeNegotiatedExtension(fd, ssl_renegotiation_info_xtn, >&siteSupportsSafeRenego) != SECSuccess >> || !siteSupportsSafeRenego) { >> >> + bool wantWarning = (ioLayerHelpers->getWarnLevelMissingRFC5746() > 0); > >We do not need to store multiple copies of the prefs, so the static members of >nsNSSIOLayerHelpers that contain prefs could remain static, and most of your pref-related >changes can be reverted. > >(In fact, it might be simpler to just move the things you made non-static--the TLS intolerant >site list and the client auth remember list--directly into SharedSSLState. Up to you. I originally tried to do this, but since the mutex is no longer static the setters for the pref values can't be static either. Storing an extra copy of the pref value is a small price to pay to extra complexity of sharing values, I think.

As explained, I did not implement your comments about the preference values.

Comment on attachment 680016 [details] [diff] [review] Part 1: Make PSM more amenable to storing concurrent private and non-private data. Review of attachment 680016 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +364,5 @@ > } > } > } > > + //nsNSSSocketInfo* socketInfo = ((nsNSSSocketInfo *) mInfoObject.get()); Remove comment. ::: security/manager/ssl/src/SharedSSLState.cpp @@ +110,5 @@ > +{ > + return gPrivateState; > +} > + > +} Add // namespace comments. ::: security/manager/ssl/src/nsClientAuthRemember.cpp @@ +86,5 @@ > > +void nsClientAuthRememberService::ClearAllRememberedDecisions() > +{ > + mozilla::psm::PublicSSLState()->GetClientAuthRememberService()->ClearRememberedDecisions(); > + mozilla::psm::PrivateSSLState()->GetClientAuthRememberService()->ClearRememberedDecisions(); Fix line length. ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +68,5 @@ > > NS_IMETHODIMP > +nsNSSCertificateDB::Init() > +{ > + nsCOMPtr<nsIObserverService> obsSvc = mozilla::services::GetObserverService(); This won't work if nsNSSCertificateDB is initialized off the main thread. I suggest that you document that GetRecentBadCertsService is main-thread-only (which does not cause any problems), and then just do this observer registration lazily in that method when you construct the first instance. Then you could just make nsRecentBadCertsService implement the observer itself. @@ +1777,5 @@ > + MOZ_ASSERT(NS_IsMainThread(), "RecentBadCerts should only be obtained on the main thread"); > + if (isPrivate) { > + if (!mPrivateRecentBadCerts) { > + mPrivateRecentBadCerts = new nsRecentBadCertsService; > + mPrivateRecentBadCerts->Init(); Since we don't check the return value of Init() we might as well just put that logic into the constructor. And, either way, we don't need to add init() to the XPCOM interface. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +901,5 @@ > > if (!wantRetry // no decision yet > && isTLSIntoleranceError(err, socketInfo->GetHasCleartextPhase())) > { > + wantRetry = socketInfo->SharedState().GetIOLayerHelpers()->rememberPossibleTLSProblemSite(socketInfo); Fix line length. @@ +930,5 @@ > if (!wantRetry // no decision yet > && !socketInfo->GetHasCleartextPhase()) // mirror PR_CONNECT_RESET_ERROR treament > { > wantRetry = > + socketInfo->SharedState().GetIOLayerHelpers()->rememberPossibleTLSProblemSite(socketInfo); Fix line length. @@ +1299,5 @@ > + setWarnLevelMissingRFC5746(warnLevel); > + > + mPrefObserver = new PrefObserver(this); > + Preferences::AddStrongObserver(mPrefObserver, "security.ssl.renego_unrestricted_hosts"); > + Preferences::AddStrongObserver(mPrefObserver, "security.ssl.treat_unsafe_negotiation_as_broken"); Fix line length / wrapping.

Josh, could you please (now and in the future) give your patches in the stack order numbers (like Part 1, Part 2)? It is impossible for me to recognize which patch depends on what and makes reviewing very hard. Thanks.

Comment on attachment 680016 [details] [diff] [review] Part 1: Make PSM more amenable to storing concurrent private and non-private data. Review of attachment 680016 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/public/nsIRecentBadCertsService.idl @@ +19,5 @@ > * including the bad cert. > * The implementation will decide how many entries it will hold, > * the number is expected to be small. > */ > [scriptable, uuid(a5ae8b05-a76e-408f-b0ba-02a831265749)] And IID change? ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +270,5 @@ > + * @param isPrivate True if the service for certs for private connections > + * is desired, false otherwise. > + * @return The requested service. > + */ > + nsIRecentBadCertsService getRecentBadCertsService(in boolean isPrivate); And IID change? ::: security/manager/ssl/src/SharedSSLState.cpp @@ +72,5 @@ > + mIOLayerHelpers.clearStoredData(); > +} > + > +void > +SharedSSLState::GlobalInit() Add "// static" comment before this method and other actually static methods. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +2515,5 @@ > nsresult rv; > PRStatus stat; > > + SharedSSLState* sharedState = > + transportFlags & nsISocketTransport::IS_PRIVATE ? PrivateSSLState() : PublicSSLState(); You could encapsulate this to some static function like GetSSLState(bool private)? Up to you to that.

Now with leaks eliminated and comments addressed. This is based off of the patch from bug 722979, so there are places that use the whole GetTransportFlags thing.

(In reply to Brian Smith (:bsmith) from comment #18) > Comment on attachment 679161 [details] [diff] [review] > Part 3: Clear SSL session cache upon leaving private browsing. > > @@ +2328,5 @@ > > + } > > + > > + { > > + nsNSSShutDownPreventionLock locker; > > + SSL_ClearSessionCache(); > > I need to investigate if there is a better way of doing this (having > separate private browsing and non-private-browsing caches). I'd rather not > reset the whole session cache if we can avoid it. Given that the existing behaviour is to clear the session cache, can we just file a bug to improve this later and achieve parity with the current implementation?

Note to self, update IID of nsIRecentBadCertsService in part 1.

Comment on attachment 679161 [details] [diff] [review] Part 3: Clear SSL session cache upon leaving private browsing. Review of attachment 679161 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +2328,5 @@ > + } > + > + { > + nsNSSShutDownPreventionLock locker; > + SSL_ClearSessionCache(); I filed bug 812598 to improve upon this. Please remove the nsNSSShutDownPreventionLock locker line and the nested scope.

Comment on attachment 681021 [details] [diff] [review] Part 1: Make PSM more amenable to storing concurrent private and non-private data. Review of attachment 681021 [details] [diff] [review]: ----------------------------------------------------------------- r+ with requested changes made. ::: docshell/base/nsDocShell.cpp @@ +4180,5 @@ > bool isStsHost = false; > + uint32_t flags = 0; > + if (sslSocketControl) { > + sslSocketControl->GetTransportFlags(&flags); > + } I think your original "am I in a private browsing window" implementation of this is better because it correctly handles the case !sslSocketControl for a private browsing window, whereas this doesn't handle that case correctly. ::: security/manager/ssl/public/nsIRecentBadCertsService.idl @@ +19,5 @@ > * including the bad cert. > * The implementation will decide how many entries it will hold, > * the number is expected to be small. > */ > [scriptable, uuid(a5ae8b05-a76e-408f-b0ba-02a831265749)] update UUID. @@ +20,5 @@ > * The implementation will decide how many entries it will hold, > * the number is expected to be small. > */ > [scriptable, uuid(a5ae8b05-a76e-408f-b0ba-02a831265749)] > interface nsIRecentBadCertsService : nsISupports { s/nsIRecentBadCertsService/nsIRecentBadCerts/ since this isn't (shouldn't be) an XPCOM service any more. ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +262,5 @@ > + * @param isPrivate True if the service for certs for private connections > + * is desired, false otherwise. > + * @return The requested service. > + */ > + nsIRecentBadCertsService getRecentBadCertsService(in boolean isPrivate); s/getRecentBadCertsService/getRecentBadCerts/ ::: security/manager/ssl/src/SharedSSLState.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- Include VIM line too: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line ::: security/manager/ssl/src/SharedSSLState.h @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- VIM. @@ +29,5 @@ > + nsClientAuthRememberService* GetClientAuthRememberService() { > + return mClientAuthRemember; > + } > + > + nsSSLIOLayerHelpers* GetIOLayerHelpers() { return nsSSLIOLayerHelpers& so we don't have to wonder "what if it returns NULL" when reading callers. ::: security/manager/ssl/src/nsClientAuthRemember.cpp @@ +87,5 @@ > } > > +void nsClientAuthRememberService::ClearAllRememberedDecisions() > +{ > + nsRefPtr<nsClientAuthRememberService> svc = Use RefPtr in PSM. ::: security/manager/ssl/src/nsNSSCertificateDB.h @@ +6,5 @@ > #define __NSNSSCERTIFICATEDB_H__ > > #include "nsIX509CertDB.h" > #include "nsIX509CertDB2.h" > +#include "nsAutoPtr.h" mozilla/RefPtr.h @@ +51,5 @@ > nsresult handleCACertDownload(nsIArray *x509Certs, > nsIInterfaceRequestor *ctx); > + > + nsRefPtr<nsRecentBadCertsService> mPublicRecentBadCerts; > + nsRefPtr<nsRecentBadCertsService> mPrivateRecentBadCerts; RefPtr ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1208,5 @@ > +PrefObserver::Observe(nsISupports *aSubject, const char *aTopic, > + const PRUnichar *someData) > +{ > + if (nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0) { > + nsNSSShutDownPreventionLock locker; You do not need nsNSSShutDownPreventionLock here. (nsNSSShutDownPreventionLock is needed for background threads, and it should always be immediately followed by a call to nsNSSShutDownObject::isAlreadyShutDown().) @@ +2485,5 @@ > if (SECSuccess != SSL_OptionSet(fd, SSL_HANDSHAKE_AS_CLIENT, true)) { > return NS_ERROR_FAILURE; > } > > + if (infoObject->SharedState().GetIOLayerHelpers()->isRenegoUnrestrictedSite(nsDependentCString(host))) { Make line length <= ~80 characters. ::: security/manager/ssl/src/nsNSSIOLayer.h @@ +93,5 @@ > PRFileDesc* mFd; > > CertVerificationState mCertVerificationState; > > + mozilla::psm::SharedSSLState* mSharedState; const mozilla::psm::SharedSSLState& mSharedState; ::: security/manager/ssl/src/nsRecentBadCerts.cpp @@ +25,5 @@ > using namespace mozilla; > > NSSCleanupAutoPtrClass(CERTCertificate, CERT_DestroyCertificate) > > +NS_IMPL_THREADSAFE_ISUPPORTS2(nsRecentBadCertsService, trailing whitespace.

Comment on attachment 679161 [details] [diff] [review] Part 3: Clear SSL session cache upon leaving private browsing. Review of attachment 679161 [details] [diff] [review]: ----------------------------------------------------------------- why does this need super review?

It is normal policy in security/ to require r+ and sr+ if the patch is not written by a peer. It is OK to do Honza's SR after checkin, if Honza can't get to it before.

This is the actual patch rebased on the landed STS changes.

>I think your original "am I in a private browsing window" implementation of this is better >because it correctly handles the case !sslSocketControl for a private browsing window, whereas >this doesn't handle that case correctly. Hmm, the bit you refer to here is actually from bug 722979; this latest patch doesn't include it. I believe I described why I felt I needed to use nsISSLSocketControl in 722979, since my original method ran afoul of the need to recreate TransportSecurityInfo without the required information available, as you previously described.

Comment on attachment 682749 [details] [diff] [review] Part 1: Make PSM more amenable to storing concurrent private and non-private data. Restoring canceled sr.

Comment on attachment 682831 [details] [diff] [review] Remove global private browsing NSS cleanup on exit. Review of attachment 682831 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js @@ +442,5 @@ > + if (aData == "enter") { > + // clear all auth tokens > + let sdr = Cc["@mozilla.org/security/sdr;1"]. > + getService(Ci.nsISecretDecoderRing); > + sdr.logoutAndTeardown(); Why do we have to do this on entering private browsing mode? I am not saying you're wrong, but it seems strange to need to do anything here, based on my understanding of how private browsing mode is supposed to work. Also, trailing whitespace.

jdm, Please see the attached patch (untested because I couldn't get it to build due to missing one of your patches, but I'm not sure which one I was missing). A patch like this is needed to avoid regressing the privacy characteristics of private browsing w.r.t. SSL sessions. Also, I think we failed to consider some race conditions. The "exiting private browsing notification" happens on the main thread, concurrent with cert verifications and (SSL) networking activity. So, it is possible that we received the "exiting private browsing" notification and clear the recent bad cert cache, and then a SSL socket adds a new recent bad cert on the socket transport thread, and then we close all the private SSL sockets. This will leave a recent bad cert in the private browsing recent bad cert cache. Similarly, on the main thread, we may receive the "exiting private browsing" notification and clear the SSL session cache, but then later on the socket transport thread a new private SSL session cache entry is created, before we close all the private connections. I suspect similar races are possible with respect to clearing SharedSSLState. It seems like we must always first close all the private SSL connections, and then attempt to clear any of the caches (SharedSSLState, recent bad certs, etc.), and then return from Observe(). In order to ensure this ordering, we should only have one observer, I think. Thoughts?

(In reply to Brian Smith (:bsmith) from comment #38) > Comment on attachment 682831 [details] [diff] [review] > Remove global private browsing NSS cleanup on exit. > > Review of attachment 682831 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js > @@ +442,5 @@ > > + if (aData == "enter") { > > + // clear all auth tokens > > + let sdr = Cc["@mozilla.org/security/sdr;1"]. > > + getService(Ci.nsISecretDecoderRing); > > + sdr.logoutAndTeardown(); > > Why do we have to do this on entering private browsing mode? I am not saying > you're wrong, but it seems strange to need to do anything here, based on my > understanding of how private browsing mode is supposed to work. > > Also, trailing whitespace. I made this change to retain behavioural compatibility with the old global PB mode (this code won't run when per-window mode is enabled). I don't have any answers for why it was originally chosen to run on both entrance and exit.

Comment on attachment 682850 [details] [diff] [review] Use a separate SSL session cache for private browsing Review of attachment 682850 [details] [diff] [review]: ----------------------------------------------------------------- I like how simple that is; after reading http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslfnc.html#1124562 I presume that it doesn't actually resolve the problem of clearing them separately, though.

>Also, I think we failed to consider some race conditions. The "exiting private browsing >notification" happens on the main thread, concurrent with cert verifications and (SSL) >networking activity. So, it is possible that we received the "exiting private browsing" >notification and clear the recent bad cert cache, and then a SSL socket adds a new recent bad >cert on the socket transport thread, and then we close all the private SSL sockets. This will >leave a recent bad cert in the private browsing recent bad cert cache. Similarly, on the main >thread, we may receive the "exiting private browsing" notification and clear the SSL session >cache, but then later on the socket transport thread a new private SSL session cache entry is >created, before we close all the private connections. I suspect similar races are possible >with respect to clearing SharedSSLState. > >It seems like we must always first close all the private SSL connections, and then attempt to >clear any of the caches (SharedSSLState, recent bad certs, etc.), and then return from >Observe(). In order to ensure this ordering, we should only have one observer, I think. >Thoughts? These ideas make sense. I might be inclined to have the separate observers, but give them their own topic like last-private-connection-terminated which could be invoked by the only NSS observer of last-pb-context-exited.

If you can give pointers for how to close the private SSL connections, I'll write a patch to implement comment 42.

(In reply to Josh Matthews [:jdm] from comment #42) > These ideas make sense. I might be inclined to have the separate observers, > but give them their own topic like last-private-connection-terminated which > could be invoked by the only NSS observer of last-pb-context-exited. I think if we were to do that, we would have a different race between last-pb-context-exited and creating the next private browsing context, which would also be problematic, right? I think it is important to make the closing of the private SSL connections and clearing the private cached/shared SSL state atomic at least as far as the main thread is concerned. (Even then, we are on thin ice because in the future we're planning to create network connections off the main thread, which means we'd need even stronger synchronization.) (In reply to Josh Matthews [:jdm] from comment #43) > If you can give pointers for how to close the private SSL connections, I'll > write a patch to implement comment 42. Isn't this something that your patchset for per-window private browsing is already doing? I had thought that the global private browsing code was closing all network connections. If I open a private browsing window, and then close it, and then open a new private browsing window, should that second private browsing window be sharing state with the original private browsing window? My understanding, based on all your patches that try to clean up state on closing the last private browsing window, is that such sharing is not allowed. That is the primary reason we must close all private browsing connections (and clear private browsing DNS cache entries), independent of these PSM-specific reasons. Anyway, looking at nsSocketTransportService2.cpp, I don't see any logic for closing private browsing connections, so I guess it isn't implemented yet. I think we basically need to dispatch a runnable to the socket transport service thread that calls a variant of nsSocketTransportService::Reset() that resets just the private connections, and then afterwards clears the private shared SSL state, and then sets a flag that tells us that it is finished. Then, on the main thread, we have to wait for that flag to be set in the Observe(), without blocking the main thread event queue.

(In reply to Josh Matthews [:jdm] from comment #40) > > Why do we have to do this on entering private browsing mode? I am not saying > > you're wrong, but it seems strange to need to do anything here, based on my > > understanding of how private browsing mode is supposed to work. > > I made this change to retain behavioural compatibility with the old global > PB mode (this code won't run when per-window mode is enabled). I don't have > any answers for why it was originally chosen to run on both entrance and > exit. We should not be logging out of the SDR in response to private browsing events (entering or exiting) at all, because this means that you're logging people out of SDR in their non-private windows too. AFAICT, that means that if you use a master password, and you sign in with your master password to get password manager to work, and then you create a private browsing window, then you will need to sign in with your master password *again*, even in your non-private window. That's not right (even for the private window).

(In reply to Josh Matthews [:jdm] from comment #41) > I like how simple that is; after reading > http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslfnc.html#1124562 > I presume that it doesn't actually resolve the problem of clearing them > separately, though. You are right, it doesn't solve the problem of clearing them separately, which is why the patch still calls SSL_ClearSessionCache. I already filed follow-up bug 812598 for improving that. I am going to be flying all evening without internet connectivity. Please test that my patch builds when applied on top of your patches and r?honzab if so, so we have a chance of landing it tomorrow.

The mobile people have decided to delay the feature until FF 20, so the time pressure no longer applies. I'm going to take my time sorting out what needs to be done here.

Comment on attachment 682831 [details] [diff] [review] Remove global private browsing NSS cleanup on exit. Brian's right; theoretically the other patches in this bug get us to the point where this should not be necessary for the current global mode as well as the per-window mode.

(In reply to comment #48) > Comment on attachment 682831 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=682831 > Remove global private browsing NSS cleanup on exit. > > Brian's right; theoretically the other patches in this bug get us to the point > where this should not be necessary for the current global mode as well as the > per-window mode. Yeah. In fact I think we should remove the code in question.

Comment on attachment 682749 [details] [diff] [review] Part 1: Make PSM more amenable to storing concurrent private and non-private data. Review of attachment 682749 [details] [diff] [review]: ----------------------------------------------------------------- sr=honzab, however I just overlooked the patch... ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ +1699,5 @@ > + NS_ADDREF(*result = mPrivateRecentBadCerts); > + } else { > + if (!mPublicRecentBadCerts) { > + mPublicRecentBadCerts = new nsRecentBadCertsService; > + mPublicRecentBadCerts->InitPrivateBrowsingObserver(); Shouldn't InitPrivateBrowsingObserver(); be rather called on the private set of bad certs? ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1209,5 @@ > + const PRUnichar *someData) > +{ > + if (nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0) { > + nsNSSShutDownPreventionLock locker; > + NS_ConvertUTF16toUTF8 prefName(someData); NS_ConvertUTF16toUTF8 prefName: two spaces between ::: security/manager/ssl/src/nsRecentBadCerts.cpp @@ +174,5 @@ > +{ > + for (size_t i=0; i<const_recently_seen_list_size; ++i) { > + RecentBadCert &entry = mCerts[i]; > + entry.Clear(); > + } *** TODO check

Comment on attachment 679161 [details] [diff] [review] Part 3: Clear SSL session cache upon leaving private browsing. Review of attachment 679161 [details] [diff] [review]: ----------------------------------------------------------------- To make PB more immediately work, this is critical to have, even we delete the whole SSL cache. Depends on bug 812598 whether it can be done soon, otherwise, I'm not against this change. sr=honzab

>::: security/manager/ssl/src/nsRecentBadCerts.cpp >@@ +174,5 @@ >> +{ >> + for (size_t i=0; i<const_recently_seen_list_size; ++i) { >> + RecentBadCert &entry = mCerts[i]; >> + entry.Clear(); >> + } > >*** TODO check I don't understand what this comment means.

(In reply to Josh Matthews [:jdm] from comment #52) > I don't understand what this comment means. Just internal note. I forgot to check you are correctly deleting the table. You do, so just ignore it.

(In reply to Josh Matthews [:jdm] from comment #52) > >::: security/manager/ssl/src/nsRecentBadCerts.cpp > >@@ +174,5 @@ > >> +{ > >> + for (size_t i=0; i<const_recently_seen_list_size; ++i) { Just please fix space like: for (size_t i = 0; i < const_recently_seen_list_size; ++i) {

Comment on attachment 684509 [details] [diff] [review] Remove global private browsing NSS cleanup on exit. Review of attachment 684509 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Brian, I didn't read comment 44 closely until after I wrote this code. Is it necessary to perform the connection teardown from nsSocketTransportService? As far as I can see, the information required is readily available in the HTTP connection manager, and I ended up just blocking via a condvar so that the whole operation is atomic from the point of view of the last-pb-context-exited notification and NSS cleanup.

Comment on attachment 684527 [details] [diff] [review] Close all private HTTP connections, and trigger NSS PB-related cleanup only once that operation completes. ># HG changeset patch ># User Josh Matthews <josh@joshmatthews.net> ># Date 1353624132 18000 ># Node ID 6239a7b342c71d66cd9c553e3d24afd2e2b6ea88 ># Parent 7e963af339bf3123fa47dbc16741faee7c6bd2b5 >Bug 769288 - Close all private HTTP connections, and trigger NSS PB-related cleanup only once that operation completes. r=mcmanus,bsmith > >diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp >--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp >+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp >@@ -57,16 +57,19 @@ nsHttpConnectionMgr::nsHttpConnectionMgr > , mMaxConns(0) > , mMaxPersistConnsPerHost(0) > , mMaxPersistConnsPerProxy(0) > , mIsShuttingDown(false) > , mNumActiveConns(0) > , mNumIdleConns(0) > , mTimeOfNextWakeUp(UINT64_MAX) > , mTimeoutTickArmed(false) >+ , mPrunePrivateLock("nsHttpConnectionMgr.mPrunePrivateLock") >+ , mPrunePrivateCondVar(mPrunePrivateLock, "nsHttpConnectionMgr.mPrunePrivateCondVar") >+ , mPrivateConnsPruned(false) > { > LOG(("Creating nsHttpConnectionMgr @%x\n", this)); > mCT.Init(); > mAlternateProtocolHash.Init(16); > mSpdyPreferredHash.Init(); > } > > nsHttpConnectionMgr::~nsHttpConnectionMgr() >@@ -114,16 +117,22 @@ nsHttpConnectionMgr::Init(uint16_t maxCo > mMaxPersistConnsPerProxy = maxPersistConnsPerProxy; > mMaxRequestDelay = maxRequestDelay; > mMaxPipelinedRequests = maxPipelinedRequests; > mMaxOptimisticPipelinedRequests = maxOptimisticPipelinedRequests; > > mIsShuttingDown = false; > } > >+ nsCOMPtr<nsIObserverService> observerService( >+ do_GetService("@mozilla.org/observer-service;1")); >+ if (observerService) { >+ observerService->AddObserver(this, "last-pb-context-exited", false); >+ } >+ > return EnsureSocketThreadTarget(); > } > > nsresult > nsHttpConnectionMgr::Shutdown() > { > LOG(("nsHttpConnectionMgr::Shutdown\n")); > >@@ -145,16 +154,22 @@ nsHttpConnectionMgr::Shutdown() > mSocketThreadTarget = 0; > > if (NS_FAILED(rv)) { > NS_WARNING("unable to post SHUTDOWN message"); > return rv; > } > } > >+ nsCOMPtr<nsIObserverService> observerService( >+ do_GetService("@mozilla.org/observer-service;1")); >+ if (observerService) { >+ observerService->RemoveObserver(this, "last-pb-context-exited"); >+ } >+ > // wait for shutdown event to complete > while (!shutdown) > NS_ProcessNextEvent(NS_GetCurrentThread()); > > return NS_OK; > } > > nsresult >@@ -249,16 +264,24 @@ nsHttpConnectionMgr::Observe(nsISupports > else if (timer == mTimeoutTick) { > TimeoutTick(); > } > else { > NS_ABORT_IF_FALSE(false, "unexpected timer-callback"); > LOG(("Unexpected timer object\n")); > return NS_ERROR_UNEXPECTED; > } >+ } else if (0 == strcmp(topic, "last-pb-context-exited")) { >+ PrunePrivateConnections(); >+ >+ nsCOMPtr<nsIObserverService> observerService( >+ do_GetService("@mozilla.org/observer-service;1")); >+ observerService->NotifyObservers(nullptr, >+ "last-private-connection-terminated", >+ nullptr); > } > > return NS_OK; > } > > > //----------------------------------------------------------------------------- > >@@ -295,16 +318,30 @@ nsHttpConnectionMgr::CancelTransaction(n > nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgCancelTransaction, > static_cast<int32_t>(reason), trans); > if (NS_FAILED(rv)) > NS_RELEASE(trans); > return rv; > } > > nsresult >+nsHttpConnectionMgr::PrunePrivateConnections() >+{ >+ nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgPrunePrivateConnections); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ mPrivateConnsPruned = false; >+ MutexAutoLock lock(mPrunePrivateLock); >+ while (!mPrivateConnsPruned) { >+ mPrunePrivateCondVar.Wait(); >+ } >+ return NS_OK; >+} >+ >+nsresult > nsHttpConnectionMgr::PruneDeadConnections() > { > return PostEvent(&nsHttpConnectionMgr::OnMsgPruneDeadConnections); > } > > nsresult > nsHttpConnectionMgr::ClosePersistentConnections() > { >@@ -751,16 +788,43 @@ nsHttpConnectionMgr::PurgeExcessIdleConn > NS_RELEASE(conn); > self->mNumIdleConns--; > self->ConditionallyStopPruneDeadConnectionsTimer(); > } > return PL_DHASH_STOP; > } > > PLDHashOperator >+nsHttpConnectionMgr::PrunePrivateConnectionsCB(const nsACString &key, >+ nsAutoPtr<nsConnectionEntry> &ent, >+ void *closure) >+{ >+ if (!ent->mConnInfo->GetPrivate()) { >+ return PL_DHASH_NEXT; >+ } >+ for (uint32_t index = 0; index < ent->mPendingQ.Length(); ++index) { >+ nsHttpTransaction *trans = ent->mPendingQ[index]; >+ trans->Close(NS_ERROR_FAILURE); >+ } >+ for (uint32_t index = 0; index < ent->mActiveConns.Length(); ++index) { >+ nsHttpConnection *conn = ent->mActiveConns[index]; >+ conn->Close(NS_ERROR_FAILURE); >+ } >+ for (uint32_t index = 0; index < ent->mIdleConns.Length(); ++index) { >+ nsHttpConnection *conn = ent->mIdleConns[index]; >+ conn->Close(NS_ERROR_FAILURE); >+ } >+ for (uint32_t index = 0; index < ent->mHalfOpens.Length(); ++index) { >+ nsHalfOpenSocket *sock = ent->mHalfOpens[index]; >+ sock->Abandon(); >+ } >+ return PL_DHASH_NEXT; >+} >+ >+PLDHashOperator > nsHttpConnectionMgr::PruneDeadConnectionsCB(const nsACString &key, > nsAutoPtr<nsConnectionEntry> &ent, > void *closure) > { > nsHttpConnectionMgr *self = (nsHttpConnectionMgr *) closure; > > LOG((" pruning [ci=%s]\n", ent->mConnInfo->HashKey().get())); > >@@ -2005,16 +2069,28 @@ nsHttpConnectionMgr::OnMsgProcessPending > // for the specified connection info. walk the connection table... > mCT.Enumerate(ProcessOneTransactionCB, this); > } > > NS_RELEASE(ci); > } > > void >+nsHttpConnectionMgr::OnMsgPrunePrivateConnections(int32_t, void *) >+{ >+ NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread"); >+ LOG(("nsHttpConnectionMgr::OnMsgPrunePrivateConnections\n")); >+ mCT.Enumerate(PrunePrivateConnectionsCB, this); >+ >+ MutexAutoLock lock(mPrunePrivateLock); >+ mPrivateConnsPruned = true; >+ mPrunePrivateCondVar.Notify(); >+} >+ >+void > nsHttpConnectionMgr::OnMsgPruneDeadConnections(int32_t, void *) > { > NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread"); > LOG(("nsHttpConnectionMgr::OnMsgPruneDeadConnections\n")); > > // Reset mTimeOfNextWakeUp so that we can find a new shortest value. > mTimeOfNextWakeUp = UINT64_MAX; > >diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.h b/netwerk/protocol/http/nsHttpConnectionMgr.h >--- a/netwerk/protocol/http/nsHttpConnectionMgr.h >+++ b/netwerk/protocol/http/nsHttpConnectionMgr.h >@@ -27,16 +27,18 @@ > class nsHttpPipeline; > > class nsIHttpUpgradeListener; > > //----------------------------------------------------------------------------- > > class nsHttpConnectionMgr : public nsIObserver > { >+ typedef mozilla::Mutex Mutex; >+ typedef mozilla::CondVar CondVar; > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIOBSERVER > > // parameter names > enum nsParamName { > MAX_CONNECTIONS, > MAX_PERSISTENT_CONNECTIONS_PER_HOST, >@@ -81,16 +83,20 @@ public: > > // called to reschedule the given transaction. it must already have been > // added to the connection manager via AddTransaction. > nsresult RescheduleTransaction(nsHttpTransaction *, int32_t priority); > > // cancels a transaction w/ the given reason. > nsresult CancelTransaction(nsHttpTransaction *, nsresult reason); > >+ // called to force the connection manager to prune its list of private >+ // connections. blocks until the operation is complete. >+ nsresult PrunePrivateConnections(); >+ > // called to force the connection manager to prune its list of idle > // connections. > nsresult PruneDeadConnections(); > > // Close all idle persistent connections and prevent any active connections > // from being reused. > nsresult ClosePersistentConnections(); > >@@ -449,16 +455,17 @@ private: > bool mIsShuttingDown; > > //------------------------------------------------------------------------- > // NOTE: these members are only accessed on the socket transport thread > //------------------------------------------------------------------------- > > static PLDHashOperator ProcessOneTransactionCB(const nsACString &, nsAutoPtr<nsConnectionEntry> &, void *); > >+ static PLDHashOperator PrunePrivateConnectionsCB(const nsACString &, nsAutoPtr<nsConnectionEntry> &, void *); > static PLDHashOperator PruneDeadConnectionsCB(const nsACString &, nsAutoPtr<nsConnectionEntry> &, void *); > static PLDHashOperator ShutdownPassCB(const nsACString &, nsAutoPtr<nsConnectionEntry> &, void *); > static PLDHashOperator PurgeExcessIdleConnectionsCB(const nsACString &, nsAutoPtr<nsConnectionEntry> &, void *); > static PLDHashOperator ClosePersistentConnectionsCB(const nsACString &, nsAutoPtr<nsConnectionEntry> &, void *); > bool ProcessPendingQForEntry(nsConnectionEntry *); > bool IsUnderPressure(nsConnectionEntry *ent, > nsHttpTransaction::Classifier classification); > bool AtActiveConnectionLimit(nsConnectionEntry *, uint8_t caps); >@@ -560,16 +567,17 @@ private: > > // message handlers > void OnMsgShutdown (int32_t, void *); > void OnMsgShutdownConfirm (int32_t, void *); > void OnMsgNewTransaction (int32_t, void *); > void OnMsgReschedTransaction (int32_t, void *); > void OnMsgCancelTransaction (int32_t, void *); > void OnMsgProcessPendingQ (int32_t, void *); >+ void OnMsgPrunePrivateConnections (int32_t, void *); > void OnMsgPruneDeadConnections (int32_t, void *); > void OnMsgSpeculativeConnect (int32_t, void *); > void OnMsgReclaimConnection (int32_t, void *); > void OnMsgCompleteUpgrade (int32_t, void *); > void OnMsgUpdateParam (int32_t, void *); > void OnMsgClosePersistentConnections (int32_t, void *); > void OnMsgProcessFeedback (int32_t, void *); > >@@ -612,11 +620,15 @@ private: > void *closure); > > // For diagnostics > void OnMsgPrintDiagnostics(int32_t, void *); > static PLDHashOperator PrintDiagnosticsCB(const nsACString &key, > nsAutoPtr<nsConnectionEntry> &ent, > void *closure); > nsCString mLogData; >+ >+ Mutex mPrunePrivateLock; >+ CondVar mPrunePrivateCondVar; >+ bool mPrivateConnsPruned; > }; > > #endif // !nsHttpConnectionMgr_h__ >diff --git a/security/manager/ssl/src/nsCertOverrideService.cpp b/security/manager/ssl/src/nsCertOverrideService.cpp >--- a/security/manager/ssl/src/nsCertOverrideService.cpp >+++ b/security/manager/ssl/src/nsCertOverrideService.cpp >@@ -120,17 +120,17 @@ nsCertOverrideService::Init() > mozilla::services::GetObserverService(); > > // If we cannot add ourselves as a profile change observer, then we will not > // 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); >- observerService->AddObserver(this, "last-pb-context-exited", true); >+ observerService->AddObserver(this, "last-private-connection-terminated", true); > // simulate a profile change so we read the current profile's settings file > Observe(nullptr, "profile-do-change", nullptr); > } > > return NS_OK; > } > > NS_IMETHODIMP >@@ -165,17 +165,17 @@ nsCertOverrideService::Observe(nsISuppor > nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mSettingsFile)); > if (NS_SUCCEEDED(rv)) { > mSettingsFile->AppendNative(NS_LITERAL_CSTRING(kCertOverrideFileName)); > } else { > mSettingsFile = nullptr; > } > Read(); > >- } else if (!nsCRT::strcmp(aTopic, "last-pb-context-exited")) { >+ } else if (!nsCRT::strcmp(aTopic, "last-private-connection-terminated")) { > ClearValidityOverride( > NS_LITERAL_CSTRING("all:temporary-certificates"), > 0); > } > > return NS_OK; > } > >diff --git a/security/manager/ssl/src/nsNSSComponent.cpp b/security/manager/ssl/src/nsNSSComponent.cpp >--- a/security/manager/ssl/src/nsNSSComponent.cpp >+++ b/security/manager/ssl/src/nsNSSComponent.cpp >@@ -2130,17 +2130,17 @@ nsNSSComponent::RandomUpdate(void *entro > > #define PROFILE_CHANGE_NET_TEARDOWN_TOPIC "profile-change-net-teardown" > #define PROFILE_CHANGE_NET_RESTORE_TOPIC "profile-change-net-restore" > #define PROFILE_APPROVE_CHANGE_TOPIC "profile-approve-change" > #define PROFILE_CHANGE_TEARDOWN_TOPIC "profile-change-teardown" > #define PROFILE_CHANGE_TEARDOWN_VETO_TOPIC "profile-change-teardown-veto" > #define PROFILE_BEFORE_CHANGE_TOPIC "profile-before-change" > #define PROFILE_DO_CHANGE_TOPIC "profile-do-change" >-#define LEAVE_PRIVATE_BROWSING_TOPIC "last-pb-context-exited" >+#define LEAVE_PRIVATE_BROWSING_TOPIC "last-private-connection-terminated" > > NS_IMETHODIMP > nsNSSComponent::Observe(nsISupports *aSubject, const char *aTopic, > const PRUnichar *someData) > { > if (nsCRT::strcmp(aTopic, PROFILE_APPROVE_CHANGE_TOPIC) == 0) { > DoProfileApproveChange(aSubject); > } >diff --git a/security/manager/ssl/src/nsRecentBadCerts.cpp b/security/manager/ssl/src/nsRecentBadCerts.cpp >--- a/security/manager/ssl/src/nsRecentBadCerts.cpp >+++ b/security/manager/ssl/src/nsRecentBadCerts.cpp >@@ -39,25 +39,25 @@ nsRecentBadCerts::nsRecentBadCerts() > nsRecentBadCerts::~nsRecentBadCerts() > { > } > > void > nsRecentBadCerts::InitPrivateBrowsingObserver() > { > nsCOMPtr<nsIObserverService> obsSvc = mozilla::services::GetObserverService(); >- obsSvc->AddObserver(this, "last-pb-context-exited", false); >+ obsSvc->AddObserver(this, "last-private-connection-terminated", false); > } > > NS_IMETHODIMP > nsRecentBadCerts::Observe(nsISupports *aSubject, > const char *aTopic, > const PRUnichar *aData) > { >- if (!nsCRT::strcmp(aTopic, "last-pb-context-exited")) { >+ if (!nsCRT::strcmp(aTopic, "last-private-connection-terminated")) { > ResetStoredCerts(); > } > return NS_OK; > } > > NS_IMETHODIMP > nsRecentBadCerts::GetRecentBadCert(const nsAString & aHostNameWithPort, > nsISSLStatus **aStatus)

Bugzilla totally did the opposite thing from what I expected there. Here is my comment: (In reply to Josh Matthews [:jdm] from comment #57) > As far as I can see, the information required is readily available in the > HTTP connection manager. This is only true for HTTP connections. AFAICT, it wouldn't work for the raw socket web API, for example. > I ended up just blocking via a condvar so that > the whole operation is atomic from the point of view of the > last-pb-context-exited notification and NSS cleanup. The main thread cannot block while it waits on a condvar from the socket transport thread, because sometimes the socket transport thread blocks waiting for the main thread (e.g. whenever SyncRunnableBase is used). It is important that we make the *entire* shared private browsing SSL state (if not all shared private browsing networking state) atomic from the point of view of the socket transport thread. That means that, for example, the call to SSL_ClearSesionCache() must be within the same atomic unit as the closing of the connections, so that we do not open a new connection using an old SSL session between the closing of the old private browsing connections and the clearing of the session cache.

I believe this patch fulfills the requirements. The main thread now waits on the main thread to signal that all cleanup is complete before any further socket events are processed.

Comment on attachment 686252 [details] [diff] [review] Close private socket connections when the lsat private browsing instance dies. Review of attachment 686252 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/nsSocketTransport2.cpp @@ +1818,5 @@ > NS_IF_ADDREF(*callbacks = mCallbacks); > return NS_OK; > } > > +class UpdatePrivacyStatus : public nsRunnable Based on your previous patches, when we create a socket transport we pass the NO_PERMANENT_STORAGE (which should be renamed back to IS_PRIVATE) flag during the creation. So, why do we need to do this? ::: netwerk/base/src/nsSocketTransportService2.cpp @@ +934,5 @@ > + // Signal the main thread to perform any remaining cleanup related to the > + // now-closed connections. We must avoid processing any further events > + // until this cleanup is finished. > + nsCOMPtr<nsIRunnable> ev = > + NS_NewRunnableMethod(this, Why do we need to trigger the cleanup of the private browsing state on the main thread? Why can't we just do it directly here on the socket transport thread? If we cleared all the networking-related state directly here, below we wouldn't have to wait for the main thread.

The SharedSSLState changes are mostly to facilitate including the header outside of security/.

Ehsan requested a combined version of all the patches for his own inscrutable purposes.

Comment on attachment 689537 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. Review of attachment 689537 [details] [diff] [review]: ----------------------------------------------------------------- In general these changes look very good to me. It would be good to get mcmanus's review of the nsSocketTransport* changes too. I will do a quick review of the updated patch. ::: netwerk/base/src/nsSocketTransportService2.cpp @@ +888,5 @@ > } > + > + if (!strcmp(topic, "last-pb-context-exited")) { > + nsCOMPtr<nsIRunnable> ev = > + NS_NewRunnableMethod(this, &nsSocketTransportService::ClosePrivateConnections); Line length? @@ +890,5 @@ > + if (!strcmp(topic, "last-pb-context-exited")) { > + nsCOMPtr<nsIRunnable> ev = > + NS_NewRunnableMethod(this, &nsSocketTransportService::ClosePrivateConnections); > + nsresult rv = Dispatch(ev, nsIEventTarget::DISPATCH_NORMAL); > + NS_ENSURE_SUCCESS(rv, rv); What happens at this point if we create a new private browsing context (on the main thread) while the socket transport service is cleaning up the private browsing connection state on the socket transport thread? We need to check with mcmanus and/or mayhemer to make sure we can we rely on the fact that any new private connections will be queued up after the runnable that clears the shared state. If we can't rely on that, then I guess we'd need to block the creation of new private browsing windows until the the ClosePrivateConnections operation is complete. @@ +910,5 @@ > + DetachSocket(mIdleList, &mIdleList[i]); > + } > + } > + > + mozilla::ClearPrivateSSLState(); Do we also need to remove the private DNS entries from the DNS cache? And, at what point do we remove private entries from the HTTP response cache and the HTTP auth caches? In general, we should make sure we're clearing everything we need to clear, either here or on the main thread, as needed. ::: netwerk/base/src/nsSocketTransportService2.h @@ +18,5 @@ > #include "prio.h" > #include "nsASocketHandler.h" > #include "nsIObserver.h" > #include "mozilla/Mutex.h" > +#include "mozilla/CondVar.h" Remove unnecessary include. @@ +185,5 @@ > #endif > bool mProbedMaxCount; > + > + void ClosePrivateConnections(); > + void NotifyLastPrivateConnectionClosed(); Remove declaration of NotifyLastPrivateConnectionClosed which isn't defined anymore. ::: security/manager/ssl/src/Makefile.in @@ +95,5 @@ > EXPORTS += \ > CryptoTask.h \ > nsNSSShutDown.h \ > ScopedNSSTypes.h \ > + SharedSSLState.h \ Let's not export SharedSSLState.h since we don't need to share the SharedSSLState class outside the module. Instead, let's just create a new file mozilla/SSL.h which declares the ClearPrivateSSLState() function. Then you can revert most of your changes to ShareSSLState. If you're feeling generous, you can move this chunk of nsSocketTransportService2.cpp to SSL.h too (removing the comment): // XXX: There is no good header file to put these in. :( namespace mozilla { namespace psm { void InitializeSSLServerCertVerificationThreads(); void StopSSLServerCertVerificationThreads(); } } // namespace mozilla::psm Otherwise, I will do that in a follow-up. ::: security/manager/ssl/src/SharedSSLState.cpp @@ +20,3 @@ > > namespace mozilla { > + namespace { @@ +31,5 @@ > + 0); > + } > + } > +}; > + } // unnamed namespace ::: security/manager/ssl/src/nsNSSCertificateDB.cpp @@ -1649,5 @@ > > NS_IMETHODIMP > nsNSSCertificateDB::GetRecentBadCerts(bool isPrivate, nsIRecentBadCerts** result) > { > - MOZ_ASSERT(NS_IsMainThread(), "RecentBadCerts should only be obtained on the main thread"); Now we need a mutex to protect mPrivateRecentBadCerts and mPublicRecentBadCerts, since this isn't single-threaded.

Comment on attachment 689537 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. Review of attachment 689537 [details] [diff] [review]: ----------------------------------------------------------------- I only reviewed the things in netwerk/base and pretty much assumed brian's comments were addressed as precondition to r+ ::: netwerk/base/src/nsSocketTransport2.cpp @@ +32,5 @@ > #include "nsINSSErrorsService.h" > #include "nsIPipe.h" > #include "nsIProgrammingLanguage.h" > #include "nsIClassInfoImpl.h" > +#include "nsIInterfaceRequestorUtils.h" I'm guessing you don't still need this. ::: netwerk/base/src/nsSocketTransportService2.cpp @@ +889,5 @@ > + > + if (!strcmp(topic, "last-pb-context-exited")) { > + nsCOMPtr<nsIRunnable> ev = > + NS_NewRunnableMethod(this, &nsSocketTransportService::ClosePrivateConnections); > + nsresult rv = Dispatch(ev, nsIEventTarget::DISPATCH_NORMAL); I'd love a comment here that this is running ClosePrivateConnections on the socket thread. no need to make that harder to figure out than necessary. @@ +890,5 @@ > + if (!strcmp(topic, "last-pb-context-exited")) { > + nsCOMPtr<nsIRunnable> ev = > + NS_NewRunnableMethod(this, &nsSocketTransportService::ClosePrivateConnections); > + nsresult rv = Dispatch(ev, nsIEventTarget::DISPATCH_NORMAL); > + NS_ENSURE_SUCCESS(rv, rv); I think this is ok but I don't know all the ins and outs of PB.. this is what I think happens 0] the last PB tab gets closed 1] the main thread generates last-pb-context-exited and this observer code runs synchronously with it. and posts an event ("A") to the socket thread 2] a new PB tab is created.. which creates an http channel on the main thread 3] that channel posts a event "B" to the connection manager to start a new private connection 4] "A" is processed on the socket thread clean up all private connections 5] "B" is processed on the socket thread making a new one A and B can't be transposed in any way I can think of. The event queue is strictly ordered and you can't touch connections from off of the socket thread which is why http channel has to post the make-a-connection message.

The DNS cache is a good point. I'll look into it in another bug.

Comment on attachment 689955 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. Review of attachment 689955 [details] [diff] [review]: ----------------------------------------------------------------- r+ based on mcmanus's r+. Good work! ::: security/manager/ssl/src/SharedSSLState.cpp @@ +28,5 @@ > + nsCOMPtr<nsICertOverrideService> icos = do_GetService(NS_CERTOVERRIDE_CONTRACTID); > + if (icos) { > + icos->ClearValidityOverride( > + NS_LITERAL_CSTRING("all:temporary-certificates"), > + 0); Nit: line wrapping is wierd. ::: security/manager/ssl/src/nsNSSCertificateDB.h @@ +16,5 @@ > class nsRecentBadCerts; > > class nsNSSCertificateDB : public nsIX509CertDB, public nsIX509CertDB2 > { > + typedef mozilla::Mutex Mutex; Nit: We usually don't do this typedef thing, in PSM at least.

(In reply to Josh Matthews [:jdm] from comment #72) > Created attachment 689957 [details] [diff] [review] > Part 7: Use separate SSL session cache entries for private connections. Honza: this patch was written by me, so I can't review it.

ship it!!!

remote: https://hg.mozilla.org/mozilla-central/rev/3008e6e5476f remote: https://hg.mozilla.org/mozilla-central/rev/499bd31197a1 remote: https://hg.mozilla.org/mozilla-central/rev/653fc39e223e remote: https://hg.mozilla.org/mozilla-central/rev/bc166c758a17 remote: https://hg.mozilla.org/mozilla-central/rev/09639e16d3cb remote: https://hg.mozilla.org/mozilla-central/rev/3b25f7b4a9ce remote: https://hg.mozilla.org/mozilla-central/rev/d0f828b1cd26

And https://hg.mozilla.org/mozilla-central/rev/3990b431f138 to stop the burning.

remote: https://hg.mozilla.org/mozilla-central/rev/087cf91da393 remote: https://hg.mozilla.org/mozilla-central/rev/a1c2ca7927ba remote: https://hg.mozilla.org/mozilla-central/rev/e16739935025 remote: https://hg.mozilla.org/mozilla-central/rev/645eb45e22d1 remote: https://hg.mozilla.org/mozilla-central/rev/e49e075893b1 remote: https://hg.mozilla.org/mozilla-central/rev/286604e4b3a0 remote: https://hg.mozilla.org/mozilla-central/rev/4d91e9934e57 remote: https://hg.mozilla.org/mozilla-central/rev/32638e411218 Burn burn burn. Maybe it's SSL.h vs ssl.h.

More cleanup work needed to occur on the main thread, it turned out.

Comment on attachment 690093 [details] [diff] [review] Part 6: Remove global private browsing NSS cleanup on exit. Wrong patch, sorry.

More cleanup work needed to occur on the main thread, it turned out.

Comment on attachment 690094 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. Still not the right patch version; I'm not sure what's happening here.

Comment on attachment 690096 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. This patch makes tests pass on my machine.

(In reply to Josh Matthews [:jdm] from comment #87) > This patch makes tests pass on my machine. Yes, but why? The assertion in nsNSSComponent is saying that nsNSSComponent received a profile-before-change notification without receiving a profile-change-net-teardown. But profile-before-change should always be received after profile-change-net-teardown. I am worried we may be doing the following during shutdown: 1. profile-change-net-teardown is broadcast 2. PSM is shut down. The nsNSSComponent instance is destroyed and NSS is shut down. 3. We process the event that clears the shared SSL state on the socket transport thread. This causes us to create a new nsNSSComponent instance, starting up NSS again. 4. We process the profile-before-change in nsNSSComponent, and see that there was profile-change-net-teardown was not broadcast since the component was constructed. (This is because profile-change-net-teardown was already broadcast and observed by the previous instance of nsNSSComponent, but not the new second instance.) If this is the sequence of events that is causing the problem, then the bug is that we're shutting down PSM (on the main thread) before we've cleared the shared SSL state ( on the socket trasnport thread). You could verify if this is the case, by setting a breakpoint in nsNSSComponent::nsNSSComponent and another one in MainThreadClearer::RunOnTargetThread. Then see if a new instance of nsNSSComponent gets created as part of getting the nsIX509CertDB service. Note that we send the event to the socket transport thread to clear the private shared networking state in response to last-pb-context-exited. Please explain when the last-pb-context-exited fires during profile change in relation to the profile-change-net-teardown, profile-before-change, and other profile-* events. If it is possible for last-pb-context-exited to fire after profile-change-net-teardown then all the components that observe last-pb-context-exited need to avoid accessing networking services. For example, it may be the case that the socket transport service should NOT be sending itself the event in response to last-pb-context-exited if profile-change-net-teardown has already occurred.

I was wrong; I see the same test_httpauth.js failure locally.

The problem in test_httpauth.js in particular is that instantiating nsICertOverrideService causes nsNSSComponent to be created if it does not exist. In this case, it has never existed over the course of the test; we trigger the last-pb-context-exited notification during the test, and the main thread clearing event happens to end up running while we're shutting down the HttpConnectionMgr and spinning the event loop in the profile-change-net-teardown observer (nsHttpHandler). Therefore, the nsNSSComponent observer doesn't get triggered, so we see the assertion later.

I suppose I could just early return out of the MainThreadClearer if PrivateSSLState() returns a null pointer - that should mean that we haven't done anything requiring cleaning up.

I filed bug 819746 about nsNSSComponent making bad assumptions about its observations of the XPCOM shutdown event sequence. (In reply to Josh Matthews [:jdm] from comment #91) > I suppose I could just early return out of the MainThreadClearer if > PrivateSSLState() returns a null pointer - that should mean that we haven't > done anything requiring cleaning up. I think that's an invalid assumption, when we consider that extensions could be manipulating the cert override service and/or recent bad cert data in ways different than what the code currently does. And, also, while the internal Gecko code may be currently aligned with that assumption, (a) it is not obvious to me that it is so, and (b) it isn't clear that it will always remain so. IMO, the best solution is to avoid sending the last-pb-context-exited notification during XPCOM shutdown (e.g. don't send it if we're closing the last window and we know we're never opening a new one), or to ensure that it gets sent before and of the profile-* shutdown events. Also, please document the last-pb-context-exited event in https://wiki.mozilla.org/XPCOM_Shutdown and/or in https://developer.mozilla.org/en-US/docs/Observer_Notifications.

last-pb-context-exited is being sent during a totally reasonable time (ie. before XPCOM shutdown and the various profile-* notifications). It's the main thread->socket thread->main thread dance which happens to be processed while shutdown is in progress.

Comment on attachment 690096 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. I think jdm and I agree this isn't the right thing to do.

It's not pretty, but it's not as bad as some of the things we considered.

Comment on attachment 690550 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. Review of attachment 690550 [details] [diff] [review]: ----------------------------------------------------------------- It wasn't good that I overlooked these issues in the initial review. It will be good to get a review from both me and mcmanus or mayhemer on the updated patch. ::: security/manager/ssl/src/SharedSSLState.cpp @@ +27,5 @@ > +public: > + MainThreadClearer() : mShouldClearSessionCache(false) {} > + > + void RunOnTargetThread() { > + if (mozilla::psm::SharedSSLState::CertOverrideExists()) { "CertOverrideExists" implies that there is at least one cert override. But, that's not the case. Rather, this variable really just means that we have instantiated the cert override service. So, the naming should be something like "CertOverrideServiceInstantiated." Similar comments apply to X509CertDBExists. X509CertDBInstantiated would be a better name. Also, we need to document (with a comment) why we're going through these contortions: This code can be called during XPCOM shutdown, and we want to avoid instantiating any of these components during XPCOM shutdown because they may (perhaps wrongly) be making assumptions about being able to observe all of the XPCOM shutdown events. We know this to be true for nsNSSComponent, which these services depend on. @@ +48,5 @@ > + } > + } > + > + mShouldClearSessionCache = mozilla::psm::PrivateSSLState() && > + mozilla::psm::PrivateSSLState()->SocketCreated(); OK, I overlooked this in the previous review and then I decided not to say anything since it'd landed. But, not that it got backed out: why is mShouldClearSessionCache an instance variable of this class when it's only used in ClearPrivateSSLState()? See also the next comment. Also, again, we need a comment that explains why we are guarding the call to SSL_ClearSessionCache with SocketCreated(). I've already forgotten why! @@ +58,5 @@ > > namespace mozilla { > + > +void ClearPrivateSSLState() > +{ Please add this: // This only works if it is called on the socket transport // service thread immediately after closing all private SSL // connections. #ifdef DEBUG nsCOMPtr<nsIEventTarget> sts = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &nrv); MOZ_ASSERT(NS_SUCCEEDED(nrv)); MOZ_ASSERT(sts->IsOnCurrentThread(&onSTSThread)) #endif Please add the similar/same comment to the caller of this function. @@ +62,5 @@ > +{ > + RefPtr<MainThreadClearer> runnable = new MainThreadClearer; > + runnable->DispatchToMainThreadAndWait(); > + > + if (runnable->mShouldClearSessionCache) { OK, I found another bug. This is related to the previous comment, When MainThreadCleaner is constructed, mShouldClearSessionCache is false. Then it is calculated on the main thread, concurrently with this code. That means there's a race condition, and in all likelihood this thread is going to win the race, which means that SSL_ClearSessionCache will never get called. This will be solved if you calculate shouldClearSessionCache here. @@ +99,5 @@ > } > > SharedSSLState::SharedSSLState() > : mClientAuthRemember(new nsClientAuthRememberService) > +, mSocketCreated(false) why is mSocketCreated an instance variable and the other two variables are static variables? Don't all three variables have exactly the same purpose? If so they should be all handled the same way (probably all three instance variables). ::: security/manager/ssl/src/SharedSSLState.h @@ +50,5 @@ > RefPtr<nsClientAuthRememberService> mClientAuthRemember; > nsSSLIOLayerHelpers mIOLayerHelpers; > + bool mSocketCreated; > + static bool sCertOverrideExists; > + static bool sCertDBExists; Why no mutex to protect these? We either need a mutex or we need to document appropriately our assumptions (using MOZ_ASSERT(NS_IsMainThread() and comments in the callers and in the setters and getters).

Comment on attachment 690550 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. Review of attachment 690550 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SharedSSLState.cpp @@ +62,5 @@ > +{ > + RefPtr<MainThreadClearer> runnable = new MainThreadClearer; > + runnable->DispatchToMainThreadAndWait(); > + > + if (runnable->mShouldClearSessionCache) { Actually, I was being dumb, sorry. The whole idea of DispatchToMainThreadAndWait() is that we wait for the main thread action to complete. Still, I think it is better to calculate shouldClearSessionCache here.

(In reply to Brian Smith (:bsmith) from comment #97) > ::: security/manager/ssl/src/SharedSSLState.cpp > @@ +48,5 @@ > > + } > > + } > > + > > + mShouldClearSessionCache = mozilla::psm::PrivateSSLState() && > > + mozilla::psm::PrivateSSLState()->SocketCreated(); > > OK, I overlooked this in the previous review and then I decided not to say > anything since it'd landed. But, not that it got backed out: why is > mShouldClearSessionCache an instance variable of this class when it's only > used in ClearPrivateSSLState()? See also the next comment. > > Also, again, we need a comment that explains why we are guarding the call to > SSL_ClearSessionCache with SocketCreated(). I've already forgotten why! If NSS has not been initialized, clearing the session cache triggers an assertion. It's an instance variable because the variable upon which the check is gated upon (gPrivateState) is initialized on the main thread (SharedSSLState::GlobalInit) and it's a lot cleaner to perform the check on the main thread rather than adding locking. > @@ +99,5 @@ > > } > > > > SharedSSLState::SharedSSLState() > > : mClientAuthRemember(new nsClientAuthRememberService) > > +, mSocketCreated(false) > > why is mSocketCreated an instance variable and the other two variables are > static variables? Don't all three variables have exactly the same purpose? > If so they should be all handled the same way (probably all three instance > variables). mSocketCreated is an instance because its value is potentially different for each instance of SharedSSLState. Granted, we don't care about the one in the public state. I realize that I do need to add locking for it, since NoteSocketCreated should be called on the socket thread (at least I'm assuming that's where nsSSLIOLayerAddToSocket runs). > ::: security/manager/ssl/src/SharedSSLState.h > @@ +50,5 @@ > > RefPtr<nsClientAuthRememberService> mClientAuthRemember; > > nsSSLIOLayerHelpers mIOLayerHelpers; > > + bool mSocketCreated; > > + static bool sCertOverrideExists; > > + static bool sCertDBExists; > > Why no mutex to protect these? We either need a mutex or we need to document > appropriately our assumptions (using MOZ_ASSERT(NS_IsMainThread() and > comments in the callers and in the setters and getters). Can the nsNSSComponent be created off the main thread? If so, yes, this needs some locking.

(In reply to Josh Matthews [:jdm] from comment #99) > If NSS has not been initialized, clearing the session cache triggers an > assertion. It's an instance variable because the variable upon which the > check is gated upon (gPrivateState) is initialized on the main thread > (SharedSSLState::GlobalInit) and it's a lot cleaner to perform the check on > the main thread rather than adding locking. Please add a comment to the code. > Can the nsNSSComponent be created off the main thread? If so, yes, this > needs some locking. It is a bug that nsNSSComponent cannot be created off the main thread today. Tomorrow it will be able to be created off the main thread. The important thing is that, when we make these subtle changes (allowing nsNSSComponent to be created off the main thread), we need to be able to find the code making assumptions about this (e.g. by looking for assertion failures) and we should be minimizing the amount of code that makes those assumptions. BTW, even though nsNSSComponent cannot be created off the main thread, nsIX509CertDB *can* be instantiated off the main thread, even today, as long as we know that nsNSSComponent has already been instantiated (on the main thread).

I believe this addresses all concerns. Sorry about the slapdash approach to documentation/asserting until now.

Comment on attachment 691057 [details] [diff] [review] Part 5: Close private socket connections when the lsat private browsing instance dies. Review of attachment 691057 [details] [diff] [review]: ----------------------------------------------------------------- Thanks jdm. ::: security/manager/ssl/src/SharedSSLState.cpp @@ +76,5 @@ > + = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + bool onSTSThread; > + sts->IsOnCurrentThread(&onSTSThread); > + MOZ_ASSERT(onSTSThread); rv = sts->IsOnCurrentThread(&onSTSThread); MOZ_ASSERT(NS_SUCCEEDED(rv) && onSTSThread); @@ +143,5 @@ > > void > SharedSSLState::ResetStoredData() > { > + MOZ_ASSERT(NS_IsMainThread(), "Not on main thread"); MOZ_ASSERT(NS_IsMainThread()) is also fine.

https://hg.mozilla.org/mozilla-central/rev/42b468942a84 https://hg.mozilla.org/mozilla-central/rev/7b0fdedb4fa9 https://hg.mozilla.org/mozilla-central/rev/e10c24f10bd2 https://hg.mozilla.org/mozilla-central/rev/e75cd1e1fca3 https://hg.mozilla.org/mozilla-central/rev/6d8d78bd56a9 https://hg.mozilla.org/mozilla-central/rev/912331d0c2fd https://hg.mozilla.org/mozilla-central/rev/b2fb475b6e4e

Backed out, because this time we leak a mutex :( https://hg.mozilla.org/mozilla-central/rev/553a3bcf1fe7 https://tbpl.mozilla.org/?rev=b2fb475b6e4e While I can technically try to figure out why, I'm too tired tonight and will probably screw something up... :/

FWIW, I bet this is the mutex which leaks <https://hg.mozilla.org/mozilla-central/rev/6d8d78bd56a9#l7.148> which is not all that surprising, given that it's a static object. Even if it did not leak, we should not use a static mutex because of static constructors...

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1dab473a7e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea45d78f5b3b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/571f73bc8d01 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/209bff5d2e80 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcf530e1c41 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cec5721713dc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c2323ca151 Leaky mutex should have been fixed this time.

Aaaaand jumped the gun. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b31461e3461 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a03abca869a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4e334e86bd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea6a655d33b0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d6d7e0b05d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0938ca4a757c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb29aa09649

Here's the interdiff of my changes, Brian. Thoughts?

As discussed. There is a lot less goop here, and that is nice.

Comment on attachment 691563 [details] [diff] [review] Remove the static lock ugliness. Review of attachment 691563 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SharedSSLState.cpp @@ +17,5 @@ > #include "PSMRunnable.h" > #include "PublicSSL.h" > #include "ssl.h" > #include "nsNetCID.h" > +#include "mozilla/ClearOnShutdown.h" unneeded now? @@ +24,5 @@ > > namespace { > > +static bool sCertOverrideSvcExists = false; > +static bool sCertDBExists = false; These should be of type PRInt32 because that is the type that PR_ATOMIC_SET is expecting. Some (all?) implementations of PR_ATOMIC_SET cast the pointer to this to (PRInt32) or equiv under the covers, but that can actually cause serious problems if sizeof(bool) != sizeof(PRInt32). @@ +197,5 @@ > > /*static*/ void > SharedSSLState::NoteCertOverrideServiceInstantiated() > { > + PR_ATOMIC_SET(&sCertOverrideSvcExists, 1); (void) or mozilla::unused << @@ +203,5 @@ > > /*static*/ void > SharedSSLState::NoteCertDBServiceInstantiated() > { > + PR_ATOMIC_SET(&sCertDBExists, 1); ditto. ::: security/manager/ssl/src/SharedSSLState.h @@ +7,5 @@ > #ifndef SharedSSLState_h > #define SharedSSLState_h > > #include "mozilla/RefPtr.h" > +#include "mozilla/StaticPtr.h" now this isn't necessary, right? @@ +54,4 @@ > // True if any sockets have been created that use this shared data. > // Requires synchronization between the socket and main threads for > // reading/writing. > + Mutex mLock; At least in PSM, if not most places, we name the mutex mMutex and we name the MutexAutoLock lock.

Fixed the comments from above and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6542f7267684

Try's looking green quite green.

Comment on attachment 691692 [details] [diff] [review] Remove the static lock ugliness. Review of attachment 691692 [details] [diff] [review]: ----------------------------------------------------------------- I think this is the one.

remote: https://hg.mozilla.org/mozilla-central/rev/9e9281ce1b63 remote: https://hg.mozilla.org/mozilla-central/rev/a80ec6b90878 remote: https://hg.mozilla.org/mozilla-central/rev/38cd815da29a remote: https://hg.mozilla.org/mozilla-central/rev/ee2822d2eab9 remote: https://hg.mozilla.org/mozilla-central/rev/072da0be4236 remote: https://hg.mozilla.org/mozilla-central/rev/761ff8b47a4e remote: https://hg.mozilla.org/mozilla-central/rev/d9336f8c0f6c

Backed out on suspicion of contributing to bug 821701: https://hg.mozilla.org/integration/mozilla-inbound/rev/f255406bb4aa (The backout of bug 818732 may have resolved the problem, but waiting for the retriggers to complete will mean keeping the tree closed for even longer, which isn't fair on the other devs).

(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #116) > Backed out on suspicion of contributing to bug 821701: > https://hg.mozilla.org/integration/mozilla-inbound/rev/f255406bb4aa Merge of backout: https://hg.mozilla.org/mozilla-central/rev/f255406bb4aa

Relanded with the crashing test disabled: https://hg.mozilla.org/mozilla-central/rev/919c713155ac https://hg.mozilla.org/mozilla-central/rev/8d86cc44bcee https://hg.mozilla.org/mozilla-central/rev/2c246a220cf5 https://hg.mozilla.org/mozilla-central/rev/cf442d34b4fa https://hg.mozilla.org/mozilla-central/rev/e2f772661bdf https://hg.mozilla.org/mozilla-central/rev/f603bf0ba354 https://hg.mozilla.org/mozilla-central/rev/86e8b382ab0a