Closed
Bug 723004
Opened 13 years ago
Closed 12 years ago
nsLoginManagerPrompter.js uses global Private Browsing state to make decisions
Categories
(Toolkit :: Password Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: jdm, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 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 3•13 years ago
|
||
Comment on attachment 600702 [details] [diff] [review]
patch
Requesting review, then, since we haven't got anything better.
Attachment #600702 -
Flags: review?(paul)
Comment 4•13 years ago
|
||
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 5•13 years ago
|
||
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•13 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.
Comment 7•13 years ago
|
||
(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?
Assignee | ||
Comment 8•13 years ago
|
||
Justin, under what circumstances can this._window be null here? Given that information we can decide what to do with that case.
Comment 9•12 years ago
|
||
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).
Assignee | ||
Comment 10•12 years ago
|
||
(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 | ||
Updated•12 years ago
|
Assignee: nobody → chrislord.net
Assignee | ||
Comment 11•12 years ago
|
||
Assignee: chrislord.net → ehsan
Attachment #600702 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #671012 -
Flags: review?(dolske)
Reporter | ||
Comment 12•12 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 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #671012 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
Target Milestone: --- → mozilla19
Comment 16•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•