Closed Bug 1398101 Opened 2 years ago Closed 2 years ago

[Form Autofill] Credit card autofill doesn't work on some of the main shopping sites

Categories

(Toolkit :: Form Manager, defect, P1, major)

defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox58 --- verified

People

(Reporter: Gabi, Assigned: selee)

References

Details

(Whiteboard: [form autofill:MVP])

Attachments

(3 files)

[Environment:]
Windows 10x64, Ubuntu 16.04, Mac Osx 10.12

Nightly 56.0a1 20170705170343

[Description:]
Credit Card Autofill doesn't work for several sites from the target tests sites as follows:

Amazon.com     
sears.com  
bestbuy.com 



Walmart - pass
homedepot - pass


[Steps:]

Preconditions
Go to Preferences/ Privacy and Security / Form Autofill / Enable Profile autofill (default in Nightly)
Make sure you have at least one saved CC profile


1.Open Firefox
2.Navigate to Amazon.com and login with a valid account
3.From the right corner hover Account &Lists and select account
4.Go to ADD a Payment Method
5.Click on Add Address
6.Double click On Name on Card/Card Number fields



[Actual Result:]
The Form autofill is not triggered by any of the fields.


[Expected Result:]
The Form autofill is triggered by any field

[Note:]
This is a Not regression

https://bugzilla.mozilla.org/show_bug.cgi?id=1392528 patch for Macys/Newegg/Costco/QVC
Attached image bestbuy
Hi Gabi,

