481598 - Starting Private browsing from window-less state shows last closed tab from PB mode in list of recently closed tabs after stop

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090304 Shiretoko/3.1b3pre ID:20090304022008

If the Private Browsing mode is started from a window-less state and the user stops it right away the "about:privatebrowsing" item is shown in the list of recently closed tabs.

Shouldn't the list be empty? It is visible since bug 481090 has been fixed.

Comment 1

[Mike Connor [:mconnor]]

I can't reproduce this.  If I have no windows, I actually go back to that state when I exit Private Browsing.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090304 Firefox/3.1b3pre

Comment 2

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

Attachment: Recently closed Tabs menu

Happens too with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 ID:20090305133223

Detailed steps:
1. Create a fresh profile
2. Start Firefox and close Window
3. Start Private Browsing Mode
4. Stop Private Browsing Mode
5. Observe History | Recently closed Tabs menu

See the attachment.

Comment 3

[Mike Connor [:mconnor]]

oddly enough, I can't always repro this.  investigating.

Comment 4

[(no longer active)]

Shouldn't we check to see if a browser window is open before opening about:privatebrowsing, like we do so before opening about:blank in <http://hg.mozilla.org/mozilla-central/file/73efb0ef0d79/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#l161>?

Comment 5

[Mike Connor [:mconnor]]

This isn't just about:privatebrowsing, I've replicated with other pages by navigating to a new page before exiting.

Comment 6

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

--> P1, as this bug will require the wider feedback of a beta release or is of sufficient complexity that we should be looking at it sooner, not later.

Comment 7

[Mike Connor [:mconnor]]

Attachment: fun

Ok, so, this was "fun" to chase down.  Should have done it with Dietrich around.  Basically, the state data saved when no windows were open has |"windows" == []| so sessionstore treats that as "nothing to restore" and we're left with the dummy state, plus the tab that got closed after that.  Only on Mac do we end up with a returned state that isn't actually restorable!  Easiest solution is to reuse the dummy blank state we're using for the transition and save that as the state to restore after PB exit if there's no browser windows open.

I'd muck around with Session Store, but returning a bogus session in all cases seems wrong.  Local hack is safer at this point, we can decide what SS should do in this situation later.

Comment 8

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

So the setBrowserState(transitionState) closes all tabs but one, and starts a load to about:blank in that remaining tab (since that's the session we're restoring). Then we call .close() on that tab before the load is complete, which causes the session store code to attempt to save it in the recently closed tab list. The session store code resets the title of the original document for loading tabs, which causes us to race with the load and potentially save the old title with the new URL (about:blank).

This fix makes sure we always store state that will clobber the state from that tab close, which seems to be fairly isolated and safe, so I don't have any objection to taking it.

I think the ideal fix would be to make it possible to have setBrowserState not reuse tabs, which would eliminate the need for the special transition state. setBrowserState could close all the tabs before it triggers loads in the new ones, and there would be no race (nor any need for the addTab/removeTab hack in _onBeforePrivateBrowsingModeChange). Can we do that in a followup? Are there any reasons why that wouldn't be doable on the 1.9.1 branch?

Comment 9

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

If we don't implement the alternate solution, we should also make sure that  setBrowserState(transitionState) will correctly wipe out anything else that could potentially be stored by a tab close.

Comment 10

[Nochum Sossonko [:Natch]]

The alternative solution to all this is to create a method in session store to restore the session synchronously. I think Simon had mentioned this once, that would eliminate the need for the hacky fix from bug 476463, which in turn (indirectly) caused a bunch of these bugs. With sync session restore, which only PB should really use, there wouldn't be this possibility.

Comment 11

[Nochum Sossonko [:Natch]]

Evaluating the following in the Error Console (with all browser windows closed) should probably yield the same results, although for me two windows are opened, one with the about:privatebrowsing the other with the previous session(?).

Components.classes["@mozilla.org/privatebrowsing;1"].
getService(Components.interfaces.nsIPrivateBrowsingService).
privateBrowsingEnabled = true;
Components.classes["@mozilla.org/privatebrowsing;1"].
getService(Components.interfaces.nsIPrivateBrowsingService).
privateBrowsingEnabled = false;

Evaluate all that, on Windows Vista Shiretoko (3.1b4pre) I get two windows. I'll try to test the patch tomorrow to see if it fixes this, the potential problem would be an extension, since technically the calls to privateBrowsingEnabled are supposed to be guaranteed to have set the mode when the call returns. Just my .02.

Comment 12

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

