From a2f7e06013aa8b9a6912e9f23c25648bf01f5387 Mon Sep 17 00:00:00 2001
From: Nika Layzell <nika@thelayzells.com>
Date: Tue, 4 Jul 2023 19:05:06 +0000
Subject: [PATCH] Bug 1841660 - Fix potential deadlock waiting for StartupCache
 ThreadedPrefetch, r=jesup

In a previous change to this logic, we missed a hidden early return in
the `MMAP_FAULT_HANDLER_CATCH()` macro which can early return on both
Linux and Windows. If this is hit, we could end up blocking at various
points waiting for the StartupCache to be prefetched.

This patch changes the logic to notify to happen in a ScopeExit instead,
ensuring that it'll happen on all return paths.

In addition, there was another potential deadlock due to the
ThreadedPrefetch thread acquiring mTableLock. This was fixed by passing
in the pointers in the runnable instead. This should be OK as we are
making sure to block on ThreadedPrefetch before we clear the startup
data already.

Differential Revision: https://phabricator.services.mozilla.com/D182732
---
 startupcache/StartupCache.cpp | 31 +++++++++++++++----------------
 startupcache/StartupCache.h   |  4 ++--
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/startupcache/StartupCache.cpp b/startupcache/StartupCache.cpp
index e38439fb71e6..5c6e97b326bd 100644
--- a/startupcache/StartupCache.cpp
+++ b/startupcache/StartupCache.cpp
@@ -255,8 +255,9 @@ void StartupCache::StartPrefetchMemory() {
     MonitorAutoLock lock(mPrefetchComplete);
     mPrefetchInProgress = true;
   }
-  NS_DispatchBackgroundTask(NewRunnableMethod(
-      "StartupCache::ThreadedPrefetch", this, &StartupCache::ThreadedPrefetch));
+  NS_DispatchBackgroundTask(NewRunnableMethod<uint8_t*, size_t>(
+      "StartupCache::ThreadedPrefetch", this, &StartupCache::ThreadedPrefetch,
+      mCacheData.get<uint8_t>().get(), mCacheData.size()));
 }
 
 /**
@@ -706,22 +707,20 @@ void StartupCache::WaitOnPrefetch() {
   }
 }
 
-void StartupCache::ThreadedPrefetch() {
-  uint8_t* buf;
-  size_t size;
-  {
-    MutexAutoLock lock(mTableLock);
-    buf = mCacheData.get<uint8_t>().get();
-    size = mCacheData.size();
-  }
+void StartupCache::ThreadedPrefetch(uint8_t* aStart, size_t aSize) {
+  // Always notify of completion, even if MMAP_FAULT_HANDLER_CATCH()
+  // early-returns.
+  auto notifyPrefetchComplete = MakeScopeExit([&] {
+    MonitorAutoLock lock(mPrefetchComplete);
+    mPrefetchInProgress = false;
+    mPrefetchComplete.NotifyAll();
+  });
+
   // PrefetchMemory does madvise/equivalent, but doesn't access the memory
-  // pointed to by buf
-  MMAP_FAULT_HANDLER_BEGIN_BUFFER(buf, size)
-  PrefetchMemory(buf, size);
+  // pointed to by aStart
+  MMAP_FAULT_HANDLER_BEGIN_BUFFER(aStart, aSize)
+  PrefetchMemory(aStart, aSize);
   MMAP_FAULT_HANDLER_CATCH()
-  MonitorAutoLock lock(mPrefetchComplete);
-  mPrefetchInProgress = false;
-  mPrefetchComplete.NotifyAll();
 }
 
 // mTableLock must be held
diff --git a/startupcache/StartupCache.h b/startupcache/StartupCache.h
index cfc785f41bae..045ad05213fc 100644
--- a/startupcache/StartupCache.h
+++ b/startupcache/StartupCache.h
@@ -213,12 +213,12 @@ class StartupCache : public nsIMemoryReporter {
   Result<Ok, nsresult> WriteToDisk() MOZ_REQUIRES(mTableLock);
 
   void WaitOnPrefetch();
-  void StartPrefetchMemory();
+  void StartPrefetchMemory() MOZ_REQUIRES(mTableLock);
 
   static nsresult InitSingleton();
   static void WriteTimeout(nsITimer* aTimer, void* aClosure);
   void MaybeWriteOffMainThread();
-  void ThreadedPrefetch();
+  void ThreadedPrefetch(uint8_t* aStart, size_t aSize);
 
   Monitor mPrefetchComplete{"StartupCachePrefetch"};
   bool mPrefetchInProgress MOZ_GUARDED_BY(mPrefetchComplete){false};