Bug 1506812 - Wrap all accesses to URLPreloader's mReaderThread in a mutex. r=decoder,nika

The shutdown code of BackgroundReadFiles races with BeginBackgroundRead.
This is paritally obfuscated by the usage of the initialEvent convenience
of NS_NewNamedThread, so this change also removes that in favour of explicit
dispatch. (xpcom devs want to remove the convenience anyway)

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

js/xpconnect/loader/URLPreloader.cpp

@@ -324,9 +324,12 @@ Result<Ok, nsresult> URLPreloader::ReadCache(
 
 void URLPreloader::BackgroundReadFiles() {
   auto cleanup = MakeScopeExit([&]() {
+    auto lock = mReaderThread.Lock();
+    auto& readerThread = lock.ref();
     NS_DispatchToMainThread(NewRunnableMethod(
-        "nsIThread::AsyncShutdown", mReaderThread, &nsIThread::AsyncShutdown));
-    mReaderThread = nullptr;
+        "nsIThread::AsyncShutdown", readerThread, &nsIThread::AsyncShutdown));
+
+    readerThread = nullptr;
   });
 
   Vector<nsZipCursor> cursors;
@@ -426,13 +429,24 @@ void URLPreloader::BackgroundReadFiles() {
 }
 
 void URLPreloader::BeginBackgroundRead() {
-  if (!mReaderThread && !mReaderInitialized && sInitialized) {
+  auto lock = mReaderThread.Lock();
+  auto& readerThread = lock.ref();
+  if (!readerThread && !mReaderInitialized && sInitialized) {
+    nsresult rv;
+    rv = NS_NewNamedThread("BGReadURLs", getter_AddRefs(readerThread));
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return;
+    }
+
     nsCOMPtr<nsIRunnable> runnable =
         NewRunnableMethod("URLPreloader::BackgroundReadFiles", this,
                           &URLPreloader::BackgroundReadFiles);
-
-    Unused << NS_NewNamedThread("BGReadURLs", getter_AddRefs(mReaderThread),
-                                runnable);
+    rv = readerThread->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      // If we can't launch the task, just destroy the thread
+      readerThread = nullptr;
+      return;
+    }
   }
 }
 

js/xpconnect/loader/URLPreloader.h

@@ -6,6 +6,7 @@
 #ifndef URLPreloader_h
 #define URLPreloader_h
 
+#include "mozilla/DataMutex.h"
 #include "mozilla/FileLocation.h"
 #include "mozilla/HashFunctions.h"
 #include "mozilla/LinkedList.h"
@@ -299,8 +300,9 @@ class URLPreloader final : public nsIObserver, public nsIMemoryReporter {
 
   // Note: We use a RefPtr rather than an nsCOMPtr here because the
   // AssertNoQueryNeeded checks done by getter_AddRefs happen at a time that
-  // violate data access invariants.
-  RefPtr<nsIThread> mReaderThread;
+  // violate data access invariants. It's wrapped in a mutex because
+  // the reader thread needs to be able to null this out to terminate itself.
+  DataMutex<RefPtr<nsIThread>> mReaderThread{"ReaderThread"};
 
   // A map of URL entries which have were either read this session, or read
   // from the last session's cache file.

mozglue/build/TsanOptions.cpp

@@ -218,9 +218,6 @@ extern "C" const char* __tsan_default_suppressions() {
          // The rest of these suppressions are miscellaneous issues in gecko
          // that should be investigated and ideally fixed.
 
-         // Bug 1506812
-         "race:BeginBackgroundRead\n"
-
          // Bug 1619162
          "race:currentNameHasEscapes\n"