Closed Bug 844561 Opened 7 years ago Closed 7 years ago

Firefox prompts when closing private window

Categories

(Firefox :: Private Browsing, defect)

21 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22
Tracking Status
firefox19 --- unaffected
firefox20 - fixed
firefox21 --- fixed
firefox22 --- fixed

People

(Reporter: chadwickgab+mozilla, Assigned: jdm)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Hmm, yes, there should not be a prompt.  What tabs are you opening?  Do you see the prompt all the time?
Blocks: PBnGen
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
(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!
Assignee: nobody → josh
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?  :)
Attachment #719074 - Flags: review?(ehsan) → review+
It's a regression but merely annoys, not breaks, the experience so please nominate for uplift when ready if low-risk.
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdeb0fd118ba

I tested it locally on OS X, so this should be bounce-free.
https://hg.mozilla.org/mozilla-central/rev/bdeb0fd118ba
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Will this be pushed to Aurora or Beta?
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.
Attachment #719074 - Flags: approval-mozilla-beta?
Attachment #719074 - Flags: approval-mozilla-aurora?
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.
Attachment #719074 - Flags: approval-mozilla-beta?
Attachment #719074 - Flags: approval-mozilla-beta+
Attachment #719074 - Flags: approval-mozilla-aurora?
Attachment #719074 - Flags: approval-mozilla-aurora+
Depends on: 850221
Verified fixed with Mozilla/5.0 (Windows NT 6.2; WOW64; rv:21.0) Gecko/20130312 Firefox/21.0
Thanks Gabriel, can you please verify against the latest Firefox 22 Nightly and 22 Beta builds?
Verified fixed with Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20130315 Firefox/22.0
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.
(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?
(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.
For me, that link tries to download Beta 4.
(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.
(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.
This issue is reproducible on Firefox 20 beta 7 (20130325214615), as well as on the 03/26 Aurora and Nightly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Could you file a new bug, please? We basically should never reopen FIXED bugs that have landed patches.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(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.
I confirm not fixed with build id 20130325214615 20.0
(In reply to Gabriel Mayrand Chadwick from comment #32)
> I confirm not fixed with build id 20130325214615 20.0

Please see bug 854926.
I'm dropping verifyme from this bug for Firefox 20 since we cannot verify until bug 854926 is fixed.
Keywords: verifyme
Reverting Gabriel's flag change since I can still reproduce this issue on Windows and Linux platforms (see bug 854926).
You need to log in before you can comment on or make changes to this bug.