Bug 1469433 - Match the spec's error reporting more closely. r=smaug draft
authorBlake Kaplan <mrbkap@gmail.com>
Tue, 19 Jun 2018 15:30:43 -0700
changeset 808829 3bc4b3c22b539fb1f571aaf446a0033d07c0ba05
parent 808580 e429320fcdd2d5236bb4713e6c435456146e42b9
child 808830 dd64947caa046523333717a3fc03a68857ddc9f9
push id113511
push userbmo:mrbkap@mozilla.com
push dateWed, 20 Jun 2018 21:38:38 +0000
reviewerssmaug
bugs1469433
milestone62.0a1
Bug 1469433 - Match the spec's error reporting more closely. r=smaug MozReview-Commit-ID: LI13cA0okeu
dom/payments/PaymentRequest.cpp
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/payment-request/payment-request-canmakepayment-method-manual.https.html
--- a/dom/payments/PaymentRequest.cpp
+++ b/dom/payments/PaymentRequest.cpp
@@ -644,33 +644,31 @@ already_AddRefed<Promise>
 PaymentRequest::CanMakePayment(ErrorResult& aRv)
 {
   if (mState != eCreated) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return nullptr;
   }
 
   if (mResultPromise) {
+    // XXX This doesn't match the spec but does match Chromium.
     aRv.Throw(NS_ERROR_DOM_NOT_ALLOWED_ERR);
     return nullptr;
   }
 
-  nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal();
+  nsIGlobalObject* global = GetOwnerGlobal();
   ErrorResult result;
   RefPtr<Promise> promise = Promise::Create(global, result);
   if (result.Failed()) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
-  if (NS_WARN_IF(!manager)) {
-    aRv.Throw(NS_ERROR_FAILURE);
-    return nullptr;
-  }
+  MOZ_ASSERT(manager);
   nsresult rv = manager->CanMakePayment(this);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     promise->MaybeReject(NS_ERROR_FAILURE);
     return promise.forget();
   }
   mResultPromise = promise;
   return promise.forget();
 }
