Fill Login/Password through context menu doesn't work

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: till, Assigned: mconley)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla58
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57+ verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(1 attachment)

Reporter

Description

2 years ago
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
[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).
Flags: qe-verify+
OS: Windows 10 → All
Priority: -- → P1
Summary: Fill Login/Password doesn't work on current Nightly → Fill Login/Password through context menu doesn't work
This seems to work for me on 56.0b11, but not in nightly (on linux); tracking for 57.
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
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Flags: needinfo?(felipc)
Yoink
Assignee: felipc → 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.
Flags: needinfo?(jiangperry)
Comment hidden (mozreview-request)
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.
Whiteboard: [reserve-photon-performance]

Comment 9

2 years ago
mozreview-review
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
Attachment #8915239 - Flags: review?(felipc) → review+
Assignee

Updated

2 years ago
Blocks: 1405835

Comment 10

2 years ago
Pushed by mconley@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/e0fa535753a5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Hi Mike, should we uplift this to beta57? If yes, please nominate a beta patch. Thanks!
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Attachment #8915239 - Flags: approval-mozilla-beta?
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+
Attachment #8915239 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.