Closed Bug 1483470 Opened 2 years ago Closed 2 years ago

Navigating during paymentRequest.show() crashes the parent process

Categories

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

63 Branch
Unspecified
macOS
defect

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

=============================================================
Component: Downloads Panel → DOM: Web Payments
Product: Firefox → Core
Version: 61 Branch → 63 Branch
Attached file PoC.html
This script crashes nightly 63.0a1 (2018-08-14) (64-bit) OSX.
Assignee: nobody → echuang
Group: core-security → dom-core-security
Priority: -- → P2
Whiteboard: [webpayments-reserve]
Status: NEW → ASSIGNED
Priority: P2 → P1
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)
Blocks: 1435878
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)
Sure, no problem, let's use closeRequest().
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)
Oh, nevermind, I re-read what you said. Eden, are you going to handle the case from comment 8 too?
Yes, I will handle the case from comment 8 too.
Whiteboard: [webpayments-reserve] → [webpayments]
Attachment #9002326 - Attachment is obsolete: true
Attached 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");
```
Attached 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
Attached patch Part4 mochitest for the bug (obsolete) — Splinter Review
Attach the correct patch
Attachment #9003724 - Attachment is obsolete: true
Attachment #9003011 - Attachment is obsolete: true
Attachment #9004434 - Flags: review?(amarchesini)
Attached 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+
Flags: qe-verify+
QA Contact: hani.yacoub
updated for code base conflict issue.
Attachment #9004436 - Attachment is obsolete: true
Attachment #9004856 - Flags: review+
Keywords: checkin-needed
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
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)
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)
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)
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.