Closed Bug 1266836 Opened 8 years ago Closed 8 years ago

Fix password manager handling of popup windows in e10s

Categories

(Toolkit :: Password Manager, defect, P1)

Product:
Toolkit
Component:
Password Manager
Version:
35 Branch
Platform:
All
Unspecified
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
e10s + ---
firefox51 --- fixed

People

(Reporter: MattN, Assigned: johannh)

References

(
URL
)

Details

(Keywords: regression, Whiteboard: [passwords:capture-UI])

Attachments

(4 files, 2 obsolete files)

Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.
58 bytes, text/x-review-board-request
MattN
: review+
liuche
: review+
Details
Bug 1266836 - Part 2 - Rename openerWindow to openerTopWindow.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1266836 - Part 3 - Rename window to chromeWindow, clean up _getNotifyWindow in nsLoginManagerPrompter.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Part 4: Followup to fix a missed rename in Part 3
1.22 KB, patch
MattN
: review+
Details | Diff | Splinter Review 
While fixing our tests for this to work in e10s (bug 1266825), I noticed that behaviour is different/broken. For example, in one of the tests the doorhanger appears in the popup which then auto-closes (e.g. like Persona) so the user has no way to save their password.

I think some of the e10s-specific handling is wrong: https://mxr.mozilla.org/mozilla-central/search?string=e10s&find=nsLoginManagerPrompter.js&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
Can someone from the front end team take this this toolkit bug?
Flags: needinfo?(dolske)
I've looked at this a bit but it's quite confusing since it seems like code was written for this to work with e10s in bug 1045987. After some fixes by me, I can get an unsafe CPOW warning which I believe is what was mentioned by billm in bug 1045987 comment 6 as being a problem in the future. Unfortunately we're living in that future now…

I agree with the sentiment in bug 1045987 that the solution there isn't very nice and we should probably refactor stuff to avoid separate code paths for e10s. e.g. pass a <browser> around instead of content windows sometimes and chrome windows other times.

I'll probably need to discuss solutions more with dolske and/or mrbkap.
Assignee: nobody → MattN+bmo
Blocks: 1045987
Status: NEW → ASSIGNED
Flags: needinfo?(dolske)
tracking-e10s: ? → +
Priority: -- → P1
I have higher priority stuff to work on at the moment so unassigning myself. Feel free to look at my debugging patch if anyone wants to figure out where the problem is.
Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
Version: unspecified → 35 Branch
Whiteboard: [passwords:capture-UI]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

This patch makes the tests pass again. I'm pretty sure it's the right way to go.

