Last Comment Bug 502307 - Closing multiple tabs in the last browser window with Private Browsing enabled yields no warning regardless of preference setting
: Closing multiple tabs in the last browser window with Private Browsing enable...
Status: RESOLVED FIXED
[fixed by bug 722994]
: dataloss, regression
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: Trunk
: All All
: -- major with 2 votes (vote)
: Firefox 16
Assigned To: Quentin Headen [:qheaden]
:
: :Ehsan Akhgari
Mentors:
: 505117 566052 638737 650734 (view as bug list)
Depends on:
Blocks: 468565
  Show dependency treegraph
 
Reported: 2009-07-03 18:44 PDT by tomhuff
Modified: 2013-04-18 19:49 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected
affected
affected
?


Attachments
Patch (v1) (1.05 KB, patch)
2009-07-11 11:25 PDT, :Ehsan Akhgari
enndeakin: review+
Details | Diff | Splinter Review
Patch (v2) (2.79 KB, patch)
2009-08-06 02:35 PDT, :Ehsan Akhgari
enndeakin: review+
dao+bmo: review+
faaborg: ui‑review+
Details | Diff | Splinter Review
Fix strict JS warning (687 bytes, patch)
2009-08-14 09:09 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Patch (v3) (3.35 KB, patch)
2009-08-14 11:48 PDT, :Ehsan Akhgari
enndeakin: review+
Details | Diff | Splinter Review

Description tomhuff 2009-07-03 18:44:30 PDT
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 2009-07-03 18:47:22 PDT
With the private browsing feature not in use, Firefox warns you as expected when closing multiple tabs.
Comment 2 :Ehsan Akhgari 2009-07-04 05:14:44 PDT
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 :Ehsan Akhgari 2009-07-04 05:20:38 PDT
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 :Ehsan Akhgari 2009-07-11 11:25:23 PDT
Created attachment 388086 [details] [diff] [review]
Patch (v1)
Comment 5 Neil Deakin 2009-07-13 04:52:30 PDT
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 2009-07-13 17:03:14 PDT
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 2009-07-13 17:03:55 PDT
Sorry, just realized like an idiot you were speaking of the code in the patch! duh!
Comment 8 :Ehsan Akhgari 2009-07-14 05:08:42 PDT
(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 10 :Ehsan Akhgari 2009-07-19 10:39:30 PDT
*** Bug 505117 has been marked as a duplicate of this bug. ***
Comment 11 Alex Faaborg [:faaborg] (Firefox UX) 2009-07-20 21:14:55 PDT
>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 X3 2009-07-21 05:00:45 PDT
(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 :Ehsan Akhgari 2009-07-22 00:57:56 PDT
(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 X3 2009-07-22 01:19:19 PDT
(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 :Ehsan Akhgari 2009-07-22 01:44:03 PDT
(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) 2009-07-22 18:05:02 PDT
>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] 2009-07-23 09:54:11 PDT
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 2009-07-23 11:20:35 PDT
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 X3 2009-07-24 02:25:32 PDT
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 :Ehsan Akhgari 2009-07-25 02:13:43 PDT
(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) 2009-08-04 14:29:26 PDT
>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 :Ehsan Akhgari 2009-08-06 01:31:52 PDT
Re-opening to implement the behavior Alex described in comment 21.
Comment 23 :Ehsan Akhgari 2009-08-06 02:35:45 PDT
Created attachment 392900 [details] [diff] [review]
Patch (v2)

Requesting review from Dao on browser/ part and from enndeakin on toolkit/ part.
Comment 24 :Ehsan Akhgari 2009-08-06 02:36:48 PDT
Comment on attachment 392900 [details] [diff] [review]
Patch (v2)

This implements comment 21.
Comment 25 Alex Faaborg [:faaborg] (Firefox UX) 2009-08-06 03:03:06 PDT
Comment on attachment 392900 [details] [diff] [review]
Patch (v2)

Thanks, sorry about all of the confusion.
Comment 26 Dão Gottwald [:dao] 2009-08-07 14:03:51 PDT
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 neil@parkwaycc.co.uk 2009-08-14 09:02:20 PDT
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 neil@parkwaycc.co.uk 2009-08-14 09:09:21 PDT
Created attachment 394516 [details] [diff] [review]
Fix strict JS warning

(Also fixes the resulting XPConnect assertion caused by generating a console message while the JS console is closing.)
Comment 29 :Ehsan Akhgari 2009-08-14 11:48:50 PDT
Created attachment 394546 [details] [diff] [review]
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 X3 2009-08-18 04:50:18 PDT
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 32 :Ehsan Akhgari 2009-08-19 00:06:36 PDT
(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 34 :Ehsan Akhgari 2009-08-19 03:07:28 PDT
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 35 :Ehsan Akhgari 2010-05-14 18:03:54 PDT
*** Bug 566052 has been marked as a duplicate of this bug. ***
Comment 36 X3 2011-02-23 01:36:21 PST
is this ever going to be fixed?
Comment 37 Alexander L. Slovesnik 2011-03-04 07:19:18 PST
*** Bug 638737 has been marked as a duplicate of this bug. ***
Comment 38 qwertyqw 2011-03-05 11:16:26 PST
Reported: 2009-07-03
When it will be fixed???!!!
Comment 39 [not reading bugmail] 2011-03-05 13:09:09 PST
Have you tested the latest FF4 beta to see if its been fixed by another bug?
Comment 40 Thomas Ahlblom 2011-04-18 00:28:47 PDT
*** Bug 650734 has been marked as a duplicate of this bug. ***
Comment 41 Thomas Ahlblom 2012-02-27 09:04:33 PST
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 :Ehsan Akhgari 2012-02-27 19:53:39 PST
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] 2012-03-02 22:36:53 PST
@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 :Ehsan Akhgari 2012-06-27 20:55:49 PDT
This bug will be fixed by bug 722994.
Comment 45 realgrouchy 2013-04-05 11:20:07 PDT
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>

Note You need to log in before you can comment on or make changes to this bug.