Bug 1555557 - Do cert override file writes off the main thread. r=keeler

Differential Revision: https://phabricator.services.mozilla.com/D35375

Author: nhnt11

security/manager/ssl/DataStorage.cpp

@@ -998,7 +998,7 @@ DataStorage::Writer::Run() {
   }
 
   const char* ptr = mData.get();
-  int32_t remaining = mData.Length();
+  uint32_t remaining = mData.Length();
   uint32_t written = 0;
   while (remaining > 0) {
     rv = outputStream->Write(ptr, remaining, &written);

security/manager/ssl/nsCertOverrideService.cpp

@@ -10,6 +10,7 @@
 #include "ScopedNSSTypes.h"
 #include "SharedSSLState.h"
 #include "mozilla/Assertions.h"
+#include "mozilla/TaskQueue.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/TextUtils.h"
 #include "mozilla/Unused.h"
@@ -38,6 +39,50 @@ using namespace mozilla::psm;
 
 #define CERT_OVERRIDE_FILE_NAME "cert_override.txt"
 
+class WriterRunnable : public Runnable {
+ public:
+  WriterRunnable(nsCertOverrideService* aService, nsCString& aData,
+                 nsCOMPtr<nsIFile> aFile)
+      : Runnable("nsCertOverrideService::WriterRunnable"),
+        mCertOverrideService(aService),
+        mData(aData),
+        mFile(std::move(aFile)) {}
+
+  NS_IMETHOD
+  Run() override {
+    mCertOverrideService->AssertOnTaskQueue();
+    nsresult rv;
+
+    nsCOMPtr<nsIOutputStream> outputStream;
+    rv = NS_NewSafeLocalFileOutputStream(
+        getter_AddRefs(outputStream), mFile,
+        PR_CREATE_FILE | PR_TRUNCATE | PR_WRONLY);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    const char* ptr = mData.get();
+    uint32_t remaining = mData.Length();
+    uint32_t written = 0;
+    while (remaining > 0) {
+      rv = outputStream->Write(ptr, remaining, &written);
+      NS_ENSURE_SUCCESS(rv, rv);
+      remaining -= written;
+      ptr += written;
+    }
+
+    nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(outputStream);
+    MOZ_ASSERT(safeStream);
+    rv = safeStream->Finish();
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    return NS_OK;
+  }
+
+ private:
+  const RefPtr<nsCertOverrideService> mCertOverrideService;
+  nsCString mData;
+  const nsCOMPtr<nsIFile> mFile;
+};
+
 void nsCertOverride::convertBitsToString(OverrideBits ob,
                                          /*out*/ nsACString& str) {
   str.Truncate();
@@ -83,19 +128,44 @@ void nsCertOverride::convertStringToBits(const nsACString& str,
 }
 
 NS_IMPL_ISUPPORTS(nsCertOverrideService, nsICertOverrideService, nsIObserver,
-                  nsISupportsWeakReference)
+                  nsISupportsWeakReference, nsIAsyncShutdownBlocker)
 
 nsCertOverrideService::nsCertOverrideService()
-    : mDisableAllSecurityCheck(false), mMutex("nsCertOverrideService.mutex") {}
+    : mDisableAllSecurityCheck(false), mMutex("nsCertOverrideService.mutex") {
+  nsCOMPtr<nsIEventTarget> target =
+      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
+  MOZ_ASSERT(target);
+
+  mWriterTaskQueue = new TaskQueue(target.forget());
+}
 
 nsCertOverrideService::~nsCertOverrideService() = default;
 
+static nsCOMPtr<nsIAsyncShutdownClient> GetShutdownBarrier() {
+  nsCOMPtr<nsIAsyncShutdownService> svc =
+      mozilla::services::GetAsyncShutdownService();
+  MOZ_RELEASE_ASSERT(svc);
+
+  nsCOMPtr<nsIAsyncShutdownClient> barrier;
+  nsresult rv = svc->GetProfileBeforeChange(getter_AddRefs(barrier));
+
+  MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));
+  MOZ_RELEASE_ASSERT(barrier);
+  return barrier;
+}
+
 nsresult nsCertOverrideService::Init() {
   if (!NS_IsMainThread()) {
     MOZ_ASSERT_UNREACHABLE("nsCertOverrideService initialized off main thread");
     return NS_ERROR_NOT_SAME_THREAD;
   }
 
+  nsresult rv = GetShutdownBarrier()->AddBlocker(
+      this, NS_LITERAL_STRING(__FILE__), __LINE__,
+      NS_LITERAL_STRING("nsCertOverrideService shutdown"));
+  // If that failed, we're likely getting initialized during shutdown.
+  NS_ENSURE_SUCCESS(rv, rv);
+
   nsCOMPtr<nsIObserverService> observerService =
       mozilla::services::GetObserverService();
 
@@ -253,31 +323,15 @@ nsresult nsCertOverrideService::Write(const MutexAutoLock& aProofOfLock) {
     return NS_OK;
   }
 
-  nsresult rv;
-  nsCOMPtr<nsIOutputStream> fileOutputStream;
-  rv = NS_NewSafeLocalFileOutputStream(getter_AddRefs(fileOutputStream),
-                                       mSettingsFile, -1, 0600);
-  if (NS_FAILED(rv)) {
-    NS_ERROR("failed to open cert_warn_settings.txt for writing");
-    return rv;
-  }
-
-  // get a buffered output stream 4096 bytes big, to optimize writes
-  nsCOMPtr<nsIOutputStream> bufferedOutputStream;
-  rv = NS_NewBufferedOutputStream(getter_AddRefs(bufferedOutputStream),
-                                  fileOutputStream.forget(), 4096);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
+  nsCString output;
 
   static const char kHeader[] =
       "# PSM Certificate Override Settings file" NS_LINEBREAK
       "# This is a generated file!  Do not edit." NS_LINEBREAK;
 
   /* see ::Read for file format */
 
-  uint32_t unused;
-  bufferedOutputStream->Write(kHeader, sizeof(kHeader) - 1, &unused);
+  output.Append(kHeader);
 
   static const char kTab[] = "\t";
   for (auto iter = mSettingsTable.Iter(); !iter.Done(); iter.Next()) {
@@ -291,34 +345,29 @@ nsresult nsCertOverrideService::Write(const MutexAutoLock& aProofOfLock) {
     nsAutoCString bits_string;
     nsCertOverride::convertBitsToString(settings.mOverrideBits, bits_string);
 
-    bufferedOutputStream->Write(entry->mHostWithPort.get(),
-                                entry->mHostWithPort.Length(), &unused);
-    bufferedOutputStream->Write(kTab, sizeof(kTab) - 1, &unused);
-    bufferedOutputStream->Write(sSHA256OIDString, sizeof(sSHA256OIDString) - 1,
-                                &unused);
-    bufferedOutputStream->Write(kTab, sizeof(kTab) - 1, &unused);
-    bufferedOutputStream->Write(settings.mFingerprint.get(),
-                                settings.mFingerprint.Length(), &unused);
-    bufferedOutputStream->Write(kTab, sizeof(kTab) - 1, &unused);
-    bufferedOutputStream->Write(bits_string.get(), bits_string.Length(),
-                                &unused);
-    bufferedOutputStream->Write(kTab, sizeof(kTab) - 1, &unused);
-    bufferedOutputStream->Write(settings.mDBKey.get(), settings.mDBKey.Length(),
-                                &unused);
-    bufferedOutputStream->Write(NS_LINEBREAK, NS_LINEBREAK_LEN, &unused);
-  }
-
-  // All went ok. Maybe except for problems in Write(), but the stream detects
-  // that for us
-  nsCOMPtr<nsISafeOutputStream> safeStream =
-      do_QueryInterface(bufferedOutputStream);
-  MOZ_ASSERT(safeStream, "Expected a safe output stream!");
-  if (safeStream) {
-    rv = safeStream->Finish();
-    if (NS_FAILED(rv)) {
-      NS_WARNING("failed to save cert warn settings file! possible dataloss");
-      return rv;
-    }
+    output.Append(entry->mHostWithPort);
+    output.Append(kTab);
+    output.Append(sSHA256OIDString);
+    output.Append(kTab);
+    output.Append(settings.mFingerprint);
+    output.Append(kTab);
+    output.Append(bits_string);
+    output.Append(kTab);
+    output.Append(settings.mDBKey);
+    output.Append(NS_LINEBREAK);
+  }
+
+  // Make a clone of the file to pass to the WriterRunnable.
+  nsCOMPtr<nsIFile> file;
+  nsresult rv;
+  rv = mSettingsFile->Clone(getter_AddRefs(file));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  nsCOMPtr<nsIRunnable> runnable = new WriterRunnable(this, output, file);
+  rv = mWriterTaskQueue->Dispatch(runnable.forget());
+
+  if (NS_FAILED(rv)) {
+    return rv;
   }
 
   return NS_OK;
@@ -678,3 +727,23 @@ void nsCertOverrideService::GetHostWithPort(const nsACString& aHostName,
   }
   _retval.Assign(hostPort);
 }
+
+// nsIAsyncShutdownBlocker implementation
+NS_IMETHODIMP
+nsCertOverrideService::GetName(nsAString& aName) {
+  aName = NS_LITERAL_STRING("nsCertOverrideService: shutdown");
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+nsCertOverrideService::GetState(nsIPropertyBag**) { return NS_OK; }
+
+NS_IMETHODIMP
+nsCertOverrideService::BlockShutdown(nsIAsyncShutdownClient*) {
+  mWriterTaskQueue->BeginShutdown();
+  mWriterTaskQueue->AwaitShutdownAndIdle();
+
+  nsresult rv = GetShutdownBarrier()->RemoveBlocker(this);
+  MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));
+  return NS_OK;
+}

