Bug 1448408 - Web Authentication - Don't immediately abort on visibility events r=keeler
authorJ.C. Jones <jjones@mozilla.com>
Fri, 29 Mar 2019 17:59:08 +0000
changeset 466886 f7937d3264db00771b46cb1fcba71640d8df05cb
parent 466885 a5c4f7e6069a1962f5364226f4746914c08ae6ef
child 466887 de24b08ddc37fd7d3fd29c6b893713d5657269e6
push id81873
push userjjones@mozilla.com
push dateFri, 29 Mar 2019 23:14:26 +0000
treeherderautoland@f7937d3264db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1448408
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1448408 - Web Authentication - Don't immediately abort on visibility events r=keeler The published recommendation of L1 for WebAuthn changed the visibility/focus listening behaviors to a SHOULD [1], and Chromium, for reasons like our SoftU2F bug [0], opted to not interrupt on tabswitch/visibility change. Let's do the same thing. This changes the visibility mechanism to set a flag on an ongoing transaction, and then, upon multiple calls to the FIDO/U2F functions, only aborts if visibility had changed. Otherwise, subsequent callers return early. This is harder to explain than it is really to use as a user. I think. At least, my testing feels natural when I'm working within two windows, both potentially prompting WebAuthn. Note: This also affects FIDO U2F API. [0] https://bugzilla.mozilla.org/show_bug.cgi?id=1448408#c0 [1] https://www.w3.org/TR/webauthn-1/#abortoperation Differential Revision: https://phabricator.services.mozilla.com/D25160
dom/u2f/U2F.cpp
dom/u2f/U2F.h
dom/u2f/tests/browser/browser_abort_visibility.js
dom/u2f/tests/frame_override_request.html
dom/webauthn/U2FTokenManager.cpp
dom/webauthn/WebAuthnManager.cpp
dom/webauthn/WebAuthnManager.h
dom/webauthn/WebAuthnManagerBase.cpp
dom/webauthn/WebAuthnManagerBase.h
dom/webauthn/tests/browser/browser_abort_visibility.js
dom/webauthn/tests/test_webauthn_override_request.html
--- a/dom/u2f/U2F.cpp
+++ b/dom/u2f/U2F.cpp
@@ -169,47 +169,56 @@ JSObject* U2F::WrapObject(JSContext* aCx
   return U2F_Binding::Wrap(aCx, this, aGivenProto);
 }
 
 template <typename T, typename C>
 void U2F::ExecuteCallback(T& aResp, nsMainThreadPtrHandle<C>& aCb) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aCb);
 
