Bug 1422661 - Fix U2F test failures in new microtask scheduling r=jcj
authorTim Taubert <ttaubert@mozilla.com>
Fri, 08 Dec 2017 16:55:52 +0100
changeset 450081 1e4539d6cff173308a2a85d89d663a499b3a00ca
parent 450080 089dce52d825c6f83fb11d517daf466653972c21
child 450082 1ece11256d13763d5d6d15e00797d9afabcc8935
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs1422661
milestone59.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 1422661 - Fix U2F test failures in new microtask scheduling r=jcj Summary: Ensure that transactions are cleared before U2FCallbacks are called, to allow reentrancy from microtask checkpoints. Move the two possible callbacks into U2FTransaction so we have nicer invariants and know that there's a callback as long as we have a transaction. Reviewers: jcj Reviewed By: jcj Bug #: 1422661 Differential Revision: https://phabricator.services.mozilla.com/D329
dom/u2f/U2F.cpp
dom/u2f/U2F.h
--- a/dom/u2f/U2F.cpp
+++ b/dom/u2f/U2F.cpp
@@ -259,37 +259,16 @@ BuildTransactionHashes(const nsCString& 
     MOZ_LOG(gU2FLog, LogLevel::Debug,
             ("dom::U2FManager::Client Data Hash (base64): %s",
               NS_ConvertUTF16toUTF8(base64).get()));
   }
 
   return NS_OK;
 }
 
-template<typename T, typename C>
-static void
-ExecuteCallback(T& aResp, Maybe<nsMainThreadPtrHandle<C>>& aCb)
-{
-  MOZ_ASSERT(NS_IsMainThread());
-  if (aCb.isNothing()) {
-    return;
-  }
-
-  // reset callback earlier to allow reentry from callback.
-  nsMainThreadPtrHandle<C> callback(aCb.ref());
-  aCb.reset();
-  MOZ_ASSERT(aCb.isNothing());
-  MOZ_ASSERT(!!callback);
-
-  ErrorResult error;
-  callback->Call(aResp, error);
-  NS_WARNING_ASSERTION(!error.Failed(), "dom::U2F::Promise callback failed");
-  error.SuppressException(); // Useful exceptions already emitted
-}
-
 /***********************************************************************
  * U2F JavaScript API Implementation
  **********************************************************************/
 
 U2F::~U2F()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -297,19 +276,16 @@ U2F::~U2F()
     RejectTransaction(NS_ERROR_ABORT);
   }
 
   if (mChild) {
     RefPtr<WebAuthnTransactionChild> c;
     mChild.swap(c);
     c->Disconnect();
   }
-
-  mRegisterCallback.reset();
-  mSignCallback.reset();
 }
 
 void
 U2F::Init(ErrorResult& aRv)
 {
   MOZ_ASSERT(mParent);
 
   nsCOMPtr<nsIDocument> doc = mParent->GetDoc();
@@ -332,45 +308,64 @@ U2F::Init(ErrorResult& aRv)
 }
 
 /* virtual */ JSObject*
 U2F::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
 {
   return U2FBinding::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);
   }
 
-  MOZ_ASSERT(mRegisterCallback.isNothing());
-  mRegisterCallback = Some(nsMainThreadPtrHandle<U2FRegisterCallback>(
-                        new nsMainThreadPtrHolder<U2FRegisterCallback>(
-                            "U2F::Register::callback", &aCallback)));
+  nsMainThreadPtrHandle<U2FRegisterCallback> callback(
+    new nsMainThreadPtrHolder<U2FRegisterCallback>("U2F::Register::callback",
+                                                   &aCallback));
 
-  uint32_t adjustedTimeoutMillis = AdjustedTimeoutMillis(opt_aTimeoutSeconds);
+  // Ensure we have a callback.
+  if (NS_WARN_IF(!callback)) {
+    return;
+  }
 
   // Evaluate the AppID
   nsString adjustedAppId;
   adjustedAppId.Assign(aAppId);
   ErrorCode appIdResult = EvaluateAppID(mParent, mOrigin, adjustedAppId);
   if (appIdResult != ErrorCode::OK) {
     RegisterResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(appIdResult));
