Bug 1640445 - Avoid a race between the GC and WriteCache() with an extra field. r=kmag,decoder

See the comment on ScriptHolder for an explanation.

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

js/xpconnect/loader/ScriptPreloader.cpp

@@ -209,8 +209,7 @@ static void TraceOp(JSTracer* trc, void* data) {
 
 void ScriptPreloader::Trace(JSTracer* trc) {
   for (auto& script : IterHash(mScripts)) {
-    JS::TraceEdge(trc, &script->mScript,
+    script->mScript.Trace(trc);
-                  "ScriptPreloader::CachedScript.mScript");
   }
 }
 
@@ -802,7 +801,7 @@ void ScriptPreloader::NoteScript(const nsCString& url,
 
   if (!script->MaybeDropScript() && !script->mScript) {
     MOZ_ASSERT(jsscript);
-    script->mScript = jsscript;
+    script->mScript.Set(jsscript);
     script->mReadyToExecute = true;
   }
 
@@ -1019,7 +1018,7 @@ void ScriptPreloader::MaybeFinishOffThreadDecode() {
   for (auto script : mParsingScripts) {
     LOG(Debug, "Finished off-thread decode of %s\n", script->mURL.get());
     if (i < jsScripts.length()) {
-      script->mScript = jsScripts[i++];
+      script->mScript.Set(jsScripts[i++]);
     }
     script->mReadyToExecute = true;
   }
@@ -1117,11 +1116,30 @@ ScriptPreloader::CachedScript::CachedScript(ScriptPreloader& cache,
   mProcessTypes = {};
 }
 
+// JS::TraceEdge() can change the value of mScript, but not whether it is
+// null, so we don't update mHasScript to avoid a race.
+void ScriptPreloader::CachedScript::ScriptHolder::Trace(JSTracer* trc) {
+  JS::TraceEdge(trc, &mScript, "ScriptPreloader::CachedScript.mScript");
+}
+
+void ScriptPreloader::CachedScript::ScriptHolder::Set(
+    JS::HandleScript jsscript) {
+  MOZ_ASSERT(NS_IsMainThread());
+  mScript = jsscript;
+  mHasScript = mScript;
+}
+
+void ScriptPreloader::CachedScript::ScriptHolder::Clear() {
+  MOZ_ASSERT(NS_IsMainThread());
+  mScript = nullptr;
+  mHasScript = false;
+}
+
 bool ScriptPreloader::CachedScript::XDREncode(JSContext* cx) {
   auto cleanup = MakeScopeExit([&]() { MaybeDropScript(); });
 
-  JSAutoRealm ar(cx, mScript);
+  JSAutoRealm ar(cx, mScript.Get());
-  JS::RootedScript jsscript(cx, mScript);
+  JS::RootedScript jsscript(cx, mScript.Get());
 
   mXDRData.construct<JS::TranscodeBuffer>();
 
@@ -1140,8 +1158,8 @@ JSScript* ScriptPreloader::CachedScript::GetJSScript(
     JSContext* cx, const JS::ReadOnlyCompileOptions& options) {
   MOZ_ASSERT(mReadyToExecute);
   if (mScript) {
-    if (JS::CheckCompileOptionsMatch(options, mScript)) {
+    if (JS::CheckCompileOptionsMatch(options, mScript.Get())) {
-      return mScript;
+      return mScript.Get();
     }
     LOG(Error, "Cached script %s has different options\n", mURL.get());
     MOZ_DIAGNOSTIC_ASSERT(false, "Cached script has different options");
@@ -1165,7 +1183,7 @@ JSScript* ScriptPreloader::CachedScript::GetJSScript(
 
   JS::RootedScript script(cx);
   if (JS::DecodeScript(cx, options, Range(), &script)) {
-    mScript = script;
+    mScript.Set(script);
 
     if (mCache.mSaveComplete) {
       FreeData();
@@ -1175,7 +1193,7 @@ JSScript* ScriptPreloader::CachedScript::GetJSScript(
   LOG(Debug, "Finished decoding in %fms",
       (TimeStamp::Now() - start).ToMilliseconds());
 
-  return mScript;
+  return mScript.Get();
 }
 
 // nsIAsyncShutdownBlocker

js/xpconnect/loader/ScriptPreloader.h

@@ -212,6 +212,50 @@ class ScriptPreloader : public nsIObserver,
       const ScriptStatus mStatus;
     };
 
+    // The purpose of this helper class is to avoid a race between
+    // ScriptPreloader::WriteCache() and the GC on a JSScript*.
+    // The former checks if the actual JSScript* is null on the save thread
+    // while holding mMonitor. Aside from GC tracing, all places that mutate
+    // the JSScript* either hold mMonitor or don't run at the same time as the
+    // save thread. The GC can move the script, which will cause the value to
+    // change, but this will not change whether it is null or not.
+    //
+    // We can't hold mMonitor while tracing, because we can end running the
+    // GC while the current thread already holds mMonitor. Instead, this class
+    // avoids the race by storing a separate field to indicate if the script is
+    // null or not. To enforce this, the mutation by the GC that cannot affect
+    // the nullness of the script is split out from other mutation.
+    class MOZ_HEAP_CLASS ScriptHolder {
+     public:
+      explicit ScriptHolder(JSScript* script)
+          : mScript(script), mHasScript(script) {}
+      ScriptHolder() : mHasScript(false) {}
+
+      // This should only be called on the main thread (either while holding
+      // the preloader's mMonitor or while the save thread isn't running), or on
+      // the save thread while holding the preloader's mMonitor.
+      explicit operator bool() const { return mHasScript; }
+
+      // This should only be called on the main thread.
+      JSScript* Get() const {
+        MOZ_ASSERT(NS_IsMainThread());
+        return mScript;
+      }
+
+      // This should only be called on the main thread (or from a GC thread
+      // while the main thread is GCing).
+      void Trace(JSTracer* trc);
+
+      // These should only be called on the main thread, either while holding
+      // the preloader's mMonitor or while the save thread isn't running.
+      void Set(JS::HandleScript jsscript);
+      void Clear();
+
+     private:
+      JS::Heap<JSScript*> mScript;
+      bool mHasScript;  // true iff mScript is non-null.
+    };
+
     void FreeData() {
       // If the script data isn't mmapped, we need to release both it
       // and the Range that points to it at the same time.
@@ -237,7 +281,7 @@ class ScriptPreloader : public nsIObserver,
     // not.
     bool MaybeDropScript() {
       if (mIsRunOnce && (HasRange() || !mCache.WillWriteScripts())) {
-        mScript = nullptr;
+        mScript.Clear();
         return true;
       }
       return false;
@@ -321,7 +365,7 @@ class ScriptPreloader : public nsIObserver,
 
     TimeStamp mLoadTime{};
 
-    JS::Heap<JSScript*> mScript;
+    ScriptHolder mScript;
 
     // True if this script is ready to be executed. This means that either the
     // off-thread portion of an off-thread decode has finished, or the script

mozglue/build/TsanOptions.cpp

@@ -286,10 +286,6 @@ extern "C" const char* __tsan_default_suppressions() {
          "race:CamerasParent::ActorDestroy\n"
          "race:CamerasParent::DispatchToVideoCaptureThread\n"
 
-         // Bug 1672230
-         "race:ScriptPreloader::Trace\n"
-         "race:ScriptPreloader::WriteCache\n"
-
          // Bug 1623541
          "race:VRShMem::PullSystemState\n"
          "race:VRShMem::PushSystemState\n"