-  // Assert that mTransaction was cleared before before we were called to allow
-  // reentrancy from microtask checkpoints.
-  MOZ_ASSERT(mTransaction.isNothing());
-
   ErrorResult error;
   aCb->Call(aResp, error);
   NS_WARNING_ASSERTION(!error.Failed(), "dom::U2F::Promise callback failed");
   error.SuppressException();  // Useful exceptions already emitted
 }
 
 void U2F::Register(const nsAString& aAppId,
                    const Sequence<RegisterRequest>& aRegisterRequests,
                    const Sequence<RegisteredKey>& aRegisteredKeys,
                    U2FRegisterCallback& aCallback,
                    const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
                    ErrorResult& aRv) {
   MOZ_ASSERT(NS_IsMainThread());
 
-  if (mTransaction.isSome()) {
-    CancelTransaction(NS_ERROR_ABORT);
-  }
-
   nsMainThreadPtrHandle<U2FRegisterCallback> callback(
       new nsMainThreadPtrHolder<U2FRegisterCallback>("U2F::Register::callback",
                                                      &aCallback));
 
   // Ensure we have a callback.
   if (NS_WARN_IF(!callback)) {
     return;
   }
 
+  if (mTransaction.isSome()) {
+    // If there hasn't been a visibility change during the current
+    // transaction, then let's let that one complete rather than
+    // cancelling it on a subsequent call.
+    if (!mTransaction.ref().mVisibilityChanged) {
+      RegisterResponse response;
+      response.mErrorCode.Construct(
+          static_cast<uint32_t>(ErrorCode::OTHER_ERROR));
+      ExecuteCallback(response, callback);
+      return;
+    }
+
+    // Otherwise, the user may well have clicked away, so let's
+    // abort the old transaction and take over control from here.
+    CancelTransaction(NS_ERROR_ABORT);
+  }
+
   // Evaluate the AppID
   nsString adjustedAppId(aAppId);
   if (!EvaluateAppID(mParent, mOrigin, adjustedAppId)) {
     RegisterResponse response;
     response.mErrorCode.Construct(
         static_cast<uint32_t>(ErrorCode::BAD_REQUEST));
     ExecuteCallback(response, callback);
     return;
@@ -337,29 +346,42 @@ void U2F::FinishMakeCredential(const uin
 
 void U2F::Sign(const nsAString& aAppId, const nsAString& aChallenge,
                const Sequence<RegisteredKey>& aRegisteredKeys,
                U2FSignCallback& aCallback,
                const Optional<Nullable<int32_t>>& opt_aTimeoutSeconds,
                ErrorResult& aRv) {
   MOZ_ASSERT(NS_IsMainThread());
 
-  if (mTransaction.isSome()) {
-    CancelTransaction(NS_ERROR_ABORT);
-  }
-
   nsMainThreadPtrHandle<U2FSignCallback> callback(
       new nsMainThreadPtrHolder<U2FSignCallback>("U2F::Sign::callback",
                                                  &aCallback));
 
   // Ensure we have a callback.
   if (NS_WARN_IF(!callback)) {
     return;
   }
 
+  if (mTransaction.isSome()) {
+    // If there hasn't been a visibility change during the current
+    // transaction, then let's let that one complete rather than
+    // cancelling it on a subsequent call.
+    if (!mTransaction.ref().mVisibilityChanged) {
+      SignResponse response;
+      response.mErrorCode.Construct(
+          static_cast<uint32_t>(ErrorCode::OTHER_ERROR));
+      ExecuteCallback(response, callback);
+      return;
+    }
+
+    // Otherwise, the user may well have clicked away, so let's
+    // abort the old transaction and take over control from here.
+    CancelTransaction(NS_ERROR_ABORT);
+  }
+
   // Evaluate the AppID
   nsString adjustedAppId(aAppId);
   if (!EvaluateAppID(mParent, mOrigin, adjustedAppId)) {
     SignResponse response;
     response.mErrorCode.Construct(
         static_cast<uint32_t>(ErrorCode::BAD_REQUEST));
     ExecuteCallback(response, callback);
     return;
@@ -540,10 +562,16 @@ void U2F::RequestAborted(const uint64_t&
                          const nsresult& aError) {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (mTransaction.isSome() && mTransaction.ref().mId == aTransactionId) {
     RejectTransaction(aError);
   }
 }
 
+void U2F::HandleVisibilityChange() {
+  if (mTransaction.isSome()) {
+    mTransaction.ref().mVisibilityChanged = true;
+  }
+}
+
 }  // namespace dom
 }  // namespace mozilla
--- a/dom/u2f/U2F.h
+++ b/dom/u2f/U2F.h
@@ -35,17 +35,19 @@ struct RegisteredKey;
 
 class U2FTransaction {
   typedef Variant<nsMainThreadPtrHandle<U2FRegisterCallback>,
                   nsMainThreadPtrHandle<U2FSignCallback>>
       U2FCallback;
 
  public:
   explicit U2FTransaction(const U2FCallback&& aCallback)
-      : mCallback(std::move(aCallback)), mId(NextId()) {
+      : mCallback(std::move(aCallback)),
+        mId(NextId()),
+        mVisibilityChanged(false) {
     MOZ_ASSERT(mId > 0);
   }
 
   bool HasRegisterCallback() {
     return mCallback.is<nsMainThreadPtrHandle<U2FRegisterCallback>>();
   }
 
   auto& GetRegisterCallback() {
@@ -61,16 +63,20 @@ class U2FTransaction {
   }
 
   // The callback passed to the API.
   U2FCallback mCallback;
 
   // Unique transaction id.
   uint64_t mId;
 
+  // Whether or not visibility has changed for the window during this
+  // transaction
+  bool mVisibilityChanged;
+
  private:
   // Generates a unique id for new transactions. This doesn't have to be unique
   // forever, it's sufficient to differentiate between temporally close
   // transactions, where messages can intersect. Can overflow.
   static uint64_t NextId() {
     static uint64_t id = 0;
     return ++id;
   }
@@ -119,17 +125,19 @@ class U2F final : public WebAuthnManager
 
   MOZ_CAN_RUN_SCRIPT
   void RequestAborted(const uint64_t& aTransactionId,
                       const nsresult& aError) override;
 
  protected:
   // Cancels the current transaction (by sending a Cancel message to the
   // parent) and rejects it by calling RejectTransaction().
-  MOZ_CAN_RUN_SCRIPT void CancelTransaction(const nsresult& aError) override;
+  MOZ_CAN_RUN_SCRIPT void CancelTransaction(const nsresult& aError);
+  // Upon a visibility change, makes note of it in the current transaction.
+  MOZ_CAN_RUN_SCRIPT void HandleVisibilityChange() override;
 
  private:
   MOZ_CAN_RUN_SCRIPT ~U2F();
 
   template <typename T, typename C>
   MOZ_CAN_RUN_SCRIPT void ExecuteCallback(T& aResp,
                                           nsMainThreadPtrHandle<C>& aCb);
 
--- a/dom/u2f/tests/browser/browser_abort_visibility.js
+++ b/dom/u2f/tests/browser/browser_abort_visibility.js
@@ -75,25 +75,33 @@ add_task(async function test_abort() {
   let tab_create = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
 
   // Start the request.
   await startMakeCredentialRequest(tab_create);
   await assertStatus(tab_create, "pending");
 
   // Open another tab and switch to it. The first will lose focus.
   let tab_get = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
+
+  // Start a GetAssertion() request in the second tab. That will abort the first.
+  await startGetAssertionRequest(tab_get);
+  await waitForStatus(tab_get, "pending");
   await waitForStatus(tab_create, "aborted");
 
-  // Start a GetAssertion() request in the second tab.
+  // Start a second request in the second tab. It should remain pending.
   await startGetAssertionRequest(tab_get);
+  await waitForStatus(tab_get, "pending");
+
+  // Switch back to the first tab. The second should still be pending
+  await BrowserTestUtils.switchTab(gBrowser, tab_create);
   await assertStatus(tab_get, "pending");
 
-  // Switch back to the first tab, the get() request is aborted.
-  await BrowserTestUtils.switchTab(gBrowser, tab_create);
-  await waitForStatus(tab_get, "aborted");
+  // Switch back to the get tab. The second should remain pending
+  await BrowserTestUtils.switchTab(gBrowser, tab_get);
+  await assertStatus(tab_get, "pending");
 
   // Close tabs.
   BrowserTestUtils.removeTab(tab_create);
   BrowserTestUtils.removeTab(tab_get);
 
   // Cleanup.
   Services.prefs.clearUserPref("security.webauth.u2f");
   Services.prefs.clearUserPref("security.webauth.webauthn_enable_softtoken");
--- a/dom/u2f/tests/frame_override_request.html
+++ b/dom/u2f/tests/frame_override_request.html
@@ -56,30 +56,32 @@
       await Promise.resolve();
     }
 
     // Test that .create() and .get() requests override any pending requests.
     (async function () {
       // Request a new credential.
       await requestMakeCredential("aborted1");
 
-      // Request another credential, the first request will abort.
+      // Request another credential, the new request will abort.
       await requestMakeCredential("aborted2");
-      local_is(status, "aborted1", "first request aborted");
-
-      // Request an assertion, the second request will abort.
-      await requestGetAssertion("aborted3");
       local_is(status, "aborted2", "second request aborted");
 
-      // Request another assertion, the third request will abort.
-      await requestGetAssertion("aborted4");
+      // Request an assertion, the new request will still abort.
+      await requestGetAssertion("aborted3");
       local_is(status, "aborted3", "third request aborted");
 
-      // Request another credential, the fourth request will abort.
+      // Request another assertion, this fourth request will abort.
+      await requestGetAssertion("aborted4");
+      local_is(status, "aborted4", "fourth request aborted");
+
+      // Request another credential, the fifth request will still abort. Why
+      // do we keep trying? Well, the test originally looked like this, and
+      // let's face it, it's kinda funny.
       await requestMakeCredential("aborted5");
-      local_is(status, "aborted4", "fourth request aborted");
+      local_is(status, "aborted5", "fifth request aborted");
 
       local_finished();
     })();
   </script>
 
 </body>
 </html>
--- a/dom/webauthn/U2FTokenManager.cpp
+++ b/dom/webauthn/U2FTokenManager.cpp
@@ -171,19 +171,22 @@ void U2FTokenManager::MaybeClearTransact
   // Only clear if we've been requested to do so by our current transaction
   // parent.
   if (mTransactionParent == aParent) {
     ClearTransaction();
   }
 }
 
 void U2FTokenManager::ClearTransaction() {
-  if (mLastTransactionId > 0) {
+  if (mLastTransactionId > 0 && mTransactionParent) {
     // Remove any prompts we might be showing for the current transaction.
     SendPromptNotification(kCancelPromptNotifcation, mLastTransactionId);
+    // Send an abort to any other ongoing transaction
+    Unused << mTransactionParent->SendAbort(mLastTransactionId,
+                                            NS_ERROR_DOM_ABORT_ERR);
   }
 
   mTransactionParent = nullptr;
 
   // Drop managers at the end of all transactions
   if (mTokenManagerImpl) {
     mTokenManagerImpl->Drop();
     mTokenManagerImpl = nullptr;
--- a/dom/webauthn/WebAuthnManager.cpp
+++ b/dom/webauthn/WebAuthnManager.cpp
@@ -175,16 +175,22 @@ void WebAuthnManager::RejectTransaction(
 void WebAuthnManager::CancelTransaction(const nsresult& aError) {
   if (!NS_WARN_IF(!mChild || mTransaction.isNothing())) {
     mChild->SendRequestCancel(mTransaction.ref().mId);
   }
 
   RejectTransaction(aError);
 }
 
+void WebAuthnManager::HandleVisibilityChange() {
+  if (mTransaction.isSome()) {
+    mTransaction.ref().mVisibilityChanged = true;
+  }
+}
+
 WebAuthnManager::~WebAuthnManager() {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (mTransaction.isSome()) {
     RejectTransaction(NS_ERROR_ABORT);
   }
 
   if (mChild) {
@@ -194,28 +200,38 @@ WebAuthnManager::~WebAuthnManager() {
   }
 }
 
 already_AddRefed<Promise> WebAuthnManager::MakeCredential(
     const PublicKeyCredentialCreationOptions& aOptions,
     const Optional<OwningNonNull<AbortSignal>>& aSignal) {
   MOZ_ASSERT(NS_IsMainThread());
 
-  if (mTransaction.isSome()) {
-    CancelTransaction(NS_ERROR_ABORT);
-  }
-
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mParent);
 
   ErrorResult rv;
   RefPtr<Promise> promise = Promise::Create(global, rv);
   if (rv.Failed()) {
     return nullptr;
   }
 
+  if (mTransaction.isSome()) {
+    // If there hasn't been a visibility change during the current
+    // transaction, then let's let that one complete rather than
+    // cancelling it on a subsequent call.
+    if (!mTransaction.ref().mVisibilityChanged) {
+      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
+      return promise.forget();
+    }
+
+    // Otherwise, the user may well have clicked away, so let's
+    // abort the old transaction and take over control from here.
+    CancelTransaction(NS_ERROR_ABORT);
+  }
+
   // Abort the request if aborted flag is already set.
   if (aSignal.WasPassed() && aSignal.Value().Aborted()) {
     promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
     return promise.forget();
   }
 
   nsString origin;
   nsCString rpId;
@@ -395,28 +411,38 @@ already_AddRefed<Promise> WebAuthnManage
   return promise.forget();
 }
 
 already_AddRefed<Promise> WebAuthnManager::GetAssertion(
     const PublicKeyCredentialRequestOptions& aOptions,
     const Optional<OwningNonNull<AbortSignal>>& aSignal) {
   MOZ_ASSERT(NS_IsMainThread());
 
-  if (mTransaction.isSome()) {
-    CancelTransaction(NS_ERROR_ABORT);
-  }
-
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mParent);
 
   ErrorResult rv;
   RefPtr<Promise> promise = Promise::Create(global, rv);
   if (rv.Failed()) {
     return nullptr;
   }
 
+  if (mTransaction.isSome()) {
+    // If there hasn't been a visibility change during the current
+    // transaction, then let's let that one complete rather than
+    // cancelling it on a subsequent call.
+    if (!mTransaction.ref().mVisibilityChanged) {
+      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
+      return promise.forget();
+    }
+
+    // Otherwise, the user may well have clicked away, so let's
+    // abort the old transaction and take over control from here.
+    CancelTransaction(NS_ERROR_ABORT);
+  }
+
   // Abort the request if aborted flag is already set.
   if (aSignal.WasPassed() && aSignal.Value().Aborted()) {
     promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
     return promise.forget();
   }
 
   nsString origin;
   nsCString rpId;
@@ -574,28 +600,38 @@ already_AddRefed<Promise> WebAuthnManage
 
   return promise.forget();
 }
 
 already_AddRefed<Promise> WebAuthnManager::Store(
     const Credential& aCredential) {
   MOZ_ASSERT(NS_IsMainThread());
 
-  if (mTransaction.isSome()) {
-    CancelTransaction(NS_ERROR_ABORT);
-  }
-
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mParent);
 
   ErrorResult rv;
   RefPtr<Promise> promise = Promise::Create(global, rv);
   if (rv.Failed()) {
     return nullptr;
   }
 
+  if (mTransaction.isSome()) {
+    // If there hasn't been a visibility change during the current
+    // transaction, then let's let that one complete rather than
+    // cancelling it on a subsequent call.
+    if (!mTransaction.ref().mVisibilityChanged) {
+      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
+      return promise.forget();
+    }
+
+    // Otherwise, the user may well have clicked away, so let's
+    // abort the old transaction and take over control from here.
+    CancelTransaction(NS_ERROR_ABORT);
+  }
+
   promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
   return promise.forget();
 }
 
 void WebAuthnManager::FinishMakeCredential(
     const uint64_t& aTransactionId,
     const WebAuthnMakeCredentialResult& aResult) {
   MOZ_ASSERT(NS_IsMainThread());
--- a/dom/webauthn/WebAuthnManager.h
+++ b/dom/webauthn/WebAuthnManager.h
@@ -43,26 +43,30 @@
  */
 
 namespace mozilla {
 namespace dom {
 
 class WebAuthnTransaction {
  public:
   explicit WebAuthnTransaction(const RefPtr<Promise>& aPromise)
-      : mPromise(aPromise), mId(NextId()) {
+      : mPromise(aPromise), mId(NextId()), mVisibilityChanged(false) {
     MOZ_ASSERT(mId > 0);
   }
 
   // JS Promise representing the transaction status.
   RefPtr<Promise> mPromise;
 
   // Unique transaction id.
   uint64_t mId;
 
+  // Whether or not visibility has changed for the window during this
+  // transaction
+  bool mVisibilityChanged;
+
  private:
   // Generates a unique id for new transactions. This doesn't have to be unique
   // forever, it's sufficient to differentiate between temporally close
   // transactions, where messages can intersect. Can overflow.
   static uint64_t NextId() {
     static uint64_t id = 0;
     return ++id;
   }
@@ -100,17 +104,19 @@ class WebAuthnManager final : public Web
 
   // AbortFollower
 
   void Abort() override;
 
  protected:
   // Cancels the current transaction (by sending a Cancel message to the
   // parent) and rejects it by calling RejectTransaction().
-  void CancelTransaction(const nsresult& aError) override;
+  void CancelTransaction(const nsresult& aError);
+  // Upon a visibility change, makes note of it in the current transaction.
+  void HandleVisibilityChange() override;
 
  private:
   virtual ~WebAuthnManager();
 
   // Clears all information we have about the current transaction.
   void ClearTransaction();
   // Rejects the current transaction and calls ClearTransaction().
   void RejectTransaction(const nsresult& aError);
--- a/dom/webauthn/WebAuthnManagerBase.cpp
+++ b/dom/webauthn/WebAuthnManagerBase.cpp
@@ -136,14 +136,14 @@ WebAuthnManagerBase::HandleEvent(Event* 
     }
 
     nsGlobalWindowInner* win = nsGlobalWindowInner::Cast(doc->GetInnerWindow());
     if (NS_WARN_IF(!win) || !win->IsTopInnerWindow()) {
       return NS_OK;
     }
   }
 
-  CancelTransaction(NS_ERROR_DOM_ABORT_ERR);
+  HandleVisibilityChange();
   return NS_OK;
 }
 
 }  // namespace dom
 }  // namespace mozilla
--- a/dom/webauthn/WebAuthnManagerBase.h
+++ b/dom/webauthn/WebAuthnManagerBase.h
@@ -44,18 +44,18 @@ class WebAuthnManagerBase : public nsIDO
   virtual void RequestAborted(const uint64_t& aTransactionId,
                               const nsresult& aError) = 0;
 
   void ActorDestroyed();
 
  protected:
   MOZ_CAN_RUN_SCRIPT virtual ~WebAuthnManagerBase();
 
-  // Needed by HandleEvent() to cancel transactions.
-  MOZ_CAN_RUN_SCRIPT virtual void CancelTransaction(const nsresult& aError) = 0;
+  // Needed by HandleEvent() to track visibilty changes.
+  MOZ_CAN_RUN_SCRIPT virtual void HandleVisibilityChange() = 0;
 
   // Visibility event handling.
   void ListenForVisibilityEvents();
   void StopListeningForVisibilityEvents();
 
   bool MaybeCreateBackgroundActor();
 
   // The parent window.
--- a/dom/webauthn/tests/browser/browser_abort_visibility.js
+++ b/dom/webauthn/tests/browser/browser_abort_visibility.js
@@ -13,18 +13,18 @@ async function assertStatus(tab, expecte
     return content.document.getElementById("status").value;
   });
   is(actual, expected, "webauthn request " + expected);
 }
 
 async function waitForStatus(tab, expected) {
   await ContentTask.spawn(tab.linkedBrowser, [expected], async function (expected) {
     return ContentTaskUtils.waitForCondition(() => {
-      info("visbility state: " + content.document.visibilityState);
-      info("docshell active: " + docShell.isActive);
+      info("expecting " + expected + ", visbility state: " + content.document.visibilityState);
+      info("expecting " + expected + ", docshell active: " + docShell.isActive);
       return content.document.getElementById("status").value == expected;
     });
   });
 
   await assertStatus(tab, expected);
 }
 
 function startMakeCredentialRequest(tab) {
@@ -98,24 +98,25 @@ add_task(async function test_switch_tab(
   let tab_create = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
 
   // Start the request.
   await startMakeCredentialRequest(tab_create);
   await assertStatus(tab_create, "pending");
 
   // Open another tab and switch to it. The first will lose focus.
   let tab_get = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL);
-  await waitForStatus(tab_create, "aborted");
+  await assertStatus(tab_create, "pending");
 
-  // Start a GetAssertion() request in the second tab.
+  // Start a GetAssertion() request in the second tab, the first is aborted
   await startGetAssertionRequest(tab_get);
+  await waitForStatus(tab_create, "aborted");
   await assertStatus(tab_get, "pending");
 
-  // Switch back to the first tab, the get() request is aborted.
-  await BrowserTestUtils.switchTab(gBrowser, tab_create);
+  // Start a second request in the second tab. It should abort.
+  await startGetAssertionRequest(tab_get);
   await waitForStatus(tab_get, "aborted");
 
   // Close tabs.
   BrowserTestUtils.removeTab(tab_create);
   BrowserTestUtils.removeTab(tab_get);
 });
 
 function waitForWindowActive(win, active) {
@@ -132,17 +133,17 @@ add_task(async function test_new_window_
   // Start a MakeCredential request.
   await startMakeCredentialRequest(tab);
   await assertStatus(tab, "pending");
 
   let windowGonePromise = waitForWindowActive(window, false);
   // Open a new window. The tab will lose focus.
   let win = await BrowserTestUtils.openNewBrowserWindow();
   await windowGonePromise;
-  await waitForStatus(tab, "aborted");
+  await assertStatus(tab, "pending");
 
   let windowBackPromise = waitForWindowActive(window, true);
   await BrowserTestUtils.closeWindow(win);
   await windowBackPromise;
 
   // Close tab.
   await BrowserTestUtils.removeTab(tab);
 });
@@ -154,17 +155,17 @@ add_task(async function test_new_window_
   // Start a GetAssertion request.
   await startGetAssertionRequest(tab);
   await assertStatus(tab, "pending");
 
   let windowGonePromise = waitForWindowActive(window, false);
   // Open a new window. The tab will lose focus.
   let win = await BrowserTestUtils.openNewBrowserWindow();
   await windowGonePromise;
-  await waitForStatus(tab, "aborted");
+  await assertStatus(tab, "pending");
 
   let windowBackPromise = waitForWindowActive(window, true);
   await BrowserTestUtils.closeWindow(win);
   await windowBackPromise;
 
   // Close tab.
   BrowserTestUtils.removeTab(tab);
 });
@@ -183,21 +184,22 @@ add_task(async function test_minimize_ma
 
   // Start a MakeCredential request.
   await startMakeCredentialRequest(tab);
   await assertStatus(tab, "pending");
 
   // Minimize the window.
   let windowGonePromise = waitForWindowActive(win, false);
   win.minimize();
-  await waitForStatus(tab, "aborted");
+  await assertStatus(tab, "pending");
   await windowGonePromise;
 
   // Restore the window.
   await new Promise(resolve => SimpleTest.waitForFocus(resolve, win));
+  await assertStatus(tab, "pending");
 
   // Close window and wait for main window to be focused again.
   let windowBackPromise = waitForWindowActive(window, true);
   await BrowserTestUtils.closeWindow(win);
   await windowBackPromise;
 });
 
 add_task(async function test_minimize_get() {
@@ -214,19 +216,20 @@ add_task(async function test_minimize_ge
 
   // Start a GetAssertion request.
   await startGetAssertionRequest(tab);
   await assertStatus(tab, "pending");
 
   // Minimize the window.
   let windowGonePromise = waitForWindowActive(win, false);
   win.minimize();
-  await waitForStatus(tab, "aborted");
+  await assertStatus(tab, "pending");
   await windowGonePromise;
 
   // Restore the window.
   await new Promise(resolve => SimpleTest.waitForFocus(resolve, win));
+  await assertStatus(tab, "pending");
 
   // Close window and wait for main window to be focused again.
   let windowBackPromise = waitForWindowActive(window, true);
   await BrowserTestUtils.closeWindow(win);
   await windowBackPromise;
 });
--- a/dom/webauthn/tests/test_webauthn_override_request.html
+++ b/dom/webauthn/tests/test_webauthn_override_request.html
@@ -70,28 +70,30 @@
       await Promise.resolve();
     }
 
     // Test that .create() and .get() requests override any pending requests.
     add_task(async () => {
       // Request a new credential.
       await requestMakeCredential("aborted1");
 
-      // Request another credential, the first request will abort.
+      // Request another credential, the new request will abort.
       await requestMakeCredential("aborted2");
-      is(status, "aborted1", "first request aborted");
-
-      // Request an assertion, the second request will abort.
-      await requestGetAssertion("aborted3");
       is(status, "aborted2", "second request aborted");
 
-      // Request another assertion, the third request will abort.
-      await requestGetAssertion("aborted4");
+      // Request an assertion, the new request will still abort.
+      await requestGetAssertion("aborted3");
       is(status, "aborted3", "third request aborted");
 
-      // Request another credential, the fourth request will abort.
+      // Request another assertion, this fourth request will abort.
+      await requestGetAssertion("aborted4");
+      is(status, "aborted4", "fourth request aborted");
+
+      // Request another credential, the fifth request will still abort. Why
+      // do we keep trying? Well, the test originally looked like this, and
+      // let's face it, it's kinda funny.
       await requestMakeCredential("aborted5");
-      is(status, "aborted4", "fourth request aborted");
+      is(status, "aborted5", "fifth request aborted");
     });
   </script>
 
 </body>
 </html>