Closed Bug 1478740 Opened 2 years ago Closed 1 year ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox64 --- verified

People

(Reporter: sfoster, Assigned: edenchuang)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [webpayments-reserve])

Attachments

(3 files, 3 obsolete files)

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.
Blocks: 1478750
Whiteboard: [webpayments] [triage]
Flags: qe-verify+
QA Contact: hani.yacoub
This may end up being a dupe of bug 1469419 or related to that.
Depends on: 1469419
Priority: -- → P2
Eden, on your radar.
Flags: needinfo?(echuang)
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?
Assignee: nobody → echuang
Flags: needinfo?(mcaceres)
Flags: needinfo?(echuang)
Flags: needinfo?(MattN+bmo)
(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.
Flags: needinfo?(MattN+bmo)
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.
> 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.
Flags: needinfo?(mcaceres)
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.
That sounds good to me.
(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.
And please take Fission into account when enforcing this.
Whiteboard: [webpayments] [triage] → [webpayments]
Whiteboard: [webpayments] → [webpayments-reserve]
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [webpayments-reserve] → [webpayments]
Whiteboard: [webpayments] → [webpayments-reserve]
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.
Attachment #9008061 - Attachment is obsolete: true
Attachment #9010627 - Flags: review?(amarchesini)
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.
Attachment #9008063 - Attachment is obsolete: true
Attachment #9010628 - Flags: review?(amarchesini)
Change the behavior to match the spec.
Attachment #9010629 - Flags: review?(amarchesini)
Attachment #9010630 - Flags: review?(amarchesini)
Attachment #9010627 - Flags: review?(amarchesini) → review+
Attachment #9010628 - Flags: review?(amarchesini) → review+
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?
Attachment #9010629 - Flags: review?(amarchesini) → review-
Attachment #9010630 - Flags: review?(amarchesini) → review+
(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 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
Attachment #9010629 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by shindli@mozilla.com:
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
Keywords: checkin-needed
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.