476463 - Cookies set onunload of page are retained on exit/enter of PB mode

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[alex_mayorga]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090202 Shiretoko/3.1b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090202 Shiretoko/3.1b3pre

Problem is that there are some cookies retained even when entering "Private Browsing", in my case coolies for delicious.com and forums.mozillazine.org show right after entering "Private Browsing" via Tools -> Start Private Browsing per the spec at https://wiki.mozilla.org/Firefox3.1/PrivateBrowsing/FunctionalSpec#Cookies
"On entry:

    * Write cookies to disk, drop the in-memory hashtable."
There are others seeing this (or similar) at http://forums.mozillazine.org/viewtopic.php?f=23&t=1066095

Reproducible: Always

Steps to Reproduce:
1. Visit http://forums.mozillazine.org/
2. Tools->Start Private Browsing
3. Notice Tools->Options->Privacy->Show Cookies shows a cookie from forums.mozillazine.org
Actual Results:  
Tools->Options->Privacy->Show Cookies shows a cookie from forums.mozillazine.org

Expected Results:  
Tools->Options->Privacy->Show Cookies shows no cookies

Comment 1

[Steve England [:stevee]]

This is also reproducible on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090202 Minefield/3.2a1pre

I'm not sure if this is enough of a privacy risk for the privacy keyword since we have a cookie that was set in Non-Private-Browsing-Mode being present in PBM, rather than a cookie set in PBM not being removed for Non-PBM. Maybe it could work the other way tho?

Comment 2

[Nochum Sossonko [:Natch]]

IIRC this was a privacy problem because it enabled detecting if the user is in private browsing mode, although exactly how I can't remember atm.

Comment 3

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

(In reply to comment #0)
 "On entry:
> 
>     * Write cookies to disk, drop the in-memory hashtable."

> Steps to Reproduce:
> 1. Visit http://forums.mozillazine.org/
> 2. Tools->Start Private Browsing
> 3. Notice Tools->Options->Privacy->Show Cookies shows a cookie from
> forums.mozillazine.org
> Actual Results:  
> Tools->Options->Privacy->Show Cookies shows a cookie from
> forums.mozillazine.org
> 
> Expected Results:  
> Tools->Options->Privacy->Show Cookies shows no cookies

Those expected results are not inconsistent with the spec text you quoted. "Write cookies to disk" means that we flush the in-memory hashtable of cookies when you enter private browsing mode, and refuse to persist cookies from then on. It doesn't mean that we delete existing cookies - you can do that from the "Clear Recent History" button that's offered on the "Welcome Private Browsing" page (or from the Tools menu).

I think this bug is INVALID.

Comment 4

[alex_mayorga]

(In reply to comment #3)
> (In reply to comment #0)
> Those expected results are not inconsistent with the spec text you quoted.
> "Write cookies to disk" means that we flush the in-memory hashtable of cookies
> when you enter private browsing mode, and refuse to persist cookies from then
> on. It doesn't mean that we delete existing cookies - you can do that from the
> "Clear Recent History" button that's offered on the "Welcome Private Browsing"
> page (or from the Tools menu).
> 
> I think this bug is INVALID.

So "drop the in-memory hashtable." doesn't mean that there should be 0 cookies while entering PBM?

Comment 5

[Nochum Sossonko [:Natch]]

See bug 467290 comment 7 and others. If the cookies are available to the site
after entering PB mode, that definitely is a bug, if it just shows up in the
Cookie Manager, that isn't a problem, afaict.

Comment 6

[Nochum Sossonko [:Natch]]

It seems like cookies are retained both when entering and exiting PB mode. The reason seems to be the same, the cookie content is null and that seems to prevent the removal of the cookie. You'll see if you enter PB mode with a mozillazine cookie only one of them will be kept in the Cookie Manager, the styl_display cookie, which is null. Similarly when exiting PB mode the cookie remains. It could also be that the cookie is set onunload (or some similar page closing event) which PB fails to remove. Either way this certainly needs more investigation. Raising severity and adding privacy keyword, as this is definitely worse than it seemed to be at the start. I'm still uncertain if the page can actually access the cookie, but it's definitely possible to see what a user has done in PB mode.

Comment 7

[Nochum Sossonko [:Natch]]

Sorry for the bugspam, this was observed on:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre

STR:

1) Load forums.mozillazine.org
2) Enter PB Mode
3) Check the Cookie Manager

and

1) Clear all cookies
2) Enter PB mode while *not* on forums.mozillazine.org
3) go to forums.mozillazine.org
4) Exit PB Mode
5) Check the Cookie Manager

Comment 8

[Nochum Sossonko [:Natch]]

Attachment: testcase

Comment 9

[Nochum Sossonko [:Natch]]

The text is wrong in the testcase, the value of the cookie doesn't matter, only the fact that it is set onunload. This enables sites to distinguish reliably between PB and non-PB sessions, by detecting only that cookie without the others. It also leaves a significant trace.

Comment 10

[Nochum Sossonko [:Natch]]

Attachment: testcase 2

Essentially the problem is that PB enters and exits before it gets rid of the current session. So in essence this problem exists for all the aspects of pb, e.g. if the page has an onunload handler that throws, the console will still show it. I think PB has to be entered and exited after the windows are closed and before about:privatebrowsing is loaded. Similarly (and this seems to be the hard part) it needs to exit after the private session's pages are unloaded but before the new session is reloaded. Attached is a sample file, when used with the STR of comment 7 it can detect when one is in pb mode or not. This is an oversimplification and would be much more efficient (and probably 100% correct) if used in conjunction with server-side storage. It also demonstrates the error console problem.

