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

@@ -62,20 +62,33 @@ void LoadedScript::AssociateWithScript(JSScript* aScript) {
   AddRef();
 }
 
-void HostFinalizeTopLevelScript(JSFreeOp* aFop, const JS::Value& aPrivate) {
+inline void CheckModuleScriptPrivate(LoadedScript* script,
-  // Decrement the reference count of a LoadedScript object that is
+                                     const JS::Value& aPrivate) {
-  // pointed to by a dying JSScript. The reference count was
-  // originally incremented by AssociateWithScript() above.
-
-  auto script = static_cast<LoadedScript*>(aPrivate.toPrivate());
-
 #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<LoadedScript*>(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<LoadedScript*>(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<JSObject*> aModuleRecord)
   MOZ_ASSERT(JS::GetModulePrivate(mModuleRecord).isUndefined());
   JS::SetModulePrivate(mModuleRecord, JS::PrivateValue(this));
   HoldJSObjects(this);
-  AddRef();
 }
 
 void

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<JSObject*> mModuleRecord;
   JS::Heap<JS::Value> mParseError;
   JS::Heap<JS::Value> mErrorToRethrow;
 
@@ -74,6 +74,8 @@ public:
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(ModuleScript,
                                                          LoadedScript)
 
+  JS::Heap<JSObject*> mModuleRecord;
+
   ModuleScript(ScriptFetchOptions* aFetchOptions, nsIURI* aBaseURL);
 
   void SetModuleRecord(JS::Handle<JSObject*> 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() {

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<ScriptLoadRequest> 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();

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<PromiseObject*> promise = promiseArg.as<PromiseObject>();
 
+    auto releasePrivate = mozilla::MakeScopeExit(
+        [&] { cx->runtime()->releaseScriptPrivate(referencingPrivate); });
+
     if (cx->isExceptionPending()) {
         return RejectPromiseWithPendingError(cx, promise);
     }

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<ModuleObject>().scriptSourceObject()->setPrivate(value);
+    JSRuntime* rt = module->compartment()->runtimeFromMainThread();
+    module->as<ModuleObject>().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)

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
+ * Hooks called when references to a script private value are created or
- * set with SetScriptPrivate() or SetModulePrivate() is finalized. This can be
+ * destroyed. This allows use of a reference counted object as the
- * used to clean up the private state. The private value is passed as an
+ * script private.
- * argument.
  */
-using ScriptPrivateFinalizeHook = void (*)(JSFreeOp*, const JS::Value&);
+using ScriptPrivateReferenceHook = void (*)(const JS::Value&);
-
-/**
- * Get the script private finalize hook for the runtime.
- */
-extern JS_PUBLIC_API(ScriptPrivateFinalizeHook)
-GetScriptPrivateFinalizeHook(JSContext* cx);
 
 /**
  * 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

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();
+    // Clear the private value, calling the release hook if necessary.
-    if (!value.isUndefined()) {
+    sso->setPrivate(fop->runtime(), UndefinedValue());
-        // The embedding may need to dispose of its private data.
-        JS::AutoSuppressGCAnalysis suppressGC;
-        if (JS::ScriptPrivateFinalizeHook hook =
-            fop->runtime()->scriptPrivateFinalizeHook) {
-                hook(fop, value);
-            }
-    }
 }
 
 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)
 {

js/src/jsscript.h

@@ -675,9 +675,7 @@ class ScriptSourceObject : public NativeObject
         return static_cast<JSScript*>(untyped);
     }
 
-    void setPrivate(const Value& value) {
+    void setPrivate(JSRuntime* rt, const Value& value);
-        setReservedSlot(PRIVATE_SLOT, value);
-    }
 
     Value getPrivate() const {
         return getReservedSlot(PRIVATE_SLOT);

js/src/vm/Runtime.cpp

@@ -252,7 +252,8 @@ JSRuntime::JSRuntime(JSRuntime* parentRuntime)
     moduleResolveHook(),
     moduleMetadataHook(),
     moduleDynamicImportHook(),
-    scriptPrivateFinalizeHook()
+    scriptPrivateAddRefHook(),
+    scriptPrivateReleaseHook()
 {
     setGCStoreBufferPtr(&gc.storeBuffer);
 

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.
+    // Hooks called when script private references are created and destroyed.
-    JS::ScriptPrivateFinalizeHook scriptPrivateFinalizeHook;
+    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 {