Closed Bug 1442453 Opened 7 years ago Closed 6 years ago

A TypeError is thrown and the payment service can get in a broken state if two requests are used in one document

Categories

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

Product:
Core
Component:
DOM: Web Payments
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: MattN, Assigned: mrbkap)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase, Whiteboard: [webpayments])

Attachments

(6 files)

Test case with one.canMakePayment => one.show => one.canMakePayment
1.82 KB, text/html
Details
Bug 1442453 - Don't enforce "one interactive request" in the wrong place.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1442453 - Fix a typo in this member name.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1442453 - Fix nits and get rid of needless QIs.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1442453 - Create a single IPC actor per PaymentRequest.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1442453 - Pass objects around instead of string IDs in the child.
59 bytes, text/x-review-board-request
baku
: review+
Details 
There are issues related to two PaymentRequests being used in the same document even if only one is shown. Internally we get an NS_ERROR_FAILURE inside PaymentRequestManager::GetPaymentChild[1] after a comment saying:
> // Only one payment request can interact with user at the same time.
but IIUC, the code isn't actually checking if the existing request is interacting with the user. In the case of my test case, one is interacting with the user (via .show) and the other is just a .canMakePayment() call.

STR:
In the attached test case:
1. Click Buy
2. Click Cancel in the dialog

Expected result:
> INFO: Can make payment
> INFO: Can make payment
> ERROR: AbortError: The operation was aborted.

Actual result:
> ERROR: TypeError: The expression cannot be converted…
> INFO: Can make payment
The dialog UI is now in a deadlock

There are a few issues:
A) It's confusing that we assume all internal errors in this path should be exposed as a TypeError. This adds confusion about whether this is an internal error or we are implementing some spec algorithm. I would suggest we use a different error or exception.[2]
B) AFAICT, we shouldn't be throwing an error in this case since only one request is interacting with the user. Even when we want to throw for two requests needing to show in the same tab we should use an AbortError to match what the `show` algorithm defines for showing multiple requests.
** Note that we want to eventually intentionally deviate from the spec and allow multiple requests dialogs at the same time in different tabs.
C) The user is not able to abort the shown request dialog because of an NS_ERROR_FAILURE from nsIPaymentRequestService.respondPayment

Side note: The test case uses a canMakePayment while a request is showing which is a bit unusual but I don't know of a specific reason to forbid this currently.

Marking as P1 as this breaks end-to-end testing with various demo sites such as https://rsolomakhin.github.io/pr/ko/fail/

Chrome handles this as expected.

[1] https://dxr.mozilla.org/mozilla-central/rev/b996cabc7ef54bbe050d647494bf00d668ec52e6/dom/payments/PaymentRequestManager.cpp#270
[2] https://dxr.mozilla.org/mozilla-central/rev/b996cabc7ef54bbe050d647494bf00d668ec52e6/dom/payments/PaymentRequest.cpp#594-595
[3] https://w3c.github.io/payment-request/#show-method
Flags: in-testsuite?
Priority: P1 → P2
Whiteboard: [webpayments]
I'm looking into this.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Priority: P2 → P1
I think I know what's going on here and will try to have a patch tomorrow.
There are at least two different bugs found by the testcase here (also, if I try the testcase in one tab, open it again in another tab in a different process, and run it again, that tab crashes).

The first is that we refuse to create a PaymentRequest if another payment is communicating with the parent process. In this case, the first payment is waiting for `show()` to return, but I don't see any reason that `canMakePayment()` wouldn't trigger the same problem.