-    ExecuteCallback(response, mRegisterCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   // Produce the AppParam from the current AppID
   nsCString cAppId = NS_ConvertUTF16toUTF8(adjustedAppId);
 
   nsAutoString clientDataJSON;
 
@@ -387,76 +382,83 @@ U2F::Register(const nsAString& aAppId,
       continue;
     }
   }
 
   // Did we not get a valid RegisterRequest? Abort.
   if (clientDataJSON.IsEmpty()) {
     RegisterResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::BAD_REQUEST));
-    ExecuteCallback(response, mRegisterCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   // Build the exclusion list, if any
   nsTArray<WebAuthnScopedCredentialDescriptor> excludeList;
   RegisteredKeysToScopedCredentialList(adjustedAppId, aRegisteredKeys,
                                        excludeList);
 
   auto clientData = NS_ConvertUTF16toUTF8(clientDataJSON);
 
   CryptoBuffer rpIdHash, clientDataHash;
   if (NS_FAILED(BuildTransactionHashes(cAppId, clientData,
                                        rpIdHash, clientDataHash))) {
     RegisterResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OTHER_ERROR));
-    ExecuteCallback(response, mRegisterCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   if (!MaybeCreateBackgroundActor()) {
     RegisterResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OTHER_ERROR));
-    ExecuteCallback(response, mRegisterCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   ListenForVisibilityEvents();
 
   // Always blank for U2F
   nsTArray<WebAuthnExtension> extensions;
 
   // Default values for U2F.
   WebAuthnAuthenticatorSelection authSelection(false /* requireResidentKey */,
                                                false /* requireUserVerification */,
                                                false /* requirePlatformAttachment */);
 
+  uint32_t adjustedTimeoutMillis = AdjustedTimeoutMillis(opt_aTimeoutSeconds);
+
   WebAuthnMakeCredentialInfo info(rpIdHash,
                                   clientDataHash,
                                   adjustedTimeoutMillis,
                                   excludeList,
                                   extensions,
                                   authSelection);
 
   MOZ_ASSERT(mTransaction.isNothing());
-  mTransaction = Some(U2FTransaction(clientData));
+  mTransaction = Some(U2FTransaction(clientData, Move(AsVariant(callback))));
   mChild->SendRequestRegister(mTransaction.ref().mId, info);
 }
 
 void
 U2F::FinishMakeCredential(const uint64_t& aTransactionId,
                           nsTArray<uint8_t>& aRegBuffer)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   // Check for a valid transaction.
   if (mTransaction.isNothing() || mTransaction.ref().mId != aTransactionId) {
     return;
   }
 
+  if (NS_WARN_IF(!mTransaction.ref().HasRegisterCallback())) {
+    RejectTransaction(NS_ERROR_ABORT);
+    return;
+  }
+
   CryptoBuffer clientDataBuf;
   if (NS_WARN_IF(!clientDataBuf.Assign(mTransaction.ref().mClientData))) {
     RejectTransaction(NS_ERROR_ABORT);
     return;
   }
 
   CryptoBuffer regBuf;
   if (NS_WARN_IF(!regBuf.Assign(aRegBuffer))) {
@@ -477,116 +479,129 @@ U2F::FinishMakeCredential(const uint64_t
 
   // Assemble a response object to return
   RegisterResponse response;
   response.mVersion.Construct(kRequiredU2FVersion);
   response.mClientData.Construct(clientDataBase64);
   response.mRegistrationData.Construct(registrationDataBase64);
   response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OK));
 
-  ExecuteCallback(response, mRegisterCallback);
+  // Keep the callback pointer alive.
+  nsMainThreadPtrHandle<U2FRegisterCallback> callback(
+    mTransaction.ref().GetRegisterCallback());
+
   ClearTransaction();
+  ExecuteCallback(response, callback);
 }
 
 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);
   }
 
-  MOZ_ASSERT(mSignCallback.isNothing());
-  mSignCallback = Some(nsMainThreadPtrHandle<U2FSignCallback>(
-                    new nsMainThreadPtrHolder<U2FSignCallback>(
-                        "U2F::Sign::callback", &aCallback)));
+  nsMainThreadPtrHandle<U2FSignCallback> callback(
+    new nsMainThreadPtrHolder<U2FSignCallback>("U2F::Sign::callback",
+                                               &aCallback));
 