(In reply to comment #10)
> The alternative solution to all this is to create a method in session store to
> restore the session synchronously.

What do you mean? As far as I can tell, setBrowserState is already synchronous... it's just that it starts loads that might take a while to complete. We can't block the UI waiting for them to finish, so there's no real way to make the transition to PB fully synchronous.

What we can do is avoid calling it twice with a tab close in between, though, which is what I'm suggesting. We can do that by having it control the tab closing directly, rather than relying on an "empty session" hack.

Comment 13

[Nochum Sossonko [:Natch]]

The reason that hack was implemented is because the loads that session store does in already open tabs are async. When session store is called to set a browser state it closes all extra tabs, and leaves the right amount open which it sets to the correct urls. This second part is done async (the loading of the urls) which is why onunload will not fire synchronously after the call to setBrowserState. Notice that this only happens for the tabs that are *not* closed (tabs that are closed will fire onunload before the next bit of javascript). At least so I was told, Simon would know more about this though. I'm pretty sure of this though, given bug 482580 comment 12.

If it was all done before the call to setBrowserState returned, there would be no need to have a transitionState (or blankState)...

Comment 14

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

(In reply to comment #13)
> If it was all done before the call to setBrowserState returned, there would be
> no need to have a transitionState (or blankState)...

That's exactly my point. All the transition state does is cause all tabs but the first to close (and then we close the last one ourselves). If setBrowserState did that directly, we wouldn't need the transitionState, wouldn't need to worry about closing the last tab ourselves, and wouldn't have to worry about the side effects of doing so (this bug).

Comment 15

[Nochum Sossonko [:Natch]]

The only difference is that I don't think session store has to _close_ all the tabs first, I think it calls the tab restoration method in a timeout, there should be another method that doesn't setTimeout anything. FYI, this originated in bug 476463 comment 20. This would make for a much smoother transition say from 10 tabs while in pb mode back to 10 tabs out of pb mode, because no tabs would be closed (vice-versa wouldn't work as the session has to be cleared regardless).

On a side note, I've just updated to tip, on a clean profile and I can't get about:privatebrowsing to load at all when I enter pb mode all I get is a blank tab (gray background) with the title "loading...", is there a bug on that or is this the fix for it?

Comment 16

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

We have to close the tabs to get synchronous removal of the current data, since we can't wait for new loads to finish to do that.

Comment 17

[Nochum Sossonko [:Natch]]

When I asked bz if there was a way to flush events on a document he said that onunload should fire synchronously. I took that to mean that when I set content.location to another url it would fire onunload (and of course onbeforeunload etc.) on the first document before my next piece of js would run. I'm not 100% sure of this though. Otherwise session store would basically have to do what we do for pb or close/open the window like pb used to do (when entering). This was in bug 482580. If this code is going to land, we can continue this effort in that bug.

Comment 18

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

Boris' comment was that unload runs synchronously from the tab close. I don't think we can avoid the tab closing, but even if we could that would be a different problem to solve. All I'm suggesting is that the tab closing should be done by sessionstore explicitly when it restores a session, rather than the current hack of forcing it to close tabs using an empty session before the actual restore.

If bug 482580 is the right place for that work, then that's fine. Most of the work involves adjusting the session restore API to allow indicating that we'd like it to avoid reusing tabs.

Comment 19

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

Should this be landing on trunk?

Comment 20

[(no longer active)]

(In reply to comment #15)
> On a side note, I've just updated to tip, on a clean profile and I can't get
> about:privatebrowsing to load at all when I enter pb mode all I get is a blank
> tab (gray background) with the title "loading...", is there a bug on that or is
> this the fix for it?

If that happens on a non-patched version of Firefox, please file a bug about it.  Thanks!

Comment 21

[Nochum Sossonko [:Natch]]

No worries, that was bug 483748.

Comment 22

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

mconnor points out that my proposal is flawed - we need to send out the private browsing notifications after the session was destroyed, but before we load the new session, so we'd still need the dummy intermediate session and two calls to setBrowserState.

Having session store do the tab closing would still eliminate the need for the tab open/close hack, and would make it possible to fix this bug in session store code, by preventing session store from capturing data while setBrowserState is on the stack, so I still think it's worth investigating.

Comment 23

[Dão Gottwald [:dao]]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

Backed out; the push this was part of seems to be causing random leak orange on Mac.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Looks like the random leak orange is still happening, so this can reland as desired...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Actually, scratch that.  This orange is on Linux, whereas the orange from the push this landed as part of was on Mac.  I recommend landing this separately from the other 4 patches that landed with it.

Comment 27

[Dão Gottwald [:dao]]

The leak exists on 1.9.1, so this didn't cause it.

Comment 28

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/9cc643750f49

Comment 29

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

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

Comment 30

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6213fb52d6c0

Comment 31

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

Verified fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090412 Shiretoko/3.5b4pre ID:20090412030914