Closed Bug 1490698 Opened Last year Closed Last year

The browser crashes if the payment process is canceled via "X" button


(Core :: DOM: Web Payments, defect, P1)




Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- disabled
firefox64 --- verified


(Reporter: tbabos, Assigned: edenchuang)



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

Crash Data


(2 files, 2 obsolete files)

[Affected versions]: 
Nightly 64.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Ubuntu 18.04 x64.

1. Set the pref dom.payments.request.enabled to "true";
2. Make sure you have at least one Shipping Address and one Credit Card saved in your browser.

[Steps to reproduce]:
1. Go to "" page and click on "Buy".
2. Select one valid option for Shipping Address and Payment Method
3. Click on "Pay"
4. Click on "X" while the payment is being processed

[Expected result]:
The Payment should be processed and the response should be displayed accordingly

[Actual result]:
The browser crashes.

Last good revision: f3147045b0b51cce8b9e5551c7f870d4a0576bcd
First bad revision: 3f6664786e8531bc621d0113328d73cf3556193f

Flags: qe-verify+
Can you please take a look at this?
Flags: needinfo?(sfoster)
Crash Signature: → [@ mozilla::dom::Promise::MaybeSomething<T> ]
I'm not sure what the expected behavior is here (apart from crashing which is obviously not good). If the response has been sent and we are waiting for complete(), what should closing the window do? The ship has sailed at that point, so ISTM any request to cancel or abort the payment should be ignored. I.e. closing the window means something different once pay() has been called. 

I guess this is technically a regression as previously either you couldn't close the dialog, or if you did it had no impact on any pending payment request (which left it effectively orphaned.) With this change we explicitly cancel the current payment request when the user closes the window, by creating a PAYMENT_REJECTED response and sending it via the payment service. 

We can try and avoid getting into this state on the front-end, but would need to be a new bug. The crash seems like it needs DOM fix
Flags: needinfo?(sfoster)
Flags: needinfo?(echuang)
Flags: needinfo?(MattN+bmo)
Yeah, we need a clear contract between the DOM and front-end (documented in the IDL) on who is responsible for avoiding a crash in these cases.
Flags: needinfo?(MattN+bmo)
Add state checking when calling RespondPayment(), ChangeShippingAddress() and ChangeShippingOption(). 

Calling ChangeShippingXXX is valid when the PaymentRequest state is interactive.
Calling RespondPayment is valid when following situations
  1. PaymentRequest state is interactive
  2. PaymentRequest state is closed but the response is for CompleteAction.
Assignee: nobody → echuang
Flags: needinfo?(echuang)
Attachment #9008581 - Flags: review?(amarchesini)
Update the patch for the case
3. PaymentRequest state is eCreated and response for CANMAKE_ACTION.
Attachment #9008581 - Attachment is obsolete: true
Attachment #9008581 - Flags: review?(amarchesini)
Attachment #9008618 - Flags: review?(amarchesini)
Comment on attachment 9008618 [details] [diff] [review]
Add state checking when calling PaymentRequestService's methods.

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

::: dom/payments/PaymentRequestData.h
@@ +225,5 @@
>                 const nsAString& aPaymentMethodErrors,
>                 const nsAString& aShippingAddressErrors);
> +  enum eState {
> +    eUnknown,

1. eUnknown is not used. Remove it
2. Write a comment for each of these states explaining what they are and when they are used.

::: dom/payments/PaymentRequestService.cpp
@@ +320,5 @@
> +      }
> +      MOZ_ASSERT(payment);
> +      payments::PaymentRequest* rowPayment =
> +        static_cast<payments::PaymentRequest*>(payment.get());
> +      MOZ_ASSERT(rowPayment);

Can we have an helper method the does this? SetPaymentStateFromId() ? You should be able to remove this block of code and the previous one.
Attachment #9008618 - Flags: review?(amarchesini) → review+
Attachment #9008582 - Flags: review?(amarchesini) → review+
Priority: -- → P1
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Keywords: checkin-needed
Pushed by
mochitest for bug 1490698. r=baku
Add state checking when interactive with PaymentRequest through PaymentRequestService. r=edenchuang
Keywords: checkin-needed
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Verified as fixed on Firefox Nightly 64.0a1 on Windows 10 x 64, Windows 7 x32 and Ubuntu 18.04 x64.
Flags: qe-verify+
Duplicate of this bug: 1490754
Component: WebPayments UI → DOM: Web Payments
Product: Firefox → Core
Target Milestone: Firefox 64 → mozilla64
You need to log in before you can comment on or make changes to this bug.