node:vm compatibility by heimskr · Pull Request #19703 · oven-sh/bun

heimskr · 2025-05-16T22:08:24Z

What does this PR do?

Adds support for vm.SourceTextModule and some other features in node:vm.

Documentation or TypeScript types
Code changes

How did you verify your code works?

Added Node.js tests

Jarred-Sumner · 2025-05-24T04:23:55Z


-static RefPtr<JSC::CachedBytecode> getBytecode(JSGlobalObject* globalObject, JSC::ProgramExecutable* executable, JSC::SourceCode source);
-static JSC::EncodedJSValue createCachedData(JSGlobalObject* globalObject, JSC::SourceCode source);
+bool extractCachedData(JSValue cachedDataValue, WTF::Vector<uint8_t>& outCachedData)


Can you add a TODO to investigate if we can avoid this clone?

Jarred-Sumner · 2025-05-24T04:25:10Z

-                auto _ = ERR::INVALID_ARG_TYPE(scope, globalObject, "options"_s, "object"_s, optionsArg);
-                return false;
-            }
+        SourceCode sourceCode(


Suggested change

SourceCode sourceCode(

RETURN_IF_EXCEPTION(scope, nullptr);

SourceCode sourceCode(

Jarred-Sumner · 2025-05-24T04:32:47Z

+        vm, JSC::PropertyName(JSC::Identifier::fromString(vm, "isModuleNamespaceObject"_s)),
+        JSC::JSFunction::create(vm, globalObject, 0, "isModuleNamespaceObject"_s, vmIsModuleNamespaceObject, ImplementationVisibility::Public), 1);
+    obj->putDirect(
+        vm, JSC::PropertyName(JSC::Identifier::fromString(vm, "kUnlinked"_s)),


Can we use a single const enum in the file for these values instead and then it'll participate in constant folding and avoid the dynamic property lookup?

Jarred-Sumner · 2025-05-24T04:34:16Z

-            lexicallyScopedFeatures, JSParserScriptMode::Classic, SourceParseMode::ProgramMode,
-            FunctionMode::None, SuperBinding::NotNeeded, ConstructorKind::None, DerivedContextType::None,
-            isEvalNode, EvalContextType::None, nullptr);
+        if (JSValue lineOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "lineOffset"_s))) {


Suggested change

if (JSValue lineOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "lineOffset"_s))) {

RETURN_IF_EXCEPTION(scope, false);

if (JSValue lineOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "lineOffset"_s))) {

Jarred-Sumner · 2025-05-24T04:34:28Z


-        program = parser.parse<ProgramNode>(error, name, ParsingContext::Normal);
-    }
+        if (JSValue columnOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "columnOffset"_s))) {


Suggested change

if (JSValue columnOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "columnOffset"_s))) {

RETURN_IF_EXCEPTION(scope, false);

if (JSValue columnOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "columnOffset"_s))) {

Jarred-Sumner · 2025-05-24T04:37:20Z

+        return NodeVMSourceTextModule::create(vm, globalObject, args);
+    }
+
+    if (disambiguator.inherits(JSArray::info())) {


Suggested change

if (disambiguator.inherits(JSArray::info())) {

if (JSC::isJSArray(disambiguator)) {

Jarred-Sumner · 2025-05-24T04:41:40Z

+
+    JSArray* array = constructEmptyArray(globalObject, nullptr, requests.size());
+
+    for (unsigned i = 0; const NodeVMModuleRequest& request : requests) {


I suggest constructing the array at the end of this, appending to a MarkedArgumentsBuffer, checking for exceptions and then passing the MarkedArgumentsBuffer to constructEmptyArray.

Suggested change

for (unsigned i = 0; const NodeVMModuleRequest& request : requests) {

MarkedArgumentsBuffer array;

for ( const NodeVMModuleRequest& request : requests) {

array.append(request.toJS(globalObject));

RETURN_IF_EXCEPTION(scope, {});

}

return JSValue::encode(constructEmptyArray(globalObject, array));

Jarred-Sumner · 2025-05-24T04:45:26Z

+
+        JSC::LexicallyScopedFeatures lexicallyScopedFeatures = globalObject->globalScopeExtension() ? JSC::TaintedByWithScopeLexicallyScopedFeature : JSC::NoLexicallyScopedFeatures;
+        JSC::SourceCodeKey key(source, {}, JSC::SourceCodeType::ProgramType, lexicallyScopedFeatures, JSC::JSParserScriptMode::Classic, JSC::DerivedContextType::None, JSC::EvalContextType::None, false, {}, std::nullopt);
+        Ref<JSC::CachedBytecode> cachedBytecode = JSC::CachedBytecode::create(std::span(cachedData), nullptr, {});


Suggested change

Ref<JSC::CachedBytecode> cachedBytecode = JSC::CachedBytecode::create(std::span(cachedData), nullptr, {});

// TODO: check if this clones the cached data an extra time

Ref<JSC::CachedBytecode> cachedBytecode = JSC::CachedBytecode::create(std::span(cachedData), nullptr, {});

Jarred-Sumner · 2025-05-24T04:47:05Z

+        ASSERT(m_cachedBytecode);
+
+        std::span<const uint8_t> bytes = m_cachedBytecode->span();
+        m_cachedBytecodeBuffer.set(vm(), this, WebCore::createBuffer(globalObject(), bytes));


this needs an exception scope technically

Suggested change

m_cachedBytecodeBuffer.set(vm(), this, WebCore::createBuffer(globalObject(), bytes));

m_cachedBytecodeBuffer.set(vm(), this, WebCore::createBuffer(globalObject(), bytes));

Jarred-Sumner · 2025-05-24T04:49:26Z

+    uint32_t lineOffset = lineOffsetValue.toUInt32(globalObject);
+    uint32_t columnOffset = columnOffsetValue.toUInt32(globalObject);
+
+    Ref<StringSourceProvider> sourceProvider = StringSourceProvider::create(sourceTextValue.toWTFString(globalObject), SourceOrigin {}, String {}, SourceTaintedOrigin::Untainted,


Does the SourceOrigin need to be set here too?

Suggested change

Ref<StringSourceProvider> sourceProvider = StringSourceProvider::create(sourceTextValue.toWTFString(globalObject), SourceOrigin {}, String {}, SourceTaintedOrigin::Untainted,

auto&& sourceCodeString = sourceTextValue.toWTFString(globalObject);

RETURN_IF_EXCEPTION(scope, nullptr);

Ref<StringSourceProvider> sourceProvider = StringSourceProvider::create(sourceCodeString, SourceOrigin {}, String {}, SourceTaintedOrigin::Untainted,

Jarred-Sumner · 2025-05-24T04:51:37Z

+        return constructEmptyArray(globalObject, nullptr, 0);
+    }
+
+    JSArray* requestsArray = constructEmptyArray(globalObject, nullptr, requests.size());


Suggested change

JSArray* requestsArray = constructEmptyArray(globalObject, nullptr, requests.size());

MarkedArgumentsBuffer requestsArray;

Jarred-Sumner · 2025-05-24T04:51:52Z

+
+        requestObject->putDirect(vm, attributesIdentifier, attributesObject);
+        addModuleRequest({ WTF::String(*request.m_specifier), WTFMove(attributeMap) });
+        requestsArray->putDirectIndex(globalObject, i, requestObject);


Suggested change

requestsArray->putDirectIndex(globalObject, i, requestObject);

requestsArray.append(requestObject);

Jarred-Sumner · 2025-05-24T04:52:34Z

+        requestsArray->putDirectIndex(globalObject, i, requestObject);
+    }
+
+    m_moduleRequestsArray.set(vm, this, requestsArray);


Suggested change

m_moduleRequestsArray.set(vm, this, requestsArray);

auto* jsArray = JSC::constructArray(globalObject, requestsArray))

m_moduleRequestsArray.set(vm, this, jsArray);

return jsArray;

Jarred-Sumner · 2025-05-24T04:54:57Z

+    if (!m_cachedBytecodeBuffer) {
+        RefPtr<CachedBytecode> cachedBytecode = bytecode(globalObject);
+        std::span<const uint8_t> bytes = cachedBytecode->span();
+        m_cachedBytecodeBuffer.set(vm(), this, WebCore::createBuffer(globalObject, bytes));


technically needs an exception check if bytes > 4.7 GB

Jarred-Sumner · 2025-05-24T04:57:42Z

+    WriteBarrier<ModuleProgramExecutable> m_cachedExecutable;
+    WriteBarrier<JSUint8Array> m_cachedBytecodeBuffer;
+    WriteBarrier<Exception> m_evaluationException;
+    RefPtr<CachedBytecode> m_bytecode;


Suggested change

RefPtr<CachedBytecode> m_bytecode;

// TODO: investigate if we're cloning the source code and the bytecode multiple times and figure out how to not do that

RefPtr<CachedBytecode> m_bytecode;

Jarred-Sumner · 2025-05-24T05:02:05Z

+    std::atomic_bool m_installed = false;
+    std::atomic_flag m_waiting {};
+    Semaphore m_semaphore;
+    std::mutex m_globalObjectsMutex;


Suggested change

std::mutex m_globalObjectsMutex;

WTF::Lock m_globalObjectsMutex;

Jarred-Sumner · 2025-05-24T05:06:49Z

+
+    // Iterates the module requests and links with the linker.
+    // Specifiers should be aligned with the moduleRequests array in order.
+    const specifiers = Array(moduleRequests.length);


Suggested change

const specifiers = Array(moduleRequests.length);

const moduleRequestsCount = moduleRequests.length;

const specifiers = Array(moduleRequestsCount);

Jarred-Sumner · 2025-05-24T05:06:56Z

+    const specifiers = Array(moduleRequests.length);
+    const modulePromises = Array(moduleRequests.length);
+    // Iterates with index to avoid calling into userspace with `Symbol.iterator`.
+    for (let idx = 0; idx < moduleRequests.length; idx++) {


Suggested change

for (let idx = 0; idx < moduleRequests.length; idx++) {

for (let idx = 0; idx < moduleRequestsCount; idx++) {

Jarred-Sumner · 2025-05-24T05:07:06Z

+    // Iterates the module requests and links with the linker.
+    // Specifiers should be aligned with the moduleRequests array in order.
+    const specifiers = Array(moduleRequests.length);
+    const modulePromises = Array(moduleRequests.length);


Suggested change

const modulePromises = Array(moduleRequests.length);

const modulePromises = Array(moduleRequestsCount);

Jarred-Sumner

A few small nitpicky comments but very close!

…d::mutex and std::unique_lock

…itions (#38228) ### Problem - `new vm.Script('1', { lineOffset: 2147483647 })` and `vm.compileFunction('return 1', [], { lineOffset: 2147483647 })` abort on assertion-enabled builds with `ASSERTION FAILED: line >= 0` in `JSC::JSTextPosition::checkConsistency()` (ParserTokens.h:231). Release builds survive and print wrapped line numbers (`big.js:-2147483648`). - `lineOffset: 2147483646` plus a two-line source, and `new vm.SourceTextModule('1;\n2;', { lineOffset: 2147483647 })`, abort the same way. - Node's validator accepts any int32 for `lineOffset` / `columnOffset`, and the bindings hand the value to JSC unchanged (`NodeVMScript.cpp` `makeSource(...)`, `NodeVM.cpp` `constructAnonymousFunction`, `NodeVMSourceTextModule.cpp` `create`). JSC stores positions as `int`: `OrdinalNumber::oneBasedInt()` is `value + 1`, and the lexer does `++m_lineNumber` per line terminator starting from that (`Lexer.cpp` `setCode` / `shiftLineTerminator`), so any offset within (source line count + 1) of `INT32_MAX` overflows. The `+ 1` overflow is undefined behaviour, which is why a debug build and the optimized asan build fail on different inputs. ### Fix - Adds `clampOffsetForSource(offset, sourceLength)` (`NodeVM.h` / `NodeVM.cpp`) and applies it to both offsets in `vm.Script`, `vm.compileFunction` and `SourceTextModule` before the `SourceCode` is built: the offset is capped at `INT32_MAX - 1 - sourceLength`. - Correct because a source cannot hold more line terminators, or a longer first line, than it has code units, so every position JSC derives (`+1` for one-based, per-line increments, first-line column additions, and `decorateParseErrorStack`'s own `line + offset`) stays within `int`. For any realistic offset the clamp is a no-op; an absurd one now reports positions just below `INT32_MAX` instead of undefined behaviour (e.g. `big.js:2147483627` for the 20-character repro). - `compileFunction` now builds its wrapper program before the standalone parse of the body and clamps against the wrapper's length, since that is the longest text it parses (the body is re-parsed as line 2 of it). The early `RETURN_IF_EXCEPTION` after building the wrapper replaces continuing with a null string. - The options struct itself is clamped, so `decorateParseErrorStack` and the provider's start position see the same value as the parser. `SourceTextModule` still passes its (zero-based) values to `SourceCode` exactly as before; the off-by-one there is fixed separately in #38235, which touches the same lines. Whichever of the two lands second only has to wrap that PR's `TextPosition` operands in `clampOffsetForSource`; the module test case here uses `INT32_MAX - 1` with a three-line source so it fails without the clamp under either line-numbering. - Removes the native `runInNewContext` / `runInThisContext` host functions from `NodeVM.cpp` (second commit), plus the `BaseVMOptions` constructor only they used. `vm.ts` has built both APIs on `Script` since #19703, so they were unreachable, and they were the only remaining places that built a `SourceCode` from unclamped offsets. - Verified with `test/js/node/vm/vm.test.ts` (`describe("node:vm lineOffset/columnOffset at the edge of int32")`): 11 subprocess cases covering construction of all three APIs and the reported line/column of runtime and compile-time errors. Without the `src/` change 9 of the 11 fail on the debug build (7 abort with the assertion above; the line/column cases otherwise report `1` / `16`, i.e. the offset silently lost); of the remaining 2, the one-line `Script` case (the reported repro) only fails on optimized assertion builds, and the `columnOffset` construction case never aborted (the column value itself is checked separately and did fail). With the change all 11 pass. - Also ran the rest of `vm.test.ts` (225 pass), `vm-sourceUrl.test.ts`, and all 95 `test/js/node/test/parallel/test-vm-*.js` files (covers `runInNewContext` / `runInThisContext` after the removal); normal offsets (`lineOffset: 5`, negative offsets, `columnOffset: 10`) report the same lines and columns as before. ### Background - `lineOffset` / `columnOffset` tell `node:vm` where a snippet sits inside a larger file so error positions line up with that file; they are zero-based and may be negative. Node validates them as int32 and otherwise passes them through, so `2147483647` is a legal input. - `OrdinalNumber` (WTF) wraps an `int` that can be read as zero- or one-based; `TextPosition` is a line/column pair of them. A JSC `SourceCode` carries the one-based first line and column to start counting from, and the lexer counts lines upward from there in a plain `int`. - `JSTextPosition::checkConsistency()` is a debug-only assertion that every token position the lexer produces has a non-negative line; it is what turns the overflow into an abort on assertion builds. - `vm.compileFunction` is implemented by wrapping the body in `(function (params) {\n<body>\n})` and compiling that as a program; the user's body is therefore line 2 of what JSC actually parses.  --- **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

heimskr added 30 commits April 28, 2025 16:26

Implement createCachedData

2c9f4b6

Produce a proper Buffer

b0b7b85

Fix wrong SourceCodeType

6f41459

Add support for produceCachedData and cachedDataProduced

3263005

Some progress on supporting the cachedData option in node:vm

737b04e

Add cachedData support in node:vm (test-vm-cached-data.js)

9fa3989

Add test-vm-createcacheddata.js

0e35e73

Add test for createCachedData

0708bae

Merge branch 'main' into kai/vm-compat-2

618c7c3

Update WebKit version

20769c4

Merge branch 'main' into kai/vm-compat-2

bae2771

Address feedback

fed7962

More cachedData tests

e58a36f

You can never have too many tests

cb482b3

Address feedback

8009728

Address feedback again

fe6c6ef

Check for parser errors too

6b31f98

Add some RETURN_IF_EXCEPTIONs

0bc2397

Add some more RETURN_IF_EXCEPTIONs

cbd5e1b

Adjust lifetimes and fix incorrect use of ParserError::isValid()

6635571

Initial work on vm.Module

32a094d

Add a null check

10d14a3

Merge branch 'kai/vm-compat-2' into kai/vm-compat-modules

2c1888d

Some more work on vm.Module

bc1654b

yay, it links

62dfa74

A couple slight changes

a8cae28

getModuleRequests

a58c2a9

Implement validateInternalField

c787145

Custom inspection for vm.Module

a0384f6

Reorganize node:vm native code

c59f7ad

Jarred-Sumner reviewed May 24, 2025

View reviewed changes

Comment thread src/vm/SigintWatcher.h Outdated

Jarred-Sumner reviewed May 24, 2025

View reviewed changes

Comment thread src/vm/SigintWatcher.h Outdated

Jarred-Sumner reviewed May 24, 2025

View reviewed changes

heimskr added 2 commits May 23, 2025 22:19

Use WTF::Thread, WTF::Lock and WTF::Locker instead of std::thread, st…

dbdc63e

…d::mutex and std::unique_lock

Remove some unused includes

76a3ed5

Jarred-Sumner merged commit 392212b into main May 24, 2025

Jarred-Sumner deleted the kai/vm-compat-modules branch May 24, 2025 06:00

Jarred-Sumner mentioned this pull request Sep 2, 2025

node:vm Module is not yet implemented in Bun. #9167

Closed

alii mentioned this pull request Jul 9, 2026

node:repl: replace the stub with Node v26.3.0's REPL — v26 readline stack, acorn recoverable-parse + top-level await, completion, history, --interactive (82 vendored upstream tests) #31827

Merged

robobun mentioned this pull request Aug 13, 2026

node:vm: keep lineOffset/columnOffset from overflowing JSC parser positions #38228

Merged

	SourceCode sourceCode(
	RETURN_IF_EXCEPTION(scope, nullptr);
	SourceCode sourceCode(

	if (JSValue lineOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "lineOffset"_s))) {
	RETURN_IF_EXCEPTION(scope, false);
	if (JSValue lineOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "lineOffset"_s))) {

	if (JSValue columnOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "columnOffset"_s))) {
	RETURN_IF_EXCEPTION(scope, false);
	if (JSValue columnOffsetOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "columnOffset"_s))) {

	if (disambiguator.inherits(JSArray::info())) {
	if (JSC::isJSArray(disambiguator)) {


		JSArray* array = constructEmptyArray(globalObject, nullptr, requests.size());

		for (unsigned i = 0; const NodeVMModuleRequest& request : requests) {

-    for (unsigned i = 0; const NodeVMModuleRequest& request : requests) {
+    MarkedArgumentsBuffer array;
+    for ( const NodeVMModuleRequest& request : requests) {
+          array.append(request.toJS(globalObject));
+          RETURN_IF_EXCEPTION(scope, {});
+     }
+     return JSValue::encode(constructEmptyArray(globalObject, array));

	Ref<JSC::CachedBytecode> cachedBytecode = JSC::CachedBytecode::create(std::span(cachedData), nullptr, {});
	// TODO: check if this clones the cached data an extra time
	Ref<JSC::CachedBytecode> cachedBytecode = JSC::CachedBytecode::create(std::span(cachedData), nullptr, {});

	m_cachedBytecodeBuffer.set(vm(), this, WebCore::createBuffer(globalObject(), bytes));
	m_cachedBytecodeBuffer.set(vm(), this, WebCore::createBuffer(globalObject(), bytes));

-    Ref<StringSourceProvider> sourceProvider = StringSourceProvider::create(sourceTextValue.toWTFString(globalObject), SourceOrigin {}, String {}, SourceTaintedOrigin::Untainted,
+    auto&& sourceCodeString = sourceTextValue.toWTFString(globalObject);
+    RETURN_IF_EXCEPTION(scope, nullptr);
+    Ref<StringSourceProvider> sourceProvider = StringSourceProvider::create(sourceCodeString, SourceOrigin {}, String {}, SourceTaintedOrigin::Untainted,

	JSArray* requestsArray = constructEmptyArray(globalObject, nullptr, requests.size());
	MarkedArgumentsBuffer requestsArray;

	requestsArray->putDirectIndex(globalObject, i, requestObject);
	requestsArray.append(requestObject);

-    m_moduleRequestsArray.set(vm, this, requestsArray);
+    auto* jsArray = JSC::constructArray(globalObject, requestsArray))
+    m_moduleRequestsArray.set(vm, this, jsArray);
+    return jsArray;

	RefPtr<CachedBytecode> m_bytecode;
	// TODO: investigate if we're cloning the source code and the bytecode multiple times and figure out how to not do that
	RefPtr<CachedBytecode> m_bytecode;

	std::mutex m_globalObjectsMutex;
	WTF::Lock m_globalObjectsMutex;

	const specifiers = Array(moduleRequests.length);
	const moduleRequestsCount = moduleRequests.length;
	const specifiers = Array(moduleRequestsCount);

	for (let idx = 0; idx < moduleRequests.length; idx++) {
	for (let idx = 0; idx < moduleRequestsCount; idx++) {

	const modulePromises = Array(moduleRequests.length);
	const modulePromises = Array(moduleRequestsCount);

Conversation

heimskr commented May 16, 2025

What does this PR do?

How did you verify your code works?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jarred-Sumner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants