1447773 - Time out the PaymentResponse object after 5 seconds, behaving as if complete() was called with no arguments

Status: RESOLVED FIXED

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

Description

[Jared Wein [:jaws] (please needinfo? me)]

From section 14.10 of the spec[1]:
---------8<--- snip --------------
After the payment request has been accepted and the PaymentResponse returned to the caller but before the caller calls complete() the payment request user interface remains in a pending state. At this point the user interface ought not offer a cancel command because acceptance of the payment request has been returned. However, if something goes wrong and the developer never calls complete() then the user interface is blocked.

For this reason, implementations MAY impose a timeout for developers to call complete(). If the timeout expires then the implementation will behave as if complete() was called with no arguments.

The complete(result) method MUST act as follows:

    Let promise be a new Promise.
    If the value of the internal slot [[completeCalled]] is true, reject promise with an "InvalidStateError" DOMException.
    Otherwise, set the value of the internal slot [[completeCalled]] to true.
    Return promise and perform the remaining steps in parallel.
    Close down any remaining user interface. The user agent MAY use the value result to influence the user experience.
    Resolve promise with undefined.
---------8<--- snip --------------

We should set completeStatus to a value of "timeout" [2] in this case, so that the front-end UI has the ability to behave differently in this case.

[1] https://www.w3.org/TR/payment-request/#dom-paymentresponse-complete
[2] https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/dom/webidl/PaymentResponse.webidl#10

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

The timeout duration should be configurable via a pref.

Comment 2

[Henri Sivonen (:hsivonen) (on vacation)]

Attachment: Bug 1447773 - Time out PaymentResponse after 5 seconds.

Review commit: https://reviewboard.mozilla.org/r/247498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247498/

Comment 3

[Henri Sivonen (:hsivonen) (on vacation)]

The MozReview patch should accomplish a timeout of the number if milliseconds set in dom.payments.response.timeout (defaults to 5000) such that the Web-visible PaymentComplete enum isn't extended but the chrome-visible value set can not show "timeout" when the completion happens due to timeout.

jaws, could you, please, write the kind of test case that you want to see for this to check that the DOM code works? I'm unsure what the test case should look like.

Comment 4

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

I'm going to put together this test case. From the spec I'm not clear if there's anything on the caller side (e.g. merchant page) to indicate a timed-out paymentresponse - that would let me write a web-platform-test style of test case. I can assert that a call to response.complete() should throw an exception after the timeout has expired. Maybe testing for the exception and  a manual instruction to consider the test failed if the payment UI doesnt close after waiting n seconds would work? 

ni myself to finish & attach this test case.

Comment 5

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

Attachment: wpt/payment-request/payment-response/complete-method-timeout.https.html

I've used the wpt framework, so if you drop this file into payment-request/payment-response you can run it as https://web-platform.test:8443/payment-request/payment-response/complete-method-timeout.https.html

It is expected to fail right now, as calling complete() after 5+ seconds does not currently throw a InvalidStateError exception. 

I'm not expecting to make a wpt PR out of this as the timeout is optional and the max delay will be variable so I'm not sure a cross-browser test is practical? But hopefully this format is somewhat useful as a test case. 

Let me know if there's anything else I can do here.

Comment 6

[Marion Daly [:mdaly]]

P1, hasn't been updated in 11 days, open attachment on bug. Can we get an update on this?

Comment 7

[Henri Sivonen (:hsivonen) (on vacation)]

Comment on attachment 8981369 [details]
Bug 1447773 - Time out PaymentResponse after 5 seconds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247498/diff/1-2/

Comment 8

[Henri Sivonen (:hsivonen) (on vacation)]

(In reply to Sam Foster [:sfoster] from comment #5)
> I've used the wpt framework, so if you drop this file into
> payment-request/payment-response you can run it as
> https://web-platform.test:8443/payment-request/payment-response/complete-
> method-timeout.https.html

Thank you. I've verified that the rebased patch that I just uploaded passes this manual test.

> Let me know if there's anything else I can do here.

Do you think it would be feasible to write a mochitest that sets the timeout pref to a shorter time value and uses automation to click the "Pay" button? Or should this just land without a test?

Comment 9

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

(In reply to Henri Sivonen (:hsivonen) from comment #8)
> Do you think it would be feasible to write a mochitest that sets the timeout
> pref to a shorter time value and uses automation to click the "Pay" button?
> Or should this just land without a test?

Verifying the outcome of clicking "Pay" button and exceeding the timeout before complete() is called - that something we can test in the browser mochitests. It probably doesnt make sense to tightly couple a DOM patch to landing a browser test. Is there no way to get a paymentresponse into this state and assert on its behavior without interacting with the UI? If the answer is "no", then I'm in favor of landing here without a test.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8981369 [details]
Bug 1447773 - Time out PaymentResponse after 5 seconds.

https://reviewboard.mozilla.org/r/247498/#review259440

I have to admit that it feels a bit odd to me to have this in DOM code instead of frontend code (with a race to complete the timeout resolved by rejecting the promise), but I don't feel terribly strongly about that. I do wonder if it wouldn't be useful for web sites to have a way to limit the timeout (up to our max limit of whatever we decide) via the payment request details or something like that.

I'm pretty worried that by simply canceling the request without first informing the page, we're setting ourselves up for cases where the charge goes through (very slowly) as we display a timeout error to the user, potentially resulting in two charges to the payment method (assuming the user ends up retrying). Is this something that we don't have to worry about at this point in the API? Clearing the review request until that is addressed.

::: dom/payments/PaymentResponse.cpp:25
(Diff revision 2)
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(PaymentResponse)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(PaymentResponse)
>  
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(PaymentResponse)
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> -  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)

We still need an entry for `nsISupports` here.

::: dom/payments/PaymentResponse.cpp:147
(Diff revision 2)
>      return nullptr;
>    }
>  
>    nsIGlobalObject* global = mOwner->AsGlobal();
>    ErrorResult errResult;
>    RefPtr<Promise> promise = Promise::Create(global, errResult);

