Bug 1656974 - Turn off StartupCache when startup is finished r=froydnj

I don't think this will fully resolve the shutdown hangs associated
with this bug. The underlying problem is that writing the startup
cache during shutdown can take a long time. This seems like A Bad Thing,
and I have a mild leaning toward the thought that we shouldn't write the
startup cache at all during shutdown. However, I do think that trading
shutdown time regressions for startup time wins is probably good, so
I don't feel super comfortable making that change.

However, this change at least fixes the problem where we A) block the main
thread waiting for the write to finish, and B) are more likely to hit a
shutdown hang, since we concentrate all of the hang time in one phase,
rather than it being spread across the shutdown timeline. This is likely
the major change that the regressing bug introduced.

The unfortunate consequence is that if we try to GetBuffer during shutdown,
we will now read out of the omnijar. However, it should overall be better
since we'll be waiting on a (hopefully) small number of small reads rather
than a large write.

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

startupcache/StartupCache.cpp

@@ -197,7 +197,6 @@ nsresult StartupCache::FullyInitSingleton() {
 
 StaticRefPtr<StartupCache> StartupCache::gStartupCache;
 ProcessType sProcessType;
-bool StartupCache::gShutdownInitiated;
 bool StartupCache::gIgnoreDiskCache;
 bool StartupCache::gFoundDiskCacheOnInit;
 
@@ -207,6 +206,7 @@ StartupCache::StartupCache()
     : mLock("StartupCache::mLock"),
       mDirty(false),
       mWrittenOnce(false),
+      mStartupFinished(false),
       mCurTableReferenced(false),
       mLoaded(false),
       mFullyInitialized(false),
@@ -821,6 +821,10 @@ 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(
@@ -836,6 +840,12 @@ 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) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   MutexAutoLock lock(mLock);
   if (!mLoaded) {
     return NS_ERROR_NOT_AVAILABLE;
@@ -904,7 +914,7 @@ 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 (StartupCache::gShutdownInitiated) {
+  if (mStartupFinished) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
@@ -1237,7 +1247,6 @@ void StartupCache::MaybeInitShutdownWrite() {
   if (mWriteTimer) {
     mWriteTimer->Cancel();
   }
-  gShutdownInitiated = true;
 
   MaybeWriteOffMainThread();
 }
@@ -1346,6 +1355,11 @@ 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;
   }
@@ -1380,7 +1394,6 @@ nsresult StartupCacheListener::Observe(nsISupports* subject, const char* topic,
   if (strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
     // Do not leave the thread running past xpcom shutdown
     sc->WaitOnPrefetchThread();
-    StartupCache::gShutdownInitiated = true;
     // Note that we don't do anything special for the background write
     // task; we expect the threadpool to finish running any tasks already
     // posted to it prior to shutdown. FastShutdown will call

startupcache/StartupCache.h

@@ -438,6 +438,7 @@ class StartupCache : public nsIMemoryReporter {
 
   Atomic<bool> mDirty;
   Atomic<bool> mWrittenOnce;
+  Atomic<bool> mStartupFinished;
   bool mCurTableReferenced;
   bool mLoaded;
   bool mFullyInitialized;
@@ -447,7 +448,6 @@ class StartupCache : public nsIMemoryReporter {
   size_t mCacheEntriesBaseOffset;
 
   static StaticRefPtr<StartupCache> gStartupCache;
-  static bool gShutdownInitiated;
   static bool gIgnoreDiskCache;
   static bool gFoundDiskCacheOnInit;
 

startupcache/test/TestStartupCache.cpp

@@ -93,14 +93,6 @@ 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) {
@@ -187,3 +179,25 @@ 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));
+}