The second is that the `canMakePayment()` and `show()` calls race with each other. For each request from the child, we try to create a PaymentRequestChild to communicate with the parent. We hold onto each PaymentRequestChild until we get its response from the parent (except for the CreateRequest message, which doesn't expect one). However, the map that allows us to get back to the proper PaymentRequestChild from the IPC message from the parent is only keyed by the PaymentRequest. In order to protect against having to have multiple entries in the hashtable, we'll occasionally reuse a PaymentRequestChild, though none of the code expects that, meaning the second message (which reuses the object) will be unable to process the response. This is ultimately the cause of the testcase in this bug not working.

I'm leery of the code as written. IPDL, for all of its faults, excels at creating reusable channels for communications between processes. It feels the code goes out of its way to not use that feature. In addition, creating a PaymentRequest, calling `canMakePayment()` and doing nothing else will cause at least 6 IPC messages. Also, because we only ever hold onto strings, we're constantly hitting hashtables to find objects (with some of them backwards, causing us to have to linearly search the hashtable to get to the object we want). IPDL already maintains most of this info for us, so I don't see the need to not use it.

I have a somewhat large patch that I've pushed to try at [1] that fixes the testcase here, establishes a 1:1 relationship between PaymentRequest and PaymentRequestChild objects, and makes the code in the child use objects instead of strings. I like the result.

Baku, are you opposed to this? This could also be fixed in other ways but this seems much more clear to me than the existing code.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b6f36df25671011023140cad7ac452a18a36a1
Flags: needinfo?(amarchesini)
> Baku, are you opposed to this? This could also be fixed in other ways but
> this seems much more clear to me than the existing code.

Absolutely not. If you patch is ready, or if you want somebody to finish it, let me know.
Flags: needinfo?(amarchesini)
I've done some additional cleanup as part of this patch, but there's more simplification to be had here. In particular, we can de-COM-ify a bunch of code, avoiding going through do_CreateInstance in favor of simple 'new' and avoiding unnecessary refcounting. I'll file a followup to look into that after this bug.
Comment on attachment 8983192 [details]
Bug 1442453 - Don't enforce "one interactive request" in the wrong place.

https://reviewboard.mozilla.org/r/249044/#review257762

Do we have tests to covert this change?
Attachment #8983192 - Flags: review?(amarchesini) → review+
Comment on attachment 8983193 [details]
Bug 1442453 - Fix a typo in this member name.

https://reviewboard.mozilla.org/r/249046/#review257764
Attachment #8983193 - Flags: review?(amarchesini) → review+
Comment on attachment 8983194 [details]
Bug 1442453 - Fix nits and get rid of needless QIs.

https://reviewboard.mozilla.org/r/249048/#review257768
Attachment #8983194 - Flags: review?(amarchesini) → review+
Comment on attachment 8983196 [details]
Bug 1442453 - Pass objects around instead of string IDs in the child.

https://reviewboard.mozilla.org/r/249052/#review257770
Attachment #8983196 - Flags: review?(amarchesini) → review+
Comment on attachment 8983195 [details]
Bug 1442453 - Create a single IPC actor per PaymentRequest.

https://reviewboard.mozilla.org/r/249050/#review257772

::: dom/payments/PaymentRequestManager.cpp:316
(Diff revision 1)
> +PaymentRequestManager::NotifyRequestDone(PaymentRequest* aRequest)
> +{
> +  auto entry = mActivePayments.Lookup(aRequest);
> +  MOZ_ASSERT(entry);
> +
> +  int32_t count = --entry.Data();

uint32_t
plus, add a MOZ_ASSERT(count > 0)
Attachment #8983195 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #11)
> Comment on attachment 8983192 [details]
> Bug 1442453 - Don't enforce "one interactive request" in the wrong place.
> Do we have tests to covert this change?

We don't! I was surprised that this passed all of the existing tests when I wrote this. Part of the reason for the lack of tests is that we don't actually know what the correct behavior is going to be here (the spec, as I understand it, is in a bit of limbo at this point).
Blocks: 1469419
I filed bug 1469419 on comment 16.
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cea1719a866e
Don't enforce "one interactive request" in the wrong place. r=baku
https://hg.mozilla.org/integration/autoland/rev/a509c5193aaa
Fix a typo in this member name. r=baku
https://hg.mozilla.org/integration/autoland/rev/4be750b97241
Fix nits and get rid of needless QIs. r=baku
https://hg.mozilla.org/integration/autoland/rev/057eef48e70b
Create a single IPC actor per PaymentRequest. r=baku
https://hg.mozilla.org/integration/autoland/rev/df705bf3c62f
Pass objects around instead of string IDs in the child. r=baku
I noticed a couple of assertions and had to re-introduce a couple of QIs.
 Have checked with Firefox 62.0b8, 63.0a1 (2018-07-15) on win 10x64,and 62.0b8 on Ubuntu 16.04LTS.
Getting the following results:

ERROR: ReferenceError: PaymentRequest is not defined 
ERROR: TypeError: request is undefined 
ERROR: ReferenceError: PaymentRequest is not defined
Flags: needinfo?(mrbkap)
You probably didn't enable the feature with: dom.payments.request.enabled=true
Flags: needinfo?(mrbkap) → needinfo?(cristian.fogel)
That helped out with Nightly.
However, for 62.0b8 the behavior is still the same even with the pref set on true.
Flags: needinfo?(cristian.fogel)
(In reply to Cristi Fogel [:cfogel] from comment #28)
> That helped out with Nightly.
> However, for 62.0b8 the behavior is still the same even with the pref set on
> true.

This feature is disabled on beta (the pref won't have any effect there).
Flags: needinfo?(cristian.fogel)
(In reply to Blake Kaplan (:mrbkap) from comment #29)

> This feature is disabled on beta (the pref won't have any effect there).

Thank you for the reply.
Based on your previous comment, clearing the qe flag since the feature is disabled on beta.
Flags: qe-verify+
Flags: needinfo?(cristian.fogel)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: