1747898 - Credit card record is not correctly passed into GeckoView

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, defect, P2)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Screen Shot 2021-12-29 at 10.11.35 AM.png

While working on supporting a save credit prompt, I noticed that only the credit card number (not the guid, expiry dates) manages to be passed into GeckoView.

My primary investigation happened on desktop:

1. Navigate to https://checkout.stripe.dev/preview and fill out a credit card information and perform a form submit.
2. In the debugger, observe what the creditCard object looks like before calling FormAutofillPrompter.promptToSaveCreditCard. See screenshot.
3. Notice that the parameters for the creditCard does that line up when translated in https://searchfox.org/mozilla-central/source/toolkit/components/formautofill/android/FormAutofillPrompter.jsm#46 to newCreditCard and CreditCard.fromGecko(newCreditCard.record).

It seemed interesting to me that that the creditCard.record parameters no longer matches the parameter that is expected when it's being translated to GeckoView prior to calling GeckoViewAutocomplete.onCreditCardSave.

In https://searchfox.org/mozilla-central/source/toolkit/components/formautofill/android/FormAutofillPrompter.jsm#53-58, we are expecting to have record.expMonth and record.expYear, but I am not quite sure if we even have those parameters anymore. From the screenshot, you can see we have cc-exp, cc-name, and cc-number in creditCard.record.

Similarly, in https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewAutocomplete.jsm#256 called from https://searchfox.org/mozilla-central/source/toolkit/components/formautofill/android/FormAutofillPrompter.jsm#64 is calling. We are also expecting a different set of parameters inside of the creditCard.record object that is passed to fromGecko.

Expected outcome is that we should be able to get the credit card name, guid, expiry date in the credit card object passed to GeckoView. It would be highly desirable to ensure we validate that none of the parameters are null by calling isValid() prior to calling GeckoViewAutocomplete.onCreditCardSave(selectedCreditCard). In Fenix, we are expecting to save a credit card with a number and expiry date since we do not expect these values to be nullable.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Chris Peterson [:cpeterson]]

Fenix's credit card autofill feature is blocked waiting for this bug fix.

https://mozilla-hub.atlassian.net/browse/FNXV2-17088

Comment 3

[Dimi Lee [:dimi]]

I'll look into this issue this week.

Comment 4

[Dimi Lee [:dimi]]

Attachment: Bug 1747898 - P1. Use default region when "browser.search.region" pref is not present r=sgalich,tgiles

This was added in Bug 1747096 but was accidentally was removed in Bug 1745248.

Comment 5

[Dimi Lee [:dimi]]

Attachment: Bug 1747898 - P2. Fix issues in `promptToSaveCreditCard` r=sgalich,tgiles

There are two bugs in promptToSaveCreditCard

1. newCreditCard doesn't contain record memeber variable
2. creditCard.record should use cc-name, cc-number, etc to access
   data instead of record.name, record.number

Depends on D141766

Comment 6

[Dimi Lee [:dimi]]

Even with these patches, expiration month and year are still not correctly passed while submitting the form in https://checkout.stripe.dev/preview, but this seems to be a generic bug. I filed bug 1760834 to track this issue.

Comment 8

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

This regression was caused by a backout of the browser.search.region pref from bug 1747096. Without this fix, credit card autofill works in Firefox Android 98, but not 99 or 100.

Is this fix safe to uplift to Beta 99?

Comment 9

[Donal Meehan [:dmeehan]]

Please submit a beta uplift request when this is ready.
This week is the final week of beta for 99

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19d2ac03e307
P1. Use default region when "browser.search.region" pref is not present r=sgalich,tgiles
https://hg.mozilla.org/integration/autoland/rev/86636c7226c3
P2. Fix issues in `promptToSaveCreditCard` r=sgalich,tgiles

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/19d2ac03e307
https://hg.mozilla.org/mozilla-central/rev/86636c7226c3

Comment 12

[Dimi Lee [:dimi]]

