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

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Josh Matthews [:jdm]]

The global service is going away. The dialog should attempt to query the relevant page docshell instead.

Comment 1

[Josh Matthews [:jdm]]

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;

Comment 2

[Saurabh Anand [:sawrubh]]

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.

Comment 3

[Dão Gottwald [:dao]]

(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).

Comment 4

[(no longer active)]

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

Comment 5

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

The concerned mochitest (browser_privatebrowsing_certexceptionsui.js) is passing the test locally. I'll push to try after f+ from Ehsan

Comment 6

[(no longer active)]

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.

Comment 7

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

Fixed the faulty logic.

Comment 8

[Saurabh Anand [:sawrubh]]

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

Comment 9

[(no longer active)]

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.

Comment 10

[Mozilla RelEng Bot]

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/[e-mail]-d5d653e3c0aa

Comment 11

[Saurabh Anand [:sawrubh]]

Attachment: 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.

Comment 12

[Saurabh Anand [:sawrubh]]

Guys, any ideas ?

Comment 13

[(no longer active)]

I don't see that failure on your try push...  Can you please post a log of the failure?

Comment 14

[Saurabh Anand [:sawrubh]]

@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.

Comment 15

[Josh Matthews [:jdm]]

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?

Comment 16

[Saurabh Anand [:sawrubh]]

@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 ?

Comment 17

[Josh Matthews [:jdm]]

Give my feedback a try.

Comment 18

[(no longer active)]

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

Comment 19

[Saurabh Anand [:sawrubh]]

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.

Comment 20

[(no longer active)]

(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!

Comment 21

[Saurabh Anand [:sawrubh]]

Attachment: Patch v4

Comment 22

[Saurabh Anand [:sawrubh]]

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 ?

Comment 23

[(no longer active)]

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);

Comment 24

[(no longer active)]

Also, note bug 774660.

Comment 25

[Josh Matthews [:jdm]]

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.

Comment 26

[Saurabh Anand [:sawrubh]]

I'm currently working on something else, and have put this on halt. Hence unassigning myself, so that I don't block someone else.

Comment 27

[(no longer active)]

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.

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 29

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 30

[(no longer active)]

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.

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 32

[(no longer active)]

False alarm.  The load context that XHR gets by default should take care of everything XHR-related here.

Comment 33

[(no longer active)]

Attachment: 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.)

Comment 34

[Josh Matthews [:jdm]]

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.

Comment 35

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2604cbda92ac

Comment 36

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/2604cbda92ac