Closed Bug 1469433 Opened Last year Closed Last year

Audit PaymentRequest and related APIs for incorrect eager exceptions

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

Attachments

(3 files)

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.
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
(I also threw in a couple of cleanups)
Attachment #8986508 - Flags: review?(amarchesini) → review?(bugs)
Attachment #8986509 - Flags: review?(amarchesini) → review?(bugs)
Attachment #8986510 - Flags: review?(amarchesini) → review?(bugs)
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 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.
Attachment #8986508 - Flags: review?(bugs) → review-
Comment on attachment 8986509 [details]
Bug 1469433 - Handle shutdown more gracefully.

https://reviewboard.mozilla.org/r/251834/#review258328
Attachment #8986509 - Flags: review?(bugs) → review+
Comment on attachment 8986510 [details]
Bug 1469433 - Remove an unused function.

https://reviewboard.mozilla.org/r/251836/#review258330
Attachment #8986510 - Flags: review?(bugs) → review+
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?
Attachment #8986508 - Flags: review?(bugs) → review+
Pushed by mrbkap@mozilla.com:
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
(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.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11599 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
You need to log in before you can comment on or make changes to this bug.