Bug 1658732 - Replace mStartupFinished with TryLock in StartupCache r=froydnj

To be honest, it's still a mystery why we observed a regression in
sessionrestore_no_auto_restore in bug 1658732. The regression won't reproduce
on profiled runs, and the bad recordings happen before the supposedly offending
code ever actually runs. It feels most likely that it is a more or less random
confluence of factors causing a regression; however, 33% is too large of a
number to ignore.

The changes in this patch do not seem to yield the same regression, and they
are arguably more correct anyway. Instead of simply turning off the cache after
startup is finished, we simply avoid blocking waiting for the write from inside
GetBuffer. This way, if the write is not getting in the way of GetBuffer, we
can still benefit from a cached version of whatever it is we're looking for.

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

startupcache/StartupCache.cpp

@@ -206,7 +206,6 @@ StartupCache::StartupCache()
     : mLock("StartupCache::mLock"),
       mDirty(false),
       mWrittenOnce(false),
-      mStartupFinished(false),
       mCurTableReferenced(false),
       mLoaded(false),
       mFullyInitialized(false),
@@ -821,16 +820,21 @@ Result<Ok, nsresult> StartupCache::DecompressEntry(StartupCacheEntry& aEntry) {
 bool StartupCache::HasEntry(const char* id) {
   AUTO_PROFILER_LABEL("StartupCache::HasEntry", OTHER);
 
-  if (mStartupFinished) {
-    return false;
-  }
-
-  MutexAutoLock lock(mLock);
-
   MOZ_ASSERT(
       strnlen(id, kStartupCacheKeyLengthCap) + 1 < kStartupCacheKeyLengthCap,
       "StartupCache key too large or not terminated.");
-  return mTable.has(id);
+
+  // The lock could be held here by the write thread, which could take a while.
+  // scache reads/writes are never critical enough to block waiting for the
+  // entire cache to be written to disk, so if we can't acquire the lock, we
+  // just exit.
+  Maybe<MutexAutoLock> tryLock;
+  if (!MutexAutoLock::TryMake(mLock, tryLock)) {
+    return false;
+  }
+
+  bool result = mTable.has(id);
+  return result;
 }
 
 nsresult StartupCache::GetBuffer(const char* id, const char** outbuf,
@@ -840,13 +844,15 @@ nsresult StartupCache::GetBuffer(const char* id, const char** outbuf,
   auto telemetry =
       MakeScopeExit([&label] { Telemetry::AccumulateCategorical(label); });
 
-  // Exit here, ensuring we collect a cache miss for telemetry, but before we
-  // lock. No need to potentially hang waiting on the write thread.
-  if (mStartupFinished) {
+  // The lock could be held here by the write thread, which could take a while.
+  // scache reads/writes are never critical enough to block waiting for the
+  // entire cache to be written to disk, so if we can't acquire the lock, we
+  // just exit.
+  Maybe<MutexAutoLock> tryLock;
+  if (!MutexAutoLock::TryMake(mLock, tryLock)) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  MutexAutoLock lock(mLock);
   if (!mLoaded) {
     return NS_ERROR_NOT_AVAILABLE;
   }
@@ -914,10 +920,6 @@ nsresult StartupCache::GetBuffer(const char* id, const char** outbuf,
 // Client gives ownership of inbuf.
 nsresult StartupCache::PutBuffer(const char* id, UniquePtr<char[]>&& inbuf,
                                  uint32_t len, bool isFromChildProcess) {
-  if (mStartupFinished) {
-    return NS_ERROR_NOT_AVAILABLE;
-  }
-
   // We use this to update the RequestedByChild flag.
   MOZ_RELEASE_ASSERT(
       inbuf || isFromChildProcess,
@@ -927,7 +929,21 @@ nsresult StartupCache::PutBuffer(const char* id, UniquePtr<char[]>&& inbuf,
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  MutexAutoLock lock(mLock);
+  // If we've already written to disk, just go ahead and exit. There's no point
+  // in putting something in the cache since we're not going to write again.
+  if (mWrittenOnce) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
+  // The lock could be held here by the write thread, which could take a while.
+  // scache reads/writes are never critical enough to block waiting for the
+  // entire cache to be written to disk, so if we can't acquire the lock, we
+  // just exit.
+  Maybe<MutexAutoLock> tryLock;
+  if (!MutexAutoLock::TryMake(mLock, tryLock)) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   if (!mLoaded) {
     return NS_ERROR_NOT_AVAILABLE;
   }
@@ -1355,11 +1371,6 @@ void StartupCache::MaybeWriteOffMainThread() {
     return;
   }
 
-  // If we're scheduling the cache to be written, then whether it's because
-  // we're shutting down, or because our write timer has finished, it's safe
-  // to say that we shouldn't think of ourselves as "starting up" anymore.
-  mStartupFinished = true;
-
   if (mWrittenOnce) {
     return;
   }

startupcache/StartupCache.h

@@ -438,7 +438,6 @@ class StartupCache : public nsIMemoryReporter {
 
   Atomic<bool> mDirty;
   Atomic<bool> mWrittenOnce;
-  Atomic<bool> mStartupFinished;
   bool mCurTableReferenced;
   bool mLoaded;
   bool mFullyInitialized;

startupcache/test/TestStartupCache.cpp

@@ -93,6 +93,14 @@ TEST_F(TestStartupCache, StartupWriteRead) {
   rv = sc->GetBuffer(id, &outbuf, &len);
   EXPECT_TRUE(NS_SUCCEEDED(rv));
   EXPECT_STREQ(buf, outbuf);
+
+  rv = sc->ResetStartupWriteTimer();
+  EXPECT_TRUE(NS_SUCCEEDED(rv));
+  WaitForStartupTimer();
+
+  rv = sc->GetBuffer(id, &outbuf, &len);
+  EXPECT_TRUE(NS_SUCCEEDED(rv));
+  EXPECT_STREQ(buf, outbuf);
 }
 
 TEST_F(TestStartupCache, WriteInvalidateRead) {
@@ -179,25 +187,3 @@ TEST_F(TestStartupCache, WriteObject) {
   EXPECT_TRUE(NS_SUCCEEDED(rv));
   ASSERT_TRUE(outSpec.Equals(spec));
 }
-
-TEST_F(TestStartupCache, GetDisabledAfterDiskWrite) {
-  nsresult rv;
-  StartupCache* sc = StartupCache::GetSingleton();
-
-  const char* buf = "Market opportunities for BeardBook";
-  const char* id = "id";
-  const char* outbuf;
-  uint32_t len;
-
-  rv = sc->PutBuffer(id, UniquePtr<char[]>(strdup(buf)), strlen(buf) + 1);
-  EXPECT_TRUE(NS_SUCCEEDED(rv));
-
-  rv = sc->GetBuffer(id, &outbuf, &len);
-  EXPECT_TRUE(NS_SUCCEEDED(rv));
-  EXPECT_STREQ(buf, outbuf);
-
-  WaitForStartupTimer();
-
-  rv = sc->GetBuffer(id, &outbuf, &len);
-  EXPECT_TRUE(NS_FAILED(rv));
-}

xpcom/threads/Mutex.h

@@ -8,7 +8,9 @@
 #define mozilla_Mutex_h
 
 #include "mozilla/BlockingResourceBase.h"
+#include "mozilla/Maybe.h"
 #include "mozilla/PlatformMutex.h"
+#include "mozilla/Unused.h"
 
 //
 // Provides:
@@ -158,6 +160,25 @@ class MOZ_RAII BaseAutoLock {
 
   ~BaseAutoLock(void) { mLock.Unlock(); }
 
+  /**
+   * TryMake
+   * Tries to lock the mutex with TryLock, and returns true with aOutAutoLock
+   * containing the BaseAutoLock if TryLock was successful. Otherwise returns
+   * false.
+   *
+   * @param aLock A valid mozilla::Mutex* returned by
+   *              mozilla::Mutex::NewMutex.
+   * @param aOutAutoLock A Maybe<BaseAutoLock<T>> to fill if TryLock succeeds.
+   **/
+  static MOZ_MUST_USE bool TryMake(T aLock,
+                                   Maybe<BaseAutoLock<T>>& aOutAutoLock) {
+    if (aLock.TryLock()) {
+      aOutAutoLock.emplace(aLock, /* aPlaceholder: */ true);
+      return true;
+    }
+    return false;
+  }
+
   // Assert that aLock is the mutex passed to the constructor and that the
   // current thread owns the mutex.  In coding patterns such as:
   //
@@ -194,8 +215,18 @@ class MOZ_RAII BaseAutoLock {
   BaseAutoLock& operator=(BaseAutoLock&);
   static void* operator new(size_t) noexcept(true);
 
+  // This is the internal cconstructor used by TryMake. We need it to be a
+  // constructor to distinguish it from the public constructor, so we add a
+  // dummy variable to track that.
+  BaseAutoLock(T aLock, bool aPlaceholder) : mLock(aLock) {
+    Unused << aPlaceholder;
+  }
+
   friend class BaseAutoUnlock<T>;
 
+  // So that Maybe can access our private constructor
+  friend class Maybe<BaseAutoLock<T>>;
+
   T mLock;
 };