My question is: can I simply remove the non-e10s code (as I've been doing here)? The tests pass for both e10s and non-e10s as far as I can tell. Can you spot any potential problems? :)
Attachment #8781973 - Flags: feedback?(MattN+bmo)
Cancelled the previous try push since I forgot to remove the "skip-if = e10s" flag.
Attachment #8781973 - Flags: feedback?(MattN+bmo)
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72272/#review69952

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1343
(Diff revision 3)
> +  _getChromeWindowForOpener: function (opener) {
> +    let windows = Services.wm.getEnumerator(null);
> +    while(windows.hasMoreElements()){
> +      let win = windows.getNext();
> +      let browser = win.gBrowser.getBrowserForContentWindow(opener);
> +      if (browser) {
> +        return {notifyWin: win, browser};
> +      }
> +    }
> +    return null;
> +  },

Does this actually work when the opener is a subframe? Do we have a test for that case?
Attachment #8781973 - Flags: review?(MattN+bmo)
> 
> Does this actually work when the opener is a subframe? Do we have a test for
> that case?

Not sure about a test, but isn't that why we make sure to always get the top frame in http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/toolkit/components/passwordmgr/LoginManagerContent.jsm#919 ?
Getting some failures on OSX https://treeherder.mozilla.org/logviewer.html#?job_id=25877124&repo=try#L1943

I can look into it, but if you know anything about these I'd appreciate a hint :)
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72272/#review70000

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:291
(Diff revision 3)
>      });
>    },
>  
>    onFormSubmit: function(hostname, formSubmitURL,
>                           usernameField, newPasswordField,
>                           oldPasswordField, opener,

Can we rename variables named `opener` or `openerWin` to `openerTopWindow`? Doing this in a separate commit would be awesome.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:692
(Diff revision 3)
>      this._opener = null;
>  
>      this.log("===== initialized =====");
>    },
>  
>    setE10sData : function (aBrowser, aOpener) {

Can you remove/rename this?

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1359
(Diff revision 3)
>  
>    _getNotifyWindow: function () {
>  
>      try {
>        // Get topmost window, in case we're in a frame.
> -      var notifyWin = this._window.top;
> +      let notifyWin = this._window.top;

`this._window` is a chrome window in e10s? If so, the `.top` isn't useful I think.
Attachment #8781973 - Flags: review?(MattN+bmo)
This should fix all tests now, at least locally for me. Let's hope try agrees.
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72272/#review70786

::: toolkit/components/passwordmgr/nsILoginManagerPrompter.idl:22
(Diff revision 5)
>     * @param aBrowser the <browser> to use for this prompter.
> +   *        If aWindow is a content window this will be the browser
> +   *        belonging to that window by default.
>     * @param aOpener the opener to use for this prompter.

So aWindow is sometimes a content window and sometimes a ChromeWindow still?

I was hoping we could just get `aWindow` from `aBrowser` or vice-versa. Do we need to pass both?

::: toolkit/components/passwordmgr/nsILoginManagerPrompter.idl:27
(Diff revision 5)
> -  void setE10sData(in nsIDOMElement aBrowser, in nsIDOMWindow aOpener);
> +  void init(in nsIDOMWindow aWindow,
> +            in nsIDOMElement aBrowser,
> +            in nsIDOMWindow aOpener);

I worry that the change to the `init` signature will break add-ons though I don't see a huge amount using it. Maybe we could mitigate this by making the new two arguments to .init optional? It's possible that any add-ons using this API are already broken in e10s if they weren't using `setE10sData` so maybe it doesn't matter.
This updated patch resolves the CI tests failures from browser not being set in the native part. Let's hope this try passes.

The only sane way to deal with this I could come up with is to retain a separation between setting the "e10s" data and the "init" function that receives a window. There is too much code entanglement in other parts to attempt a refactor in this bug. I split the setE10s method into "setBrowser" and "setOpener" since there's no logical connection between the two.

This has the nice side effect of keeping "init" fully API-compatible for addons and automatically e10s-transitioning if you pass a content window.

> So aWindow is sometimes a content window and sometimes a ChromeWindow still?

Yes, there is too much code around that uses either. I wanted to focus on solving the problem at hand. Maybe we should file a followup that targets refactoring consumers of this module.
> Can we rename variables named `opener` or `openerWin` to `openerTopWindow`? Doing this in a separate commit would be awesome.

Maybe that is even a bit too simple but we could turn it into a good first bug.
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72272/#review71860

(In reply to Johann Hofmann [:johannh] from comment #24)
> > Can we rename variables named `opener` or `openerWin` to `openerTopWindow`? Doing this in a separate commit would be awesome.
> 
> Maybe that is even a bit too simple but we could turn it into a good first
> bug.

I think it would be best to do this now while you have the context as it's hard for me to review with the current variable names.

::: toolkit/components/passwordmgr/nsILoginManagerPrompter.idl:37
(Diff revision 6)
> +  /**
> +   * Set the opener that was used to open the window passed to init.
> +   * The opener can be used to determine in which window the prompt
> +   * should be shown. Must be a content window.
> +   *
>     * @param aOpener the opener to use for this prompter.
>     */
> -  void setE10sData(in nsIDOMElement aBrowser, in nsIDOMWindow aOpener);
> +  void setOpener(in nsIDOMWindow aOpener);

This needs to be the top window of the opener or not?

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:698
(Diff revision 6)
> -  setE10sData : function (aBrowser, aOpener) {
> +  setBrowser(aBrowser) {
> -    if (!(this._window instanceof Ci.nsIDOMChromeWindow))
> -      throw new Error("Unexpected call");
>      this._browser = aBrowser;
> +  },
> +
> +  setOpener(aOpener) {
>      this._opener = aOpener;
>    },

I think you can make these real JS setters e.g.:
```
set browser(aBrowser) {
…
}
```
Ignore this idea if I'm wrong about xpconnect's ability here.
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72272/#review72330
Attachment #8781973 - Flags: review?(MattN+bmo)
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72270/#review72338

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1359
(Diff revision 7)
> -
>    _getNotifyWindow: function () {
> -
>      try {
>        // Get topmost window, in case we're in a frame.
> -      var notifyWin = this._window.top;
> +      let win = this._window;

Nit: Can you name this chromeWindow please? Feel free to do this in a separate commit or whichever commit is easiest.
Comment on attachment 8785170 [details]
Bug 1266836 - Part 2 - Rename openerWindow to openerTopWindow.

https://reviewboard.mozilla.org/r/74476/#review72340
Attachment #8785170 - Flags: review?(MattN+bmo) → review+
Attachment #8746961 - Attachment is obsolete: true
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72272/#review72342

Thank you very much for investigating this and wading through the mess. It's much clearer now with unified code paths and clearer variables names. :)
Attachment #8781973 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8785227 [details]
Bug 1266836 - Part 3 - Rename window to chromeWindow, clean up _getNotifyWindow in nsLoginManagerPrompter.

While doing the renaming I noticed that the try-catch sequence is completely unnecessary and actually leads to the dangerous assumption that errors in _getNotifyWindow will not propagate. We've always been using it in combination with unchecked property access through destructuring, which would lead to a TypeError since the catch clause it returning null.

With the function greatly simplified I really see no reason to retain a way to recover from any "common" exceptions that could arise from this code.
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

I noticed that we might break things in Android when not changing the function signature there. I'll try running it on Android on Monday, can you take a quick look at the interdiff and see if this still looks ok to you?

(I also fixed a case where I missed to replace setE10sData, causing tests to fail)
Attachment #8781973 - Flags: review+ → review?(MattN+bmo)
Comment on attachment 8785227 [details]
Bug 1266836 - Part 3 - Rename window to chromeWindow, clean up _getNotifyWindow in nsLoginManagerPrompter.

https://reviewboard.mozilla.org/r/74502/#review72456

r=me but it would be nice if you could fix `isContentWindowPrivate` even though you didn't cause that problem.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:260
(Diff revision 1)
> -    if (this._window) {
> -      return PrivateBrowsingUtils.isContentWindowPrivate(this._window);
> +    if (this._chromeWindow) {
> +      return PrivateBrowsingUtils.isContentWindowPrivate(this._chromeWindow);
>      }

It seems like `isContentWindowPrivate` probably isn't the correct API to use with a chrome window. This is why it's a good idea to distinguish content and chrome windows in variables for code that deals with both.

I think it's as simple as changing to `isWindowPrivate`
Attachment #8785227 - Flags: review?(MattN+bmo) → review+
Attachment #8781973 - Flags: review?(MattN+bmo) → review+
So, judging from manual testing on Android this version seems to do it. Since we were now passing a chrome window instead of a content window to Android, I had to change some things around.
Attachment #8781973 - Flags: review?(liuche)
liuche, can you take a look at the Android parts of the code and check if it looks good to you? Also, the regression I needed to fix wasn't caught by robocop tests as far as I can see. Do you know if there are any tests for the password manager prompts?
Comment on attachment 8781973 [details]
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s.

https://reviewboard.mozilla.org/r/72272/#review73816

Thanks for making these changes, Johann.

I don't believe there are tests for the password manager :/ Our Android test chain uses UI input, and doesn't have a good way to send JS input. We should be able to find a way to trigger the doorhanger though, and get a ref to BrowserToolbar in order to check the visibility of the Doorhanger, though there aren't easy ways to span JS and Java calls.
Attachment #8781973 - Flags: review?(liuche) → review+
Thaks for the review! I think this may be a worthwhile thing to look into but I don't want to slow down this bug even more, so let's make a separate bug for it!
https://hg.mozilla.org/integration/fx-team/rev/6a563348b8be866a9c0a3ac18c323b151a73d18f
Bug 1266836 - Part 1 - Fix password manager handling of popup windows in e10s. r=MattN r=liuche

https://hg.mozilla.org/integration/fx-team/rev/b308e34362cc9fd7569ea51737104c32e9ae7435
Bug 1266836 - Part 2 - Rename openerWindow to openerTopWindow. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/d3bf9bbe73ba35c4257dcd10339f6ccecbb1fd5e
Bug 1266836 - Part 3 - Rename window to chromeWindow, clean up _getNotifyWindow in nsLoginManagerPrompter. r=MattN
Depends on: 1300059
This caused a bunch of test failures over on comm-central.
Attachment #8788007 - Flags: review?(MattN+bmo)
Taking the liberty to reopen this since it seems to be incomplete without landing Aleth'es fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch might be a bit more controversial, but it fixes some xpcshell failures on c-c in tests completely unrelated to logins where the loginManagerPrompter init() ends up getting called incidentally. In this case, there is no window. It should be a safe change as if anything actually tries to use a null window, it will still fail.
Attachment #8788009 - Flags: review?(MattN+bmo)
Attachment #8788007 - Flags: review?(MattN+bmo) → review+
Thanks for catching this! What's the reason you can not simply remove the calls to init() with a null window?

If there's no way around it allowing null seems fine to me.
(In reply to Johann Hofmann [:johannh] from comment #62)
> Thanks for catching this! What's the reason you can not simply remove the
> calls to init() with a null window?

I'm having trouble figuring out what exactly is causing the call to getPrompt() - my guess is it's some side effect of setting up the mock accounts in the xpcshell context. Looking at the stack one ends up in the async test event loop, which is not helpful. The actual subtests all pass.
Does that fix some/all of your Xpcshell errors from bug 1300059? What about part 5 here. Do we need it or can we follow the advice from comment #62?
(In reply to Jorg K (GMT+2, PTO during summer) from comment #65)
> Does that fix some/all of your Xpcshell errors from bug 1300059? What about
> part 5 here. Do we need it or can we follow the advice from comment #62?

See bug 1300059 and comment 63. The landed part fixes some of the failures, but not all.
Now my question:
> What about part 5 here. Do we need it or can we follow the advice from comment #62?
Comment #63 says it's hard. Are you looking into it further or are we waiting for the review here?
(In reply to Jorg K (GMT+2, PTO during summer) from comment #67)
> > What about part 5 here. Do we need it or can we follow the advice from comment #62?
> Comment #63 says it's hard. Are you looking into it further or are we
> waiting for the review here?

I'm not looking into it any more for now. Imho the patch is a reasonable fix.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #60)
> Taking the liberty to reopen this since it seems to be incomplete without
> landing Aleth'es fix.

This is the incorrect process. We only reopen bugs if the landed code is backed out. Follow-up work is always filed as a separate bug blocking the bug that caused the issue. The reason for this is because having follow-up commits land many days apart can mean that not all of them end up in the same Fx/Gecko version which is a tracking nightmare. It also adds noise to a normally already long bug like we now have with comment 59 - comment 68.

(In reply to aleth [:aleth] from comment #61)
> Created attachment 8788009 [details] [diff] [review]
> Part 5: Avoid xpcshell test failures due to initialization without a window
> 
> This patch might be a bit more controversial, but it fixes some xpcshell
> failures on c-c in tests completely unrelated to logins where the
> loginManagerPrompter init() ends up getting called incidentally. In this
> case, there is no window. It should be a safe change as if anything actually
> tries to use a null window, it will still fail.

Please move this patch to a separate bug blocking this one as it needs it own investigation and discussion.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Flags: needinfo?(aleth)
Resolution: --- → FIXED
Attachment #8788009 - Attachment is obsolete: true
Attachment #8788009 - Flags: review?(MattN+bmo)
Depends on: 1301109
Flags: needinfo?(aleth)
Depends on: 1308817
Depends on: 1350152
Depends on: 1367191
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: