502307 - Closing multiple tabs in the last browser window with Private Browsing enabled yields no warning regardless of preference setting

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[tomhuff]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5

When private browsing is enabled, Firefox 3.5 will no longer warn you when closing multiple tabs regardless of the preference setting.

Reproducible: Always

Steps to Reproduce:
1. In the tabs preference, select "Warn me when closing multiple tabs".
2. In the privacy preference, select "Automatically start Firefox in a private browsing session.
3. Close and restart Firefox. Open multiple tabs and choose to close Firefox completely from the menu bar.
Actual Results:  
Firefox will not warn you when closing multiple tabs while using the private browsing preference.

Expected Results:  
Firefox should warn you when closing multiple tabs regardless of whether or not the private browsing feature is in use.

Comment 1

[tomhuff]

With the private browsing feature not in use, Firefox warns you as expected when closing multiple tabs.

Comment 2

[(no longer active)]

This is a regression from the fix in bug 468565: we still need to display the tabbrowser warning prompt when closing the last window inside the private browsing window, because the browser glue prompt never gets shown inside the private browsing mode.

Taking.  Also requesting wanted1.9.1.x since this could be considered a dataloss issue: if a user unintentionally closes her last window inside the PB mode and the browser doesn't prompt here even though it's required to by the preferences, the user loses her tabs and all the work she's done on them with no way of getting them back at all.

Comment 3

[(no longer active)]

Note to self: OR the condition on <http://mxr.mozilla.org/mozilla-central/source/toolkit/content/globalOverlay.js#19> with the private browsing mode current status.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[Neil Deakin]

I'm not following this patch. What happens when private browsing is enabled and only one tab is open? What happens on Mac (which doesn't check the state)?

Comment 6

[tomhuff]

Neil,

I don't understand the one tab question? This bug report was created because no warning is issued when closing a browser session with multiple tabs open while in private browsing mode, regardless of the fact that the preference setting for warning when closing multiple tabs is set.

As for the platform, I am using a Mac running OS X v10.5.7 and Firefox v3.5

Comment 7

[tomhuff]

Sorry, just realized like an idiot you were speaking of the code in the patch! duh!

Comment 8

[(no longer active)]