(In reply to Chris Peterson [:cpeterson] from comment #8)

Is this fix safe to uplift to Beta 99?

Yes, I think this is safe to uplift.
Is it possible someone in your team help test fenix with the latest build? (but not with the reported website https://checkout.stripe.dev/preview because there is still an issue for this site). If everything works fine, I'll request uplift, thanks!

Comment 13

[Donal Meehan [:dmeehan]]

Please note, tomorrow is the final beta for 99. Please submit an uplift request as soon as this ready. Thanks.

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Dimi Lee [:dimi][:dlee] from comment #12)

Is it possible someone in your team help test fenix with the latest build? (but not with the reported website https://checkout.stripe.dev/preview because there is still an issue for this site). If everything works fine, I'll request uplift, thanks!

I filed a PI request asking Delia (the QA tester who tested CC Autofill in Android v98 and found the v99 regression) to retest CC Autofill in Nightly v100:

https://mozilla-hub.atlassian.net/browse/QA-1441

If it’s too late to uplift the fix to Beta v99, we can just let it ride the trains in v100. In that case, CC recording will work in v98, stop working in v99, and start working again four weeks later in v100. Not the end of the world.

Comment 15

[Dimi Lee [:dimi]]

Hi Chris, thank you for filing the PI request.
I just test this with latest build with geckoview example app on emulator (verify debug message in geckoview side because I believe we don't support prompt with the example app). It looks good.
Since this is simple change and part of the change has already been verified before (before been backout), I'll just request uplift now.

Comment 16

[Dimi Lee [:dimi]]

Comment on attachment 9268908 [details]
Bug 1747898 - P2. Fix issues in promptToSaveCreditCard r=sgalich,tgiles

Beta/Release Uplift Approval Request

• User impact if declined: Credit Card Capture doesn't work on fenix
• Is this code covered by automated tests?: Unknown. Not sure if mobile team has testcase covers this up.
• Has the fix been verified in Nightly?: Yes, with geckoview example app.
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Save:

1. navigate to a web page that requires credit card information
2. fill in the credit card information and then submit the form
3. Check the save credit card prompt shows up and contain the correct information
   Update:
4. navigate to a web page that requires credit card information
5. autofill the credit card information previously saved
6. Change one of the fields (name or expiration date) and then submit the form
7. Check the update credit card prompt shows up and contain the correct information

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): There are two part of this patch. The part 1 of this patch has already been verified before. We only restore the change back. The change itself is also only reading a default value when the pref doesn't exist.
  The part 2 of this patch doesn't change the logic, it only fixes issue by accessing the correct member variable.
• String changes made/needed:

Comment 17

[Dimi Lee [:dimi]]

Hi Chris, I'm sorry I didn't look into this issue earlier, I thought this was a site-specific issue (I thought if this was a generic problem, it would be discovered by our testcases). So in order to make the desktop change not break the support of cc on mobile, I would like to know whether we have testcases that cover "save" and "update" credit card doorhanger on the mobile. If yes, why it didn't catch this change? What can our team do to prevent this from happening again? Thanks!

Comment 18

[Donal Meehan [:dmeehan]]

:dimi looks like Part 1, Attachment #9268907 [details], should also have approval-mozilla-beta set to "?"
Please confirm

Comment 19

[Delia Pop]

Hello,

I attempted testing the CC Autofill with Firefox Nightly 100.0a1 (Build 2015870571 GV: 100.0a1-20220323094932) from 7 hours ago, but the feature is not available.
Should we expect the feature to be available in the tomorrow's Nightly build?
If the feature is making it to the latest Beta v99 from this week, I will proceed and test it when the build will be provided.

Comment 20

[Dimi Lee [:dimi]]

Comment on attachment 9268907 [details]
Bug 1747898 - P1. Use default region when "browser.search.region" pref is not present r=sgalich,tgiles

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9268907 [details]
Bug 1747898 - P1. Use default region when "browser.search.region" pref is not present r=sgalich,tgiles

Approved for 99.0b8. Thanks.

Comment 22

[Donal Meehan [:dmeehan]]

Comment on attachment 9268908 [details]
Bug 1747898 - P2. Fix issues in promptToSaveCreditCard r=sgalich,tgiles

Approved for 99.0b8. Thanks.

Comment 23

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6292a376c805
https://hg.mozilla.org/releases/mozilla-beta/rev/60924ff82bdd

Comment 24

[Chris Peterson [:cpeterson]]

(In reply to Dimi Lee [:dimi][:dlee] from comment #17)

in order to make the desktop change not break the support of cc on mobile, I would like to know whether we have testcases that cover "save" and "update" credit card doorhanger on the mobile. If yes, why it didn't catch this change? What can our team do to prevent this from happening again?

Tim or Gabriel, do you know if the credit card autofill tests run on Android? Did this regression only happen for non-US countries (UK, FR, and DE)? Do we run credit card autofill tests for different countries?

Comment 25

[Tim Giles [:tgiles]]

(In reply to Chris Peterson [:cpeterson] from comment #24)

Tim or Gabriel, do you know if the credit card autofill tests run on Android?

I don't know if Android has their own credit card tests or not. :gl will be able to answer that better than I can :)

Did this regression only happen for non-US countries (UK, FR, and DE)?

This regression was independent of region.

Do we run credit card autofill tests for different countries?

On Desktop, we test combinations of supported and unsupported locales to ensure the feature works as expected. Can't speak for Android though.

Comment 26

[Delia Pop]

(In reply to Chris Peterson [:cpeterson] from comment #24)

(In reply to Dimi Lee [:dimi][:dlee] from comment #17)

in order to make the desktop change not break the support of cc on mobile, I would like to know whether we have testcases that cover "save" and "update" credit card doorhanger on the mobile. If yes, why it didn't catch this change? What can our team do to prevent this from happening again?

Tim or Gabriel, do you know if the credit card autofill tests run on Android? Did this regression only happen for non-US countries (UK, FR, and DE)? Do we run credit card autofill tests for different countries?

Hello,

For the Fenix Android browser we have a Subsection of test cases regarding the Credit Cards functionality as part of the Full Functional Tests Suite: https://testrail.stage.mozaws.net/index.php?/suites/view/3192&group_by=cases:section_id&group_order=asc&group_id=227739 .

This section is tested upon every Geckoview release (Beta 1), however we did not catch the issue with the Autofill before since we used the https://checkout.stripe.dev/preview testing page that already has a build in prompt that can be used to copy the credit card fields, which mislead us.

Based on the latest implementations, we will update/add more test cases for the feature, and continue to test it after every new version release.

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Seems like Delia has answered about tests. I have tested the latest Beta and can see the Select Credit Card prompt working. I will pass the news over to QA to do another smoke test.