exceptionDialog.js makes use of the global Private Browsing service to make decisions

RESOLVED FIXED in mozilla19

Status

Core Graveyard
Security: UI
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

(Depends on: 1 bug)

Trunk
mozilla19
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [This UI may be going away soon])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
The global service is going away. The dialog should attempt to query the relevant page docshell instead.
(Reporter)

Comment 1

6 years ago
This looks like a fairly simple change to me - we can just modify inPrivateBrowsingMode() (http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/exceptionDialog.js#397) to use this goop instead of checking the global service:

return window.getInterface(Ci.nsIWebNavigation)
             .QueryInterface(Ci.nsIDocShellTreeItem)
             .treeOwner
             .QueryInterface(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsIXULWindow)
             .docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;
Whiteboard: [mentor=jdm][lang=js]
Assignee: nobody → saurabhanandiit
I think we should be using privateWindow API. I'm CC'ing Dao just to confirm some doubts I have regarding accessing window in the QI Goop.
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #1)
> return window.getInterface(Ci.nsIWebNavigation)
>              .QueryInterface(Ci.nsIDocShellTreeItem)
>              .treeOwner
>              .QueryInterface(Ci.nsIInterfaceRequestor)
>              .getInterface(Ci.nsIXULWindow)
>              .docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;

"window" belongs to exceptionDialog.xul, so I don't see how this is supposed to work. It seems to me that handlePrivateBrowsing (http://mxr.mozilla.org/mozilla-central/search?string=handlePrivateBrowsing) should be replaced with a privateBrowsing flag (browser.js would obviously use the privateWindow API to set that flag).
(Assignee)

Comment 4

6 years ago
(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment
> #1)
> > return window.getInterface(Ci.nsIWebNavigation)
> >              .QueryInterface(Ci.nsIDocShellTreeItem)
> >              .treeOwner
> >              .QueryInterface(Ci.nsIInterfaceRequestor)
> >              .getInterface(Ci.nsIXULWindow)
> >              .docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;
> 
> "window" belongs to exceptionDialog.xul, so I don't see how this is supposed
> to work. It seems to me that handlePrivateBrowsing
> (http://mxr.mozilla.org/mozilla-central/search?string=handlePrivateBrowsing)
> should be replaced with a privateBrowsing flag (browser.js would obviously
> use the privateWindow API to set that flag).

I'd prefer if we just used the privateWindow API off window.opener (with the usual checks to make sure that gPrivateBrowsingUI exists, etc.

That way we can get rid of handlePrivateBrowsing altogether, since other call sites (such as http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/certManager.js#596) will have an opener without gPrivateBrowsingUI.
Created attachment 637167 [details] [diff] [review]
Patch v1

The concerned mochitest (browser_privatebrowsing_certexceptionsui.js) is passing the test locally. I'll push to try after f+ from Ehsan
Attachment #637167 - Flags: feedback?(ehsan)
(Assignee)

Comment 6

6 years ago
Comment on attachment 637167 [details] [diff] [review]
Patch v1

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

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +366,5 @@
>    if (args && args[0] && args[0].handlePrivateBrowsing) {
>      // detect if the private browsing mode is active
>      try {
> +      let opener = window.opener;
> +      if (!opener || !("gPrivateBrowsingUI" in window))

in opener.

@@ +368,5 @@
>      try {
> +      let opener = window.opener;
> +      if (!opener || !("gPrivateBrowsingUI" in window))
> +        return opener.gPrivateBrowsingUI.privateWindow;
> +      return false;

This logic is wrong here, I think.  You want to return false if opener is null or if it doesn't have a gPrivateBrowsingUI property, not the other way around.
Attachment #637167 - Flags: feedback?(ehsan) → feedback-
Created attachment 637179 [details] [diff] [review]
Patch v2

Fixed the faulty logic.
Attachment #637167 - Attachment is obsolete: true
Attachment #637179 - Flags: review?(ehsan)
(Assignee)

Comment 9

6 years ago
Comment on attachment 637179 [details] [diff] [review]
Patch v2

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

Like I said, you should also get rid of handlePrivateBrowsing.
Attachment #637179 - Flags: review?(ehsan) → review-

Comment 10

6 years ago
Try run for d5d653e3c0aa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d5d653e3c0aa
Results (out of 90 total builds):
    exception: 31
    success: 23
    warnings: 3
    failure: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/saurabhanandiit@gmail.com-d5d653e3c0aa
Created attachment 637246 [details] [diff] [review]
Patch v3

Problems currently being faced :

a) I'm getting an error in |step2| of http://dxr.lanedo.com/mozilla-central/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_certexceptionsui.js.html which is failing.
Attachment #637179 - Attachment is obsolete: true
Guys, any ideas ?
(Assignee)

Comment 13

6 years ago
I don't see that failure on your try push...  Can you please post a log of the failure?
@Ehsan, the Try link that I had posted was of the previous patch. The try run for the current patch (Patch v3) is this https://tbpl.mozilla.org/?tree=Try&rev=fdb0be8fe4e0 . One of the failures seems to be intermittent, I am not sure about the other one. Afaict it seems to be an intermittent to me too ;). But please confirm.
(Reporter)

Comment 15

6 years ago
You can remove the whole |args && args[0]| check from inPrivateBrowsingMode. I'm not whether we should be checking for Firefox-only browsing properties in this code instead of cross-product stuff like nsILoadContext; Brian, your thoughts on the matter?
@Josh, should I wait for Brian's comments or should I go ahead with your comment i.e removing |args && args[0]| and using nsILoadContext ?
(Reporter)

Comment 17

6 years ago
Give my feedback a try.
(Assignee)

Comment 18

6 years ago
(In reply to Saurabh Anand [:sawrubh] from comment #14)
> @Ehsan, the Try link that I had posted was of the previous patch. The try
> run for the current patch (Patch v3) is this
> https://tbpl.mozilla.org/?tree=Try&rev=fdb0be8fe4e0 . One of the failures
> seems to be intermittent, I am not sure about the other one. Afaict it seems
> to be an intermittent to me too ;). But please confirm.

Hmm, I don't see any obvious js errors in the logs here which would give me an idea on why this is failing.  Let me know if you want me to debug this.
I'll make the changes proposed by Josh and push that updated patch to try again and then see if the test still fails and only then will debug it if need be.
(Assignee)

Comment 20

6 years ago
(In reply to comment #19)
> I'll make the changes proposed by Josh and push that updated patch to try again
> and then see if the test still fails and only then will debug it if need be.

Sounds good!
Created attachment 640380 [details] [diff] [review]
Patch v4
Attachment #637246 - Attachment is obsolete: true
Attachment #640380 - Flags: feedback?(josh)
Here is the try run for Patch(v4) :

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

The failure that's coming on the local run is :

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_certexceptionsui.js | the permanent checkbox should not be disabled when not handling the private browsing mode
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_certexceptionsui.js | the permanent checkbox should be checked when not handling the private browsing mode

This is the same error that was coming on the last try run (https://tbpl.mozilla.org/?tree=Try&rev=fdb0be8fe4e0) .

I think this might be somewhere connected to removing |handlePrivateBrowsing| from the params object. I searched and couldn't understand whether <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindow.idl#414> might be the thing causing it, because now the param object has one less parameter in it, than what it used to have. I'm not able to figure out the reason for the failure. Any help guys ?
(Assignee)

Comment 23

6 years ago
This is the failure case: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_certexceptionsui.js#51>  We used to respect handlePrivateBrowsing and this section of the test does not pass that, which used to cause the cert exception dialog to not handle anything.  But with your changes, we look at the window opener which still has the gPrivateBrowsingUI member in it, and then continue to handle the private browsing mode.

I think the correct fix here is to open the preferences window in the second case (using the openPreferences window) and call openDialog on that window.  This will look roughly like this:

  let prefWindow = openPreferences();
  prefWindow.addEventListener("load", function() {
    // remove the event handler here

    var win = prefWindow.openDialog(...);
  }, false);
(Assignee)

Comment 24

6 years ago
Also, note bug 774660.
(Reporter)

Updated

6 years ago
Attachment #640380 - Flags: feedback?(josh)
(Reporter)

Comment 25

6 years ago
To get the window object when opening Preferences, you should be able to crib from http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug735471.js#31 - I think it will work in both the in-content and dialog cases.
I'm currently working on something else, and have put this on halt. Hence unassigning myself, so that I don't block someone else.
Assignee: saurabhanandiit → nobody
(Assignee)

Updated

6 years ago
Blocks: 797256
(Assignee)

Comment 27

6 years ago
What needs to happen here is take the current patch, replace the usage of opener.gPrivateBrowsingUI.privateWindow with isWindowPrivate, read comment 23 which describes the test failures we were seeing with it, do what comment 25 suggests, and test it to make sure that it works.  Should be fairly easy.
Whiteboard: [mentor=jdm][lang=js] → [mentor=jdm][lang=js][appcoast]
I am going to mark this as depending on bug 795771, because we may be replacing this UI soon. We already have a first draft of a spec.
Depends on: 795771
Whiteboard: [mentor=jdm][lang=js][appcoast] → [This UI may be going away soon][mentor=jdm][lang=js][appcoast]
Oh, I see this already has a patch. Maybe it doesn't need to depend on bug 795771. Note that this dialog box does an XHR and decides whether to do a permanent or temporary exception. Both of those have implications for private browsing.
(Assignee)

Comment 30

6 years ago
So I was chatting with bsmith over this on IRC and he pointed me to <https://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/exceptionDialog.js#134>, where we create a channel out of thin air, which is a Firefox 18 blocker.  So I guess I'll work on this myself.
Assignee: nobody → ehsan
Blocks: 787743
status-firefox18: --- → fixed
status-firefox19: --- → fixed
tracking-firefox18: --- → ?
tracking-firefox19: --- → +
Whiteboard: [This UI may be going away soon][mentor=jdm][lang=js][appcoast] → [This UI may be going away soon]
(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> So I was chatting with bsmith over this on IRC and he pointed me to
> <https://mxr.mozilla.org/mozilla-central/source/security/manager/pki/
> resources/content/exceptionDialog.js#134>, where we create a channel out of
> thin air, which is a Firefox 18 blocker.  So I guess I'll work on this
> myself.

Also note that Fennec has similar-but-different logic. See mobile/android/chrome/content/exception.js.
(Assignee)

Comment 32

6 years ago
False alarm.  The load context that XHR gets by default should take care of everything XHR-related here.
status-firefox18: fixed → ---
status-firefox19: fixed → ---
tracking-firefox18: ? → ---
tracking-firefox19: + → ---
(Assignee)

Comment 33

6 years ago
Created attachment 671024 [details] [diff] [review]
Patch (v5)

(removed the second half of the test which is no longer needed, since we no longer open this dialog from the preferences window.)
Attachment #640380 - Attachment is obsolete: true
Attachment #671024 - Flags: review?(josh)
(Assignee)

Updated

6 years ago
No longer blocks: 787743
(Reporter)

Comment 34

6 years ago
Comment on attachment 671024 [details] [diff] [review]
Patch (v5)

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

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +381,4 @@
>  }
>  
>  /**
> + * Returns true if the opener of this dialog was in private browsing mode.

This comment is no longer accurate.
Attachment #671024 - Flags: review?(josh) → review+
(Assignee)

Comment 36

6 years ago
https://hg.mozilla.org/mozilla-central/rev/2604cbda92ac
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.