@@ -682,48 +680,51 @@ PaymentRequest::RespondCanMakePayment(bo
   mResultPromise->MaybeResolve(aResult);
   mResultPromise = nullptr;
 }
 
 already_AddRefed<Promise>
 PaymentRequest::Show(const Optional<OwningNonNull<Promise>>& aDetailsPromise,
                      ErrorResult& aRv)
 {
-  if (mState != eCreated) {
-    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
-    return nullptr;
-  }
-
   if (!EventStateManager::IsHandlingUserInput()) {
     aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
     return nullptr;
   }
 
-  nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal();
+  nsIGlobalObject* global = GetOwnerGlobal();
+  nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(global);
+  MOZ_ASSERT(win);
+  nsIDocument* doc = win->GetExtantDoc();
+  if (!doc || !doc->IsCurrentActiveDocument()) {
+    aRv.Throw(NS_ERROR_DOM_ABORT_ERR);
+    return nullptr;
+  }
+
+  if (mState != eCreated) {
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
+    return nullptr;
+  }
+
   ErrorResult result;
   RefPtr<Promise> promise = Promise::Create(global, result);
   if (result.Failed()) {
     mState = eClosed;
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
-  RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
-  if (NS_WARN_IF(!manager)) {
-    mState = eClosed;
-    aRv.Throw(NS_ERROR_FAILURE);
-    return nullptr;
-  }
-
   if (aDetailsPromise.WasPassed()) {
     aDetailsPromise.Value().AppendNativeHandler(this);
     mUpdating = true;
     mDeferredShow = true;
   }
 
+  RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
+  MOZ_ASSERT(manager);
   nsresult rv = manager->ShowPayment(this);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     if (rv == NS_ERROR_ABORT) {
       promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
     } else {
       promise->MaybeReject(NS_ERROR_DOM_NOT_ALLOWED_ERR);
     }
     mState = eClosed;
@@ -789,35 +790,31 @@ already_AddRefed<Promise>
 PaymentRequest::Abort(ErrorResult& aRv)
 {
   if (mState != eInteractive) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return nullptr;
   }
 
   if (mAbortPromise) {
-    aRv.Throw(NS_ERROR_DOM_NOT_ALLOWED_ERR);
+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return nullptr;
   }
 
-  nsCOMPtr<nsIGlobalObject> global = GetOwnerGlobal();
+  nsIGlobalObject* global = GetOwnerGlobal();
   ErrorResult result;
   RefPtr<Promise> promise = Promise::Create(global, result);
   if (result.Failed()) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
-  if (NS_WARN_IF(!manager)) {
-    aRv.Throw(NS_ERROR_FAILURE);
-    return nullptr;
-  }
-
-  // It's possible for to call this between show and its promise resolving.
+  MOZ_ASSERT(manager);
+  // It's possible to be called between show and its promise resolving.
   nsresult rv = manager->AbortPayment(this, mDeferredShow);
   mDeferredShow = false;
   if (NS_WARN_IF(NS_FAILED(rv))) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
   mAbortPromise = promise;
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -597410,17 +597410,17 @@
    "2280f0ef821cdc3093e10c2162d3756f5eeb78de",
    "testharness"
   ],
   "payment-request/payment-request-abort-method.https.html": [
    "a9d811dc41487ba7a8e5d55319574364b93362aa",
    "testharness"
   ],
   "payment-request/payment-request-canmakepayment-method-manual.https.html": [
-   "7531ea3b11f6e8de8cf71666b19861dbc5267cf7",
+   "11df3310832e043e533a11245b012b9de1c0bb26",
    "manual"
   ],
   "payment-request/payment-request-constructor-crash.https.html": [
    "383d1c3f9505ee63d504bee87e13efa90ba49f3d",
    "testharness"
   ],
   "payment-request/payment-request-constructor.https.html": [
    "511c4d25939d344f187fe4a8daa4a8ff926b613d",
--- a/testing/web-platform/tests/payment-request/payment-request-canmakepayment-method-manual.https.html
+++ b/testing/web-platform/tests/payment-request/payment-request-canmakepayment-method-manual.https.html
@@ -121,19 +121,22 @@ promise_test(async t => {
       );
       break;
     }
   }
 }, `Optionally, at the user agent's discretion, return a promise rejected with a "NotAllowedError" DOMException.`);
 
 function manualTest1(elem){
   elem.disabled = true;
+
+  // NB: request.show has to be called outside of promise_test to ensure the
+  // user's click is still visible to PaymentRequest.show.
+  const request = new PaymentRequest(defaultMethods, defaultDetails);
+  const acceptPromise = request.show(); // Sets state to "interactive"
   promise_test(async t => {
-    const request = new PaymentRequest(defaultMethods, defaultDetails);
-    const acceptPromise = request.show(); // Sets state to "interactive"
     const canMakePaymentPromise = request.canMakePayment();
     try {
       const result = await canMakePaymentPromise;
       assert_true(
         false,
         `canMakePaymentPromise should have thrown InvalidStateError`
       );
     } catch (err) {
@@ -143,21 +146,24 @@ function manualTest1(elem){
       await promise_rejects(t, "AbortError", acceptPromise);
     }
     // The state should be "closed"
     await promise_rejects(t, "InvalidStateError", request.canMakePayment());
   }, elem.textContent.trim());
 }
 
 function manualTest2(elem){
- elem.disabled = true;
+  elem.disabled = true;
+
+  // See above for why it's important for these lines to be outside of
+  // promise_test.
+  const request = new PaymentRequest(defaultMethods, defaultDetails);
+  const acceptPromise = request.show(); // The state is now "interactive"
+  acceptPromise.catch(() => {}); // no-op, just to silence unhandled rejection in devtools.
   promise_test(async t => {
-    const request = new PaymentRequest(defaultMethods, defaultDetails);
-    const acceptPromise = request.show(); // The state is now "interactive"
-    acceptPromise.catch(() => {}); // no-op, just to silence unhandled rejection in devtools.
     await request.abort(); // The state is now "closed"
     await promise_rejects(t, "InvalidStateError", request.canMakePayment());
     try {
       const result = await request.canMakePayment();
       assert_true(
         false,
         `should have thrown InvalidStateError, but instead returned "${result}"`
       );