From 4413fcfdaaef91104cbc5d6664faa7b21bb907ce Mon Sep 17 00:00:00 2001 From: "Matt A. Tobin" Date: Sat, 22 Aug 2020 09:48:34 -0400 Subject: [PATCH 01/33] Update old.configure for --enable-phoenix-extensions --- build/moz.configure/old.configure | 1 + 1 file changed, 1 insertion(+) diff --git a/build/moz.configure/old.configure b/build/moz.configure/old.configure index 9d1a33c66e..32db54184d 100644 --- a/build/moz.configure/old.configure +++ b/build/moz.configure/old.configure @@ -311,6 +311,7 @@ def old_configure_options(*options): '--disable-browser-statusbar', '--disable-sync', '--disable-personas', + '--enable-phoenix-extensions', # Below are configure flags used by Basilisk '--disable-webextensions', From 933fb5441577a7ae6f42825d909d4081951df393 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Wed, 8 Jul 2020 10:50:53 +0000 Subject: [PATCH 02/33] Issue #618 - Fix error events fired when loading JS module dependencies fail When module dependencies fail, don't spam with errors for each import; only fire the error event once. Ref: BZ 1421259 --- dom/script/ScriptElement.cpp | 4 ++-- dom/script/ScriptLoader.cpp | 3 ++- dom/script/ScriptLoader.h | 3 ++- dom/script/nsIScriptLoaderObserver.idl | 9 +++++---- dom/xslt/xslt/txMozillaXMLOutput.cpp | 2 +- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/dom/script/ScriptElement.cpp b/dom/script/ScriptElement.cpp index 0cb17dcb06..9cb239c66c 100644 --- a/dom/script/ScriptElement.cpp +++ b/dom/script/ScriptElement.cpp @@ -21,11 +21,11 @@ using namespace mozilla::dom; NS_IMETHODIMP ScriptElement::ScriptAvailable(nsresult aResult, nsIScriptElement *aElement, - bool aIsInline, + bool aIsInlineClassicScript, nsIURI *aURI, int32_t aLineNo) { - if (!aIsInline && NS_FAILED(aResult)) { + if (!aIsInlineClassicScript && NS_FAILED(aResult)) { nsCOMPtr parser = do_QueryReferent(mCreatorParser); if (parser) { parser->PushDefinedInsertionPoint(); diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index a530989742..1426c30c9c 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -2327,7 +2327,8 @@ ScriptLoader::OnStreamComplete(nsIIncrementalStreamLoader* aLoader, MOZ_ASSERT(!modReq->IsTopLevel()); MOZ_ASSERT(!modReq->isInList()); modReq->Cancel(); - FireScriptAvailable(rv, request); + // A single error is fired for the top level module, so don't use + // FireScriptAvailable here. } else if (mParserBlockingRequest == request) { MOZ_ASSERT(!request->isInList()); mParserBlockingRequest = nullptr; diff --git a/dom/script/ScriptLoader.h b/dom/script/ScriptLoader.h index e6b75bf3bb..b07dd4ec66 100644 --- a/dom/script/ScriptLoader.h +++ b/dom/script/ScriptLoader.h @@ -100,7 +100,8 @@ public: void FireScriptAvailable(nsresult aResult) { - mElement->ScriptAvailable(aResult, mElement, mIsInline, mURI, mLineNo); + bool isInlineClassicScript = mIsInline && !IsModuleRequest(); + mElement->ScriptAvailable(aResult, mElement, isInlineClassicScript, mURI, mLineNo); } void FireScriptEvaluated(nsresult aResult) { diff --git a/dom/script/nsIScriptLoaderObserver.idl b/dom/script/nsIScriptLoaderObserver.idl index ed71965255..880738360d 100644 --- a/dom/script/nsIScriptLoaderObserver.idl +++ b/dom/script/nsIScriptLoaderObserver.idl @@ -20,15 +20,16 @@ interface nsIScriptLoaderObserver : nsISupports { * a script. If this is a failure code, script evaluation * will not occur. * @param aElement The element being processed. - * @param aIsInline Is this an inline script or externally loaded? + * @param aIsInline Is this an inline classic script (as opposed to an + * externally loaded classic script or module script)? * @param aURI What is the URI of the script (the document URI if * it is inline). * @param aLineNo At what line does the script appear (generally 1 * if it is a loaded script). */ - void scriptAvailable(in nsresult aResult, + void scriptAvailable(in nsresult aResult, in nsIScriptElement aElement, - in boolean aIsInline, + in boolean aIsInlineClassicScript, in nsIURI aURI, in int32_t aLineNo); @@ -40,7 +41,7 @@ interface nsIScriptLoaderObserver : nsISupports { * @param aElement The element being processed. * @param aIsInline Is this an inline script or externally loaded? */ - void scriptEvaluated(in nsresult aResult, + void scriptEvaluated(in nsresult aResult, in nsIScriptElement aElement, in boolean aIsInline); diff --git a/dom/xslt/xslt/txMozillaXMLOutput.cpp b/dom/xslt/xslt/txMozillaXMLOutput.cpp index 704d8ac110..21b3c228f2 100644 --- a/dom/xslt/xslt/txMozillaXMLOutput.cpp +++ b/dom/xslt/xslt/txMozillaXMLOutput.cpp @@ -955,7 +955,7 @@ NS_IMPL_ISUPPORTS(txTransformNotifier, NS_IMETHODIMP txTransformNotifier::ScriptAvailable(nsresult aResult, nsIScriptElement *aElement, - bool aIsInline, + bool aIsInlineClassicScript, nsIURI *aURI, int32_t aLineNo) { From 0d874713ef18edd8d150945117ae6577140a7b63 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Wed, 8 Jul 2020 12:58:30 +0000 Subject: [PATCH 03/33] Issue #618 - Align error handling for module scripts with the spec (again) This updates module implementation to match spec regarding handling of instantiation errors, after it was changed yet again, this time to not remember instantiation errors, but instead immediately rethrow applicable ones. Ref: BZ 1420420 --- dom/script/ModuleLoadRequest.cpp | 4 +- dom/script/ModuleScript.cpp | 43 +-- dom/script/ModuleScript.h | 12 +- dom/script/ScriptLoader.cpp | 77 +++-- dom/script/ScriptLoader.h | 1 + js/src/builtin/Module.js | 292 +++++++++--------- js/src/builtin/ModuleObject.cpp | 40 ++- js/src/builtin/ModuleObject.h | 13 +- js/src/builtin/SelfHostingDefines.h | 14 +- js/src/builtin/TestingFunctions.cpp | 6 + js/src/jit-test/tests/modules/bug-1284486.js | 12 +- .../jit-test/tests/modules/bug-1420420-2.js | 19 ++ .../jit-test/tests/modules/bug-1420420-3.js | 9 + .../jit-test/tests/modules/bug-1420420-4.js | 16 + js/src/jit-test/tests/modules/bug-1420420.js | 19 ++ js/src/js.msg | 1 - js/src/jsapi.cpp | 12 - js/src/jsapi.h | 6 - js/src/vm/EnvironmentObject.cpp | 2 +- js/src/vm/SelfHosting.cpp | 1 - 20 files changed, 332 insertions(+), 267 deletions(-) create mode 100644 js/src/jit-test/tests/modules/bug-1420420-2.js create mode 100644 js/src/jit-test/tests/modules/bug-1420420-3.js create mode 100644 js/src/jit-test/tests/modules/bug-1420420-4.js create mode 100644 js/src/jit-test/tests/modules/bug-1420420.js diff --git a/dom/script/ModuleLoadRequest.cpp b/dom/script/ModuleLoadRequest.cpp index d622143044..a12fcb1629 100644 --- a/dom/script/ModuleLoadRequest.cpp +++ b/dom/script/ModuleLoadRequest.cpp @@ -83,7 +83,7 @@ ModuleLoadRequest::ModuleLoaded() // been loaded. mModuleScript = mLoader->GetFetchedModule(mURI); - if (!mModuleScript || mModuleScript->IsErrored()) { + if (!mModuleScript || mModuleScript->HasParseError()) { ModuleErrored(); return; } @@ -95,7 +95,7 @@ void ModuleLoadRequest::ModuleErrored() { mLoader->CheckModuleDependenciesLoaded(this); - MOZ_ASSERT(!mModuleScript || mModuleScript->IsErrored()); + MOZ_ASSERT(!mModuleScript || mModuleScript->HasParseError()); CancelImports(); SetReady(); diff --git a/dom/script/ModuleScript.cpp b/dom/script/ModuleScript.cpp index 28b97a3cb5..1bf9d0b0fc 100644 --- a/dom/script/ModuleScript.cpp +++ b/dom/script/ModuleScript.cpp @@ -26,7 +26,8 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ModuleScript) NS_IMPL_CYCLE_COLLECTION_UNLINK(mLoader) NS_IMPL_CYCLE_COLLECTION_UNLINK(mBaseURL) tmp->UnlinkModuleRecord(); - tmp->mError.setUndefined(); + tmp->mParseError.setUndefined(); + tmp->mErrorToRethrow.setUndefined(); NS_IMPL_CYCLE_COLLECTION_UNLINK_END NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(ModuleScript) @@ -35,7 +36,8 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(ModuleScript) NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mModuleRecord) - NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mError) + NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mParseError) + NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mErrorToRethrow) NS_IMPL_CYCLE_COLLECTION_TRACE_END NS_IMPL_CYCLE_COLLECTING_ADDREF(ModuleScript) @@ -48,7 +50,8 @@ ModuleScript::ModuleScript(ScriptLoader *aLoader, nsIURI* aBaseURL) MOZ_ASSERT(mLoader); MOZ_ASSERT(mBaseURL); MOZ_ASSERT(!mModuleRecord); - MOZ_ASSERT(mError.isUndefined()); + MOZ_ASSERT(!HasParseError()); + MOZ_ASSERT(!HasErrorToRethrow()); } void @@ -74,7 +77,8 @@ void ModuleScript::SetModuleRecord(JS::Handle aModuleRecord) { MOZ_ASSERT(!mModuleRecord); - MOZ_ASSERT(mError.isUndefined()); + MOZ_ASSERT(!HasParseError()); + MOZ_ASSERT(!HasErrorToRethrow()); mModuleRecord = aModuleRecord; @@ -85,37 +89,24 @@ ModuleScript::SetModuleRecord(JS::Handle aModuleRecord) } void -ModuleScript::SetPreInstantiationError(const JS::Value& aError) +ModuleScript::SetParseError(const JS::Value& aError) { MOZ_ASSERT(!aError.isUndefined()); + MOZ_ASSERT(!HasParseError()); + MOZ_ASSERT(!HasErrorToRethrow()); UnlinkModuleRecord(); - mError = aError; - + mParseError = aError; HoldJSObjects(this); } -bool -ModuleScript::IsErrored() const +void +ModuleScript::SetErrorToRethrow(const JS::Value& aError) { - if (!mModuleRecord) { - MOZ_ASSERT(!mError.isUndefined()); - return true; - } + MOZ_ASSERT(!aError.isUndefined()); + MOZ_ASSERT(!HasErrorToRethrow()); - return JS::IsModuleErrored(mModuleRecord); -} - -JS::Value -ModuleScript::Error() const -{ - MOZ_ASSERT(IsErrored()); - - if (!mModuleRecord) { - return mError; - } - - return JS::GetModuleError(mModuleRecord); + mErrorToRethrow = aError; } } // dom namespace diff --git a/dom/script/ModuleScript.h b/dom/script/ModuleScript.h index 5713598592..f765aa0fad 100644 --- a/dom/script/ModuleScript.h +++ b/dom/script/ModuleScript.h @@ -23,7 +23,8 @@ class ModuleScript final : public nsISupports RefPtr mLoader; nsCOMPtr mBaseURL; JS::Heap mModuleRecord; - JS::Heap mError; + JS::Heap mParseError; + JS::Heap mErrorToRethrow; ~ModuleScript(); @@ -35,14 +36,17 @@ public: nsIURI* aBaseURL); void SetModuleRecord(JS::Handle aModuleRecord); - void SetPreInstantiationError(const JS::Value& aError); + void SetParseError(const JS::Value& aError); + void SetErrorToRethrow(const JS::Value& aError); ScriptLoader* Loader() const { return mLoader; } JSObject* ModuleRecord() const { return mModuleRecord; } nsIURI* BaseURL() const { return mBaseURL; } - bool IsErrored() const; - JS::Value Error() const; + JS::Value ParseError() const { return mParseError; } + JS::Value ErrorToRethrow() const { return mErrorToRethrow; } + bool HasParseError() const { return !mParseError.isUndefined(); } + bool HasErrorToRethrow() const { return !mErrorToRethrow.isUndefined(); } void UnlinkModuleRecord(); }; diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 1426c30c9c..3dbbcaceae 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -493,7 +493,7 @@ ScriptLoader::ProcessFetchedModuleSource(ModuleLoadRequest* aRequest) return rv; } - if (!aRequest->mModuleScript->IsErrored()) { + if (!aRequest->mModuleScript->HasParseError()) { StartFetchingModuleDependencies(aRequest); } @@ -568,7 +568,7 @@ ScriptLoader::CreateModuleScript(ModuleLoadRequest* aRequest) return NS_ERROR_FAILURE; } - moduleScript->SetPreInstantiationError(error); + moduleScript->SetParseError(error); aRequest->ModuleErrored(); return NS_OK; } @@ -580,8 +580,6 @@ ScriptLoader::CreateModuleScript(ModuleLoadRequest* aRequest) nsCOMArray urls; rv = ResolveRequestedModules(aRequest, urls); if (NS_FAILED(rv)) { - // ResolveRequestedModules sets pre-instanitation error on failure. - MOZ_ASSERT(moduleScript->IsErrored()); aRequest->ModuleErrored(); return NS_OK; } @@ -627,7 +625,7 @@ HandleResolveFailure(JSContext* aCx, ModuleScript* aScript, return NS_ERROR_OUT_OF_MEMORY; } - aScript->SetPreInstantiationError(error); + aScript->SetParseError(error); return NS_OK; } @@ -723,7 +721,6 @@ ResolveRequestedModules(ModuleLoadRequest* aRequest, nsCOMArray &aUrls) } // Let url be the result of resolving a module specifier given module script and requested. - ModuleScript* ms = aRequest->mModuleScript; nsCOMPtr uri = ResolveModuleSpecifier(ms, specifier); if (!uri) { nsresult rv = HandleResolveFailure(cx, ms, specifier); @@ -746,7 +743,7 @@ void ScriptLoader::StartFetchingModuleDependencies(ModuleLoadRequest* aRequest) { MOZ_ASSERT(aRequest->mModuleScript); - MOZ_ASSERT(!aRequest->mModuleScript->IsErrored()); + MOZ_ASSERT(!aRequest->mModuleScript->HasParseError()); MOZ_ASSERT(!aRequest->IsReadyToRun()); aRequest->mProgress = ModuleLoadRequest::Progress::FetchingImports; @@ -843,7 +840,7 @@ HostResolveImportedModule(JSContext* aCx, unsigned argc, JS::Value* vp) ModuleScript* ms = script->Loader()->GetFetchedModule(uri); MOZ_ASSERT(ms, "Resolved module not found in module map"); - MOZ_ASSERT(!ms->IsErrored()); + MOZ_ASSERT(!ms->HasParseError()); *vp = JS::ObjectValue(*ms->ModuleRecord()); return true; @@ -871,18 +868,16 @@ void ScriptLoader::CheckModuleDependenciesLoaded(ModuleLoadRequest* aRequest) { RefPtr moduleScript = aRequest->mModuleScript; - if (moduleScript && !moduleScript->IsErrored()) { - for (auto childRequest : aRequest->mImports) { - ModuleScript* childScript = childRequest->mModuleScript; - if (!childScript) { - // Load error - aRequest->mModuleScript = nullptr; - return; - } else if (childScript->IsErrored()) { - // Script error - moduleScript->SetPreInstantiationError(childScript->Error()); - return; - } + if (!moduleScript || moduleScript->HasParseError()) { + return; + } + + for (auto childRequest : aRequest->mImports) { + ModuleScript* childScript = childRequest->mModuleScript; + if (!childScript) { + aRequest->mModuleScript = nullptr; + // Load error on script load request; bail. + return; } } } @@ -892,7 +887,7 @@ ScriptLoader::ProcessLoadedModuleTree(ModuleLoadRequest* aRequest) { if (aRequest->IsTopLevel()) { ModuleScript* moduleScript = aRequest->mModuleScript; - if (moduleScript && !moduleScript->IsErrored()) { + if (moduleScript && !moduleScript->HasErrorToRethrow()) { if (!InstantiateModuleTree(aRequest)) { aRequest->mModuleScript = nullptr; } @@ -906,6 +901,28 @@ ScriptLoader::ProcessLoadedModuleTree(ModuleLoadRequest* aRequest) } } +JS::Value +ScriptLoader::FindFirstParseError(ModuleLoadRequest* aRequest) +{ + MOZ_ASSERT(aRequest); + + ModuleScript* moduleScript = aRequest->mModuleScript; + MOZ_ASSERT(moduleScript); + + if (moduleScript->HasParseError()) { + return moduleScript->ParseError(); + } + + for (ModuleLoadRequest* childRequest : aRequest->mImports) { + JS::Value error = FindFirstParseError(childRequest); + if (!error.isUndefined()) { + return error; + } + } + + return JS::UndefinedValue(); +} + bool ScriptLoader::InstantiateModuleTree(ModuleLoadRequest* aRequest) { @@ -916,6 +933,14 @@ ScriptLoader::InstantiateModuleTree(ModuleLoadRequest* aRequest) ModuleScript* moduleScript = aRequest->mModuleScript; MOZ_ASSERT(moduleScript); + + JS::Value parseError = FindFirstParseError(aRequest); + if (!parseError.isUndefined()) { + // Parse error found in the requested script + moduleScript->SetErrorToRethrow(parseError); + return true; + } + MOZ_ASSERT(moduleScript->ModuleRecord()); nsAutoMicroTask mt; @@ -937,7 +962,7 @@ ScriptLoader::InstantiateModuleTree(ModuleLoadRequest* aRequest) return false; } MOZ_ASSERT(!exception.isUndefined()); - // Ignore the exception. It will be recorded in the module record. + moduleScript->SetErrorToRethrow(exception); } return true; @@ -1462,7 +1487,7 @@ ScriptLoader::ProcessScriptElement(nsIScriptElement *aElement) } else { AddDeferRequest(request); } - if (!modReq->mModuleScript->IsErrored()) { + if (!modReq->mModuleScript->HasParseError()) { StartFetchingModuleDependencies(modReq); } return false; @@ -1966,9 +1991,9 @@ ScriptLoader::EvaluateScript(ScriptLoadRequest* aRequest) MOZ_ASSERT(!request->mOffThreadToken); ModuleScript* moduleScript = request->mModuleScript; - if (moduleScript->IsErrored()) { - // Module has an error status - JS::Rooted error(cx, moduleScript->Error()); + if (moduleScript->HasErrorToRethrow()) { + // Module has an error status to be rethrown + JS::Rooted error(cx, moduleScript->ErrorToRethrow()); JS_SetPendingException(cx, error); return NS_OK; // An error is reported by AutoEntryScript. } diff --git a/dom/script/ScriptLoader.h b/dom/script/ScriptLoader.h index b07dd4ec66..db2eeed31a 100644 --- a/dom/script/ScriptLoader.h +++ b/dom/script/ScriptLoader.h @@ -583,6 +583,7 @@ private: nsresult ProcessFetchedModuleSource(ModuleLoadRequest* aRequest); void CheckModuleDependenciesLoaded(ModuleLoadRequest* aRequest); void ProcessLoadedModuleTree(ModuleLoadRequest* aRequest); + JS::Value FindFirstParseError(ModuleLoadRequest* aRequest); bool InstantiateModuleTree(ModuleLoadRequest* aRequest); void StartFetchingModuleDependencies(ModuleLoadRequest* aRequest); diff --git a/js/src/builtin/Module.js b/js/src/builtin/Module.js index 64d62d2160..86b44880dc 100644 --- a/js/src/builtin/Module.js +++ b/js/src/builtin/Module.js @@ -69,8 +69,13 @@ function ModuleSetStatus(module, newStatus) { assert(newStatus >= MODULE_STATUS_ERRORED && newStatus <= MODULE_STATUS_EVALUATED, "Bad new module status in ModuleSetStatus"); - if (newStatus !== MODULE_STATUS_ERRORED) - assert(newStatus > module.status, "New module status inconsistent with current status"); + + // Note that under OOM conditions we can fail the module instantiation + // process even after modules have been marked as instantiated. + assert((module.status <= MODULE_STATUS_INSTANTIATED && + newStatus === MODULE_STATUS_UNINSTANTIATED) || + newStatus > module.status, + "New module status inconsistent with current status"); UnsafeSetReservedSlot(module, MODULE_OBJECT_STATUS_SLOT, newStatus); } @@ -80,20 +85,15 @@ function ModuleSetStatus(module, newStatus) // Returns an object describing the location of the resolved export or // indicating a failure. // -// On success this returns: { resolved: true, module, bindingName } +// On success this returns a resolved binding record: { module, bindingName } // -// There are three failure cases: +// There are two failure cases: // -// - The resolution failure can be blamed on a particular module. -// Returns: { resolved: false, module, ambiguous: false } +// - If no definition was found or the request is found to be circular, *null* +// is returned. // -// - No culprit can be determined and the resolution failure was due to star -// export ambiguity. -// Returns: { resolved: false, module: null, ambiguous: true } -// -// - No culprit can be determined and the resolution failure was not due to -// star export ambiguity. -// Returns: { resolved: false, module: null, ambiguous: false } +// - If the request is found to be ambiguous, the string `"ambiguous"` is +// returned. // function ModuleResolveExport(exportName, resolveSet = []) { @@ -106,53 +106,47 @@ function ModuleResolveExport(exportName, resolveSet = []) let module = this; // Step 2 - assert(module.status !== MODULE_STATUS_ERRORED, "Bad module status in ResolveExport"); - - // Step 3 for (let i = 0; i < resolveSet.length; i++) { let r = resolveSet[i]; if (r.module === module && r.exportName === exportName) { // This is a circular import request. - return {resolved: false, module: null, ambiguous: false}; + return null; } } - // Step 4 + // Step 3 _DefineDataProperty(resolveSet, resolveSet.length, {module: module, exportName: exportName}); - // Step 5 + // Step 4 let localExportEntries = module.localExportEntries; for (let i = 0; i < localExportEntries.length; i++) { let e = localExportEntries[i]; if (exportName === e.exportName) - return {resolved: true, module, bindingName: e.localName}; + return {module, bindingName: e.localName}; } - // Step 6 + // Step 5 let indirectExportEntries = module.indirectExportEntries; for (let i = 0; i < indirectExportEntries.length; i++) { let e = indirectExportEntries[i]; if (exportName === e.exportName) { let importedModule = CallModuleResolveHook(module, e.moduleRequest, MODULE_STATUS_UNINSTANTIATED); - let resolution = callFunction(importedModule.resolveExport, importedModule, e.importName, - resolveSet); - if (!resolution.resolved && !resolution.module) - resolution.module = module; - return resolution; + return callFunction(importedModule.resolveExport, importedModule, e.importName, + resolveSet); } } - // Step 7 + // Step 6 if (exportName === "default") { // A default export cannot be provided by an export *. - return {resolved: false, module: null, ambiguous: false}; + return null; } - // Step 8 + // Step 7 let starResolution = null; - // Step 9 + // Step 8 let starExportEntries = module.starExportEntries; for (let i = 0; i < starExportEntries.length; i++) { let e = starExportEntries[i]; @@ -160,27 +154,31 @@ function ModuleResolveExport(exportName, resolveSet = []) MODULE_STATUS_UNINSTANTIATED); let resolution = callFunction(importedModule.resolveExport, importedModule, exportName, resolveSet); - if (!resolution.resolved && (resolution.module || resolution.ambiguous)) + if (resolution === "ambiguous") return resolution; - if (resolution.resolved) { + if (resolution !== null) { if (starResolution === null) { starResolution = resolution; } else { if (resolution.module !== starResolution.module || resolution.bindingName !== starResolution.bindingName) { - return {resolved: false, module: null, ambiguous: true}; + return "ambiguous"; } } } } - // Step 10 - if (starResolution !== null) - return starResolution; + // Step 9 + return starResolution; +} - return {resolved: false, module: null, ambiguous: false}; +function IsResolvedBinding(resolution) +{ + assert(resolution === "ambiguous" || typeof resolution === "object", + "Bad module resolution result"); + return typeof resolution === "object" && resolution !== null; } // 15.2.1.18 GetModuleNamespace(module) @@ -189,12 +187,12 @@ function GetModuleNamespace(module) // Step 1 assert(IsModule(module), "GetModuleNamespace called with non-module"); - // Step 2 + // Steps 2-3 assert(module.status !== MODULE_STATUS_UNINSTANTIATED && - module.status !== MODULE_STATUS_ERRORED, + module.status !== MODULE_STATUS_EVALUATED_ERROR, "Bad module status in GetModuleNamespace"); - // Step 3 + // Step 4 let namespace = module.namespace; if (typeof namespace === "undefined") { @@ -203,7 +201,7 @@ function GetModuleNamespace(module) for (let i = 0; i < exportedNames.length; i++) { let name = exportedNames[i]; let resolution = callFunction(module.resolveExport, module, name); - if (resolution.resolved) + if (IsResolvedBinding(resolution)) _DefineDataProperty(unambiguousNames, unambiguousNames.length, name); } namespace = ModuleNamespaceCreate(module, unambiguousNames); @@ -225,7 +223,7 @@ function ModuleNamespaceCreate(module, exports) for (let i = 0; i < exports.length; i++) { let name = exports[i]; let binding = callFunction(module.resolveExport, module, name); - assert(binding.resolved, "Failed to resolve binding"); + assert(IsResolvedBinding(binding), "Failed to resolve binding"); AddModuleNamespaceBinding(ns, name, binding.module, binding.bindingName); } @@ -236,11 +234,6 @@ function GetModuleEnvironment(module) { assert(IsModule(module), "Non-module passed to GetModuleEnvironment"); - // Check for a previous failed attempt to instantiate this module. This can - // only happen due to a bug in the module loader. - if (module.status === MODULE_STATUS_ERRORED) - ThrowInternalError(JSMSG_MODULE_INSTANTIATE_FAILED, module.status); - let env = UnsafeGetReservedSlot(module, MODULE_OBJECT_ENVIRONMENT_SLOT); assert(env === undefined || IsModuleEnvironment(env), "Module environment slot contains unexpected value"); @@ -248,19 +241,6 @@ function GetModuleEnvironment(module) return env; } -function RecordModuleError(module, error) -{ - // Set the module's status to 'errored' to indicate a failed module - // instantiation and record the exception. The environment slot is also - // reset to 'undefined'. - - assert(IsObject(module) && IsModule(module), "Non-module passed to RecordModuleError"); - - ModuleSetStatus(module, MODULE_STATUS_ERRORED); - UnsafeSetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT, error); - UnsafeSetReservedSlot(module, MODULE_OBJECT_ENVIRONMENT_SLOT, undefined); -} - function CountArrayValues(array, value) { let count = 0; @@ -280,6 +260,16 @@ function ArrayContains(array, value) return false; } +function HandleModuleInstantiationFailure(module) +{ + // Reset the module to the "uninstantiated" state. Don't reset the + // environment slot as the environment object will be required by any + // possible future instantiation attempt. + ModuleSetStatus(module, MODULE_STATUS_UNINSTANTIATED); + UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_INDEX_SLOT, undefined); + UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT, undefined); +} + // 15.2.1.16.4 ModuleInstantiate() function ModuleInstantiate() { @@ -301,38 +291,30 @@ function ModuleInstantiate() // Steps 4-5 try { - InnerModuleDeclarationInstantiation(module, stack, 0); + InnerModuleInstantiation(module, stack, 0); } catch (error) { for (let i = 0; i < stack.length; i++) { let m = stack[i]; - assert(m.status === MODULE_STATUS_INSTANTIATING || - m.status === MODULE_STATUS_ERRORED, - "Bad module status after failed instantiation"); - - RecordModuleError(m, error); + assert(m.status === MODULE_STATUS_INSTANTIATING, + "Expected instantiating status during failed instantiation"); + HandleModuleInstantiationFailure(m); } - if (stack.length === 0 && - typeof(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT)) === "undefined") - { - // This can happen due to OOM when appending to the stack. - assert(error === "out of memory", - "Stack must contain module unless we hit OOM"); - RecordModuleError(module, error); - } + // Handle OOM when appending to the stack or over-recursion errors. + if (stack.length === 0) + HandleModuleInstantiationFailure(module); - assert(module.status === MODULE_STATUS_ERRORED, - "Bad module status after failed instantiation"); - assert(SameValue(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT), error), - "Module has different error set after failed instantiation"); + assert(module.status === MODULE_STATUS_UNINSTANTIATED, + "Expected uninstantiated status after failed instantiation"); throw error; } // Step 6 - assert(module.status == MODULE_STATUS_INSTANTIATED || - module.status == MODULE_STATUS_EVALUATED, + assert(module.status === MODULE_STATUS_INSTANTIATED || + module.status === MODULE_STATUS_EVALUATED || + module.status === MODULE_STATUS_EVALUATED_ERROR, "Bad module status after successful instantiation"); // Step 7 @@ -344,8 +326,8 @@ function ModuleInstantiate() } _SetCanonicalName(ModuleInstantiate, "ModuleInstantiate"); -// 15.2.1.16.4.1 InnerModuleDeclarationInstantiation(module, stack, index) -function InnerModuleDeclarationInstantiation(module, stack, index) +// 15.2.1.16.4.1 InnerModuleInstantiation(module, stack, index) +function InnerModuleInstantiation(module, stack, index) { // Step 1 // TODO: Support module records other than source text module records. @@ -353,42 +335,40 @@ function InnerModuleDeclarationInstantiation(module, stack, index) // Step 2 if (module.status === MODULE_STATUS_INSTANTIATING || module.status === MODULE_STATUS_INSTANTIATED || - module.status === MODULE_STATUS_EVALUATED) + module.status === MODULE_STATUS_EVALUATED || + module.status === MODULE_STATUS_EVALUATED_ERROR) { return index; } // Step 3 - if (module.status === MODULE_STATUS_ERRORED) - throw module.error; - - // Step 4 assert(module.status === MODULE_STATUS_UNINSTANTIATED, "Bad module status in ModuleDeclarationInstantiation"); - // Steps 5 + // Step 4 ModuleSetStatus(module, MODULE_STATUS_INSTANTIATING); - // Step 6-8 + // Steps 5-7 UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_INDEX_SLOT, index); UnsafeSetReservedSlot(module, MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT, index); index++; - // Step 9 + // Step 8 _DefineDataProperty(stack, stack.length, module); - // Step 10 + // Step 9 let requestedModules = module.requestedModules; for (let i = 0; i < requestedModules.length; i++) { let required = requestedModules[i]; - let requiredModule = CallModuleResolveHook(module, required, MODULE_STATUS_ERRORED); + let requiredModule = CallModuleResolveHook(module, required, MODULE_STATUS_UNINSTANTIATED); - index = InnerModuleDeclarationInstantiation(requiredModule, stack, index); + index = InnerModuleInstantiation(requiredModule, stack, index); assert(requiredModule.status === MODULE_STATUS_INSTANTIATING || requiredModule.status === MODULE_STATUS_INSTANTIATED || - requiredModule.status === MODULE_STATUS_EVALUATED, - "Bad required module status after InnerModuleDeclarationInstantiation"); + requiredModule.status === MODULE_STATUS_EVALUATED || + requiredModule.status === MODULE_STATUS_EVALUATED_ERROR, + "Bad required module status after InnerModuleInstantiation"); assert((requiredModule.status === MODULE_STATUS_INSTANTIATING) === ArrayContains(stack, requiredModule), @@ -404,16 +384,16 @@ function InnerModuleDeclarationInstantiation(module, stack, index) } } - // Step 11 + // Step 10 ModuleDeclarationEnvironmentSetup(module); - // Steps 12-13 + // Steps 11-12 assert(CountArrayValues(stack, module) === 1, "Current module should appear exactly once in the stack"); assert(module.dfsAncestorIndex <= module.dfsIndex, "Bad DFS ancestor index"); - // Step 14 + // Step 13 if (module.dfsAncestorIndex === module.dfsIndex) { let requiredModule; do { @@ -434,11 +414,9 @@ function ModuleDeclarationEnvironmentSetup(module) for (let i = 0; i < indirectExportEntries.length; i++) { let e = indirectExportEntries[i]; let resolution = callFunction(module.resolveExport, module, e.exportName); - assert(resolution.resolved || resolution.module, - "Unexpected failure to resolve export in ModuleDeclarationEnvironmentSetup"); - if (!resolution.resolved) { - return ResolutionError(resolution, "indirectExport", e.exportName, - e.lineNumber, e.columnNumber) + if (!IsResolvedBinding(resolution)) { + ThrowResolutionError(module, resolution, "indirectExport", e.exportName, + e.lineNumber, e.columnNumber); } } @@ -458,12 +436,9 @@ function ModuleDeclarationEnvironmentSetup(module) } else { let resolution = callFunction(importedModule.resolveExport, importedModule, imp.importName); - if (!resolution.resolved && !resolution.module) - resolution.module = module; - - if (!resolution.resolved) { - return ResolutionError(resolution, "import", imp.importName, - imp.lineNumber, imp.columnNumber); + if (!IsResolvedBinding(resolution)) { + ThrowResolutionError(module, resolution, "import", imp.importName, + imp.lineNumber, imp.columnNumber); } CreateImportBinding(env, imp.localName, resolution.module, resolution.bindingName); @@ -473,38 +448,58 @@ function ModuleDeclarationEnvironmentSetup(module) InstantiateModuleFunctionDeclarations(module); } -// 15.2.1.16.4.3 ResolutionError(module) -function ResolutionError(resolution, kind, name, line, column) +function ThrowResolutionError(module, resolution, kind, name, line, column) { - let module = resolution.module; - assert(module !== null, - "Null module passed to ResolutionError"); - - assert(module.status === MODULE_STATUS_UNINSTANTIATED || - module.status === MODULE_STATUS_INSTANTIATING, - "Unexpected module status in ResolutionError"); + assert(module.status === MODULE_STATUS_INSTANTIATING, + "Unexpected module status in ThrowResolutionError"); assert(kind === "import" || kind === "indirectExport", - "Unexpected kind in ResolutionError"); + "Unexpected kind in ThrowResolutionError"); assert(line > 0, "Line number should be present for all imports and indirect exports"); + let ambiguous = resolution === "ambiguous"; + let errorNumber; - if (kind === "import") { - errorNumber = resolution.ambiguous ? JSMSG_AMBIGUOUS_IMPORT - : JSMSG_MISSING_IMPORT; - } else { - errorNumber = resolution.ambiguous ? JSMSG_AMBIGUOUS_INDIRECT_EXPORT - : JSMSG_MISSING_INDIRECT_EXPORT; - } + if (kind === "import") + errorNumber = ambiguous ? JSMSG_AMBIGUOUS_IMPORT : JSMSG_MISSING_IMPORT; + else + errorNumber = ambiguous ? JSMSG_AMBIGUOUS_INDIRECT_EXPORT : JSMSG_MISSING_INDIRECT_EXPORT; let message = GetErrorMessage(errorNumber) + ": " + name; let error = CreateModuleSyntaxError(module, line, column, message); - RecordModuleError(module, error); throw error; } +function GetModuleEvaluationError(module) +{ + assert(IsObject(module) && IsModule(module), + "Non-module passed to GetModuleEvaluationError"); + assert(module.status === MODULE_STATUS_EVALUATED_ERROR, + "Bad module status in GetModuleEvaluationError"); + return UnsafeGetReservedSlot(module, MODULE_OBJECT_EVALUATION_ERROR_SLOT); +} + +function RecordModuleEvaluationError(module, error) +{ + // Set the module's EvaluationError slot to indicate a failed module + // evaluation. + + assert(IsObject(module) && IsModule(module), + "Non-module passed to RecordModuleEvaluationError"); + + if (module.status === MODULE_STATUS_EVALUATED_ERROR) { + // It would be nice to assert that |error| is the same as the one we + // previously recorded, but that's not always true in the case of out of + // memory and over-recursion errors. + return; + } + + ModuleSetStatus(module, MODULE_STATUS_EVALUATED_ERROR); + UnsafeSetReservedSlot(module, MODULE_OBJECT_EVALUATION_ERROR_SLOT, error); +} + // 15.2.1.16.5 ModuleEvaluate() function ModuleEvaluate() { @@ -515,9 +510,9 @@ function ModuleEvaluate() let module = this; // Step 2 - if (module.status !== MODULE_STATUS_ERRORED && - module.status !== MODULE_STATUS_INSTANTIATED && - module.status !== MODULE_STATUS_EVALUATED) + if (module.status !== MODULE_STATUS_INSTANTIATED && + module.status !== MODULE_STATUS_EVALUATED && + module.status !== MODULE_STATUS_EVALUATED_ERROR) { ThrowInternalError(JSMSG_BAD_MODULE_STATUS); } @@ -535,27 +530,20 @@ function ModuleEvaluate() assert(m.status === MODULE_STATUS_EVALUATING, "Bad module status after failed evaluation"); - RecordModuleError(m, error); + RecordModuleEvaluationError(m, error); } - if (stack.length === 0 && - typeof(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT)) === "undefined") - { - // This can happen due to OOM when appending to the stack. - assert(error === "out of memory", - "Stack must contain module unless we hit OOM"); - RecordModuleError(module, error); - } + // Handle OOM when appending to the stack or over-recursion errors. + if (stack.length === 0) + RecordModuleEvaluationError(module, error); - assert(module.status === MODULE_STATUS_ERRORED, + assert(module.status === MODULE_STATUS_EVALUATED_ERROR, "Bad module status after failed evaluation"); - assert(SameValue(UnsafeGetReservedSlot(module, MODULE_OBJECT_ERROR_SLOT), error), - "Module has different error set after failed evaluation"); throw error; } - assert(module.status == MODULE_STATUS_EVALUATED, + assert(module.status === MODULE_STATUS_EVALUATED, "Bad module status after successful evaluation"); assert(stack.length === 0, "Stack should be empty after successful evaluation"); @@ -571,19 +559,19 @@ function InnerModuleEvaluation(module, stack, index) // TODO: Support module records other than source text module records. // Step 2 - if (module.status === MODULE_STATUS_EVALUATING || - module.status === MODULE_STATUS_EVALUATED) - { + if (module.status === MODULE_STATUS_EVALUATED_ERROR) + throw GetModuleEvaluationError(module); + + if (module.status === MODULE_STATUS_EVALUATED) return index; - } // Step 3 - if (module.status === MODULE_STATUS_ERRORED) - throw module.error; + if (module.status === MODULE_STATUS_EVALUATING) + return index; // Step 4 assert(module.status === MODULE_STATUS_INSTANTIATED, - "Bad module status in ModuleEvaluation"); + "Bad module status in InnerModuleEvaluation"); // Step 5 ModuleSetStatus(module, MODULE_STATUS_EVALUATING); @@ -605,8 +593,8 @@ function InnerModuleEvaluation(module, stack, index) index = InnerModuleEvaluation(requiredModule, stack, index); - assert(requiredModule.status == MODULE_STATUS_EVALUATING || - requiredModule.status == MODULE_STATUS_EVALUATED, + assert(requiredModule.status === MODULE_STATUS_EVALUATING || + requiredModule.status === MODULE_STATUS_EVALUATED, "Bad module status after InnerModuleEvaluation"); assert((requiredModule.status === MODULE_STATUS_EVALUATING) === diff --git a/js/src/builtin/ModuleObject.cpp b/js/src/builtin/ModuleObject.cpp index 30e7120c06..a9a2571784 100644 --- a/js/src/builtin/ModuleObject.cpp +++ b/js/src/builtin/ModuleObject.cpp @@ -18,10 +18,10 @@ using namespace js; using namespace js::frontend; -static_assert(MODULE_STATUS_ERRORED < MODULE_STATUS_UNINSTANTIATED && - MODULE_STATUS_UNINSTANTIATED < MODULE_STATUS_INSTANTIATING && +static_assert(MODULE_STATUS_UNINSTANTIATED < MODULE_STATUS_INSTANTIATING && MODULE_STATUS_INSTANTIATING < MODULE_STATUS_INSTANTIATED && - MODULE_STATUS_INSTANTIATED < MODULE_STATUS_EVALUATED, + MODULE_STATUS_INSTANTIATED < MODULE_STATUS_EVALUATED && + MODULE_STATUS_EVALUATED < MODULE_STATUS_EVALUATED_ERROR, "Module statuses are ordered incorrectly"); template @@ -273,12 +273,12 @@ IndirectBindingMap::trace(JSTracer* trc) } bool -IndirectBindingMap::putNew(JSContext* cx, HandleId name, - HandleModuleEnvironmentObject environment, HandleId localName) +IndirectBindingMap::put(JSContext* cx, HandleId name, + HandleModuleEnvironmentObject environment, HandleId localName) { RootedShape shape(cx, environment->lookup(cx, localName)); MOZ_ASSERT(shape); - if (!map_.putNew(name, Binding(environment, shape))) { + if (!map_.put(name, Binding(environment, shape))) { ReportOutOfMemory(cx); return false; } @@ -359,7 +359,7 @@ ModuleNamespaceObject::addBinding(JSContext* cx, HandleAtom exportedName, RootedModuleEnvironmentObject environment(cx, &targetModule->initialEnvironment()); RootedId exportedNameId(cx, AtomToId(exportedName)); RootedId localNameId(cx, AtomToId(localName)); - return bindings->putNew(cx, exportedNameId, environment, localNameId); + return bindings->put(cx, exportedNameId, environment, localNameId); } const char ModuleNamespaceObject::ProxyHandler::family = 0; @@ -660,6 +660,9 @@ ModuleObject::finalize(js::FreeOp* fop, JSObject* obj) ModuleEnvironmentObject* ModuleObject::environment() const { +//- MOZ_ASSERT(status() != MODULE_STATUS_ERRORED); +//+ MOZ_ASSERT(!hadEvaluationError()); + Value value = getReservedSlot(EnvironmentSlot); if (value.isUndefined()) return nullptr; @@ -723,7 +726,7 @@ void ModuleObject::init(HandleScript script) { initReservedSlot(ScriptSlot, PrivateValue(script)); - initReservedSlot(StatusSlot, Int32Value(MODULE_STATUS_ERRORED)); + initReservedSlot(StatusSlot, Int32Value(MODULE_STATUS_UNINSTANTIATED)); } void @@ -827,7 +830,8 @@ ModuleObject::script() const static inline void AssertValidModuleStatus(ModuleStatus status) { - MOZ_ASSERT(status >= MODULE_STATUS_ERRORED && status <= MODULE_STATUS_EVALUATED); + MOZ_ASSERT(status >= MODULE_STATUS_UNINSTANTIATED && + status <= MODULE_STATUS_EVALUATED_ERROR); } ModuleStatus @@ -838,11 +842,17 @@ ModuleObject::status() const return status; } -Value -ModuleObject::error() const +bool +ModuleObject::hadEvaluationError() const { - MOZ_ASSERT(status() == MODULE_STATUS_ERRORED); - return getReservedSlot(ErrorSlot); + return status() == MODULE_STATUS_EVALUATED_ERROR; +} + +Value +ModuleObject::evaluationError() const +{ + MOZ_ASSERT(hadEvaluationError()); + return getReservedSlot(EvaluationErrorSlot); } Value @@ -1005,7 +1015,7 @@ ModuleObject::Evaluate(JSContext* cx, HandleModuleObject self) DEFINE_GETTER_FUNCTIONS(ModuleObject, namespace_, NamespaceSlot) DEFINE_GETTER_FUNCTIONS(ModuleObject, status, StatusSlot) -DEFINE_GETTER_FUNCTIONS(ModuleObject, error, ErrorSlot) +DEFINE_GETTER_FUNCTIONS(ModuleObject, evaluationError, EvaluationErrorSlot) DEFINE_GETTER_FUNCTIONS(ModuleObject, requestedModules, RequestedModulesSlot) DEFINE_GETTER_FUNCTIONS(ModuleObject, importEntries, ImportEntriesSlot) DEFINE_GETTER_FUNCTIONS(ModuleObject, localExportEntries, LocalExportEntriesSlot) @@ -1020,7 +1030,7 @@ GlobalObject::initModuleProto(JSContext* cx, Handle global) static const JSPropertySpec protoAccessors[] = { JS_PSG("namespace", ModuleObject_namespace_Getter, 0), JS_PSG("status", ModuleObject_statusGetter, 0), - JS_PSG("error", ModuleObject_errorGetter, 0), + JS_PSG("evaluationError", ModuleObject_evaluationErrorGetter, 0), JS_PSG("requestedModules", ModuleObject_requestedModulesGetter, 0), JS_PSG("importEntries", ModuleObject_importEntriesGetter, 0), JS_PSG("localExportEntries", ModuleObject_localExportEntriesGetter, 0), diff --git a/js/src/builtin/ModuleObject.h b/js/src/builtin/ModuleObject.h index be03152152..5a809c8652 100644 --- a/js/src/builtin/ModuleObject.h +++ b/js/src/builtin/ModuleObject.h @@ -107,8 +107,8 @@ class IndirectBindingMap void trace(JSTracer* trc); - bool putNew(JSContext* cx, HandleId name, - HandleModuleEnvironmentObject environment, HandleId localName); + bool put(JSContext* cx, HandleId name, + HandleModuleEnvironmentObject environment, HandleId localName); size_t count() const { return map_.count(); @@ -218,7 +218,7 @@ class ModuleObject : public NativeObject EnvironmentSlot, NamespaceSlot, StatusSlot, - ErrorSlot, + EvaluationErrorSlot, HostDefinedSlot, RequestedModulesSlot, ImportEntriesSlot, @@ -238,8 +238,8 @@ class ModuleObject : public NativeObject "EnvironmentSlot must match self-hosting define"); static_assert(StatusSlot == MODULE_OBJECT_STATUS_SLOT, "StatusSlot must match self-hosting define"); - static_assert(ErrorSlot == MODULE_OBJECT_ERROR_SLOT, - "ErrorSlot must match self-hosting define"); + static_assert(EvaluationErrorSlot == MODULE_OBJECT_EVALUATION_ERROR_SLOT, + "EvaluationErrorSlot must match self-hosting define"); static_assert(DFSIndexSlot == MODULE_OBJECT_DFS_INDEX_SLOT, "DFSIndexSlot must match self-hosting define"); static_assert(DFSAncestorIndexSlot == MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT, @@ -269,7 +269,8 @@ class ModuleObject : public NativeObject ModuleEnvironmentObject* environment() const; ModuleNamespaceObject* namespace_(); ModuleStatus status() const; - Value error() const; + bool hadEvaluationError() const; + Value evaluationError() const; Value hostDefinedField() const; ArrayObject& requestedModules() const; ArrayObject& importEntries() const; diff --git a/js/src/builtin/SelfHostingDefines.h b/js/src/builtin/SelfHostingDefines.h index a06c2aa62c..6afa641b12 100644 --- a/js/src/builtin/SelfHostingDefines.h +++ b/js/src/builtin/SelfHostingDefines.h @@ -99,15 +99,15 @@ #define MODULE_OBJECT_ENVIRONMENT_SLOT 2 #define MODULE_OBJECT_STATUS_SLOT 4 -#define MODULE_OBJECT_ERROR_SLOT 5 +#define MODULE_OBJECT_EVALUATION_ERROR_SLOT 5 // 4 #define MODULE_OBJECT_DFS_INDEX_SLOT 16 #define MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT 17 -#define MODULE_STATUS_ERRORED 0 -#define MODULE_STATUS_UNINSTANTIATED 1 -#define MODULE_STATUS_INSTANTIATING 2 -#define MODULE_STATUS_INSTANTIATED 3 -#define MODULE_STATUS_EVALUATING 4 -#define MODULE_STATUS_EVALUATED 5 +#define MODULE_STATUS_UNINSTANTIATED 0 +#define MODULE_STATUS_INSTANTIATING 1 +#define MODULE_STATUS_INSTANTIATED 2 +#define MODULE_STATUS_EVALUATING 3 +#define MODULE_STATUS_EVALUATED 4 +#define MODULE_STATUS_EVALUATED_ERROR 5 #endif diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp index 0256207669..e93ebeec3d 100644 --- a/js/src/builtin/TestingFunctions.cpp +++ b/js/src/builtin/TestingFunctions.cpp @@ -3585,6 +3585,9 @@ GetModuleEnvironmentNames(JSContext* cx, unsigned argc, Value* vp) return false; } +//- if (module->status() == MODULE_STATUS_ERRORED) { +//+ if (module->hadEvaluationError()) { + RootedModuleEnvironmentObject env(cx, GetModuleEnvironment(cx, args[0])); Rooted ids(cx, IdVector(cx)); if (!JS_Enumerate(cx, env, &ids)) @@ -3622,6 +3625,9 @@ GetModuleEnvironmentValue(JSContext* cx, unsigned argc, Value* vp) return false; } +//- if (module->status() == MODULE_STATUS_ERRORED) { +//+ if (module->hadEvaluationError()) { + RootedModuleEnvironmentObject env(cx, GetModuleEnvironment(cx, args[0])); RootedString name(cx, args[1].toString()); RootedId id(cx); diff --git a/js/src/jit-test/tests/modules/bug-1284486.js b/js/src/jit-test/tests/modules/bug-1284486.js index 08286393a3..6fbd67d9d9 100644 --- a/js/src/jit-test/tests/modules/bug-1284486.js +++ b/js/src/jit-test/tests/modules/bug-1284486.js @@ -1,9 +1,8 @@ -// This tests that attempting to perform ModuleDeclarationInstantation a second -// time after a failure re-throws the same error. +// This tests that module instantiation can succeed when executed a second +// time after a failure. // // The first attempt fails becuase module 'a' is not available. The second -// attempt fails because of the previous failure (it would otherwise succeed as -// 'a' is now available). +// attempt succeeds as 'a' is now available. load(libdir + "dummyModuleResolveHook.js"); @@ -25,12 +24,9 @@ let a = moduleRepo['a'] = parseModule("export var a = 1; export var b = 2;"); let d = moduleRepo['d'] = parseModule("import { a } from 'c'; a;"); threw = false; -let e2; try { d.declarationInstantiation(); } catch (exc) { threw = true; - e2 = exc; } -assertEq(threw, true); -assertEq(e1, e2); +assertEq(threw, false); diff --git a/js/src/jit-test/tests/modules/bug-1420420-2.js b/js/src/jit-test/tests/modules/bug-1420420-2.js new file mode 100644 index 0000000000..0511e81264 --- /dev/null +++ b/js/src/jit-test/tests/modules/bug-1420420-2.js @@ -0,0 +1,19 @@ +// Test re-instantiation module after failure. + +load(libdir + "asserts.js"); +load(libdir + "dummyModuleResolveHook.js"); + +moduleRepo["good"] = parseModule(`export let x`); + +moduleRepo["y1"] = parseModule(`export let y`); +moduleRepo["y2"] = parseModule(`export let y`); +moduleRepo["bad"] = parseModule(`export* from "y1"; export* from "y2";`); + +moduleRepo["a"] = parseModule(`import* as ns from "good"; import {y} from "bad";`); + +let b = moduleRepo["b"] = parseModule(`import "a";`); +let c = moduleRepo["c"] = parseModule(`import "a";`); + +assertThrowsInstanceOf(() => b.declarationInstantiation(), SyntaxError); +assertThrowsInstanceOf(() => c.declarationInstantiation(), SyntaxError); + diff --git a/js/src/jit-test/tests/modules/bug-1420420-3.js b/js/src/jit-test/tests/modules/bug-1420420-3.js new file mode 100644 index 0000000000..236c023b9c --- /dev/null +++ b/js/src/jit-test/tests/modules/bug-1420420-3.js @@ -0,0 +1,9 @@ +if (!('stackTest' in this)) + quit(); + +let a = parseModule(`throw new Error`); +a.declarationInstantiation(); +stackTest(function() { + a.evaluation(); +}); + diff --git a/js/src/jit-test/tests/modules/bug-1420420-4.js b/js/src/jit-test/tests/modules/bug-1420420-4.js new file mode 100644 index 0000000000..721c770bc7 --- /dev/null +++ b/js/src/jit-test/tests/modules/bug-1420420-4.js @@ -0,0 +1,16 @@ +load(libdir + "asserts.js"); +load(libdir + "dummyModuleResolveHook.js"); + +moduleRepo["a"] = parseModule(`throw undefined`); + +let b = moduleRepo["b"] = parseModule(`import "a";`); +let c = moduleRepo["c"] = parseModule(`import "a";`); + +b.declarationInstantiation(); +c.declarationInstantiation(); + +let count = 0; +try { b.evaluation() } catch (e) { count++; } +try { c.evaluation() } catch (e) { count++; } +assertEq(count, 2); + diff --git a/js/src/jit-test/tests/modules/bug-1420420.js b/js/src/jit-test/tests/modules/bug-1420420.js new file mode 100644 index 0000000000..cd1101c76b --- /dev/null +++ b/js/src/jit-test/tests/modules/bug-1420420.js @@ -0,0 +1,19 @@ +// Test re-instantiation module after failure. + +load(libdir + "asserts.js"); +load(libdir + "dummyModuleResolveHook.js"); + +moduleRepo["good"] = parseModule(`export let x`); + +moduleRepo["y1"] = parseModule(`export let y`); +moduleRepo["y2"] = parseModule(`export let y`); +moduleRepo["bad"] = parseModule(`export* from "y1"; export* from "y2";`); + +moduleRepo["a"] = parseModule(`import {x} from "good"; import {y} from "bad";`); + +let b = moduleRepo["b"] = parseModule(`import "a";`); +let c = moduleRepo["c"] = parseModule(`import "a";`); + +assertThrowsInstanceOf(() => b.declarationInstantiation(), SyntaxError); +assertThrowsInstanceOf(() => c.declarationInstantiation(), SyntaxError); + diff --git a/js/src/js.msg b/js/src/js.msg index 9c508ebbdc..587481864d 100644 --- a/js/src/js.msg +++ b/js/src/js.msg @@ -579,7 +579,6 @@ MSG_DEF(JSMSG_MISSING_IMPORT, 0, JSEXN_SYNTAXERR, "import not found") MSG_DEF(JSMSG_AMBIGUOUS_IMPORT, 0, JSEXN_SYNTAXERR, "ambiguous import") MSG_DEF(JSMSG_MISSING_NAMESPACE_EXPORT, 0, JSEXN_SYNTAXERR, "export not found for namespace") MSG_DEF(JSMSG_MISSING_EXPORT, 1, JSEXN_SYNTAXERR, "local binding for export '{0}' not found") -MSG_DEF(JSMSG_MODULE_INSTANTIATE_FAILED, 0, JSEXN_INTERNALERR, "attempt to re-instantiate module after failure") MSG_DEF(JSMSG_BAD_MODULE_STATUS, 0, JSEXN_INTERNALERR, "module record has unexpected status") // Promise diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index cf5880e038..3e0c63811c 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -4739,18 +4739,6 @@ JS::GetModuleScript(JSContext* cx, JS::HandleObject moduleArg) return moduleArg->as().script(); } -JS_PUBLIC_API(bool) -JS::IsModuleErrored(JSObject* moduleArg) -{ - return moduleArg->as().status() == MODULE_STATUS_ERRORED; -} - -JS_PUBLIC_API(JS::Value) -JS::GetModuleError(JSObject* moduleArg) -{ - return moduleArg->as().error(); -} - JS_PUBLIC_API(JSObject*) JS_New(JSContext* cx, HandleObject ctor, const JS::HandleValueArray& inputArgs) { diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 9c3bf81518..76781cf065 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -4395,12 +4395,6 @@ GetRequestedModules(JSContext* cx, JS::HandleObject moduleRecord); extern JS_PUBLIC_API(JSScript*) GetModuleScript(JSContext* cx, JS::HandleObject moduleRecord); -extern JS_PUBLIC_API(bool) -IsModuleErrored(JSObject* moduleRecord); - -extern JS_PUBLIC_API(JS::Value) -GetModuleError(JSObject* moduleRecord); - } /* namespace JS */ extern JS_PUBLIC_API(bool) diff --git a/js/src/vm/EnvironmentObject.cpp b/js/src/vm/EnvironmentObject.cpp index 7834940f14..871ae3f191 100644 --- a/js/src/vm/EnvironmentObject.cpp +++ b/js/src/vm/EnvironmentObject.cpp @@ -491,7 +491,7 @@ ModuleEnvironmentObject::createImportBinding(JSContext* cx, HandleAtom importNam RootedId importNameId(cx, AtomToId(importName)); RootedId localNameId(cx, AtomToId(localName)); RootedModuleEnvironmentObject env(cx, &module->initialEnvironment()); - if (!importBindings().putNew(cx, importNameId, env, localNameId)) { + if (!importBindings().put(cx, importNameId, env, localNameId)) { ReportOutOfMemory(cx); return false; } diff --git a/js/src/vm/SelfHosting.cpp b/js/src/vm/SelfHosting.cpp index 0dfeffc366..f98e968b85 100644 --- a/js/src/vm/SelfHosting.cpp +++ b/js/src/vm/SelfHosting.cpp @@ -2087,7 +2087,6 @@ intrinsic_CreateNamespaceBinding(JSContext* cx, unsigned argc, Value* vp) // the slot directly. RootedShape shape(cx, environment->lookup(cx, name)); MOZ_ASSERT(shape); - MOZ_ASSERT(environment->getSlot(shape->slot()).isMagic(JS_UNINITIALIZED_LEXICAL)); environment->setSlot(shape->slot(), args[2]); args.rval().setUndefined(); return true; From 1cfaf06d8fd4cc8b83e180663d5ed59f8aa0ab17 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Wed, 8 Jul 2020 14:12:57 +0000 Subject: [PATCH 04/33] Issue #618 - Use a single slot for the module's environment object. According to the spec this isn't created until the module is instantiated, but we create it when we compile the module. We stored this previously in InitialEnvironmentSlot and copied it to EnvironmentSlot when it was supposed to be created, but we can just store it in the latter slot straight away and check the module's status and return null if it shouldn't exist yet. This reduces the number of slots needed on a moduleObject to 17. Re: BZ 1420412 Part 1 We can't implement the second part to further reduce our number of slots, because it relies on SetProxyReservedSlot which in turn relies on rearchitecturing JS proxies to make reserved slots dynamic. That's a rabbit hole we really don't want to fall into. So, we'll end up being a bit slower because it can't be in-line allocated with having more than 16 slots, but so be it. I sincerely doubt it will make any practical difference. --- js/src/builtin/Module.js | 7 +++-- js/src/builtin/ModuleObject.cpp | 41 +++++++++++++---------------- js/src/builtin/ModuleObject.h | 4 --- js/src/builtin/SelfHostingDefines.h | 10 +++---- js/src/builtin/TestingFunctions.cpp | 2 -- js/src/vm/SelfHosting.cpp | 12 --------- 6 files changed, 29 insertions(+), 47 deletions(-) diff --git a/js/src/builtin/Module.js b/js/src/builtin/Module.js index 86b44880dc..c9f20c18c8 100644 --- a/js/src/builtin/Module.js +++ b/js/src/builtin/Module.js @@ -234,8 +234,11 @@ function GetModuleEnvironment(module) { assert(IsModule(module), "Non-module passed to GetModuleEnvironment"); + assert(module.status >= MODULE_STATUS_INSTANTIATING, + "Attempt to access module environement before instantation"); + let env = UnsafeGetReservedSlot(module, MODULE_OBJECT_ENVIRONMENT_SLOT); - assert(env === undefined || IsModuleEnvironment(env), + assert(IsModuleEnvironment(env), "Module environment slot contains unexpected value"); return env; @@ -421,7 +424,7 @@ function ModuleDeclarationEnvironmentSetup(module) } // Steps 5-6 - CreateModuleEnvironment(module); + // Note that we have already created the environment by this point. let env = GetModuleEnvironment(module); // Step 8 diff --git a/js/src/builtin/ModuleObject.cpp b/js/src/builtin/ModuleObject.cpp index a9a2571784..14d27845c8 100644 --- a/js/src/builtin/ModuleObject.cpp +++ b/js/src/builtin/ModuleObject.cpp @@ -657,17 +657,24 @@ ModuleObject::finalize(js::FreeOp* fop, JSObject* obj) fop->delete_(funDecls); } +ModuleEnvironmentObject& +ModuleObject::initialEnvironment() const +{ + Value value = getReservedSlot(EnvironmentSlot); + return value.toObject().as(); +} + ModuleEnvironmentObject* ModuleObject::environment() const { -//- MOZ_ASSERT(status() != MODULE_STATUS_ERRORED); -//+ MOZ_ASSERT(!hadEvaluationError()); + MOZ_ASSERT(!hadEvaluationError()); - Value value = getReservedSlot(EnvironmentSlot); - if (value.isUndefined()) + // According to the spec the environment record is created during + // instantiation, but we create it earlier than that. + if (status() < MODULE_STATUS_INSTANTIATED) return nullptr; - return &value.toObject().as(); + return &initialEnvironment(); } bool @@ -732,7 +739,7 @@ ModuleObject::init(HandleScript script) void ModuleObject::setInitialEnvironment(HandleModuleEnvironmentObject initialEnvironment) { - initReservedSlot(InitialEnvironmentSlot, ObjectValue(*initialEnvironment)); + initReservedSlot(EnvironmentSlot, ObjectValue(*initialEnvironment)); } void @@ -867,12 +874,6 @@ ModuleObject::setHostDefinedField(const JS::Value& value) setReservedSlot(HostDefinedSlot, value); } -ModuleEnvironmentObject& -ModuleObject::initialEnvironment() const -{ - return getReservedSlot(InitialEnvironmentSlot).toObject().as(); -} - Scope* ModuleObject::enclosingScope() const { @@ -898,16 +899,6 @@ ModuleObject::trace(JSTracer* trc, JSObject* obj) funDecls->trace(trc); } -void -ModuleObject::createEnvironment() -{ - // The environment has already been created, we just neet to set it in the - // right slot. - MOZ_ASSERT(!getReservedSlot(InitialEnvironmentSlot).isUndefined()); - MOZ_ASSERT(getReservedSlot(EnvironmentSlot).isUndefined()); - setReservedSlot(EnvironmentSlot, getReservedSlot(InitialEnvironmentSlot)); -} - bool ModuleObject::noteFunctionDeclaration(ExclusiveContext* cx, HandleAtom name, HandleFunction fun) { @@ -923,7 +914,10 @@ ModuleObject::noteFunctionDeclaration(ExclusiveContext* cx, HandleAtom name, Han /* static */ bool ModuleObject::instantiateFunctionDeclarations(JSContext* cx, HandleModuleObject self) { +#ifdef DEBUG + MOZ_ASSERT(self->status() == MODULE_STATUS_INSTANTIATING); MOZ_ASSERT(IsFrozen(cx, self)); +#endif FunctionDeclarationVector* funDecls = self->functionDeclarations(); if (!funDecls) { @@ -954,7 +948,10 @@ ModuleObject::instantiateFunctionDeclarations(JSContext* cx, HandleModuleObject /* static */ bool ModuleObject::execute(JSContext* cx, HandleModuleObject self, MutableHandleValue rval) { +#ifdef DEBUG + MOZ_ASSERT(self->status() == MODULE_STATUS_EVALUATING); MOZ_ASSERT(IsFrozen(cx, self)); +#endif RootedScript script(cx, self->script()); RootedModuleEnvironmentObject scope(cx, self->environment()); diff --git a/js/src/builtin/ModuleObject.h b/js/src/builtin/ModuleObject.h index 5a809c8652..bd3e7044ec 100644 --- a/js/src/builtin/ModuleObject.h +++ b/js/src/builtin/ModuleObject.h @@ -214,7 +214,6 @@ class ModuleObject : public NativeObject enum { ScriptSlot = 0, - InitialEnvironmentSlot, EnvironmentSlot, NamespaceSlot, StatusSlot, @@ -286,9 +285,6 @@ class ModuleObject : public NativeObject void setHostDefinedField(const JS::Value& value); - // For intrinsic_CreateModuleEnvironment. - void createEnvironment(); - // For BytecodeEmitter. bool noteFunctionDeclaration(ExclusiveContext* cx, HandleAtom name, HandleFunction fun); diff --git a/js/src/builtin/SelfHostingDefines.h b/js/src/builtin/SelfHostingDefines.h index 6afa641b12..a3ba36804d 100644 --- a/js/src/builtin/SelfHostingDefines.h +++ b/js/src/builtin/SelfHostingDefines.h @@ -97,11 +97,11 @@ #define REGEXP_STRING_ITERATOR_FLAGS_SLOT 2 #define REGEXP_STRING_ITERATOR_DONE_SLOT 3 -#define MODULE_OBJECT_ENVIRONMENT_SLOT 2 -#define MODULE_OBJECT_STATUS_SLOT 4 -#define MODULE_OBJECT_EVALUATION_ERROR_SLOT 5 // 4 -#define MODULE_OBJECT_DFS_INDEX_SLOT 16 -#define MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT 17 +#define MODULE_OBJECT_ENVIRONMENT_SLOT 1 +#define MODULE_OBJECT_STATUS_SLOT 3 +#define MODULE_OBJECT_EVALUATION_ERROR_SLOT 4 +#define MODULE_OBJECT_DFS_INDEX_SLOT 15 +#define MODULE_OBJECT_DFS_ANCESTOR_INDEX_SLOT 16 #define MODULE_STATUS_UNINSTANTIATED 0 #define MODULE_STATUS_INSTANTIATING 1 diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp index e93ebeec3d..f589b8076b 100644 --- a/js/src/builtin/TestingFunctions.cpp +++ b/js/src/builtin/TestingFunctions.cpp @@ -3566,8 +3566,6 @@ GetModuleEnvironment(JSContext* cx, HandleValue moduleValue) // before they have been instantiated. RootedModuleEnvironmentObject env(cx, &module->initialEnvironment()); MOZ_ASSERT(env); - MOZ_ASSERT_IF(module->environment(), module->environment() == env); - return env; } diff --git a/js/src/vm/SelfHosting.cpp b/js/src/vm/SelfHosting.cpp index f98e968b85..50b0c01def 100644 --- a/js/src/vm/SelfHosting.cpp +++ b/js/src/vm/SelfHosting.cpp @@ -2048,17 +2048,6 @@ intrinsic_HostResolveImportedModule(JSContext* cx, unsigned argc, Value* vp) return true; } -static bool -intrinsic_CreateModuleEnvironment(JSContext* cx, unsigned argc, Value* vp) -{ - CallArgs args = CallArgsFromVp(argc, vp); - MOZ_ASSERT(args.length() == 1); - RootedModuleObject module(cx, &args[0].toObject().as()); - module->createEnvironment(); - args.rval().setUndefined(); - return true; -} - static bool intrinsic_CreateImportBinding(JSContext* cx, unsigned argc, Value* vp) { @@ -2658,7 +2647,6 @@ static const JSFunctionSpec intrinsic_functions[] = { CallNonGenericSelfhostedMethod>, 2, 0), JS_FN("HostResolveImportedModule", intrinsic_HostResolveImportedModule, 2, 0), JS_FN("IsModuleEnvironment", intrinsic_IsInstanceOfBuiltin, 1, 0), - JS_FN("CreateModuleEnvironment", intrinsic_CreateModuleEnvironment, 1, 0), JS_FN("CreateImportBinding", intrinsic_CreateImportBinding, 4, 0), JS_FN("CreateNamespaceBinding", intrinsic_CreateNamespaceBinding, 3, 0), JS_FN("InstantiateModuleFunctionDeclarations", From 39050b430e627b62bebcd0e918bd332a8068b23e Mon Sep 17 00:00:00 2001 From: Moonchild Date: Wed, 8 Jul 2020 14:53:31 +0000 Subject: [PATCH 05/33] Issue #618 - Lazily initialise module binding maps Make it so they are not allocated on a background thread in a different zone to the final module. Ref: BZ 1372258 --- js/src/builtin/ModuleObject.cpp | 46 +++++++++++++++++++-------------- js/src/builtin/ModuleObject.h | 16 +++++++----- 2 files changed, 35 insertions(+), 27 deletions(-) diff --git a/js/src/builtin/ModuleObject.cpp b/js/src/builtin/ModuleObject.cpp index 14d27845c8..44e5a2c887 100644 --- a/js/src/builtin/ModuleObject.cpp +++ b/js/src/builtin/ModuleObject.cpp @@ -248,21 +248,13 @@ IndirectBindingMap::Binding::Binding(ModuleEnvironmentObject* environment, Shape : environment(environment), shape(shape) {} -IndirectBindingMap::IndirectBindingMap(Zone* zone) - : map_(ZoneAllocPolicy(zone)) -{ -} - -bool -IndirectBindingMap::init() -{ - return map_.init(); -} - void IndirectBindingMap::trace(JSTracer* trc) { - for (Map::Enum e(map_); !e.empty(); e.popFront()) { + if (!map_) + return; + + for (Map::Enum e(*map_); !e.empty(); e.popFront()) { Binding& b = e.front().value(); TraceEdge(trc, &b.environment, "module bindings environment"); TraceEdge(trc, &b.shape, "module bindings shape"); @@ -276,9 +268,22 @@ bool IndirectBindingMap::put(JSContext* cx, HandleId name, HandleModuleEnvironmentObject environment, HandleId localName) { + // This object might have been allocated on the background parsing thread in + // different zone to the final module. Lazily allocate the map so we don't + // have to switch its zone when merging compartments. + if (!map_) { + MOZ_ASSERT(!cx->zone()->group()->createdForHelperThread()); + map_.emplace(cx->zone()); + if (!map_->init()) { + map_.reset(); + ReportOutOfMemory(cx); + return false; + } + } + RootedShape shape(cx, environment->lookup(cx, localName)); MOZ_ASSERT(shape); - if (!map_.put(name, Binding(environment, shape))) { + if (!map_->put(name, Binding(environment, shape))) { ReportOutOfMemory(cx); return false; } @@ -289,7 +294,10 @@ IndirectBindingMap::put(JSContext* cx, HandleId name, bool IndirectBindingMap::lookup(jsid name, ModuleEnvironmentObject** envOut, Shape** shapeOut) const { - auto ptr = map_.lookup(name); + if (!map_) + return false; + + auto ptr = map_->lookup(name); if (!ptr) return false; @@ -625,10 +633,9 @@ ModuleObject::create(ExclusiveContext* cx) RootedModuleObject self(cx, &obj->as()); Zone* zone = cx->zone(); - IndirectBindingMap* bindings = zone->new_(zone); - if (!bindings || !bindings->init()) { + IndirectBindingMap* bindings = zone->new_(); + if (!bindings) { ReportOutOfMemory(cx); - js_delete(bindings); return nullptr; } @@ -974,10 +981,9 @@ ModuleObject::createNamespace(JSContext* cx, HandleModuleObject self, HandleObje return nullptr; Zone* zone = cx->zone(); - IndirectBindingMap* bindings = zone->new_(zone); - if (!bindings || !bindings->init()) { + IndirectBindingMap* bindings = zone->new_(); + if (!bindings) { ReportOutOfMemory(cx); - js_delete(bindings); return nullptr; } diff --git a/js/src/builtin/ModuleObject.h b/js/src/builtin/ModuleObject.h index bd3e7044ec..5e4e7f342f 100644 --- a/js/src/builtin/ModuleObject.h +++ b/js/src/builtin/ModuleObject.h @@ -7,6 +7,8 @@ #ifndef builtin_ModuleObject_h #define builtin_ModuleObject_h +#include "mozilla/Maybe.h" + #include "jsapi.h" #include "jsatom.h" @@ -102,27 +104,27 @@ typedef Handle HandleExportEntryObject; class IndirectBindingMap { public: - explicit IndirectBindingMap(Zone* zone); - bool init(); - void trace(JSTracer* trc); bool put(JSContext* cx, HandleId name, HandleModuleEnvironmentObject environment, HandleId localName); size_t count() const { - return map_.count(); + return map_ ? map_->count() : 0; } bool has(jsid name) const { - return map_.has(name); + return map_ ? map_->has(name) : false; } bool lookup(jsid name, ModuleEnvironmentObject** envOut, Shape** shapeOut) const; template void forEachExportedName(Func func) const { - for (auto r = map_.all(); !r.empty(); r.popFront()) + if (!map_) + return; + + for (auto r = map_->all(); !r.empty(); r.popFront()) func(r.front().key()); } @@ -136,7 +138,7 @@ class IndirectBindingMap typedef HashMap, ZoneAllocPolicy> Map; - Map map_; + mozilla::Maybe map_; }; class ModuleNamespaceObject : public ProxyObject From 40017437ab125f0fcc20cbb40f7ff2e43cc3d5d5 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Sun, 2 Aug 2020 07:20:25 +0000 Subject: [PATCH 06/33] Issue #618 - Record module dependency before starting fetch so that error handling works correctly Ref BZ 1395896 --- dom/script/ScriptLoader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 3dbbcaceae..4050caadaa 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -793,6 +793,7 @@ ScriptLoader::StartFetchingModuleAndDependencies(ModuleLoadRequest* aRequest, childRequest->mIsInline = false; childRequest->mReferrerPolicy = aRequest->mReferrerPolicy; childRequest->mParent = aRequest; + aRequest->mImports.AppendElement(childRequest); RefPtr ready = childRequest->mReady.Ensure(__func__); @@ -803,7 +804,6 @@ ScriptLoader::StartFetchingModuleAndDependencies(ModuleLoadRequest* aRequest, return ready; } - aRequest->mImports.AppendElement(childRequest); return ready; } From f600824850f69d1f7c28f7fea4df5230e67111f4 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 3 Aug 2020 14:04:26 -0400 Subject: [PATCH 07/33] Issue #618 - Lazily initialise module binding maps - Debug follow up The added debug assertion does not work due to missing API. They were added in BZ 1337491, 1395366, and others, but were primarily used for multi-threading. This uses our existing non-multithreaded syntax instead, resolving a `no member named` build error. --- js/src/builtin/ModuleObject.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/builtin/ModuleObject.cpp b/js/src/builtin/ModuleObject.cpp index 44e5a2c887..728929e8c0 100644 --- a/js/src/builtin/ModuleObject.cpp +++ b/js/src/builtin/ModuleObject.cpp @@ -272,7 +272,7 @@ IndirectBindingMap::put(JSContext* cx, HandleId name, // different zone to the final module. Lazily allocate the map so we don't // have to switch its zone when merging compartments. if (!map_) { - MOZ_ASSERT(!cx->zone()->group()->createdForHelperThread()); + MOZ_ASSERT(!cx->zone()->usedByExclusiveThread); map_.emplace(cx->zone()); if (!map_->init()) { map_.reset(); From f3d348ebd7f366cad541db7ead922a8ea38c50da Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 3 Aug 2020 14:31:43 -0400 Subject: [PATCH 08/33] Issue #618 - Align error handling for module scripts with the spec - Debug follow up MODULE_STATUS_ERRORED is no more. Replacing with newer API. Ref: BZ 1420420 --- js/src/builtin/Module.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/js/src/builtin/Module.js b/js/src/builtin/Module.js index c9f20c18c8..355303d44d 100644 --- a/js/src/builtin/Module.js +++ b/js/src/builtin/Module.js @@ -67,7 +67,8 @@ function ModuleGetExportedNames(exportStarSet = []) function ModuleSetStatus(module, newStatus) { - assert(newStatus >= MODULE_STATUS_ERRORED && newStatus <= MODULE_STATUS_EVALUATED, + assert(newStatus >= MODULE_STATUS_UNINSTANTIATED && + newStatus <= MODULE_STATUS_EVALUATED_ERROR, "Bad new module status in ModuleSetStatus"); // Note that under OOM conditions we can fail the module instantiation From f0d6e293a3bcbd3fb4fd535e8635268e8d8ae808 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Sun, 2 Aug 2020 12:46:52 -0400 Subject: [PATCH 09/33] Issue #618 - Simplify module map interface Ref: BZ 1365187 --- dom/script/ScriptLoader.cpp | 24 ++++++++++++------------ dom/script/ScriptLoader.h | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 4050caadaa..7d5a19faf5 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -388,12 +388,12 @@ ScriptLoader::ModuleScriptsEnabled() } bool -ScriptLoader::ModuleMapContainsModule(ModuleLoadRequest *aRequest) const +ScriptLoader::ModuleMapContainsURL(nsIURI* aURL) const { // Returns whether we have fetched, or are currently fetching, a module script - // for the request's URL. - return mFetchingModules.Contains(aRequest->mURI) || - mFetchedModules.Contains(aRequest->mURI); + // for a URL. + return mFetchingModules.Contains(aURL) || + mFetchedModules.Contains(aURL); } bool @@ -410,7 +410,7 @@ ScriptLoader::SetModuleFetchStarted(ModuleLoadRequest *aRequest) // Update the module map to indicate that a module is currently being fetched. MOZ_ASSERT(aRequest->IsLoading()); - MOZ_ASSERT(!ModuleMapContainsModule(aRequest)); + MOZ_ASSERT(!ModuleMapContainsURL(aRequest->mURI)); mFetchingModules.Put(aRequest->mURI, nullptr); } @@ -443,21 +443,21 @@ ScriptLoader::SetModuleFetchFinishedAndResumeWaitingRequests(ModuleLoadRequest * } RefPtr -ScriptLoader::WaitForModuleFetch(ModuleLoadRequest *aRequest) +ScriptLoader::WaitForModuleFetch(nsIURI* aURL) { - MOZ_ASSERT(ModuleMapContainsModule(aRequest)); + MOZ_ASSERT(ModuleMapContainsURL(aURL)); RefPtr promise; - if (mFetchingModules.Get(aRequest->mURI, getter_AddRefs(promise))) { + if (mFetchingModules.Get(aURL, getter_AddRefs(promise))) { if (!promise) { promise = new GenericPromise::Private(__func__); - mFetchingModules.Put(aRequest->mURI, promise); + mFetchingModules.Put(aURL, promise); } return promise; } RefPtr ms; - MOZ_ALWAYS_TRUE(mFetchedModules.Get(aRequest->mURI, getter_AddRefs(ms))); + MOZ_ALWAYS_TRUE(mFetchedModules.Get(aURL, getter_AddRefs(ms))); if (!ms) { return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__); } @@ -984,8 +984,8 @@ ScriptLoader::StartLoad(ScriptLoadRequest *aRequest, const nsAString &aType, // Check whether the module has been fetched or is currently being fetched, // and if so wait for it. ModuleLoadRequest* request = aRequest->AsModuleRequest(); - if (ModuleMapContainsModule(request)) { - WaitForModuleFetch(request) + if (ModuleMapContainsURL(request->mURI)) { + WaitForModuleFetch(request->mURI) ->Then(AbstractThread::GetCurrent(), __func__, request, &ModuleLoadRequest::ModuleLoaded, &ModuleLoadRequest::LoadFailed); diff --git a/dom/script/ScriptLoader.h b/dom/script/ScriptLoader.h index db2eeed31a..955ac2cb71 100644 --- a/dom/script/ScriptLoader.h +++ b/dom/script/ScriptLoader.h @@ -572,8 +572,8 @@ private: bool IsFetchingModule(ModuleLoadRequest *aRequest) const; - bool ModuleMapContainsModule(ModuleLoadRequest *aRequest) const; - RefPtr WaitForModuleFetch(ModuleLoadRequest *aRequest); + bool ModuleMapContainsURL(nsIURI* aURL) const; + RefPtr WaitForModuleFetch(nsIURI* aURL); ModuleScript* GetFetchedModule(nsIURI* aURL) const; friend bool From 6639e9cdacbac102bb89b0ee86f0c792de804ce7 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 3 Aug 2020 10:44:39 -0400 Subject: [PATCH 10/33] Issue #618 - Keep track of which modules in a graph have been fetched using a visited set Ref: BZ 1365187 --- dom/script/ModuleLoadRequest.cpp | 33 +++++++++--- dom/script/ModuleLoadRequest.h | 31 ++++++++--- dom/script/ScriptLoader.cpp | 93 +++++++++++++------------------- dom/script/ScriptLoader.h | 7 ++- 4 files changed, 92 insertions(+), 72 deletions(-) diff --git a/dom/script/ModuleLoadRequest.cpp b/dom/script/ModuleLoadRequest.cpp index a12fcb1629..a75a922e24 100644 --- a/dom/script/ModuleLoadRequest.cpp +++ b/dom/script/ModuleLoadRequest.cpp @@ -17,26 +17,48 @@ NS_INTERFACE_MAP_END_INHERITING(ScriptLoadRequest) NS_IMPL_CYCLE_COLLECTION_INHERITED(ModuleLoadRequest, ScriptLoadRequest, mBaseURL, mLoader, - mParent, mModuleScript, mImports) NS_IMPL_ADDREF_INHERITED(ModuleLoadRequest, ScriptLoadRequest) NS_IMPL_RELEASE_INHERITED(ModuleLoadRequest, ScriptLoadRequest) -ModuleLoadRequest::ModuleLoadRequest(nsIScriptElement* aElement, +ModuleLoadRequest::ModuleLoadRequest(nsIURI* aURI, + nsIScriptElement* aElement, uint32_t aVersion, CORSMode aCORSMode, const SRIMetadata &aIntegrity, ScriptLoader* aLoader) : ScriptLoadRequest(ScriptKind::Module, + aURI, aElement, aVersion, aCORSMode, aIntegrity), mIsTopLevel(true), - mLoader(aLoader) -{} + mLoader(aLoader), + mVisitedSet(new VisitedURLSet()) +{ + mVisitedSet->PutEntry(aURI); +} + +ModuleLoadRequest::ModuleLoadRequest(nsIURI* aURI, + ModuleLoadRequest* aParent) + : ScriptLoadRequest(ScriptKind::Module, + aURI, + aParent->mElement, + aParent->mJSVersion, + aParent->mCORSMode, + aParent->mIntegrity), + mIsTopLevel(false), + mLoader(aParent->mLoader), + mVisitedSet(aParent->mVisitedSet) +{ + MOZ_ASSERT(mVisitedSet->Contains(aURI)); + + mIsInline = false; + mReferrerPolicy = aParent->mReferrerPolicy; +} void ModuleLoadRequest::Cancel() { @@ -132,8 +154,7 @@ ModuleLoadRequest::LoadFinished() { mLoader->ProcessLoadedModuleTree(this); mLoader = nullptr; - mParent = nullptr; } } // dom namespace -} // mozilla namespace \ No newline at end of file +} // mozilla namespace diff --git a/dom/script/ModuleLoadRequest.h b/dom/script/ModuleLoadRequest.h index 7b06dd2cf3..2e96528816 100644 --- a/dom/script/ModuleLoadRequest.h +++ b/dom/script/ModuleLoadRequest.h @@ -8,6 +8,7 @@ #define mozilla_dom_ModuleLoadRequest_h #include "mozilla/dom/ScriptLoader.h" +#include "nsURIHashKey.h" #include "mozilla/MozPromise.h" namespace mozilla { @@ -16,6 +17,16 @@ namespace dom { class ModuleScript; class ScriptLoader; +// A reference counted set of URLs we have visited in the process of loading a +// module graph. +class VisitedURLSet : public nsTHashtable +{ + NS_INLINE_DECL_REFCOUNTING(VisitedURLSet) + +private: + ~VisitedURLSet() = default; +}; + // A load request for a module, created for every top level module script and // every module import. Load request can share a ModuleScript if there are // multiple imports of the same module. @@ -31,12 +42,18 @@ public: NS_DECL_ISUPPORTS_INHERITED NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ModuleLoadRequest, ScriptLoadRequest) - ModuleLoadRequest(nsIScriptElement* aElement, + // Create a top-level module load request. + ModuleLoadRequest(nsIURI* aURI, + nsIScriptElement* aElement, uint32_t aVersion, CORSMode aCORSMode, const SRIMetadata& aIntegrity, ScriptLoader* aLoader); + // Create a module load request for an imported module. + ModuleLoadRequest(nsIURI* aURI, + ModuleLoadRequest* aParent); + bool IsTopLevel() const { return mIsTopLevel; } @@ -55,7 +72,7 @@ private: public: // Is this a request for a top level module script or an import? - bool mIsTopLevel; + const bool mIsTopLevel; // The base URL used for resolving relative module imports. nsCOMPtr mBaseURL; @@ -64,10 +81,6 @@ public: // finishes. RefPtr mLoader; - // The importing module, or nullptr for top level module scripts. Used to - // implement the ancestor list checked when fetching module dependencies. - RefPtr mParent; - // Set to a module script object after a successful load or nullptr on // failure. RefPtr mModuleScript; @@ -79,9 +92,13 @@ public: // Array of imported modules. nsTArray> mImports; + + // Set of module URLs visited while fetching the module graph this request is + // part of. + RefPtr mVisitedSet; }; } // dom namespace } // mozilla namespace -#endif // mozilla_dom_ModuleLoadRequest_h \ No newline at end of file +#endif // mozilla_dom_ModuleLoadRequest_h diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 7d5a19faf5..7a3cd8fe3c 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -501,7 +501,7 @@ ScriptLoader::ProcessFetchedModuleSource(ModuleLoadRequest* aRequest) } static nsresult -ResolveRequestedModules(ModuleLoadRequest* aRequest, nsCOMArray& aUrls); +ResolveRequestedModules(ModuleLoadRequest* aRequest, nsCOMArray* aUrlsOut); nsresult ScriptLoader::CreateModuleScript(ModuleLoadRequest* aRequest) @@ -577,8 +577,7 @@ ScriptLoader::CreateModuleScript(ModuleLoadRequest* aRequest) // Validate requested modules and treat failure to resolve module specifiers // the same as a parse error. - nsCOMArray urls; - rv = ResolveRequestedModules(aRequest, urls); + rv = ResolveRequestedModules(aRequest, nullptr); if (NS_FAILED(rv)) { aRequest->ModuleErrored(); return NS_OK; @@ -665,33 +664,7 @@ ResolveModuleSpecifier(ModuleScript* aScript, } static nsresult -RequestedModuleIsInAncestorList(ModuleLoadRequest* aRequest, nsIURI* aURL, bool* aResult) -{ - const size_t ImportDepthLimit = 100; - - *aResult = false; - size_t depth = 0; - while (aRequest) { - if (depth++ == ImportDepthLimit) { - return NS_ERROR_FAILURE; - } - - bool equal; - nsresult rv = aURL->Equals(aRequest->mURI, &equal); - NS_ENSURE_SUCCESS(rv, rv); - if (equal) { - *aResult = true; - return NS_OK; - } - - aRequest = aRequest->mParent; - } - - return NS_OK; -} - -static nsresult -ResolveRequestedModules(ModuleLoadRequest* aRequest, nsCOMArray &aUrls) +ResolveRequestedModules(ModuleLoadRequest* aRequest, nsCOMArray* aUrlsOut) { ModuleScript* ms = aRequest->mModuleScript; @@ -728,11 +701,8 @@ ResolveRequestedModules(ModuleLoadRequest* aRequest, nsCOMArray &aUrls) return NS_ERROR_FAILURE; } - bool isAncestor; - nsresult rv = RequestedModuleIsInAncestorList(aRequest, uri, &isAncestor); - NS_ENSURE_SUCCESS(rv, rv); - if (!isAncestor) { - aUrls.AppendElement(uri.forget()); + if (aUrlsOut) { + aUrlsOut->AppendElement(uri.forget()); } } @@ -746,17 +716,33 @@ ScriptLoader::StartFetchingModuleDependencies(ModuleLoadRequest* aRequest) MOZ_ASSERT(!aRequest->mModuleScript->HasParseError()); MOZ_ASSERT(!aRequest->IsReadyToRun()); + auto visitedSet = aRequest->mVisitedSet; + MOZ_ASSERT(visitedSet->Contains(aRequest->mURI)); + aRequest->mProgress = ModuleLoadRequest::Progress::FetchingImports; nsCOMArray urls; - nsresult rv = ResolveRequestedModules(aRequest, urls); + nsresult rv = ResolveRequestedModules(aRequest, &urls); if (NS_FAILED(rv)) { aRequest->ModuleErrored(); return; } - if (urls.Length() == 0) { - // There are no descendents to load so this request is ready. + // Remove already visited URLs from the list. Put unvisited URLs into the + // visited set. + int32_t i = 0; + while (i < urls.Count()) { + nsIURI* url = urls[i]; + if (visitedSet->Contains(url)) { + urls.RemoveObjectAt(i); + } else { + visitedSet->PutEntry(url); + i++; + } + } + + if (urls.Count() == 0) { + // There are no descendants to load so this request is ready. aRequest->DependenciesLoaded(); return; } @@ -779,21 +765,14 @@ ScriptLoader::StartFetchingModuleDependencies(ModuleLoadRequest* aRequest) } RefPtr -ScriptLoader::StartFetchingModuleAndDependencies(ModuleLoadRequest* aRequest, +ScriptLoader::StartFetchingModuleAndDependencies(ModuleLoadRequest* aParent, nsIURI* aURI) { MOZ_ASSERT(aURI); - RefPtr childRequest = - new ModuleLoadRequest(aRequest->mElement, aRequest->mJSVersion, - aRequest->mCORSMode, aRequest->mIntegrity, this); + RefPtr childRequest = new ModuleLoadRequest(aURI, aParent); - childRequest->mIsTopLevel = false; - childRequest->mURI = aURI; - childRequest->mIsInline = false; - childRequest->mReferrerPolicy = aRequest->mReferrerPolicy; - childRequest->mParent = aRequest; - aRequest->mImports.AppendElement(childRequest); + aParent->mImports.AppendElement(childRequest); RefPtr ready = childRequest->mReady.Ensure(__func__); @@ -1201,17 +1180,19 @@ CSPAllowsInlineScript(nsIScriptElement *aElement, nsIDocument *aDocument) ScriptLoadRequest* ScriptLoader::CreateLoadRequest(ScriptKind aKind, + nsIURI* aURI, nsIScriptElement* aElement, uint32_t aVersion, CORSMode aCORSMode, const SRIMetadata &aIntegrity) { if (aKind == ScriptKind::Classic) { - return new ScriptLoadRequest(aKind, aElement, aVersion, aCORSMode, + return new ScriptLoadRequest(aKind, aURI, aElement, + aVersion,aCORSMode, aIntegrity); } MOZ_ASSERT(aKind == ScriptKind::Module); - return new ModuleLoadRequest(aElement, aVersion, aCORSMode, aIntegrity, this); + return new ModuleLoadRequest(aURI, aElement, aVersion, aCORSMode, aIntegrity, this); } bool @@ -1343,9 +1324,8 @@ ScriptLoader::ProcessScriptElement(nsIScriptElement *aElement) } } - request = CreateLoadRequest(scriptKind, aElement, version, ourCORSMode, - sriMetadata); - request->mURI = scriptURI; + request = CreateLoadRequest(scriptKind, scriptURI, aElement, + version, ourCORSMode, sriMetadata); request->mIsInline = false; request->mReferrerPolicy = ourRefPolicy; @@ -1466,11 +1446,11 @@ ScriptLoader::ProcessScriptElement(nsIScriptElement *aElement) } // Inline scripts ignore ther CORS mode and are always CORS_NONE - request = CreateLoadRequest(scriptKind, aElement, version, CORS_NONE, + request = CreateLoadRequest(scriptKind, mDocument->GetDocumentURI(), aElement, + version, CORS_NONE, SRIMetadata()); // SRI doesn't apply request->mJSVersion = version; request->mIsInline = true; - request->mURI = mDocument->GetDocumentURI(); request->mLineNo = aElement->GetScriptLineNumber(); if (request->IsModuleRequest()) { @@ -2603,9 +2583,8 @@ ScriptLoader::PreloadURI(nsIURI *aURI, const nsAString &aCharset, } RefPtr request = - CreateLoadRequest(ScriptKind::Classic, nullptr, 0, + CreateLoadRequest(ScriptKind::Classic, aURI, nullptr, 0, Element::StringToCORSMode(aCrossOrigin), sriMetadata); - request->mURI = aURI; request->mIsInline = false; request->mReferrerPolicy = aReferrerPolicy; diff --git a/dom/script/ScriptLoader.h b/dom/script/ScriptLoader.h index 955ac2cb71..3cbecbf030 100644 --- a/dom/script/ScriptLoader.h +++ b/dom/script/ScriptLoader.h @@ -62,6 +62,7 @@ protected: public: ScriptLoadRequest(ScriptKind aKind, + nsIURI* aURI, nsIScriptElement* aElement, uint32_t aVersion, mozilla::CORSMode aCORSMode, @@ -81,6 +82,7 @@ public: mScriptTextBuf(nullptr), mScriptTextLength(0), mJSVersion(aVersion), + mURI(aURI), mLineNo(1), mCORSMode(aCORSMode), mIntegrity(aIntegrity), @@ -165,7 +167,7 @@ public: char16_t* mScriptTextBuf; // Holds script text for non-inline scripts. Don't size_t mScriptTextLength; // use nsString so we can give ownership to jsapi. uint32_t mJSVersion; - nsCOMPtr mURI; + const nsCOMPtr mURI; nsCOMPtr mOriginPrincipal; nsAutoCString mURL; // Keep the URI's filename alive during off thread parsing. int32_t mLineNo; @@ -470,6 +472,7 @@ private: ScriptLoadRequest* CreateLoadRequest( ScriptKind aKind, + nsIURI* aURI, nsIScriptElement* aElement, uint32_t aVersion, mozilla::CORSMode aCORSMode, @@ -588,7 +591,7 @@ private: void StartFetchingModuleDependencies(ModuleLoadRequest* aRequest); RefPtr - StartFetchingModuleAndDependencies(ModuleLoadRequest* aRequest, nsIURI* aURI); + StartFetchingModuleAndDependencies(ModuleLoadRequest* aParent, nsIURI* aURI); nsIDocument* mDocument; // [WEAK] nsCOMArray mObservers; From 976239c8d0fac55d5593fa4312a72a5ab527990d Mon Sep 17 00:00:00 2001 From: Moonchild Date: Thu, 6 Aug 2020 18:27:32 +0000 Subject: [PATCH 11/33] [js] Add some utility functions to get the current JS runtime. --- js/src/jsapi.cpp | 5 +++++ js/src/jsapi.h | 3 +++ 2 files changed, 8 insertions(+) diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 3e0c63811c..77124355cf 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -483,6 +483,11 @@ JS_DestroyContext(JSContext* cx) DestroyContext(cx); } +JS_PUBLIC_API(JSRuntime*) +JS_GetRuntime(JSContext* cx) { + return cx->runtime(); +} + static JS_CurrentEmbedderTimeFunction currentEmbedderTimeFunction; JS_PUBLIC_API(void) diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 76781cf065..c6299e3f57 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -995,6 +995,9 @@ JS_NewContext(uint32_t maxbytes, extern JS_PUBLIC_API(void) JS_DestroyContext(JSContext* cx); +extern JS_PUBLIC_API(JSRuntime*) +JS_GetRuntime(JSContext* cx); + typedef double (*JS_CurrentEmbedderTimeFunction)(); /** From 34db761758dc4fa5cf453af802b2a89a59aec5d1 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Thu, 6 Aug 2020 18:31:36 +0000 Subject: [PATCH 12/33] Issue #618 - Simplify module resolve hook to be a function pointer This is an ahead-of time port to try and address #1624. This is based on BZ 1461751 and Jon Coppeard's work in it. --- dom/script/ScriptLoader.cpp | 45 +++++++++++++------------------------ dom/script/ScriptLoader.h | 5 +++-- js/public/Class.h | 2 +- js/src/jsapi.cpp | 15 +++++-------- js/src/jsapi.h | 16 +++++++------ js/src/shell/js.cpp | 33 +++++++++++++++++++++++---- js/src/vm/GlobalObject.h | 14 ------------ js/src/vm/Runtime.cpp | 6 +++-- js/src/vm/Runtime.h | 3 +++ js/src/vm/SelfHosting.cpp | 15 +++++++------ 10 files changed, 78 insertions(+), 76 deletions(-) diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 7a3cd8fe3c..9de9a1d037 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -787,24 +787,20 @@ ScriptLoader::StartFetchingModuleAndDependencies(ModuleLoadRequest* aParent, } // 8.1.3.8.1 HostResolveImportedModule(referencingModule, specifier) -bool -HostResolveImportedModule(JSContext* aCx, unsigned argc, JS::Value* vp) +JSObject* +HostResolveImportedModule(JSContext* aCx, JS::Handle aModule, + JS::Handle aSpecifier) { - MOZ_ASSERT(argc == 2); - JS::CallArgs args = JS::CallArgsFromVp(argc, vp); - JS::Rooted module(aCx, &args[0].toObject()); - JS::Rooted specifier(aCx, args[1].toString()); - // Let referencing module script be referencingModule.[[HostDefined]]. - JS::Value value = JS::GetModuleHostDefinedField(module); + JS::Value value = JS::GetModuleHostDefinedField(aModule); auto script = static_cast(value.toPrivate()); - MOZ_ASSERT(script->ModuleRecord() == module); + MOZ_ASSERT(script->ModuleRecord() == aModule); // Let url be the result of resolving a module specifier given referencing // module script and specifier. nsAutoJSString string; - if (!string.init(aCx, specifier)) { - return false; + if (!string.init(aCx, aSpecifier)) { + return nullptr; } nsCOMPtr uri = ResolveModuleSpecifier(script, string); @@ -820,27 +816,20 @@ HostResolveImportedModule(JSContext* aCx, unsigned argc, JS::Value* vp) MOZ_ASSERT(ms, "Resolved module not found in module map"); MOZ_ASSERT(!ms->HasParseError()); + MOZ_ASSERT(ms->ModuleRecord()); - *vp = JS::ObjectValue(*ms->ModuleRecord()); - return true; + return ms->ModuleRecord(); } -static nsresult +static void EnsureModuleResolveHook(JSContext* aCx) { - if (JS::GetModuleResolveHook(aCx)) { - return NS_OK; + JSRuntime* rt = JS_GetRuntime(aCx); + if (JS::GetModuleResolveHook(rt)) { + return; } - JS::Rooted func(aCx); - func = JS_NewFunction(aCx, HostResolveImportedModule, 2, 0, - "HostResolveImportedModule"); - if (!func) { - return NS_ERROR_FAILURE; - } - - JS::SetModuleResolveHook(aCx, func); - return NS_OK; + JS::SetModuleResolveHook(rt, HostResolveImportedModule); } void @@ -928,8 +917,7 @@ ScriptLoader::InstantiateModuleTree(ModuleLoadRequest* aRequest) return false; } - nsresult rv = EnsureModuleResolveHook(jsapi.cx()); - NS_ENSURE_SUCCESS(rv, false); + EnsureModuleResolveHook(jsapi.cx()); JS::Rooted module(jsapi.cx(), moduleScript->ModuleRecord()); bool ok = NS_SUCCEEDED(nsJSUtils::ModuleInstantiate(jsapi.cx(), module)); @@ -1963,8 +1951,7 @@ ScriptLoader::EvaluateScript(ScriptLoadRequest* aRequest) } if (aRequest->IsModuleRequest()) { - rv = EnsureModuleResolveHook(cx); - NS_ENSURE_SUCCESS(rv, rv); + EnsureModuleResolveHook(cx); ModuleLoadRequest* request = aRequest->AsModuleRequest(); MOZ_ASSERT(request->mModuleScript); diff --git a/dom/script/ScriptLoader.h b/dom/script/ScriptLoader.h index 3cbecbf030..4155f08f8c 100644 --- a/dom/script/ScriptLoader.h +++ b/dom/script/ScriptLoader.h @@ -579,8 +579,9 @@ private: RefPtr WaitForModuleFetch(nsIURI* aURL); ModuleScript* GetFetchedModule(nsIURI* aURL) const; - friend bool - HostResolveImportedModule(JSContext* aCx, unsigned argc, JS::Value* vp); + friend JSObject* + HostResolveImportedModule(JSContext* aCx, JS::Handle aModule, + JS::Handle aSpecifier); nsresult CreateModuleScript(ModuleLoadRequest* aRequest); nsresult ProcessFetchedModuleSource(ModuleLoadRequest* aRequest); diff --git a/js/public/Class.h b/js/public/Class.h index 3d73bce441..f4fa9ccafb 100644 --- a/js/public/Class.h +++ b/js/public/Class.h @@ -779,7 +779,7 @@ struct JSClass { // application. #define JSCLASS_GLOBAL_APPLICATION_SLOTS 5 #define JSCLASS_GLOBAL_SLOT_COUNT \ - (JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 2 + 46) + (JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 2 + 45) #define JSCLASS_GLOBAL_FLAGS_WITH_SLOTS(n) \ (JSCLASS_IS_GLOBAL | JSCLASS_HAS_RESERVED_SLOTS(JSCLASS_GLOBAL_SLOT_COUNT + (n))) #define JSCLASS_GLOBAL_FLAGS \ diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 77124355cf..69a3ba2ac2 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -4667,21 +4667,16 @@ JS::Evaluate(JSContext* cx, const ReadOnlyCompileOptions& optionsArg, return ::Evaluate(cx, optionsArg, filename, rval); } -JS_PUBLIC_API(JSFunction*) -JS::GetModuleResolveHook(JSContext* cx) +JS_PUBLIC_API(JS::ModuleResolveHook) +JS::GetModuleResolveHook(JSRuntime* rt) { - AssertHeapIsIdle(cx); - CHECK_REQUEST(cx); - return cx->global()->moduleResolveHook(); + return rt->moduleResolveHook; } JS_PUBLIC_API(void) -JS::SetModuleResolveHook(JSContext* cx, HandleFunction func) +JS::SetModuleResolveHook(JSRuntime* rt, JS::ModuleResolveHook func) { - AssertHeapIsIdle(cx); - CHECK_REQUEST(cx); - assertSameCompartment(cx, func); - cx->global()->setModuleResolveHook(func); + rt->moduleResolveHook = func; } JS_PUBLIC_API(bool) diff --git a/js/src/jsapi.h b/js/src/jsapi.h index c6299e3f57..5cdfd958e7 100644 --- a/js/src/jsapi.h +++ b/js/src/jsapi.h @@ -4325,17 +4325,19 @@ extern JS_PUBLIC_API(bool) Evaluate(JSContext* cx, const ReadOnlyCompileOptions& options, const char* filename, JS::MutableHandleValue rval); -/** - * Get the HostResolveImportedModule hook for a global. - */ -extern JS_PUBLIC_API(JSFunction*) -GetModuleResolveHook(JSContext* cx); +using ModuleResolveHook = JSObject* (*)(JSContext*, HandleObject, HandleString); /** - * Set the HostResolveImportedModule hook for a global to the given function. + * Get the HostResolveImportedModule hook for the runtime. + */ +extern JS_PUBLIC_API(ModuleResolveHook) +GetModuleResolveHook(JSRuntime* rt); + +/** + * Set the HostResolveImportedModule hook for the runtime to the given function. */ extern JS_PUBLIC_API(void) -SetModuleResolveHook(JSContext* cx, JS::HandleFunction func); +SetModuleResolveHook(JSRuntime* rt, ModuleResolveHook func); /** * Parse the given source buffer as a module in the scope of the current global diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp index 8cd821b311..bbf35459ff 100644 --- a/js/src/shell/js.cpp +++ b/js/src/shell/js.cpp @@ -274,6 +274,7 @@ struct ShellContext JS::PersistentRooted jobQueue; ExclusiveData asyncTasks; bool drainingJobQueue; + JS::PersistentRootedFunction moduleResolveHook; /* * Watchdog thread state. @@ -439,7 +440,8 @@ ShellContext::ShellContext(JSContext* cx) exitCode(0), quitting(false), readLineBufPos(0), - spsProfilingStackSize(0) + spsProfilingStackSize(0), + moduleResolveHook(cx) {} static ShellContext* @@ -4030,13 +4032,34 @@ SetModuleResolveHook(JSContext* cx, unsigned argc, Value* vp) return false; } - RootedFunction hook(cx, &args[0].toObject().as()); - Rooted global(cx, cx->global()); - global->setModuleResolveHook(hook); + ShellContext* sc = GetShellContext(cx); + sc->moduleResolveHook = &args[0].toObject().as(); + args.rval().setUndefined(); return true; } +static JSObject* +CallModuleResolveHook(JSContext* cx, HandleObject module, HandleString specifier) +{ + ShellContext* sc = GetShellContext(cx); + + JS::AutoValueArray<2> args(cx); + args[0].setObject(*module); + args[1].setString(specifier); + + RootedValue result(cx); + if (!JS_CallFunction(cx, nullptr, sc->moduleResolveHook, args, &result)) + return nullptr; + + if (!result.isObject() || !result.toObject().is()) { + JS_ReportErrorASCII(cx, "Module resolve hook did not return Module object"); + return nullptr; + } + + return &result.toObject(); +} + static bool GetModuleLoadPath(JSContext* cx, unsigned argc, Value* vp) { @@ -7962,6 +7985,8 @@ main(int argc, char** argv, char** envp) js::SetPreserveWrapperCallback(cx, DummyPreserveWrapperCallback); + JS::SetModuleResolveHook(cx->runtime(), CallModuleResolveHook); + result = Shell(cx, &op, envp); #ifdef DEBUG diff --git a/js/src/vm/GlobalObject.h b/js/src/vm/GlobalObject.h index 720f63e698..a9f07d4956 100644 --- a/js/src/vm/GlobalObject.h +++ b/js/src/vm/GlobalObject.h @@ -119,7 +119,6 @@ class GlobalObject : public NativeObject DEBUGGERS, INTRINSICS, FOR_OF_PIC_CHAIN, - MODULE_RESOLVE_HOOK, WINDOW_PROXY, GLOBAL_THIS_RESOLVED, @@ -879,19 +878,6 @@ class GlobalObject : public NativeObject setReservedSlot(WINDOW_PROXY, ObjectValue(*windowProxy)); } - void setModuleResolveHook(HandleFunction hook) { - MOZ_ASSERT(hook); - setSlot(MODULE_RESOLVE_HOOK, ObjectValue(*hook)); - } - - JSFunction* moduleResolveHook() { - Value value = getSlotRef(MODULE_RESOLVE_HOOK); - if (value.isUndefined()) - return nullptr; - - return &value.toObject().as(); - } - // Returns either this global's star-generator function prototype, or null // if that object was never created. Dodgy; for use only in also-dodgy // GlobalHelperThreadState::mergeParseTaskCompartment(). diff --git a/js/src/vm/Runtime.cpp b/js/src/vm/Runtime.cpp index 284a4f3d7c..5e0246b85a 100644 --- a/js/src/vm/Runtime.cpp +++ b/js/src/vm/Runtime.cpp @@ -241,8 +241,10 @@ JSRuntime::JSRuntime(JSRuntime* parentRuntime) lastAnimationTime(0), performanceMonitoring(thisFromCtor()), ionLazyLinkListSize_(0), - stackFormat_(parentRuntime ? js::StackFormat::Default - : js::StackFormat::SpiderMonkey) + stackFormat_(parentRuntime ? + js::StackFormat::Default : + js::StackFormat::SpiderMonkey), + moduleResolveHook() { setGCStoreBufferPtr(&gc.storeBuffer); diff --git a/js/src/vm/Runtime.h b/js/src/vm/Runtime.h index e60371e38c..247a2dc9d7 100644 --- a/js/src/vm/Runtime.h +++ b/js/src/vm/Runtime.h @@ -1294,6 +1294,9 @@ struct JSRuntime : public JS::shadow::Runtime, // For inherited heap state accessors. friend class js::gc::AutoTraceSession; friend class JS::AutoEnterCycleCollection; + + // The implementation-defined abstract operation HostResolveImportedModule. + JS::ModuleResolveHook moduleResolveHook; }; namespace js { diff --git a/js/src/vm/SelfHosting.cpp b/js/src/vm/SelfHosting.cpp index 50b0c01def..f324a0a67f 100644 --- a/js/src/vm/SelfHosting.cpp +++ b/js/src/vm/SelfHosting.cpp @@ -2026,25 +2026,26 @@ intrinsic_HostResolveImportedModule(JSContext* cx, unsigned argc, Value* vp) { CallArgs args = CallArgsFromVp(argc, vp); MOZ_ASSERT(args.length() == 2); - MOZ_ASSERT(args[0].toObject().is()); - MOZ_ASSERT(args[1].isString()); + RootedModuleObject module(cx, &args[0].toObject().as()); + RootedString specifier(cx, args[1].toString()); - RootedFunction moduleResolveHook(cx, cx->global()->moduleResolveHook()); + JS::ModuleResolveHook moduleResolveHook = cx->runtime()->moduleResolveHook; if (!moduleResolveHook) { JS_ReportErrorASCII(cx, "Module resolve hook not set"); return false; } - RootedValue result(cx); - if (!JS_CallFunction(cx, nullptr, moduleResolveHook, args, &result)) + RootedObject result(cx); + result = moduleResolveHook(cx, module, specifier); + if (!result) return false; - if (!result.isObject() || !result.toObject().is()) { + if (!result->is()) { JS_ReportErrorASCII(cx, "Module resolve hook did not return Module object"); return false; } - args.rval().set(result); + args.rval().setObject(*result); return true; } From 97617a3ab19ef0d27c5aaba1ba479a38ddcf2f82 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Thu, 13 Aug 2020 13:42:52 +0000 Subject: [PATCH 13/33] Issue #618: Ignore 'event' and 'for' attributes for module scripts. Because the spec says so. --- dom/script/ScriptLoader.cpp | 53 +++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 9de9a1d037..903822ef57 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -299,8 +299,12 @@ ScriptLoader::~ScriptLoader() //