1379925 - Crash while testing PaymentRequest.onshippingaddresschange/onshippingoptionchange

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, defect)

Description

[Ben Tian (inactive)]

Crash occurs while running web platform tests for payment request shippingaddresschange/shippingoptionchange

https://w3c-test.org/payment-request/payment-request-onshippingaddresschange-attribute.https.html
https://w3c-test.org/payment-request/payment-request-onshippingoptionchange-attribute.https.html

The crash disappears if [1] is commented out.
[1] https://github.com/w3c/web-platform-tests/blob/master/payment-request/payment-request-onshippingaddresschange-attribute.https.html#L36

Comment 1

[Ben Tian (inactive)]

The crash results from [1] with log [2] and disappears with [1] commented out. Keep checking.

[1] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/dom/payments/PaymentRequestUpdateEvent.cpp#53
[2] Hit MOZ_CRASH(DOMEventTargetHelper not thread-safe) at /Users/bentian/Workspace/central/xpcom/base/nsISupportsImpl.cpp:43

Comment 2

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #1)
> [1]
> http://searchfox.org/mozilla-central/rev/
> 5dadcbe55b4ddd1e448c06c77390ff6483aa009b/dom/payments/
> PaymentRequestUpdateEvent.cpp#53

|aOwner| is NOT a payment request if the event is created from WebIDL constructor. Will revise to set |mRequest| only when the event is created by a payment request.

Comment 3

[Ben Tian (inactive)]

Attachment: Bug 1379925 - Set PaymentRequestUpdateEvent.mRequest only if event target is a payment request

The patch adds extra argument |aIsOwnerPaymentRequest| to tell event constructor whether the event target is a payment request or not.

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8886008 [details] [diff] [review]
Bug 1379925 - Set PaymentRequestUpdateEvent.mRequest only if event target is a payment request

updateWith may need to throw InvalidStateError if event's target is not PaymentRequest (i.e., mRequest == nullptr) per [1]. Cancel r? to revise.

[1] https://github.com/w3c/browser-payment-api/pull/547#issuecomment-307305938

Comment 5

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Set PaymentRequestUpdateEvent.mRequest only when event target is a PaymentRequest

Change:
Instead of in event constructor, |mRequest| is set by |PaymentRequest::DispatchUpdateEvent| after |SetTrusted|, to allow |IsTrusted| assertion check before event sets |Request|.

Comment 6

[Ben Tian (inactive)]

Attachment: [final] Patch 2: Revise method |updateWith| based on spec change, r=baku

The patch revises |updateWith| flow based on spec changes [1][2]. This patch also ensures |mRequest| is only accessible by trusted event.

[1] https://github.com/w3c/browser-payment-api/commit/cac8a0cd20bd4446f98fdecdc046fa809016cdb4
[2] https://github.com/w3c/browser-payment-api/commit/3b7f783be9161de0907add494e32a365a411d345

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8886455 [details] [diff] [review]
Patch 1 (v1): Set PaymentRequestUpdateEvent.mRequest only when event target is a PaymentRequest

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

::: dom/payments/PaymentRequestUpdateEvent.cpp
@@ +117,5 @@
>  
> +void
> +PaymentRequestUpdateEvent::SetRequest(PaymentRequest* aRequest)
> +{
> +  MOZ_ASSERT(IsTrusted());

MOZ_ASSERT(!mRequest);
MOZ_ASSERT(aRequest);

Comment 8

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Set PaymentRequestUpdateEvent.mRequest only when event target is a PaymentRequest, r=baku

Revise per reviewer's suggestion.

Comment 9

[Ben Tian (inactive)]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85ec39ac7a936c2749481e5cfde5bff9decf1550

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39dc330b3560
Part 1: Set PaymentRequestUpdateEvent.mRequest only when event target is a PaymentRequest. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/10677a3988fc
Part 2: Revise method |updateWith| based on spec change. r=baku

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/39dc330b3560
https://hg.mozilla.org/mozilla-central/rev/10677a3988fc

Comment 13

[Alphan Chen [:alchen]]

Comment on attachment 8886457 [details] [diff] [review]
[final] Patch 2: Revise method |updateWith| based on spec change, r=baku

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

LGTM