952925 - No quit warning or multiple tab quit warning in Private Browsing Mode, even with about:config options set True

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Troy W.]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

1) Enable Private Browsing Mode (Preferences/Privacy/AlwaysUsePrivateBrowsingMode)

2) Enable "Warn me when closing multiple tabs" (Preferences/Tabs)

2) In about:config set the following as True:
---) browser.warnOnQuit 
---) browser.showQuitWarning
---) browser.warnOnRestart
---) browser.tabs.warnOnClose
---) browser.tabs.warnOnCloseOtherTabs

3) Open multiple tabs.

4) Hit Command+Q (Quit Firefox)



Actual results:

Firefox closed without any warning at all.



Expected results:

Firefox should have provided at least one (or both) of the following warnings:

1) A warning that Firefox is closing, asking the user to confirm.
OR...
2) A warning that multiple tabs are being closed, asking the user to confirm.

Additional testing:
Retested using same steps, but with Private Browsing Mode *disabled*. When quitting, Firefox asks user to confirm intention to quit, as expected. Upon re-enabling Private Browsing Mode, incorrect behavior returns. Thus, problem seems to occur only with Private Browsing Mode.

Comment 1

[Troy W.]

Anyone?

Comment 3

[Troy W.]

(In reply to Josh Matthews [:jdm] from comment #2)
> 
> *** This bug has been marked as a duplicate of bug 987821 ***

This is not a duplicate. 987821 refers to temporary private windows. The bug I'm referring to is in reference to when the Enable Private Browsing mode is set in preferences.

Comment 4

[Josh Matthews [:jdm]]

My mistake. That is definitely a problem.

Comment 5

[Troy W.]

Problem persists in Firefox 28. Revising to 28 Branch.

Comment 6

[Troy W.]

Has this bug been forgotten?

Comment 7

[(no longer active)]

Josh, got cycles for this?

Comment 8

[Biru Mohanathas [:poiru]]

Attachment: Show quit warning for private windows as well

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c2655a4697d8

The wording of the new message is similar to: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/tabbrowser.properties#29

Comment 9

[(no longer active)]

Comment on attachment 8455123 [details] [diff] [review]
Show quit warning for private windows as well

Review of attachment 8455123 [details] [diff] [review]:
-----------------------------------------------------------------

The thing you want to differentiate on is permanent private browsing, aka PrivateBrowsingUtils.permanentPrivateBrowsing.  Your patch makes the warning show on normal private windows too which is undesirable.

::: browser/components/nsBrowserGlue.js
@@ +880,5 @@
>          // we should show the window closing warning instead. warnAboutClosing
>          // tabs checks browser.tabs.warnOnClose and returns if it's ok to close
>          // the window. It doesn't actually close the window.
> +        aCancelQuit.data =
> +          !win.gBrowser.warnAboutClosingTabs(win.gBrowser.closingTabsEnum.ALL);

This part is just clean-up, right?

@@ +886,5 @@
>        return;
>      }
>  
> +    let prompt = Services.prompt;
> +    let quitBundle = Services.strings.createBundle("chrome://browser/locale/quitDialog.properties");

I'd rather if you didn't change all of these var's into let's in this patch... In general it's best to keep cleanups and functional changes separate into different patches so that the reviewer can review the individual bits more easily.  Same for the above clean-up.

@@ -889,5 @@
>        return;
>      }
>  
> -    // Never show a prompt inside private browsing mode
> -    if (allWindowsPrivate)

I think the only fix necessary here is to check PrivateBrowsingUtils.permanentPrivateBrowsing as appropriate here.

::: browser/locales/en-US/chrome/browser/quitDialog.properties
@@ +9,5 @@
>  saveTitle=&Save and Quit
>  neverAsk2=&Do not ask next time
>  message=Do you want %S to save your tabs and windows for the next time it starts?
>  messageNoWindows=Do you want %S to save your tabs for the next time it starts?
> +messageQuit=You are about to quit %S. Are you sure you want to continue?

I don't think we want a new message here, why can't we use the existing ones?

Comment 10

[Biru Mohanathas [:poiru]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> Comment on attachment 8455123 [details] [diff] [review]
> Show quit warning for private windows as well
> 
> Review of attachment 8455123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The thing you want to differentiate on is permanent private browsing, aka
> PrivateBrowsingUtils.permanentPrivateBrowsing.  Your patch makes the warning
> show on normal private windows too which is undesirable.

Ah, thanks for the clarification.

> ::: browser/locales/en-US/chrome/browser/quitDialog.properties
> @@ +9,5 @@
> >  saveTitle=&Save and Quit
> >  neverAsk2=&Do not ask next time
> >  message=Do you want %S to save your tabs and windows for the next time it starts?
> >  messageNoWindows=Do you want %S to save your tabs for the next time it starts?
> > +messageQuit=You are about to quit %S. Are you sure you want to continue?
> 
> I don't think we want a new message here, why can't we use the existing ones?

I believe the existing messages are:
- 'You are about to close N tabs. Are you sure you want to continue?', which is misleading if there are multiple windows open.
- 'Do you want Firefox to save your tabs (and windows) for the next time it starts?' (with 'Quit', 'Cancel', and 'Save & Quit' buttons), which is misleading since we don't save anything in private browsing.

Do you know of another existing, more appropriate message?

Comment 11

[(no longer active)]

(In reply to Birunthan Mohanathas [:poiru] from comment #10)
> > ::: browser/locales/en-US/chrome/browser/quitDialog.properties
> > @@ +9,5 @@
> > >  saveTitle=&Save and Quit
> > >  neverAsk2=&Do not ask next time
> > >  message=Do you want %S to save your tabs and windows for the next time it starts?
> > >  messageNoWindows=Do you want %S to save your tabs for the next time it starts?
> > > +messageQuit=You are about to quit %S. Are you sure you want to continue?
> > 
> > I don't think we want a new message here, why can't we use the existing ones?
> 
> I believe the existing messages are:
> - 'You are about to close N tabs. Are you sure you want to continue?', which
> is misleading if there are multiple windows open.
> - 'Do you want Firefox to save your tabs (and windows) for the next time it
> starts?' (with 'Quit', 'Cancel', and 'Save & Quit' buttons), which is
> misleading since we don't save anything in private browsing.
> 
> Do you know of another existing, more appropriate message?

Hmm, yeah you're right.  Gavin, do you have any suggestions?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Sounds like we need a new string, or UX input in general. Can you mail firefox-dev?

Comment 13

[(no longer active)]

email sent!

Comment 14

[Biru Mohanathas [:poiru]]

Should we go ahead with Eric's suggestion?

Comment 15

[(no longer active)]

(In reply to Birunthan Mohanathas [:poiru] from comment #14)
> Should we go ahead with Eric's suggestion?

Sure, nobody else has replied yet, so Eric's wins!

Comment 16

[Biru Mohanathas [:poiru]]

Attachment: Part 1: Clean-up BrowserGlue._onQuitRequest

Comment 17

[Biru Mohanathas [:poiru]]

Attachment: Part 2: Show quit warning for private windows as well

Comment 18

[Biru Mohanathas [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/293166bebf5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dadb81f1db4f

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d5795053e886

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/293166bebf5a
https://hg.mozilla.org/mozilla-central/rev/dadb81f1db4f

Comment 20

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

Reproduced the initial issue on old Nightly (2014-08-07), verified that using latest Nightly on Mac OS X 10.9.4 the issue does not reproduce, after hitting cmd+q a pop-ul appears that alerts the user to Quit Firefox or cancel.