1478740 - Showing a 2nd payment request raises DOMException, "The operation was aborted".

Status: VERIFIED FIXED

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

Description

[Sam Foster [:sfoster] (he/him)]

STR: 
1. Load https://paymentrequest.show/demo/ and click the "Buy Now" button in the 1st window to show the payments UI. 
2. Open a new browser window, and load https://paymentrequest.show/demo/ and click the "Buy Now"

Expected: 
Payments UI dialog pops up for both the 1st and 2nd window. Both payments can be completed by clicking "Pay". 

Actual: 
The payments dialog does not open for the 2nd window. An DOM exception is logged "The operation was aborted". 

Note that if you click "Buy Now" a second time in the 2nd window, the payments dialog is actually opened.

Comment 1

[Matthew N. [:MattN]]

This may end up being a dupe of bug 1469419 or related to that.

Comment 2

[Marion Daly [:mdaly]]

Eden, on your radar.

Comment 3

[Eden Chuang[:edenchuang]]

The current implementation is according to the spec https://www.w3.org/TR/payment-request/#show-method.

I capture the description from the spec.
 
> It is not possible to show multiple PaymentRequests at the same time within one user agent. If a PaymentRequest is already
> showing, calling show() —from any Web site— will return a promise rejected with an "AbortError" DOMException.

However, it doesn't detailly describe what one user agent is.

I think this bug is a case of bug 1469419.
Here we might need to consider lots of situations for multiple PaymentRequest.show() is called. At least, we need to consider followings

1. multiple PaymentRequest.show() from different tabs.
2. multiple PaymentRequest.show() from different windows.

Whatever our solution is, I think multiple PaymentRequest.show() at the same time from the same tab should be rejected with "AbortError" DOMException which fits the spec mentioned.

Matt, Marcos, how do you think?

Comment 4

[Matthew N. [:MattN]]

