bug 1499766 - rework the threading model of OSKeyStore r=jcj
authorDana Keeler <dkeeler@mozilla.com>
Tue, 23 Oct 2018 01:14:14 +0000
changeset 490841 f3273aa382dca7f8149d9839603635d183dc2cf5
parent 490840 147e4ace74cc3035a8f995b043529d46ab77ae17
child 490842 d9e373554f5125fb413029b85a5123b4b2ff42e0
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersjcj
bugs1499766
milestone65.0a1
bug 1499766 - rework the threading model of OSKeyStore r=jcj The original threading model of OSKeyStore could lead to a deadlock if an asynchronous event were dispatched and then the isNSSKeyStore attribute were queried. This patch removes that pitfall by moving the determination of the attribute to OSKeyStore rather than the underlying implementation. Additionally, the original threading model was inefficient in that it created and destroyed a thread per asynchronous operation. This patch reworks this to only ever create one worker thread. Differential Revision: https://phabricator.services.mozilla.com/D9299
security/manager/ssl/NSSKeyStore.cpp
security/manager/ssl/NSSKeyStore.h
security/manager/ssl/OSKeyStore.cpp
security/manager/ssl/OSKeyStore.h
security/manager/ssl/tests/unit/test_oskeystore.js
--- a/security/manager/ssl/NSSKeyStore.cpp
+++ b/security/manager/ssl/NSSKeyStore.cpp
@@ -27,22 +27,16 @@ NSSKeyStore::NSSKeyStore()
     // OSKeyStore, which is ParentProcessOnly.
     return;
   }
   Unused << EnsureNSSInitializedChromeOrContent();
   Unused << InitToken();
 }
 NSSKeyStore::~NSSKeyStore() {}
 
-bool
-NSSKeyStore::IsNSSKeyStore()
-{
-  return true;
-}
-
 nsresult
 NSSKeyStore::InitToken()
 {
   if (!mSlot) {
     mSlot = UniquePK11SlotInfo(PK11_GetInternalKeySlot());
     if (!mSlot) {
       MOZ_LOG(
         gNSSKeyStoreLog, LogLevel::Debug, ("Error getting internal key slot"));
--- a/security/manager/ssl/NSSKeyStore.h
+++ b/security/manager/ssl/NSSKeyStore.h
@@ -22,17 +22,16 @@ public:
   virtual nsresult DeleteSecret(const nsACString& label) override;
   virtual nsresult Lock() override;
   virtual nsresult Unlock() override;
   virtual nsresult EncryptDecrypt(const nsACString& label,
                                   const std::vector<uint8_t>& inBytes,
                                   std::vector<uint8_t>& outBytes,
                                   bool encrypt) override;
   virtual bool SecretAvailable(const nsACString& label) override;
-  virtual bool IsNSSKeyStore() override;
   virtual ~NSSKeyStore();
 
 private:
   nsresult InitToken();
   UniquePK11SlotInfo mSlot = nullptr;
 };
 
 #endif // NSSKeyStore_h
--- a/security/manager/ssl/OSKeyStore.cpp
+++ b/security/manager/ssl/OSKeyStore.cpp
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "OSKeyStore.h"
 
 #include "mozilla/Base64.h"
 #include "mozilla/dom/Promise.h"
 #include "nsIRandomGenerator.h"
+#include "nsXPCOM.h"
 #include "pk11pub.h"
 
 #ifdef MOZ_LIB_SECRET
 #include "LibSecret.h"
 #elif defined(XP_MACOSX)
 #include "KeychainSecret.h"
 #elif defined(XP_WIN)
 #include "CredentialManagerSecret.h"
@@ -24,29 +25,74 @@
 NS_IMPL_ISUPPORTS(OSKeyStore, nsIOSKeyStore)
 
 using namespace mozilla;
 using dom::Promise;
 
 mozilla::LazyLogModule gOSKeyStoreLog("oskeystore");
 
 OSKeyStore::OSKeyStore()
-  : mMutex("OSKeyStore-mutex")
+ : mKs(nullptr)
+ , mKsThread(nullptr)
+ , mKsIsNSSKeyStore(false)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (NS_WARN_IF(!NS_IsMainThread())) {
+    return;
+  }
+
 #ifdef MOZ_LIB_SECRET
   mKs.reset(new LibSecret());
 #elif defined(XP_MACOSX)
   mKs.reset(new KeychainSecret());
 #elif defined(XP_WIN)
   mKs.reset(new CredentialManagerSecret());
 #else
   mKs.reset(new NSSKeyStore());
+  mKsIsNSSKeyStore = true;
 #endif
+
+  nsCOMPtr<nsIThread> thread;
+  nsresult rv = NS_NewNamedThread("OSKeyStore", getter_AddRefs(thread));
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    mKs = nullptr;
+    return;
+  }
+  mKsThread = thread;
+
+  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
+  if (NS_WARN_IF(!obs)) {
+    mKsThread = nullptr;
+    mKs = nullptr;
+    return;
+  }
+  rv = obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    mKsThread = nullptr;
+    mKs = nullptr;
+  }
 }
