Conversation
…e links SyntheticModuleRecord::materializeLazyExport() reads a lazy export off its source object when something first binds to it. For a named import that is during link(), in CyclicModuleRecord::initializeEnvironment() and in WebAssemblyModuleRecord::initializeImports(). User code can redefine the property as an accessor after the record is created. The getter then ran inside link(). A getter that loads an ES module starts a second link() there. The second link() takes the records of the first one, which are LINKING, for linked. materializeLazyExport() now looks the property up first. With UserDefinedGetter::Skip, the default, it does not call a getter that is not a host function or a builtin function, and the export is undefined. A read through a module namespace object is not part of a link(), so it passes UserDefinedGetter::Call and behaves as before.
|
Preview build of de7c4b0: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp— Users who mock a lazy builtin property with an accessor (e.g. jest.spyOn(process, 'x', 'get') or sinon .get()) now get a silent, permanentundefinedfrom every later named import instead of the mock's value. In Skip mode the skipped getter leaves value as jsUndefined() (line 266) and line 287 stores it into the module environment, so the later namespace read at JSModuleNamespaceObject.cpp:201 sees a non-empty value and never calls the getter even with UserDefinedGetter::Call. Fix: do not commitundefinedfor a skipped user getter; either leave the binding empty and read it after link() returns, or report a clear link error, so a mocked accessor is never silently flattened to undefined. [also at: Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp:282 - A namespace read of a lazy export whose user getter imports a module that names the same export returnsundefinedinstead of the getter's result, where the base returned the getter's value.]Why this was flagged
A test file does jest.spyOn(process, 'title', 'get').mockReturnValue('x') after node:process has loaded, then imports an ES module containing
import { title } from "node:process"orimport * as p from "node:process". During link, CyclicModuleRecord.cpp:327 calls materializeLazyExport with the default Skip. At SyntheticModuleRecord.cpp:269-271 the getter is a plain JSFunction, so skipGetter is true and value stays jsUndefined() from line 266. Line 287 writes undefined into the environment slot via symbolTablePutTouchWatchpointSet. The named import then reads undefined. A later p.title read goes through JSModuleNamespaceObject.cpp:201, whereif (!value)is false because undefined is a non-empty JSValue, so the Call-mode materialize at line 204 is never reached and the getter never runs. On the base branch source->get() at the old line 261 ran the getter and the import observed 'x'. No error or warning is produced, so the mock silently stops working with no indication why.Verification: SyntheticModuleRecord.cpp line 266
JSValue value = jsUndefined();; lines 269-271 setskipGetter = true, and line 287 commitsundefinedinto the module environment. JSModuleNamespaceObject.cpp:198-201 sees a non-emptyundefinedand skips the Call-mode materialization. On the base branch this path didsource->get(globalObject, localName)and returned the mock's value. -
🟡
Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp— A WebAssembly ES module that imports a Bun lazy export which user code has redefined as an accessor now getsundefinedand fails to instantiate, where the base called the getter. The call at WebAssemblyModuleRecord.cpp:235 takes the new Skip default, but initializeImports with CreationMode::FromModuleLoader never runs inside link(): CyclicModuleRecord.cpp:663-664 and AbstractModuleRecord.cpp:1526 call it from execute()/evaluate(), after linking is done. The PR text says this caller runs inside a link(); the code shows it does not. Fix: pass UserDefinedGetter::Call at every materializeLazyExport site that runs outside the link stack, including WebAssemblyModuleRecord.cpp:235, so Skip stays confined to CyclicModuleRecord::initializeEnvironment. [also at: Source/JavaScriptCore/runtime/SyntheticModuleRecord.h:95 - A WebAssembly ES module importing a lazy export that user code redefined as an accessor now getsundefined(a LinkError for a function import) instead of the getter's value, although that read is not inside link().]Why this was flagged
Bun loads a .wasm file through the module loader; the wasm module has an import such as (import "node:process" "title" (global externref)) or a function import from a lazy builtin, and before that user code did Object.defineProperty(process, 'title', { configurable: true, get() { return 'x'; } }) after node:process loaded. The wasm record is linked by CyclicModuleRecord::initializeEnvironment at CyclicModuleRecord.cpp:428 via WebAssemblyModuleRecord::link at WebAssemblyModuleRecord.cpp:110, which only creates the environment. Imports are read later from CyclicModuleRecord::execute at CyclicModuleRecord.cpp:663-664, i.e. during evaluation. WebAssemblyModuleRecord.cpp:235 calls materializeLazyExport without a mode, so SyntheticModuleRecord.cpp:269-271 skips the user getter and SyntheticModuleRecord.cpp:287 stores undefined. The snapshot at WebAssemblyModuleRecord.cpp:254 reads undefined and the import type check fails with a LinkError, and the undefined stays cached for every later import. On the base branch source->get() ran the getter and the wasm import received its value.
Verification: WebAssemblyModuleRecord.cpp:235 calls materializeLazyExport with no mode argument, so it takes the
UserDefinedGetter::Skipdefault (SyntheticModuleRecord.h:95) and caches undefined. The base didsource->get(globalObject, localName), which invoked the getter. FromModuleLoader callers such asCyclicModuleRecord::execute(CyclicModuleRecord.cpp:663-664) run after linking.
| if (userDefinedGetter == UserDefinedGetter::Skip && slot.isAccessor()) { | ||
| auto* getter = dynamicDowncast<JSFunction>(slot.getterSetter()->getter()); |
There was a problem hiding this comment.
🟡 (optional) Named imports of a lazy export whose getter is a bound host function or a non-JSFunction callable now bind undefined, where the base called the getter. SyntheticModuleRecord.cpp:269 downcasts the getter to JSFunction; a JSBoundFunction fails isNonBoundHostFunction() and any InternalFunction or callable object fails the downcast, so line 270 sets skipGetter even when no user code would run. Fix: classify by what the getter would execute, not by its type; treat a bound function whose target is host/builtin and any InternalFunction (other than user-reachable Proxy targets) as safe, or only skip getters whose executable is user script.
Why this was flagged
Trigger: user or library code installs an accessor on a lazy builtin property with get: hostFn.bind(obj) (e.g. get: process.cwd.bind(process)) or with an InternalFunction. A named import import { x } from "node:process" links via CyclicModuleRecord.cpp:327 with the Skip default. At SyntheticModuleRecord.cpp:269 dynamicDowncast yields a JSBoundFunction, whose isNonBoundHostFunction() is false, or yields null for an InternalFunction; line 270 sets skipGetter and line 287 stores jsUndefined() permanently, so the import and every later namespace read return undefined. On the base, source->get() ran the bound host function and stored its result. A bound host function runs no user script and could not start a link(); skipping it is a behavior regression without the safety benefit. Remedy: unwrap JSBoundFunction to its target (boundFunction->targetFunction()) before classifying, and treat InternalFunction getters as host.
Verification: With a JSBoundFunction or InternalFunction getter on a lazy builtin property bound by a named import (CyclicModuleRecord.cpp:327, Skip default), at SyntheticModuleRecord.cpp:269-270 the bound function fails isNonBoundHostFunction() and isBuiltinFunction() and the InternalFunction downcast yields null; skipGetter is true, value stays jsUndefined() and is stored at line 287, where the base called the getter.
|
|
||
| JSValue value = source->get(globalObject, localName); | ||
| PropertySlot slot(source, PropertySlot::InternalMethodType::Get); | ||
| bool hasProperty = source->getPropertySlot(globalObject, localName, slot); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: A Proxy on the lazy-exports source or its prototype chain still runs user code inside link(), so the crash this change targets remains reachable. The getPropertySlot call at SyntheticModuleRecord.cpp:263 runs before the accessor check at line 269, and with InternalMethodType::Get a ProxyObject dispatches straight to performGet (ProxyObject.cpp:465-466), invoking the user's get trap. Fix: in Skip mode, look the property up without running any user-controlled hook, e.g. walk the chain with VMInquiry and treat a ProxyObject (or any non-ordinary object) in the chain as user-defined and skip it, while still honouring native custom getters that the declaring code installed. [also at: Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp:275 - pre-existing: the link-time crash this change targets is still reachable, because the skip test looks only at the getter's identity, not at what it runs.; Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp:274 - Bun can still run user code and crash inside link() through a host or builtin getter that dispatches on this, so the guard this change adds is incomplete.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
User code does Object.setPrototypeOf(process, new Proxy({}, { get() { require("./Q.mjs"); } })) after node:process loaded, with a lazy property such as title not present as an own property, then an ES module with import { title } from "node:process" links. CyclicModuleRecord.cpp:327 calls materializeLazyExport with the default UserDefinedGetter::Skip. SyntheticModuleRecord.cpp:263 calls source->getPropertySlot with InternalMethodType::Get; the chain walk reaches the ProxyObject and ProxyObject.cpp:465-466 routes to performGet, which calls the user's get trap. The trap's require() starts a nested link() while the outer records are LINKING, the state that fails ASSERTION FAILED: (status == Status::Linking) == stack.contains(requiredModule) on debug and segfaults on release. The new guard at line 269 only inspects slot.isAccessor() after the lookup has already run the trap, so it does not prevent this; the base branch ran the trap too via source->get(), so this is a path the fix leaves failing as before.
Verification: Pre-existing: the base's source->get() already ran a Proxy trap on this path, and the change closes only the accessor case, so merging leaves this route exactly as broken as before. SyntheticModuleRecord.cpp:262-263 calls source->getPropertySlot in Get mode before the accessor check at line 269; ProxyObject.cpp:465-466 routes Get to performGet, which invokes the user's get trap.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 1 remain after this review. WalkthroughSynthetic module lazy-export materialization now accepts a getter policy. The default policy skips user-defined accessor getters, while JavaScript module namespace reads pass ChangesLazy export materialization
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No confirmed issue blocks merging. The accessor-slot safety question remains unresolved. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a clear problem statement, fix, behavior change, testing details, and reproduction. However, it omits required template information, including the Bugzilla bug title and URL, the reviewed-by line, and the changed file/function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
A lazy export does not call a getter that user code defined while a module links. The CommonJS change that follows declares lazy exports for user modules and needs that.
1 participant
Needed by a Bun change on the branch
robobun/ea2b5b58/cjs-static-table-lazy-exports.Problem
link(). If the getter loads an ES module, Bun crashes:ASSERTION FAILED: (status == Status::Linking) == stack.contains(requiredModule)on a debug build,Segmentation fault at address 0x0on 1.4.3 canaryf4d755a9c.materializeLazyExport(runtime/SyntheticModuleRecord.cpp:261) callssource->get(). User code can redefine a lazy property as an accessor before anything binds to it.Fix
materializeLazyExportlooks the property up first. By default it does not call a getter that is not a host or builtin function. The export is thenundefined.link(). It passesUserDefinedGetter::Call, as before.fb1167ebf2cband pass here. Thejscshell cannot declare a lazy export.Background
SyntheticModuleRecordexport declared without a value. Bun uses it fornode:process,node:module,bunand itssrc/jsbuiltins.link()inside another one treats the outer LINKING records as linked.link()returns. It keeps the value, but needs a pending list and a rule for a getter that throws.Downsides
undefined.isAccessor()test per lazy bind. Other imports return athasLazyExports()first.Notes
Repro on Bun 1.4.3 canary
f4d755a9c(bun main.mjs):The getter's
require("./Q.mjs")links Q, Y and X while E links. X imports E, which is LINKING on the outer stack, so X is marked LINKED on its own. Y throws, the getter throws, the link of E fails and E is UNLINKED again.import("./X.mjs")then evaluates X with an unlinked E.oven-sh/bun#39812 closed this for an accessor that exists when the builtin loads: Bun reads it then and does not declare it lazy. This change closes it for an accessor that user code defines after the load.
The WebAssembly caller (
WebAssemblyModuleRecord::initializeImports) runs inside alink()too and takes the default.Checked locally with a debug ASAN build of this branch:
test/js/bun/resolve/builtin-esm-lazy-exports.test.tsof the Bun change, 34 of 34 pass.