467565 - Private browsing doesn't respect "Quit Firefox" Dialog choice if browser.privatebrowsing.autostart is set to true

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P3)

Description

[Tracy Walker [:tracy]]

Seen on 3.1beta2 on Mac and minefield nightly on Windows XP 

STR:

1) From normal browsing mode, open a few tabs
2) Switch to Tools > Private Browsing
3) Go to about:config and change browser.privatebrowsing.autostart to 
"true"
Note: steps 2 and 3 are interchangeable.
4) Quit the browser, select "quit" in the save tabs dialog.
5) restart the browser.

Tested results:
the browser starts with the tabs opened in step 1.

Expected results:
The browser starts in private browsing mode with about:privatebrowsing as the only page loaded on start.

Steps to reproduce expected behavior:
1) From normal browsing mode, open a few tabs
2) Go to about:config and change browser.privatebrowsing.autostart to 
"true"
3) Quit the browser, select "quit" in the save tabs dialog.
4) restart the browser.

Comment 1

[(no longer active)]

I think this is expected.  We don't want the browser to behave differently in many aspects if it's set to autostart in private browsing mode.  Another example would be leaving the browser windows' titles alone in this mode.

mconnor, do you agree?

Comment 2

[Tracy Walker [:tracy]]

Once in private browsing mode, we shouldn't be displaying tabs/pages on start that were initially opened during a normal browsing session.  (especially since I clicked "quit" to not remember those tabs)

Comment 3

[(no longer active)]

We never attempt not to remember stiff which happened in non-private browsing mode.  Another example of this behavior would be if we crash inside the private browsing mode.  Next time the browser starts, the tabs/windows as they were prior to starting the private mode will be restored.  Same thing happens if the user quits Firefox while inside the private mode.

Comment 4

[Tracy Walker [:tracy]]

I understand that when returning to normal mode, we'd want that last normal mode session restored.  It just seems odd to be restoring normal mode session tabs when still in private mode?  If the user turns off autostart of private mode, then on restart, restore the last normal mode session.  

The crash during private mode is also odd.  We shouldn't restore the private session as it was for risk of restoring to an unauthorized user. I'd say after normal shutdown on restart begin at private mode page. But at abnormal shutdown, restore last normal browsing session.

Anyway, go ahead an mark this bug as invalid if this is really by design.

Comment 5

[(no longer active)]

(In reply to comment #4)
> I understand that when returning to normal mode, we'd want that last normal
> mode session restored.  It just seems odd to be restoring normal mode session
> tabs when still in private mode?  If the user turns off autostart of private
> mode, then on restart, restore the last normal mode session.  

What's your browser.startup.page pref set to?

> The crash during private mode is also odd.  We shouldn't restore the private
> session as it was for risk of restoring to an unauthorized user. I'd say after
> normal shutdown on restart begin at private mode page. But at abnormal
> shutdown, restore last normal browsing session.

That's what we do.  Maybe I wasn't clear in comment 3.  Sorry for the confusion.

> Anyway, go ahead an mark this bug as invalid if this is really by design.

I think I'll hold off till we hear from mconnor on this.

Comment 6

[Tracy Walker [:tracy]]

>What's your browser.startup.page pref set to?
 that's set to 1

>That's what we do.  Maybe I wasn't clear in comment 3.  Sorry for the
>confusion.

You were clear. I was elaborating too much. :-) 

After a little more testing, I see that the home page is opened on restart when doing the following:
1) From normal browsing mode, open a few tabs
2) Go to about:config and change browser.privatebrowsing.autostart to 
"true"
3) Quit the browser, select "quit" in the save tabs dialog.
4) restart the browser.

That seems correct. My initial expectation should be changed to show user homepage(s). So the bug is why doesn't the same thing happen on restart when the browser.privatebrowsing.autostart pref is changed during or before a private browsing session is active, then Quit the browser, select "quit" in the Quit Firefox dialog.  That choice is not respected on next startup.  The normal session tabs are again restored instead of going to the user homepage.

Comment 7

[Tracy Walker [:tracy]]

ah it doesn't respect the Quit dialog choice at all.

Comment 8

[(no longer active)]

This is a problem introduced by bug 463188.   The source of the problem is that we set the browser.sessionstore.resume_session_once pref to true when entering the private browsing mode to make sure we can recover from crashes cleanly.

