diff --git a/js/xpconnect/loader/ScriptPreloader.cpp b/js/xpconnect/loader/ScriptPreloader.cpp index 6b9fa97249c2..606753fd0545 100644 --- a/js/xpconnect/loader/ScriptPreloader.cpp +++ b/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, - "ScriptPreloader::CachedScript.mScript"); + script->mScript.Trace(trc); } } @@ -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); - JS::RootedScript jsscript(cx, mScript); + JSAutoRealm ar(cx, mScript.Get()); + JS::RootedScript jsscript(cx, mScript.Get()); mXDRData.construct(); @@ -1140,8 +1158,8 @@ JSScript* ScriptPreloader::CachedScript::GetJSScript( JSContext* cx, const JS::ReadOnlyCompileOptions& options) { MOZ_ASSERT(mReadyToExecute); if (mScript) { - if (JS::CheckCompileOptionsMatch(options, mScript)) { - return mScript; + if (JS::CheckCompileOptionsMatch(options, mScript.Get())) { + 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 diff --git a/js/xpconnect/loader/ScriptPreloader.h b/js/xpconnect/loader/ScriptPreloader.h index 9b3a21f92166..d1720aa9e2b2 100644 --- a/js/xpconnect/loader/ScriptPreloader.h +++ b/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 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 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 diff --git a/mozglue/build/TsanOptions.cpp b/mozglue/build/TsanOptions.cpp index 435ccbe73a44..62a9317b51e3 100644 --- a/mozglue/build/TsanOptions.cpp +++ b/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"