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

Status: VERIFIED FIXED

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

Description

[Timea Cernea [:tbabos][inactive]]

[Affected versions]: 
Nightly 64.0a1

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

[Preconditions]:
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 "https://rsolomakhin.github.io/pr/us/" 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.

[Regression-Range]:
Last good revision: f3147045b0b51cce8b9e5551c7f870d4a0576bcd
First bad revision: 3f6664786e8531bc621d0113328d73cf3556193f

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f3147045b0b51cce8b9e5551c7f870d4a0576bcd&tochange=3f6664786e8531bc621d0113328d73cf3556193f

Comment 1

[Timea Cernea [:tbabos][inactive]]

Can you please take a look at this?

Comment 2

[Sam Foster [:sfoster] (he/him)]

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

Comment 3

[Matthew N. [:MattN]]

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.

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Add state checking when calling PaymentRequestService's methods.

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.

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: mochitest for the state checking when interacting with PaymentRequest.

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Add state checking when calling PaymentRequestService's methods.

Update the patch for the case
3. PaymentRequest state is eCreated and response for CANMAKE_ACTION.

Comment 7

[Andrea Marchesini [:baku]]

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.

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Add state checking when calling PaymentRequestService's methods.

Update the patch according to comment 7.

and tryserver result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5247ef08539954b5101de69f74def37045e0d77b&selectedJob=199093772

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13885f4cecfc
mochitest for bug 1490698. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/45fc4a1d397f
Add state checking when interactive with PaymentRequest through PaymentRequestService. r=edenchuang

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/13885f4cecfc
https://hg.mozilla.org/mozilla-central/rev/45fc4a1d397f

Comment 11

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 64.0a1 on Windows 10 x 64, Windows 7 x32 and Ubuntu 18.04 x64.