1373130 - [Form Autofill] Explicitly dispatch a notification after "identifyAutofillFields" invoked to ensure the tests have right timing to press down-key to avoid potential timeout

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Ray Lin[:ralin]]

Right now, we defer the timing of invoking identifyAutofillFields at the point user "focusin" the element. It's intentional for performance improvement and doesn't harm normal usage. However, in our mochitest/bc tests, there's no delay between "focusin" and doKey("down")[1], and that potentially causes the timeout since no popup would show up until identifyAutofillFields is invoked. 

I think we should consider sending out a message like "FormAutofill:FieldsIdentified" to replace current workaround[2] as an accordance for our tests to wait for.



[0] http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/browser/extensions/formautofill/content/FormAutofillFrameScript.js#51
[1] http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html#107-108
[2] http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/browser/extensions/formautofill/test/mochitest/formautofill_common.js#14-24

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

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

Comment 2

[Ray Lin[:ralin]]

Not sure it's right way to go or is more reliable than setTimeout, will see the result on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c01b9d259891f373e15c8c412b33af9b718a2d5

Comment 3

[Ray Lin[:ralin]]

Take this bug since identifyAutofillFields is still bothering Bug 1300995 with timeout problem.

Comment 4

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/1-2/

Comment 5

[Ray Lin[:ralin]]

A additional try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2f6bac14cdc568813ba94749e859a3f069d8989&selectedJob=107484068&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable

both looks good after patched. Luke could you help me to review this patch? Thanks.

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

https://reviewboard.mozilla.org/r/149472/#review154238

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:20
(Diff revision 2)
> -  input.focus();
> +    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
> +    if (docIdentified) {
> +      resolve(input);
> +    } else {
> +      formFillChromeScript.addMessageListener("FormAutofillTest:Identified", () => {

I suppose we need to `removeMessageListener` as it should be invoked only once.

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:23
(Diff revision 2)
> -  //       figure out a better way after the heuristics land.
> -  SimpleTest.requestFlakyTimeout("Guarantee asynchronous identifyAutofillFields is invoked");
> -  return new Promise(resolve => setTimeout(() => {
> -    resolve(input);
> +        resolve(input);
> -  }, 500));
> +      });
> +      docIdentified = true;

Since `docIdentified` actually happens after `FormAutofillTest:Identified`, it should be in the handler.

However, according to your implementation, `FormAutofill:FieldsIdentified` is sent every time upon `focusin`, I think we don't need `docIdentified`.

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/2-3/

Comment 8

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/3-4/

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

https://reviewboard.mozilla.org/r/149472/#review154868

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:52
(Diff revision 4)
>            return;
>          }
>  
> -        let doIdentifyAutofillFields =
> -          () => setTimeout(() => FormAutofillContent.identifyAutofillFields(doc));
> +        let doIdentifyAutofillFields = () => setTimeout(() => {
> +          FormAutofillContent.identifyAutofillFields(doc);
> +          sendAsyncMessage("FormAutofill:FieldsIdentified");

Please comment on this that it's only used by tests for now.

Comment 10

[Ray Lin[:ralin]]

Thank you Luke,

I was thinking to eliminate the setTimeout workaround in our tests, however, by looking at the latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d48f9d461dc81321f823a31e1eca73c20cd095, I doubt this is reliable enough. I'd like to leave setTimeout as-is if it doesn't bring up huge impact in the near future.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/4-5/

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/5-6/

Comment 13

[Ray Lin[:ralin]]

Hi Luke,

Though you've r+'d the patch, could you help me to review again and see if it's still valid? Thanks.

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/6-7/

Comment 15

[Ray Lin[:ralin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c03aa8e523b891c095c35d1c8cf3d0ab63d49863

I think the TH result shows that this patch is reliable enough to settle some intermittent failures, including test_basic_creditcard_autocomplete_form.html.

Comment 16

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/7-8/

Comment 17

[Ray Lin[:ralin]]

Hey Luke,

I've updated the patch according to what we discussed yesterday. Could you help me to review it again? Thanks.

Comment 18

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

https://reviewboard.mozilla.org/r/149472/#review195774

::: browser/extensions/formautofill/test/browser/head.js:103
(Diff revision 8)
>    await new Promise(resolve => setTimeout(resolve, ms));
>  }
>  
> +async function focusAndWaitForFieldsIdentified(browser, selector) {
> +  /* eslint no-shadow: ["error", { "allow": ["selector", "focused", "identified"] }] */
> +  const {focused, identified} = await ContentTask.spawn(browser, {selector}, async function({selector}) {

nit: `previouslyFocused` and `previouslyIdentified`

::: browser/extensions/formautofill/test/browser/head.js:119
(Diff revision 8)
> +
> +  if (identified) {
> +    return;
> +  }
> +
> +  if (!focused) {

It would be clearer to leave some comments explaining `FormAutofill:FieldsIdentified` may not be triggered if an input has been focused previously.

::: browser/extensions/formautofill/test/browser/head.js:127
(Diff revision 8)
> +        Services.mm.removeMessageListener("FormAutofill:FieldsIdentified", onIdentified);
> +        resolve();
> +      });
> +    });
> +  }
> +  // Wait 500ms for identifyAutofillFields if the form hasn't been identified yet.

This comment may need some updates since it's mainly for inputs that were focused previously, isn't it?

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:19
(Diff revision 8)
>    await new Promise(resolve => setTimeout(resolve, ms));
>  }
>  
> -async function setInput(selector, value) {
> -  let input = document.querySelector("input" + selector);
> -  input.value = value;
> +async function focusAndWaitForFieldsIdentified(input) {
> +  const rootElement = input.form || input.ownerDocument.documentElement;
> +  const focused = input != document.activeElement;

nit: `previouslyFocused`

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:34
(Diff revision 8)
>    // "identifyAutofillFields" is invoked asynchronously in "focusin" event. We
>    // should make sure fields are ready for popup before doing tests.
>    //
>    // TODO: "sleep" 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.

Comments need to be updated, too.

::: browser/extensions/formautofill/test/mochitest/test_on_address_submission.html:35
(Diff revision 8)
> +  return Object.entries(profile)
> +    .map(([key, value]) => setInput("#" + key, value))
> +    .reduce((p, c) => p.then(c), Promise.resolve());

Since for-loop does well, how about keep using it?

Comment 19

[Luke Chang [:lchang] (inactive)]

(In reply to Ray Lin[:ralin] from comment #17)
> I've updated the patch according to what we discussed yesterday. Could you
> help me to review it again? Thanks.

Looks good to me. Only a few comments left. Thanks for addressing this.

Comment 20

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/8-9/

Comment 21

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

https://reviewboard.mozilla.org/r/149472/#review195774

Thanks for the review, the issues are fixed in the latest patch.

> Since for-loop does well, how about keep using it?

reverted the change, thanks.

Comment 22

[Ray Lin[:ralin]]

TH result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e17044d1e4f99092fb1d03780690384f70cdcb

Thank you Luke for reviewing this.

Comment 23

[Ray Lin[:ralin]]

Unmark checkin-needed, I didn't notice that: https://treeherder.mozilla.org/logviewer.html#?job_id=138105981&repo=try&lineNumber=2293, let me see why this patch causes new failure only on "test_on_address_submission.html"

Comment 24

[Ray Lin[:ralin]]

Comment on attachment 8878059 [details]
Bug 1373130 - Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149472/diff/9-10/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7649437b6b0c
Send message after identifyAutofillFields being invoked to indicate that formautofill is ready to open popup. r=lchang

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/7649437b6b0c