-OSKeyStore::~OSKeyStore() {}
+
+NS_IMETHODIMP
+OSKeyStore::Observe(nsISupports*, const char* aTopic, const char16_t*)
+{
+  MOZ_ASSERT(!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID));
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
+  if (mKsThread) {
+    mKsThread->Shutdown();
+    mKsThread = nullptr;
+    mKs = nullptr;
+  }
+
+  return NS_OK;
+}
 
 static nsresult
 GenerateRandom(std::vector<uint8_t>& r)
 {
   if (r.size() < 1) {
     return NS_ERROR_INVALID_ARG;
   }
   UniquePK11SlotInfo slot(PK11_GetInternalSlot());
@@ -62,28 +108,26 @@ GenerateRandom(std::vector<uint8_t>& r)
 
   return NS_OK;
 }
 
 nsresult
 OSKeyStore::SecretAvailable(const nsACString& aLabel,
                             /* out */ bool* aAvailable)
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   nsAutoCString label = mLabelPrefix + aLabel;
   *aAvailable = mKs->SecretAvailable(label);
   return NS_OK;
 }
 
 nsresult
 OSKeyStore::GenerateSecret(const nsACString& aLabel,
                            /* out */ nsACString& aRecoveryPhrase)
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   size_t keyByteLength = mKs->GetKeyByteLength();
   std::vector<uint8_t> secret(keyByteLength);
   nsresult rv = GenerateRandom(secret);
   if (NS_FAILED(rv) || secret.size() != keyByteLength) {
     return NS_ERROR_FAILURE;
   }
   nsAutoCString secretString;
@@ -105,17 +149,16 @@ OSKeyStore::GenerateSecret(const nsACStr
   aRecoveryPhrase = base64;
   return NS_OK;
 }
 
 nsresult
 OSKeyStore::RecoverSecret(const nsACString& aLabel,
                           const nsACString& aRecoveryPhrase)
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   nsAutoCString secret;
   nsresult rv = Base64Decode(aRecoveryPhrase, secret);
   if (NS_FAILED(rv)) {
     return rv;
   }
   nsAutoCString label = mLabelPrefix + aLabel;
   rv = mKs->StoreSecret(secret, label);
@@ -124,17 +167,16 @@ OSKeyStore::RecoverSecret(const nsACStri
   }
 
   return NS_OK;
 }
 
 nsresult
 OSKeyStore::DeleteSecret(const nsACString& aLabel)
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   nsAutoCString label = mLabelPrefix + aLabel;
   return mKs->DeleteSecret(label);
 }
 
 enum Cipher
 {
   Encrypt = true,
@@ -142,17 +184,16 @@ enum Cipher
 };
 
 nsresult
 OSKeyStore::EncryptBytes(const nsACString& aLabel,
                          uint32_t inLen,
                          uint8_t* inBytes,
                          /*out*/ nsACString& aEncryptedBase64Text)
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   NS_ENSURE_ARG_POINTER(inBytes);
 
   nsAutoCString label = mLabelPrefix + aLabel;
   aEncryptedBase64Text.Truncate();
   const std::vector<uint8_t> in(inBytes, inBytes + inLen);
   std::vector<uint8_t> outBytes;
   nsresult rv = mKs->EncryptDecrypt(label, in, outBytes, Cipher::Encrypt);
@@ -173,17 +214,16 @@ OSKeyStore::EncryptBytes(const nsACStrin
 }
 
 nsresult
 OSKeyStore::DecryptBytes(const nsACString& aLabel,
                          const nsACString& aEncryptedBase64Text,
                          /*out*/ uint32_t* outLen,
                          /*out*/ uint8_t** outBytes)
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   NS_ENSURE_ARG_POINTER(outLen);
   NS_ENSURE_ARG_POINTER(outBytes);
   *outLen = 0;
   *outBytes = nullptr;
 
   nsAutoCString ciphertext;
   nsresult rv = Base64Decode(aEncryptedBase64Text, ciphertext);