(In reply to Eden Chuang[:edenchuang] from comment #3)
> The current implementation is according to the spec
> https://www.w3.org/TR/payment-request/#show-method.

Well, it doesn't actually work properly IIUC, see bug 1469419.

> I capture the description from the spec.
>  
> > It is not possible to show multiple PaymentRequests at the same time within one user agent. If a PaymentRequest is already
> > showing, calling show() —from any Web site— will return a promise rejected with an "AbortError" DOMException.
> 
> However, it doesn't detailly describe what one user agent is.

Right, we have always planned to intentionally violate that. See https://mozilla.invisionapp.com/share/YAFI31XR3KP#/screens/275361837 (which hasn't changed much from Juwei's version).

> I think this bug is a case of bug 1469419.

Right, I thought it probably was too so feel free to dupe it if you think so.

> Here we might need to consider lots of situations for multiple
> PaymentRequest.show() is called. At least, we need to consider followings
> 
> 1. multiple PaymentRequest.show() from different tabs.

We ideally want to support this for the MVP but we don't have a tab-modal widget yet (bug 1433047) and I'm not sure if that will end up in scope or not.

> 2. multiple PaymentRequest.show() from different windows.

We definitely want this for the MVP and ideally M3.

> Whatever our solution is, I think multiple PaymentRequest.show() at the same
> time from the same tab should be rejected with "AbortError" DOMException
> which fits the spec mentioned.

I agree.

Comment 5

[Sam Foster [:sfoster] (he/him)]

One more thing from comment #0, currently after creating and showing Payment Request while one is already visible in another window, we get the DOMException as described. But if we create another PaymentRequest and show it directly afterwards - same conditions, same payment request already showing in another window - it works. ISTM either we should always throw that DOMException when a payment request is showing already, or we should never throw it. Throwing then not throwing sounds like a bug to me.

Comment 6

[Marcos Caceres [:marcosc]]

> However, it doesn't detailly describe what one user agent is.

"User Agent" is the whole browser (every window and every tab) - that is, to spec, there MUST only be 1 payment sheet ever shown at any one time.

Now, as we have discussed previously, we may choose to relax that or change the behavior. 

For example:

 1. site A shows a payment sheet. 
 2. site B calls .show().
 3. site A's payment sheet "aborts".
 4. site B's payment sheet is shown.

The above is how Safari works (and I think that will become important for Payment Handlers that require second factor authentication through biometrics). 

Personally, following Safari would be my preference... but I'm totally open to us experimenting and figuring out what is best for users.

Comment 7

[Eden Chuang[:edenchuang]]

Before we have a perfect solution for multi-tabs and multi-windows, I would like to make the implementation fits the spec.

That means we have only one showing PaymentRequest at the same time for all cases.

Then a following bug for the perfect solution once bug 1433047 and other possible issues are resolved.

Comment 8

[Marcos Caceres [:marcosc]]

That sounds good to me.

Comment 9

[Matthew N. [:MattN]]

(In reply to Eden Chuang[:edenchuang] from comment #7)
> Before we have a perfect solution for multi-tabs and multi-windows, I would
> like to make the implementation fits the spec.
> 
> That means we have only one showing PaymentRequest at the same time for all
> cases.

I guess that's fine if it's not going to require a lot of effort to revert later.

> Then a following bug for the perfect solution once bug 1433047 and other
> possible issues are resolved.

In the meantime we are currently window modal though FYI.

Comment 10

[Matthew N. [:MattN]]

And please take Fission into account when enforcing this.

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Part 1 - Fix the problem when clearing the PaymentRequestService::mShowingRequest.

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: Part 2 - Remove PaymentRequestManager::mShowingRequest.

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Part 1 - Fix the bug when clearing PaymentRequestService::mShowingRequest.

1. Adding a checking before clearing PaymentRequestService::mShowingRequest to make sure it is cleared when rejecting the showing one.

2. Adding a new method 

    nsresult ShowPayment(const nsAString& aRequestId);

   to reuse showing payment code in updating payment.

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Part 2 - Remove PaymentRequestManager::mShowingRequest.

Removing the PaymentRequestManager::mShowingRequest and related codes.
PaymentRequest showing is controlled by PaymentRequestService::mShowingRequest in the chrome process. it makes sure there is only one showing request at the same time. Before we come out the solution for bug 1469419, we needn't showing control in the content process.

Comment 15

[Eden Chuang[:edenchuang]]

Attachment: Part 3 - Instead of throwing errors directly, rejecting the promise with errors when using PaymentRequest APIs.

Change the behavior to match the spec.

Comment 16

[Eden Chuang[:edenchuang]]

Attachment: Part 4 - mochitest for the bug

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 9010629 [details] [diff] [review]
Part 3 - Instead of throwing errors directly, rejecting the promise with errors when using PaymentRequest APIs.

Review of attachment 9010629 [details] [diff] [review]:
-----------------------------------------------------------------

This is not needed. WebIDL binding does this for you. Do you really see exceptions instead of promise rejection?

Comment 18

[Eden Chuang[:edenchuang]]

(In reply to Andrea Marchesini [:baku] from comment #17)
> Comment on attachment 9010629 [details] [diff] [review]
> Part 3 - Instead of throwing errors directly, rejecting the promise with
> errors when using PaymentRequest APIs.
> 
> Review of attachment 9010629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is not needed. WebIDL binding does this for you. Do you really see
> exceptions instead of promise rejection?

No exceptions now.

Meet some problems a long time ago for the case

let request = new PaymentRequest(...);
request.show().then((result) => {...}, (rejectResult) => {...}).catch(err) {...};
It goes into the catch(err) case, not the reject callback.

But I cannot reproduce it anymore.

Comment 19

[Eden Chuang[:edenchuang]]

Comment on attachment 9010629 [details] [diff] [review]
Part 3 - Instead of throwing errors directly, rejecting the promise with errors when using PaymentRequest APIs.

Remove the non-necessary patch

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2cdfa925504
Part 1 - Fix the bug when clearing PaymentRequestService::mShowingRequest. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b377d15afd7
Part 2 - remove PaymentRequestManager::mShowingRequest. r=baku.
https://hg.mozilla.org/integration/mozilla-inbound/rev/34606bea674b
mochitest for multiple PaymentRequest.show(). r= baku

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e2cdfa925504
https://hg.mozilla.org/mozilla-central/rev/9b377d15afd7
https://hg.mozilla.org/mozilla-central/rev/34606bea674b

Comment 22

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 64.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.