1483470 - Navigating during paymentRequest.show() crashes the parent process

Status: VERIFIED FIXED

(Core :: DOM: Web Payments, defect, P1)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

=============================================================

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Attachment: PoC.html

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

Comment 2

[Eden Chuang[:edenchuang]]

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.

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Part1. Handling nsIPaymentRequestService::RespondPayment gracefully when responding a cleanup PaymentReuqest.

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Part2. Adding a new method cleanupPayment() into nsIPaymentUIService to notify UI the specified PaymentRequest is not valid anymore.

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Part3. Improving the testcase to reproduce the bug.

Comment 6

[Matthew N. [:MattN]]

(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.

Comment 7

[Eden Chuang[:edenchuang]]

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

Comment 8

[Timea Cernea [:tbabos][inactive]]

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)

Comment 9

[Matthew N. [:MattN]]

Timea, I think your comment is in the wrong bug?

Comment 10

[Matthew N. [:MattN]]

Oh, nevermind, I re-read what you said. Eden, are you going to handle the case from comment 8 too?

Comment 11

[Eden Chuang[:edenchuang]]

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

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: Part1. Handling nsIPaymentRequestService::RespondPayment() to a closed PaymentRequest

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Part2 Adding a new method closePayment in nsIPaymentUIService.

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Part3. Handling PaymentRequestUpdateEvent::UpdateWith() to a responded PaymentRequest

Comment 15

[Eden Chuang[:edenchuang]]

Attachment: Part4 mochitest for the bug

Comment 16

[Marcos Caceres [:marcosc]]

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

[Marcos Caceres [:marcosc]]

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");
```

Comment 18

[Eden Chuang[:edenchuang]]

Attachment: 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.

Comment 19

[Eden Chuang[:edenchuang]]

Attachment: Part4 mochitest for the bug

Attach the correct patch

Comment 20

[Eden Chuang[:edenchuang]]

Attachment: Part1. Handling nsIPaymentRequestService::RespondPayment() to a closed PaymentRequest

Comment 21

[Eden Chuang[:edenchuang]]

Attachment: Part2. Adding a new method closePayment in nsIPaymentUIService.

Comment 22

[Eden Chuang[:edenchuang]]

Attachment: Part3. Handling PaymentRequestUpdateEvent::UpdateWith() to a responded PaymentRequest

Comment 23

[Eden Chuang[:edenchuang]]

Attachment: Part4. mochitest for the bug

Comment 24

[Eden Chuang[:edenchuang]]

Attachment: Part4. mochitest for the bug

updated for code base conflict issue.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Arthur Iakab [arthur_iakab]]

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

Comment 27

[Timea Cernea [:tbabos][inactive]]

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?

Comment 28

[Eden Chuang[:edenchuang]]

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. :)

Comment 29

[Timea Cernea [:tbabos][inactive]]

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.

Comment 30

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 31

[Timea Cernea [:tbabos][inactive]]

Thanks Paul!

Based on Comment 27 and Comment 30 I will mark this issue as Verified - Fixed on all OS.