Perform cleanup analogous to the relevant effects of LogoutAndTearDown when leaving private browsing mode

RESOLVED FIXED in mozilla20

Status

()

Core
Security: PSM
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 24 obsolete attachments)

67.90 KB, patch
Details | Diff | Splinter Review
5.69 KB, patch
Details | Diff | Splinter Review
1.81 KB, patch
Details | Diff | Splinter Review
3.54 KB, patch
Details | Diff | Splinter Review
4.42 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
27.41 KB, patch
briansmith
: review+
mcmanus
: review+
Details | Diff | Splinter Review
5.87 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
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.

Updated

6 years ago
Assignee: nobody → chrislord.net
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.

Comment 2

6 years ago
(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?

Updated

6 years ago
Blocks: 794502
Formalizing the request in comment 4.
Flags: needinfo?(bsmith)
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).
Flags: needinfo?(bsmith)
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.
Depends on: 722979
Created attachment 678480 [details] [diff] [review]
Make PSM more amenable to storing concurrent private and non-private data.
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?
Attachment #678480 - Flags: feedback?(bsmith)
Assignee: chrislord.net → josh
Summary: Support concurrent private/public SecretDecoderRing auth tokens → Perform cleanup analogous to the relevant effects of LogoutAndTearDown when leaving private browsing mode
Created attachment 678488 [details] [diff] [review]
Part 2: Avoid storing intermediate cert data for private contexts.
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
Created attachment 679160 [details] [diff] [review]
Clear all temporary cert overrides upon leaving private browsing.

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.
Attachment #679160 - Flags: review?(bsmith)
Created attachment 679161 [details] [diff] [review]
Part 3: Clear SSL session cache upon leaving private browsing.
Attachment #679161 - Flags: review?(bsmith)
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.
Attachment #678488 - Flags: feedback?(bsmith)
Attachment #678488 - Flags: superreview?(honzab.moz)
Attachment #678488 - Flags: review+
Attachment #678488 - Flags: feedback?(bsmith)
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.
Attachment #678480 - Flags: feedback?(bsmith) → feedback+
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.
Attachment #679160 - Flags: review?(bsmith) → review-
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.
Created attachment 680016 [details] [diff] [review]
Part 1: Make PSM more amenable to storing concurrent private and non-private data.

