[JSC] A link that a termination cuts leaves no environment on a module that is linked again

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`.

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

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) {

Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp

@@ -32,6 +32,7 @@
 #include "JSModuleEnvironment.h"
 #include "JSModuleNamespaceObject.h"
 #include "JSONObject.h"
+#include <wtf/Scope.h>
 
 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)