Closed Bug 467565 Opened 16 years ago Closed 15 years ago

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

Categories

(Firefox :: Private Browsing, defect, P3)

Product:
Firefox
Component:
Private Browsing
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3.6a1

People

(Reporter: tracy, Assigned: ehsan.akhgari)

References

Details

(Keywords: verified1.9.1) 

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.
Flags: in-litmus+
Flags: blocking-firefox3.1?
Component: Session Restore → General
QA Contact: session.restore → general
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?
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
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)
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.
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.
(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.
>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.
ah it doesn't respect the Quit dialog choice at all.
Summary: Private browsing doesn't start correctly if browser.privatebrowsing.autostart is set to true → Private browsing doesn't respect "Quit Firefox" Dialog choice if browser.privatebrowsing.autostart is set to true
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.
Blocks: 463188
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).
(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.
>> 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.
(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.
>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.).
(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?
(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.
(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.
Flags: blocking-firefox3.1? → blocking-firefox3.1-
Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.
Component: General → Private Browsing
QA Contact: general → private.browsing
Whiteboard: [PB-P2]
bug 468565 addressed this, no?
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.
Priority: -- → P3
Target Milestone: --- → Firefox 3.2a1
I have lowered its priority, but I'll try to get to this anyway for 3.1 if time permits.
Whiteboard: [PB-P2] → [PB-P3]
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?
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.
Flags: blocking-firefox3.1- → blocking-firefox3.1?
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.
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?
yes afaik
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: blocking-firefox3.1? → blocking-firefox3.1-
Resolution: --- → FIXED
yep, other than the transitional case, it works with the build mentioned in comment #26
Status: RESOLVED → VERIFIED
(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.
(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.
Keywords: fixed1.9.1
Whiteboard: [PB-P3]
verified FIXED on Shiretoko: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090513 Shiretoko/3.5b5pre ID:20090513030639
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.