Closed Bug 829568 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Private Browsing, defect, major)

20 Branch
All
Windows 7
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 + verified
firefox21 --- verified

People

(Reporter: ioana_damy, Assigned: andreshm)

References

Details

(Keywords: verifyme, Whiteboard: [appcoast])

Attachments

(4 files, 1 obsolete file)

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.
Assignee: nobody → josh
Blocks: PBnGen
Andres, this would be good for appcoast to look into.
Assignee: josh → nobody
In order to reproduce this, you need to set the browser.startup.page pref to 3 ("restore my tabs and windows from last time").
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.
Assignee: nobody → andres
Whiteboard: [appcoast]
Severity: normal → major
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #701342 - Flags: review?(ehsan)
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.
Attachment #701342 - Flags: review?(ehsan) → review?(ttaubert)
Duplicate of this bug: 830015
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.
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.
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.
(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?
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 on attachment 701342 [details] [diff] [review]
Patch v1

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

I don't think we should take this path.
Attachment #701342 - Flags: review?(ttaubert) → review-
(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.
Attachment #701342 - Attachment is obsolete: true
Attachment #714327 - Attachment description: don't save (invalid) states without any windows → part 1 - don't save (invalid) states without any windows
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.
This is the same patch as part 2 just without any white space changes to ease review a little bit.
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 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!
Attachment #714327 - Flags: review?(felipc)
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.
Attachment #714328 - Flags: review?(felipc)
Attachment #714330 - Flags: review?(felipc)
Attachment #714327 - Flags: review?(felipc) → review+
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?
Attachment #714330 - Flags: review?(felipc) → review+
(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 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!
Attachment #714328 - Flags: review?(felipc) → review+
It would be great to have QA verify this as fixed before requesting the uplift to beta.
Keywords: qawanted
Target Milestone: Firefox 21 → ---
Target Milestone: --- → Firefox 21
QA Contact: virgil.dicu
QA Contact: virgil.dicu → ioana.budnar
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.
Status: RESOLVED → VERIFIED
Keywords: qawanted
Awesome, thank you Ioana for verifying this so quickly.
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
Attachment #714327 - Flags: approval-mozilla-beta?
Attachment #714327 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 844260
Duplicate of this bug: 844260
I'm confused, it appears this landed on m-c after the uplift (so Fx22). Does this need to land on Aurora too?
Yes.
Target Milestone: Firefox 21 → Firefox 22
Attachment #714327 - Flags: approval-mozilla-aurora+
The patches landed in time for Aurora. I just checked and the changes are all there.
Target Milestone: Firefox 22 → Firefox 21
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)
is #647655 a duplicate of this?
(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).
Depends on: 848347
No longer depends on: 848347
See Also: → 848347
Depends on: 853779
Depends on: 578619
You need to log in before you can comment on or make changes to this bug.