Matthew N. [:MattN] Reporter
	Description • 7 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 1 • 6 years ago

I'm looking into this.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 2 • 6 years ago

I think I know what's going on here and will try to have a patch tomorrow.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 3 • 6 years ago

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

	Andrea Marchesini [:baku]
	Comment 4 • 6 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 10 • 6 years ago

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.

	Andrea Marchesini [:baku]
	Comment 11 • 6 years ago
mozreview-review

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?

	Andrea Marchesini [:baku]
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8983193 [details]
Bug 1442453 - Fix a typo in this member name.

https://reviewboard.mozilla.org/r/249046/#review257764

	Andrea Marchesini [:baku]
	Comment 13 • 6 years ago
mozreview-review

Comment on attachment 8983194 [details]
Bug 1442453 - Fix nits and get rid of needless QIs.

https://reviewboard.mozilla.org/r/249048/#review257768

	Andrea Marchesini [:baku]
	Comment 14 • 6 years ago
mozreview-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

	Andrea Marchesini [:baku]
	Comment 15 • 6 years ago
mozreview-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)

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 16 • 6 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 17 • 6 years ago

I filed bug 1469419 on comment 16.

	Pulsebot
	Comment 23 • 6 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 24 • 6 years ago

I noticed a couple of assertions and had to re-introduce a couple of QIs.

	Raul Gurzau (:RaulG)
	Comment 25 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cea1719a866e
https://hg.mozilla.org/mozilla-central/rev/a509c5193aaa
https://hg.mozilla.org/mozilla-central/rev/4be750b97241
https://hg.mozilla.org/mozilla-central/rev/057eef48e70b
https://hg.mozilla.org/mozilla-central/rev/df705bf3c62f

	Cristian Fogel [:cfogel]
	Comment 26 • 6 years ago

 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

	Matthew N. [:MattN] Reporter
	Comment 27 • 6 years ago

You probably didn't enable the feature with: dom.payments.request.enabled=true

	Cristian Fogel [:cfogel]
	Comment 28 • 6 years ago

That helped out with Nightly.
However, for 62.0b8 the behavior is still the same even with the pref set on true.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 29 • 6 years ago

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

	Cristian Fogel [:cfogel]
	Comment 30 • 6 years ago

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