test: each vm.SourceTextModule runs its own importModuleDynamically hook by robobun · Pull Request #42496 · oven-sh/bun

robobun · 2026-09-12T17:16:40Z

Behaviour change: none

Problem

main has no test for a regression that Upgrade WebKit to cf1b36ec8703 #42319 introduced and the WebKit bump in Bump WebKit to 1600131e46b5 #44500 removed.
A second vm.SourceTextModule with the same identifier and source text as an earlier one called the first module's importModuleDynamically hook on import() (hook-calls=0 got=first want=second). Its stack traces reported the first module's lineOffset.
The cause was in JSC: records with the same key and source text ran one ModuleProgramExecutable, which keeps the first module's source. [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639 ended that for sources with different SourceOrigins.

Fix

This PR now only adds tests. The WEBKIT_VERSION change is gone, because main's pin contains [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639.
test/js/node/vm/vm.test.ts: each module calls its own hook with itself as referrer, and reports positions with its own lineOffset.
vm-script-fetcher-leak.test.ts: the alive-sameSourceModules scenario. Each module's hook stays alive while that module's function is alive.
Verified: a debug build of main at f8bc171 passes both files (335 pass, 0 fail). The release canary 1.4.3-canary.1+367d939d9, from before the WebKit change, fails both vm.test.ts tests.

Background

