Closed Bug 1490698 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- disabled
firefox64 --- verified

People

(Reporter: tbabos, Assigned: edenchuang)

References

Details

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

Crash Data

Attachments

(2 files, 2 obsolete files)

[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
Flags: qe-verify+
Can you please take a look at this?
Flags: needinfo?(sfoster)
Crash Signature: https://crash-stats.mozilla.com/report/index/b43fed91-638d-4fe3-a7da-713c10180912 → [@ 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+
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/13885f4cecfc
https://hg.mozilla.org/mozilla-central/rev/45fc4a1d397f
Status: ASSIGNED → RESOLVED
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.
Status: RESOLVED → VERIFIED
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.