[robobun]

Related to #966

Problem

• A withdrawn termination cuts the link of a module. A build with assertions then aborts at the next import of it: ASSERTION FAILED: !m_moduleEnvironment (AbstractModuleRecord.cpp:1736).
• CyclicModuleRecord::initializeEnvironment() sets the environment in step 6. It arms the scope exit that clears it behind the exception check of that step (CyclicModuleRecord.cpp:242). A termination can begin at that check. link() then puts the module back to unlinked, with the environment still set.
• SyntheticModuleRecord::initializeExports() has the same gap for a record with a deferred generator: Duplicate export entry name.

Fix

• initializeEnvironment(): arm the scope exit before step 5.
• initializeExports(): add the export entries last, and clear the environment when the call does not get to its end.
• Correct because each function then leaves the record as it was, and the next link starts from there.
• Verified: JSTests/stress/module-link-cut-short-by-termination.js aborts without each change on a build with assertions, and passes with both.

Background

• A termination can begin at any exception check (RETURN_IF_EXCEPTION polls for it). An embedder can withdraw it (VM::cancelTermination(): node:vm's timeout in Bun) and use the same modules again.
• A deferred synthetic module has no text. Its generator makes the exports when the graph is about to be linked (SyntheticSourceProvider::createDeferred()). Bun has no caller today: only the shell hook of [JSC] Test hooks for a termination inside a module load: $vm.callWithTerminationAtTrapCheck(), $vm.moduleLoaderLoadSync(), setModuleLoaderFetchHook() #975 reaches it.
• The test needs two hooks of [JSC] Test hooks for a termination inside a module load: $vm.callWithTerminationAtTrapCheck(), $vm.moduleLoaderLoadSync(), setModuleLoaderFetchHook() #975: $vm.callWithTerminationAtTrapCheck() and setModuleLoaderFetchHook().

Downsides

• None found. The assertions are the only symptom: a release build passes the test both ways. Checked initializeExports(): nothing reads the export entries between their old and new place.

Notes

The code of initializeEnvironment() is the same in WebKit/WebKit (CyclicModuleRecord.cpp), so that change also fits upstream. The deferred generator is code of this fork.

This PR is stacked on #975 and goes to main when #975 has landed. Only the last commit is new.

Found by the walk of #966 (the cyclic record) and by the review of this PR (the synthetic record).

The test fails if no round cuts the import() in its link. It counts a round only when the cut import() ran no module, and the later import() fetched nothing and ran each module. That is the part between the end of the load and the first module body. For the module without text the round also has to be behind the read of the exports. On the lto build of cb3fc7f (the head since then, e6e90a9, differs only in jsc.cpp of #975) the test counts 28 of 120 rounds for the two modules (n = 68 to 95) and 44 of 138 for the module without text (n = 70 to 113). It takes 13 ms there.

On the debug build of #975 (e7c9e4d), which has neither change, the later import() aborts at 2 of 122 trap checks for the two modules (80 and 91, one for each module) and at 6 of 140 for the module without text (77 to 81: Duplicate export entry name 'a', and 101: !m_moduleEnvironment).

Size on the lto-aot build, #975 against this head: .text -64 B. SyntheticModuleRecord::initializeExports() -69 B (1,835 to 1,766), CyclicModuleRecord::initializeEnvironment() +1 B.

[robobun]

Status

How to reproduce on main with #975 (a build with assertions):

jsc --useDollarVM=1 --useJIT=0 JSTests/stress/module-link-cut-short-by-termination.js

It stops with ASSERTION FAILED: !m_moduleEnvironment (AbstractModuleRecord.cpp:1736). With the change in CyclicModuleRecord.cpp alone it stops at ASSERTION FAILED: Duplicate export entry name 'a', in the walk of the module without text. With both changes it passes. A release build passes in each case.

The pushes of today add the change in SyntheticModuleRecord::initializeExports(), which the review of this PR found, and the second walk (head e6e90a9).

This PR is stacked on #975 (the test hooks). Only the last commit is new.

[claude]

I reviewed this PR and didn't find any bugs. Since the change is in JavaScriptCore's module-linking core (a path listed under jsc-reviewers in CODEOWNERS) and the regression it fixes is only visible on an assertions build that CI does not run, a human look would still be worthwhile.

What was reviewed:

• The moved scope exit in CyclicModuleRecord::initializeEnvironment(): its !m_initialized condition is unchanged, and m_initialized is only set later in the function (lines 439 and 530), so the path that finishes initialization behaves as before.
• The two earlier returns now covered (after getOrMakeExecutable and JSModuleEnvironment::create) clear a field that is still null; the only new effect is on the exception check after setModuleEnvironment(), which is where a termination leaves m_moduleEnvironment set while link() resets the status to UNLINKED.
• The new test's dependencies ($vm.callWithTerminationAtTrapCheck, hasPendingTermination, createModuleLoader, moduleLoaderImport and the resources/module-loader-load-sync fixtures) are present in this checkout.

Extended reasoning...

The diff is one commit touching two files: a six-line relocation of an existing WTF::makeScopeExit in Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp so it is armed before step 5/6 of InitializeEnvironment, plus a new stress test JSTests/stress/module-link-cut-short-by-termination.js. It touches no security-sensitive surface (no auth, crypto, or input parsing), only the cleanup of a module record's environment after a withdrawn termination. The code change is small and its correctness follows from the unchanged condition and from AbstractModuleRecord::setModuleEnvironment() storing the environment only at its end. Defer rather than approve because /Source/JavaScriptCore and /JSTests are covered by CODEOWNERS (jsc-reviewers) and the fix is validated only by an assertion-build run that the fork's CI lanes do not exercise.

[github-actions]

Preview build of e6e90a9: autobuild-preview-pr-976-e6e90a93

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding (which is pre-existing in the synthetic record path, not introduced here), I checked the moved scope exit in Source/JavaScriptCore/runtime/CyclicModuleRecord.cpp:233 — the two exits now covered between steps 5 and 6 (getOrMakeExecutable and JSModuleEnvironment::create) clear a still-empty field, and the !m_initialized guard is unchanged, so the initialized path behaves as before. The test's new ranBeforeTheCut/fetched gating in commit ac90b8 does make the final linkedAgain check able to fail, which addresses the earlier nit.

Extended reasoning...

The PR moves a WTF::makeScopeExit six lines earlier in CyclicModuleRecord::initializeEnvironment() so a termination taken at the exception check after setModuleEnvironment() still clears m_moduleEnvironment, plus a new stress test and a one-row doc table entry. No security-sensitive surface is touched. The paths are covered by CODEOWNERS (jsc-reviewers) and one inline finding about the analogous SyntheticModuleRecord path is posted, so a human look is still appropriate rather than approval.

[claude]

I reviewed the updated PR (through cb3fc7f) and found no bugs; the new commit covers both of the inline threads from the earlier runs. Because the change is in JSC module linking, which CODEOWNERS routes to jsc-reviewers, a human look is still worthwhile.

What was reviewed:

• CyclicModuleRecord::initializeEnvironment(): the scope exit keeps its !m_initialized condition and now also covers the getOrMakeExecutable()/setModuleEnvironment() exception checks; on the path where jsModule is null it clears an already-empty field.
• SyntheticModuleRecord::initializeExports(): moving addExportEntry() after the put loop has no reader in between (the puts go through the symbol table, not the export entries), and the isInitialized guard leaves no environment or entries behind on either the termination or the ordinary-exception path; runDeferredGenerator() discards the record's generator on an ordinary exception, so the cleared state is never re-linked there.
• The test: the !fetched and inTheLink() conditions make the linkedAgain count require that the cut import() had already loaded the graph, so if (!linkedAgain) can fire; the fixture resources/module-loader-load-sync/root.js exists and the second graph comes entirely from the fetch hook.

Extended reasoning...

The PR touches two JSC runtime files (CyclicModuleRecord.cpp and SyntheticModuleRecord.cpp, about 20 lines of logic), adds one stress test, and adds a line to JSTests/BUN-TEST-DIFFERENCES.md. It touches no security-sensitive surface (no injection, auth, or data exposure); the change is cleanup ordering around a termination exception during module link. The bug hunt ran dry with no findings, and the latest commit addresses both earlier inline comments. Approval is withheld only because /Source/JavaScriptCore and /JSTests are covered by a CODEOWNERS entry, so the guidelines route this to a human.

[claude]

LGTM — the latest push covers both points raised earlier (the synthetic record's cleanup and the test's round counting), and the two source hunks are small and self-contained.

What was reviewed:

• CyclicModuleRecord::initializeEnvironment(): the scope exit now also covers exceptions at getOrMakeExecutable(), JSModuleEnvironment::create() and setModuleEnvironment(); at those points m_moduleEnvironment is either null or stale from a cut link, and the WebAssembly path (environment set by wasmModule->link()) is unchanged.
• SyntheticModuleRecord::initializeExports(): checked that nothing between the old and new place of addExportEntry() reads export entries (setModuleEnvironment() and symbolTablePutTouchWatchpointSet() only touch the symbol table), and that a non-termination failure still ends up in m_deferredGeneratorError with no environment left behind.
• Test: linkedAgain now requires wasCut, no module run before the cut, zero fetches and every text module run by the later import(), so the n = 1 case no longer counts.

Extended reasoning...

The change is two short hunks in JSC's module-link path (CyclicModuleRecord.cpp and SyntheticModuleRecord.cpp) that make a termination-cut link leave no module environment or export entries on a record that is linked again, plus a new stress test and a one-row table entry in JSTests/BUN-TEST-DIFFERENCES.md. It touches no security-sensitive surface (no auth, crypto, injection or data exposure); it only reorders a scope exit and the point at which export entries are added. The diff is small enough to trace fully, I found nothing between the moved statements that depends on the old order, and the new commit addresses both inline comments from earlier runs. The only prior reviews are bot comments with no CHANGES_REQUESTED state, and the author account is the fork's bot.