@@ -204,36 +244,32 @@ OSKeyStore::DecryptBytes(const nsACStrin
   memcpy(*outBytes, plaintextBytes.data(), plaintextBytes.size());
   *outLen = plaintextBytes.size();
   return NS_OK;
 }
 
 nsresult
 OSKeyStore::Lock()
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   return mKs->Lock();
 }
 
 nsresult
 OSKeyStore::Unlock()
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_STATE(mKs);
   return mKs->Unlock();
 }
 
 NS_IMETHODIMP
 OSKeyStore::GetIsNSSKeyStore(bool* aNSSKeyStore)
 {
-  MutexAutoLock lock(mMutex);
   NS_ENSURE_ARG_POINTER(aNSSKeyStore);
-  NS_ENSURE_STATE(mKs);
-  *aNSSKeyStore = mKs->IsNSSKeyStore();
+  *aNSSKeyStore = mKsIsNSSKeyStore;
   return NS_OK;
 }
 
 // Async interfaces that return promises because the key store implementation
 // might block, e.g. asking for a password.
 
 nsresult
 GetPromise(JSContext* aCx, /* out */ RefPtr<Promise>& aPromise)
@@ -245,34 +281,16 @@ GetPromise(JSContext* aCx, /* out */ Ref
   ErrorResult result;
   aPromise = Promise::Create(globalObject, result);
   if (NS_WARN_IF(result.Failed())) {
     return result.StealNSResult();
   }
   return NS_OK;
 }
 