While you're here, the bindings layer already converts exceptions to promises if this function returns an error through aRv, so it would be more consistent to wait until after successfully call `PaymentRequestManager::CompletePayment` before creating the promise.

Then the failure case there could be simply `aRv.Throw(NS_ERROR_FAILURE); return nullptr;` like all of the rest of the error paths.

::: dom/payments/PaymentResponse.cpp:182
(Diff revision 2)
>  {
> -  MOZ_ASSERT(mPromise);
> -
> +  // mPromise may be null when timing out
> +  if (mPromise) {
> -  mPromise->MaybeResolve(JS::UndefinedHandleValue);
> +    mPromise->MaybeResolve(JS::UndefinedHandleValue);
> +  }
>    mPromise = nullptr;

Nit: might as well put this assignment in the if statement, since we only need to do work if `mPromise` was non-null.

Comment 11

[Henri Sivonen (:hsivonen) (on vacation)]

Comment on attachment 8981369 [details]
Bug 1447773 - Time out PaymentResponse after 5 seconds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247498/diff/2-3/

Comment 12

[Henri Sivonen (:hsivonen) (on vacation)]

(In reply to Blake Kaplan (:mrbkap) from comment #10)
> I'm pretty worried that by simply canceling the request without first
> informing the page, we're setting ourselves up for cases where the charge
> goes through (very slowly) as we display a timeout error to the user,
> potentially resulting in two charges to the payment method (assuming the
> user ends up retrying). Is this something that we don't have to worry about
> at this point in the API?

That does seem bad. It would be good if Marcos and/or the front end team could comment if this patch is still wanted.

I uploaded an amended changeset that addresses the other comments.

Comment 13

[Marcos Caceres [:marcosc]]

> I do wonder if it wouldn't be useful for web sites to have a way to limit the timeout (up to our max limit of whatever we decide) via the payment request details or something like that.

So, `.retry()` will put the PaymentRequest instance back into the "interactive" state. So, the site can call `request.abort()` when "retrying", which will also invalidate the payment `response` (i.e., the site will have an escape hatch, and they can use `setTimeout()` to control it imperatively).

> Is this something that we don't have to worry about at this point in the API?

Technically, I don't think this is really our problem - as it's the site that is handling the transaction. If the sheet goes down, the site can still display a success or error message.  

Imagine:

```
const response = await request.show();
// sheet is waiting for complete
const didPaySucceed = await fetch("/pay", {method: "POST", body: response.toJSON()});
//... bit's taking ages... so sheet shuts down or user hits "ESC"! 
// finally, we get "didPaySucceed" result.
try {
  // oh crap, invalid state error! The sheet was closed! 
  await request.complete("success");
} catch(err){
  // site: we got this, show success or error.
  displayCompleteResult(didPaySucceed); 
}
``` 

Put differently, the Payment Sheet is a nice to have UI... but it can't control the sites payment speed.

A strategy for sites might be:

```
const response = await request.show();
const controller = new AbortController();
const signal = controller.signal;
const payPromise = fetch("/pay", {
  method: "POST",
  body: response.toJSON(),
  signal,
});
const timeBomb = new Promise((_, reject) => setTimeout(()=>{reject(new Error("timeout"))}, 5000));
try {
  await Promise.race([payPromise, timeBomb])
} catch(err){
  if(err.message === "timeout") {
    // taking too long
    controller.abort();
    // could .retry() here... for instance
    await response.complete("error");
  } else {
   // fetch error...
   response.retry({error: "Try again?"})
  }
}
```

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8981369 [details]
Bug 1447773 - Time out PaymentResponse after 5 seconds.

https://reviewboard.mozilla.org/r/247498/#review259766

r=me based on Marcos' comment. I think we'll have to be very careful in what UI we show (if any) for the timeout case, since we really don't know at any point if the payment succeeded or failed. On IRC, I suggested maybe adding an ontimeout handler on the PaymentResponse so the page would at least be able to react to the timeout in some way.

Comment 15

[Marcos Caceres [:marcosc]]

Filed a bug on the spec:
https://github.com/w3c/payment-request/issues/737

tl;dr: we might need a `response.ontimeout` event to make this more sane.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f3f19b029f7
Time out PaymentResponse after 5 seconds. r=mrbkap

Comment 17

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/4f3f19b029f7