463256 - SSL sites sometimes don't load after re-entering regular browsing mode (from Private Browsing)

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Marcia Knous [:marcia]]

Seen while testing Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081105 Minefield/3.1b2pre. I have now seen this on two different macs. In each instance I saw it after switching from PB-> Back to regular browsing.

STR:

1. Enter PB mode. Browse around.
2. Switch back to regular mode.
3. Open a new tab and type in the awesome bar to locate an entry
4. Highlight that entry and press Enter (or use the mouse).

Expected: The site will load.
Actual: The Site does not load.

Comment 1

[Marcia Knous [:marcia]]

I see this happening when entering PB mode as well as when exiting it.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marcia, can you test with a fresh profile? Do you have initially multiple tabs open? Would be good to have minimized steps to reproduce because I cannot see this on my box with 10.5.

Comment 3

[(no longer active)]

Marcia: do you only see this on Macs?  Can you provide an exact STR for me to follow (preferably on a fresh profile)?  Anything logged in the error console?  What happens if you disconnect your network connection for example, or use a site with an invalid certificate; do you get an error page or still nothing?

Comment 4

[Marcia Knous [:marcia]]

Ehsan: I see this testing on my 10.5 machine. I don't see any errors in the error console. It is one of those difficult, intermittent bugs but I will try to get exact STR the issue.

Comment 5

[Marcia Knous [:marcia]]

I was able to show stephend this issue on my machine. One thing I noticed for sure it it happens fairly regular with both litmus.mozilla.org and bugzilla.mozilla.org

Comment 6

[(no longer active)]

Does it only happen with SSL sites?

Simon, I seem to remember that we had observed this problem.  IIRC the problem was seen with the below STR:

1. Load AMO (or any other SSL site) in non-private mode.
2. Switch to private browsing mode.
3. Switch back to normal mode.

The AMO tab wouldn't be restored.

I remember you had a clue why this might happen.  Can you please refresh my memory?

Marcia: I just tested the above STR and the problem happens on Windows at least.  Is this what you were observing?  Do you think that it's relevant?

Comment 7

[Marcia Knous [:marcia]]

Ehsan: Yes, I can confirm that the STR you describe in Comment 6 happen using the latest nightly on two separate Mac machines. When I was seeing the issue the other day I could still reload the page and get the bugzilla or Litmus page, but today when I try it on several other sites here is what I see:

1.  Visit bankofamerica.com site in regular mode
2.  Switch to PB mode.
3.  Switch back to regular mode.
4.  The bankofamerica.com tab is there but "untitled", and I cannot reload the site since all the toolbar icons are greyed out. 

(In reply to comment #6)
> Does it only happen with SSL sites?
> 
> Simon, I seem to remember that we had observed this problem.  IIRC the problem
> was seen with the below STR:
> 
> 1. Load AMO (or any other SSL site) in non-private mode.
> 2. Switch to private browsing mode.
> 3. Switch back to normal mode.
> 
> The AMO tab wouldn't be restored.
> 
> I remember you had a clue why this might happen.  Can you please refresh my
> memory?
> 
> Marcia: I just tested the above STR and the problem happens on Windows at
> least.  Is this what you were observing?  Do you think that it's relevant?

Comment 8

[Simon Bünzli]

(In reply to comment #6)
> Simon, I seem to remember that we had observed this problem.

See bug 248970 comment #311 (keyword: gotoIndex).

Comment 9

[Marcia Knous [:marcia]]

If I follow my STR in Comment 7 with a fresh profile on Mac, I pretty much consistently see the problem with the bankofamerica.com site not loading when switching from PB->back to regular browsing mode. I also had https://wellsfargo.com loaded, and when I switched back that https site was fine.

Comment 10

[(no longer active)]

(In reply to comment #8)
> (In reply to comment #6)
> > Simon, I seem to remember that we had observed this problem.
> 
> See bug 248970 comment #311 (keyword: gotoIndex).

I worked on this a little bit, and I'm not sure I understand what's going on here.  I added debug messages to GotoIndex, and I didn't observe any failure there.

One interesting thing is that after restoring and observing the problem, opening a new window and trying to use the URL bar to load the same site which has problems does work, so effectively this is a problem with restored windows, and nothing global.  And it happens on all of the restored windows.

Simon, do you have any other suspicions on what's failing inside setBrowserState?  I'm feeling clueless here.

Comment 11

[Simon Bünzli]

(In reply to comment #10)
> I added debug messages to GotoIndex, and I didn't observe any failure there.

Have you actually re-read bug 248970 comment #311? It seemed that GotoIndex silently failed in nsDocShell::IsNavigationAllowed.

> Simon, do you have any other suspicions on what's failing inside
> setBrowserState?  I'm feeling clueless here.

As I've already said in bug 248970 comment #304, the failure isn't in setBrowserState as that code path works just fine when PB mode isn't involved.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

When I do the steps from comment 7 the about:privatebrowsing page stays open when leaving the PB mode. The bankofamerica.com website isn't opened again. Same happens for all other SSL websites. There is no error shown in the error console.

I'm running Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081114 Minefield/3.1b2pre

What further information do you need?

Comment 13

[(no longer active)]

Attachment: What I tried

(In reply to comment #11)
> (In reply to comment #10)
> > I added debug messages to GotoIndex, and I didn't observe any failure there.
> 
> Have you actually re-read bug 248970 comment #311? It seemed that GotoIndex
> silently failed in nsDocShell::IsNavigationAllowed.

Yes, I have, but as it turns out, I was wrong in bug 248970 comment 311.  I tried running in debug mode with this patch applied, and none of the assertions were triggered, which means that nsDocShell::IsNavigationAllowed and nsDocShell::GotoIndex are not failing.

> > Simon, do you have any other suspicions on what's failing inside
> > setBrowserState?  I'm feeling clueless here.
> 
> As I've already said in bug 248970 comment #304, the failure isn't in
> setBrowserState as that code path works just fine when PB mode isn't involved.

I need to do some more investigation then...

Comment 14

[Simon Bünzli]

Could we be hitting the following silent return in nsSHistory::LoadEntry: <http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1175>?

Comment 15

[(no longer active)]

(In reply to comment #14)
> Could we be hitting the following silent return in nsSHistory::LoadEntry:
> <http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1175>?

Unfortunately, no.  I tested this as well with an assertion which was not triggered when the problem did occur.

Comment 16

[(no longer active)]

(In reply to comment #11)
> As I've already said in bug 248970 comment #304, the failure isn't in
> setBrowserState as that code path works just fine when PB mode isn't involved.

I decided to check if setBrowserState is doing its job properly outside of the private browsing mode context, and the first thing I did was to look for tests.  But seems like the only code which uses [sg]etBrowserState is the private browsing code...

<http://mxr.mozilla.org/mozilla-central/ident?i=getBrowserState>
<http://mxr.mozilla.org/mozilla-central/ident?i=setBrowserState>

Anyway, I was thinking of writing a browser chrome tests for this (which is useful anyway), so I was wondering if that would be possible given the constraints of the test framework?

Comment 17

[(no longer active)]

OK, after *days* of debugging, I think I've finally found the culprit:

<http://hg.mozilla.org/mozilla-central/file/a730d0337646/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#l140>

I'm going to get started on finding a solution now.

Mike and Mike: I think this should be a beta 2 blocker, since this is a major functionality which is broken.  That is, of course, if I can come up with a patch soon.

Comment 18

[(no longer active)]

On further investigation, it seems that calling nsISecretDecoderRing::LogoutAndTeardown does cause this problem, while calling nsISecretDecoderRing::Logout doesn't.

So, according to the difference of the implementations of these methods, this should have something to do with nsNSSComponent::LogoutAuthenticatedPK11 <http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#2143>.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking, but not blocking beta 2.

Comment 20

[Marcia Knous [:marcia]]

adding relnote keyword...

Comment 21

[(no longer active)]

Moving to Core.

Here is an analysis of the problem:

nsNSSComponent::LogoutAuthenticatedPK11 calls nsOnPK11LogoutCancelObject::logout on all the cancel objects <http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#2155>, which results in all of the objects marking themselves as logged out.  This causes nsSSLThread to fail either in requestRead <http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsSSLThread.cpp#628> or in requestWrite <http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsSSLThread.cpp#847>.  This failure is propagated to higher layers of networking code, and at the highest level manifests itself as a canceled load.

My understanding of this code is limited, but the problem seems to lie down in the fact that Necko might try to re-use existing sockets when trying to navigate to SSL sites, ignorant of the fact that nsISecretDecoderRing::LogoutAndTeardown might have been called along the way, making any re-use of those sockets doomed.

I'm not sure about the best way to tackle this problem, I'm CCing a bunch of people randomly hoping to get some insight.  :-)

Comment 22

[Shawn Wilsher :sdwilsh]

blocking-firefox-3.1+ got lost

Comment 23

[Boris Zbarsky [:bzbarsky]]

Kai is your friend here, I think.

Comment 24

[(no longer active)]

Kai: ping?

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Kai, can you please have a look at comment 21?

Comment 26

[Kai Engert (:KaiE:)]

The "teardown" functions were done to support a profile switch, without a restart.

A full profile switch requires to completely shut down NSS and all resources, prior to attempting a new NSS init.

A successful shut down of NSS requires to release all NSS resources.

At the time this got implemented, we we required to find a solution to this "clean up requirement" that were fully self contained inside PSM (/security/manager) code.

This is why we try to destroy/logout all remembered NSS resources, even though Mozilla's network code still has handles to SSL sockets.

Therefore we remember this "disabled state" for network connections, and let the network code know on the next read/write operation.

(the above is some background information, more comments to follow).

Comment 27

[Kai Engert (:KaiE:)]

Ideally you should find a way to "reset" the networking engine (Necko), and require it to stop reusing any old socket.

Comment 28

[Kai Engert (:KaiE:)]

nsIOService::Observe

has:

...
    else if (!strcmp(topic, kProfileChangeNetTeardownTopic)) {
        if (!mOffline) {
            SetOffline(PR_TRUE);
            mOfflineForProfileChange = PR_TRUE;
        }
    }
    else if (!strcmp(topic, kProfileChangeNetRestoreTopic)) {
        if (mOfflineForProfileChange) {
            mOfflineForProfileChange = PR_FALSE;
            if (!mManageOfflineStatus ||
                NS_FAILED(TrackNetworkLinkStatusForOffline())) {
                SetOffline(PR_FALSE);
            }
        }
    }
...

Maybe using 
  SetOffline(PR_TRUE);
  SetOffline(PR_FALSE);
might work?


Question, is private browsing mode implemented 
using "the same profile" or "a separate profile"?

Comment 29

[Kai Engert (:KaiE:)]

Ehsan, in comment 18 you said, calling "logout" instead of "logoutAndTeardown" avoids this bug.

Under the assumption that private browsing mode uses a single profile 
... 
(no separate profile for private browsing, 
 no profile switch when switching modes)
...
then you *could* avoid the teardown in order to avoid this bug, but...

There is a risk, maybe some SSL socket could get reused where client authentication has been used, and where the SSL session caching is still active.

This might happen when switching from regular (having authenticated) to private (don't use client cert) mode.

So doing a full teardown of sockets seems cleaner.

Comment 30

[Kai Engert (:KaiE:)]

Having read my prior comment, I hope you did implement private browsing using a separate profile, or at least a separate NSS session, where no personal certificates are active when visiting SSL sites.

Comment 31

[Kai Engert (:KaiE:)]

So I just tested the new private browsing mode for the first time.

I learned, when your profile contains personal certificates, those personal certs appear to be still "visible" in your "private browsing" session. This is problematic when you connect to a site that requests SSL client authentication. NSS may present the user's cert to sites when asked.

While Firefox 3 by default uses value "ask every time" when asked to present a personal cert, it still means a user might accidentally hit "ok" and identified themselves to the site.

Even more dangerous, when the user has changed pref
  security.default_personal_cert
to 
  "Select Automatically"
this pref value is preserved when switching to private browsing mode, meaning, personal certs for identification will be presented to sites automatically (when the site asks for it).

I'm worried that this might not be what's intended by a private browsing mode. I strongly recommend to switch to a temporary empty profile, or do indeed a full NSS session shutdown and re-init (with empty NSS database) by other means.

Comment 32

[(no longer active)]

(In reply to comment #28)
> Maybe using 
>   SetOffline(PR_TRUE);
>   SetOffline(PR_FALSE);
> might work?

FWIW, this does work.

> Question, is private browsing mode implemented 
> using "the same profile" or "a separate profile"?

Like you've found out, using the same profile.

(In reply to comment #31)
> I'm worried that this might not be what's intended by a private browsing mode.
> I strongly recommend to switch to a temporary empty profile, or do indeed a
> full NSS session shutdown and re-init (with empty NSS database) by other means.

Yes, we don't want that.

Can you guide me how to shut down NSS completely and re-init it?  I'm not that familiar with NSS code.  Also, could you please let me know what is stored in the NSS db (besides client certs)?  If using an empty NSS db may cause other problems, I'm thinking we need to fail nsNSS_SSLGetClientAuthData inside the private browsing mode, so that no personal certificate is ever sent.

Comment 33

[Kai Engert (:KaiE:)]

(cc'ing Honza only FYI)

(In reply to comment #32)
> Can you guide me how to shut down NSS completely and re-init it?  I'm not that
> familiar with NSS code.  Also, could you please let me know what is stored in
> the NSS db (besides client certs)?  If using an empty NSS db may cause other
> problems, I'm thinking we need to fail nsNSS_SSLGetClientAuthData inside the
> private browsing mode, so that no personal certificate is ever sent.

The NSS init and shutdown calls must be done in code in security/manager/ssl

The only currently supported approach is via a profile switch.
The profile manager talks to PSM using events which are observed by PSM.
That code is in nsNSSComponent.cpp

You should be able to simulate a profile switch by sending appropriate notifications to PSM.

We have the following topics:
#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_AFTER_CHANGE_TOPIC "profile-after-change"

The order of events should be identical to what profile manager uses today.

I think you must start with approve-change.

However, PSM might reject/veto that request.
Which means, a user's attempt to switch to private browsing mode may be vetoed by PSM/NSS. Hopefully you can avoid that by closing all open windows, however, if there is currently some blocking crypto UI open (NSS prompt), not sure if you'll succeed. So I propose to plan for some failure message (like "unable to switch to private browsing mode at this time").

The reason, if there is some crypto UI (window/prompt) currently open, then we will be unable to free NSS resources and unable to shut down NSS. Therefore PSM attempts to track what's open, and if there is, it will reject the profile change. (PSM calls nsIProfileChangeStatus::VetoChange on the context object passed along with the notification).

I forgot who sends profile-change-teardown-veto
I suspect profile manager sends it when some component vetoed, so all components are aware the change won't happen.

Next is profile-change-net-teardown,
then send profile-change-teardown (may call veto),
then profile-before-change (which does the shutdown).

Now would be the time to declare the new directory for the upcoming NSS re-init. PSM currently makes decisions about this place on its own, as of today it always uses the profile directory.
This is planned to change in the future (on trunk) as we implement shared database support (applications shared the same NSS database).

So, if you would like to re-init NSS with a different, empty NSS database in private browsing mode, we'd have to change PSM to query for that mode, and do NSS init accordingly.

Next you send profile-after-change which triggers the NSS re-init,
and finally profile-send-net-restore.


(Finally FYI, should we have any bad leaking of NSS resources in PSM or NSS itself, NSS shutdown will fail, too. Should we detect that state, we'll have to hunt and clear those leaks. Hopefully we're in good shape.)

Comment 34

[Honza Bambas (:mayhemer)]

From the user perspective it would be great to avoid any dialogs like "cannot switch to PB mode now". I'm afraid there will be cases when user won't be able to close any dialogs or so to continue, user might get stuck.

So, the goal during switch TO the PB mode here is to:
1. shutdown all existing ssl sockets to avoid its reuse during PB on
2. disallow client certificate authentication

The goal when switching FROM the PB mode is:
3. re-enable crypto and ssl in the original state
4. throw away all existing ssl sockets again (aka 1)

To achieve 1 and 4 we can use SetOffline(TRUE/FALSE). This closes all sosckets on necko level, cleanly.

To achieve 2 we can use upcoming patch from b1.9.1+ bug 466080. There is way to set an anonymous flag to completely disallow client certificate authentication. (for Kai: when anonymous mode is set socket is not configured with the clientcertcallback and uses "anon-host:port" peer id, so it is also cheaper from point of server and client to reuse anonymous sessions).

Under this light to achieve 3 we can just return the anonymous flag to FALSE.

Question is if we can be sure that there is no cypto op that could potentially survive the PB on switch and identify the user somehow. I don't about any personally at this moment.

This also would save reentering master password when returning from the PB mode (if desired). But I am not perfectly aware of the PB mode spec.

Makes sense?

Comment 35

[(no longer active)]

(In reply to comment #34)
> From the user perspective it would be great to avoid any dialogs like "cannot
> switch to PB mode now". I'm afraid there will be cases when user won't be able
> to close any dialogs or so to continue, user might get stuck.

Agreed.

> So, the goal during switch TO the PB mode here is to:
> 1. shutdown all existing ssl sockets to avoid its reuse during PB on
> 2. disallow client certificate authentication
> 
> The goal when switching FROM the PB mode is:
> 3. re-enable crypto and ssl in the original state
> 4. throw away all existing ssl sockets again (aka 1)

I think this should cover what we need perfectly fine, so maybe there's no reason to complicate things by simulating a profile switch.

> This also would save reentering master password when returning from the PB mode
> (if desired). But I am not perfectly aware of the PB mode spec.

The PB mode spec says nothing about master password prompt, but I don't think we'd want that prompt anyway, since we don't save any passwords in this mode, and don't autofill saved passwords either.

Comment 36

[Kai Engert (:KaiE:)]

I understand that "private browsing mode" is about "no history is recorded".

Maybe I made an incorrect assumption about the intention of PB, therefore, an clarification question:

Is PB also about "user is anonymous when talking to web servers"?
In other words, in PB mode, will existing cookies be sent, etc.?

If user is supposed to be anonymous, then the topic about SSL client auth is relevant.

Comment 37

[Kai Engert (:KaiE:)]

I apologize for making comments in this bug, I'm sure there would be better places, but I have the following additional concerns related to PB and SSL.

The user interface for PB clearly reports to the user "no history will be recorded".

At least with regards to SSL, this seems wrong.
I tried to add an SSL server exception, and it got permanently recorded in the profile.

What about other actions that record stuff into the user's NSS database?
Client certificate enrollment?
Installation of new CA certificate authorities?

I'm worried you haven't added code that would clean this up yet (my build is 2 weeks old), and I'm worried about the code complexity it would require to prevent those actions, or to clean up correctly.

I vote for a NSS re-init.

Comment 38

[Kai Engert (:KaiE:)]

Please disregard my comment about "cert exceptions are added", I'm way behind on my reviews and bugmail, and just discovered your patch to disable the add-exception UI in PB and have meanwhile given it r+.

However, my worries about enrolling for a client cert and about installing a new CA remain. While the latter may be acceptable per the specs, the former might be problematic.

Comment 39

[(no longer active)]

(In reply to comment #36)
> Is PB also about "user is anonymous when talking to web servers"?
> In other words, in PB mode, will existing cookies be sent, etc.?

No.  But in addition to "no history recorded", we try to make it difficult for websites to detect if the user is inside the private browsing mode or not.  That is why we don't send existing cookies inside the private browsing mode, or for example don't use the existing cache entries.

> If user is supposed to be anonymous, then the topic about SSL client auth is
> relevant.

With this explanation, do you still think it's relevant?

(In reply to comment #38)
> Please disregard my comment about "cert exceptions are added", I'm way behind
> on my reviews and bugmail, and just discovered your patch to disable the
> add-exception UI in PB and have meanwhile given it r+.

Please note that we will still allow adding cert exceptions if done explicitly inside the prefpane.  What we're trying to do is to protect users from implicit actions, but we do allow explicit actions (like we don't record history, but we do record bookmarks).

> However, my worries about enrolling for a client cert and about installing a
> new CA remain. While the latter may be acceptable per the specs, the former
> might be problematic.

I'm not too familiar with how these things are done.  Can you please provide a set of steps to do each of them in a typical scenario?  I want to see if these can be considered explicit actions or not.

Thanks!

Comment 40

[Kai Engert (:KaiE:)]

(In reply to comment #39)

If you don't send cookies, I conclude you want the user to feel somewhat anonymously.


> > If user is supposed to be anonymous, then the topic about SSL client auth is
> > relevant.
> 
> With this explanation, do you still think it's relevant?

Yes, because when using SSL client auth, the user is not anonymous.

 
> > However, my worries about enrolling for a client cert and about installing a
> > new CA remain. While the latter may be acceptable per the specs, the former
> > might be problematic.
> 
> I'm not too familiar with how these things are done.  Can you please provide a
> set of steps to do each of them in a typical scenario?  I want to see if these
> can be considered explicit actions or not.

Certain HTML tags (<keygen>) or JS (crypto.generateCRMFRequest()) will cause a keypair for public/private key encryption to be created and stored in the user's NSS database. In addition, the public key is usually sent to the web server as a certificate request. The web site can immediately respond with a generated certificate. Although there is usually a delay and some verification happening, there is the risk an attacker could use this approach to install such a client certificate as super-cookie in the user's profile.

If the browser is (explicitly) configured to do client auth automatically, without asking the user, then such certs will be used for client authentication without further prompts, this identifying the user to the server.


So, even if you use SSL socket level tricks to prevent the use of SSL client auth while in private mode, the user may acquire a new certificate while in PB mode, and when back in regular mode, that new cert will still be around and can be used for identification (or tracking) by servers.


The other issue is about certificate installation.
Any certificates that will be installed in PB mode will live on.

We automatically collect valid intermediate CA certs as we walk the web.
Maybe some "private" site is the only site who uses a particular intermediate cert, thus giving a clue to having visited the private site on later profile inspection?


Then we have installation of CA certificates.
Yes, when a site delivers a mime type application/x-x509-ca-certificate we bring up a prompt asking the user to install it.
But we all know that end users may often simply click OK, thus confirming the import.
Is confirming a prompt an explicit action?
As an end user I'd say "hey, I was in private mode, so why did this end up in my profile????"


I'm not sure I have enumerated all potential cases.
I'm not in favour of trying to explicitly disable recording code that we happen to find in the PB coding work, in a huge codebase that implements a big amount of features. I personally wouldn't call it PB if we can't be sure we have identified and disabled all possible mechanisms that record data in the user's profile.
I would be very much in favour of a temporary profile that can be expunged completely, but that's just my $.02 .
If you don't want to do that for all profile information, let's at least do it for the NSS database.

Comment 41

[(no longer active)]

(In reply to comment #40)
> (In reply to comment #39)
> 
> If you don't send cookies, I conclude you want the user to feel somewhat
> anonymously.

We don't want to provide anonymity.  What we're trying to protect against is services like Google search history, which work based on cookies.  But I guess similar services could be implemented using client certificates as well.

> I would be very much in favour of a temporary profile that can be expunged
> completely, but that's just my $.02 .
> If you don't want to do that for all profile information, let's at least do it
> for the NSS database.

What would the implications of re-initing the NSS database be for other parts of the browser?  For example, if someone protects their passwords db with a master password, would re-initing the NSS database mean that none of their passwords would be accessible?  Is there any other component of the browser with a strong tie to the stuff stored in NSS database?

Also, mconnor, it'd be great to see what you think here.

Comment 42

[Kai Engert (:KaiE:)]

(In reply to comment #41)
> We don't want to provide anonymity.  What we're trying to protect against is
> services like Google search history, which work based on cookies.

I don't understand the difference between 
  I am surfing anonymously
and
  this web site does not know what I've searched earlier
  (or: this site doesn't know I've been here before)

I would describe both statements as equivalent.


> What would the implications of re-initing the NSS database be for other parts
> of the browser?  For example, if someone protects their passwords db with a
> master password, would re-initing the NSS database mean that none of their
> passwords would be accessible? 

That's correct. And IMHO, if you allow users to make use of their stored password in the private mode, it's another way to allow sites to know who you are.


I think, if you allow the users to make use of stored passwords, give them all the comfort they usually have, they might "forget" they are "currently in private mode".

They may make mistakes, give out their identity (by allowing the use of a passwort), although their intention has been to "leave no tracks".


Maybe it's just me, but I'd prefer a clearer line to be drawn between regular mode and private mode.

Maybe it would be help not naming your intended mode "private" (which implies being anonymous in my understanding), but rather something like "leave no tracks mode on local system".


The private browsing mode starts with a "mask" icon. A mask, IMHO, means that other people I meet in the web will not see who I am (which is not your intention, as you say), unless I explicitly remove my mask (disable the mode). But even in private mode I can accidentally submit my stored passwords to sites.

I predict confused users.

Comment 43

[Kai Engert (:KaiE:)]

So, in order to fix this bug (SSL sites don't load),
we need a new patch, that uses the notifications or online/offline magic for networking as described earlier.

Ehsan, you said you have tried it and it worked.
Do you have a patch?


All my other concerns expressed in this bug are unrelated to the SSL failure.

Besides my general point of view what PB should be or not be, I think we agree that a new personal identification certificate obtained in PB mode is undesirable, because it can be seen as a super-cookie. I filed bug 475881.

Comment 44

[(no longer active)]

(In reply to comment #42)
> I don't understand the difference between 
>   I am surfing anonymously
> and
>   this web site does not know what I've searched earlier
>   (or: this site doesn't know I've been here before)
> 
> I would describe both statements as equivalent.

Anonymity is a much broader concept.  For example, it includes protection against traffic analysis, eavesdropping, etc.  See the Tor project for an example of a tool targeted at providing anonymity (and not privacy per se).

> That's correct. And IMHO, if you allow users to make use of their stored
> password in the private mode, it's another way to allow sites to know who you
> are.

We don't autofill the passwords, but they're available if the user name is selected via the autocomplete list.  This is the tradeoff between usability and safety.

> Maybe it would be help not naming your intended mode "private" (which implies
> being anonymous in my understanding), but rather something like "leave no
> tracks mode on local system".

This is exactly the description we're trying to convey via blog posts, about:privatebrowsing text, and Firefox user documentation.

> The private browsing mode starts with a "mask" icon. A mask, IMHO, means that
> other people I meet in the web will not see who I am (which is not your
> intention, as you say), unless I explicitly remove my mask (disable the mode).
> But even in private mode I can accidentally submit my stored passwords to
> sites.

The mask only protects you against other users of your computer.  As soon as the scope gets broader than your computer, we don't make any promises.

Comment 45

[(no longer active)]

(In reply to comment #43)
> So, in order to fix this bug (SSL sites don't load),
> we need a new patch, that uses the notifications or online/offline magic for
> networking as described earlier.
> 
> Ehsan, you said you have tried it and it worked.
> Do you have a patch?

Yes, I'm trying to come up with a unit test, and hopefully I will attach the patch to this bug later today.

> All my other concerns expressed in this bug are unrelated to the SSL failure.
> 
> Besides my general point of view what PB should be or not be, I think we agree
> that a new personal identification certificate obtained in PB mode is
> undesirable, because it can be seen as a super-cookie. I filed bug 475881.

Yes, we do.  Thanks for filing that bug.

Comment 46

[(no longer active)]

(In reply to comment #45)
> Yes, I'm trying to come up with a unit test, and hopefully I will attach the
> patch to this bug later today.

Actually, we can't test this automatically for now, because it requires closing down the current session and re-opening the new session which can't be done in the existing test frameworks.

Comment 47

[(no longer active)]

Attachment: Patch (v1)

Comment 49

[Marcia Knous [:marcia]]

It appears this bug is manifesting itself in a bit of a different way now. I noticed while running yesterday's nightly with a new profile that if I have an existing bugzilla bug in my regular browsing mode and then switch to Private Browsing, when I come back the Bugzilla bug does not show but instead shows an "Untitled Tab."  However, the URL is shown in the location bar.  

STR:
1. Using a new profile launch the browser.
2. Surf to bugzilla and look at a bug
3. Switch to PB mode
4. Immediately switch back to regular browsing mode

Expected: I would see all tabs
Actual: The bugzilla tab is labeled "Untitled" but the bug ID shows in the location bar.

Comment 50

[(no longer active)]

I'm positive that this is the same issue, but to make sure I've submitted a try server build with this patch so that you can see if the patch fixes this issue as well.  I'll post a link when it's available (the identifier string is "pbssl").

Comment 51

[(no longer active)]

Try server builds available at: <https://build.mozilla.org/tryserver-builds/2009-02-04_12:[e-mail]-pbssl/>

Comment 52

[Ed Lee :Mardak]

Initial tests seem to not have bug 476802 trigger for me. With and without the add-on. (Panic Mode)

Comment 53

[Marcia Knous [:marcia]]

In testing the Build in Comment 51 things look much better - I am not seeing the issue of bugzilla and other SSL sites not loading when I go back and forth between regular and PB modes.

Comment 54

[(no longer active)]

(In reply to comment #53)
> In testing the Build in Comment 51 things look much better - I am not seeing
> the issue of bugzilla and other SSL sites not loading when I go back and forth
> between regular and PB modes.

Are you implying that there are still problems regarding this bug in that build (by "much better")?

Comment 55

[Marcia Knous [:marcia]]

Sorry, should have been clearer. I did not see the issue in testing the tryserver build so I think that the patch resolves the issue I was seeing.  Despite repeated tries in and out of PB mode I did not notice trouble with SSL sites not loading.

Comment 56

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 359521 [details] [diff] [review]
Patch (v1)

My only concern here is that the offline setter triggers a bunch of notifications, and some listeners might not deal with the rapid change back and forth very well. Looking at the in-tree observers:

-download manager suspends all downloads, but then resumes them again (should be fine)
-nsXPInstallManager seems to shut everything down on the "going offline" notification, and doesn't ever start up again - this seems like an existing bug that could also be triggered by our automatic offline detection code
-ftp code seems to shut down open connections, but that's not likely to be a problem (since they should be reestablished if needed, I think)
-browser UI code looks like it will deal with the rapid change correctly

Comment 57

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #56)
> -nsXPInstallManager seems to shut everything down on the "going offline"

Mossop points out that this isn't a problem, because nsXPInstallManager isn't a service. A new instance is created for each download, so this just cancels any in-progress downloads.

Comment 58

[(no longer active)]

(In reply to comment #56)
> -download manager suspends all downloads, but then resumes them again (should
> be fine)

And toggling the private browsing code already does that.

Comment 59

[(no longer active)]

Kai: ping?  This has been waiting for your review for quite some time.  Would you please specify an ETA for this?

Comment 60

[(no longer active)]

Marcia: can you please test to see if you can reproduce this bug on a recent nightly (Minefield or Shiretoko)?

I was trying to write an automated unit test for this, and I noticed that the unit test is passing without this patch as well.  I tried reproducing this manually but couldn't do that even once.  I'm suspecting that this may be fixed as a result of bug 476463.  Can you please confirm?

Comment 61

[Nochum Sossonko [:Natch]]

Ehsan, this should probably still land as insurance, IMHO.

Comment 62

[(no longer active)]

(In reply to comment #61)
> Ehsan, this should probably still land as insurance, IMHO.

I totally agree.  I just wanted to make sure I'm not missing something obvious in my test.

Comment 63

[(no longer active)]

Attachment: Patch (v1) + test

Patch + test.

Requesting review on the test from mconnor.  Based on the previous comments, and since this change is specific to browser/, I'm not going to ask kaie for review on this patch, unless mconnor requires this.

Comment 64

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #60)
> Marcia: can you please test to see if you can reproduce this bug on a recent
> nightly (Minefield or Shiretoko)?

No, it's not fixed. I can see this either on OS X and Windows. The pages aren't loaded and untitled tabs are opened. After a restart everything is fine and the tabs have their content back.

Comment 65

[Mike Connor [:mconnor]]

Comment on attachment 364771 [details] [diff] [review]
Patch (v1) + test

This is fine, doesn't need kaie's review.

Comment 66

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/fc48221141fc

Comment 67

[Robert Sayre]

this failed TUnit on all three platforms in the download manager tests.

Comment 68

[Robert Sayre]

(backed out)

Comment 69

[(no longer active)]

Sample logs for investigation:

<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235970402.1235975768.15154.gz>
<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235970402.1235977222.17981.gz>
<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235970402.1235975122.13849.gz>

Comment 70

[Marcia Knous [:marcia]]

Ehsan: I still see this issue regularly in my testing, most especially when switching back from PB mode to regular with bugzilla bugs open.

(In reply to comment #60)
> Marcia: can you please test to see if you can reproduce this bug on a recent
> nightly (Minefield or Shiretoko)?
> 
> I was trying to write an automated unit test for this, and I noticed that the
> unit test is passing without this patch as well.  I tried reproducing this
> manually but couldn't do that even once.  I'm suspecting that this may be fixed
> as a result of bug 476463.  Can you please confirm?

Comment 71

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marcia, the patch on this bug had to be backed out. We should wait until the tests are updated and the patch will be checked-in again. Until then we have to live with that.

Comment 72

[(no longer active)]

I have not still debugged this thoroughly, but here is what seems to happen:

1. A download is created before entering the PB mode.
2. PB mode is initiated (SetOffline(TRUE/FALSE) here).
3. A download is created inside the PB mode.
4. PB mode is terminated (SetOffline(TRUE/FALSE) here).
5. A download is created after leaving the PB mode.
6. The download fails before entering the PB mode once again.  No SetOffline calls are made between 4 and 6.

Does anyone have any helpful suggestions?

Comment 73

[(no longer active)]

The error code BTW is NS_ERROR_CONNECTION_REFUSED.

Comment 74

[(no longer active)]

Attachment: Download manager test corrected

I can't believe I spent so much time debugging this while the problem was lying elsewhere!  The HTTP server was being stopped because of entering the offline mode.  Simply starting the server right before the third download fixes the test issue.  The other two downloads were from data URIs so the problem happened on the third download.

Comment 75

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As what I can see the SSL pages don't get loaded each second try. Means after leaving PB mode the first time, blank tabs are shown. Entering and leaving the PB mode again all the ssl pages show up correctly.

Comment 76

[Shawn Wilsher :sdwilsh]

Comment on attachment 365503 [details] [diff] [review]
Download manager test corrected

>+        // Prevent any SSL sockets from remaining open (bug 463256)
>+        let ios = Cc["@mozilla.org/network/io-service;1"].
>+                  getService(Ci.nsIIOService);
>+        if (!ios.offline) {
>+          ios.offline = true;
>+          ios.offline = false;
>+        }
Can you add a better comment here please?  Just referencing the bug is nice, but then someone has to read this whole bug to figure out why the test is that way, whereas a description in here that provides a more detailed summary means nobody has to go read the bug in most cases.

>+++ b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sslsite_transition.js
Not sure I'm a suitable peer for this (or any of this patch really)

>+ * The Initial Developer of the Original Code is
>+ * Ehsan Akhgari.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Ehsan Akhgari <[e-mail]> (Original Author)
You are the original developer, so you don't need to list yourself here as well

r=sdwilsh, but you'll likely need a browser peer to look at this as well.

Comment 77

[(no longer active)]

Thanks for your review, Shawn.

(In reply to comment #76)
> Not sure I'm a suitable peer for this (or any of this patch really)

Actually, I asked your review on the download manager test changes; as the rest of this patch had already mconnor's r+.  Sorry for the confusion.

I'll address your comments upon landing.

Comment 78

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #76)
> >+ * Contributor(s):
> >+ *   Ehsan Akhgari <[e-mail]> (Original Author)
> You are the original developer, so you don't need to list yourself here as well

It makes sense to list yourself in both places, since it's not practical to include an email address in the "original developer" line.

Comment 79

[(no longer active)]

(In reply to comment #78)
> (In reply to comment #76)
> > >+ * Contributor(s):
> > >+ *   Ehsan Akhgari <[e-mail]> (Original Author)
> > You are the original developer, so you don't need to list yourself here as well
> 
> It makes sense to list yourself in both places, since it's not practical to
> include an email address in the "original developer" line.

Yeah, that was why I did that.

Comment 80

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As what I've seen by writing a MozMill test for entering/leaving the Private Browsing mode, all pages of a domain aren't loaded after we leave the mode. Means even opening new tabs afterward and loading different pages result in an empty tab. Only the location bar shows the correct URL while the tab shows an untitled tab. Pages located on other domains (which weren't open when entering PB mode) can be loaded via HTTPS.

Comment 81

[(no longer active)]

It does depend on timing as well, right?  IOW, if you wait some time before attempting to load the SSL sites again, it does work, isn't it?  (Because Necko should drop the SSL sockets on its own after some time, AFAIK)

Comment 82

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ah ok. Yes, that works. I've tested after 2 minutes again and I'm able to reload the page. Thanks for this note.

Comment 83

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/113bda3be8df

Henrik, can you please run the MozMill (and manual) tests that you have with a build containing this fix, and let us know what you see?

Comment 84

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Pulled from m-c and updated my debug build 4 minutes ago. I did a testrun with my test and it doesn't work. HTTPS sites are still not loaded. I would wait for the next nightly and test there again. I'll reopen if it is still not fixed.

Comment 85

[(no longer active)]

Do you see the failure on Mac?  Can you please test this across all platforms?  It seems like the Mac unit test failed as well, but I made sure that Windows and Linux tests pass...

Comment 86

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't have debug builds on Windows or Linux. So I cannot test it until official nightly builds are available.

Failure of unit test:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sslsite_transition.js | Timed out

Something timed out here.

Comment 87

[(no longer active)]

Backed out due to Mac test failures:

<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236848594.1236851651.20213.gz>

Comment 88

[(no longer active)]

Henrik, sadly this patch didn't last long enough in the tree in order to make it into the nightly.  Would it be possible to you to apply the patch manually to your builds and try it out?

Comment 89

[Jim Jeffery not reading bug-mail 1/2/11]

While I don't have any way to run tests locally, I can say with good confidence after testing with an hourly build that this patch does fix the issue of https pages not loading after exit from PB. 

Vista HP SP2 RC
Hourly build:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre Firefox/3.0.4 ID:20090312044012
changeset: http://hg.mozilla.org/mozilla-central/rev/69322c1764ff

Comment 90

[(no longer active)]

Thanks for testing this, Jim.  Can you test it on Mac as well (because the unit test problem was Mac specific)?  I have just uploaded a try server patch, which should generate builds in a few hours.  I'll be asleep by then, but I've tagged the builds with "pbssl" so that you can find them by keeping an eye on this page: <https://build.mozilla.org/tryserver-builds/?C=M;O=D>

Comment 91

[Jim Jeffery not reading bug-mail 1/2/11]

No, sorry.. I don't have access to a Mac.

Comment 92

[Marcia Knous [:marcia]]

Ehsan: I will keep an eye out for the build and test it on the mac when it comes out.

Comment 93

[Nochum Sossonko [:Natch]]

Ehsan, the timeout in the test may be due to the fact that you register the listener _after_ you load the page the second and third time (note for the first load you register the load listener before, correctly). Just a thought...

Comment 94

[(no longer active)]

(In reply to comment #93)
> Ehsan, the timeout in the test may be due to the fact that you register the
> listener _after_ you load the page the second and third time (note for the
> first load you register the load listener before, correctly). Just a thought...

The problem is, before changing |pb.privateBrowsingEnabled|, we don't have access to the tab to attach the listener to, so attaching the listener before transitioning the private browsing mode won't really help.

Based on comment 84 from Henrik, I think the real problem here might be that the bug is not fixed at all on Mac.  I have verified the fix on both Windows and Linux using the simple steps to reproduce from comment 6 and the like, but I really need someone with a Mac to verify the fix manually so that we know which part is guilty for this failure, the code or the test itself.

Marcia: did you manage to test the build?  It's available from <https://build.mozilla.org/tryserver-builds/2009-03-12_15:[e-mail]-pbssl/>, FWIW.

One other question: Henrik, do you have complete MozMill tests for this bug?  If yes, and if those tests would be run automatically, then I'd be OK with removing the unit test from this patch and landing it without the tests, that is of course if we can be sure that the problem is really fixed on all platforms.

Comment 95

[(no longer active)]

In fact, I'm thinking that maybe a better automated test would be an xpcshell test which verifies that the offline status change notifications are being observed when transitioning the private browsing mode, accompanied by Litmus and MozMill tests.

Comment 96

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The try server build works fine for me. Looks like I hadn't the right changeset yesterday while testing it with a debug build. Or did you made other changes in-between?

You should definitely add an automated test here. MozMill isn't integrated in the build system yet. So it makes test results very unreliable. We don't run them at all in a regularly basis for now.

Comment 97

[Marcia Knous [:marcia]]

I ran the try server build today on Mac with no issues. The most common site that doesn't load for me is bugzilla, and when I switched back and forth between PB the bugzilla pages loaded fine for me, as well as other pages such as Citibank and British airways.  So I would say I can verify that I no longer see the issue on mac using the tryserver build.

Comment 98

[Nochum Sossonko [:Natch]]

(In reply to comment #94)
> The problem is, before changing |pb.privateBrowsingEnabled|, we don't have
> access to the tab to attach the listener to, so attaching the listener before
> transitioning the private browsing mode won't really help.

Which is why it may still be necessary to use reuse_same_session (the pref that doesn't close the tabs/windows). If you want to test this, you can put in dumps around the event registration and in the handlers themselves and see if they're being called correctly. I can't really see any other reason why the test would timeout (i.e. not fail, but timeout)...

Comment 99

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #97)
> I ran the try server build today on Mac with no issues. 

OK

> The most common site that doesn't load for me is bugzilla, 

Do you mean that doesn't load in builds without this fix?  
Or do you mean that still doesn't load with this fix?

> and when I switched back and forth between PB the bugzilla pages 

What's PB?

> loaded fine for me, as well as other pages such as Citibank and British 
> airways.  So I would say I can verify that I no longer see the issue on 
> mac using the tryserver build.

Comment 100

[Nelson Bolyard (seldom reads bugmail)]

> What's PB?
D'oh!    Need more coffee.  :-/

Comment 101

[Marcia Knous [:marcia]]

Nelson: With the tryserver build, bugzilla pages load fine when I switch back to Private Browsing mode from regular browsing. This was not happening with the nightlies and Beta 3 builds, as when I switched back I was getting an Untitled tag and had to hit the arrow icon in the URL bar several times to get the page to reload.  So all is well with this tryserver build.

[snip]
> 
> > The most common site that doesn't load for me is bugzilla, 
> 
> Do you mean that doesn't load in builds without this fix?  
> Or do you mean that still doesn't load with this fix?
> 
> > and when I switched back and forth between PB the bugzilla pages 
> 
> What's PB?
> 
> > loaded fine for me, as well as other pages such as Citibank and British 
> > airways.  So I would say I can verify that I no longer see the issue on 
> > mac using the tryserver build.

Comment 103

[(no longer active)]

(In reply to comment #98)
> Which is why it may still be necessary to use reuse_same_session (the pref that
> doesn't close the tabs/windows). If you want to test this, you can put in dumps
> around the event registration and in the handlers themselves and see if they're
> being called correctly. I can't really see any other reason why the test would
> timeout (i.e. not fail, but timeout)...

Yeah, I think you're right.  I was hesitant to use that pref in order for the test to get the situation exactly like the real world, but I guess that won't be an option now.

Comment 104

[(no longer active)]

I think I have an idea about the source of the timeout issue with the test for this bug.  I think the reason is that the Mochitest HTTP server cannot handle turning on the offline mode while it's running.  I found out that a simple test which tried to open https://example.com/ was consistently failing with a network timeout error on my local build here, and I finally came to realize that it seems like it takes the Mochitest server a bit of time to get back on its feet once the offline mode has been toggled, and during that time, loading the SSL page fails.

Is there any sane way to handle this?

Comment 105

[(no longer active)]

Attachment: Patch with reworked tests

The tests in this patch have been reworked, and pass for me locally.  I have also added a xpcshell test to make sure the offline mode is changed as expected.

Comment 106

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, can you start a try server build? That will give us a chance to see if the tests pass.

Comment 107

[(no longer active)]

(In reply to comment #106)
> Ehsan, can you start a try server build? That will give us a chance to see if
> the tests pass.

Sure, but the try server seems largely broken right now, so I'm waiting for things to get into a better shape before doing this.  I'll update here once I get results from the try server.

Comment 108

[(no longer active)]

OK, I still get failures with this test every now and then locally.  :-(

It seems like this is a timing issue, but I haven't figured out the cause yet.  Switching executeSoon with a setTimeout of 1000ms seems to solve the test failure completely (no failure in ~100 runs).  But I'm hesitant to modify the test to be based on a timeout.

Frankly, I'm out of ideas to move forward with here.  Any suggestions?

Comment 110

[Mike Connor [:mconnor]]

Comment on attachment 369297 [details] [diff] [review]
Patch with reworked tests

Looks good, sorry for the delay on the review.

Comment 111

[(no longer active)]

(In reply to comment #110)
> (From update of attachment 369297 [details] [diff] [review])
> Looks good, sorry for the delay on the review.

Like I said in comment 108, the browser chrome test still fails intermittently on my own local box, but the xpcshell test seems to be fine.  I'd like to land this patch with the browser chrome test disabled for now, until we figure out what's wrong with that test.  Are you OK with this, Mike?

Comment 112

[Mike Connor [:mconnor]]

Yeah, let's do that for now, and file a followup on getting that test debugged further?

Comment 113

[(no longer active)]

(In reply to comment #112)
> Yeah, let's do that for now, and file a followup on getting that test debugged
> further?

Filed bug 486640 for that.

http://hg.mozilla.org/mozilla-central/rev/1886b176f000

Comment 114

[Boris Zbarsky [:bzbarsky]]

This was backed out due to test failures.  See bug 486655.

Comment 115

[(no longer active)]

Relanded with the fix to bug 486655:

http://hg.mozilla.org/mozilla-central/rev/3f5521e1c5a3

Comment 116

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, seems like we don't have to backout again. I run a test on OS X and Windows by switching a couple of times between both modes with SSL pages open in several tabs and windows. All content is restored now when leaving the PB mode.

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090403 Minefield/3.6a1pre ID:20090403030700

Ehsan, do we still need a litmus test? I don't think so.

Comment 117

[(no longer active)]

(In reply to comment #116)
> Ok, seems like we don't have to backout again. I run a test on OS X and Windows
> by switching a couple of times between both modes with SSL pages open in
> several tabs and windows. All content is restored now when leaving the PB mode.

w00t!

> Ehsan, do we still need a litmus test? I don't think so.

Oh yes, we definitely do.  The current automated test doesn't actually test the problem observed in comment 0, so we need a Litmus test for it at least as long as bug 486640 is not fixed.

Comment 118

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/012518661356

Comment 119

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090404 Shiretoko/3.5b4pre ID:20090404035045 and on Windows too.

Marcia, can you add the Litmus test for now?

Comment 120

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7643 added to Litmus.

Comment 121

[(no longer active)]

(In reply to comment #120)
> https://litmus.mozilla.org/show_test.cgi?id=7643 added to Litmus.

This bug is time-sensitive, in that the entering and exiting the private browsing mode should happen pretty fast.  If, for example, the user leaves his browser between these two tasks for a few minutes, then the test would pass even on a broken build.  Therefore we need to incorporate a time limit in the Litmus test case.

I think mentioning that the tester should end the private browsing mode right away is enough.  Sorry for being picky.  :-)

Comment 122

[Marcia Knous [:marcia]]

No problem, QA is all about being picky :) - I have updated the case accordingly.

(In reply to comment #121)
> (In reply to comment #120)
> > https://litmus.mozilla.org/show_test.cgi?id=7643 added to Litmus.
> 
> This bug is time-sensitive, in that the entering and exiting the private
> browsing mode should happen pretty fast.  If, for example, the user leaves his
> browser between these two tasks for a few minutes, then the test would pass
> even on a broken build.  Therefore we need to incorporate a time limit in the
> Litmus test case.
> 
> I think mentioning that the tester should end the private browsing mode right
> away is enough.  Sorry for being picky.  :-)

Comment 123

[(no longer active)]

(In reply to comment #122)
> No problem, QA is all about being picky :) - I have updated the case
> accordingly.

Thanks, the test looks great now!

Comment 124

[Marcia Knous [:marcia]]

Removing relnote keyword as I have not seen this in my testing since it was fixed.

Comment 125

[Jason Duell]

From the discussion in bug 480619, it sounds like we might be able to redo this fix with a much lighter touch (i.e. w/o restarting necko).  See the 54th comment of that bug. (is there a way in comments like this to link to a numbered comment in a different bug?).

If so, I assume the process is to open a new bug (even though the eventual fix would probably involve rolling back much or all of the patch here).

Comment 126

[Mike Connor [:mconnor]]

the format you want is bug 480619 comment 54 

yes, please open a new bug to work on a better solution

Comment 127

[(no longer active)]

(In reply to comment #125)
> From the discussion in bug 480619, it sounds like we might be able to redo this
> fix with a much lighter touch (i.e. w/o restarting necko).  See the 54th
> comment of that bug. (is there a way in comments like this to link to a
> numbered comment in a different bug?).
> 
> If so, I assume the process is to open a new bug (even though the eventual fix
> would probably involve rolling back much or all of the patch here).

Filed bug 496335 about this.

Comment 128

[Nelson Bolyard (seldom reads bugmail)]

The changes made for this bug are now suspected of causing bug 503418,
and/or bug 489880.

Comment 129

[(no longer active)]

(In reply to comment #128)
> The changes made for this bug are now suspected of causing bug 503418,
> and/or bug 489880.

These changes were effectively backed out as part of the fix for bug 496335.  We'll probably take that fix on 1.9.1 branch as well.

Comment 130

[Nelson Bolyard (seldom reads bugmail)]

I was mistaken,  It is more likely that the "fix" for bug 496335 is causing
bug 503418, and/or bug 489880.  That problem is seen only on the trunk 
where the "fix" for bug 496335 has been committed.