From e6e90a9315ca26c521689271cb51c40597f0a88c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 31 Oct 2026 16:49:17 +0000 Subject: [PATCH] [JSC] A link that a termination cuts leaves no environment on a module that is linked again CyclicModuleRecord::initializeEnvironment() sets the module environment in step 6 and clears it again when it ends before the module is initialized. The scope exit that clears it was armed behind the exception check of step 6. That check handles traps, so a termination can begin there. Link() then puts the module back to UNLINKED with the environment still set, and the next Link() of the module stops at ASSERT(!m_moduleEnvironment) in setModuleEnvironment(). Arm the scope exit before step 5. SyntheticModuleRecord::initializeExports() had the same gap for a record with a deferred generator. After a termination the generator runs again at the next link, and initializeExports() then met the export entries and the environment of the call that was cut ("Duplicate export entry name", ASSERT(!m_moduleEnvironment)). Add the export entries last, and clear the environment when the call does not get to its end. Test: JSTests/stress/module-link-cut-short-by-termination.js. It aborts without each change on a build with assertions. The assertions are the only symptom, so a release build passes it with and without the changes. --- JSTests/BUN-TEST-DIFFERENCES.md | 1 + .../module-link-cut-short-by-termination.js | 98 +++++++++++++++++++ .../runtime/CyclicModuleRecord.cpp | 11 ++- .../runtime/SyntheticModuleRecord.cpp | 12 ++- 4 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 JSTests/stress/module-link-cut-short-by-termination.js diff --git a/JSTests/BUN-TEST-DIFFERENCES.md b/JSTests/BUN-TEST-DIFFERENCES.md index d6377d6c29064..d4e25f01a9f3c 100644 --- a/JSTests/BUN-TEST-DIFFERENCES.md +++ b/JSTests/BUN-TEST-DIFFERENCES.md @@ -239,6 +239,7 @@ Not upstream's: a sync neither brings nor changes them. Most are named `stress/b | `stress/vm-termination-at-trap-check.js` and `-with-watchdog.js` | `$vm.callWithTerminationAtTrapCheck(fn, n)` requests a termination at the n-th trap check of a call and withdraws it when the call ends: each larger n lets the call get at least as far, native code is stopped too, no termination is pending before that check (`Atomics.wait()` goes to its timeout), each trap check counts once, none counts while terminations are deferred, a time limit around the call or inside its function still ends what it limits, and nothing is left pending. A VM with a watchdog and script that runs with terminations deferred are refused. The test hook for what a termination leaves behind in the middle of an operation | | `stress/module-loader-load-sync.js` | `$vm.moduleLoaderLoadSync({ loader }, specifier)` is `JSModuleLoader::loadModuleSync()`, the load of `require()` of an ES module in Bun: the graph has run when the call returns, a load that `import()` began is taken over, what the module throws is thrown, and a top-level await is a `TypeError` | | `stress/shell-module-loader-fetch-hook.js` | `setModuleLoaderFetchHook(fn)` of the shell: `fn(key)` runs before the file of a module is read, a string that it returns is the text of the module, an object makes a module without text whose exports are read when the graph is about to be linked (a deferred synthetic module, `SyntheticSourceProvider::createDeferred()`), the hook is asked for each fetch of a JavaScript module, and what it throws is what the load fails with. Tests use it for modules without files and for script inside the fetch step of a load | +| `stress/module-link-cut-short-by-termination.js` | a termination that begins in the link of a module, in `CyclicModuleRecord::initializeEnvironment()` or in `SyntheticModuleRecord::initializeExports()` of a module whose exports are made at the link, and that is withdrawn leaves no environment and no export entries on a module that is linked again: the next `import()` links it (an assertion build stopped at `ASSERT(!m_moduleEnvironment)` or at `Duplicate export entry name`) | `jsc` cannot compile a program ahead of time by itself (the compiler's input comes from the front end, in Bun's repository), so the programs that test ahead-of-time compilation are in Bun's `test/aot`. diff --git a/JSTests/stress/module-link-cut-short-by-termination.js b/JSTests/stress/module-link-cut-short-by-termination.js new file mode 100644 index 0000000000000..f2baeae5d35e4 --- /dev/null +++ b/JSTests/stress/module-link-cut-short-by-termination.js @@ -0,0 +1,98 @@ +//@ runDefault("--useDollarVM=1", "--useJIT=0") + +// A termination can begin inside the link of a module, behind the step that sets the environment of the module: +// in CyclicModuleRecord::initializeEnvironment(), and in SyntheticModuleRecord::initializeExports() for a module +// without text whose exports are made at the link. The module is then linked again by the next load, and that link +// sets an environment again. So the cut link has to leave none: ASSERT(!m_moduleEnvironment) in an assertion build. +// +// Each walk takes a termination at each trap check of an import() in turn ($vm.callWithTerminationAtTrapCheck()) and +// imports the module again. A cut inside the load is not the subject here: there the later import() can stay pending. + +function shouldBe(actual, expected, what) { + if (actual !== expected) + throw new Error(`${what}: expected ${expected}, got ${actual}`); +} + +const terminationValue = "JavaScript execution terminated."; +// The modules with text add their names here when they run. +globalThis.loadSyncLog = []; +// The number of modules that the loader asked the shell for. +let fetched = 0; +setModuleLoaderFetchHook(key => { + ++fetched; + if (key.endsWith("/imports-object.js")) + return `import { a, b, c } from "./object.js"; globalThis.loadSyncLog.push("imports-object"); export const value = a + b + c;`; + // A module without text. The shell reads its exports when the graph is about to be linked. + if (key.endsWith("/object.js")) { + return { + get a() { + exportsWereRead = true; + return 1; + }, + b: 2, + c: 38, + }; + } +}); +// Whether the exports of the module without text have been read. +let exportsWereRead = false; + +function importRoot(loader, root) { + const load = { state: "pending", value: undefined }; + $vm.moduleLoaderImport(loader, root).then(namespace => { + load.value = namespace.value; + load.state = "fulfilled"; + }, error => { + load.value = error; + load.state = "rejected"; + }); + drainMicrotasks(); + return load; +} + +// `inTheLink()` tells whether a cut import() that has loaded its graph and has run no module was cut in the link, where +// the test can tell. +function walk(what, root, modulesWithText, inTheLink = () => true) { + // A function has more trap checks while its inline caches are empty, so the walk imports the graph a few times first. + for (let i = 0; i < 10; ++i) + importRoot($vm.createModuleLoader(), root); + + let linkedAgain = 0; + for (let n = 1; ; ++n) { + if (n > 100000) + throw new Error(`${what}: the import() has no end`); + const loader = $vm.createModuleLoader(); + loadSyncLog.length = 0; + exportsWereRead = false; + const wasCut = $vm.callWithTerminationAtTrapCheck(() => { importRoot(loader, root); }, n); + shouldBe($vm.hasPendingTermination(), false, `${what}, trap check ${n}: a termination is still pending`); + const ranBeforeTheCut = loadSyncLog.length; + const wasCutInTheLink = inTheLink(); + + loadSyncLog.length = 0; + fetched = 0; + const later = importRoot(loader, root); + if (later.state == "fulfilled") { + shouldBe(later.value, 41, `${what}, trap check ${n}: the value of the later import()`); + // The later import() fetched nothing and ran each module. So the cut import() had loaded the graph and + // had run none of it: the termination began in the link or next to it, and the later import() linked + // the graph. + if (wasCut && !ranBeforeTheCut && wasCutInTheLink && !fetched && loadSyncLog.length == modulesWithText) + ++linkedAgain; + } else if (later.state == "rejected") { + // A module that the termination stopped in its evaluation keeps that as its error. + if (later.value !== null && later.value !== terminationValue) + throw new Error(`${what}, trap check ${n}: the later import() rejected with ${later.value}`); + } + if (!wasCut) { + shouldBe(later.state, "fulfilled", `${what}, trap check ${n}: the later import() of a call that was not cut`); + break; + } + } + if (!linkedAgain) + throw new Error(`${what}: no later import() linked a graph again whose link the termination had cut`); +} + +walk("two modules", "./resources/module-loader-load-sync/root.js", 2); +// The exports of the module without text are read in the link, before SyntheticModuleRecord::initializeExports(). +walk("a module without text", "./module-link-cut-short-by-termination/imports-object.js", 1, () => exportsWereRead); diff --git a/Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp b/Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp index 338a503cd9951..3a675068e1028 100644 --- a/Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp +++ b/Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp @@ -229,6 +229,12 @@ void CyclicModuleRecord::initializeEnvironment(JSGlobalObject* globalObject, Ref #endif // 3. Let realm be module.[[Realm]]. // 4. Assert: realm is not undefined. + // Armed before step 6: the exception check behind it can begin a termination, and Link() then puts the module back to UNLINKED. + auto cleanup = WTF::makeScopeExit([&] { + if (!m_initialized) + m_moduleEnvironment.clear(); + }); + SymbolTable* symbolTable = nullptr; if (jsModule) { // 5. Let env be NewModuleEnvironment(realm.[[GlobalEnv]]). @@ -242,11 +248,6 @@ void CyclicModuleRecord::initializeEnvironment(JSGlobalObject* globalObject, Ref RETURN_IF_EXCEPTION(scope, void()); } - auto cleanup = WTF::makeScopeExit([&] { - if (!m_initialized) - m_moduleEnvironment.clear(); - }); - // 7. For each ImportEntry Record in of module.[[ImportEntries]], do // Steps 7.b onward for one entry; the caller checks for an exception. auto linkImportEntry = [&](const ImportEntry& in, AbstractModuleRecord* importedModule) { diff --git a/Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp b/Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp index 6117662dd2f53..4593d3ed2da71 100644 --- a/Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp +++ b/Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp @@ -32,6 +32,7 @@ #include "JSModuleEnvironment.h" #include "JSModuleNamespaceObject.h" #include "JSONObject.h" +#include namespace JSC { @@ -199,10 +200,16 @@ void SyntheticModuleRecord::initializeExports(JSGlobalObject* globalObject, cons for (auto& exportName : exportNames) { auto offset = exportSymbolTable->takeNextScopeOffset(NoLockingNecessary); exportSymbolTable->add(NoLockingNecessary, exportName.impl(), SymbolTableEntry(VarOffset(offset))); - moduleRecord->addExportEntry(ExportEntry::createLocal(exportName, exportName)); } JSModuleEnvironment* moduleEnvironment = JSModuleEnvironment::create(vm, globalObject, nullptr, exportSymbolTable, jsTDZValue(), moduleRecord); + // A termination can begin at each exception check below, and a record with a deferred generator then makes its + // exports again at the next link. So the record keeps nothing of a call that does not get to its end. + bool isInitialized = false; + auto cleanup = WTF::makeScopeExit([&] { + if (!isInitialized) + m_moduleEnvironment.clear(); + }); moduleRecord->setModuleEnvironment(globalObject, moduleEnvironment); RETURN_IF_EXCEPTION(scope, void()); @@ -229,10 +236,13 @@ void SyntheticModuleRecord::initializeExports(JSGlobalObject* globalObject, cons ASSERT(putResult); } + for (auto& exportName : exportNames) + moduleRecord->addExportEntry(ExportEntry::createLocal(exportName, exportName)); #if USE(BUN_JSC_ADDITIONS) if (hasLazyExports) moduleRecord->m_lazyExportsSource.set(vm, moduleRecord, lazyExportsSource); #endif + isInitialized = true; } #if USE(BUN_JSC_ADDITIONS)