As explained, I did not implement your comments about the preference values.
Attachment #680016 - Flags: review?(bsmith)
Attachment #678480 - Attachment is obsolete: true
Created attachment 680017 [details] [diff] [review]
Part 4: Clear all temporary cert overrides upon leaving private browsing.
Attachment #680017 - Flags: review?(bsmith)
Attachment #679160 - Attachment is obsolete: true
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.
Attachment #680016 - Flags: review?(bsmith) → review-
Attachment #680017 - Flags: review?(bsmith) → review+
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.
Attachment #678488 - Flags: superreview?(honzab.moz) → superreview+
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.
Attachment #680016 - Attachment description: Make PSM more amenable to storing concurrent private and non-private data. → Part 1: Make PSM more amenable to storing concurrent private and non-private data.
Attachment #678488 - Attachment description: Avoid storing intermediate cert data for private contexts. → Part 2: Avoid storing intermediate cert data for private contexts.
Attachment #679161 - Attachment description: Clear SSL session cache upon leaving private browsing. → Part 3: Clear SSL session cache upon leaving private browsing.
Attachment #680017 - Attachment description: Clear all temporary cert overrides upon leaving private browsing. → Part 4: Clear all temporary cert overrides upon leaving private browsing.
Created attachment 681021 [details] [diff] [review]
Part 1: Make PSM more amenable to storing concurrent private and non-private data.

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.
Attachment #681021 - Flags: review?(bsmith)
Attachment #680016 - Attachment is obsolete: true
(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.
Attachment #679161 - Flags: superreview?(honzab.moz)
Attachment #679161 - Flags: review?(bsmith)
Attachment #679161 - Flags: review+
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.
Attachment #681021 - Flags: superreview?(honzab.moz)
Attachment #681021 - Flags: review?(bsmith)
Attachment #681021 - Flags: review+
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.
Created attachment 682749 [details] [diff] [review]
Part 1: Make PSM more amenable to storing concurrent private and non-private data.

This is the actual patch rebased on the landed STS changes.
Attachment #681021 - Attachment is obsolete: true
Attachment #681021 - Flags: superreview?(honzab.moz)
>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.
Attachment #682749 - Flags: superreview?(honzab.moz)
Created attachment 682831 [details] [diff] [review]
Remove global private browsing NSS cleanup on exit.
Attachment #682831 - Flags: review?(ehsan)
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.
Created attachment 682850 [details] [diff] [review]
Use a separate SSL session cache for private browsing

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?
Attachment #682850 - Flags: feedback?(josh)
(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.
Attachment #682850 - Flags: feedback?(josh) → feedback+
>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.
Attachment #682831 - Flags: review?(ehsan) → review-

Comment 49

6 years ago
(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
Attachment #682749 - Flags: superreview?(honzab.moz) → superreview+
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
Attachment #679161 - Flags: superreview?(honzab.moz) → superreview+
>::: 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) {
Created attachment 684509 [details] [diff] [review]
Remove global private browsing NSS cleanup on exit.
Attachment #684509 - Flags: review?(ehsan)
Attachment #682831 - Attachment is obsolete: true

Comment 56

6 years ago
Comment on attachment 684509 [details] [diff] [review]
Remove global private browsing NSS cleanup on exit.

Review of attachment 684509 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #684509 - Flags: review?(ehsan) → review+
Created attachment 684527 [details] [diff] [review]
Close all private HTTP connections, and trigger NSS PB-related cleanup only once that operation completes.

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.
Attachment #684527 - Flags: feedback?(bsmith)
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)
Attachment #684527 - Flags: feedback?(bsmith) → feedback-
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.
Created attachment 686252 [details] [diff] [review]
Close private socket connections when the lsat private browsing instance dies.

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.
Attachment #686252 - Flags: review?
Attachment #686252 - Flags: feedback?(bsmith)
Attachment #684527 - Attachment is obsolete: true
Attachment #686252 - Flags: review?

Updated

5 years ago
Blocks: 818732

Updated

5 years ago
Blocks: 818800
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.
Attachment #686252 - Flags: feedback?(bsmith)
Created attachment 689537 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.

The SharedSSLState changes are mostly to facilitate including the header outside of security/.
Attachment #689537 - Flags: review?(bsmith)
Attachment #686252 - Attachment is obsolete: true
Created attachment 689539 [details] [diff] [review]
Rollup

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.
Attachment #689537 - Flags: review?(mcmanus)
Attachment #689537 - Flags: review?(bsmith)
Attachment #689537 - Flags: review-
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.
Attachment #689537 - Flags: review?(mcmanus) → review+
Created attachment 689931 [details] [diff] [review]
Part 1: Make PSM more amenable to storing concurrent private and non-private data.
Attachment #682749 - Attachment is obsolete: true
Created attachment 689932 [details] [diff] [review]
Part 2: Avoid storing intermediate cert data for private contexts.  s
Attachment #678488 - Attachment is obsolete: true
Created attachment 689934 [details] [diff] [review]
Part 3: Clear all temporary cert overrides upon leaving private browsing.
Attachment #680017 - Attachment is obsolete: true
Created attachment 689935 [details] [diff] [review]
Part 4: Clear SSL session cache upon leaving private browsing.
Attachment #679161 - Attachment is obsolete: true
Created attachment 689936 [details] [diff] [review]
Part 6: Remove global private browsing NSS cleanup on exit.
Attachment #684509 - Attachment is obsolete: true
Created attachment 689955 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.

The DNS cache is a good point. I'll look into it in another bug.
Attachment #689955 - Flags: review?(bsmith)
Attachment #689537 - Attachment is obsolete: true
Created attachment 689957 [details] [diff] [review]
Part 7: Use separate SSL session cache entries for private connections.
Attachment #689957 - Flags: review?(honzab.moz)
Attachment #682850 - Attachment is obsolete: true
Attachment #689539 - Attachment is obsolete: true
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.
Attachment #689955 - Flags: review?(bsmith) → review+
(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.
Attachment #689957 - Flags: review?(honzab.moz) → review+

Comment 75

5 years ago
ship it!!!

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Created attachment 690010 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.
Attachment #689955 - Attachment is obsolete: true
Created attachment 690093 [details] [diff] [review]
Part 6: Remove global private browsing NSS cleanup on exit.

More cleanup work needed to occur on the main thread, it turned out.
Attachment #690093 - Flags: review?(bsmith)
Comment on attachment 690093 [details] [diff] [review]
Part 6: Remove global private browsing NSS cleanup on exit.

Wrong patch, sorry.
Attachment #690093 - Flags: review?(bsmith)
Created attachment 690094 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.

More cleanup work needed to occur on the main thread, it turned out.
Attachment #690094 - Flags: review?(bsmith)
Attachment #690010 - Attachment is obsolete: true
Attachment #689936 - Attachment is obsolete: true
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.
Attachment #690094 - Attachment is obsolete: true
Attachment #690094 - Flags: review?(bsmith)
Created attachment 690095 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.
Created attachment 690096 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.
Attachment #690095 - Attachment is obsolete: true
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.
Attachment #690096 - Flags: review?(bsmith)
(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.
Attachment #690096 - Flags: review?(bsmith) → review-
Created attachment 690550 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.

It's not pretty, but it's not as bad as some of the things we considered.
Attachment #690550 - Flags: review?(bsmith)
Attachment #690096 - Attachment is obsolete: true
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).
Attachment #690550 - Flags: review?(bsmith) → review-
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).
Created attachment 691057 [details] [diff] [review]
Part 5: Close private socket connections when the lsat private browsing instance dies.

I believe this addresses all concerns. Sorry about the slapdash approach to documentation/asserting until now.
Attachment #691057 - Flags: review?(bsmith)
Attachment #690550 - Attachment is obsolete: true
Attachment #691057 - Flags: review?(mcmanus)
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.
Attachment #691057 - Flags: review?(bsmith) → review+
Attachment #691057 - Flags: review?(mcmanus) → review+

Comment 104

5 years ago
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... :/

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 105

5 years ago
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...
Created attachment 691342 [details] [diff] [review]
Chagnes to lock to make it never leak.

Here's the interdiff of my changes, Brian. Thoughts?
Attachment #691342 - Flags: review?(bsmith)
Created attachment 691563 [details] [diff] [review]
Remove the static lock ugliness.

As discussed. There is a lot less goop here, and that is nice.
Attachment #691563 - Flags: review?(bsmith)
Attachment #691342 - Attachment is obsolete: true
Attachment #691342 - Flags: review?(bsmith)
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.
Attachment #691563 - Flags: review?(bsmith) → review-
Fixed the comments from above and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=6542f7267684
Created attachment 691692 [details] [diff] [review]
Remove the static lock ugliness.

Try's looking green quite green.
Attachment #691692 - Flags: review?(bsmith)
Attachment #691563 - Attachment is obsolete: true
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.
Attachment #691692 - Flags: review?(bsmith) → review+

Updated

5 years ago
Depends on: 821701
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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

Updated

5 years ago
Blocks: 823342
No longer blocks: 823342
Depends on: 823342

Updated

5 years ago
Depends on: 827272
Duplicate of this bug: 577689
Duplicate of this bug: 808331
You need to log in before you can comment on or make changes to this bug.