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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Security: PSM
P2
normal
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: marcia, Assigned: Ehsan)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.2a1
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Summary: Sites sometimes don't load after re-entering regular browsing mode (from PB) → Sites sometimes don't load after re-entering regular browsing mode (from Private Browsing)
(Reporter)

Comment 1

9 years ago
I see this happening when entering PB mode as well as when exiting it.
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.
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?
Blocks: 248970
(Reporter)

Comment 4

9 years ago
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.
(Reporter)

Comment 5

9 years ago
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
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?
(Reporter)

Comment 7

9 years ago
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

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

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

Comment 9

9 years ago
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.
(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.
Keywords: helpwanted

Comment 11

9 years ago
(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.
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?
Flags: blocking-firefox3.1?
OS: Mac OS X → All
Hardware: PC → All
(Reporter)

Updated

9 years ago
Summary: Sites sometimes don't load after re-entering regular browsing mode (from Private Browsing) → SSL sites sometimes don't load after re-entering regular browsing mode (from Private Browsing)
Created attachment 348328 [details] [diff] [review]
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

9 years ago
Could we be hitting the following silent return in nsSHistory::LoadEntry: <http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHistory.cpp#1175>?
(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.
(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?
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.
Assignee: nobody → ehsan.akhgari
Keywords: helpwanted
Target Milestone: --- → Firefox 3.1b2
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>.
Status: NEW → ASSIGNED
Blocking, but not blocking beta 2.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Target Milestone: Firefox 3.1b2 → Firefox 3.1
(Reporter)

Comment 20

9 years ago
adding relnote keyword...
Keywords: relnote
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.  :-)
Component: Places → Security: PSM
Flags: blocking-firefox3.1+
Keywords: relnote
Product: Firefox → Core
QA Contact: places → psm
Target Milestone: Firefox 3.1 → mozilla1.9.1
blocking-firefox-3.1+ got lost
Flags: blocking1.9.1?
Keywords: relnote
Kai is your friend here, I think.
Kai: ping?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Keywords: helpwanted
Kai, can you please have a look at comment 21?
Whiteboard: [needs status update kaie]

Comment 26

9 years ago
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

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

Comment 28

9 years ago
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"?
Whiteboard: [needs status update kaie]

Comment 29

9 years ago
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

9 years ago
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

9 years ago
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.
(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

9 years ago
(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.)
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?
(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

9 years ago
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

9 years ago
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

9 years ago
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.
(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

9 years ago
(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.
(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

9 years ago
(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

9 years ago
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.
(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.
(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.
(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.
Flags: in-litmus?
Keywords: helpwanted
Created attachment 359521 [details] [diff] [review]
Patch (v1)
Attachment #348328 - Attachment is obsolete: true
Attachment #359521 - Flags: review?(kaie)
Attachment #359521 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][needs review gavin][needs review kaie]
Duplicate of this bug: 476802
(Reporter)

Comment 49

9 years ago
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.
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").
Try server builds available at: <https://build.mozilla.org/tryserver-builds/2009-02-04_12:13-ehsan.akhgari@gmail.com-pbssl/>

Comment 52

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

Comment 53

9 years ago
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.
(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")?
(Reporter)

Comment 55

9 years ago
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 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
Attachment #359521 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin][needs review kaie] → [has patch][needs review kaie]
(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.
(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.
Kai: ping?  This has been waiting for your review for quite some time.  Would you please specify an ETA for this?
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?
Ehsan, this should probably still land as insurance, IMHO.
(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.
Created attachment 364771 [details] [diff] [review]
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.
Attachment #359521 - Attachment is obsolete: true
Attachment #364771 - Flags: review?(mconnor)
Attachment #359521 - Flags: review?(kaie)
(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 on attachment 364771 [details] [diff] [review]
Patch (v1) + test

This is fine, doesn't need kaie's review.
Attachment #364771 - Flags: review?(mconnor) → review+
http://hg.mozilla.org/mozilla-central/rev/fc48221141fc
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][needs review kaie] → [needs 1.9.1 landing]
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1

Comment 67

8 years ago
this failed TUnit on all three platforms in the download manager tests.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 68

8 years ago
(backed out)
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>
Whiteboard: [needs 1.9.1 landing]
(Reporter)

Comment 70

8 years ago
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?
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.
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?
The error code BTW is NS_ERROR_CONNECTION_REFUSED.
Created attachment 365503 [details] [diff] [review]
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.
Attachment #364771 - Attachment is obsolete: true
Attachment #365503 - Flags: review?(sdwilsh)
Status: REOPENED → ASSIGNED
Whiteboard: [has patch][needs review sdwilsh]
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 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 <ehsan.akhgari@gmail.com> (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.
Attachment #365503 - Flags: review?(sdwilsh) → review+

Updated

8 years ago
Whiteboard: [has patch][needs review sdwilsh] → [has patch]
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.
(In reply to comment #76)
> >+ * Contributor(s):
> >+ *   Ehsan Akhgari <ehsan.akhgari@gmail.com> (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.
(In reply to comment #78)
> (In reply to comment #76)
> > >+ * Contributor(s):
> > >+ *   Ehsan Akhgari <ehsan.akhgari@gmail.com> (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.
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.
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)
Ah ok. Yes, that works. I've tested after 2 minutes again and I'm able to reload the page. Thanks for this note.
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Whiteboard: [needs 1.9.1 landing]
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.
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...
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.
Backed out due to Mac test failures:

<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236848594.1236851651.20213.gz>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs 1.9.1 landing]
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?
Whiteboard: [PB-P2]
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
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>
No, sorry.. I don't have access to a Mac.
(Reporter)

Comment 92

8 years ago
Ehsan: I will keep an eye out for the build and test it on the mac when it comes out.
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...
(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:31-ehsan.akhgari@gmail.com-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.
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.
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.
Status: REOPENED → ASSIGNED
(Reporter)

Comment 97

8 years ago
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.
(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)...
(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.
> What's PB?
D'oh!    Need more coffee.  :-/
(Reporter)

Comment 101

8 years ago
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.
Duplicate of this bug: 483773
(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.
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?
Created attachment 369297 [details] [diff] [review]
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.
Attachment #365503 - Attachment is obsolete: true
Attachment #369297 - Flags: review?(mconnor)
Whiteboard: [PB-P2]
Ehsan, can you start a try server build? That will give us a chance to see if the tests pass.
(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.
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?
Duplicate of this bug: 486217
Comment on attachment 369297 [details] [diff] [review]
Patch with reworked tests

Looks good, sorry for the delay on the review.
Attachment #369297 - Flags: review?(mconnor) → review+
(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?
Yeah, let's do that for now, and file a followup on getting that test debugged further?
Depends on: 486640
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Depends on: 486655
This was backed out due to test failures.  See bug 486655.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded with the fix to bug 486655:

http://hg.mozilla.org/mozilla-central/rev/3f5521e1c5a3
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
(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.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/012518661356
Keywords: fixed1.9.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?
Keywords: fixed1.9.1 → verified1.9.1
(Reporter)

Comment 120

8 years ago
https://litmus.mozilla.org/show_test.cgi?id=7643 added to Litmus.
Flags: in-litmus? → in-litmus+
(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.  :-)
Flags: in-litmus+ → in-litmus?
(Reporter)

Comment 122

8 years ago
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.  :-)
(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!
Flags: in-litmus? → in-litmus+
Depends on: 488772
(Reporter)

Comment 124

8 years ago
Removing relnote keyword as I have not seen this in my testing since it was fixed.
Keywords: relnote
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).
the format you want is bug 480619 comment 54 

yes, please open a new bug to work on a better solution
Depends on: 496335
(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.
The changes made for this bug are now suspected of causing bug 503418,
and/or bug 489880.
(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.
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.
You need to log in before you can comment on or make changes to this bug.