1399068 - Fill Login/Password through context menu doesn't work

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Till Schneidereit [:till]]

Neither Fill Login nor Fill Password work for me in current Nightly. This is with a fresh profile on Windows 10.

STR:
1. Navigate to a page with a login form, fill in some login data, submit.
2. Save login data in the doorhanger dialog.
3. Reload page, remove auto-restored login data (if any, won't be there in all cases).
4. Right-click on either username or password field.
5. Select "Fill Login" / "Fill Password", respectively.
6. Click on the previously stored login.

Expected result: Login should be filled.

Actual result: Login isn't filled. Instead, the following error is logged in the browser console:

Error: createFromField requires a password or username field in a document  LoginManagerContent.jsm:1618:13

Comment 1

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]:
Users can't fill passwords and usernames from the context menu anymore.

I can reproduce on Mac as well, so I assume this is on all platforms. I'm suspiciously looking at bug 1371525 and its cousins, but we'd need a regression range to know.

In any case, it's weird that none of our automated tests is failing (maybe because some are only running in non-e10s? who knows).

Comment 2

[Julien Cristau [:jcristau]]

This seems to work for me on 56.0b11, but not in nightly (on linux); tracking for 57.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Bisect says: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=604fd3302562dc2059a8de3231818a1b618b8ffe&tochange=bf7793529f82ab9cb885a62446ed0f3e18c51634

Jiang, Felipe, could you take a look?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

There appears to be a CPOW bug here. The error message that shows up comes from here: http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/toolkit/components/passwordmgr/LoginManagerContent.jsm#1618

The aField object is meant to be a CPOW. If I log Cu.isCrossProcessWrapper, it returns true, as expected. But if I try to print it, I get this warning: "CPOWToString called on an incompatible object" [1], and all the checks at the beginning of this createFromField function fails.

I'll continue to investigate it a bit before I ping people.

Alternatively, the reason that this is a CPOW is that the contextmenu hiding message arrives before the command from the context menu. Perry looked into this and there was some weird bug, but we might have to find ways to work around that if we can't figure out what's wrong in the CPOW implementation.

[1] http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/js/ipc/WrapperOwner.cpp#380

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Yoink

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Okay, found out what's going on here; we're not actually passing the input element as a CPOW around anymore. We do an attempt at serializing it inside ContextMenu.jsm via _cleanContext, so what gets passed up goes through data instead of objects (so isn't a CPOW).

It looks like the LoginManager stuff was expecting the target to be a CPOW though, since it passes it back down through objects. 

So what we end up doing is taking an object that was created in ContextMenu.jsm in the content process, serialized properly through JSON (so not converted into a CPOW), held the parent in nsContextMenu.js, passed to LoginManagerContextMenu.jsm, and then sent down through objects (so then that object is converted into a CPOW), which LoginManagerContent.jsm doesn't recognize as anything useful.

What I suggest we do is to pass the target as a CPOW (explicitly marked so!) to nsContextMenu.js. Nothing in nsContextMenu.js should try to access the CPOW (in fact, this should cause us to throw since the contextmenu message is no longer synchronous), but we can use that CPOW as a reference to the DOM node in the content process and pass it back down with the RemoteLogins:fillForm message, so that the other side knows which input node the context menu was opened on.

Patch coming up.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1399068 - Send the context menu target node up to the parent as an explicit CPOW to fix login selection.

LoginManagerParent expects either a CPOW or a DOM node (in the non-e10s case) to
be passed as the inputElement for LoginManagerParent.fillForm. In bug 1360406,
we serialized the target in the content process so that the parent didn't have
access to the CPOW anymore.

This patch adds a targetAsCPOW property to the nsContextMenu solely for passing
back and forth between the parent and content processes in order to refer to
the target DOM node.

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

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey mmucci, this was some fallout from some Photon Performance work that I took on once felipe went on PTO, so we should probably add to our tracking.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8915239 [details]
Bug 1399068 - Send the context menu target node up to the parent as an explicit CPOW to fix login selection.

https://reviewboard.mozilla.org/r/186466/#review191594

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0fa535753a5
Send the context menu target node up to the parent as an explicit CPOW to fix login selection. r=Felipe

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e0fa535753a5

Comment 12

[Adrian Florinescu [:aflorinescu]]

Confirmed the issue on 57.0b5 20171002181526 and on 58.0a1 - 2017.10.03 and verified the fix on:
- Windows 10x64, Ubuntu 16.04, Osx 10.12
- 58.0a1 20171005220204

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mike, should we uplift this to beta57? If yes, please nominate a beta patch. Thanks!

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8915239 [details]
Bug 1399068 - Send the context menu target node up to the parent as an explicit CPOW to fix login selection.

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1360406

[User impact if declined]:

Users in the default configuration on Firefox 57 won't be able to fill in logins using the context menu "Fill Logins" feature.

[Is this code covered by automated tests?]:

Good chunks of it are, yes - strangely, the automated tests didn't catch this particular bug, and I haven't had the cycles to figure out why yet. Tracking that in bug 1405835.

[Has the fix been verified in Nightly?]:

Yes, I was able to reproduce this on the original Nightly, and can now confirm that it is fixed.

[Needs manual test from QE? If yes, steps to reproduce]: 

Probably not, no.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're making the context menu code use a CPOW (safely!) which is what the login manager code is prepared for. So it lets us operate in the same way as we were operating before bug 1360406 landing.

[String changes made/needed]:

None.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915239 [details]
Bug 1399068 - Send the context menu target node up to the parent as an explicit CPOW to fix login selection.

Photon related, Beta57+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d40416856dd2

Comment 17

[Mihai Boldan, Desktop QA [:mboldan]]

I've also managed to reproduce this issue on Firefox 57.0a1 (2017-09-12), under Windows 10x64.
The issue is no longer reproducible on Firefox 57.0b7.
Tests were performed under Windows 10x64, Ubuntu 16.04x64 and under macOS 10.12.6.