Conversation
Collaborator
Author
|
Updated 11:01 AM PT - Sep 12th, 2026
❌ @robobun, your commit 5e97450 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 42496That installs a local version of the PR into your bun-42496 --bun |
Collaborator
Author
|
Status
|
…hook Bump WebKit to the preview build of oven-sh/WebKit#639. Module records with the same key and source text no longer share a ModuleProgramExecutable when their sources differ in SourceOrigin or start position. A second vm.SourceTextModule with the same identifier and source text as an earlier one called the earlier module's hook on import() and reported its lineOffset.
…ep their own hook alive The importModuleDynamically hook lives as long as code from its source is alive. When two SourceTextModules ran one shared executable, only the first module's fetcher was rooted, so the second module's function reached the first module's hook. The new lifetime scenario keeps only each module's exported function and checks that each one reaches its own hook and referrer.
robobun
force-pushed
the
robobun/e246f767/vm-module-own-import-hook
branch
from
September 23, 2026 06:49
5e97450 to
5f4e995
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft until oven-sh/WebKit#639 lands on
main. ThenWEBKIT_VERSIONmoves to that sha.Problem
vm.SourceTextModulewith the same identifier and source text as an earlier one never calls its ownimportModuleDynamicallyhook. Itsimport()calls the first module's hook (hook-calls=0 got=first want=second), and its stack traces report the first module'slineOffset. Regression from Upgrade WebKit to cf1b36ec8703 #42319. Bun 1.4.2 and Node are correct.ModuleProgramExecutable, which keeps the first module's source.node:vmfinds the hook through that source.Fix
WEBKIT_VERSIONto the preview build of [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639. Records share an executable only when their sources have the sameSourceOrigin, taint state and start position.vm.SourceTextModulehas its ownSourceOrigin, so each links its own executable, as in 1.4.2.test/js/node/vm/vm.test.tsand one scenario invm-script-fetcher-leak.test.ts. A debug build ofmainfails them. The patched build passes them.Background
import()to the embedder with the running code'sSourceOrigin: a URL plus an optionalScriptFetcher, an embedder object.node:vmcreates oneNodeVMScriptFetcherper 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). Shared code kept only the first module's hook alive.Downsides
vm.SourceTextModules no longer share linked code: 6.5 KB more per live duplicate of a 41-function module (8187 against 1641 bytes). No release shipped the sharing.Notes
Repro (
bun repro.mjs, ornode --experimental-vm-modules repro.mjs):mainat 6d504dd (WebKit 299c5323) fails the twovm.test.tstests and thealive-sameSourceModulesscenario, with and withoutBUN_JSC_collectContinuously=1. It printshook-calls=0 got=first want=secondfor the second line of the repro. Node 26.3.0 and a build with the patched WebKit printhook-calls=1 got=second want=second. The release canary1.4.3-canary.1+6a92015fc(the Upgrade WebKit to cf1b36ec8703 #42319 commit) shows the same failure.Weakthat 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). The second module's functions ran the first module's executable, so nothing rooted the second module's fetcher. Thealive-sameSourceModulesscenario keeps only each module's exported function, collects, and expects each function to reach its own hook with its own module as referrer.mainanswers{"result":"hooked by first"}for the second function. The fixture also runs under Node, which gives the expected output.FinalizationRegistryon the hook fires. The hook does not leak.1.4.3-canary.1+367d939d9, which still shares. With one identifier: 1ModuleProgramExecutable, 1541 heap bytes plus 100 bytes of extra memory per module, 0.062 ms per module. With one identifier each (never shared, which is what every module gets after the bump): 2000 executables, 5419 plus 2768 bytes, 0.071 ms. The byte counts were identical in two runs. On the patched debug build both modes report one executable per module.mainon 2026-09-23. The pin moved from the preview build of the first version of [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position WebKit#639 to the preview build of its rebase onto WebKitmain(299c5323, whichmainpins since node:vm: importModuleDynamically lives as long as the code that can import() #43724).vm.Scriptandvm.compileFunctionare correct, because the map of executables is per global object and compares the key and the text.JSGlobalObject::m_moduleProgramExecutables) is aWeakGCMap. After a full collection the first executable can be dead, and then the second module links its own. That is why the wrong hook shows in most iterations of a loop and not in all of them. The tests keep every module reachable and export a function from every source, because a module's functions are what keeps its executable alive after evaluation.import()inside an exported function of the second module also reached the first module's hook. The first test covers a top-levelimport()and one inside a function.lineOffset: 100on the first module andlineOffset: 0on the second, a stack trace in the second module reported line 100. The second test compares against a module with an identifier that was never seen, so it does not depend on the absolute line numbers (node:vm: apply SourceTextModule lineOffset/columnOffset like Node and name frames after the identifier #38235 changes those).delete require.cache[path], import it again. The namespaces are distinct and a tagged template yields the same template object for both, before and after the bump.autobuild-preview-pr-639-411bf2c6(debug + ASAN, Linux x64). The first version of the patch was also built from source through thedebug-localprofile.test/js/node/vm/vm.test.ts(304 pass, 0 fail),vm-script-fetcher-leak.test.ts(24 pass),sourcetextmodule-leak,sourcetextmodule-link-gc,vm-sourceUrl,happy-dom-vm-16277, and the 23test-vm-module-*/test-vm-source-map-urlfiles intest/js/node/test/parallel.script-leak.test.ts(avm.Scripttest) exceeds its 5 s timeout on debug builds with and without the bump.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