722994 - globalOverlay.js uses global Private Browsing state to control display of a prompt

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Josh Matthews [:jdm]]

The global state is going away. If possible, docshell queries should be used instead, or we may need to add a window-level PB status.

Comment 1

[Josh Matthews [:jdm]]

We should just be able to duplicate the code from _getBrowserWindowForContentWindow (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6426) and check gPrivateBrowsingUI.privateWindow on the result instead of the existing global state check.

Comment 2

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

Comment 3

[(no longer active)]

Comment on attachment 633915 [details] [diff] [review]
Patch v1

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

r=me with the below comment.  Please submit a patch with this comment addressed for check-in.

::: toolkit/content/globalOverlay.js
@@ +23,5 @@
> +                                .QueryInterface(Ci.nsIDocShellTreeItem)
> +                                .rootTreeItem
> +                                .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIDOMWindow)
> +                                .wrappedJSObject;

Please fix the spacing here, like this:

   ... = window.QueryInterface(...
               .getInterface(...
               ...

Comment 4

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

Comment 5

[(no longer active)]

Comment on attachment 634178 [details] [diff] [review]
Patch v2

Thanks a lot for your patch!

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

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 634178 [details] [diff] [review]
Patch v2

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/19bfe36cace8 - varying numbers of mochitest-chrome failures, starting with a timeout in test_close_download_manager.xul, a la https://tbpl.mozilla.org/php/getParsedLog.php?id=12778051&tree=Mozilla-Inbound.

Comment 7

[Saurabh Anand [:sawrubh]]

Sorry Philor, for my patch causing troubles. I'll look into the reasons as soon as possible.

Comment 8

[Saurabh Anand [:sawrubh]]

I asked Ed Morley on IRC and he said that most probably this regression is not caused by me. But I'm still looking.

Comment 9

[(no longer active)]

Have you submitted this patch to the try server?

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 634178 [details] [diff] [review]
Patch v2

This patch is completely bogus. gPrivateBrowsingUI only exists in browser windows, not in any other window using globalOverlay.js.

I just saw this in the pushlog and was going to back it out. Happy to see this already happened thanks to the test suite catching this.

Comment 11

[Josh Matthews [:jdm]]

Dão, would other windows have parent browser windows we could check instead?

Comment 12

[Dão Gottwald [:dao]]

They might have an opener window, but 1) none of them cares whether the opener window was in private browsing mode, so I don't see how it would be a useful check, and 2) the opener window would be gone in this case, since inPrivateBrowsing only comes into play here if windowCount == 1.

I'm not sure why this code takes private browsing into account, but it might be the case that it should just stop doing this given the move to private windows instead of a global private browsing mode.

Comment 13

[Philip Chee]

globalOverlay.js is used by more than just Firefox. Some consumers may not even have the concept of a "browser" window.

Comment 14

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #12)
> They might have an opener window, but 1) none of them cares whether the
> opener window was in private browsing mode, so I don't see how it would be a
> useful check, and 2) the opener window would be gone in this case, since
> inPrivateBrowsing only comes into play here if windowCount == 1.
> 
> I'm not sure why this code takes private browsing into account, but it might
> be the case that it should just stop doing this given the move to private
> windows instead of a global private browsing mode.

Bug 502307 would still be a concern, right?  I think we should just check for the existence of gPrivateBrowsingUI as well.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > They might have an opener window, but 1) none of them cares whether the
> > opener window was in private browsing mode, so I don't see how it would be a
> > useful check, and 2) the opener window would be gone in this case, since
> > inPrivateBrowsing only comes into play here if windowCount == 1.
> > 
> > I'm not sure why this code takes private browsing into account, but it might
> > be the case that it should just stop doing this given the move to private
> > windows instead of a global private browsing mode.
> 
> Bug 502307 would still be a concern, right?

Could you elaborate? That bug claims that "Firefox should warn you when closing multiple tabs regardless of whether or not the private browsing feature is in use", so my naive thinking would be that the code responsible for this shouldn't care about private browsing.

Comment 16

[(no longer active)]

Hmm, after re-reading bug 502307, I'm pretty confused.  I don't remember why the original patch there was needed, and my second one didn't stick.  So perhaps someone needs to take this whole code out and then test whether bug 502307 comment 0 still applies.  :-)

Saurabh, do you want to do that?

Comment 17

[Saurabh Anand [:sawrubh]]

I want to do anything to get PBnGen landed ;) I will get down to doing this soon.

Comment 18

[Saurabh Anand [:sawrubh]]

