-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JSC] ReferenceError when multiple modules are simultaneously importing a module containing a top-level await #24122
base: main
Are you sure you want to change the base?
Conversation
EWS run on previous version of this PR (hash 7aabdfe) |
This is a horrible bug. |
Is there a work around for this until this is fixed? This is causing a lot of issues for a product I'm working on and supporting Safari. |
The fix proposed in this PR breaks the case of module importing itself. I don't see any other way except for re-implementing the module loader to adhere to the spec (https://tc39.es/ecma262/#cyclic-module-record), which will take some time. |
So what do we do then until this is fixed, tell people not to use Safari? I mean this is a pretty big problem for any modern web application. |
Would be great if this could be prioritized. A lot of people using modern frameworks like SvelteKit or Astro (that use native ESM modules) are running into this, and it's often very hard to debug, so they don't realize it's this bug. |
Thank you for linking the issues, it's super important for our internal bookkeeping. Good news is that this issue being worked on, we are rewriting module loader to adhere to the spec, ETA is only a few weeks. |
@shvaikalesh, Is there an update on this? Also, when this gets merged, what is the usual process/timeline for getting this fix out to end users? |
7aabdfe
to
aee0d36
Compare
aee0d36
to
a0c5449
Compare
EWS run on previous version of this PR (hash a0c5449)
|
a0c5449
to
8c637db
Compare
EWS run on previous version of this PR (hash 8c637db)
|
8c637db
to
3aacdb9
Compare
EWS run on previous version of this PR (hash 3aacdb9)
|
3aacdb9
to
7f9c271
Compare
EWS run on previous version of this PR (hash 63e48fc)
|
8f80a0b
to
008d151
Compare
EWS run on previous version of this PR (hash 8f80a0b)
|
008d151
to
b6abb4b
Compare
b6abb4b
to
ef165bf
Compare
EWS run on previous version of this PR (hash b6abb4b)
|
EWS run on previous version of this PR (hash 008d151)
|
2 similar comments
β¦ng a module containing a top-level await https://bugs.webkit.org/show_bug.cgi?id=242740 <rdar://problem/97370038> Reviewed by NOBODY (OOPS!). This change completely re-implements module loader to precisely follow the spec steps [1], resolving multiple issues revolving around importing modules with top-level `await`. Infinite retry of module fetching & parsing is removed as it's not part of the spec, nor other browsers behave in the same way. This patch also rewires the way module loader is hooked into workers / worklets, which became necessary due to functions like evaluateModule() became always `async`. Along with this, errors occuring in modules with top-level `await` are now properly reported to the console, resolving another developers' pain point. [1]: https://tc39.es/ecma262/#sec-cyclic-module-records * JSTests/test262/expectations.yaml: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/json-module/cors-crossorigin-requests-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/json-module/repeated-imports.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/json-module/repeated-imports.any.sharedworker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/json-module/repeated-imports.any.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/microtasks/checkpoint-after-workerglobalscope-onerror-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/microtasks/checkpoint-after-workerglobalscope-onerror-module-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/microtasks/evaluation-order-1-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/microtasks/evaluation-order-1-throw-importScripts.any.sharedworker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/microtasks/evaluation-order-1-throw-importScripts.any.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/microtasks/evaluation-order-2-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/choice-of-error-2-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/choice-of-error-3-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/crossorigin-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url.any.sharedworker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url.any.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-fetch-error.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/error-type-2-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/error-type-3-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-1-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-2-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-3-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-4-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-1-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-2-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-3-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-4-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-5-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-6-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/instantiation-error-7-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/events/event-handler-processing-algorithm-error/worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/workers/modules/dedicated-worker-import-blob-url.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/workers/modules/dedicated-worker-import-blob-url.any.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/workers/modules/dedicated-worker-import-failure-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/workers/modules/dedicated-worker-parse-error-failure-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/workers/modules/shared-worker-import-blob-url.window-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/workers/modules/shared-worker-parse-error-failure-expected.txt: * Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj: * Source/JavaScriptCore/builtins/BuiltinNames.h: * Source/JavaScriptCore/builtins/ModuleLoader.js: (visibility.PrivateRecursive.ensureModuleMapEntry): (visibility.PrivateRecursive.hostFetchAndLoadImportedModule): (visibility.PrivateRecursive.hostLoadImportedModule): (visibility.PrivateRecursive.fetchDescendantsOfAndLink): (visibility.PrivateRecursive.innerModuleLoading): (visibility.PrivateRecursive.moduleLinking): (visibility.PrivateRecursive.innerModuleLinking): (visibility.PrivateRecursive.moduleEvaluation): (visibility.PrivateRecursive.innerModuleEvaluation): (visibility.PrivateRecursive.async executeAsyncModule): (linkTimeConstant.gatherAvailableAncestors): (visibility.PrivateRecursive.asyncModuleExecutionFulfilled): (linkTimeConstant.asyncModuleExecutionRejected): (visibility.PrivateRecursive.fetchModule): (visibility.PrivateRecursive.fetchModuleAndEvaluate): (visibility.PrivateRecursive.evaluateModule): (visibility.PrivateRecursive.loadModule): (visibility.PrivateRecursive.loadModuleAndEvaluate): (visibility.PrivateRecursive.requestImportModule): (visibility.PrivateRecursive.dependencyKeysIfEvaluated): (linkTimeConstant.setStateToMax): Deleted. (linkTimeConstant.newRegistryEntry): Deleted. (visibility.PrivateRecursive.ensureRegistered): Deleted. (linkTimeConstant.forceFulfillPromise): Deleted. (linkTimeConstant.fulfillFetch): Deleted. (visibility.PrivateRecursive.requestFetch): Deleted. (linkTimeConstant.cacheSatisfy): Deleted. (async linkTimeConstant.cacheSatisfyAndReturn): Deleted. (visibility.PrivateRecursive.requestSatisfy): Deleted. (visibility.PrivateRecursive.requestSatisfyUtil): Deleted. (visibility.PrivateRecursive.link): Deleted. (visibility.PrivateRecursive.async asyncModuleEvaluation): Deleted. (visibility.PrivateRecursive.provideFetch): Deleted. (visibility.PrivateRecursive.async loadModule): Deleted. (visibility.PrivateRecursive.linkAndEvaluateModule): Deleted. (visibility.PrivateRecursive.async loadAndEvaluateModule): Deleted. (visibility.PrivateRecursive.async requestImportModule): Deleted. * Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp: (JSC::BytecodeIntrinsicRegistry::BytecodeIntrinsicRegistry): * Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h: * Source/JavaScriptCore/jsc.cpp: (dumpException): (runWithOptions): * Source/JavaScriptCore/parser/Parser.cpp: (JSC::Parser<LexerType>::parseInner): * Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp: (JSC::AbstractModuleRecord::hostResolveImportedModule): (JSC::AbstractModuleRecord::link): * Source/JavaScriptCore/runtime/ArrayPrototype.cpp: (JSC::ArrayPrototype::finishCreation): * Source/JavaScriptCore/runtime/Completion.cpp: (JSC::createSymbolForEntryPointModule): Deleted. (JSC::rejectPromise): Deleted. (JSC::loadAndEvaluateModule): Deleted. (JSC::loadModule): Deleted. (JSC::linkAndEvaluateModule): Deleted. * Source/JavaScriptCore/runtime/Completion.h: * Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp: (JSC::JSC_DEFINE_HOST_FUNCTION): * Source/JavaScriptCore/runtime/JSModuleLoader.cpp: (JSC::JSModuleLoader::finishCreation): (JSC::createSymbolForEntryPointModule): (JSC::JSModuleLoader::loadModule): (JSC::JSModuleLoader::loadModuleAndEvaluate): (JSC::JSModuleLoader::fetchModule): (JSC::JSModuleLoader::fetchModuleAndEvaluate): (JSC::JSModuleLoader::evaluateModule): (JSC::JSC_DEFINE_HOST_FUNCTION): (JSC::JSModuleLoader::provideFetch): Deleted. (JSC::JSModuleLoader::loadAndEvaluateModule): Deleted. (JSC::JSModuleLoader::linkAndEvaluateModule): Deleted. * Source/JavaScriptCore/runtime/JSModuleLoader.h: * Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp: (JSC::SyntheticModuleRecord::link): * Source/JavaScriptCore/runtime/SyntheticModuleRecord.h: * Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp: (JSC::WebAssemblyModuleRecord::link): * Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.h: * Source/WebCore/bindings/js/JSExecState.h: (WebCore::JSExecState::loadModule): (WebCore::JSExecState::loadModuleAndEvaluate): (WebCore::JSExecState::fetchModule): (WebCore::JSExecState::fetchModuleAndEvaluate): (WebCore::JSExecState::evaluateModule): (WebCore::JSExecState::linkAndEvaluateModule): Deleted. * Source/WebCore/bindings/js/ScheduledAction.cpp: (WebCore::ScheduledAction::execute): * Source/WebCore/bindings/js/ScriptController.cpp: (WebCore::ScriptController::loadModuleScriptInWorld): (WebCore::ScriptController::evaluateModuleScriptInWorld): (WebCore::ScriptController::evaluateModuleScript): (WebCore::ScriptController::linkAndEvaluateModuleScriptInWorld): Deleted. (WebCore::ScriptController::linkAndEvaluateModuleScript): Deleted. * Source/WebCore/bindings/js/ScriptController.h: * Source/WebCore/bindings/js/WorkerScriptFetcher.h: * Source/WebCore/dom/ScriptElement.cpp: (WebCore::ScriptElement::executeModuleScript): * Source/WebCore/workers/DedicatedWorkerThread.h: (WebCore::DedicatedWorkerThread::workerObjectProxy const): (WebCore::DedicatedWorkerThread::start): Deleted. * Source/WebCore/workers/WorkerOrWorkletScriptController.cpp: (WebCore::WorkerOrWorkletScriptController::evaluateAndReportException): (WebCore::WorkerOrWorkletScriptController::evaluate): (WebCore::WorkerOrWorkletScriptController::loadModuleAndEvaluate): (WebCore::WorkerOrWorkletScriptController::fetchWorkletModuleAndEvaluate): (WebCore::jsValueToModuleKey): Deleted. (WebCore::WorkerOrWorkletScriptController::loadModuleSynchronously): Deleted. (WebCore::WorkerOrWorkletScriptController::linkAndEvaluateModule): Deleted. (WebCore::WorkerOrWorkletScriptController::loadAndEvaluateModule): Deleted. * Source/WebCore/workers/WorkerOrWorkletScriptController.h: * Source/WebCore/workers/WorkerOrWorkletThread.cpp: (WebCore::WorkerOrWorkletThread::workerOrWorkletThread): * Source/WebCore/workers/WorkerOrWorkletThread.h: (WebCore::WorkerOrWorkletThread::evaluateScriptIfNecessary): * Source/WebCore/workers/WorkerThread.cpp: (WebCore::WorkerThread::evaluateScriptIfNecessary): * Source/WebCore/workers/WorkerThread.h: * Source/WebCore/workers/shared/context/SharedWorkerContextManager.cpp: (WebCore::SharedWorkerContextManager::registerSharedWorkerThread): * Source/WebCore/worklets/WorkletGlobalScope.cpp: (WebCore::WorkletGlobalScope::evaluate): (WebCore::WorkletGlobalScope::fetchAndInvokeScript):
ef165bf
to
544b0f0
Compare
EWS run on previous version of this PR (hash ef165bf)
|
2 similar comments
Related: whatwg/html#10327 Side comment: as the person who recently rewrote most of the modules loading spec algorithms, I'm happy to see that following step-by-step them is actually a doable implementation strategy :) |
if (entry) | ||
return entry; | ||
|
||
entry = @newRegistryEntry(key); | ||
this.registry.@set(key, entry); | ||
entry = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the https://tc39.es/ecma262/#cyclic-module-record
link here?
entry.fetch = @undefined; | ||
return this.requestFetch(entry, parameters, fetcher); | ||
}); | ||
if (entry.errorToRethrow) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this since all fetchDescendantsOfAndLink
uses are guarded by the error check?
isLoading: true, | ||
pendingModulesCount: 1, | ||
visited: new @Set, | ||
parseError: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this? Or just use entry.errorToRethrow
is fine?
var loadedEntry = entry.loadedModules.@get(specifier); | ||
if (loadedEntry) { | ||
if (loadedEntry.errorToRethrow) { | ||
state.parseError = loadedEntry.errorToRethrow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we throw an error here?
|
||
for (var i = 0, { length } = entry.requestedModules; i < length; i++) { | ||
var specifier = entry.requestedModules[i]; | ||
var loadedEntry = entry.loadedModules.@get(specifier); // This implements step 2 of GetImportedModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw entry.errorToRethrow; | ||
return entry; | ||
}) | ||
.then(() => this.fetchDescendantsOfAndLink(entry, fetcher)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use .then((entry) => this.fetchDescendantsOfAndLink(entry, fetcher));
is better?
Satisfy, | ||
Link, | ||
Ready, | ||
enum class Status : uint8_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add See [[Status]] in https://tc39.es/ecma262/#cyclic-module-record
here?
putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().everyPrivateName(), getDirect(vm, vm.propertyNames->builtinNames().everyPublicName()), static_cast<unsigned>(PropertyAttribute::ReadOnly)); | ||
putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().filterPrivateName(), getDirect(vm, vm.propertyNames->builtinNames().filterPublicName()), static_cast<unsigned>(PropertyAttribute::ReadOnly)); | ||
putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().findPrivateName(), getDirect(vm, vm.propertyNames->builtinNames().findPublicName()), static_cast<unsigned>(PropertyAttribute::ReadOnly)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use them in builtin code. Use normal iteration.
|
||
entry.hasTLA = this.linkModuleRecord(entry.moduleRecord, fetcher); | ||
|
||
@assert(stack.@filter(e => e === entry).length === 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use filter
.
return this.asyncModuleEvaluation(entry, fetcher, dependencies); | ||
this.evaluate(entry.key, entry.moduleRecord, fetcher); | ||
|
||
@assert(stack.@filter(e => e === entry).length === 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
@assert(execList.@every(e => e.asyncEvaluation)); | ||
@assert(execList.@every(e => e.pendingAsyncDependencies === 0)); | ||
@assert(execList.@every(e => e.evaluationError === null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use them.
}; | ||
return moduleType === @ModuleTypeJavaScript | ||
? this.moduleMap.@get(key) | ||
: this.nonJSModuleArray.@find(entry => entry.key === key && entry.moduleType === moduleType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use find.
try { | ||
this.innerModuleLinking(entry, stack, 0, fetcher); | ||
} catch (error) { | ||
for (var i = 0, { length } = stack; i < length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
state.pendingModulesCount--; | ||
if (state.pendingModulesCount === 0) { | ||
state.isLoading = false; | ||
state.visited.@forEach(loadedEntry => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
state.visited.@add(entry); | ||
state.pendingModulesCount += entry.requestedModules.length; | ||
|
||
var { fetcher } = state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
state.pendingModulesCount += entry.requestedModules.length; | ||
|
||
var { fetcher } = state; | ||
for (var i = 0, { length } = entry.requestedModules; i < length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
var moduleType = parameters === @undefined ? @ModuleTypeJavaScript : this.getModuleType(parameters); | ||
var entry = this.ensureModuleMapEntry(key, moduleType); | ||
|
||
entry.fetchAndParsePromise ??= this.fetch(key, parameters, fetcher).then(sourceCode => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. do not use ??=
.
544b0f0
544b0f0