(In reply to comment #5)
> I'm not following this patch.

Let me clarify.  Normally, closeWindow doesn't prompt when only one window is open, because according to the user prefs, the browser glue code prompts whether to save the session or not.  We disabled that prompt from the browser glue code inside the private browsing mode because it didn't make any sense (i.e., no session was being saved anyway).  This caused the additional problem that this bug is about: now when closing a window with multiple tabs inside the PB mode, if it's not the last window, the tab close warning prompt gets shown like normal.  If that window is the only one remaining, closeWindow refrains from showing the prompt in hopes that the browser glue code shows its own prompt, which is doesn't, so effectively no prompt gets shown.  I merely make sure that if the PB mode is active, the number of windows doesn't affect this decision, because the browser glue code won't generate a prompt anyway.

> What happens when private browsing is enabled and
> only one tab is open?

Nothing special, no prompt is displayed.

> What happens on Mac (which doesn't check the state)?

I don't have a Mac, but by reading the code, Mac is not affected by thing bug, and the fix doesn't apply to it either, because a Mac app may remain open even if all of its windows have been closed.  Marcia, can you please confirm?

Comment 9

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89bf34c42f88

Comment 11

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

>Firefox should warn you when closing multiple tabs regardless of whether or not
>the private browsing feature is in use.

Just to confirm, this makes sense for the state where Firefox is in always on private browsing mode.  But in the case of a temporary private browsing session, the user may want to close the window very quickly, so we should place a dialog in their way (bug 468565).

Comment 12

[Jet]

(In reply to comment #11)
> >Firefox should warn you when closing multiple tabs regardless of whether or not
> >the private browsing feature is in use.
> 
> Just to confirm, this makes sense for the state where Firefox is in always on
> private browsing mode.  But in the case of a temporary private browsing
> session, the user may want to close the window very quickly, so we should place
> a dialog in their way (bug 468565).

It makes sense also to grey out options that cancel each other

Comment 13

[(no longer active)]

(In reply to comment #11)
> >Firefox should warn you when closing multiple tabs regardless of whether or not
> >the private browsing feature is in use.
> 
> Just to confirm, this makes sense for the state where Firefox is in always on
> private browsing mode.  But in the case of a temporary private browsing
> session, the user may want to close the window very quickly, so we should place
> a dialog in their way (bug 468565).

So, do you suggest to modify the patch in this bug to that effect?

Comment 14

[Jet]

(In reply to comment #13)
> (In reply to comment #11)
> > >Firefox should warn you when closing multiple tabs regardless of whether or not
> > >the private browsing feature is in use.
> > 
> > Just to confirm, this makes sense for the state where Firefox is in always on
> > private browsing mode.  But in the case of a temporary private browsing
> > session, the user may want to close the window very quickly, so we should place
> > a dialog in their way (bug 468565).
> 
> So, do you suggest to modify the patch in this bug to that effect?

I suggest this elusive patch to include also the greying out of options that cancel each other out....

This is not a new bug and yet it has not been fixed in 5 releases of firefox or more.

Comment 15

[(no longer active)]

(In reply to comment #14)
> I suggest this elusive patch to include also the greying out of options that
> cancel each other out....

Can you please elaborate what you mean here?

Comment 16

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

>So, do you suggest to modify the patch in this bug to that effect?

Yeah, in the case of always on private browsing there is a concern of potential dataloss.  In the case of a temporary private browsing session, letting the user quiet very quickly I think takes priority, so we shouldn't display the dialog.

>> I suggest this elusive patch to include also the greying out of options that
>> cancel each other out....
>
>Can you please elaborate what you mean here?

I'm afraid I'm not following that either, if we do display the dialog for always on private browsing mode, then the options aren't really canceling each other out anymore.  For a temporary session, the options don't really apply.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 388086 [details] [diff] [review]
Patch (v1)

Apparently this patch doesn't have ui-r and needs it as per previous comments.

Alex: I understand what you're saying; quick exit of PB mode is hindered by this approach, and users perhaps want that. At the same time, I don't think we want to vary to wildly between "always PB" and "instance PB" modes as it makes testing and predicting behaviour pretty difficult.

My recommendation would be to not make any changes until we've heard more user feedback on this issue. It's unclear to me that this is a problem in need of a fix, or if we accidentally made the right design choice, here!

Comment 18

[tomhuff]

Mike:

I would have to comment that if this is going to be a design feature of Firefox, and hence not be patched, that the following be changed to more logically represent the preferences:

In Firefox>Preferences>Privacy if "Automatically start Firefox in a private browsing session" is checked, then the condition of that check should be that it grays out the "Warn me when closing multiple tabs" option located in Firefox>Preferences>Tabs.

I don't quite understand the speed issue when closing a private browsing session? If someone is looking at porn at work, they could simply Command>Tab>Q to quit Firefox as fast as they could use the mouse on a Mac. And if using Windows, then make use of the Windows Key>M to minimize all Windows instantly.

With that in mind, and if the patch is going to be thrown out in favor of classifying the current actions as a design feature, then the option above to gray out closing multiple tabs when in private browsing should definitely be invoked to logically make more since out of the application preferences, and behavior thereafter.

Any thoughts?

Comment 19

[Jet]

To all the patchers.

I agree with Mike and that has been my line of reasoning all along, your so called "design feature" is indeed a bug, and should be handled by the way mike described it and I posted on my original bug report which you attavhed to this instead and yet has not been fixed, but rather just left to age with time passing.

Comment 20

[(no longer active)]

(In reply to comment #17)
> Alex: I understand what you're saying; quick exit of PB mode is hindered by
> this approach, and users perhaps want that. At the same time, I don't think we
> want to vary to wildly between "always PB" and "instance PB" modes as it makes
> testing and predicting behaviour pretty difficult.
> 
> My recommendation would be to not make any changes until we've heard more user
> feedback on this issue. It's unclear to me that this is a problem in need of a
> fix, or if we accidentally made the right design choice, here!

By "not making any changes", do you mean to back this bug out so that we get the same behavior as 1.9.1 (no warning when closing multiple tabs in both always PB and instance PB modes), leaving things as they are (no warning when closing multiple tabs in both always PB and instance PB modes on 1.9.1, and the usual close multiple tabs warning on trunk in both always PB and instance PB modes), or incorporating Alex's suggestion in comment 16 (no warning when closing multiple tabs in both always PB and instance PB modes on 1.9.1, the usual close multiple tabs warning on trunk in the always PB mode, and no warning on trunk in the instance PB mode)?

Sorry if this is exceedingly confusing. :-)

Comment 21

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

>do you mean to back this bug out so that we get
>the same behavior as 1.9.1

I'm a little confused on what the current and patched behavior is, but here is the intended proposed behavior:

PB instance: no close multiple tab warning (in case the user needs to close the browser window quickly

PB always on mode: same behavior as normally running Firefox, with the close multiple tab warning.

Comment 22

[(no longer active)]

Re-opening to implement the behavior Alex described in comment 21.

Comment 23

[(no longer active)]

Attachment: Patch (v2)

Requesting review from Dao on browser/ part and from enndeakin on toolkit/ part.

Comment 24

[(no longer active)]

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

This implements comment 21.

Comment 25

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

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

Thanks, sorry about all of the confusion.

Comment 26

[Dão Gottwald [:dao]]

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

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> #ifdef XP_MACOSX
>   // OS X doesn't quit the application when the last window is closed, but keeps
>   // the session alive. Hence don't prompt users to save tabs, but warn about
>   // closing multiple tabs.
>   return gBrowser.warnAboutClosingTabs(true);
> #else
>-  return true;
>+  // If we're inside auto-started private browsing mode, always warn
>+  // about closing multiple tabs (bug 502307).
>+  if (gPrivateBrowsingUI.privateBrowsingEnabled &&
>+      gPrivateBrowsingUI.autoStarted)
>+    return gBrowser.warnAboutClosingTabs(true);
>+  else
>+    return true;
> #endif
> }

nit: remove the else after return.

Maybe also consider re-organizing the code like this:

> #ifdef XP_MACOSX
>   // On OS X, always warn about closing multiple tabs, as closing the
>   // last window won't quit the application.
> #else
>   // If we're inside auto-started private browsing mode, always warn
>   // about closing multiple tabs (bug 502307).
>   if (gPrivateBrowsingUI.privateBrowsingEnabled &&
>       gPrivateBrowsingUI.autoStarted)
> #endif
>     return gBrowser.warnAboutClosingTabs(true);
> 
>   return true;

Comment 27

[[e-mail]]

Comment on attachment 388086 [details] [diff] [review]
Patch (v1)

>+  var inPrivateBrowsing = false;
>+  try {
>+    var pbSvc = Components.classes["@mozilla.org/privatebrowsing;1"]
>+                          .getService(Components.interfaces.nsIPrivateBrowsingService);
>+    inPrivateBrowsing = pbSvc.privateBrowsingEnabled;
>+  } catch(e) {
>+    // safe to ignore
Doesn't help with the strict JS warning though.

Comment 28

[[e-mail]]

Attachment: Fix strict JS warning

(Also fixes the resulting XPConnect assertion caused by generating a console message while the JS console is closing.)

Comment 29

[(no longer active)]

Attachment: Patch (v3)

This patch incorporates Dao's comment, and also applies Neil's patch to the new version.  Re-requesting Neil's review request from enndeakin.

Comment 30

[Jet]

please realise that its not just private browsing which causes this behaiviour history switch off causes same issue. Not only that the warning check boxes should be cross cancelling greing out the dependants settings to behave as expected in such events, its not just a question of pumping out a warning the UI's respective settings must IMO respect this and be disabled for ease of understanding.

Comment 31

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/405a715a4d81

Comment 32

[(no longer active)]

(In reply to comment #30)
> please realise that its not just private browsing which causes this behaiviour
> history switch off causes same issue. Not only that the warning check boxes
> should be cross cancelling greing out the dependants settings to behave as
> expected in such events, its not just a question of pumping out a warning the
> UI's respective settings must IMO respect this and be disabled for ease of
> understanding.

If by "history switch off" you mean the "never remember history" option, it's merely an always-on private browsing mode.

Comment 33

[Dave Townsend [:mossop]]

I've backed this out due to the test failures on windows and linux:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1250665586.1250671232.12054.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1250665586.1250671250.12070.gz&fulltext=1

Comment 34

[(no longer active)]

Comment on attachment 394546 [details] [diff] [review]
Patch (v3)

Failures:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_248970_a.js | outside private browsing - sessionStore.js timestamp has not changed - Got 1250671929086, expected 1250671929102
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_354894.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | Recently Closed Windows are removed when entering Private Browsing - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | Recently Closed Windows data is cleared when entering Private Browsing - Got [{"tabs":[{"entries":[{"url":"about:config","ID":226,"owner_b64":"SmIS26zLEdO3ZQBgsLbOywAAAAAAAAAAwAAAAAAAAEY=","scroll":"0,0","formdata":{"#textbox":""}}],"index":1,"attributes":{},"_formDataSaved":true},{"entries":[],"attributes":{}}],"selected":1,"_closedTabs":[],"extData":{"bug 394759 Non-PB":"unik1250671968045"},"_hosts":{},"width":994,"height":986,"screenX":4,"screenY":4,"sizemode":"normal","title":"about:config"}], expected []
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | The correct number of recently closed windows were restored when exiting PB mode - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | The data associated with the recently closed window was restored when exiting PB mode
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_448741.js | data contains our value?
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_448741.js | found and removed the specific tab value
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_448741.js | ready to check the cleaned state?
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/docshell/test/browser/browser_bug134911.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/docshell/test/browser/browser_bug92473.js | Timed out

Comment 36

[Jet]

is this ever going to be fixed?

Comment 38

[qwertyqw]

Reported: 2009-07-03
When it will be fixed???!!!

Comment 39

[[not reading bugmail]]

Have you tested the latest FF4 beta to see if its been fixed by another bug?

Comment 41

[Thomas Ahlblom]

Reproduced:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.27) Gecko/20120216 Firefox/3.6.27
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a2) Gecko/20120227 Firefox/12.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120227 Firefox/13.0a1

Comment 42

[(no longer active)]

Josh, do you know any contributors who could work on this?  This requires a bit of front-end hacking experience.  Basically someone needs to take the old patch, see what it breaks these days and figure out why...

Comment 43

[Quentin Headen [:qheaden]]

@Ehsan

I'm assigning myself to this one. I'll try the patch, see if it breaks anything, and try to fix any broken tests.

I will take this little pest of a bug off of your shoulders. :)

Comment 44

[(no longer active)]

This bug will be fixed by bug 722994.

Comment 45

[realgrouchy]

Um, bad news folks: I think this bug still exists (OS X 10.8.2). In v19.0.2 if you have two tabs open in private browsing mode, cmd+q will close without prompting (I tested with TabMixPlus disabled and confirmed all warnings were set to true in about:config, including ShowQuitWarning).

The error also occurs in v20.0 (which I just upgraded to): if all you have open is one private-browsing window with two tabs, it will close without warning (undesired behaviour)

Workaround: luckily in 20.0, I see that private browsing opens a new window and keeps the non-private browsing window open (this is really cool and I applaud everyone who worked to make that happen!). One tab in a private browsing window and one tab in a non-private browsing window will force the quit warning to display (desired behaviour).

I haven't tested in safe mode etc, because the new private-browsing handling will make this error sufficiently unlikely for me.

- RG>