If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsLoginManagerPrompter.js uses global Private Browsing state to make decisions

RESOLVED FIXED in mozilla19

Status

()

Toolkit
Password Manager
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

unspecified
mozilla19
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Created attachment 600702 [details] [diff] [review]
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?
Attachment #600702 - Flags: feedback?(josh)
(Reporter)

Comment 2

6 years ago
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.
Attachment #600702 - Flags: feedback?(josh) → feedback+
Comment on attachment 600702 [details] [diff] [review]
patch

Requesting review, then, since we haven't got anything better.
Attachment #600702 - Flags: review?(paul)
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.
Attachment #600702 - Flags: review?(paul) → review+
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.
Attachment #600702 - Flags: review-
(Reporter)

Comment 6

6 years ago
>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.
(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?
Justin, under what circumstances can this._window be null here?  Given that information we can decide what to do with that case.
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).
(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.
Assignee: nobody → chrislord.net
Blocks: 797256
Created attachment 671012 [details] [diff] [review]
Patch (v2)
Assignee: chrislord.net → ehsan
Attachment #600702 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #671012 - Flags: review?(dolske)
(Reporter)

Comment 12

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

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

That's not going to fly.
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. ;-)
Attachment #671012 - Flags: review?(dolske) → review+
Created attachment 671025 [details] [diff] [review]
Flying!
Attachment #671012 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/5091701dcc45
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/5091701dcc45
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Depends on: 801916
Depends on: 833971
You need to log in before you can comment on or make changes to this bug.