(In reply to Gabi Cheta from comment #0)
> Amazon.com     
> sears.com  
Amazon.com and sears.com can not show the popup indeed. I am trying to figure out why `startSearch()` is not invoked.

> bestbuy.com 
The popup for bestbuy works and can fill the card number. (see the attachment)
May I know how do you verify it?

> https://bugzilla.mozilla.org/show_bug.cgi?id=1392528 patch for
> Macys/Newegg/Costco/QVC
Does this mean the patch in bug 1392528 can fix these sites (Macys/Newegg/Costco/QVC)?
Assignee: nobody → selee
Status: NEW → ASSIGNED
There are some details I forgot to address.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
> > Amazon.com     
> > sears.com  
> Amazon.com and sears.com can not show the popup indeed. I am trying to
> figure out why `startSearch()` is not invoked.
The card fields in both amazon.com and sears.com are all with autocomplete=off but bestbuy. (I suppose that's why bestbuy works.)
The previous patch (bug 1392528) only allows form[autocomplete="off"] case.
The latest patch allows input[autocomplete="off"] now.
After verifying Amazon, Sears and Macy's, they are able to show the popup and fill the selected credit card profile.
(In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
> Created attachment 8906439 [details]
> bestbuy
> 
> Hi Gabi,
> 
> (In reply to Gabi Cheta from comment #0)
> > Amazon.com     
> > sears.com  
> Amazon.com and sears.com can not show the popup indeed. I am trying to
> figure out why `startSearch()` is not invoked.
> 
> > bestbuy.com 
> The popup for bestbuy works and can fill the card number. (see the
> attachment)
> May I know how do you verify it?
> 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1392528 patch for
> > Macys/Newegg/Costco/QVC
> Does this mean the patch in bug 1392528 can fix these sites
> (Macys/Newegg/Costco/QVC)?

Hi Sean, re-verified bestbuy on the latest nightly and its seems to work but not from the 1st try, the auto fill CC is triggered only after refreshing the checkout page.
Comment on attachment 8906481 [details]
Bug 1398101 - Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`.

https://reviewboard.mozilla.org/r/178238/#review183858

This should have a test so it doesn't regress.
Attachment #8906481 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8906481 [details]
Bug 1398101 - Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`.

https://reviewboard.mozilla.org/r/178238/#review183900

I think we also need to update the code that falls back to Form History to ensure it doesn't fallback for autocomplete=off
Attachment #8906481 - Flags: review+ → review-
Blocks: 1400112
Depends on: 1393374
Whiteboard: [form autofill:MVP]
Since bug 1393374 blocks this bug, I am working with Ray to investigate the memory leak issue of the credit card mochitest in bug 
1393374.
Tree herder results are for investigating if the new test in this patch has the same memory leak issue:
with skip-if=debug:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bffced397855660fef228564a4bdf862941f1cc
without skip-if=debug:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=164266e7931f41f39e19ccd20740dd36ea436743
Comment on attachment 8906481 [details]
Bug 1398101 - Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`.

https://reviewboard.mozilla.org/r/178238/#review188534

Thanks

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:148
(Diff revision 5)
> +  info("not expecting a popup");
> +  return new Promise((resolve, reject) => {
> +    expectingPopup = reject.bind(this, "Unexpected Popup");
> +    // TODO: We don't have an event to notify no popup showing, so wait for 500
> +    // ms (in default) to predict any unexpected popup showing.
> +    setTimeout(resolve, ms);

I think you may have to add the new eslint rule to disable timeouts in tests for this line… not sure if that applies to mochitest-plain though. You'll see if it fails when running eslint.

::: browser/extensions/formautofill/test/mochitest/test_basic_creditcard_autocomplete_form.html:219
(Diff revision 5)
>    checkMenuEntries(MOCK_STORAGE.map(patchRecordCCNumber).map(cc => JSON.stringify({
>      primary: cc["cc-name"],
>      secondary: cc.ccNumberFmt.affix + cc.ccNumberFmt.label,
>    })));
> +
> +  await cleanUpCreditCards();

This should be done in the `SimpleTest.registerCleanupFunction` of autofill_common so all tests cleanup after themselves. That way you don't have to think about what needs to be cleaned up in each test.

::: browser/extensions/formautofill/test/mochitest/test_creditcard_autocomplete_off.html:83
(Diff revision 5)
> +  checkMenuEntries(MOCK_STORAGE.map(patchRecordCCNumber).map(cc => JSON.stringify({
> +    primary: cc["cc-name"],
> +    secondary: cc.ccNumberFmt.affix + cc.ccNumberFmt.label,
> +  })));
> +
> +  await cleanUpCreditCards();

Same here, just have it done in the common file for all tests.
Attachment #8906481 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8906481 [details]
Bug 1398101 - Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`.

https://reviewboard.mozilla.org/r/178238/#review188534

> I think you may have to add the new eslint rule to disable timeouts in tests for this line… not sure if that applies to mochitest-plain though. You'll see if it fails when running eslint.

Let me drop it at this moment since bug 1389234 is not landed yet.

> This should be done in the `SimpleTest.registerCleanupFunction` of autofill_common so all tests cleanup after themselves. That way you don't have to think about what needs to be cleaned up in each test.

I found we had a cleanup code `formFillChromeScript.sendAsyncMessage("cleanup");` in the cleanup handler of `formautofill_common.js`, but it is without `await` to wait for the cleanup procedure finished. After adding async and await like this `await formFillChromeScript.promiseOneMessage("cleanup");`, the test case can not proceed correctly. I am trying to fix this case.
Comment on attachment 8913614 [details]
Bug 1398101 - Make sure all cleanup code paths are with `await` to ensure the cleanup process finished.

https://reviewboard.mozilla.org/r/185002/#review190078

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:169
(Diff revision 1)
>      }
>    });
>  
> -  SimpleTest.registerCleanupFunction(() => {
> +  SimpleTest.registerCleanupFunction(async () => {
>      formFillChromeScript.sendAsyncMessage("cleanup");
> +    await formFillChromeScript.promiseOneMessage("cleanup-results");

The test should wait "cleanup" operation finished. Since it's an async message, we need another cleanup finished message e.g. "cleanup-results" to make sure the promise is resolved.

I suspect these unresolved promises are the root cause of the memory leak issue in bug 1401454

::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js:92
(Diff revision 1)
>    },
>  
>    async cleanUpAddresses() {
>      const guids = (await this._getRecords(ADDRESSES_COLLECTION_NAME)).map(record => record.guid);
>  
> +    if (guids.length == 0) {

An empty storage won't fire a storage change event for a "remove" operation since nothing is changed or removed. That's why `await cleanUpAddresses()` and `await cleanUpCreditCards()` will not be resolved, and guid lenght check is necessary here.
Comment on attachment 8913614 [details]
Bug 1398101 - Make sure all cleanup code paths are with `await` to ensure the cleanup process finished.

https://reviewboard.mozilla.org/r/185002/#review192282
Attachment #8913614 - Flags: review?(MattN+bmo) → review+
There is an error
"application crashed [@ mozilla::binding_danger::TErrorResult<mozilla::binding_danger::AssertAndSuppressCleanupPolicy>::~TErrorResult()] "
at "test_autofocus_form.html" test in the debug build.

I add "skip-if=debug" at "test_autofocus_form.html" to test others, but others have the same problem.[1]

I am investigating if the issue is related to the part 1 patch.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dbd127a461186fdf064a753f2cfa8c72e690b0f
Comment on attachment 8906481 [details]
Bug 1398101 - Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`.

https://reviewboard.mozilla.org/r/178238/#review193304

Are you sure the issue is related to your patches or perhaps you rebased on a bad revision?

::: browser/extensions/formautofill/FormAutofillContent.jsm:118
(Diff revision 7)
> +      if (focusedInput.autocomplete == "off") {
> +        return;
> +      }

Perhaps you need to call onSearchResult with an empty result? I'm not sure if that's required or not?
Hey MattN, thanks for the suggestion! I pushed a try based on your suggestion:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb19651398ab4b38c74c368c752b2d843767f3b9
The assertion looks to be related to a syncMessage (we only use one) so it's probably easiest to narrow down the lines of code causing the problem.
Comment on attachment 8906481 [details]
Bug 1398101 - Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`.

https://reviewboard.mozilla.org/r/178238/#review193400

::: browser/extensions/formautofill/FormAutofillContent.jsm:118
(Diff revision 7)
> +      if (focusedInput.autocomplete == "off") {
> +        return;
> +      }

After adding `listener.onSearchResult(this, null);`, these issues still happen in debug build only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb19651398ab4b38c74c368c752b2d843767f3b9&selectedJob=136066614

Another try-push is with only part 1 patch, so I think it's related to the part 2 (mochitest refactor) only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc210b647bfea8549a272bd5616a909a5f2485ea
Comment on attachment 8913614 [details]
Bug 1398101 - Make sure all cleanup code paths are with `await` to ensure the cleanup process finished.

https://reviewboard.mozilla.org/r/185002/#review193430

::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js
(Diff revision 2)
> +
>      await this.operateCreditCard("remove", {guids}, "FormAutofillTest:CreditCardsCleanedUp");
>    },
>  
>    async cleanup() {
> -    Services.obs.removeObserver(this, "formautofill-storage-changed");

Sorry to jump in :D

I guess it might worth trying to revert this part since the observer might send some unnecessary messages to content while doing storage cleanup.
Comment on attachment 8913614 [details]
Bug 1398101 - Make sure all cleanup code paths are with `await` to ensure the cleanup process finished.

https://reviewboard.mozilla.org/r/185002/#review193458

::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js
(Diff revision 2)
> +
>      await this.operateCreditCard("remove", {guids}, "FormAutofillTest:CreditCardsCleanedUp");
>    },
>  
>    async cleanup() {
> -    Services.obs.removeObserver(this, "formautofill-storage-changed");

Hey Ray, thanks for the suggestion! Unfortunately, it doesn't work. ;(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=324ffb741b4a3044d72d41d4ffe2a4179e25a5eb

BTW, my original idea is that the clean up process should wait for "formautofill-storage-changed" message.
Comment on attachment 8913614 [details]
Bug 1398101 - Make sure all cleanup code paths are with `await` to ensure the cleanup process finished.

https://reviewboard.mozilla.org/r/185002/#review193508

::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js:209
(Diff revision 3)
>  addMessageListener("FormAutofillTest:CleanUpCreditCards", (msg) => {
>    ParentUtils.cleanUpCreditCards();
>  });
>  
>  addMessageListener("cleanup", () => {
> -  ParentUtils.cleanup();
> +  ParentUtils.cleanup().then(() => {

The crash is caused by the `async` handler of `addMessageListener` in my previous patch, so I use a` promise` rather than `async` function.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=267072d6f2fa93894fdbff48977d4b27f5c97d86
Hey MattN,

The mochitest issue is resolved by the solution at comment 34. The latest patch is also with `listener.onSearchResult(this, result);` for focusedInput.autocomplete == "off" case in FormHistory fallback.

I think it's ready to land. Thank you for the review!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/50e7e1b822ec
Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`. r=MattN
https://hg.mozilla.org/integration/autoland/rev/f2808379c028
Make sure all cleanup code paths are with `await` to ensure the cleanup process finished. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/50e7e1b822ec
https://hg.mozilla.org/mozilla-central/rev/f2808379c028
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Verified as fixed with 58.0a1 on Windows 10x64, Ubuntu 14.4 and MacOS 10.12
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.