JSC hands an import() to the embedder with the running code's SourceOrigin: a URL plus an optional ScriptFetcher, an embedder object.
node:vm creates one NodeVMScriptFetcher per module. It holds the hook weakly, alive while code from that source is alive (node:vm: importModuleDynamically lives as long as the code that can import() #43724).
JSC finds an executable to share through a weak map, and a module's functions keep the executable alive. So every test source exports a function and the tests keep every module reachable.

Notes

Repro (bun repro.mjs, or node --experimental-vm-modules repro.mjs):

import vm from "node:vm";
const mk = async tag => {
  const m = new vm.SourceTextModule(`export const who = ${JSON.stringify(tag)};`, { identifier: "dep-" + tag });
  await m.link(() => {});
  await m.evaluate();
  return m;
};
async function once(identifier, tag) {
  const d = await mk(tag);
  let calls = 0;
  const m = new vm.SourceTextModule("export const p = import('z');", {
    identifier,
    importModuleDynamically() {
      calls++;
      return d;
    },
  });
  await m.link(() => {});
  await m.evaluate();
  const ns = await m.namespace.p;
  return `hook-calls=${calls} got=${ns.who} want=${tag}`;
}
console.log(await once("same", "first"));
console.log(await once("same", "second"));

How main got the change: [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639 merged on 2026-10-02 as 52bda5ef48aa. The first pin that contains it is 1600131e46b5 (Bump WebKit to 1600131e46b5 #44500). The pin before it, fb1167ebf2cb, does not. The current pin 87ee10da087b contains it, and its JSModuleRecord.cpp has hasSameObservableSource. Checked with git merge-base --is-ancestor on the WebKit history.
The repro prints hook-calls=1 got=second want=second on a debug build of main at f8bc171 and on Node 26.3.0. The release canary 1.4.3-canary.1+367d939d9 (WebKit 000c48997255, before the change) prints hook-calls=0 got=first want=second.
Tests on the debug build of main: bun bd test test/js/node/vm/vm.test.ts test/js/node/vm/vm-script-fetcher-leak.test.ts gives 335 pass, 4 skip, 61 todo, 0 fail.
Tests without the WebKit change:
- The release canary 1.4.3-canary.1+367d939d9 fails both new vm.test.ts tests. The first receives the first module's hook calls and imported: ["first", "first"] for the second module. The second receives line 100 where it expects line 1.
- On 2026-09-23 an ASAN debug build of main at 6d504dd (WebKit 299c5323, with node:vm: importModuleDynamically lives as long as the code that can import() #43724, without [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639) failed the two vm.test.ts tests and the alive-sameSourceModules scenario, with and without BUN_JSC_collectContinuously=1. The second module's function answered {"result":"hooked by first"}. The same tree with the preview build of [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639 passed them.
The alive-sameSourceModules fixture scenario also runs under Node (--expose-gc --experimental-vm-modules --no-compilation-cache) and prints the expected output.
Hook lifetime: the fetcher holds the hook in a Weak that lives while an executable of that source is alive ([JSC] A live ScriptExecutable makes its source's ScriptFetcher an opaque root WebKit#712 makes the fetcher an opaque root of the executable). When the second module's functions ran the first module's executable, nothing rooted the second module's fetcher. The scenario keeps only each module's exported function, collects, and expects each function to reach its own hook with its own module as referrer.
Extent of the old bug: only the same identifier with the same source text in one context. A different identifier, a different text, no identifier, one context per module, vm.Script and vm.compileFunction were correct.
The map of executables (JSGlobalObject::m_moduleProgramExecutables) is a WeakGCMap. After a full collection the first executable could be dead, and then the second module linked its own. That is why the tests keep every module reachable and export a function from every source.
The lineOffset test compares against a module with an identifier that was never seen, so it does not depend on the absolute line numbers.
main already has a test named "SourceTextModules with the same identifier and source keep their own import bindings" (Upgrade WebKit to d3720d515e14: module linking without sorting import names or resolving every import up front #42823). It covers the import bindings of such records, not the hook or the offsets. The new block sits next to it.
bun check on the two changed .ts files reports no type errors.
History of this PR: earlier versions pinned a preview build of [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639 and were a draft until that PR merged. The pin is gone and the branch is one commit on main. The cost of the WebKit change (identical vm.SourceTextModules no longer share linked code) is measured in [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639.

no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/vm/vm.test.ts, test/js/node/vm/vm-script-fetcher-leak.test.ts

robobun · 2026-09-12T17:16:51Z

^{Updated 6:43 PM PT - Oct 16th, 2026}

❌ @robobun, your commit 0836b83 has 1 failures in Build #126899 (All Failures):

test/js/web/workers/worker-terminate-lifetime.test.ts - code 1 on 🍎 any x64

🧪 To try this PR locally:

bunx bun-pr 42496

That installs a local version of the PR into your bun-42496 executable, so you can run:

bun-42496 --bun

robobun · 2026-09-12T17:17:06Z

Status

[JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639 merged on 2026-10-02, and main's WebKit pin contains it since Bump WebKit to 1600131e46b5 #44500. The bug no longer reproduces on main: on a debug build at f8bc171 the script in the PR notes prints hook-calls=1 got=second want=second. The release canary 1.4.3-canary.1+367d939d9, from before the WebKit change, prints hook-calls=0 got=first want=second.
This PR is now one commit on main that only adds tests. The WEBKIT_VERSION change is gone.
bun bd test test/js/node/vm/vm.test.ts test/js/node/vm/vm-script-fetcher-leak.test.ts on that debug build: 335 pass, 0 fail. The pre-change canary fails both new vm.test.ts tests.
Ready for review.

Two SourceTextModules with the same identifier and source text ran one shared ModuleProgramExecutable. The second module's import() called the first module's hook, its stack traces reported the first module's lineOffset, and only the first module's hook stayed alive with the code. The WebKit bump in #44500 (oven-sh/WebKit#639) gives each module its own executable again. Add tests for the hook, the referrer, the line offset and the hook lifetime.

coderabbitai · 2026-10-16T21:57:47Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 8ddbefaa-17d1-4606-8637-fb68815d8441

📥 Commits

Reviewing files that changed from the base of the PR and between f8bc171 and 0836b83.

📒 Files selected for processing: 3

test/js/node/vm/vm-import-callback-lifetime-fixture.js
test/js/node/vm/vm-script-fetcher-leak.test.ts
test/js/node/vm/vm.test.ts

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

claude

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

claude · 2026-10-16T22:05:14Z

+    }
+    const shifted = await lineOfThrow("same-identifier-other-offset.mjs", 100);
+    const unshifted = await lineOfThrow("same-identifier-other-offset.mjs", 0);
+    expect(unshifted).toBe(await lineOfThrow("never-seen-identifier.mjs", 0));


🟡 nit (optional): a wrong lineOffset applied to the second same-identifier module (off by one, doubled, partially shifted) still passes this test. The assertion at test/js/node/vm/vm.test.ts:1144 only checks shifted > unshifted, although lineOffset 100 must shift the reported line by exactly 100. Fix: assert the exact invariant, expect(shifted).toBe(unshifted + 100), which stays independent of absolute line numbers and matches the exact-line assertions the existing lineOffset test at vm.test.ts:2544 already makes. [also at: test/js/node/vm/vm.test.ts:1146 - nit: the lineOffset test only checks shifted is greater than unshifted, so a module that applied a wrong non-zero offset would still pass.]

Why this was flagged

The test at test/js/node/vm/vm.test.ts:1130-1145 compiles two SourceTextModules with identifier "same-identifier-other-offset.mjs", lineOffset 100 then 0, and reads the line from new Error("here").stack. At vm.test.ts:1144 it asserts only expect(shifted).toBeGreaterThan(unshifted). Any regression where the offset of a shared or re-used executable is applied wrongly but still positive (for example line 2, 50 or 201 instead of 101) passes. The repository review rules ask for the strongest invariant; the sibling test at vm.test.ts:2544 asserts the exact line for lineOffset 5. The base branch has no test here, so this is a weaker-than-necessary new assertion rather than a regression.

Verification: nit. Triggering condition: any regression that applies a SourceTextModule lineOffset of 100 as some other positive shift (off by one, doubled, partial) at function-call time. Mechanism verified: /home/claude/bun/test/js/node/vm/vm.test.ts:1144 reads expect(shifted).toBeGreaterThan(unshifted);. Line 1144 accepts 2, 50 or 201 just as readily as 101.

claude · 2026-10-16T22:05:14Z

+  test("each call their own importModuleDynamically", async () => {
+    const first = await load("first");
+    const second = await load("second");
+    const calls = [
+      { specifier: "dep", referrerIsThisModule: true },
+      { specifier: "dep", referrerIsThisModule: true },
+    ];
+    expect({ first, second }).toEqual({
+      first: { calls, imported: ["first", "first"] },
+      second: { calls, imported: ["second", "second"] },
+    });
+  });


🟡 nit (optional): maintainers get coverage for same-identifier executable sharing only in the main context, while the sibling test directly above runs both contexts. The new describe block at test/js/node/vm/vm.test.ts:1079 never passes a context option, so both tests only exercise the Zig global object. Fix: run the new hook and lineOffset tests over the same ["the main context", false], ["a new context", true] matrix the sibling at vm.test.ts:1033 uses, passing context to every SourceTextModule and SyntheticModule it creates. The executable map that enabled the old sharing is per JSGlobalObject, so a vm context is a separate instance of the hazard. [also at: test/js/node/vm/vm.test.ts:1079 - nit: two same-identifier modules sharing one vm.createContext() are not covered, so a regression limited to non-main contexts would pass this suite.]

Why this was flagged

The existing test at test/js/node/vm/vm.test.ts:1033-1077 covers the same-identifier-and-source hazard with test.each over the main context and a createContext({}) context, because the module executable map lives on each JSGlobalObject. The new describe at test/js/node/vm/vm.test.ts:1079 adds the hook and lineOffset variants but load() at vm.test.ts:1091 and lineOfThrow at vm.test.ts:1131 never set context, so every module lands on the main Zig::GlobalObject. A vm context is a NodeVMGlobalObject with its own executable map and its own import() entry at src/jsc/bindings/NodeVM.cpp:1700. If executable sharing came back for records on a NodeVMGlobalObject only, the second module in a context would again run the first module's importModuleDynamically hook and report the first module's lineOffset, and neither new test would fail. On the base branch there is no coverage for either context; after merging there is coverage for one of the two the sibling test treats as distinct.

Verification: A future regression of same-identifier executable sharing that is specific to vm contexts would go uncaught, because the new tests only run on the main global. The new describe block (test/js/node/vm/vm.test.ts:1079-1145) constructs every SourceTextModule and SyntheticModule without a context option. The sibling test directly above (vm.test.ts:1033-1077) runs a test.each matrix over both contexts.

robobun assigned Jarred-Sumner Sep 12, 2026

github-actions Bot added the claude label Sep 12, 2026

robobun mentioned this pull request Sep 12, 2026

[JSC] Module records share an executable only when their sources have the same SourceOrigin and start position oven-sh/WebKit#639

Merged

robobun force-pushed the robobun/e246f767/vm-module-own-import-hook branch from 5e97450 to 5f4e995 Compare September 23, 2026 06:49

robobun changed the title ~~node:vm: stop a SourceTextModule from running another module's importModuleDynamically hook~~ test: each vm.SourceTextModule runs its own importModuleDynamically hook Oct 16, 2026

robobun force-pushed the robobun/e246f767/vm-module-own-import-hook branch from 5f4e995 to 0836b83 Compare October 16, 2026 21:57

robobun marked this pull request as ready for review October 16, 2026 21:57

claude Bot reviewed Oct 16, 2026

View reviewed changes

Navigation Menu

Repository navigation

test: each vm.SourceTextModule runs its own importModuleDynamically hook - #42496

test: each vm.SourceTextModule runs its own importModuleDynamically hook#42496
robobun wants to merge 1 commit into
mainfrom
robobun/e246f767/vm-module-own-import-hook

robobun commented Sep 12, 2026 •

edited

Loading

Uh oh!

robobun commented Sep 12, 2026 •

edited

Loading

Uh oh!

robobun commented Sep 12, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 16, 2026

Review limit reached

Uh oh!

claude Bot left a comment

Uh oh!

claude Bot Oct 16, 2026

Uh oh!

claude Bot Oct 16, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

Uh oh!

robobun commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 16, 2026

Review limit reached

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Sep 12, 2026 •

edited

Loading

robobun commented Sep 12, 2026 •

edited

Loading

robobun commented Sep 12, 2026 •

edited

Loading