829568 - Starting a private window from the Firefox icon pinned to the taskbar wipes your regular session

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Ioana (away)]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:21.0) Gecko/20130109 Firefox/21.0 - 20130109030942

Steps to reproduce:
1. Open Firefox and load a few web sites in new tabs.
2. Pin Firefox to the Windows taskbar.
3. Close Firefox.
4. Right click the pinned icon on the taskbar and select "New Private Window".

A PW is opened and it contains my tabs from the normal session + a tab with about:privatebrowsing. When I open a normal window from the private one, I can see that the normal session has been lost - I can't restore the tabs it had in any way.

Note:
The option set for what gets opened when starting the browser (home page, tabs and windows from last time etc) has no impact on this bug.

Comment 1

[Josh Matthews [:jdm]]

Andres, this would be good for appcoast to look into.

Comment 2

[(no longer active)]

In order to reproduce this, you need to set the browser.startup.page pref to 3 ("restore my tabs and windows from last time").

Comment 3

[(no longer active)]

I think we should modify the session restore to make sure that when it's restoring the old tabs and windows at startup, it doesn't reuse private windows.

Comment 4

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 5

[(no longer active)]

Comment on attachment 701342 [details] [diff] [review]
Patch v1

Review of attachment 701342 [details] [diff] [review]:
-----------------------------------------------------------------

I think Tim is a better reviewer for this.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

What is the expected workflow when a user starts with a private window and opens a non-private window a couple some time after that? Should we then restore the last session automatically to this window? What if the user has multiple windows? Restoring them all when they opened a new non-private windows is probably a quite strange and unexpected behavior.

Comment 8

[Josh Matthews [:jdm]]

Since they presumably have the option of explicitly restoring their session via the menu item/about:home button, it doesn't seem like the session should automatically be restored in that case.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

The problem here is that we then have a private and a non-private window. We'd normally save session data for the non-private window but would have to ignore it here? Or create a second session that doesn't overwrite the original one? This sounds like complexity we probably don't want to introduce for this rare workflow.

Comment 10

[(no longer active)]

