Closed Bug 1469433 Opened 6 years ago Closed 6 years ago

Audit PaymentRequest and related APIs for incorrect eager exceptions

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla62

Tracking Flags:

Tracking

Status

firefox62

---

fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

Attachments

(3 files)

Bug 1469433 - Match the spec's error reporting more closely. 6 years ago Blake Kaplan (:mrbkap) (inactive) 59 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1469433 - Handle shutdown more gracefully. 6 years ago Blake Kaplan (:mrbkap) (inactive) 59 bytes, text/x-review-board-request	smaug : review+	Details
Bug 1469433 - Remove an unused function. 6 years ago Blake Kaplan (:mrbkap) (inactive) 59 bytes, text/x-review-board-request	smaug : review+	Details

Blake Kaplan (:mrbkap) (inactive)

Assignee

Description

•

6 years ago

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.

Blake Kaplan (:mrbkap) (inactive)

Assignee

Comment 1

•

6 years ago

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

Blake Kaplan (:mrbkap) (inactive)

Assignee

Comment 2

•

6 years ago

(I also threw in a couple of cleanups)

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Blake Kaplan (:mrbkap) (inactive)

Assignee

Updated

•

6 years ago

Attachment #8986508 - Flags: review?(amarchesini) → review?(bugs)

Attachment #8986509 - Flags: review?(amarchesini) → review?(bugs)

Attachment #8986510 - Flags: review?(amarchesini) → review?(bugs)

Blake Kaplan (:mrbkap) (inactive)

Assignee

Comment 6

•

6 years ago

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

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 7

•

6 years ago

mozreview-review

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-

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 8

•

6 years ago

mozreview-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+

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 9

•

6 years ago

mozreview-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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 13

•

6 years ago

mozreview-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+

Pulsebot

Comment 14

•

6 years ago

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

Blake Kaplan (:mrbkap) (inactive)

Assignee

Comment 15

•

6 years ago

(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.

Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)

Comment 16

•

6 years ago

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

Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)

Comment 17

•

6 years ago

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

Narcis Beleuzu [:NarcisB]

Comment 18

•

6 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox62: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla62

Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)

Comment 19

•

6 years ago

Upstream PR merged

You need to log in before you can comment on or make changes to this bug.