Closed Bug 1379925 Opened 2 years ago Closed 2 years ago

Crash while testing PaymentRequest.onshippingaddresschange/onshippingoptionchange

Categories

(Core :: DOM: Web Payments, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ben.tian, Assigned: ben.tian)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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
(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.
Assignee: nobody → btian
The patch adds extra argument |aIsOwnerPaymentRequest| to tell event constructor whether the event target is a payment request or not.
Attachment #8886008 - Flags: review?(amarchesini)
Attachment #8886008 - Flags: feedback?(alchen)
No longer blocks: 1318987
Attachment #8886008 - Flags: feedback?(alchen) → feedback+
Blocks: 1380550
Blocks: 1380552
Blocks: 1380553
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
Attachment #8886008 - Flags: review?(amarchesini)
No longer blocks: 1380552
Change:
Instead of in event constructor, |mRequest| is set by |PaymentRequest::DispatchUpdateEvent| after |SetTrusted|, to allow |IsTrusted| assertion check before event sets |Request|.
Attachment #8886008 - Attachment is obsolete: true
Attachment #8886455 - Flags: review?(amarchesini)
Attachment #8886455 - Flags: feedback?(alchen)
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
Attachment #8886457 - Flags: review?(amarchesini)
Attachment #8886457 - Flags: feedback?(alchen)
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);
Attachment #8886455 - Flags: review?(amarchesini) → review+
Attachment #8886457 - Flags: review?(amarchesini) → review+
Attachment #8886455 - Attachment is obsolete: true
Attachment #8886455 - Flags: feedback?(alchen)
Attachment #8886457 - Attachment description: Patch 2 (v1): Revise method |updateWith| based on spec change → [final] Patch 2: Revise method |updateWith| based on spec change, r=baku
Duplicate of this bug: 1380553
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39dc330b3560
https://hg.mozilla.org/mozilla-central/rev/10677a3988fc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
Attachment #8886457 - Flags: feedback?(alchen) → feedback+
You need to log in before you can comment on or make changes to this bug.