Closed
Bug 1483470
Opened 6 years ago
Closed 6 years ago
Navigating during paymentRequest.show() crashes the parent process
Categories
(Core :: DOM: Web Payments, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | unaffected |
firefox61 | --- | unaffected |
firefox62 | --- | unaffected |
firefox63 | --- | verified |
People
(Reporter: pauljt, Assigned: edenchuang)
References
(Blocks 1 open bug)
Details
(Keywords: crash, Whiteboard: [webpayments])
Crash Data
Attachments
(5 files, 10 obsolete files)
1.80 KB,
text/plain
|
Details | |
8.18 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
4.30 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
29.38 KB,
patch
|
edenchuang
:
review+
|
Details | Diff | Splinter Review |
I just started poking at paymentRequest. First thing I tried crashed the parent process :/
1. calling history.forward() then paymentRequest.show() crashes
2. calling paymentRequest.show() then location.href = .... also crashes.
Crash is from the latter. I'll attach a PoC to repro.
If Im reading crashstats correctly this is just a null pointer crash (crash address 0x0, can someone confirm im reading this right?) but I'm marking this sec-sensitive just in case. Note: Payment request is NOT enabled on release, only nightly.
This bug was filed from the Socorro interface and is
report bp-8f492e9c-eeb5-44b2-9fc4-5cb940180815.
=============================================================
Top 10 frames of crashing thread:
0 XUL mozilla::dom::PaymentRequestService::RequestPayment dom/payments/PaymentRequestService.cpp:336
1 XUL mozilla::dom::PaymentRequestParent::RecvRequestPayment dom/payments/ipc/PaymentRequestParent.cpp:167
2 XUL mozilla::dom::PPaymentRequestParent::OnMessageReceived ipc/ipdl/PPaymentRequestParent.cpp:201
3 XUL mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2236
4 XUL mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2166
5 XUL mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:2045
6 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1235
7 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
8 XUL nsXULWindow::ShowModal xpcom/threads/nsThreadUtils.h:324
9 XUL nsWindowWatcher::OpenWindowInternal toolkit/components/windowwatcher/nsWindowWatcher.cpp:1268
=============================================================
Reporter | ||
Updated•6 years ago
|
Component: Downloads Panel → DOM: Web Payments
Product: Firefox → Core
Version: 61 Branch → 63 Branch
Reporter | ||
Comment 1•6 years ago
|
||
This script crashes nightly 63.0a1 (2018-08-14) (64-bit) OSX.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → echuang
Updated•6 years ago
|
Group: core-security → dom-core-security
Updated•6 years ago
|
Priority: -- → P2
Whiteboard: [webpayments-reserve]
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee | ||
Comment 2•6 years ago
|
||
This is a side effect of bug 1408234. I did not consider the situation that sending a response to the cleanup request.
Matt, I would like to add a new method into nsIPaymentUIService
void cleanupPayment(in AString aRequestId)
to notify the UI that the PaymentRequest is not valid anymore. So if UI is showing the dialog, it should be closed.
Flags: needinfo?(MattN+bmo)
Updated•6 years ago
|
Group: dom-core-security
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
(In reply to Eden Chuang[:edenchuang] from comment #2)
> This is a side effect of bug 1408234. I did not consider the situation that
> sending a response to the cleanup request.
>
> Matt, I would like to add a new method into nsIPaymentUIService
>
> void cleanupPayment(in AString aRequestId)
>
> to notify the UI that the PaymentRequest is not valid anymore. So if UI is
> showing the dialog, it should be closed.
How about `closeRequest(in AString aRequestId)` since to me "cleanup" doesn't necessarily imply close and I would think that is more for test cleanup.
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 7•6 years ago
|
||
Sure, no problem, let's use closeRequest().
Comment 8•6 years ago
|
||
Steps to reproduce the crash (manually) with the same signature:
[Prerequisites]:
- set the pref dom.payments.request.enabled to "true"
- make sure to have at least 2 Shipping Address saved on your profile one from US and one outside US (such as DE)
1. Go to "https://rsolomakhin.github.io/pr/us/" and click on "Buy"
2. Select an address outside of US
3. Click on Cancel or "X"
Note: I can only reproduce this issue while the two generic merchant errors are displayed, see also Bug 1481243 (Actual result: Both the "Cannot ship outside the US." and "Can't ship to this address. Select a different address" error strings are displayed, check: https://streamable.com/5mkzl)
Flags: needinfo?(MattN+bmo)
Comment hidden (obsolete) |
Comment 10•6 years ago
|
||
Oh, nevermind, I re-read what you said. Eden, are you going to handle the case from comment 8 too?
Assignee | ||
Comment 11•6 years ago
|
||
Yes, I will handle the case from comment 8 too.
Updated•6 years ago
|
Whiteboard: [webpayments-reserve] → [webpayments]
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #9002325 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years ago
|
||
Attachment #9002326 -
Attachment is obsolete: true
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #9002327 -
Attachment is obsolete: true
Assignee | ||
Comment 15•6 years ago
|
||
Comment 16•6 years ago
|
||
Comment on attachment 9003013 [details] [diff] [review]
Part4 mochitest for the bug
Review of attachment 9003013 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/payments/test/test_closePayment.html
@@ +166,5 @@
> + document.body.appendChild(ifr);
> + });
> + }
> +
> + function teardown() {
I think teardown has the potential to be racy, because it doesn't actually wait for the teardown before finishing. I'd suggest:
```
function teardown() {
return new Promise(resolve => {
gScript.addMessageListener(
"teardown-complete",
function teardownCompleteHandler() {
gScript.removeMessageListener(
"teardown-complete",
teardownCompleteHandler
);
gScript.removeMessageListener("test-fail", testFailHandler);
gScript.removeMessageListener("test-pass", testPassHandler);
gScript.destroy();
SimpleTest.finish();
resolve();
}
);
gScript.sendAsyncMessage("teardown");
});
}
```
Comment 17•6 years ago
|
||
Comment on attachment 9003013 [details] [diff] [review]
Part4 mochitest for the bug
Review of attachment 9003013 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/payments/test/test_closePayment.html
@@ +25,5 @@
> + }
> + gScript.addMessageListener("test-fail", testFailHandler);
> + gScript.addMessageListener("test-pass", testPassHandler);
> +
> + function closeCheck(resolveFunc, respondPayment) {
Passing the resolver is generally not considered best practice - it also introduces a boolean trap. Can I suggest something like:
```JS
async function closeCheck(postAction) {
gScript.sendAsyncMessage("close-check");
await new Promise(resolve => {
gScript.addMessageListener("close-check-complete", function msgListener() {
gScript.removeMessageListener("close-check-complete", msgListener);
resolve();
});
});
if (!postAction) return;
gScript.sendAsyncMessage(postAction);
await new Promise(resolve => {
gScript.addMessageListener(`${postAction}-complete`, function listener() {
gScript.removeMessageListener(`${postAction}-complete`, listener);
resolve();
});
});
}
```
Then you can:
```JS
await closeCheck();
await closeCheck("respond-payment");
```
Assignee | ||
Comment 18•6 years ago
|
||
1. Update the patch according to the comment 16 and comment 17.
2. Adding a new test case for the case mentioned in comment 8.
Attachment #9003013 -
Attachment is obsolete: true
Assignee | ||
Comment 19•6 years ago
|
||
Attach the correct patch
Attachment #9003724 -
Attachment is obsolete: true
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #9003010 -
Attachment is obsolete: true
Attachment #9004433 -
Flags: review?(amarchesini)
Assignee | ||
Comment 21•6 years ago
|
||
Attachment #9003011 -
Attachment is obsolete: true
Attachment #9004434 -
Flags: review?(amarchesini)
Assignee | ||
Comment 22•6 years ago
|
||
Attachment #9003012 -
Attachment is obsolete: true
Attachment #9004435 -
Flags: review?(amarchesini)
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #9003738 -
Attachment is obsolete: true
Attachment #9004436 -
Flags: review?(amarchesini)
Updated•6 years ago
|
Attachment #9004433 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #9004435 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #9004436 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #9004434 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Flags: qe-verify+
QA Contact: hani.yacoub
Assignee | ||
Comment 24•6 years ago
|
||
updated for code base conflict issue.
Attachment #9004436 -
Attachment is obsolete: true
Attachment #9004856 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 25•6 years ago
|
||
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3438a06960a
Handling PaymentRequestService::RespondPayment() to a closed PaymentRequest. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/de6a929297e0
Adding new method closePayment in nsIPaymentUIService.idl. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/13c13a8c4dd3
Handling PaymentRequestUpdateEvent::updateWith() to a responed PaymentRequest. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7292d051a0
Improve the test case for responding a closed PaymentRequest. r=baku
Keywords: checkin-needed
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3438a06960a
https://hg.mozilla.org/mozilla-central/rev/de6a929297e0
https://hg.mozilla.org/mozilla-central/rev/13c13a8c4dd3
https://hg.mozilla.org/mozilla-central/rev/cd7292d051a0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
status-firefox61:
--- → unaffected
status-firefox62:
--- → unaffected
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
Comment 27•6 years ago
|
||
I verified the fix by following the steps I mentioned in Comment 8 on Windows 7/10, Ubuntu 16.04. and MAC OS 10.13. The crash can no longer be reproduced.
Should this be enough to close it as verified - fixed or further, different testing is needed?
Flags: needinfo?(echuang)
Assignee | ||
Comment 28•6 years ago
|
||
I will say more testing would be good, but I am okay if you close it as verified.
BTW, I added a mochitest for the case mentioned in comment 8. So it must be passed. :)
Flags: needinfo?(echuang)
Comment 29•6 years ago
|
||
Paul, can you please check if the crash can be reproduced based on the description from the bug report?
Just to be sure that all the possible scenarios are covered.
Thank you.
Flags: needinfo?(ptheriault)
Reporter | ||
Comment 30•6 years ago
|
||
Opening the script attached in comment 2 no longer crashes the browser. I tested a few other scenarios (window.open, setting location, history.forward) with different timings (setTimeout, async etc) but didn't get any unexpected behavior. I'm not "sure all the possible scenarios are covered" (its negative assurance. so you can never be sure) but I didn't obverse any anomalies in brief re-testing.
Flags: needinfo?(ptheriault)
Comment 31•6 years ago
|
||
Thanks Paul!
Based on Comment 27 and Comment 30 I will mark this issue as Verified - Fixed on all OS.
You need to log in
before you can comment on or make changes to this bug.
Description
•