1045987 - "Remember password?" door hanger is now a window-modal dialog in e10s window

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Chris Peterson [:cpeterson]]

In a non-e10s window, the "remember password?" door hanger is correctly displayed under the address bar.

Comment 1

[Matthew N. [:MattN]]

This was mentioned in bug 949617 so it may have been intentional for the short-term. Blake, is that right?

Comment 2

[Chris Peterson [:cpeterson]]

I bisected this change to the following pushlog, which does include Blake's fix for bug 949617:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=afa67a2f7905&tochange=b6408c32a170

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, I didn't want to block that bug landing based on this. I can look into fixing it.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This patch is slightly undertested, I will fix that soon. However, I believe that it's mostly correct (enough that I'm asking for review instead of feedback). The reason for the original problem reported here was that the existing code didn't deal well with the hack that I added to LoginManagerParent to pass the chrome window in the e10s case. Fixing that bug, however, left an odd "show the notification on the opener in some rare instances" case being broken.

That turned out to be pretty hairy to fix: the computation could only be done in the parent (due to the "chromehidden" check) but there was no way to then recover the browser for the opener's window solely in chrome. I thought of three solutions:

* In the case where we detected the opener was wrong, send a message back to the child and have it resend the original message to the right message manager (which gives us the right browser for free).

* Do the "chromeless" check in a sync message (ew).

* Pass the opener as a CPOW and between that and the chrome window's opener, recover the browser via linear search.

I chose the third solution here, although I'm unhappy with it. If anybody has any suggestions or improvement (or would prefer one of the other solutions) I'd be happy to do that (except the sync message).

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8471225 [details] [diff] [review]
patch v1

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

The approach here seems fine to me. As far as I'm concerned, passing CPOWs around without accessing them is totally acceptable. Also, until very recently, this code was doing a linear search to find the document anyway, so that seems fine.

I do want to mention that this will break with bug 1051017. Maybe we should provide a special CPOW version of getBrowserForX?

I don't have any comments on the implementation since that's dolske's area.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +713,5 @@
>          this.log("===== initialized =====");
>      },
>  
> +    setE10sData : function (aData) {
> +        if (!this._window instanceof Ci.nsIDOMChromeWindow)

I think ! binds tighter than instanceof, so this doesn't do what it's supposed to.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1.1

I fixed billm's comments. I decided to make _getTabForContentWindow work for both CPOWs and regular windows.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

dolske: review ping?

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 8477665 [details] [diff] [review]
patch v1.1

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

One trivial thing to fix, one thing I want to look at again in the morning.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +1213,5 @@
> +                } else {
> +                    var webnav = notifyWin.
> +                                 QueryInterface(Ci.nsIInterfaceRequestor).
> +                                 getInterface(Ci.nsIWebNavigation);
> +                    hasHistory = webnav.sessionHistory.count > 1;

This should be == 1, not > 1, as in the original code.

The intent was to handle the case of:

1) Visit crappy website, click login
2) Get popup window for authentication
3) Enter creds, submit, popup window is closed

If you're in a popup window with deeper history, presumably it's a useful window that's going to stick around because you've already navigated it at least once.

@@ +1245,5 @@
> +                    maybeBrowser.result =
> +                          chromeWin.gBrowser.
> +                                    getBrowserForDocument(notifyWin.top.document);
> +                }
> +            }

It makes me sad to see how much convoluted conditional stuff E10S is adding. :(

Specifically, I think it's not very good to have a function that returns a window and sometimes an browser via an outparam. Seems like it should either always return both (ideally via destructuring assignment) or only 1.

Let me think about that in the morning when my head is functional.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Justin Dolske [:Dolske] from comment #9)
> > +                    hasHistory = webnav.sessionHistory.count > 1;
> 
> This should be == 1, not > 1, as in the original code.

This was actually intentional as I negate the boolean below, in the condition. I changed it because otherwise my variable name would have had a negative and the condition would have been a double-negative, which would have been worse.

> Specifically, I think it's not very good to have a function that returns a
> window and sometimes an browser via an outparam. Seems like it should either
> always return both (ideally via destructuring assignment) or only 1.

Yeah, I was a little caught because not all of the callers need the browser. I'll write up the version with the outparam so you can make a final call on that.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: No out parameters

Is this better?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

This is ready to go once it's reviewed... https://tbpl.mozilla.org/?tree=Try&rev=7e04f119059e

Comment 13

[Justin Dolske [:Dolske]]

(In reply to Blake Kaplan (:mrbkap) from comment #10)
> (In reply to Justin Dolske [:Dolske] from comment #9)
> > > +                    hasHistory = webnav.sessionHistory.count > 1;
> > 
> > This should be == 1, not > 1, as in the original code.
> 
> This was actually intentional as I negate the boolean below

Oh, indeed. Might be worth a comment, or maybe "hasNavigated" would be a little more self-documenting of intent. This is why I shouldn't review code in the evening. (What time is it now? Hush, you.)

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 8480851 [details] [diff] [review]
No out parameters

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

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +834,5 @@
>                      }
>                  }
>              ];
>  
> +            var { notifyWin, browser } = this._getNotifyWindow();

This actually helps a lot, and is sorta what I meant by wanting to look into more... Instead of making code return extra stuff for E10S, it would be cleaner to convert as much as possible to do the same thing in all cases.

The chrome window was the original "handle" for doing stuff, now we should be using the browser.

All the callers of _getNotifyWindow() are just using the window as a step to get to the browser, so in fact this function can just become _getNotifyBrowser() that always returns a browser. Specifically, you already did this at the other callsite, notifyWin is unused here too.

I bet we could remove more window usage with some additional refactoring, but don't need to do so here.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7e196cbd9d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d7e196cbd9d