1469433 - Audit PaymentRequest and related APIs for incorrect eager exceptions

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I realized while reading the Payments spec that there are at least a few places in PaymentRequest's implementation where we eagerly throw an exception where the spec says we should return a promise rejected with an error. This needs to be fixed before we ask authors to start using these APIs because a synchronous exception has very different characteristics from the caller's perspective than asynchronous errors.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

I have a patch to fix this. Worryingly to me, there were no changes in the tests. I found a manual test, but it wasn't very clear how to run it. I managed to make it run and found that it didn't work in Firefox, but fixing the test made us pass it.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(I also threw in a couple of cleanups)

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1469433 - Match the spec's error reporting more closely.

The spec is very precise about how to report exceptional situations to the
caller and it's important to get this right. Failed promises don't stop the
caller from running and allow us more leeway as to where we compute the
exceptional circumstance (e.g. can we make a roundtrip to the parent before
reporting the error). This makes us match the spec.

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

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1469433 - Handle shutdown more gracefully.

If we end up shutting down the content process while there's a request active
we'll end up "leaking" the request and asserting. This makes sure that we
let go of the PaymentRequest in that edge case. I can cause this by forceably
closing the payment request window on Linux without hitting OK or cancel.

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

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1469433 - Remove an unused function.

This was just sitting around.

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

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Olli, please see the spec at [1] (and the other functions on PaymentRequest). Feel free to ask questions or for clarifications.

[1] https://w3c.github.io/payment-request/#show-method

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8986508 [details]
Bug 1469433 - Match the spec's error reporting more closely.

https://reviewboard.mozilla.org/r/251832/#review258326

::: dom/payments/PaymentRequest.cpp:655
(Diff revision 1)
>      aRv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }
>  
> -  RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
> -  if (NS_WARN_IF(!manager)) {
> +  if (mState != eCreated) {
> +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);

I don't know why we need this explicit rejection. webidl layer should do this automatically.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8986509 [details]
Bug 1469433 - Handle shutdown more gracefully.

https://reviewboard.mozilla.org/r/251834/#review258328

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8986510 [details]
Bug 1469433 - Remove an unused function.

https://reviewboard.mozilla.org/r/251836/#review258330

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8986508 [details]
Bug 1469433 - Match the spec's error reporting more closely.

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

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8986509 [details]
Bug 1469433 - Handle shutdown more gracefully.

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

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8986510 [details]
Bug 1469433 - Remove an unused function.

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

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8986508 [details]
Bug 1469433 - Match the spec's error reporting more closely.

https://reviewboard.mozilla.org/r/251832/#review258332

::: dom/payments/PaymentRequest.cpp:652
(Diff revision 2)
>      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>      return nullptr;
>    }
>  
>    if (mResultPromise) {
> +    // XXX This doesn't match the spec but does match Chromium.

File a spec bug?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83f67349e19f
Match the spec's error reporting more closely. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ef0c3f86216f
Handle shutdown more gracefully. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5e99f060685d
Remove an unused function. r=smaug

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Olli Pettay [:smaug] from comment #13)
> File a spec bug?

I went to do this and then realized that this is probably more of a bug in Chomium. The spec allows canMakePayment to throw a NotAllowedError, which is what we do (though a conforming implementation would be to always throw NotAllowed). Blink simply throws an InvalidStateError. I guess the spec is fine, if a bit vague.

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11599 for changes under testing/web-platform/tests

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/83f67349e19f
https://hg.mozilla.org/mozilla-central/rev/ef0c3f86216f
https://hg.mozilla.org/mozilla-central/rev/5e99f060685d

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged