Bug 1738282 - Delete ScriptPreloader singleton after CCPostLastCycleCollection phase. r=mccr8

This stops destructing the stencils held by ScriptPreloader on release build,
to avoid the shutdown hang caused by thrashing.

Also:
  * Move ScriptPreloader::Cleanup from xpcom-shutdown notification to ScriptPreloader destructor, given there's no reason for having 2 steps
  * Replace ClearOnShutdown call with explicit function call for the singleton deletion, for simplicity
  * Remove now-unused PostJSShutDown phase

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

js/xpconnect/loader/ScriptPreloader.cpp

@@ -14,7 +14,6 @@
 #include "mozilla/URLPreloader.h"
 
 #include "mozilla/ArrayUtils.h"
-#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/Components.h"
 #include "mozilla/FileUtils.h"
 #include "mozilla/IOBuffers.h"
@@ -47,7 +46,6 @@
 #define DOC_ELEM_INSERTED_TOPIC "document-element-inserted"
 #define CONTENT_DOCUMENT_LOADED_TOPIC "content-document-loaded"
 #define CACHE_WRITE_TOPIC "browser-idle-startup-tasks-finished"
-#define CLEANUP_TOPIC "xpcom-shutdown"
 #define CACHE_INVALIDATE_TOPIC "startupcache-invalidate"
 
 // The maximum time we'll wait for a child process to finish starting up before
@@ -105,25 +103,24 @@ nsresult ScriptPreloader::CollectReports(nsIHandleReportCallback* aHandleReport,
   return NS_OK;
 }
 
+StaticRefPtr<ScriptPreloader> ScriptPreloader::gScriptPreloader;
+StaticRefPtr<ScriptPreloader> ScriptPreloader::gChildScriptPreloader;
+UniquePtr<AutoMemMap> ScriptPreloader::gCacheData;
+UniquePtr<AutoMemMap> ScriptPreloader::gChildCacheData;
+
 ScriptPreloader& ScriptPreloader::GetSingleton() {
-  static UniquePtr<AutoMemMap> cacheDataSingleton;
-  static RefPtr<ScriptPreloader> singleton;
-
-  if (!singleton) {
+  if (!gScriptPreloader) {
     if (XRE_IsParentProcess()) {
-      cacheDataSingleton = MakeUnique<AutoMemMap>();
-      singleton = new ScriptPreloader(cacheDataSingleton.get());
-      singleton->mChildCache = &GetChildSingleton();
-      Unused << singleton->InitCache();
-      ClearOnShutdown(&cacheDataSingleton, ShutdownPhase::JSPostShutDown);
+      gCacheData = MakeUnique<AutoMemMap>();
+      gScriptPreloader = new ScriptPreloader(gCacheData.get());
+      gScriptPreloader->mChildCache = &GetChildSingleton();
+      Unused << gScriptPreloader->InitCache();
     } else {
-      singleton = &GetChildSingleton();
+      gScriptPreloader = &GetChildSingleton();
     }
-
-    ClearOnShutdown(&singleton);
   }
 
-  return *singleton;
+  return *gScriptPreloader;
 }
 
 // The child singleton is available in all processes, including the parent, and
@@ -150,21 +147,30 @@ ScriptPreloader& ScriptPreloader::GetSingleton() {
 //  probably use the cache data written during this session if there was no
 //  previous cache file, but I'd rather do that as a follow-up.
 ScriptPreloader& ScriptPreloader::GetChildSingleton() {
-  static UniquePtr<AutoMemMap> cacheDataSingleton;
-  static RefPtr<ScriptPreloader> singleton;
-
-  if (!singleton) {
-    cacheDataSingleton = MakeUnique<AutoMemMap>();
-    singleton = new ScriptPreloader(cacheDataSingleton.get());
+  if (!gChildScriptPreloader) {
+    gChildCacheData = MakeUnique<AutoMemMap>();
+    gChildScriptPreloader = new ScriptPreloader(gChildCacheData.get());
     if (XRE_IsParentProcess()) {
-      Unused << singleton->InitCache(u"scriptCache-child"_ns);
+      Unused << gChildScriptPreloader->InitCache(u"scriptCache-child"_ns);
     }
-
-    ClearOnShutdown(&singleton);
-    ClearOnShutdown(&cacheDataSingleton, ShutdownPhase::JSPostShutDown);
   }
 
-  return *singleton;
+  return *gChildScriptPreloader;
+}
+
+/* static */
+void ScriptPreloader::DeleteSingleton() {
+  gScriptPreloader = nullptr;
+  gChildScriptPreloader = nullptr;
+}
+
+/* static */
+void ScriptPreloader::DeleteCacheDataSingleton() {
+  MOZ_ASSERT(!gScriptPreloader);
+  MOZ_ASSERT(!gChildScriptPreloader);
+
+  gCacheData = nullptr;
+  gChildCacheData = nullptr;
 }
 
 void ScriptPreloader::InitContentChild(ContentParent& parent) {
@@ -229,10 +235,11 @@ ScriptPreloader::ScriptPreloader(AutoMemMap* cacheData)
     obs->AddObserver(this, CACHE_WRITE_TOPIC, false);
   }
 
-  obs->AddObserver(this, CLEANUP_TOPIC, false);
   obs->AddObserver(this, CACHE_INVALIDATE_TOPIC, false);
 }
 
+ScriptPreloader::~ScriptPreloader() { Cleanup(); }
+
 void ScriptPreloader::Cleanup() {
   // Wait for any pending parses to finish before clearing the mScripts
   // hashtable, since the parse tasks depend on memory allocated by those
@@ -333,8 +340,6 @@ nsresult ScriptPreloader::Observe(nsISupports* subject, const char* topic,
     FinishContentStartup();
   } else if (!strcmp(topic, "timer-callback")) {
     FinishContentStartup();
-  } else if (!strcmp(topic, CLEANUP_TOPIC)) {
-    Cleanup();
   } else if (!strcmp(topic, CACHE_INVALIDATE_TOPIC)) {
     InvalidateCache();
   }

js/xpconnect/loader/ScriptPreloader.h

@@ -84,9 +84,19 @@ class ScriptPreloader : public nsIObserver,
   NS_DECL_NSINAMED
   NS_DECL_NSIASYNCSHUTDOWNBLOCKER
 
+ private:
+  static StaticRefPtr<ScriptPreloader> gScriptPreloader;
+  static StaticRefPtr<ScriptPreloader> gChildScriptPreloader;
+  static UniquePtr<AutoMemMap> gCacheData;
+  static UniquePtr<AutoMemMap> gChildCacheData;
+
+ public:
   static ScriptPreloader& GetSingleton();
   static ScriptPreloader& GetChildSingleton();
 
+  static void DeleteSingleton();
+  static void DeleteCacheDataSingleton();
+
   static ProcessType GetChildProcessType(const nsACString& remoteType);
 
   // Fill some options that should be consistent across all scripts stored
@@ -137,7 +147,7 @@ class ScriptPreloader : public nsIObserver,
   static void InitContentChild(dom::ContentParent& parent);
 
  protected:
-  virtual ~ScriptPreloader() = default;
+  virtual ~ScriptPreloader();
 
  private:
   enum class ScriptStatus {
@@ -515,9 +525,9 @@ class ScriptPreloader : public nsIObserver,
   nsCOMPtr<nsITimer> mSaveTimer;
 
   // The mmapped cache data from this session's cache file.
-  // The instance is held by the static variable in GetSingleton or
-  // GetChildSingleton, and its lifetime is guaranteed to be longer than
-  // ScriptPreloader instance.
+  // The instance is held by either `gCacheData` or `gChildCacheData` static
+  // fields, and its lifetime is guaranteed to be longer than ScriptPreloader
+  // instance.
   AutoMemMap* mCacheData;
 
   Monitor mMonitor;

xpcom/base/AppShutdown.cpp

@@ -60,8 +60,7 @@ const char* sPhaseObserverKeys[] = {
     "xpcom-shutdown-threads",           // XPCOMShutdownThreads
     nullptr,                            // XPCOMShutdownLoaders
     nullptr,                            // XPCOMShutdownFinal
-    nullptr,                            // CCPostLastCycleCollection
-    nullptr                             // PostJSShutDown
+    nullptr                             // CCPostLastCycleCollection
 };
 
 static_assert(sizeof(sPhaseObserverKeys) / sizeof(sPhaseObserverKeys[0]) ==

xpcom/base/ShutdownPhase.h

@@ -24,7 +24,6 @@ enum class ShutdownPhase {
   XPCOMShutdownLoaders,
   XPCOMShutdownFinal,
   CCPostLastCycleCollection,
-  JSPostShutDown,
   ShutdownPhase_Length,         // never pass this value
   First = AppShutdownConfirmed  // for iteration
 };

xpcom/build/XPCOMInit.cpp

@@ -715,6 +715,7 @@ nsresult ShutdownXPCOM(nsIServiceManager* aServMgr) {
       mozilla::ShutdownPhase::CCPostLastCycleCollection);
 
   mozilla::scache::StartupCache::DeleteSingleton();
+  mozilla::ScriptPreloader::DeleteSingleton();
 
   PROFILER_MARKER_UNTYPED("Shutdown xpcom", OTHER);
 
@@ -733,7 +734,7 @@ nsresult ShutdownXPCOM(nsIServiceManager* aServMgr) {
     sInitializedJS = false;
   }
 
-  mozilla::KillClearOnShutdown(ShutdownPhase::JSPostShutDown);
+  mozilla::ScriptPreloader::DeleteCacheDataSingleton();
 
   // Release shared memory which might be borrowed by the JS engine.
   xpc::SelfHostedShmem::Shutdown();