-  uint32_t adjustedTimeoutMillis = AdjustedTimeoutMillis(opt_aTimeoutSeconds);
+  // Ensure we have a callback.
+  if (NS_WARN_IF(!callback)) {
+    return;
+  }
 
   // Evaluate the AppID
   nsString adjustedAppId;
   adjustedAppId.Assign(aAppId);
   ErrorCode appIdResult = EvaluateAppID(mParent, mOrigin, adjustedAppId);
   if (appIdResult != ErrorCode::OK) {
     SignResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(appIdResult));
-    ExecuteCallback(response, mSignCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   // Produce the AppParam from the current AppID
   nsCString cAppId = NS_ConvertUTF16toUTF8(adjustedAppId);
 
   nsAutoString clientDataJSON;
   nsresult rv = AssembleClientData(mOrigin, kGetAssertion, aChallenge,
                                    clientDataJSON);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     SignResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::BAD_REQUEST));
-    ExecuteCallback(response, mSignCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   // Build the key list, if any
   nsTArray<WebAuthnScopedCredentialDescriptor> permittedList;
   RegisteredKeysToScopedCredentialList(adjustedAppId, aRegisteredKeys,
                                        permittedList);
 
   auto clientData = NS_ConvertUTF16toUTF8(clientDataJSON);
 
   CryptoBuffer rpIdHash, clientDataHash;
   if (NS_FAILED(BuildTransactionHashes(cAppId, clientData,
                                        rpIdHash, clientDataHash))) {
     SignResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OTHER_ERROR));
-    ExecuteCallback(response, mSignCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   if (!MaybeCreateBackgroundActor()) {
     SignResponse response;
     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OTHER_ERROR));
-    ExecuteCallback(response, mSignCallback);
+    ExecuteCallback(response, callback);
     return;
   }
 
   ListenForVisibilityEvents();
 
   // Always blank for U2F
   nsTArray<WebAuthnExtension> extensions;
 
+  uint32_t adjustedTimeoutMillis = AdjustedTimeoutMillis(opt_aTimeoutSeconds);
+
   WebAuthnGetAssertionInfo info(rpIdHash,
                                 clientDataHash,
                                 adjustedTimeoutMillis,
                                 permittedList,
                                 extensions);
 
   MOZ_ASSERT(mTransaction.isNothing());
-  mTransaction = Some(U2FTransaction(clientData));
+  mTransaction = Some(U2FTransaction(clientData, Move(AsVariant(callback))));
   mChild->SendRequestSign(mTransaction.ref().mId, info);
 }
 
 void
 U2F::FinishGetAssertion(const uint64_t& aTransactionId,
                         nsTArray<uint8_t>& aCredentialId,
                         nsTArray<uint8_t>& aSigBuffer)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   // Check for a valid transaction.
   if (mTransaction.isNothing() || mTransaction.ref().mId != aTransactionId) {
     return;
   }
 
+  if (NS_WARN_IF(!mTransaction.ref().HasSignCallback())) {
+    RejectTransaction(NS_ERROR_ABORT);
+    return;
+  }
+
   CryptoBuffer clientDataBuf;
   if (NS_WARN_IF(!clientDataBuf.Assign(mTransaction.ref().mClientData))) {
     RejectTransaction(NS_ERROR_ABORT);
     return;
   }
 
   CryptoBuffer credBuf;
   if (NS_WARN_IF(!credBuf.Assign(aCredentialId))) {
@@ -615,50 +630,62 @@ U2F::FinishGetAssertion(const uint64_t& 
   }
 
   SignResponse response;
   response.mKeyHandle.Construct(keyHandleBase64);
   response.mClientData.Construct(clientDataBase64);
   response.mSignatureData.Construct(signatureDataBase64);
   response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OK));
 
-  ExecuteCallback(response, mSignCallback);
+  // Keep the callback pointer alive.
+  nsMainThreadPtrHandle<U2FSignCallback> callback(
+    mTransaction.ref().GetSignCallback());
+
   ClearTransaction();