I think the solution to this problem would be to set this pref to false when we're in private mode and we explicitly want to quit Firefox.  I think a good place to do that would be <http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#344>.

CCing Simon to see if he can think of something which would break as a result of doing this.

Comment 9

[Simon Bünzli]

IMO we should never display "Save and Quit" while in PB mode (as we won't save what you see, anyway) and just not resume a session at startup when we're already in PB mode once SessionStore is initialized (instead load the data directly into _stateBackup).

The only question left for a UX person would be whether we should keep the last non-PB session around until the user finally exits PB mode or if we just want to drop it if .autostart is set to true (in which case I'd set .resume_session_once in nsSessionStore.js only if !.autostart).

Comment 10

[(no longer active)]

(In reply to comment #9)
> IMO we should never display "Save and Quit" while in PB mode (as we won't save
> what you see, anyway) and just not resume a session at startup when we're
> already in PB mode once SessionStore is initialized (instead load the data
> directly into _stateBackup).

I agree with the first part.  I'm CCing Alex to comment here.

In the second part, you mean when the autostart pref is set to true, right?

> The only question left for a UX person would be whether we should keep the last
> non-PB session around until the user finally exits PB mode or if we just want
> to drop it if .autostart is set to true (in which case I'd set
> .resume_session_once in nsSessionStore.js only if !.autostart).

Keeping that session around may be confusing, because the pref may be toggled a long time after it's turned on, and then restoring an old session would be kind of weird.  But not keeping it around may be bad as well, because someone may just want to experience with this pref, and they could be annoyed to see that setting this pref has deleted their stored session.

I'd personally vote for keeping the session data around, but let's see what Alex thinks here as well.

Comment 11

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

>> IMO we should never display "Save and Quit" while in PB mode (as we won't save
>> what you see, anyway) and just not resume a session at startup when we're
>> already in PB mode once SessionStore is initialized (instead load the data
>> directly into _stateBackup).

>I agree with the first part.  I'm CCing Alex to comment here.

I agree as well. mconnor points out that this could be a good time for us to ask the user if they wanted to switch back to their previous session, or just close Firefox.  I'm not sure I agree, discussion in the follow up bug 468565.

> The only question left for a UX person would be whether we should keep the last
> non-PB session around until the user finally exits PB mode or if we just want
> to drop it if .autostart is set to true 

Just to make sure I understand, .autostart gets set in the UI with the drop down at the top of the newly designed privacy prefpane, right? ("Firefox should: Not remember any private data").  If this is the case, than the user is sending a very clear message that we should drop the session data prior to entering private browsing mode, since the user's overall goal is to prevent Firefox from collecting data.

Comment 12

[(no longer active)]

(In reply to comment #11)
> I agree as well. mconnor points out that this could be a good time for us to
> ask the user if they wanted to switch back to their previous session, or just
> close Firefox.  I'm not sure I agree, discussion in the follow up bug 468565.

OK, I'll comment there.  So, we will handle the Quit dialog in that bug.

> Just to make sure I understand, .autostart gets set in the UI with the drop
> down at the top of the newly designed privacy prefpane, right? ("Firefox
> should: Not remember any private data").

Yes, once that new prefpane lands.

> If this is the case, than the user is
> sending a very clear message that we should drop the session data prior to
> entering private browsing mode, since the user's overall goal is to prevent
> Firefox from collecting data.

Note that we are talking about user's exisiting session in which she sets that pref.  Are you suggesting that we should clear only the session data, and not other private data as we clear in the CPD dialog?  If yes, then why the inconsistency?  If no, then I think we need to ask for user's confirmation before doing that, since clearing *all* of their private data may not be what they really want (use case: when someone tries to experience with this pref with an existing profile with their history, passwords, cookies, etc.

Comment 13

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

>Note that we are talking about user's exisiting session in which she sets that
>pref.  Are you suggesting that we should clear only the session data, and not
>other private data as we clear in the CPD dialog?

Yes, although I'm not entirely sure we are talking about the same thing.  The session data I'm thinking about is only used when the user crashes and we automatically restart (or in multiple crashes show about:sessionrestore).  Once the user enters into always on private browsing mode, it would be odd for a session from months ago to be restored on their first crash.  However, it shouldn't be the case that switching into always on private browsing mode clears your current tab set, or removes any other data that is listed in the clear recent history window (like cache, visited pages, etc.).

Comment 14

[(no longer active)]

(In reply to comment #9)
> IMO we should never display "Save and Quit" while in PB mode (as we won't save
> what you see, anyway) and just not resume a session at startup when we're
> already in PB mode once SessionStore is initialized (instead load the data
> directly into _stateBackup).

Would it be possible for the SessionStore component to be initialized inside the private browsing mode without autostart being set to true?

Comment 15

[(no longer active)]

(In reply to comment #13)
> Yes, although I'm not entirely sure we are talking about the same thing.  The
> session data I'm thinking about is only used when the user crashes and we
> automatically restart (or in multiple crashes show about:sessionrestore).  Once
> the user enters into always on private browsing mode, it would be odd for a
> session from months ago to be restored on their first crash.  However, it
> shouldn't be the case that switching into always on private browsing mode
> clears your current tab set, or removes any other data that is listed in the
> clear recent history window (like cache, visited pages, etc.).

I think we really want to discard the previous session when starting up with autostart set to true, as Simon suggests in comment 9.  But anyway now I see what you mean.

Comment 16

[Simon Bünzli]

(In reply to comment #14)
> Would it be possible for the SessionStore component to be initialized inside
> the private browsing mode without autostart being set to true?

SessionStore is initialized from the first browser window's delayedStartup, so theoretically a user (resp. an extension on her behalf) could already have initialized PB mode.

Comment 18

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 20

[Mike Connor [:mconnor]]

bug 468565 addressed this, no?

Comment 21

[Mike Connor [:mconnor]]

Kinda, but not quite, we should still drop the user's session on shutdown if autostart is true.  Not a UI pref for 3.1, so I'm okay with this getting fixed later.

Comment 22

[(no longer active)]

I have lowered its priority, but I'll try to get to this anyway for 3.1 if time permits.

Comment 23

[(no longer active)]

Tracy, now that the quit dialog is not displayed in PB mode at all (with bug 468565 being fixed), can you reproduce this any more?

Comment 24

[juan becerra [:juanb]]

While testing Fx3.1b3build2 (Build ID: 20090305134136 - litmus test case 7415), when I set the pref to true and I quit the browser, it asks me whether I want to save the tabs. I select to save them. When I restart the browser, the previous session's tabs are opened. The "Stop Private Browsing" option is grayed out.

Should the option to "Stop Private Browsing" be grayed out after setting the browser.privatebrowsing.autostart preference to true?

I would expect the previous session to go away, or to go away until I get out of private browsing mode. And I expect the menu option to get out of private browsing mode to be there.

I could see why this might be by design, but I'm renominating it to get it triaged.

Comment 25

[Tracy Walker [:tracy]]

I still see this bug on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b4pre) Gecko/20090307 Shiretoko/3.1b4pre.  The Quit dialog still appears in PB mode.

Comment 26

[Tracy Walker [:tracy]]

Ok sorry, this is kind of odd.  After a restart the quit dialog no longer appears on quit.  strange? does it take a restart to make about:config  change browser.privatebrowsing.autostart to "true" take effect?

Comment 27

[Nochum Sossonko [:Natch]]

yes afaik

Comment 28

[Mike Connor [:mconnor]]

Currently, yes, setting the pref doesn't immediately force the user into private browsing mode, so anything that depends on being in private browsing mode won't happen.

That said, I think that it should, but we should file a new bug on getting the right behaviour when changing the pref.  This bug isn't it, and this bug should be FIXED now, since other than the transition case, things work properly.

Comment 29

[Tracy Walker [:tracy]]

yep, other than the transitional case, it works with the build mentioned in comment #26

Comment 30

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

(In reply to comment #28)
> That said, I think that it should, but we should file a new bug on getting the
> right behaviour when changing the pref.  This bug isn't it, and this bug should
> be FIXED now, since other than the transition case, things work properly.

Independent from this bug I have filed bug 482334 yesterday.

Comment 31

[(no longer active)]

(In reply to comment #28)
> That said, I think that it should, but we should file a new bug on getting the
> right behaviour when changing the pref.  This bug isn't it, and this bug should
> be FIXED now, since other than the transition case, things work properly.

Filed bug 482430.

Comment 32

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090513 Shiretoko/3.5b5pre ID:20090513030639