Comment 11

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

(mconnor totally volunteered by marking this blocking)

Comment 12

[(no longer active)]

As Natch states in comment 10, the private browsing service sends the private-browsing notification before closing down the session and re-opening <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#304>.  So, the correct point to do this should be here: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#166>.

This is a major change which should be handled with great care, because some of the stuff in the service may depend on the specific ordering in these calls.  I'm going to give it a shot soon if mconnor doesn't beat me to it with a patch.

Comment 13

[Nochum Sossonko [:Natch]]

Should this be relnote'd for beta 3?

Comment 14

[(no longer active)]

(In reply to comment #13)
> Should this be relnote'd for beta 3?

Yes, thanks for noting this.

Comment 15

[(no longer active)]

Attachment: WIP

This should work in theory, but it doesn't.

This patch makes sure that before transitioning the private browsing mode, all current windows and tabs are closed and a blank session is opened.  The rest of the session management (opening about:privatebrowsing when entering the private browsing mode, and restoring the previous session when leaving this mode) is done after the private-browsing notification is sent.

The problem seems to be that the onunload events fire asynchronously, IOW, the mere fact that a tab is closed does not guarantee that the respective onunload event has fired.

Does anybody know why this happens, and what can I do to make it work synchronously (or alternatively determine when all unonload events have been fired)?

Comment 16

[(no longer active)]

Simon, can you also take a look at this patch please?

Comment 17

[Nochum Sossonko [:Natch]]

Attachment: Proto

Ehsan, I think something like this would be necessary. This patch fixes this bug for me.

Comment 18

[Nochum Sossonko [:Natch]]

Attachment: Proto 2

This is actually a better impl. This loads about:privatebrowsing on enter, waits for it to load, then notifies the observers. On exit a blank "dummy" page is restored, the observers are notified, then the old session is restored. I'm still not 100% sure this is wholly correct, but this could definitely be a starting point.

Note that lots of tests fail because of the asynchronous change (i.e. setting privateBrowsingEnabled returns without it being actually entered), but that can be changed. With this style, callers would have to set the private browsing bit, then wait for the notification. i also ran into bug 463256 I lot more frequently, but your patch there helped me get the false-positive testing out of the way.

Comment 19

[(no longer active)]

(In reply to comment #18)
> Note that lots of tests fail because of the asynchronous change (i.e. setting
> privateBrowsingEnabled returns without it being actually entered), but that can
> be changed. With this style, callers would have to set the private browsing
> bit, then wait for the notification.

This is not good at all API-wise, and I'm not sure if it would be appropriate to take for 1.9.1 at this stage.  And even if it were, I'm not sure if I'm comfortable with complicating the API this way.

We need to do something to match the behavior of your patch synchronously.

Simon, nsISessionStore::setBrowserState works synchronously, right?  Why is that the unload even for the previous tabs is not fired before it returns?

Comment 20

[Simon Bünzli]

Comment on attachment 361069 [details] [diff] [review]
WIP

>-  _onPrivateBrowsingModeChanged: function PBS__onPrivateBrowsingModeChanged() {
>+          let newState = {windows: [{tabs: [{entries: [{url: "about:blank"}]}]}]};
>+          if (oldState.sizemode)
>+            newState.sizemode = oldState.sizemode;

sizemode is a property of the window, not the state itself.

>+          newState.windows[0].tabs[0].entries[0].url = "about:privatebrowsing";
>+          ss.setBrowserState(JSON.stringify(newState));

Any reason you couldn't just load about:privatebrowsing in the blank tab opened above?

(In reply to comment #19)
> Simon, nsISessionStore::setBrowserState works synchronously, right?

Not entirely. Closing all tabs but the one to be reused does happen synchronously, but loading the new tab (and thus overwriting the old one) won't.

One possible low-impact solution to this issue would be to open as many new tabs as required in sss_restoreWindow and close all the existing ones afterwards. Then, at least closing all the existing tabs should happen in sync.

Comment 21

[Nochum Sossonko [:Natch]]

Attachment: Proto 3

Here is an updated patch with all the comments addressed. This passes all the unit and mochi tests.

I'll add some tests for this bug shortly.

Comment 22

[Simon Bünzli]

Comment on attachment 361323 [details] [diff] [review]
Proto 3

A quick review from my SessionStore perspective:

>+          "_closedTabs": [],

Nice catch. We should clear these inside SessionStore, though.

>+          "selected": 1,

This and "selectedWindow" aren't needed, as they have sane defaults.

>+          "sizemode": "maximized",

This'll maximize even non-maximized windows, which we don't want.

>+                      getMostRecentWindow("navigator:browser").getBrowser();

/getBrowser()/gBrowser/ (getBrowser() has been deprecated in Firefox 3.1).

>+                "title": "Would you like to start Private Browsing?",

This isn't l10n-friendly, at all. Besides, about:privatebrowsing will set that title itself once it's loaded.

>+              "attributes": {
>+                "image": "chrome://global/skin/icons/question-16.png",

This is just a place where changing the icon will get forgotten in the future.

Comment 23

[Nochum Sossonko [:Natch]]

>Nice catch. We should clear these inside SessionStore, though.
Not sure what that means, should I restore and then clear these with
deleteWindowValue? That means a bit more work, but is that preferred?

(in reply to the rest of the comment)
So I should just restore the url itself, right? Oh, and sizemode, only I'll have to keep it the same value as it was plus add the height/width.

Comment 24

[Simon Bünzli]

(In reply to comment #23)
> Not sure what that means

For and and this specific patch, it doesn't mean anything at all. We should just later on fix bug 477657 and then remove that one line again (or fix bug 477657 first and omit that line).

> So I should just restore the url itself, right?

Right. As for sizemode, it'd be significantly saner to just include the sizemode related changes from my patch in bug 477657 into yours so the PB service doesn't have to care about it.

Comment 25

[Nochum Sossonko [:Natch]]

Attachment: Patch for review

Patch + Test. I can't tell for sure if the test works because without this patch you can't test privatebrowsing without setting keep_current_session. I'll try testing it later on today with a modified privatebrowsing service.

Comment 26

[Simon Bünzli]

Comment on attachment 361374 [details] [diff] [review]
Patch for review

>+      // dummy session used to transition from/to pb mode, see bug 476463
>+      let transitionState = {

Any particular reason this isn't right before where it's needed?

>+        // this ensures a clean slate from which to transition into or out of pb
>+        browser.addTab();

Couldn't you already load about:privatebrowsing at this very point? If not, it should at lest be enough to load about:privatebrowsing into the tab you've added (passing it forward) instead of the following SessionStore call:

>+        // Transition into private browsing mode
>+        ss.setBrowserState(JSON.stringify(privateBrowsingState));

>+		unload_test.html \

browser_privatebrowsing_unload_test.html ?

r+=me for the SessionStore related bits.

Comment 27

[Nochum Sossonko [:Natch]]

(In reply to comment #26)
> (From update of attachment 361374 [details] [diff] [review])
> >+      // dummy session used to transition from/to pb mode, see bug 476463
> >+      let transitionState = {
> 
> Any particular reason this isn't right before where it's needed?
No, it was just an oversight from a different approach, I'll fix it with the next patch.

> >+        // this ensures a clean slate from which to transition into or out of pb
> >+        browser.addTab();
> 
> Couldn't you already load about:privatebrowsing at this very point? If not, it
> should at lest be enough to load about:privatebrowsing into the tab you've
> added (passing it forward) instead of the following SessionStore call:
Ok, here the order of events is a bit complicated, I have that dummy session because I need to have a clean browser on which I can make assumptions (specifically that there is only one tab). After that I *must* call removeTab to get all the relevant events to fire which gives us a clean state. In order to call removeTab and not kill the browser I first call addTab. The _onPrivateBrowsingModeChanged makes the decision on what comes next (there's a lot of conditions involved that the code watches out for). I *can* change it to loading about:privatebrowsing right in _onBefore[snip] but that would result in tedious ifs and elses. What I should probably do is in _onPrivate[snip] just load about:privatebrowsing in the tab instead of going through sessionstore, which I'll fix in the next patch if you're ok with that.

> >+        // Transition into private browsing mode
> >+        ss.setBrowserState(JSON.stringify(privateBrowsingState));
> 
> >+		unload_test.html \
> 
> browser_privatebrowsing_unload_test.html ?
No, this page is loaded from the .js page, this by itself isn't of much use and doesn't test anything.

> r+=me for the SessionStore related bits.

Thanks for the review!

Comment 28

[Nochum Sossonko [:Natch]]

>tedious ifs and elses. What I should probably do is in _onPrivate[snip] just
>load about:privatebrowsing in the tab instead of going through sessionstore,
>which I'll fix in the next patch if you're ok with that.

Actually even that might not be worth it as I'll need to get the window again, but your call.

Comment 29

[Simon Bünzli]

(In reply to comment #28)
> Actually even that might not be worth it as I'll need to get the window again

Alright, in that case let's go with this and see that we can get these bits cleaned up for Firefox 3.2.

Comment 30

[Mike Connor [:mconnor]]

I'd like to see the final version of this before we do more code review.

Comment 31

[Nochum Sossonko [:Natch]]

(In reply to comment #30)
> I'd like to see the final version of this before we do more code review.

Code-wise this would be the final version minus one nit on where the transitionState object is placed. The tests though do not work and are proving to be somewhat difficult (private browsing has to to be stopped with the cookie page still loaded, then the test has to check for cookie values and error console entries once the page has unloaded), I'm working on one that will hopefully work and I'll update the patch then.

Comment 32

[Nochum Sossonko [:Natch]]

Attachment: Patch+Test that works

Ok here's the same patch, just moved the transitionState declaration and redid the test.

Comment 33

[(no longer active)]

Comment on attachment 361374 [details] [diff] [review]
Patch for review

Here is a preliminary review, please address the below comments in your next version of the patch.

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>-  _onPrivateBrowsingModeChanged: function PBS__onPrivateBrowsingModeChanged() {
>+  _onBeforePrivateBrowsingModeChanged: function PBS__onBeforePrivateBrowsingModeChanged() {

Nit: s/_onBeforePrivateBrowsingModeChanged/_onBeforePrivateBrowsingModeChange/.

Ditto for _onAfterPrivateBrowsingModeChanged.

>+      // when keep_current_session is set to true, or when we're quitting,
>+      // we don't want to save the session or do any transitioning.
>+      let shouldTransition = true;

Please eliminate this variable and use (this._quitting || !this._saveSession) instead (which is actually the same thing).

>+        let browser = Cc["@mozilla.org/appshell/window-mediator;1"].
>+                  getService(Ci.nsIWindowMediator).
>+                  getMostRecentWindow("navigator:browser").gBrowser;

Nit: please align the two last lines to |Cc| on the first line.

>+        // this ensures a clean slate from which to transition into or out of pb
>+        browser.addTab();
>+        browser.removeTab(browser.tabContainer.firstChild);

This is kind of hackish.  Please try to use the solution Simon proposed in comment 20 by changing sss_restoreWindow.

>+      // now that the session is clear, begin removing all private data
>+      this._onPrivateBrowsingModeChanged();

Doing this here is conceptually wrong (because the mode has not changed yet).  Instead, please add an observer for the private-browsing notification and do this there (like I did in attachment 361069 [details] [diff] [review]).  Also, please do the clearing of the error console from there as well (like you currently do in the _onPrivateBrowsingModeChanged implementation.

>+  _onAfterPrivateBrowsingModeChanged: function PBS__onAfterPrivateBrowsingModeChanged() {
>+    if (!this._autoStart) { // nothing to do here if we're auto-starting
>+      let ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+               getService(Ci.nsISessionStore);
>+      // if we're transitioning out of pb and the session is to be restored, do
>+      // it now

Nit: s/we're transitioning/we have transitioned/
And: s/pb/private browsing mode/

>+      if (!this._inPrivateBrowsing && this._saveSession) {
>+        ss.setBrowserState(this._savedBrowserState);
>+        this._savedBrowserState = null;
>+      }
>+      // otherwise, if we're transitioning into pb, show about:privatebrowsing

Nit: s/we're transitioning/we have transitioned/
And: s/pb/private browsing mode/

>+      else if (this._inPrivateBrowsing && this._saveSession) {

Please restructure the if blocks to have a big if block testing for this._saveSession, and inside it use an if/else block testing this._inPrivateBrowsing.

>+        // destroy the current session and start initial cleanup
>+        this._onBeforePrivateBrowsingModeChanged();

Nit: please add a blank line after this change.

>diff --git a/browser/components/privatebrowsing/test/browser/Makefile.in b/browser/components/privatebrowsing/test/browser/Makefile.in
>+		browser_privatebrowsing_unload.js \

Please use this name: browser_privatebrowsing_transition_cookie.js (see below).

>+		unload_test.html \

transition_unload_setcookie.html is a much better name, I think.

>+  let location = "https://example.com/browser/browser/components/privatebrowsing/test/browser/cookie_test.html";

Shouldn't this be unload_test.html (or transition_unload_setcookie.html for that matter)?  :-)

>+  let testBrowser = gBrowser.getBrowserForTab(testTab);
>+  testBrowser.addEventListener("load", function () {
>+    testTab.removeEventListener("load", arguments.callee, true);

s/testTab/testBrowser/.

>+    let consoleService = Cc["@mozilla.org/consoleservice;1"].
>+                         getService(Ci.nsIConsoleService);
>+    let count = {};
>+    consoleService.getMessageArray({}, count);
>+    is(count.value, 0, "There should be no messages in the error console!");

Why test this here?  browser_console_clear.js already tests the clearing of the error console properly, so I don't think that we need to test the same thing here again.

>+  content.location = location;

Just pass |location| to addTab above.

>diff --git a/browser/components/privatebrowsing/test/browser/unload_test.html b/browser/components/privatebrowsing/test/browser/unload_test.html
>+<body onunload="document.cookie = 'PBOn=1;'; val.bogus = bogus;">

Since testing the console clearing is not needed here, |val.bogus = bogus;| can be eliminated.




Also, we need a test which enforces the exact ordering that we expect.  The test should:

0. Make sure that only a single tab is open (sanity check), and point it to a particular URL (like data:text/plain,private%20browsing).
1. Install a handler for the unload event of the current tab.  Inside this, make sure that the observer below has not fired yet.
2. Install an observer for private-browsing.  Inside this, make sure that the unload event handler has already been fired.
3. Turn on private browsing mode.
4. After the property set in step 3 returns, this should check to make sure that a single window with a single tab displaying about:privatebrowsing is open.
5. Re-install the unload event handler.
6. Turn off private browsing mode.
7. Make sure that a single tab is open displaying the URL in step 0.  Also the new unload event handler must make sure that the observer has not been called a second time, and the observer must ensure that it has been called after the unload event handler.
8. Make sure that each of the unload event handlers have been called exactly once, and the observer twice.

This is rather complicated, so if you're not up for it, I'll be happy to add this test once you submit your final patch.  This should be called browser_privatebrowsing_transition.js.

BTW, thanks a lot for being a brave soul to submit patches for the private browsing service!  :-)

Comment 34

[(no longer active)]

Comment on attachment 361612 [details] [diff] [review]
Patch+Test that works

Firstly, thanks for your patch!

From a quick look at your patch, all my previous comments still apply here as well (except for the test, which looks much better and similar to what I suggested).

Please address those comments if possible.

Also, I would like to have a test around as well which explicitly tests the cookie problem as mentioned in comment 0.  The test in your previous patch could probably be modified to do just that.

Comment 35

[(no longer active)]

BTW, even though it's set as P2 by Beltzner, I'd like it to be P1 since the patch changes the private browsing service in significant ways, and I'd really like it to get some beta testing to make sure we have not regressed anything.  Beltzner, do you agree?

Also, Natch, have you run the private browsing tests which are spread in various places of the tree with your patch applied?  I think this is a complete list:

<http://mxr.mozilla.org/mozilla-central/find?text=&kind=text&string=248970>
<http://mxr.mozilla.org/mozilla-central/find?string=privbrowsing&tree=mozilla-central&hint=>

and also:

<http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_privatebrowsing.js>

Comment 36

[Nochum Sossonko [:Natch]]

(in reply to the previous 3 comments)
>This is kind of hackish.  Please try to use the solution Simon proposed in
>comment 20 by changing sss_restoreWindow.

That won't work, when the dummy session is restored by the real session it gets messed up, I suspect this is a problem with the sessionstore service, although maybe I'm not doing things right. The problem seems to be that the sessionstore somehow loses count when one of the tabs that it restored are closed, then when it gets called again (to put in the real session) it just loads a blank tab. Maybe due to the asynchronous nature of sessionstore?

>Doing this here is conceptually wrong (because the mode has not changed yet). 
>Instead, please add an observer for the private-browsing notification and do
>this there (like I did in attachment 361069 [details] [diff] [review]).  Also, please do the 

This will make the call to privateBrowsingEnabled asynchronous, no? I thought we were trying to avoid that?

> Also, I would like to have a test around as well which explicitly tests the
> cookie problem as mentioned in comment 0.  The test in your previous patch
> could probably be modified to do just that.

The test can't really test for that: in builds that do work, the test will have to be (correctly) in the observer, in builds that don't the test will have to be in the unload handler (which is counter intuitive because builds that do work will get caught). Also, previous builds will probably timeout anyways, as this test doesn't set keep_current_session (impossible to test there as there is no unload event).

>Also, Natch, have you run the private browsing tests which are spread in
>various places of the tree with your patch applied?  I think this is a complete
>list:

No I haven't, I'll do that now. I'll post a new patch with the rest of your comments.

Comment 37

[(no longer active)]

(In reply to comment #36)
> That won't work, when the dummy session is restored by the real session it gets
> messed up, I suspect this is a problem with the sessionstore service, although
> maybe I'm not doing things right. The problem seems to be that the sessionstore
> somehow loses count when one of the tabs that it restored are closed, then when
> it gets called again (to put in the real session) it just loads a blank tab.
> Maybe due to the asynchronous nature of sessionstore?

Simon can probably comment on this.  But can you please submit a patch for him and me to take a look?

> This will make the call to privateBrowsingEnabled asynchronous, no? I thought
> we were trying to avoid that?

No, it won't.  The privateBrowsingEnabled setter fires the notification, and the observer gets called and does its magic before the call to notifyObservers returns, so it all gets done before the setter returns.

> The test can't really test for that: in builds that do work, the test will have
> to be (correctly) in the observer, in builds that don't the test will have to
> be in the unload handler (which is counter intuitive because builds that do
> work will get caught). Also, previous builds will probably timeout anyways, as
> this test doesn't set keep_current_session (impossible to test there as there
> is no unload event).

We don't necessarily want a test which can be executed on a build without your patch and fail.  We want to have a test which is executed on the build with your patch and all the future builds to make sure that the cookie won't leak between transition boundaries.  Although the other test makes sure that the ordering logic is right, I want us to have a test to make sure that the problem in comment 0 is really fixed and it won't regress in the future.

> No I haven't, I'll do that now. I'll post a new patch with the rest of your
> comments.

Good.  I don't think that the patch should regress any of those tests (because most of the changed code patch is not executed at all in those tests) but never hurts to try before taking the risk of making the whole tree orange upon landing.  :-)

Also, yes, this means that with your patch, we can get a better test coverage on the private browsing service, and we can now run the tests on exactly the same code which gets run by real users.  And we can also convert a lot of private browsing Litmus tests into automated ones.  Yay!

Comment 38

[Nochum Sossonko [:Natch]]

Attachment: Updated to comments

Ok, here's the patch with all your comments addressed. I've updated the test as well to include a cookie checking test. I've also ran all the tests you pointed me at, and they all pass.

Comment 39

[Nochum Sossonko [:Natch]]

Just a thought: I think the line that saves the session, this._savedBrowserState = ss.getBrowserState, can be moved down to the next block. I left it where it was because there was a check for (if !this._savedBrowserState), is that necessary? Shouldn't we be saving the session anyhow? I don't even think it's possible to reach a state where _savedBrowserState would be non-null at that stage.

Comment 40

[Nochum Sossonko [:Natch]]

(In reply to comment #39)

Hrm, I knew I had gone through that in my head before, it needs to be in the if (this._inPrivateBrowsing) block... :-/

Comment 41

[(no longer active)]

Comment on attachment 361645 [details] [diff] [review]
Updated to comments

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>+        let browser = Cc["@mozilla.org/appshell/window-mediator;1"].
>+                      getService(Ci.nsIWindowMediator).
>+                      getMostRecentWindow("navigator:browser").gBrowser;
>+        // this ensures a clean slate from which to transition into or out of
>+        // private browsing
>+        browser.addTab();
>+        browser.removeTab(browser.tabContainer.firstChild);

I'd still like to see an alternative version of this patch which fixes this at sessionstore level, but I'm not going to hold this patch because of this.  We can even address this in a followup if Simon thinks that the sessionstore fix can be made to work.

>+        // clear plain HTTP auth sessions
>+        let authMgr = Cc['@mozilla.org/network/http-auth-manager;1'].
>+                      getService(Ci.nsIHttpAuthManager);
>+        authMgr.clearAll();
>+        if (!this._inPrivateBrowsing) {

Nit: please add a blank line before this if.

>diff --git a/browser/components/privatebrowsing/test/browser/Makefile.in b/browser/components/privatebrowsing/test/browser/Makefile.in
>+		browser_privatebrowsing_unload.js \

I still think browser_privatebrowsing_transition.js is a much better name...

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_unload.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_unload.js
>+// Test for bug 476463, https://bugzilla.mozilla.org/show_bug.cgi?id=476463
>+let cookieManager = Cc["@mozilla.org/cookiemanager;1"].
>+                    getService(Ci.nsICookieManager2);
>+let pb = Cc["@mozilla.org/privatebrowsing;1"].
>+         getService(Ci.nsIPrivateBrowsingService);
>+let _obs = Cc["@mozilla.org/observer-service;1"].
>+           getService(Ci.nsIObserverService);
>+let testTab, observerNotified = false;
>+function startTests() {
>+  testTab.linkedBrowser.addEventListener("unload", finshTests, true);
>+  pb.privateBrowsingEnabled = false;
>+}
>+function finshTests() {

Nit: spelling.

General comments on the test:

1. Can you please restructure the test to use closures?
2. (nit) Can you please add a license header to the test similar to the ones in the existing tests in that directory?
3. Will you be able to extend this test to include my ideas in comment 33?

Comment 42

[(no longer active)]

(In reply to comment #39)
> Just a thought: I think the line that saves the session,
> this._savedBrowserState = ss.getBrowserState, can be moved down to the next
> block. I left it where it was because there was a check for (if
> !this._savedBrowserState), is that necessary? Shouldn't we be saving the
> session anyhow? I don't even think it's possible to reach a state where
> _savedBrowserState would be non-null at that stage.

_savedBrowserState would be null all the time there unless something seriously has went wrong, so it's more of a sanity check (which you can ignore, as far as I can tell).

Comment 43

[Nochum Sossonko [:Natch]]

Attachment: fixed up test

Ehsan, is this ok with you? I implemented nearly everything you wrote there, although to get the url I had to go through sessionstore (probably because I wasn't waiting for load events, but that would be pretty annoying).

Comment 44

[Nochum Sossonko [:Natch]]

Attachment: final patch (I hope!)

I really don't need sessionstore for the test. Rest is the same.

Comment 45

[(no longer active)]

Comment on attachment 361700 [details] [diff] [review]
final patch (I hope!)

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>+        // clear plain HTTP auth sessions
>+        let authMgr = Cc['@mozilla.org/network/http-auth-manager;1'].
>+                      getService(Ci.nsIHttpAuthManager);
>+        authMgr.clearAll();
>+        if (!this._inPrivateBrowsing) {

Nit (again): please add a blank line before this if.

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_transition.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_transition.js
>+ * The Initial Developer of the Original Code is
>+ * Ehsan Akhgari.

Nit: you probably want to list your name here.

>+// Test for bug 476463, https://bugzilla.mozilla.org/show_bug.cgi?id=476463

Nit: please replace this with a short comment which actually states what the test does.  Example:

// Test the expected order of events for transitions of the private browsing mode.

>+let pbObserver = {
>+  observe: function(aSubject, aTopic, aData) {
>+    if (aTopic == "private-browsing") {
>+      switch(aData) {
>+        case "enter":
>+          observerNotified++;

Please test the value of observerNotified here to make sure it equals 1.

>+          is(firstUnloadFired, 1, "The first unload event should have been processed by now");
>+          break;
>+        case "exit":
>+          _obs.removeObserver(this, "private-browsing");
>+          observerNotified++;

Please test observerNotified here to make sure it's 2.

>+function test() {
...
>+  content.location =
>+    "about:robots";

Nit: to make the test a bit more readable, remove this line, and pass "about:robots" to the addTab call above it inside |test|.


This looks great.  r=me with the above comments and nits addressed.  We also need the stamp of a browser peer on this patch in order for it to land, so let's try gavin.

Comment 46

[Mike Connor [:mconnor]]

Comment on attachment 361700 [details] [diff] [review]
final patch (I hope!)

Let's try me, actually.  Gavin's pretty heavily loaded.  Should have this done in the AM, agree we want for b3 if possible.

Comment 47

[Nochum Sossonko [:Natch]]

> >+function test() {
>...
> >+  content.location =
> >+    "about:robots";

>Nit: to make the test a bit more readable, remove this line, and pass
>"about:robots" to the addTab call above it inside |test|.

The problem is that I have to use a load handler for the first page to ensure we give it time, otherwise sessionstore won't catch that url since we don't give it any time to load, and then the last test |is(uri, "about:robots"...)| will fail. I fixed the rest of the nits locally, and will post the next version (no code changes, just some nits and extra tests  per comment 45) once I get a browser peer review.

I was also wondering if I should remove (or maybe add?) the |!this._savedBrowserState| sanity check in favor of |!this._quitting|, because there's no reason to grab the session if we're quitting. That can be left for another bug though...

Comment 48

[Nochum Sossonko [:Natch]]

Attachment: Modified Tests

The url testing doesn't work (at least not always) for the very same reason this bug is here :) . I've removed those tests from this patch as there is no good way of testing this aside from hooking up a progressListener (even then it's not trivial). The rest of the tests are the same, and with them this bug should be covered well enough.

Sorry for the bugspam.

Comment 49

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 4

I bumped into a problem with sessionstore and quitting while in private browsing. The session that was being saved (and reloaded when firefox was next started) was a dummy blank tab session. I fixed this by putting the session saving code in a private-browsing-cancel-vote observer. I don't know if this is the best way to handle it or if we should define a new notification (private-browsing-granted?) to handle this.

With the patch in this bug the notification is sent while "in between" sessions so that the correct data is purged/retained. I don't think this is a problem for any of the other listeners, but sessionstore was relying on a good session at that notification.

Comment 50

[Nochum Sossonko [:Natch]]

Comment on attachment 361726 [details] [diff] [review]
Patch ver. 4

Simon, can you have a look at the sessionstore change?

Comment 51

[Simon Bünzli]

Comment on attachment 361726 [details] [diff] [review]
Patch ver. 4

I'm not quite clear about when the "private-browsing" notification is to be observed (somewhere in between the transition, I guess), however "-cancel-vote" is too early: An extension might update/save session related state during that notification (e.g. through setWindowValue) and that state wouldn't be preserved if it happened to register its observer after SessionStore's. Of course, the same would happen, if you introduced a "private-browsing-granted" notification (with extensions having the work-around available to use the "-cancel-vote" notification as fallback).

Comment 52

[(no longer active)]

Comment on attachment 361726 [details] [diff] [review]
Patch ver. 4

(In reply to comment #49)
> I bumped into a problem with sessionstore and quitting while in private
> browsing. The session that was being saved (and reloaded when firefox was next
> started) was a dummy blank tab session. I fixed this by putting the session
> saving code in a private-browsing-cancel-vote observer. I don't know if this is
> the best way to handle it or if we should define a new notification
> (private-browsing-granted?) to handle this.

private-browsing-cancel-vote is an opportunity for modules and extensions to abort the transition into or out of the private browsing mode.  In other words, the listener cannot even assume that the transition will happen in this notification.

> With the patch in this bug the notification is sent while "in between" sessions
> so that the correct data is purged/retained. I don't think this is a problem
> for any of the other listeners, but sessionstore was relying on a good session
> at that notification.

Yes, that's a precise description of the problem.

The best solution I can think of is this:  Get rid of the _stateBackup variable in session store completely, and in nsPrivateBrowsingService, upon receiving the private-browsing notification with the "exit" data parameter, raise a new notification (called something like private-browsing-save-session) and pass _savedBrowserState as its data parameter.  Inside nsSessionStore, listen for that notification, parse the received data parameter as a JSON string and set oState.session.state appropriately (like we currently do in nsSessionStore) and call _saveStateObject on this object.

This will take care of the problem Simon mentioned.  If possible, I'd like to take a look at a version of the patch which does this before handing it off to mconnor for a final review.

Comment 53

[(no longer active)]

(In reply to comment #47)
> >Nit: to make the test a bit more readable, remove this line, and pass
> >"about:robots" to the addTab call above it inside |test|.
> 
> The problem is that I have to use a load handler for the first page to ensure
> we give it time, otherwise sessionstore won't catch that url since we don't
> give it any time to load, and then the last test |is(uri, "about:robots"...)|
> will fail.

Passing the URL to addTab works fine with the load handler because the content
is loaded asynchronously.  I'm kind of ambivalent here, and I'm fine with
either solution; it's not a big deal.

> I was also wondering if I should remove (or maybe add?) the
> |!this._savedBrowserState| sanity check in favor of |!this._quitting|, because
> there's no reason to grab the session if we're quitting. That can be left for
> another bug though...

_quitting cannot be true if the private browsing mode is being turned on, so I
don't think this would be necessary.

(In reply to comment #48)
> The url testing doesn't work (at least not always) for the very same reason
> this bug is here :)

I think it should work reliably if the URLs are read at an appropriate time. 
Can you please elaborate what the problem here is?

(BTW, the test at its current state is fine for the purpose of this bug.  I'm
just trying to understand the problem, so that we can improve the test either
in this bug or a followup to verify the URLs as well).

Comment 54

[Nochum Sossonko [:Natch]]

>_quitting cannot be true if the private browsing mode is being turned on, so I
>don't think this would be necessary.

Right, I was just being stupid :)

>I think it should work reliably if the URLs are read at an appropriate time. 
>Can you please elaborate what the problem here is?

The only way you can get a reliable load (and hence be able to test the page and url that was loaded) is by registering a listener *before* |content.location| is set. Otherwise the situation is racy (i.e. maybe your listener won't get called because you registered too late). In this case it is impossible to set a listener before the page is loaded, as we don't have the tab element yet that will load that page (transitioning to/from pb now causes all tabs to be closed).

For the sessionstore problem, I'd like to introduce a new notification "private-browsing-before-change" similar to the profile notifications. We'd do the sessionstore saving then. I don't think sending in the JSON string is any better than having sessionstore get the session itself, so I'll leave that code alone, only it will be triggered on "private-browsing-before-change". The notification will have all the details that "private-browsing" has, only it will be sent before any changes are made. Initial patch forthcoming, I'll try to write up some tests for this sometime later today/tomorrow.

Comment 55

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 5

Here's the change, can we get a tryserver for the patches here so others can test?

Comment 56

[(no longer active)]

(In reply to comment #54)
> The only way you can get a reliable load (and hence be able to test the page
> and url that was loaded) is by registering a listener *before*
> |content.location| is set. Otherwise the situation is racy (i.e. maybe your
> listener won't get called because you registered too late). In this case it is
> impossible to set a listener before the page is loaded, as we don't have the
> tab element yet that will load that page (transitioning to/from pb now causes
> all tabs to be closed).

I see.

> For the sessionstore problem, I'd like to introduce a new notification
> "private-browsing-before-change" similar to the profile notifications. We'd do
> the sessionstore saving then.

This won't solve the issue in comment 51.

> I don't think sending in the JSON string is any
> better than having sessionstore get the session itself,

Actually it is, we have a bug on file (bug 462218) to eliminate the copy of the
data which the session store component currently keeps, so doing this will kill
two birds with one stone.  So, please make this change.

> so I'll leave that code
> alone, only it will be triggered on "private-browsing-before-change". The
> notification will have all the details that "private-browsing" has, only it
> will be sent before any changes are made.

Like Simon stated in comment 51, this approach won't work reliably.

Comment 57

[(no longer active)]

Comment on attachment 361787 [details] [diff] [review]
Patch ver. 5

r- based on comment 56.  I suspect that Simon agrees with me here.

If you can post a modified patch soon, I'll use that for a try server build, otherwise I'll upload this patch to the try server for preliminary testing.

Comment 58

[Nochum Sossonko [:Natch]]

>observed (somewhere in between the transition, I guess), however "-cancel-vote"
>is too early: An extension might update/save session related state during that
>notification (e.g. through setWindowValue) and that state wouldn't be preserved
>if it happened to register its observer after SessionStore's. Of course, the
>same would happen, if you introduced a "private-browsing-granted" notification
>(with extensions having the work-around available to use the "-cancel-vote"
>notification as fallback).

I thought I had commented on what Simon said, oh well. The reason this won't work (and doesn't work afaict in the current impl.) is because any notification we send can be observed by extensions and they can change the state then, which will put us in the same place as what we have now. The only real way to get this fixed is to introduce a new api in the sessionstore (something like saveStateBackup) and call that without sending any notifications.

Just to be clear: Extension a wants to save a "welcome back" screen when you go into pb mode, so that when you exit you'll get a cute little message. It observes <<place any notification here (e.g. in the current impl. "private-browsing")>> and modifies the session state as appropriate. Now, if it gets called before sessionstore does, then all is fine. If not, the extensions welcome screen will not be shown. With this patch it is possible (by observing the vote notification) to do this, which it can then erase on quit-application-granted if pb mode has not entered. I know this is probably not optimal, but do we have to optimize for this case? Shouldn't there be a separate bug for that? Like I said, I'm pretty sure the same problem exists in the current impl.

>Actually it is, we have a bug on file (bug 462218) to eliminate the copy of the
>data which the session store component currently keeps, so doing this will kill
>two birds with one stone.  So, please make this change.

That's fine for that bug, but I think we should leave this alone in this bug for now, there's enough going on here to warrant a separate bug for that, and I don't want to introduce possible regressions coming from that code change, if you still want it, I can definitely do it.

Comment 59

[Nochum Sossonko [:Natch]]

Comment on attachment 361787 [details] [diff] [review]
Patch ver. 5

re-requesting review per previous comment.

Comment 60

[(no longer active)]

(In reply to comment #58)
> I thought I had commented on what Simon said, oh well. The reason this won't
> work (and doesn't work afaict in the current impl.) is because any notification
> we send can be observed by extensions and they can change the state then, which
> will put us in the same place as what we have now. The only real way to get
> this fixed is to introduce a new api in the sessionstore (something like
> saveStateBackup) and call that without sending any notifications.

Even a new API won't fix the problem, since extensions can override that API as well.

> That's fine for that bug, but I think we should leave this alone in this bug
> for now, there's enough going on here to warrant a separate bug for that, and I
> don't want to introduce possible regressions coming from that code change, if
> you still want it, I can definitely do it.

OK, fair enough.  I'm fine with this as it is, and I'll handle the rest in bug 462218.

Comment 61

[(no longer active)]

Comment on attachment 361787 [details] [diff] [review]
Patch ver. 5

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
>+    case "private-browsing-before-change":

Can you please rename this notification to "private-browsing-change-granted"?

r=me with that fixed.

Comment 62

[Simon Bünzli]

Comment on attachment 361787 [details] [diff] [review]
Patch ver. 5

We'll probably do want a new API like e.g. updateSessionFile for forcing the current session state to be written synchronously to sessionstore.js (in our tests we're currently using a hacky work-around instead). Then we'd need a better way of handling the _stateBackup, as well, though, which might be too tight for Firefox 3.1.

For the rest, mconnor's review will be enough.

Comment 63

[Nochum Sossonko [:Natch]]

Attachment: Slightly Update ver. 5.1

I've slightly updated the tests so that the cookie test actually works now. I also incorporated the change from the review comment.

The way I've been testing is I would comment out in nsPrivateBrowsing.js:

ss.setBrowserState(JSON.stringify(transitionState));
[snip]
browser.removeTab(browser.tabContainer.firstChild);

in _onBeforePrivate... now all tests (besides the unload and observer counts, they correctly run twice with or without the patch) fail with this modification and pass with the patch as is.

Comment 64

[Mike Connor [:mconnor]]

Comment on attachment 361804 [details] [diff] [review]
Slightly Update ver. 5.1

ok, this looks pretty good.

Comment 65

[Nochum Sossonko [:Natch]]

Since this is a P1, I'll remove the relnote keyword.

Comment 66

[(no longer active)]

Landed on m-c: <http://hg.mozilla.org/mozilla-central/rev/ecad4aa3be7e>

I'll land it on 1.9.1 after a green cycle.  Thanks for working on this, Natch!

Comment 67

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/14ff6d620957

Comment 68

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090514 Minefield/3.6a1pre ID:20090514031253

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090514 Shiretoko/3.5b5pre ID:20090514031007