security/manager/ssl/nsCertOverrideService.h

@@ -11,7 +11,9 @@
 
 #include "mozilla/HashFunctions.h"
 #include "mozilla/Mutex.h"
+#include "mozilla/TaskQueue.h"
 #include "mozilla/TypedEnumBits.h"
+#include "nsIAsyncShutdown.h"
 #include "nsICertOverrideService.h"
 #include "nsIFile.h"
 #include "nsIObserver.h"
@@ -90,11 +92,13 @@ class nsCertOverrideEntry final : public PLDHashEntryHdr {
 
 class nsCertOverrideService final : public nsICertOverrideService,
                                     public nsIObserver,
-                                    public nsSupportsWeakReference {
+                                    public nsSupportsWeakReference,
+                                    public nsIAsyncShutdownBlocker {
  public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSICERTOVERRIDESERVICE
   NS_DECL_NSIOBSERVER
+  NS_DECL_NSIASYNCSHUTDOWNBLOCKER
 
   nsCertOverrideService();
 
@@ -116,7 +120,11 @@ class nsCertOverrideService final : public nsICertOverrideService,
   static void GetHostWithPort(const nsACString& aHostName, int32_t aPort,
                               nsACString& _retval);
 
- protected:
+  void AssertOnTaskQueue() const {
+    MOZ_ASSERT(mWriterTaskQueue->IsOnCurrentThread());
+  }
+
+ private:
   ~nsCertOverrideService();
 
   bool mDisableAllSecurityCheck;
@@ -136,6 +144,8 @@ class nsCertOverrideService final : public nsICertOverrideService,
                           nsCertOverride::OverrideBits ob,
                           const nsACString& dbKey,
                           const mozilla::MutexAutoLock& aProofOfLock);
+
+  RefPtr<TaskQueue> mWriterTaskQueue;
 };
 
 #define NS_CERTOVERRIDE_CID                          \

xpcom/threads/TaskQueue.h

@@ -114,6 +114,7 @@ class TaskQueue : public AbstractThread, public nsIDirectTaskDispatcher {
   // Returns true if the current thread is currently running a Runnable in
   // the task queue.
   bool IsCurrentThreadIn() const override;
+  using nsISerialEventTarget::IsOnCurrentThread;
 
  protected:
   virtual ~TaskQueue();