+  ExecuteCallback(response, callback);
 }
 
 void
 U2F::ClearTransaction()
 {
   if (!NS_WARN_IF(mTransaction.isNothing())) {
     StopListeningForVisibilityEvents();
   }
 
   mTransaction.reset();
 }
 
 void
 U2F::RejectTransaction(const nsresult& aError)
 {
-  if (!NS_WARN_IF(mTransaction.isNothing())) {
-    ErrorCode code = ConvertNSResultToErrorCode(aError);
-
-    if (mRegisterCallback.isSome()) {
-      RegisterResponse response;
-      response.mErrorCode.Construct(static_cast<uint32_t>(code));
-      ExecuteCallback(response, mRegisterCallback);
-    }
-
-    if (mSignCallback.isSome()) {
-      SignResponse response;
-      response.mErrorCode.Construct(static_cast<uint32_t>(code));
-      ExecuteCallback(response, mSignCallback);
-    }
+  if (NS_WARN_IF(mTransaction.isNothing())) {
+    return;
   }
 
-  ClearTransaction();
+  StopListeningForVisibilityEvents();
+
+  // Clear out mTransaction before calling ExecuteCallback() below to allow
+  // reentrancy from microtask checkpoints.
+  Maybe<U2FTransaction> maybeTransaction(Move(mTransaction));
+  MOZ_ASSERT(mTransaction.isNothing() && maybeTransaction.isSome());
+
+  U2FTransaction& transaction = maybeTransaction.ref();
+  ErrorCode code = ConvertNSResultToErrorCode(aError);
+
+  if (transaction.HasRegisterCallback()) {
+    RegisterResponse response;
+    response.mErrorCode.Construct(static_cast<uint32_t>(code));
+    ExecuteCallback(response, transaction.GetRegisterCallback());
+  }
+
+  if (transaction.HasSignCallback()) {
+    SignResponse response;
+    response.mErrorCode.Construct(static_cast<uint32_t>(code));
+    ExecuteCallback(response, transaction.GetSignCallback());
+  }
 }
 
 void
 U2F::CancelTransaction(const nsresult& aError)
 {
   if (!NS_WARN_IF(!mChild || mTransaction.isNothing())) {
     mChild->SendRequestCancel(mTransaction.ref().mId);
   }
--- a/dom/u2f/U2F.h
+++ b/dom/u2f/U2F.h
@@ -26,27 +26,51 @@ class U2FRegisterCallback;
 class U2FSignCallback;
 
 // Defined in U2FBinding.h by the U2F.webidl; their use requires a JSContext.
 struct RegisterRequest;
 struct RegisteredKey;
 
 class U2FTransaction
 {
+  typedef Variant<nsMainThreadPtrHandle<U2FRegisterCallback>,
+                  nsMainThreadPtrHandle<U2FSignCallback>> U2FCallback;
+
 public:
-  explicit U2FTransaction(const nsCString& aClientData)
+  explicit U2FTransaction(const nsCString& aClientData,
+                          const U2FCallback&& aCallback)
     : mClientData(aClientData)
+    , mCallback(Move(aCallback))
     , mId(NextId())
   {
     MOZ_ASSERT(mId > 0);
   }
 
+  bool HasRegisterCallback() {
+    return mCallback.is<nsMainThreadPtrHandle<U2FRegisterCallback>>();
+  }
+
+  auto& GetRegisterCallback() {
+    return mCallback.as<nsMainThreadPtrHandle<U2FRegisterCallback>>();
+  }
+
+  bool HasSignCallback() {
+    return mCallback.is<nsMainThreadPtrHandle<U2FSignCallback>>();
+  }
+
+  auto& GetSignCallback() {
+    return mCallback.as<nsMainThreadPtrHandle<U2FSignCallback>>();
+  }
+
   // Client data used to assemble reply objects.
   nsCString mClientData;
 
+  // The callback passed to the API.
+  U2FCallback mCallback;
+
   // Unique transaction id.
   uint64_t mId;
 
 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() {
@@ -112,27 +136,26 @@ public:
 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;
 
 private:
   ~U2F();
 
+  template<typename T, typename C>
+  void ExecuteCallback(T& aResp, nsMainThreadPtrHandle<C>& aCb);
+
   // Clears all information we have about the current transaction.
   void ClearTransaction();
-  // Rejects the current transaction and calls ClearTransaction().
+  // Rejects the current transaction and clears it.
   void RejectTransaction(const nsresult& aError);
 
   nsString mOrigin;
 
-  // U2F API callbacks.
-  Maybe<nsMainThreadPtrHandle<U2FRegisterCallback>> mRegisterCallback;
-  Maybe<nsMainThreadPtrHandle<U2FSignCallback>> mSignCallback;
-
   // The current transaction, if any.
   Maybe<U2FTransaction> mTransaction;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_U2F_h