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)

defect
Not set
normal

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)

Attached file 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: nobody → ralin
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 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 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 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 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 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
Keywords: checkin-needed
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
Hi Gabi, Bogdan, could you help to verify our patches for this issue ?

Thanks
Flags: qe-verify+
Flags: needinfo?(gasofie)
Flags: needinfo?(bogdan.maris)
(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)
Flags: needinfo?(bogdan.maris)
Please request Beta approval on this when you get a chance.
Flags: needinfo?(ralin)
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?
See Also: → 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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.