-nsresult
-OSKeyStore::FinishAsync(RefPtr<Promise>& aPromiseHandle,
-                        /* out*/ Promise** promiseOut,
-                        const nsACString& aName,
-                        nsCOMPtr<nsIRunnable> aRunnable)
-{
-  // Note that if the NSS PK11 token is handling the key store, locking and
-  // unlocking functions will be pushed to the main thread again.
-  nsCOMPtr<nsIThread> thread;
-  nsresult rv = NS_NewNamedThread(aName, getter_AddRefs(thread), aRunnable);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  aPromiseHandle.forget(promiseOut);
-  return NS_OK;
-}
-
 void
 BackgroundUnlock(RefPtr<Promise>& aPromise, RefPtr<OSKeyStore> self)
 {
   nsAutoCString recovery;
   nsresult rv = self->Unlock();
   nsCOMPtr<nsIRunnable> runnable(NS_NewRunnableFunction(
     "BackgroundUnlockOSKSResolve", [rv, aPromise = std::move(aPromise)]() {
       if (NS_FAILED(rv)) {
@@ -282,32 +300,38 @@ BackgroundUnlock(RefPtr<Promise>& aPromi
       }
     }));
   NS_DispatchToMainThread(runnable.forget());
 }
 
 NS_IMETHODIMP
 OSKeyStore::AsyncUnlock(JSContext* aCx, Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
   nsCOMPtr<nsIRunnable> runnable(
     NS_NewRunnableFunction("BackgroundUnlock", [self, promiseHandle]() mutable {
       BackgroundUnlock(promiseHandle, self);
     }));
 
-  return FinishAsync(
-    promiseHandle, promiseOut, NS_LITERAL_CSTRING("UnlockKSThread"), runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 void
 BackgroundLock(RefPtr<Promise>& aPromise, RefPtr<OSKeyStore> self)
 {
   nsresult rv = self->Lock();
   nsCOMPtr<nsIRunnable> runnable(NS_NewRunnableFunction(
     "BackgroundLockOSKSResolve", [rv, aPromise = std::move(aPromise)]() {
@@ -318,32 +342,38 @@ BackgroundLock(RefPtr<Promise>& aPromise
       }
     }));
   NS_DispatchToMainThread(runnable.forget());
 }
 
 NS_IMETHODIMP
 OSKeyStore::AsyncLock(JSContext* aCx, Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
   nsCOMPtr<nsIRunnable> runnable(
     NS_NewRunnableFunction("BackgroundLock", [self, promiseHandle]() mutable {
       BackgroundLock(promiseHandle, self);
     }));
 
-  return FinishAsync(
-    promiseHandle, promiseOut, NS_LITERAL_CSTRING("LockKSThread"), runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 void
 BackgroundGenerateSecret(const nsACString& aLabel,
                          RefPtr<Promise>& aPromise,
                          RefPtr<OSKeyStore> self)
 {
   nsAutoCString recovery;
@@ -364,35 +394,39 @@ BackgroundGenerateSecret(const nsACStrin
   NS_DispatchToMainThread(runnable.forget());
 }
 
 NS_IMETHODIMP
 OSKeyStore::AsyncGenerateSecret(const nsACString& aLabel,
                                 JSContext* aCx,
                                 Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
   nsCOMPtr<nsIRunnable> runnable(NS_NewRunnableFunction(
     "BackgroundGenerateSecret",
     [self, promiseHandle, aLabel = nsAutoCString(aLabel)]() mutable {
       BackgroundGenerateSecret(aLabel, promiseHandle, self);
     }));
 
-  return FinishAsync(promiseHandle,
-                     promiseOut,
-                     NS_LITERAL_CSTRING("GenerateKSThread"),
-                     runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 void
 BackgroundSecretAvailable(const nsACString& aLabel,
                           RefPtr<Promise>& aPromise,
                           RefPtr<OSKeyStore> self)
 {
   bool available = false;
@@ -409,35 +443,39 @@ BackgroundSecretAvailable(const nsACStri
   NS_DispatchToMainThread(runnable.forget());
 }
 
 NS_IMETHODIMP
 OSKeyStore::AsyncSecretAvailable(const nsACString& aLabel,
                                  JSContext* aCx,
                                  Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
   nsCOMPtr<nsIRunnable> runnable(NS_NewRunnableFunction(
     "BackgroundSecretAvailable",
     [self, promiseHandle, aLabel = nsAutoCString(aLabel)]() mutable {
       BackgroundSecretAvailable(aLabel, promiseHandle, self);
     }));
 
-  return FinishAsync(promiseHandle,
-                     promiseOut,
-                     NS_LITERAL_CSTRING("AvaiableKSThread"),
-                     runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 void
 BackgroundRecoverSecret(const nsACString& aLabel,
                         const nsACString& aRecoveryPhrase,
                         RefPtr<Promise>& aPromise,
                         RefPtr<OSKeyStore> self)
 {
@@ -455,17 +493,23 @@ BackgroundRecoverSecret(const nsACString
 }
 
 NS_IMETHODIMP
 OSKeyStore::AsyncRecoverSecret(const nsACString& aLabel,
                                const nsACString& aRecoveryPhrase,
                                JSContext* aCx,
                                Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
@@ -473,18 +517,18 @@ OSKeyStore::AsyncRecoverSecret(const nsA
     "BackgroundRecoverSecret",
     [self,
      promiseHandle,
      aLabel = nsAutoCString(aLabel),
      aRecoveryPhrase = nsAutoCString(aRecoveryPhrase)]() mutable {
       BackgroundRecoverSecret(aLabel, aRecoveryPhrase, promiseHandle, self);
     }));
 
-  return FinishAsync(
-    promiseHandle, promiseOut, NS_LITERAL_CSTRING("RecoverKSThread"), runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 void
 BackgroundDeleteSecret(const nsACString& aLabel,
                        RefPtr<Promise>& aPromise,
                        RefPtr<OSKeyStore> self)
 {
   nsresult rv = self->DeleteSecret(aLabel);
@@ -500,33 +544,39 @@ BackgroundDeleteSecret(const nsACString&
   NS_DispatchToMainThread(runnable.forget());
 }
 
 NS_IMETHODIMP
 OSKeyStore::AsyncDeleteSecret(const nsACString& aLabel,
                               JSContext* aCx,
                               Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
   nsCOMPtr<nsIRunnable> runnable(NS_NewRunnableFunction(
     "BackgroundDeleteSecret",
     [self, promiseHandle, aLabel = nsAutoCString(aLabel)]() mutable {
       BackgroundDeleteSecret(aLabel, promiseHandle, self);
     }));
 
-  return FinishAsync(
-    promiseHandle, promiseOut, NS_LITERAL_CSTRING("DeleteKSThread"), runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 void
 BackgroundEncryptBytes(const nsACString& aLabel,
                        std::vector<uint8_t> inBytes,
                        RefPtr<Promise>& aPromise,
                        RefPtr<OSKeyStore> self)
 {
@@ -550,18 +600,24 @@ BackgroundEncryptBytes(const nsACString&
 
 NS_IMETHODIMP
 OSKeyStore::AsyncEncryptBytes(const nsACString& aLabel,
                               uint32_t inLen,
                               uint8_t* inBytes,
                               JSContext* aCx,
                               Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
   NS_ENSURE_ARG_POINTER(inBytes);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
@@ -569,18 +625,18 @@ OSKeyStore::AsyncEncryptBytes(const nsAC
     "BackgroundEncryptBytes",
     [promiseHandle,
      inBytes = std::vector<uint8_t>(inBytes, inBytes + inLen),
      aLabel = nsAutoCString(aLabel),
      self]() mutable {
       BackgroundEncryptBytes(aLabel, inBytes, promiseHandle, self);
     }));
 
-  return FinishAsync(
-    promiseHandle, promiseOut, NS_LITERAL_CSTRING("EncryptKSThread"), runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 void
 BackgroundDecryptBytes(const nsACString& aLabel,
                        const nsACString& aEncryptedBase64Text,
                        RefPtr<Promise>& aPromise,
                        RefPtr<OSKeyStore> self)
 {
@@ -608,17 +664,23 @@ BackgroundDecryptBytes(const nsACString&
 }
 
 NS_IMETHODIMP
 OSKeyStore::AsyncDecryptBytes(const nsACString& aLabel,
                               const nsACString& aEncryptedBase64Text,
                               JSContext* aCx,
                               Promise** promiseOut)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+
   NS_ENSURE_ARG_POINTER(aCx);
+  NS_ENSURE_STATE(mKsThread);
 
   RefPtr<Promise> promiseHandle;
   nsresult rv = GetPromise(aCx, promiseHandle);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   RefPtr<OSKeyStore> self = this;
@@ -626,18 +688,18 @@ OSKeyStore::AsyncDecryptBytes(const nsAC
     "BackgroundDecryptBytes",
     [promiseHandle,
      self,
      aEncryptedBase64Text = nsAutoCString(aEncryptedBase64Text),
      aLabel = nsAutoCString(aLabel)]() mutable {
       BackgroundDecryptBytes(aLabel, aEncryptedBase64Text, promiseHandle, self);
     }));
 
-  return FinishAsync(
-    promiseHandle, promiseOut, NS_LITERAL_CSTRING("DecryptKSThread"), runnable);
+  promiseHandle.forget(promiseOut);
+  return mKsThread->Dispatch(runnable.forget());
 }
 
 // Generic AES-GCM cipher wrapper for NSS functions.
 
 nsresult
 AbstractOSKeyStore::BuildAesGcmKey(std::vector<uint8_t> aKeyBytes,
                                    /* out */ UniquePK11SymKey& aKey)
 {
@@ -751,22 +813,16 @@ AbstractOSKeyStore::DoCipher(const Uniqu
   if (outLen < outBytes.size()) {
     outBytes.resize(outLen);
   }
 
   return NS_OK;
 }
 
 bool
-AbstractOSKeyStore::IsNSSKeyStore()
-{
-  return false;
-}
-
-bool
 AbstractOSKeyStore::SecretAvailable(const nsACString& aLabel)
 {
   nsAutoCString secret;
   nsresult rv = RetrieveSecret(aLabel, secret);
   if (NS_FAILED(rv) || secret.Length() == 0) {
     return false;
   }
   return true;
--- a/security/manager/ssl/OSKeyStore.h
+++ b/security/manager/ssl/OSKeyStore.h
@@ -26,18 +26,16 @@ public:
   virtual nsresult StoreSecret(const nsACString& secret,
                                const nsACString& label) = 0;
   // Delete the secret with the given label.
   virtual nsresult DeleteSecret(const nsACString& label) = 0;
   // Lock the key store.
   virtual nsresult Lock() = 0;
   // Unlock the key store.
   virtual nsresult Unlock() = 0;
-  // Identify the fallback NSS key store.
-  virtual bool IsNSSKeyStore();
   virtual ~AbstractOSKeyStore() {}
 
   // Returns true if the secret with the given label is available in the key
   // store, false otherwise.
   virtual bool SecretAvailable(const nsACString& label);
   // Perform encryption or decryption operation with the given secret and input
   // bytes. The output is written in outBytes. This function can make use of the
   // AesGcm class to use NSS for encryption and decryption.
@@ -65,20 +63,22 @@ private:
 };
 
 #define NS_OSKEYSTORE_CONTRACTID "@mozilla.org/security/oskeystore;1"
 #define NS_OSKEYSTORE_CID \
   { 0x57972956, 0x5718, 0x42d2, { 0x80, 0x70, 0xb3, 0xfc, 0x72, 0x21, 0x2e, 0xaf } }
 
 nsresult GetPromise(JSContext* aCx, /* out */ RefPtr<mozilla::dom::Promise>& aPromise);
 
-class OSKeyStore : public nsIOSKeyStore
+class OSKeyStore final : public nsIOSKeyStore
+                       , public nsIObserver
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSIOBSERVER
   NS_DECL_NSIOSKEYSTORE
 
   OSKeyStore();
   nsresult GenerateSecret(const nsACString& aLabel,
                           /* out */ nsACString& aRecoveryPhrase);
   nsresult SecretAvailable(const nsACString& aLabel,
                            /* out */ bool* aAvailable);
   nsresult RecoverSecret(const nsACString& aLabel,
@@ -90,24 +90,31 @@ public:
                         /*out*/ nsACString& aEncryptedBase64Text);
   nsresult DecryptBytes(const nsACString& aLabel,
                         const nsACString& aEncryptedBase64Text,
                         /*out*/ uint32_t* outLen,
                         /*out*/ uint8_t** outBytes);
   nsresult Lock();
   nsresult Unlock();
 
-protected:
-  virtual ~OSKeyStore();
+private:
+  ~OSKeyStore() = default;
 
-private:
-  nsresult FinishAsync(RefPtr<mozilla::dom::Promise>& aPromise,
-                       /* out*/ mozilla::dom::Promise** anotherPromise,
-                       const nsACString& aName,
-                       nsCOMPtr<nsIRunnable> aRunnable);
+  // Thread safety for OSKeyStore:
+  // * mKsThread is only accessed on the main thread
+  // * mKsThread is shut down on the main thread when OSKeyStore receives the
+  //   "xpcom-shutdown" notification
+  // * mKs is created and destroyed on the main thread, but is only accessed on
+  //   mKsThread
+  // * XPCOM notifies "xpcom-shutdown" before shutting down the component
+  //   manager, so mKsThread is shut down before mKs is destroyed
+  // Essentially, the lifetime of mKsThread is contained by the lifetime of mKs.
+  // Since the value of mKs never changes while mKsThread is alive and any uses
+  // of it are serial, there shouldn't be any memory safety issues.
+  std::unique_ptr<AbstractOSKeyStore> mKs;
+  nsCOMPtr<nsIThread> mKsThread;
 
-  std::unique_ptr<AbstractOSKeyStore> mKs = nullptr;
-  Mutex mMutex;
+  bool mKsIsNSSKeyStore;
   const nsCString mLabelPrefix =
     NS_LITERAL_CSTRING("org.mozilla.nss.keystore.");
 };
 
 #endif // OSKeyStore_h
--- a/security/manager/ssl/tests/unit/test_oskeystore.js
+++ b/security/manager/ssl/tests/unit/test_oskeystore.js
@@ -131,8 +131,33 @@ add_task(async function() {
 
     // Set the correct password so that the test operations should succeed.
     gMockPrompter.passwordToTry = "hunter2";
     await encrypt_decrypt_test();
     ok(gMockPrompter.numPrompts == 1, "There should've been one password prompt.");
     await delete_all_secrets();
   }
 });
+
+// Test that if we kick off a background operation and then call a synchronous function on the
+// keystore, we don't deadlock.
+add_task(async function() {
+  await delete_all_secrets();
+
+  let keystore = Cc["@mozilla.org/security/oskeystore;1"]
+                   .getService(Ci.nsIOSKeyStore);
+  let recoveryPhrase = await keystore.asyncGenerateSecret(LABELS[0]);
+  ok(recoveryPhrase, "A recovery phrase should've been created.");
+
+  try {
+    let text = new Uint8Array(8192);
+    let promise = keystore.asyncEncryptBytes(LABELS[0], text.length, text);
+    /* eslint-disable no-unused-expressions */
+    keystore.isNSSKeyStore; // we don't care what this is - we just need to access it
+    /* eslint-enable no-unused-expressions */
+    let ciphertext = await promise;
+    ok(ciphertext, "We should have a ciphertext now.");
+  } catch (e) {
+    ok(false, "Error encrypting " + e);
+  }
+
+  await delete_all_secrets();
+});