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

RESOLVED FIXED in mozilla16

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jdm, Assigned: sawrubh)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm][lang=js], )

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

8 years ago
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.
Reporter

Comment 1

7 years ago
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.
OS: Mac OS X → All
Hardware: x86 → All
Reporter

Updated

7 years ago
Whiteboard: [mentor=jdm][lang=js]
Assignee

Comment 2

7 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → saurabhanandiit
Status: NEW → ASSIGNED
Attachment #633915 - Flags: feedback?(ehsan)

Comment 3

7 years ago
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(...
               ...
Attachment #633915 - Flags: feedback?(ehsan) → review+
Assignee

Comment 4

7 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #633915 - Attachment is obsolete: true
Attachment #634178 - Flags: checkin?(ehsan)

Updated

7 years ago
Target Milestone: --- → mozilla16

Comment 5

7 years ago
Comment on attachment 634178 [details] [diff] [review]
Patch v2

Thanks a lot for your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2fdd5d3aae
Attachment #634178 - Flags: checkin?(ehsan) → checkin+
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.
Attachment #634178 - Flags: checkin+ → checkin-
Assignee

Comment 7

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

Comment 8

7 years ago
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

7 years ago
Have you submitted this patch to the try server?
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.
Attachment #634178 - Flags: review-
Reporter

Comment 11

7 years ago
Dão, would other windows have parent browser windows we could check instead?
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

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

Comment 14

7 years ago
(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.
(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

7 years ago
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?
Assignee

Comment 17

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

Comment 18

7 years ago
@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

7 years ago
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.
Assignee

Comment 20

7 years ago
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.
Assignee

Comment 22

7 years ago
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

7 years ago
(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!  :-)
Assignee

Comment 24

7 years ago
Posted patch Patch v3Splinter Review
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.
Attachment #634178 - Attachment is obsolete: true
Attachment #637148 - Flags: feedback?(ehsan)
Assignee

Comment 25

7 years ago
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

7 years ago
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.
Attachment #637148 - Flags: feedback?(ehsan) → review+
Assignee

Comment 28

7 years ago
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.
Attachment #637148 - Flags: checkin?(ehsan)

Updated

7 years ago
Attachment #637148 - Flags: checkin?(ehsan) → checkin+
https://hg.mozilla.org/mozilla-central/rev/bf8f2961d0cc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.