Closed Bug 1497219 Opened 11 months ago Closed 9 months ago

Crash in mozilla::dom::PaymentRequest::Show

Categories

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

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- disabled
firefox65 --- disabled
firefox66 --- fixed

People

(Reporter: marcia, Assigned: edenchuang)

References

Details

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

Crash Data

Attachments

(2 files, 3 obsolete files)

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

=============================================================
Priority: -- → P2
Eden: can you take a look at this?
Flags: needinfo?(echuang)
The root cause is accessing the deleted PaymentRequest object.
So JS code cannot find the binding PaymentRequest, then it gets crash.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Flags: needinfo?(echuang)
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.
Attachment #9018296 - Flags: review?(amarchesini)
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
Attachment #9018296 - Flags: review?(amarchesini) → review+
Attachment #9018296 - Attachment is obsolete: true
Attachment #9018977 - Flags: review+
Priority: P2 → P1
Whiteboard: [webpayments-reserve]
Duplicate of this bug: 1499954
Flags: qe-verify-
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.
Attachment #9028586 - Flags: review?(amarchesini) → review+
Update patch according to reviewer's comment.
Update patch with new C++ coding style change.
Attachment #9028586 - Attachment is obsolete: true
Attachment #9031423 - Flags: review+
Keywords: checkin-needed
Pushed by ccoroiu@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2738bb9f6d54
https://hg.mozilla.org/mozilla-central/rev/7ba89c2d426d
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.