From eef37b2cd5f1fdfb15236911fb3852f8f1c29774 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Mon, 27 Nov 2023 23:30:25 -0600 Subject: [PATCH] Issue #2387 - Collection of fixes related to a crash while canceling a dynamic import. Add AddRef/Release hooks for embedding's script or module private value and set this script source object where appropriate. https://bugzilla.mozilla.org/show_bug.cgi?id=1519140 Partial, no Value passing changes. Clear the list of dynamic import requests after cancelling them in ScriptLoader::ParsingComplete as we do for other requests. https://bugzilla.mozilla.org/show_bug.cgi?id=1291535 ScriptLoader::OnStreamComplete never returns a failure. https://bugzilla.mozilla.org/show_bug.cgi?id=1627275 Also cancel the correct kind of parser for Modules. --- dom/script/ModuleScript.cpp | 29 ++++++++++++++++++++--------- dom/script/ModuleScript.h | 8 +++++--- dom/script/ScriptLoader.cpp | 20 ++++++++++++++++---- js/src/builtin/ModuleObject.cpp | 6 ++++++ js/src/jsapi.cpp | 19 ++++++++----------- js/src/jsapi.h | 18 ++++++------------ js/src/jsscript.cpp | 30 +++++++++++++++++++++--------- js/src/jsscript.h | 4 +--- js/src/vm/Runtime.cpp | 3 ++- js/src/vm/Runtime.h | 17 +++++++++++++++-- 10 files changed, 100 insertions(+), 54 deletions(-) diff --git a/dom/script/ModuleScript.cpp b/dom/script/ModuleScript.cpp index 425da8207a..6c2fba6a95 100644 --- a/dom/script/ModuleScript.cpp +++ b/dom/script/ModuleScript.cpp @@ -62,20 +62,33 @@ void LoadedScript::AssociateWithScript(JSScript* aScript) { AddRef(); } -void HostFinalizeTopLevelScript(JSFreeOp* aFop, const JS::Value& aPrivate) { - // Decrement the reference count of a LoadedScript object that is - // pointed to by a dying JSScript. The reference count was - // originally incremented by AssociateWithScript() above. - - auto script = static_cast(aPrivate.toPrivate()); - +inline void CheckModuleScriptPrivate(LoadedScript* script, + const JS::Value& aPrivate) { #ifdef DEBUG if (script->IsModuleScript()) { JSObject* module = script->AsModuleScript()->mModuleRecord.unbarrieredGet(); MOZ_ASSERT_IF(module, JS::GetModulePrivate(module) == aPrivate); } #endif +} +void HostAddRefTopLevelScript(const JS::Value& aPrivate) { + // Increment the reference count of a LoadedScript object that is now pointed + // to by a JSScript. The reference count is decremented by + // HostReleaseTopLevelScript() below. + + auto script = static_cast(aPrivate.toPrivate()); + CheckModuleScriptPrivate(script, aPrivate); + script->AddRef(); +} + +void HostReleaseTopLevelScript(const JS::Value& aPrivate) { + // Decrement the reference count of a LoadedScript object that was pointed to + // by a JSScript. The reference count was originally incremented by + // HostAddRefTopLevelScript() above. + + auto script = static_cast(aPrivate.toPrivate()); + CheckModuleScriptPrivate(script, aPrivate); script->Release(); } @@ -132,7 +145,6 @@ ModuleScript::UnlinkModuleRecord() this); JS::SetModulePrivate(mModuleRecord, JS::UndefinedValue()); mModuleRecord = nullptr; - Release(); } } @@ -157,7 +169,6 @@ ModuleScript::SetModuleRecord(JS::Handle aModuleRecord) MOZ_ASSERT(JS::GetModulePrivate(mModuleRecord).isUndefined()); JS::SetModulePrivate(mModuleRecord, JS::PrivateValue(this)); HoldJSObjects(this); - AddRef(); } void diff --git a/dom/script/ModuleScript.h b/dom/script/ModuleScript.h index b3eb841778..64b16db67d 100644 --- a/dom/script/ModuleScript.h +++ b/dom/script/ModuleScript.h @@ -18,7 +18,8 @@ namespace dom { class ScriptLoader; -void HostFinalizeTopLevelScript(JSFreeOp* aFop, const JS::Value& aPrivate); +void HostAddRefTopLevelScript(const JS::Value& aPrivate); +void HostReleaseTopLevelScript(const JS::Value& aPrivate); class ClassicScript; class ModuleScript; @@ -63,7 +64,6 @@ class ClassicScript final : public LoadedScript class ModuleScript final : public LoadedScript { - JS::Heap mModuleRecord; JS::Heap mParseError; JS::Heap mErrorToRethrow; @@ -74,6 +74,8 @@ public: NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(ModuleScript, LoadedScript) + JS::Heap mModuleRecord; + ModuleScript(ScriptFetchOptions* aFetchOptions, nsIURI* aBaseURL); void SetModuleRecord(JS::Handle aModuleRecord); @@ -89,7 +91,7 @@ public: void UnlinkModuleRecord(); - friend void HostFinalizeTopLevelScript(JSFreeOp*, const JS::Value&); + friend void CheckModuleScriptPrivate(LoadedScript*, const JS::Value&); }; ClassicScript* LoadedScript::AsClassicScript() { diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 89a74c1541..06fcee7aca 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -185,7 +185,11 @@ ScriptLoadRequest::MaybeCancelOffThreadScript() } JSContext* cx = danger::GetJSContext(); - JS::CancelOffThreadScript(cx, mOffThreadToken); + if (IsModuleRequest()) { + JS::CancelOffThreadModule(cx, mOffThreadToken); + } else { + JS::CancelOffThreadScript(cx, mOffThreadToken); + } mOffThreadToken = nullptr; } @@ -1109,8 +1113,9 @@ void ScriptLoader::EnsureModuleHooksInitialized() { JS::SetModuleResolveHook(rt, HostResolveImportedModule); JS::SetModuleMetadataHook(jsapi.cx(), HostPopulateImportMeta); - JS::SetScriptPrivateFinalizeHook(jsapi.cx(), HostFinalizeTopLevelScript); - + JS::SetScriptPrivateReferenceHooks(jsapi.cx(), HostAddRefTopLevelScript, + HostReleaseTopLevelScript); + Preferences::RegisterCallbackAndCall(DynamicImportPrefChangedCallback, "javascript.options.dynamicImport", (void*)nullptr); @@ -2648,7 +2653,7 @@ ScriptLoader::OnStreamComplete(nsIIncrementalStreamLoader* aLoader, // Process our request and/or any pending ones ProcessPendingRequests(); - return NS_OK; + return rv; } nsresult @@ -2758,6 +2763,9 @@ ScriptLoader::HandleLoadError(ScriptLoadRequest *aRequest, nsresult aResult) { if (aRequest->isInList()) { RefPtr req = mDynamicImportRequests.Steal(aRequest); modReq->Cancel(); + // FinishDynamicImport must happen exactly once for each dynamic import + // request. If the load is aborted we do it when we remove the request + // from mDynamicImportRequests. FinishDynamicImport(modReq, aResult); } } else { @@ -2979,8 +2987,12 @@ ScriptLoader::ParsingComplete(bool aTerminated) for (ScriptLoadRequest* req = mDynamicImportRequests.getFirst(); req; req = req->getNext()) { req->Cancel(); + // FinishDynamicImport must happen exactly once for each dynamic import + // request. If the load is aborted we do it when we remove the request + // from mDynamicImportRequests. FinishDynamicImport(req->AsModuleRequest(), NS_ERROR_ABORT); } + mDynamicImportRequests.Clear(); if (mParserBlockingRequest) { mParserBlockingRequest->Cancel(); diff --git a/js/src/builtin/ModuleObject.cpp b/js/src/builtin/ModuleObject.cpp index dd03017fc0..16a5808ab2 100644 --- a/js/src/builtin/ModuleObject.cpp +++ b/js/src/builtin/ModuleObject.cpp @@ -1607,7 +1607,10 @@ js::StartDynamicModuleImport(JSContext* cx, HandleValue referencingPrivate, Hand JS::ModuleDynamicImportHook importHook = cx->runtime()->moduleDynamicImportHook; MOZ_ASSERT(importHook); + cx->runtime()->addRefScriptPrivate(referencingPrivate); if (!importHook(cx, referencingPrivate, specifier, promise)) { + cx->runtime()->releaseScriptPrivate(referencingPrivate); + if (!RejectPromiseWithPendingError(cx, promise)) return nullptr; return promise; @@ -1622,6 +1625,9 @@ js::FinishDynamicModuleImport(JSContext* cx, HandleValue referencingPrivate, Han { Handle promise = promiseArg.as(); + auto releasePrivate = mozilla::MakeScopeExit( + [&] { cx->runtime()->releaseScriptPrivate(referencingPrivate); }); + if (cx->isExceptionPending()) { return RejectPromiseWithPendingError(cx, promise); } diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 9067ca3d3d..1be4e39113 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -4729,7 +4729,8 @@ JS::CompileModule(JSContext* cx, const ReadOnlyCompileOptions& options, JS_PUBLIC_API(void) JS::SetModulePrivate(JSObject* module, const JS::Value& value) { - module->as().scriptSourceObject()->setPrivate(value); + JSRuntime* rt = module->compartment()->runtimeFromMainThread(); + module->as().scriptSourceObject()->setPrivate(rt, value); } JS_PUBLIC_API(JS::Value) @@ -4741,7 +4742,8 @@ JS::GetModulePrivate(JSObject* module) JS_PUBLIC_API(void) JS::SetScriptPrivate(JSScript* script, const JS::Value& value) { - script->scriptSourceUnwrap().setPrivate(value); + JSRuntime* rt = script->compartment()->runtimeFromMainThread(); + script->scriptSourceUnwrap().setPrivate(rt, value); } JS_PUBLIC_API(JS::Value) @@ -4764,18 +4766,13 @@ JS::GetScriptedCallerPrivate(JSContext* cx) return FindScriptOrModulePrivateForScript(iter.script()); } -JS_PUBLIC_API(JS::ScriptPrivateFinalizeHook) -JS::GetScriptPrivateFinalizeHook(JSContext* cx) -{ - AssertHeapIsIdle(cx); - return cx->runtime()->scriptPrivateFinalizeHook; -} - JS_PUBLIC_API(void) -JS::SetScriptPrivateFinalizeHook(JSContext* cx, JS::ScriptPrivateFinalizeHook func) +JS::SetScriptPrivateReferenceHooks(JSContext* cx, JS::ScriptPrivateReferenceHook addRefHook, + JS::ScriptPrivateReferenceHook releaseHook) { AssertHeapIsIdle(cx); - cx->runtime()->scriptPrivateFinalizeHook = func; + cx->runtime()->scriptPrivateAddRefHook = addRefHook; + cx->runtime()->scriptPrivateReleaseHook = releaseHook; } JS_PUBLIC_API(bool) diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 951b612502..444b9e2ed2 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -4432,24 +4432,18 @@ extern JS_PUBLIC_API(JS::Value) GetScriptedCallerPrivate(JSContext* cx); /** - * A hook that's called whenever a script or module which has a private value - * set with SetScriptPrivate() or SetModulePrivate() is finalized. This can be - * used to clean up the private state. The private value is passed as an - * argument. + * Hooks called when references to a script private value are created or + * destroyed. This allows use of a reference counted object as the + * script private. */ -using ScriptPrivateFinalizeHook = void (*)(JSFreeOp*, const JS::Value&); - -/** - * Get the script private finalize hook for the runtime. - */ -extern JS_PUBLIC_API(ScriptPrivateFinalizeHook) -GetScriptPrivateFinalizeHook(JSContext* cx); +using ScriptPrivateReferenceHook = void (*)(const JS::Value&); /** * Set the script private finalize hook for the runtime to the given function. */ extern JS_PUBLIC_API(void) -SetScriptPrivateFinalizeHook(JSContext* cx, ScriptPrivateFinalizeHook func); +SetScriptPrivateReferenceHooks(JSContext* cx, ScriptPrivateReferenceHook addRefHook, + ScriptPrivateReferenceHook releaseHook); /* * Perform the ModuleInstantiate operation on the given source text module diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp index 5cf0d19195..a438f051c9 100644 --- a/js/src/jsscript.cpp +++ b/js/src/jsscript.cpp @@ -1419,15 +1419,8 @@ ScriptSourceObject::finalize(FreeOp* fop, JSObject* obj) sso->source()->decref(); sso->setReservedSlot(SOURCE_SLOT, PrivateValue(nullptr)); - Value value = sso->canonicalPrivate(); - if (!value.isUndefined()) { - // The embedding may need to dispose of its private data. - JS::AutoSuppressGCAnalysis suppressGC; - if (JS::ScriptPrivateFinalizeHook hook = - fop->runtime()->scriptPrivateFinalizeHook) { - hook(fop, value); - } - } + // Clear the private value, calling the release hook if necessary. + sso->setPrivate(fop->runtime(), UndefinedValue()); } static const ClassOps ScriptSourceObjectClassOps = { @@ -1532,6 +1525,25 @@ ScriptSourceObject::initFromOptions(JSContext* cx, HandleScriptSource source, return true; } +void ScriptSourceObject::setPrivate(JSRuntime* rt, const Value& value) +{ + // Update the private value, calling addRef/release hooks if necessary + // to allow the embedding to maintain a reference count for the + // private data. + Value prevValue = getReservedSlot(PRIVATE_SLOT); + if (!prevValue.isUndefined()) { + if (auto releaseHook = rt->scriptPrivateReleaseHook) { + releaseHook(prevValue); + } + } + setReservedSlot(PRIVATE_SLOT, value); + if (!value.isUndefined()) { + if (auto addRefHook = rt->scriptPrivateAddRefHook) { + addRefHook(value); + } + } +} + /* static */ bool JSScript::loadSource(JSContext* cx, ScriptSource* ss, bool* worked) { diff --git a/js/src/jsscript.h b/js/src/jsscript.h index 78afa2eec2..68e9884321 100644 --- a/js/src/jsscript.h +++ b/js/src/jsscript.h @@ -675,9 +675,7 @@ class ScriptSourceObject : public NativeObject return static_cast(untyped); } - void setPrivate(const Value& value) { - setReservedSlot(PRIVATE_SLOT, value); - } + void setPrivate(JSRuntime* rt, const Value& value); Value getPrivate() const { return getReservedSlot(PRIVATE_SLOT); diff --git a/js/src/vm/Runtime.cpp b/js/src/vm/Runtime.cpp index c5deff940c..a80359f282 100644 --- a/js/src/vm/Runtime.cpp +++ b/js/src/vm/Runtime.cpp @@ -252,7 +252,8 @@ JSRuntime::JSRuntime(JSRuntime* parentRuntime) moduleResolveHook(), moduleMetadataHook(), moduleDynamicImportHook(), - scriptPrivateFinalizeHook() + scriptPrivateAddRefHook(), + scriptPrivateReleaseHook() { setGCStoreBufferPtr(&gc.storeBuffer); diff --git a/js/src/vm/Runtime.h b/js/src/vm/Runtime.h index 13523844c1..6f781d101c 100644 --- a/js/src/vm/Runtime.h +++ b/js/src/vm/Runtime.h @@ -1316,8 +1316,21 @@ struct JSRuntime : public JS::shadow::Runtime, // HostImportModuleDynamically. JS::ModuleDynamicImportHook moduleDynamicImportHook; - // A hook called on script finalization. - JS::ScriptPrivateFinalizeHook scriptPrivateFinalizeHook; + // Hooks called when script private references are created and destroyed. + JS::ScriptPrivateReferenceHook scriptPrivateAddRefHook; + JS::ScriptPrivateReferenceHook scriptPrivateReleaseHook; + + void addRefScriptPrivate(const JS::Value& value) { + if (!value.isUndefined() && scriptPrivateAddRefHook) { + scriptPrivateAddRefHook(value); + } + } + + void releaseScriptPrivate(const JS::Value& value) { + if (!value.isUndefined() && scriptPrivateReleaseHook) { + scriptPrivateReleaseHook(value); + } + } }; namespace js {