723004 - nsLoginManagerPrompter.js uses global Private Browsing state to make decisions

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Josh Matthews [:jdm]]

The global PB state is going away. The prompter needs some way to decide whether it's prompting in a PB instance or not - it can probably grab a docshell off of this._window.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

Here's a patch which at least doesn't make things worse on my Linux machine, but I still see the same UNEXPECTED-FAILs with and without the patch.

The giant query is a cargo-cult from nsPrivateBrowsingService.js; is there a better way to get to what I'm looking for from a window?

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 600702 [details] [diff] [review]
patch

As far as I know, the answer is no at this time. We're talking about various APIs in bug 723353, which would probably reduce the amount of rigamarole necessary here.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 600702 [details] [diff] [review]
patch

Requesting review, then, since we haven't got anything better.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 600702 [details] [diff] [review]
patch

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

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +279,5 @@
>  
>  
>      // Whether we are in private browsing mode
>      get _inPrivateBrowsing() {
> +      var window = this._window;

No need to add a local var.
Can you also add a comment/file a followup to use whatever API will exist that is less gross than this? This should work cross product, but I feel like we want something on the window, but I guess it'll depend on bug 723353 and co.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 600702 [details] [diff] [review]
patch

Driveby review!

>     // Whether we are in private browsing mode
>     get _inPrivateBrowsing() {
>-      // The Private Browsing service might not be available.
>-      try {
>-        var pbs = Cc["@mozilla.org/privatebrowsing;1"].
>-                  getService(Ci.nsIPrivateBrowsingService);
>-        return pbs.privateBrowsingEnabled;
>-      } catch (e) {
>-        return false;
>-      }
>+      var window = this._window;
>+      return window.getInterface(Ci.nsIWebNavigation)
>+                   .QueryInterface(Ci.nsIDocShellTreeItem)
>+                   .treeOwner
>+                   .QueryInterface(Ci.nsIInterfaceRequestor)
>+                   .getInterface(Ci.nsIXULWindow)
>+                   .docShell.QueryInterface(Ci.nsILoadContext)
>+                   .usePrivateBrowsing;
>     },


Hmmmm... _window can be null. How is the nextgen PB stuff planning on dealing with things (like components) not bound to a window?

Also, this glob of QI/gI can probably be reduced, based on how _getChromeWindow() [in this very file] has minimized over time. It's converting a content |window| to a chrome |window|, but I _think_ this should work...

    var window = this._window;
    return window.QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIWebNavigation)
                 .QueryInterface(Ci.nsIDocShell)
                 .chromeEventHandler
                 /* .ownerDocument.defaultView ? */
                 .docShell.QueryInterface(Ci.nsILoadContext)
                 .usePrivateBrowsing;

See how much better that is? No? Sigh. Yeah, it's pretty terrible either way. We should really put a nsIGetTheDamnChromeWindow onto the content window or something.

Comment 6

[Josh Matthews [:jdm]]

>Hmmmm... _window can be null. How is the nextgen PB stuff planning on dealing with
>things (like components) not bound to a window?

Magic. There's no one-size fits all solution, but the hope is that we can find some kind of sensible context for as many components as possible, or else add an isPrivate flag that callers with that context can provide.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Justin Dolske [:Dolske] from comment #5)
> Hmmmm... _window can be null. How is the nextgen PB stuff planning on
> dealing with things (like components) not bound to a window?

Not where _inPrivateBrowsing is used though right? Or did I miss that?

Comment 8

[(no longer active)]

Justin, under what circumstances can this._window be null here?  Given that information we can decide what to do with that case.

Comment 9

[Justin Dolske [:Dolske]]

At slight risk of tautology, the window can be null whenever a nsIPromptService-et-al caller provides a null window. :) This could be the case if, say, some XPCOM component wants to do an alert("printer is on fire"), and doesn't have / care / know what window it should be attached to.

I suspect the only relevant case (beyond throwing an exception) for authentication prompts would be XHR and related forms of network loads triggered via components. I _think_ you can get those to show a prompt not bound to a window. At least, I vaguely remember a patch from shortly after I started at Mozilla that was explicitly disabling HTTP auto for microsummaries (!), because wanted them to fail instead of prompt.

I think it would be legit to simply assume PB Mode in such cases (ie, don't allow saving logins from such prompts).

Comment 10

[(no longer active)]

(In reply to Justin Dolske [:Dolske] from comment #9)
> I think it would be legit to simply assume PB Mode in such cases (ie, don't
> allow saving logins from such prompts).

OK, that is the exact answer I was looking for, thanks!

Nathan, do you still want to work on this bug?  What needs to be addressed is comment 9, comment 5 and also using the API added in bug 723353.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 671012 [details] [diff] [review]
Patch (v2)

>+      if (this._window) {
>+        return PrivateBrowsingUtils.isWindowPrivate(window);

That's not going to fly.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 671012 [details] [diff] [review]
Patch (v2)

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

r+ assuming you make it fly per comment 12. ;-)

Comment 14

[(no longer active)]

Attachment: Flying!

Comment 15

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5091701dcc45

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5091701dcc45