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

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Private Browsing
P1
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: whimboo, Assigned: mconnor)

Tracking

({verified1.9.1})

3.5 Branch
Firefox 3.6a1
All
Mac OS X
verified1.9.1
Points:
---
Bug Flags:
blocking-firefox3.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

9 years ago
Created attachment 365847 [details]
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.
(Assignee)

Comment 3

9 years ago
oddly enough, I can't always repro this.  investigating.
Assignee: nobody → mconnor
Status: NEW → ASSIGNED
Flags: blocking-firefox3.1+
Priority: -- → P2
Summary: Starting Private browsing from window-less state shows "about:privatebrowsing" entry in list of recently closed tabs after stop → Starting Private browsing from window-less state shows last closed tab from PB mode in list of recently closed tabs after stop
Target Milestone: --- → Firefox 3.1

Comment 4

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

Comment 5

9 years ago
This isn't just about:privatebrowsing, I've replicated with other pages by navigating to a new page before exiting.
--> 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.
Priority: P2 → P1
(Assignee)

Comment 7

9 years ago
Created attachment 367204 [details] [diff] [review]
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.
Attachment #367204 - Flags: review?(gavin.sharp)
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?
Attachment #367204 - Flags: review?(gavin.sharp) → review+
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.
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.
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.
(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.
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)...
(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).
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?
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.
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.
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.
Should this be landing on trunk?
Target Milestone: Firefox 3.1 → Firefox 3.5b4

Comment 20

9 years ago
(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!
No worries, that was bug 483748.
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.
Target Milestone: Firefox 3.5b4 → Firefox 3
(Reporter)

Updated

9 years ago
Target Milestone: Firefox 3 → Firefox 3.5b4
Whiteboard: [needs landing]
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b1237eca3670
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Target Milestone: Firefox 3.5b4 → Firefox 3.6a1
Backed out; the push this was part of seems to be causing random leak orange on Mac.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Looks like the random leak orange is still happening, so this can reland as desired...
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.
The leak exists on 1.9.1, so this didn't cause it.
http://hg.mozilla.org/mozilla-central/rev/9cc643750f49
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 29

9 years ago
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
Status: RESOLVED → VERIFIED

Comment 30

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6213fb52d6c0
Flags: in-testsuite?
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
(Reporter)

Comment 31

9 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.