510881 - Entering Private Browsing via command line shows tabs from previous session

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Open several tabs.  Close Firefox/Minefield and if prompted use: Save & Quit
Enter into Private Browsing via command line:  firefox.exe -private
Note all your prior tabs are displayed. 

Part of the design considerations in bug 471997 was to open the command line session to the Homepage as stated in:
https://bugzilla.mozilla.org/show_bug.cgi?id=471997#c51

The fact that is shows prior tabs is not working IMO if entering via command line is to show Homepage or at least about:blank (especially in a kiosk setting)

Comment 1

[(no longer active)]

CCing Alex here to get input on the desired UE here.

Comment 2

[Mike Connor [:mconnor]]

We've been down this discussion path before wrt autostart and previous session... what did we decide then?

Comment 3

[Nochum Sossonko [:Natch]]

That discussion was wrt autostart as a permanent decision (pref-based), which makes a lot more sense to not show the tabs for the previous session. Additionally, that setting takes affect immediately from the previous session, whereas here this is a startup command-line flag *after* the session has already been saved, so arguably the session should start as it always does, only in private browsing mode.

See bug 481786, bug 467565 comment 11 and others, bug 482334, and bug 482430 comment 12 and onwards. All deal with this issue in some way or another.

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

As a matter of intrest perhaps IE8 on Win7 RC you can use the 'jumplist', I know another bug on that exists, but for simplicity and comparison, when entering into: "InPrivate" from the jumplist it brings up the InPrivate page and no offers to 'restore' last session or links to that last session. 

If you close Firefox and Save&Close your session, then later decide to open via command line a PB session, it should be basically the same as using the jumplist in IE8 on Win7, blank - Plus there is the indicator you have PB enabled.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Launching Firefox into private browsing mode from the command line is a one time thing (it won't launch into private browsing mode again unless you use the command line argument again), so it seems like this is action more analogous to tools > start private browsing, as opposed to the autostart pref.  Under those assumptions, the most logical thing to do is not to display the user's session (also externally consistent with IEs behavior described in comment #4).

Comment 6

[(no longer active)]

(In reply to comment #2)
> We've been down this discussion path before wrt autostart and previous
> session... what did we decide then?

We decided not to restore the user's old session in that case (bug 482334).

Taking...

Comment 7

[(no longer active)]

Attachment: Patch (v1)

This patch makes command-line-startup a globally observable notification so that the browser startup component can gain access to command line params before final-ui-startup.

Comment 8

[Simon Bünzli]

Is there any reason for not setting nsIPrivateBrowsingService:autoStarted when this command line flag is set (so that all callers that already handle starting up in PB mode will also handle this flag correctly without any further change)?

Comment 9

[(no longer active)]

(In reply to comment #8)
> Is there any reason for not setting nsIPrivateBrowsingService:autoStarted when
> this command line flag is set (so that all callers that already handle starting
> up in PB mode will also handle this flag correctly without any further change)?

I do set it - the problem is that the command line arguments are handled after final-ui-startup, which means at that point nsSessionStartup.init has already been called.  :-(

Comment 10

[Simon Bünzli]

(In reply to comment #9)
> I do set it - the problem is that the command line arguments are handled after
> final-ui-startup

So wouldn't the solution be to set it already when observing "command-line-startup" instead of waiting as long as you currently do? IOW: The code you're adding to nsSessionStartup.js could just as well go into nsPrivateBrowsingService in order to make autoStarted right soon enough.

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #9)
> > I do set it - the problem is that the command line arguments are handled after
> > final-ui-startup
> 
> So wouldn't the solution be to set it already when observing
> "command-line-startup" instead of waiting as long as you currently do? IOW: The
> code you're adding to nsSessionStartup.js could just as well go into
> nsPrivateBrowsingService in order to make autoStarted right soon enough.

Yes, I think that would work.  I'll give it a shot and submit a new patch.

Comment 12

[(no longer active)]

Attachment: Patch (v2)

Move the command-line-startup handling to nsPrivateBrowsingService

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 396129 [details] [diff] [review]
Patch (v2)

r=me on the browser bits, will let bsmedberg weigh in on the rest, but seems kosher to me.

Comment 14

[(no longer active)]

I think the blocking-firefox3.6 decision needs to be reconsidered now that bug 471997 has been deemed as a 3.6 blocker.

Comment 15

[Benjamin Smedberg]

Comment on attachment 396129 [details] [diff] [review]
Patch (v2)

I really don't like this: I wish session restore was able to hook in to something later, like when the first browser window opens, or the default command-line handling happens.

Anyway, this is good enough and prevents our house of cards from tumbling down.

Comment 16

[Benjamin Smedberg]

This really really needs to end up in litmus.

Comment 17

[(no longer active)]

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

Comment 18

[Jim Jeffery not reading bug-mail 1/2/11]

Testing with latest hourly, this seems to be fixed, no longer seeing prvious tab set loaded, and get the Minefield Start page. 

I am seeing in Console2 however two errors:
Error: this._prefBranch is undefined
Source file: file:///J:/Program%20Files%20(x86)/Minefield/components/nsSessionStore.js
Line: 2616


Error: Exception thrown while processing the private browsing mode change request: [Exception... "'[JavaScript Error: "this._prefBranch is undefined" {file: "file:///J:/Program%20Files%20(x86)/Minefield/components/nsSessionStore.js" line: 2616}]' when calling method: [nsISessionStore::setBrowserState]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///J:/Program%20Files%20(x86)/Minefield/components/nsPrivateBrowsingService.js :: PBS__onAfterPrivateBrowsingModeChange :: line 238"  data: yes]
Source file: file:///J:/Program%20Files%20(x86)/Minefield/components/nsPrivateBrowsingService.js
Line: 390

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090915 Minefield/3.7a1pre Firefox/3.7 (.NET CLR 3.5.30729) ID:20090915050128

cset: http://hg.mozilla.org/mozilla-central/rev/cdcf1519121f

Comment 19

[(no longer active)]

Do you see the same error in hourlies without this patch?  I seem to recall this error happening before...

Comment 20

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/40ca2b5cc972

Comment 21

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #19)
> Do you see the same error in hourlies without this patch?  I seem to recall
> this error happening before...

Yes indeed.  Same error shows in an hourly without patch.  Guess its harmless, as the session appears to not be broken or anything.