Closed Bug 1497514 Opened 3 years ago Closed 3 years ago

CC is saved even when "There was an error saving the payment card." message is displayed


(Firefox :: WebPayments UI, defect, P1)

64 Branch



Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- verified


(Reporter: hani.yacoub, Assigned: dpino)


(Keywords: regression, Whiteboard: [webpayments-reserve])


(2 files, 3 obsolete files)

[Affected versions]: 
Nightly 64.0a1 

[Affected platforms]:
Platforms: Windows 10 x64, Ubuntu 18.04, Mac OS 10.13

1. Set the pref dom.payments.request.enabled to "true";
2. Make sure you are using a clean profile.

[Steps to reproduce]:
1. Go to "" page and click on "Buy".
2. Fill "Add shipping address" from correctly.
3. Fill "Add Credit Card" form correctly.
4. Close the payment dialog.
5. Go to "about:preferences" -> Privacy & Security -> Forms & Autofill.
6. Click on "Saved Credit Cards.." and delete the CC introduced before.
7. Go back to "" page and click on "Buy".
8. Fill "Add Credit Card" form correctly. 

[Expected result]:
CC should be saved without any errors.

[Actual result]:
CC is saved even when "There was an error saving the payment card." message is displayed.
Flags: qe-verify+
QA Contact: hani.yacoub
Whiteboard: [webpayments] [triage]
I found new steps to reproduce this issue:
1. Make sure you are on a FTU flow.
2. Go to "" page and click on "Buy".
3. Fill "Add shipping address" from correctly.
4. From "Add Credit Card" form click on "Cancel" or on the "X" button.
5. From "" page and click on "Buy" again.
6. Fill "Add Credit Card" form correctly.
7. Observe the error message displayed.
So if I turned on logging I can see the error is caused by this error,435

It is unclear to me where selectedStateKey is set, and what is supposed to be. Matt, is that something obvious to you? If not I will keep debugging.
Flags: needinfo?(MattN+bmo)
Priority: -- → P1
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Wait, I've just realized the comment 1 linked to the other thing I am working on, which is totally unrelated to payment.

Timea, would you be able to check the regression range again? I backed out the bugs (and their dependencies) locally and I can still reproduce this bug.

(Leaving unassigned for now)

@  489509:31af1457befc timdream tip Backed out changeset 50d1b7c5b1a1
o  489508:ba8caabbafce timdream Backed out changeset d8dca67e2440
o  489507:578b0fd3e5d0 timdream Backed out changeset 4392b5198fb7
o  489506:a979b65806ad timdream Backed out changeset a632efc6cac4
o  489505:96549b50926d timdream Backed out changeset 039c4b2029a4
o    489504:20773596530b apavel central Backed out changeset 39f0d4e66898 (bug 675428) for causing Bug 1496380 a=backout
Assignee: timdream → nobody
Flags: needinfo?(MattN+bmo) → needinfo?(timea.babos)
Priority: P1 → P3
Whiteboard: [webpayments-reserve] → [webpayments] [triage]
Hi Tim,

Thanks for checking it on your side! I checked it again with mozregression using the steps from Comment 2 (makes it easier to repro) and you were right. I found the issue that is actually Payments related, makes sense now. Sorry for the inconvenience created before :(

Last good revision: 39762ef5d56e07f8046bf9dfb1b32f194c6ea1bd
First bad revision: 4ff0fcd61c7a1f791414516a6e22efca94530f19

Bug 1490805 - Add the CVV security code field to the add card form and make it required in all places 

Matt, can you please take a look at this?
Flags: needinfo?(timea.babos) → needinfo?(MattN+bmo)
I took a look at this bug.

As pointed out in the bug description, the error happens at:

The first time the basic credit card form is shown, selectedStateKey is initialized to 'selectedPaymentCard'. The initialization happens at:

The second time, since an address already exists, selectedStateKey is not initialized. In fact, its value is initialized to null at:

And since its value is null an Error is thrown (

If basicCardPage.selectedStateKey is initialized to 'selectedPaymentCard' in PaymentStateSubscriberMixin.js the error doesn't happen. That's what the attached patch does to avoid the bug. But I'm not sure that's the right fix.
Attachment #9017523 - Flags: review?(MattN+bmo)
Assignee: nobody → dpino
Priority: P3 → P1
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Comment on attachment 9017523 [details] [diff] [review]

Review of attachment 9017523 [details] [diff] [review]:

::: browser/components/payments/res/mixins/PaymentStateSubscriberMixin.js
@@ +16,5 @@
>    orderDetailsShowing: false,
>    "basic-card-page": {
>      guid: null,
>      // preserveFieldValues: true,
> +    selectedStateKey: "selectPaymentCard",

Can you find the place where the is set to "basic-card-page" and set this there instead?
Attachment #9017523 - Flags: review?(MattN+bmo)
Updated patch.
Attachment #9017523 - Attachment is obsolete: true
Attachment #9017822 - Flags: review?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
Comment on attachment 9017822 [details] [diff] [review]

Review of attachment 9017822 [details] [diff] [review]:

Thanks! That's what I was looking for. Now can you add the following after the check[1] in basic-card-form to prevent this from happening again by causing test failures:

if (!basicCardPage.selectedStateKey) {
  throw new Error("A `selectedStateKey` is required");

Then run all tests and ensure they pass or modify then to add a selectedStateKey: 
> ./mach test browser/components/payments/

Attachment #9017822 - Flags: review?(MattN+bmo) → feedback+
I've added a new patch adding the check suggested to `basic-card-form.js`. After that, I run the tests and there was one set of tests that reported several errors. All the fails where in one single test: `test/mochitest/test_basic_card_form.html`.

I tried to fix the tests by adding a `selectedStateKey: "selectedPaymentCard"` to all `basic-card-page` state objects defined (or defining it if necessary). Now the tests don't fail due to the extra check added in `basic-card-form.js`, but report an error in function `checkCCForm` (

It seems that when the function tries to fetch a property from the document, that property doesn't exist. I tried to fix the issue, but no success yet.
Attachment #9018197 - Flags: feedback?(MattN+bmo)
Comment on attachment 9018197 [details] [diff] [review]

Review of attachment 9018197 [details] [diff] [review]:

I changed my mind that it's fine to change the default value as long as we have the new `throw`.
Attachment #9018197 - Flags: feedback?(MattN+bmo) → review+
Attachment #9017822 - Flags: feedback+ → review+
Pushed by
Initialize BasicCardPage selectedStateKey to 'selectPaymentCard'. r=MattN
It looks like the failure is only with QuantumRender… Diego, can you import and debug the failure using Try (or locally if you can run Quantum Render)? There is probably an existing race in the test.
Flags: needinfo?(hani.yacoub) → needinfo?(dpino)
I imported the patch and run the test. It seems some tests that were using "basic-card-page" needed to be initialized with "selectedStateKey: selectedPaymentCard". After that change, the test passed.
Attachment #9017822 - Attachment is obsolete: true
Attachment #9018197 - Attachment is obsolete: true
Flags: needinfo?(dpino)
Attachment #9018523 - Flags: review?(MattN+bmo)
Comment on attachment 9018523 [details] [diff] [review]

Review of attachment 9018523 [details] [diff] [review]:

Ah, yeah, of course. Thanks!
Attachment #9018523 - Flags: review?(MattN+bmo) → review+
Are you able to push to try server?
Keywords: checkin-needed
Pushed by
Initialize BasicCardPage selectedStateKey to 'selectPaymentCard'. r=MattN
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
No longer blocks: 1493525
Verified as fixed on Firefox Nightly 64.0a1 (2018-10-21) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.