@Ehsan, can you elaborate what you want me to test. Do you want me test comment 0 of bug 502307 after importing the patch in that bug, or without patch queues applied. I tested in my nightly without having applied any patches, neither this one nor the patch submitted in Bug 502307 and that Comment still seems to apply.

I'm not clear how to proceed with this bug. Can someone please clarify it to me.

Comment 19

[(no longer active)]

yes, I meant testing bug 502307 comment 0.  Thanks for doing that!

I did a bit more digging around, and I think I know more now.  This dialog should be shown using this code: <http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#529> which is being skipped by the check on line 499.  This patch <http://hg.mozilla.org/mozilla-central/rev/89bf34c42f88> was landed to mitigate that problem.

Now, I think a proper fix would be the following:

1. Revert http://hg.mozilla.org/mozilla-central/rev/89bf34c42f88.
2. Move this block <http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#495> down to the |if (!showPrompt)| check.
3. The code block in step 2 is using the global PB mode, but that is bug 722984, which has a patch which needs a few more comments addressed.  That patch should be rebased on top of the patch in this one.

If you test a build with steps 1 and 2 above, bug 502307 comment 0 should not happen any longer.

Comment 20

[Saurabh Anand [:sawrubh]]

AFAIK, there is no need for the check in http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#495 since we must show the warning irrespective of the fact whether the browser is in PB mode or not. Please correct me if I am wrong.

@Ehsan, why do you want me to move the block to before the |if (!showPrompt)| check, because then again if the user is in PB mode then the browser will not warn, even though |showPrompt| might have been true.

Comment 21

[Saurabh Anand [:sawrubh]]

I think http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#462 contradicts what Bug 502307 wants.

Comment 22

[Saurabh Anand [:sawrubh]]

For that rebasing, do I need to make the same changes that are being made in bug 722984 in this patch also, because then the same changes will be in two patches and while merging the patches might collide (aka bitrotten). Or will this rebasing be done while landing this patch into the tree ?

I made the changes in Step 1 and 2 and bug 502307 comment 0 is not happening anymore. But still I don't have my doubts from my previous comments.

Comment 23

[(no longer active)]

(In reply to Saurabh Anand [:sawrubh] from comment #20)
> AFAIK, there is no need for the check in
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js#495 since we must show the warning irrespective of the fact
> whether the browser is in PB mode or not. Please correct me if I am wrong.

By default, the quit prompt offers the user the option to save their session.  This is not very meaningful in PB, since we explicitly will not save the users's session in PB mode.  This is why this block of code was added in bug 468565.

> @Ehsan, why do you want me to move the block to before the |if
> (!showPrompt)| check, because then again if the user is in PB mode then the
> browser will not warn, even though |showPrompt| might have been true.

Because that function also has the task of warning the user if they're closing multiple tabs when they have the warn_on_close_multiple_tabs pref set, and the fact that we bail out early of that function is the root cause of bug 502307, I think.

(In reply to Saurabh Anand [:sawrubh] from comment #22)
> For that rebasing, do I need to make the same changes that are being made in
> bug 722984 in this patch also, because then the same changes will be in two
> patches and while merging the patches might collide (aka bitrotten). Or will
> this rebasing be done while landing this patch into the tree ?

No, you can leave that part to bug 722984 to make the review easier.  It will make rebasing on that bug a little bit harder, but the patch there shouldn't be too big.

> I made the changes in Step 1 and 2 and bug 502307 comment 0 is not happening
> anymore. But still I don't have my doubts from my previous comments.

I think that is the desired fix for bug 502307 then!  :-)

Comment 24

[Saurabh Anand [:sawrubh]]

Attachment: Patch v3

This patch includes :

a)Reverts the revisions as pointed by Ehsan.
b)Moves the block of code to after the if-else loop and before the |if(!showPrompt)| check. This does not deliberately remove the current code to access private browsing service and make it access the window's PB flag since that is planned to be done in Bug 722984.

Comment 25

[Saurabh Anand [:sawrubh]]

I'll push to try after I get a feedback+ from Ehsan, and I promise Philor, won't ask for review until get a green tree this time ;)

Comment 26

[(no longer active)]

Comment on attachment 637148 [details] [diff] [review]
Patch v3

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

Please mark for checkin when you get green on try.

Comment 27

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=8ca66050cc01

Comment 28

[Saurabh Anand [:sawrubh]]

Comment on attachment 637148 [details] [diff] [review]
Patch v3

The tree looks pretty green to me. If you feel it's ok, then please checkin this patch.

Comment 29

[(no longer active)]

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

Comment 30

[Ed Morley [:emorley]]

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