Navigating during paymentRequest.show() crashes the parent process

VERIFIED FIXED in Firefox 63

Status

()

defect
P1
critical
VERIFIED FIXED
10 months ago
10 months ago

People

(Reporter: pauljt, Assigned: edenchuang)

Tracking

(Blocks 1 bug, {crash})

63 Branch
mozilla63
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63 verified)

Details

(Whiteboard: [webpayments], crash signature)

Attachments

(5 attachments, 10 obsolete attachments)

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

=============================================================
Component: Downloads Panel → DOM: Web Payments
Product: Firefox → Core
Version: 61 Branch → 63 Branch
Posted file PoC.html
This script crashes nightly 63.0a1 (2018-08-14) (64-bit) OSX.
Assignee

Updated

10 months ago
Assignee: nobody → echuang
Group: core-security → dom-core-security
Priority: -- → P2
Whiteboard: [webpayments-reserve]

Updated

10 months ago
Status: NEW → ASSIGNED
Priority: P2 → P1
Blocks: 1366652
Assignee

Comment 2

10 months 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)
Group: dom-core-security
(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

10 months ago
Sure, no problem, let's use closeRequest().

Comment 8

10 months 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)
Oh, nevermind, I re-read what you said. Eden, are you going to handle the case from comment 8 too?
Assignee

Comment 11

10 months ago
Yes, I will handle the case from comment 8 too.

Updated

10 months ago
Whiteboard: [webpayments-reserve] → [webpayments]
Assignee

Comment 13

10 months ago
Attachment #9002326 - Attachment is obsolete: true
Assignee

Comment 15

10 months ago
Posted patch Part4 mochitest for the bug (obsolete) — Splinter Review
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 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

10 months ago
Posted patch Part4 mochitest for the bug. (obsolete) — Splinter Review
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

10 months ago
Posted patch Part4 mochitest for the bug (obsolete) — Splinter Review
Attach the correct patch
Attachment #9003724 - Attachment is obsolete: true
Assignee

Comment 21

10 months ago
Attachment #9003011 - Attachment is obsolete: true
Attachment #9004434 - Flags: review?(amarchesini)
Assignee

Comment 23

10 months ago
Posted patch Part4. mochitest for the bug (obsolete) — Splinter Review
Attachment #9003738 - Attachment is obsolete: true
Attachment #9004436 - Flags: review?(amarchesini)
Attachment #9004433 - Flags: review?(amarchesini) → review+
Attachment #9004435 - Flags: review?(amarchesini) → review+
Attachment #9004436 - Flags: review?(amarchesini) → review+
Attachment #9004434 - Flags: review?(amarchesini) → review+

Updated

10 months ago
Flags: qe-verify+
QA Contact: hani.yacoub
Assignee

Comment 24

10 months ago
updated for code base conflict issue.
Attachment #9004436 - Attachment is obsolete: true
Attachment #9004856 - Flags: review+
Assignee

Updated

10 months ago
Keywords: checkin-needed

Comment 25

10 months 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 27

10 months 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

10 months 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

10 months 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)
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

10 months ago
Thanks Paul!

Based on Comment 27 and Comment 30 I will mark this issue as Verified - Fixed on all OS.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.