Navigating during paymentRequest.show() crashes the parent process

VERIFIED FIXED in Firefox 63

Status

()

P1
critical
VERIFIED FIXED
4 months ago
3 months ago

People

(Reporter: pauljt, Assigned: edenchuang)

Tracking

(Blocks: 1 bug, {crash})

63 Branch
mozilla63
Unspecified
Mac OS X
crash
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
(Reporter)

Description

4 months ago
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

4 months ago
Component: Downloads Panel → DOM: Web Payments
Product: Firefox → Core
Version: 61 Branch → 63 Branch
(Reporter)

Comment 1

4 months ago
Created attachment 9000168 [details]
PoC.html

This script crashes nightly 63.0a1 (2018-08-14) (64-bit) OSX.
(Assignee)

Updated

4 months ago
Assignee: nobody → echuang
Group: core-security → dom-core-security
Priority: -- → P2
Whiteboard: [webpayments-reserve]
Status: NEW → ASSIGNED
Priority: P2 → P1
(Reporter)

Updated

4 months ago
Blocks: 1366652
(Assignee)

Comment 2

4 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
(Assignee)

Comment 3

4 months ago
Created attachment 9002325 [details] [diff] [review]
Part1. Handling nsIPaymentRequestService::RespondPayment gracefully when responding a cleanup PaymentReuqest.
(Assignee)

Comment 4

4 months ago
Created attachment 9002326 [details] [diff] [review]
Part2. Adding a new method cleanupPayment() into nsIPaymentUIService to notify UI the specified PaymentRequest is not valid anymore.
(Assignee)

Comment 5

4 months ago
Created attachment 9002327 [details] [diff] [review]
Part3. Improving the testcase to reproduce the bug.
(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

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

Comment 8

4 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

4 months ago
Yes, I will handle the case from comment 8 too.
Whiteboard: [webpayments-reserve] → [webpayments]
(Assignee)

Comment 12

4 months ago
Created attachment 9003010 [details] [diff] [review]
Part1. Handling nsIPaymentRequestService::RespondPayment() to a closed PaymentRequest
Attachment #9002325 - Attachment is obsolete: true
(Assignee)

Comment 13

4 months ago
Created attachment 9003011 [details] [diff] [review]
Part2 Adding a new method closePayment in nsIPaymentUIService.
Attachment #9002326 - Attachment is obsolete: true
(Assignee)

Comment 14

4 months ago
Created attachment 9003012 [details] [diff] [review]
Part3. Handling PaymentRequestUpdateEvent::UpdateWith() to a responded PaymentRequest
Attachment #9002327 - Attachment is obsolete: true
(Assignee)

Comment 15

4 months ago
Created attachment 9003013 [details] [diff] [review]
Part4 mochitest for the bug
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

4 months ago
Created attachment 9003724 [details] [diff] [review]
Part4 mochitest for the bug.

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

4 months ago
Created attachment 9003738 [details] [diff] [review]
Part4 mochitest for the bug

Attach the correct patch
Attachment #9003724 - Attachment is obsolete: true
(Assignee)

Comment 20

4 months ago
Created attachment 9004433 [details] [diff] [review]
Part1. Handling nsIPaymentRequestService::RespondPayment() to a closed PaymentRequest
Attachment #9003010 - Attachment is obsolete: true
Attachment #9004433 - Flags: review?(amarchesini)
(Assignee)

Comment 21

4 months ago
Created attachment 9004434 [details] [diff] [review]
Part2. Adding a new method closePayment in nsIPaymentUIService.
Attachment #9003011 - Attachment is obsolete: true
Attachment #9004434 - Flags: review?(amarchesini)
(Assignee)

Comment 22

4 months ago
Created attachment 9004435 [details] [diff] [review]
Part3. Handling PaymentRequestUpdateEvent::UpdateWith() to a responded PaymentRequest
Attachment #9003012 - Attachment is obsolete: true
Attachment #9004435 - Flags: review?(amarchesini)
(Assignee)

Comment 23

4 months ago
Created attachment 9004436 [details] [diff] [review]
Part4. mochitest for the bug
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+
Flags: qe-verify+
QA Contact: hani.yacoub
(Assignee)

Comment 24

4 months ago
Created attachment 9004856 [details] [diff] [review]
Part4. mochitest for the bug

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

Updated

4 months ago
Keywords: checkin-needed

Comment 25

4 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 26

4 months 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
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
status-firefox61: --- → unaffected
status-firefox62: --- → unaffected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → unaffected

Comment 27

3 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

3 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

3 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)
(Reporter)

Comment 30

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

3 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
status-firefox63: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.