Reject PaymentRequest.show() with AbortError DOMException if the user agent's "payment request is showing" boolean is true

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM: Web Payments
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: btian, Assigned: edenchuang)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla56
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
Web platform test for PaymentRequest.show() fails:
https://w3c-test.org/payment-request/payment-request-show-method.https.html

If the user agent's "payment request is showing" boolean is true, then return a promise rejected with an "AbortError" DOMException.

We should reject with AbortError instead of NS_ERROR_FAILURE.
(Reporter)

Updated

3 months ago
Blocks: 1318987
(Reporter)

Updated

3 months ago
Blocks: 1380546
(Reporter)

Updated

3 months ago
No longer blocks: 1318987
(Assignee)

Updated

3 months ago
Assignee: nobody → echuang
(Assignee)

Comment 1

3 months ago
Created attachment 8888476 [details] [diff] [review]
Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r?baku
Attachment #8888476 - Flags: review?(amarchesini)
Adding ddn, just so we can check whether the docs are correct on this point.
Keywords: dev-doc-needed
Comment on attachment 8888476 [details] [diff] [review]
Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r?baku

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

::: dom/payments/PaymentRequest.cpp
@@ +403,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> +    if (rv == NS_ERROR_ABORT) {
> +      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
> +    } else {
> +      promise->MaybeReject(NS_ERROR_FAILURE);

NS_ERROR_FAILURE is a wrong error. Can we use invalid state? or anything else? What does the spec say here?

::: dom/payments/PaymentRequestService.cpp
@@ +327,5 @@
>        }
>        break;
>      }
>      /*
>       *  TODO: Launch/inform payment UI here once the UI module is implemented.

Is this comment still valid?
Attachment #8888476 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 4

3 months ago
(In reply to Andrea Marchesini [:baku] from comment #3)
> Comment on attachment 8888476 [details] [diff] [review]
> Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a
> request is showing. r?baku
> 
> Review of attachment 8888476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/payments/PaymentRequest.cpp
> @@ +403,5 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    if (rv == NS_ERROR_ABORT) {
> > +      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
> > +    } else {
> > +      promise->MaybeReject(NS_ERROR_FAILURE);
> 
> NS_ERROR_FAILURE is a wrong error. Can we use invalid state? or anything
> else? What does the spec say here?

When PaymentRequestManager::ShowPayment returns NS_ERROR_FAILURE, it can be IPC transfer error or some other internal error.
The spec doesn't describe the situations about internal errors. According to the spec, the state is valid, so returning invalid state error might not the best. I prefer returning NS_ERROR_DOM_NOT_ALLOWED_ERR or NS_ERROR_DOM_UNKNOWN_ERR. How do you think?

> 
> ::: dom/payments/PaymentRequestService.cpp
> @@ +327,5 @@
> >        }
> >        break;
> >      }
> >      /*
> >       *  TODO: Launch/inform payment UI here once the UI module is implemented.
> 
> Is this comment still valid?

Yes, and this comment will be remove after bug 1382092 is resolved.
Flags: needinfo?(amarchesini)
> the spec, the state is valid, so returning invalid state error might not the
> best. I prefer returning NS_ERROR_DOM_NOT_ALLOWED_ERR or
> NS_ERROR_DOM_UNKNOWN_ERR. How do you think?

Better. I prefer NS_ERROR_DOM_NOT_ALLOWED_ERR but this should be covered by spec.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 6

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d359a4ed0e37f6710d85a6a840f8e9ab22865b82
(Assignee)

Comment 7

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eb2199f34042bb4686791125a28a5bdf276321f
(Assignee)

Comment 8

3 months ago
Created attachment 8889805 [details] [diff] [review]
Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r=baku
Attachment #8888476 - Attachment is obsolete: true
Attachment #8889805 - Flags: review+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 9

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd757326f72b
Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r=baku
Keywords: checkin-needed

Comment 10

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd757326f72b
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I've added details of exceptions to this page:

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/show#Exceptions

Let me know if that looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Comment 12

3 months ago
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> show#Exceptions
> 
> Let me know if that looks OK. Thanks!

Correction:

In |AbortError| description, "payment request is showing" boolean is true means _another_ payment has already been shown, not this payment. If this payment is showing, call to show() would be rejected with |InvalidStateError| since the request's state is 'interactive' (i.e., the payment request is being presented to the user), not 'created' [2].

[1] https://w3c.github.io/payment-request/#dfn-payment-request-is-showing
[2] step 3 in https://w3c.github.io/payment-request/#show()-method
(In reply to Ben Tian [:btian] from comment #12)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> > https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> > show#Exceptions
> > 
> > Let me know if that looks OK. Thanks!
> 
> Correction:
> 
> In |AbortError| description, "payment request is showing" boolean is true
> means _another_ payment has already been shown, not this payment. If this
> payment is showing, call to show() would be rejected with
> |InvalidStateError| since the request's state is 'interactive' (i.e., the
> payment request is being presented to the user), not 'created' [2].
> 
> [1] https://w3c.github.io/payment-request/#dfn-payment-request-is-showing
> [2] step 3 in https://w3c.github.io/payment-request/#show()-method

Cool, thanks for the corrections. I've updated the AbortError description, and added an InvalidStateError description.
You need to log in before you can comment on or make changes to this bug.