(In reply to comment #9)
> The problem here is that we then have a private and a non-private window. We'd
> normally save session data for the non-private window but would have to ignore
> it here? Or create a second session that doesn't overwrite the original one?
> This sounds like complexity we probably don't want to introduce for this rare
> workflow.

We don't persist the data for the private window right?  I mean, here's how I expect things to work:

* You start with a private window.  The sessionstore information from the previous session is untouched.
* You open a non-private window.  We won't restore the session just yet.
* You do History -> Restore Previous Session.  We restore the previous session that we have kept around.

Is is not easy to make that work?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Andres' patch is definitely an easy solution and seems to work well for the issue stated here.

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #10)
> * You open a non-private window.  We won't restore the session just yet.

With Andres' patch we restore the old session right away when opening a new window which I think is actually maybe better? Otherwise I'd be confused where my session has gone and the user can't lose it by closing the private and then the new window.

What troubles me is that so far, the first window sets SS._loadState = STATE_RUNNING and restores the last session, if any. With this patch we'd change this behavior to defer this until the first non-private windows is loaded. I really don't know if we can safely assume that everything just continues to work. There could be code paths depending on STATE_RUNNING which I don't know of.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 701342 [details] [diff] [review]
Patch v1

Review of attachment 701342 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should take this path.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - don't save (invalid) states without any windows

(Sorry for stealing this, Andres. We ideally want to see this fixed before the next merge and so I'll just submit the patches I wrote right after digging into it. Thank you anyway for taking a stab at this in the first place!)

This first patch makes sure that we don't save invalid states without any windows. That way we can start up and set the state to STATE_RUNNING without losing the last session.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - back up the initial session when starting up with a private window

This second patch lets us save the initialState when the first, private window is loaded. We also will not restore the last session in to the private window anymore.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 (without white space changes)

This is the same patch as part 2 just without any white space changes to ease review a little bit.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 3 - restore the initial session in case the user opens a non-private window

This last patch takes the data we stored in part 2 and restores it as soon as the user opens the first non-private window in their session.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 714327 [details] [diff] [review]
part 1 - don't save (invalid) states without any windows

Felipe, can you please take a look at this bug and review the patches maybe even today? Sorry for the short notice but they're tiny patches and we'd really like to land this sooner than later to uplift it to Aurora :) Thanks!

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 714328 [details] [diff] [review]
part 2 - back up the initial session when starting up with a private window

I attached a version of this patch (attachment 714329 [details] [diff] [review]) without the white space changes so that it's a little clearer to see what changed.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 714328 [details] [diff] [review]
part 2 - back up the initial session when starting up with a private window

Review of attachment 714328 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +693,2 @@
>      if (!this._isWindowLoaded(aWindow))
>        this._windows[aWindow.__SSi]._restoring = true;

do we need to avoid setting this _restoring to true if we're not gonna restore now?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Felipe Gomes from comment #19)
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +693,2 @@
> >      if (!this._isWindowLoaded(aWindow))
> >        this._windows[aWindow.__SSi]._restoring = true;
> 
> do we need to avoid setting this _restoring to true if we're not gonna
> restore now?

|this._isWindowLoaded()| is false only if |window.__SS_restoreID| is present, which is set by |_openWindowWithState()|. See here from the same method:

> // this window was opened by _openWindowWithState
> else if (!this._isWindowLoaded(aWindow)) {
>   let followUp = this._statesToRestore[aWindow.__SS_restoreID].windows.length == 1;
>   this.restoreWindow(aWindow, this._statesToRestore[aWindow.__SS_restoreID], true, followUp);
> }

TL;DR we're not setting |_restoring=true| when starting with a private window.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 714328 [details] [diff] [review]
part 2 - back up the initial session when starting up with a private window

Review of attachment 714328 [details] [diff] [review]:
-----------------------------------------------------------------

I see!

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/a919f0c65b69
https://hg.mozilla.org/integration/fx-team/rev/bcc9c69de523
https://hg.mozilla.org/integration/fx-team/rev/c398fc5f72cf

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a919f0c65b69
https://hg.mozilla.org/mozilla-central/rev/bcc9c69de523
https://hg.mozilla.org/mozilla-central/rev/c398fc5f72cf

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

It would be great to have QA verify this as fixed before requesting the uplift to beta.

Comment 25

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20130220 Firefox/22.0 (20130220031126)

When "Show my windows and tabs from last time" is selected and Firefox was closed:
1. Right click the icon pinned on the taskbar and select "New private window", then open a regular window => No tabs from the previous regular window are restored in the private window. The whole previous session is restored in the regular window.
2. Right click the icon pinned on the taskbar and select "Open new window", the previous session is restored.

When "Show my home page" is selected and Firefox was closed:
1. Right click the icon pinned on the taskbar and select "New private window", then open a regular window => No tabs from the previous regular window are restored in the private window, nor in the regular window. The option of restoring the previous session is only offered in the regular window and it works fine.
2. Right click the icon pinned on the taskbar and select "Open new window", the option of restoring the previous session is available in the regular window and it works fine.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Awesome, thank you Ioana for verifying this so quickly.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 714327 [details] [diff] [review]
part 1 - don't save (invalid) states without any windows

Requesting beta approval for all three patches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Per-window private browsing.
User impact if declined: Possible data (session) loss on windows after private browsing use.
Testing completed (on m-c, etc.): Landed on m-c, verified.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I'm confused, it appears this landed on m-c after the uplift (so Fx22). Does this need to land on Aurora too?

Comment 30

[Josh Matthews [:jdm]]

Yes.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Corrected a little typo in part 1:

https://hg.mozilla.org/integration/fx-team/rev/652d420b1800

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

The patches landed in time for Aurora. I just checked and the changes are all there.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/648403650289
https://hg.mozilla.org/releases/mozilla-beta/rev/072453198e17
https://hg.mozilla.org/releases/mozilla-beta/rev/8df29822474e

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/652d420b1800

Comment 35

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130227 Firefox/21.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 (beta 2)

Comment 36

[vfede]

is #647655 a duplicate of this?

Comment 37

[Christian Ascheberg]

(In reply to vfede from comment #36)
> is #647655 a duplicate of this?

Not exactly, because bug 647655 is not in the context of per-window PB. Still, I'd think that per-window PB fixes that bug (as I said in my comment there).