1497219 - Crash in mozilla::dom::PaymentRequest::Show

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-58f7bb00-bc2b-4aff-9e42-47f400181008.
=============================================================

Seen while looking at Mac nightly crash stats - although only crash so far, I thought it was interesting to file since I don't see many Web Payments crashes.

Top 10 frames of crashing thread:

0 XUL mozilla::dom::PaymentRequest::Show xpcom/base/nsCOMPtr.h:919
1 XUL mozilla::dom::PaymentRequest_Binding::show_promiseWrapper dom/bindings/PaymentRequestBinding.cpp:3181
2 XUL bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ConvertExceptionsToPromises> dom/bindings/BindingUtils.cpp:3315
3 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:461
4 XUL js::ForwardingProxyHandler::call const js/src/vm/Interpreter.cpp:626
5 XUL js::CrossCompartmentWrapper::call const js/src/proxy/CrossCompartmentWrapper.cpp:355
6 XUL js::Proxy::call js/src/proxy/Proxy.cpp:560
7 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:528
8 XUL Interpret js/src/vm/Interpreter.cpp:613
9 XUL js::RunScript js/src/vm/Interpreter.cpp:440

=============================================================

Comment 1

[Marion Daly [:mdaly]]

Eden: can you take a look at this?

Comment 2

[Eden Chuang[:edenchuang]]

The root cause is accessing the deleted PaymentRequest object.
So JS code cannot find the binding PaymentRequest, then it gets crash.

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Define a new IPC structure for response data

In the current design, payment method response data is passed between processes through a simple nsString. It means a special encoder/decoder is needed for special response data, ex. BasicCardResponse, to serialize/deserialize into/from the nsString. However, when a token splitter, ':', ';' and '@', is used in response data, it makes the encoder/decoder can not work normally. It is hard to define a suitable token splitter set for the encoder/decoder. So instead of using an error-prone encoder/decoder, this patch defines a new IPC structure for response data.

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 9018296 [details] [diff] [review]
Define a new IPC structure for response data

Review of attachment 9018296 [details] [diff] [review]:
-----------------------------------------------------------------

much better! Thanks.

::: dom/payments/PaymentRequestManager.cpp
@@ +661,5 @@
>        switch (response.status()) {
>          case nsIPaymentActionResponse::PAYMENT_ACCEPTED: {
>            rejectedReason = NS_OK;
> +          NS_ENSURE_SUCCESS(ConvertResponseData(aRequest->GetOwner(),
> +                                            response.data(),

indentation

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Define a new IPC structure for response data

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: Reject PaymentRequest actions when its owner global object is null by navigating to other pages.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 9028586 [details] [diff] [review]
Reject PaymentRequest actions when its owner global object is null by navigating to other pages.

Review of attachment 9028586 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequest.cpp
@@ +730,2 @@
>    nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(global);
> +  if (!win) {

This cannot happen.

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Reject PaymentRequest actions when its owner global object is null.

Update patch according to reviewer's comment.
Update patch with new C++ coding style change.

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: mochitest for payment in not fully active document.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2738bb9f6d54
Rejecting PaymentRequest::API calls when the PaymentRequest is not in fully active document. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba89c2d426d
Mochitest for PaymentRequest in not fully active document. r=baku

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2738bb9f6d54
https://hg.mozilla.org/mozilla-central/rev/7ba89c2d426d