Gabi Cheta [:Gabi] Desktop QA Reporter
	Description • 7 years ago

[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

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 1 • 7 years ago

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)?

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 2 • 7 years ago

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.)

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 4 • 7 years ago

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.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 5 • 7 years ago

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

	Gabi Cheta [:Gabi] Desktop QA Reporter
	Comment 6 • 7 years ago

(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.

	Matthew N. [:MattN]
	Comment 7 • 7 years ago
mozreview-review

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.

	Matthew N. [:MattN]
	Comment 8 • 7 years ago
mozreview-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

	Matthew N. [:MattN]
	Comment 9 • 7 years ago
mozreview-review-reply

Comment on attachment 8906481 [details]
Bug 1398101 - Allow the FormAutofill fields with autocomplete="off" to proceed `startSearch`.

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

https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/browser/extensions/formautofill/FormAutofillContent.jsm#111-120

Please add a test for this too.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 10 • 7 years ago

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

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 11 • 7 years ago

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.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 15 • 7 years ago

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

	Matthew N. [:MattN]
	Comment 17 • 7 years ago
mozreview-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

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.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 18 • 7 years ago
mozreview-review-reply

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.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 21 • 7 years ago
mozreview-review

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.

	Matthew N. [:MattN]
	Comment 22 • 7 years ago
mozreview-review

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

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 25 • 7 years ago

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

	Matthew N. [:MattN]
	Comment 26 • 7 years ago
mozreview-review

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?

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 27 • 7 years ago

Hey MattN, thanks for the suggestion! I pushed a try based on your suggestion:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb19651398ab4b38c74c368c752b2d843767f3b9

	Matthew N. [:MattN]
	Comment 28 • 7 years ago

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.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 29 • 7 years ago
mozreview-review

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

	Ray Lin[:ralin]
	Comment 30 • 7 years ago
mozreview-review

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.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 31 • 7 years ago
mozreview-review

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.

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 34 • 7 years ago
mozreview-review

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

	Sean Lee [:seanlee][:weilonge] Assignee
	Comment 37 • 7 years ago

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!

	Pulsebot
	Comment 38 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 39 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/50e7e1b822ec
https://hg.mozilla.org/mozilla-central/rev/f2808379c028

	Gabi Cheta [:Gabi] Desktop QA Reporter
	Comment 40 • 7 years ago

Verified as fixed with 58.0a1 on Windows 10x64, Ubuntu 14.4 and MacOS 10.12