Closed
Bug 1395173
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Autofill stops working after switching between two forms
Categories
(Toolkit :: Form Manager, defect)
Toolkit
Form Manager
Tracking
()
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | verified |
firefox57 | --- | verified |
People
(Reporter: bmaris, Assigned: ralin)
References
Details
(Keywords: regression, Whiteboard: [form autofill:MVP])
Attachments
(3 files)
[Affected versions]: - beta 56.0b7 - latest Nightly 57.0a1 [Affected platforms]: - Windows 10 x64, macOS 10.12.6, Ubuntu 16.04 x64 [Steps to reproduce]: 1. Modify extensions.formautofill.available to "on" in about:config (if necessary) 2. In about:preferences#privacy click on 'Saved Addresses' 3. Create a new profile that only has First Name, Middle Name, Last Name and Company. 4. Access the attached .html 5. Double click on the Given-Name, Additional-Name/Middle or FamilyName-LastName fields. 6. Double click on Organization field. 7. Then double click on the Given-Name, Additional-Name/Middle or FamilyName-LastName fields. [Expected result]: - A dropdown with the names you filled in the profile appears and you can select them. [Actual result]: - No dropdown appears after you double clicked. [Regression range]: - last good buildid: 20170807113452 - first bad buildid: 20170808114032 - pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=47248637eafa9a38dade8dc3aa6c4736177c8d8d&tochange=a921bfb8a2cf3db4d9edebe9b35799a3f9d035da - There are three autofill bugs in the pushlog, but I'm not entirely sure which is the culprit here. [Additional notes]: - It happens only about 90% of the time. So this bug reproduces almost all the time. - This happens when you switch between the two forms because the first one has only two valid entries (The feature doesn't work with only two entries). And because that one is blocked, it determines the other form to be blocked too, even if it has three valid entries.
Updated•7 years ago
|
Whiteboard: [form autofill] → [form autofill:MVP]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ralin
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
The computed "allFieldNames" is cached in FormAutofillHandler.prototype(or instance's __proto__), so we actually store it in a shared memory among different forms. I think we can have a straight fix moving _cacheValue to constructor to ensure an isolated cache per form.
Status: NEW → ASSIGNED
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8903072 [details] Bug 1395173 - Part 1. Cache computed values in form handler instance instead of prototype. https://reviewboard.mozilla.org/r/174848/#review180242 ::: browser/extensions/formautofill/FormAutofillHandler.jsm:60 (Diff revision 1) > filledRecordGUID: null, > }; > + > + this._cacheValue = { > + allFieldNames: null, > + oneLineStreetAddress: null, The `oneLineStreetAddress` is independent of Handler's instance now but I think it's reasonable to move it to constructor as well in case it'll be adapted to each form in the future.
Attachment #8903072 -
Flags: review?(lchang) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8903440 [details] Bug 1395173 - Part2. Add a mochitest for the display of form autofill popup while switching between forms. https://reviewboard.mozilla.org/r/175286/#review180410 ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:22 (Diff revision 1) > input.focus(); > > // "identifyAutofillFields" is invoked asynchronously in "focusin" event. We > // should make sure fields are ready for popup before doing tests. > // > // TODO: "setTimeout" is used here temporarily because there's no event to nit: ... "sleep" is used here ... ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:25 (Diff revision 1) > // should make sure fields are ready for popup before doing tests. > // > // TODO: "setTimeout" is used here temporarily because there's no event to > // notify us of the state of "identifyAutofillFields" for now. We should > // figure out a better way after the heuristics land. > SimpleTest.requestFlakyTimeout("Guarantee asynchronous identifyAutofillFields is invoked"); I suppose `requestFlakyTimeout` needs to be moved to `sleep` as well. ::: browser/extensions/formautofill/test/mochitest/test_multiple_forms.html:42 (Diff revision 1) > + await expectPopup(); > + > + // We need an intentional wait here before switching form. > + await sleep(); > + await setInput("#organization", ""); > + doKey("down"); Should we expect no popup here?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903440 [details] Bug 1395173 - Part2. Add a mochitest for the display of form autofill popup while switching between forms. https://reviewboard.mozilla.org/r/175286/#review180410 Thanks :D > I suppose `requestFlakyTimeout` needs to be moved to `sleep` as well. Yeah, I initially thought the sleep function should be generic and keep the timeout request reason out of it, but latter found we don't have other reason rather than "Guarantee asynchronous identifyAutofillFields is invoked" for now, so I guess it's no harm to move in sleep. Thanks. > Should we expect no popup here? A check for popup open state added.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8903440 [details] Bug 1395173 - Part2. Add a mochitest for the display of form autofill popup while switching between forms. https://reviewboard.mozilla.org/r/175286/#review180844 ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:10 (Diff revision 3) > "use strict"; > > let formFillChromeScript; > let expectingPopup = null; > > -function setInput(selector, value) { > +async function sleep(ms = 500, reason = "Guarantee asynchronous identifyAutofillFields is invoked") { nit: The reason here can be more generic since it's now a helper function. (e.g. "Intentionally wait for UI ready")
Attachment #8903440 -
Flags: review?(lchang) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903440 [details] Bug 1395173 - Part2. Add a mochitest for the display of form autofill popup while switching between forms. https://reviewboard.mozilla.org/r/175286/#review180844 Thank you Luke, the issue is fixed. waiting for the try reuslt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=deaca1ab77551f3cfb0a97ce85876516032295c5
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7943bf751331 Part 1. Cache computed values in form handler instance instead of prototype. r=lchang https://hg.mozilla.org/integration/autoland/rev/0b89aba4df90 Part2. Add a mochitest for the display of form autofill popup while switching between forms. r=lchang
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7943bf751331 https://hg.mozilla.org/mozilla-central/rev/0b89aba4df90
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Hi Gabi, Bogdan, could you help to verify our patches for this issue ? Thanks
Flags: qe-verify+
Flags: needinfo?(gasofie)
Flags: needinfo?(bogdan.maris)
Comment 17•7 years ago
|
||
(In reply to Vance Chen [:vchen][skype:vance.lucida][vchen@mozilla.com] from comment #16) > Hi Gabi, Bogdan, could you help to verify our patches for this issue ? > > Thanks Hi Vance, I can confirm that this is fixed in the latest 57.0a1 build on Windows 10x64, Ubuntu 14.04 and Mac 10.12.6. Thanks
Flags: needinfo?(gasofie)
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(bogdan.maris)
Comment 18•7 years ago
|
||
Please request Beta approval on this when you get a chance.
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(ralin)
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8903072 [details] Bug 1395173 - Part 1. Cache computed values in form handler instance instead of prototype. Approval Request Comment [Feature/Bug causing the regression]: Not a regression [User impact if declined]: break autofill popup if switch between forms [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: low [Why is the change risky/not risky?]: only moving a property from prototype chain to its constructor, unlikely go wrong as no logic change. [String changes made/needed]: none Thanks.
Flags: needinfo?(ralin)
Attachment #8903072 -
Flags: approval-mozilla-beta?
Comment 20•7 years ago
|
||
Comment on attachment 8903072 [details] Bug 1395173 - Part 1. Cache computed values in form handler instance instead of prototype. Fix form autofill issue and include tests. Beta56+.
Attachment #8903072 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a758d80438a4 https://hg.mozilla.org/releases/mozilla-beta/rev/9055f0524b2d
Flags: in-testsuite+
Reporter | ||
Comment 22•7 years ago
|
||
Verified that this is fixed using Firefox 56.0b10 across platforms (macOS 10.12.6, Ubuntu 16.04 32bit and Windows 10 64bit).
You need to log in
before you can comment on or make changes to this bug.
Description
•