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?

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.

Review commit: https://reviewboard.mozilla.org/r/49649/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49649/

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.

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? :)

Cancelled the previous try push since I forgot to remove the "skip-if = e10s" flag.

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?

> > 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.

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

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

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. :)

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)

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`

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.

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.

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

https://hg.mozilla.org/mozilla-central/rev/6a563348b8be https://hg.mozilla.org/mozilla-central/rev/b308e34362cc https://hg.mozilla.org/mozilla-central/rev/d3bf9bbe73ba

This caused a bunch of test failures over on comm-central.

Taking the liberty to reopen this since it seems to be incomplete without landing Aleth'es fix.

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.

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5029e21db2453b5147c0f842cb4c6e30b4600fa Bug 1266836 - Part 4: Followup to fix a missed rename in Part 3. r=MattN

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.