[Form Autofill] Autofill stops working after switching between two forms

VERIFIED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
VERIFIED FIXED
9 months ago
9 months ago

People

(Reporter: bogdan_maris, Assigned: ralin)

Tracking

({regression})

Trunk
mozilla57
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 verified, firefox57 verified)

Details

(Whiteboard: [form autofill:MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Created attachment 8902703 [details]
Demo

[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.
Whiteboard: [form autofill] → [form autofill:MVP]
(Assignee)

Updated

9 months ago
Assignee: nobody → ralin
Comment hidden (mozreview-request)
(Assignee)

Comment 2

9 months 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

9 months 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

9 months 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

9 months 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

9 months 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

9 months 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

9 months ago
Keywords: checkin-needed

Comment 14

9 months 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
https://hg.mozilla.org/mozilla-central/rev/7943bf751331
https://hg.mozilla.org/mozilla-central/rev/0b89aba4df90
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox57: affected → fixed
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

9 months 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

9 months ago
status-firefox57: fixed → verified
Flags: needinfo?(bogdan.maris)
Please request Beta approval on this when you get a chance.
status-firefox-esr52: --- → unaffected
Flags: needinfo?(ralin)
(Assignee)

Comment 19

9 months 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?
(Assignee)

Updated

9 months ago
See Also: → bug 1397000
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+
Verified that this is fixed using Firefox 56.0b10 across platforms (macOS 10.12.6, Ubuntu 16.04 32bit and Windows 10 64bit).
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.