844561 - Firefox prompts when closing private window

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Gabriel Chadwick]

When closing a private browsing window Firefox prompts that you are closing multiples tabs. It should not.

STR :

Open private browsing window.
Open multiple tabs.
Close window.

You get a prompt with 21.0a2 (2013-02-23)

There should not be a prompt as with previous behavior when in private navigation. A user should be able the close rapidly a private window.

Comment 1

[(no longer active)]

Hmm, yes, there should not be a prompt.  What tabs are you opening?  Do you see the prompt all the time?

Comment 2

[Gabriel Chadwick]

I just tested it with a new profile. I opened a private browsing window. The I opened three tabs :

firefox.com
hotmail.com
gmail.com

Got a prompt asking to confirm to close window.

Comment 3

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6410 is the logic that looks wrong to me. We enumerate the open browser windows, and if any of the other ones have a visible toolbar, we end up prompting to close the current one.

Comment 4

[(no longer active)]

(In reply to comment #3)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6410
> is the logic that looks wrong to me. We enumerate the open browser windows, and
> if any of the other ones have a visible toolbar, we end up prompting to close
> the current one.

Yes, you're right.  Do you want to take this, Josh?  Thanks!

Comment 5

[Josh Matthews [:jdm]]

Attachment: Avoid prompting about closing private windows unnecessarily.

Comment 6

[(no longer active)]

Comment on attachment 719074 [details] [diff] [review]
Avoid prompting about closing private windows unnecessarily.

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

::: browser/base/content/test/browser_private_no_prompt.js
@@ +2,5 @@
> +  waitForExplicitFinish();
> +  var privateWin = OpenBrowserWindow({private: true});
> +  privateWin.addEventListener("load", function onload() {
> +    privateWin.removeEventListener("load", onload, false);
> +    ok(true, "Load listener called");

Nit: this is technically unnecessary as there is a failure case without this check (the test timing out.)

@@ +7,5 @@
> +
> +    privateWin.BrowserOpenTab();
> +    privateWin.BrowserTryToCloseWindow();
> +    ok(true, "didn't prompt");
> +    finish();                        

Nit: trailing space!  (Man it's been a while since I gave you this kind of nit, hasn't it?  :)

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

It's a regression but merely annoys, not breaks, the experience so please nominate for uplift when ready if low-risk.

Comment 8

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61e57c281b9a

Comment 9

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0e052d44c6

Comment 10

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c92da2ea3d7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Man, you and Ehsan are on a roll today. Backed out for mochitest b-c failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29a926e2bdb

https://tbpl.mozilla.org/php/getParsedLog.php?id=20345988&tree=Mozilla-Inbound

09:28:07     INFO -  TEST-START | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js
09:28:07     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | Console message: OpenGL LayerManager Initialized Succesfully.
09:28:07     INFO -  Version: 2.1 APPLE-8.0.51
09:28:07     INFO -  Vendor: Intel Inc.
09:28:07     INFO -  Renderer: Intel HD Graphics 3000 OpenGL Engine
09:28:07     INFO -  FBO Texture Target: TEXTURE_2D
09:28:07     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | Load listener called
09:28:07     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | Console message: OpenGL LayerManager Initialized Succesfully.
09:28:07     INFO -  Version: 2.1 APPLE-8.0.51
09:28:07     INFO -  Vendor: Intel Inc.
09:28:07     INFO -  Renderer: Intel HD Graphics 3000 OpenGL Engine
09:28:07     INFO -  FBO Texture Target: TEXTURE_2D
09:28:37  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | Test timed out
09:28:37  WARNING -  This is a harness error.
09:28:37     INFO -  args: ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png', '/var/folders/np/253ng38x5ln5h1fwm2gjh92h00000w/T/mozilla-test-fail_5dulBc']
09:28:37     INFO -  libpng warning: zero length keyword
09:28:37     INFO -  libpng warning: Empty language field in iTXt chunk
09:28:39     INFO -  SCREENSHOT: <see log>
09:28:39     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | finished in 30010ms
09:28:39     INFO -  TEST-INFO | checking window state
09:28:39  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | Found a browser window after previous test timed out
09:28:39  WARNING -  This is a harness error.
09:28:39  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | Found a  after previous test timed out
09:28:39  WARNING -  This is a harness error.
09:28:39     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_private_no_prompt.js | must wait for focus
09:28:39     INFO -  TEST-START | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | one tab is open initially
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | tab without referrer was opened to the far right
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | tab without referrer was opened to the far right
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | tab with referrer opened immediately to the right
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | next tab with referrer opened further to the right
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | tab selection changed, tab opens immediately to the right
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | blank tab with referrer opens to the right of 3rd original tab where removed tab was
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | tab has moved, new tab opens immediately to the right
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | blank tab without referrer opens at the end
09:28:39     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | tab without referrer opens at the end
09:28:39     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | finished in 294ms
09:28:39     INFO -  TEST-INFO | checking window state
09:28:39  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_relatedTabs.js | Found an unexpected browser window at the end of test run
09:28:39  WARNING -  This is a harness error.

Comment 12

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdeb0fd118ba

I tested it locally on OS X, so this should be bounce-free.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bdeb0fd118ba

Comment 14

[Gabriel Chadwick]

Will this be pushed to Aurora or Beta?

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 719074 [details] [diff] [review]
Avoid prompting about closing private windows unnecessarily.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 729867
User impact if declined: Private windows are harder to close in a hurry.
Testing completed (on m-c, etc.): m-c, automated test
Risk to taking this patch (and alternatives if risky): Risk is negligible; it's a very small, well-scoped behaviour change.
String or UUID changes made by this patch: None.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 719074 [details] [diff] [review]
Avoid prompting about closing private windows unnecessarily.

Looks good, small low-risk change, please land today so it's in tomorrow's Beta.

Comment 17

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da71f1dc78cc
https://hg.mozilla.org/releases/mozilla-beta/rev/ce3612642928

Comment 18

[Gabriel Chadwick]

Verified fixed with Mozilla/5.0 (Windows NT 6.2; WOW64; rv:21.0) Gecko/20130312 Firefox/21.0

Comment 19

[u279076]

Thanks Gabriel, can you please verify against the latest Firefox 22 Nightly and 22 Beta builds?

Comment 20

[Gabriel Chadwick]

Verified fixed with Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20130315 Firefox/22.0

Comment 21

[Gabriel Chadwick]

Not fixed with Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 wich is latest beta I can find.

Comment 22

[u279076]

(In reply to Gabriel Mayrand Chadwick from comment #21)
> Not fixed with Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20100101
> Firefox/20.0 wich is latest beta I can find.

This landed on Beta on March 11. Can you please tell me the value of browser.startup.homepage_override.buildID from Help > Troubleshooting Information? If you are using the latest Beta (20.0b5) it should be 20130313170052. If not, you'll want to update.

Can you please confirm you are definitely using the latest Beta?

Comment 23

[Gabriel Chadwick]

(Windows NT 6.2; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 is the build available at http://www.mozilla.org/fr/firefox/beta/

I can't find a build with this fixed.

Comment 24

[Josh Matthews [:jdm]]

For me, that link tries to download Beta 4.

Comment 25

[Josh Matthews [:jdm]]

Try a build from http://download.cdn.mozilla.net/pub/mozilla.org/firefox/releases/20.0b5/ .

Comment 26

[Gabriel Chadwick]

(In reply to Josh Matthews [:jdm] from comment #25)
> Try a build from
> http://download.cdn.mozilla.net/pub/mozilla.org/firefox/releases/20.0b5/ .

When I download a build from that emplacement. I does not change the user agent string and it does not fix the bug. I downloaded the french build.

Comment 27

[u279076]

(In reply to Gabriel Mayrand Chadwick from comment #26)
> (In reply to Josh Matthews [:jdm] from comment #25)
> > Try a build from
> > http://download.cdn.mozilla.net/pub/mozilla.org/firefox/releases/20.0b5/ .
> 
> It does not change the user agent string

The user agent string will remain unchanged. What should change is the build ID from Help > Troubleshooting Information > Important Modified Prefs > browser.startup.homepage_override.buildID

If the value of that preference is greater than 20130311000000 it should be fixed.

Comment 28

[Ioana (away)]

This issue is reproducible on Firefox 20 beta 7 (20130325214615), as well as on the 03/26 Aurora and Nightly.

Comment 29

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

Could you file a new bug, please? We basically should never reopen FIXED bugs that have landed patches.

Comment 30

[Ioana (away)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #29)
> Could you file a new bug, please? We basically should never reopen FIXED
> bugs that have landed patches.

I filed bug 854926.

I don't really agree with what you are saying though. When the bug isn't fixed, it should not be kept as fixed. The REOPENED state exists in the lifecycle of a bug for a reason.

I understand doing this for bugs that were partially fixed by a patch, or when a bug regresses the x-th time, but in this case, it doesn't really make sense.

Comment 31

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

(In reply to Ioana Budnar [QA] from comment #30)
> I filed bug 854926.

Thanks.

> I don't really agree with what you are saying though. When the bug isn't
> fixed, it should not be kept as fixed. The REOPENED state exists in the
> lifecycle of a bug for a reason.

On b.m.o, it exists for when patches are backed out. That's not what's going to happen in this case, and so REOPENED doesn't apply.

Comment 32

[Gabriel Chadwick]

I confirm not fixed with build id 20130325214615 20.0

Comment 33

[u279076]

(In reply to Gabriel Mayrand Chadwick from comment #32)
> I confirm not fixed with build id 20130325214615 20.0

Please see bug 854926.

Comment 34

[u279076]

I'm dropping verifyme from this bug for Firefox 20 since we cannot verify until bug 854926 is fixed.

Comment 35

[Ioana (away)]

Reverting Gabriel's flag change since I can still reproduce this issue on Windows and Linux platforms (see bug 854926).