Skip to content

SyntheticModuleRecord: support a live exports source for namespace reads #380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 5 commits into
base: main
Choose a base branch
from
Open

SyntheticModuleRecord: support a live exports source for namespace reads #380

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a59646c
SyntheticModuleRecord: support a live exports source for namespace reads
robobun Aug 1, 2026
1ee717b
overrideExportValue: write through to the live-exports source
robobun Aug 1, 2026
3cf0ca7
Fall back to the env slot when the live source lacks the property
robobun Aug 1, 2026
abed2db
Invalidate the module-namespace IC when a live-exports source is inst…
robobun Aug 1, 2026
1b4afe5
setLiveExportsSource: accept null and fire the watchpoint once
robobun Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
#include "JSCInlines.h"
#include "JSModuleEnvironment.h"
#include "JSModuleRecord.h"
#if USE(BUN_JSC_ADDITIONS)
#include "SyntheticModuleRecord.h"
#endif

namespace JSC {

Expand Down Expand Up @@ -195,6 +198,25 @@
return false;
}

#if USE(BUN_JSC_ADDITIONS)
// Bun's mock.module / loader:"object" may back a synthetic module with the
// factory-returned object so that accessor exports stay live. The module
// environment slots still hold the first-read snapshot for static imports
// (which read slots directly), but dynamic import namespace access
// re-evaluates through the source object on every read. Returning a plain
// uncacheable value here keeps the JIT's module-namespace IC (which would
// inline the raw slot) from being installed.
if (auto* synthetic = dynamicDowncast<SyntheticModuleRecord>(exportEntry.moduleRecord.get())) {
if (JSObject* source = synthetic->liveExportsSource()) [[unlikely]] {
slot.disableCaching();
JSValue liveValue = source->get(globalObject, propertyName);
RETURN_IF_EXCEPTION(scope, false);
slot.setValue(this, static_cast<unsigned>(PropertyAttribute::DontDelete), liveValue);
return true;
}
Comment on lines +213 to +225

@claude claude Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The live-source lookup uses propertyName (the export name on this namespace) instead of exportEntry.localName (the binding name in the target synthetic module). When another module does export { foo as bar } from './mocked', propertyName is bar but the source object only has foo, so the read returns undefined — a regression vs. the environment-slot fallback. Additionally, this branch must be skipped when exportEntry.localName == vm.propertyNames->starNamespacePrivateName: for export * as X from './mocked' the resolved local name is the private star-namespace symbol, which the user's factory object cannot have, so nsB.X also becomes undefined instead of the mocked module's namespace object.

Extended reasoning...

What the bug is

m_exports on a JSModuleNamespaceObject maps this namespace's export names to ExportEntry { localName, moduleRecord }, where moduleRecord / localName are the resolved binding after following re-export chains (see the constructor, which stores resolution.localName and resolution.moduleRecord). The new live-source branch reads the source object with propertyName — the key on this namespace — instead of exportEntry.localName — the key in the target synthetic module's environment (and thus on its backing object).

For direct access on the synthetic module's own namespace these happen to coincide, because tryCreateWithExportNamesAndValues calls addExportEntry(ExportEntry::createLocal(exportName, exportName)). They diverge as soon as another module re-exports from the mocked module.

Step-by-step: renamed re-export

  1. ./mocked is a SyntheticModuleRecord created via mock.module(id, () => ({ get foo() { ... } })) with a liveExportsSource that has a foo accessor.
  2. Module B contains export { foo as bar } from './mocked'.
  3. resolveExportImpl walks the Indirect entry on B, enqueues (mockedRecord, 'foo'), and resolves at the synthetic module's Local entry to Resolution{ Resolved, moduleRecord: mockedSyntheticRecord, localName: 'foo' }.
  4. B's namespace stores m_exports['bar'] = { localName: 'foo', moduleRecord: mockedSyntheticRecord }.
  5. Reading nsB.bar enters getOwnPropertySlotCommon with propertyName == 'bar' and exportEntry.localName == 'foo'.
  6. dynamicDowncast<SyntheticModuleRecord>(exportEntry.moduleRecord.get()) succeeds, liveExportsSource() is non-null, and the code executes source->get(globalObject, propertyName) → source.bar → undefined.

Before this PR, step 6 fell through to setValueModuleNamespace with the environment slot value (the snapshot of foo), so this is a regression, not merely an incomplete feature. Note that the immediately preceding line already uses the correct key: getValue(environment, exportEntry.localName, scopeOffset).

Step-by-step: export * as X

  1. Module B contains export * as X from './mocked'.
  2. resolveExport / getModuleNamespace produce Resolution{ Resolved, moduleRecord: mockedSyntheticRecord, localName: starNamespacePrivateName } (AbstractModuleRecord.cpp handling for star-namespace bindings).
  3. B's namespace stores m_exports['X'] = { localName: starNamespacePrivateName, moduleRecord: mockedSyntheticRecord }.
  4. Reading nsB.X: the earlier if (exportEntry.localName == starNamespacePrivateName) block materializes the namespace, and getValue(environment, starNamespacePrivateName, ...) correctly fetches the mocked module's JSModuleNamespaceObject from the slot.
  5. But then the live-source branch matches (exportEntry.moduleRecord is the synthetic record with a live source) and calls source->get(globalObject, 'X') on the user's plain factory object → undefined.

Fixing case 1 by switching to exportEntry.localName does not fix case 2: it would call source->get(starNamespacePrivateName), a private symbol the user object cannot have, still yielding undefined.

Why nothing prevents it

The branch is guarded only by dynamicDowncast<SyntheticModuleRecord>(exportEntry.moduleRecord) and liveExportsSource(). It does not check whether this namespace is the synthetic module's own namespace, nor whether the resolved local name is the star-namespace sentinel. Because exportEntry.moduleRecord is the resolved target record (not m_moduleRecord), any namespace whose export chain terminates at the mocked module hits this path.

Impact

Both cases return undefined where the pre-PR code returned the correct value (snapshot for the rename case, the namespace object for the star case). This is user-visible incorrect behavior for anyone who re-exports from a mock.module'd module — a common pattern when mocking a package that is itself re-exported through a barrel file.

Suggested fix

if (auto* synthetic = dynamicDowncast<SyntheticModuleRecord>(exportEntry.moduleRecord.get())) {
    if (JSObject* source = synthetic->liveExportsSource();
        source && exportEntry.localName != vm.propertyNames->starNamespacePrivateName) [[unlikely]] {
        slot.disableCaching();
        JSValue liveValue = source->get(globalObject, exportEntry.localName);
        RETURN_IF_EXCEPTION(scope, false);
        slot.setValue(this, static_cast<unsigned>(PropertyAttribute::DontDelete), liveValue);
        return true;
    }
}

i.e. (a) look up by exportEntry.localName, and (b) skip the branch entirely for the star-namespace sentinel so it falls through to setValueModuleNamespace with the already-correct slot value.

@claude claude Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hasOwn fallback in 3cf0ca7 removes the undefined regression for both the renamed-re-export and export * as X cases (the source lacks bar/X, so we drop through to the env slot), but the lookup key is still propertyName rather than exportEntry.localName, so the underlying wrong-key issue remains:

  • Live behavior is lost through a rename. export { foo as bar } from './mocked' → nsB.bar misses on source.bar and returns the env-slot snapshot, while nsMocked.foo returns the live value. The feature silently stops working across a barrel/re-export.
  • Wrong value when the outer name collides. If the mocked module also exports bar, the source does own bar, so hasOwn is true and nsB.bar returns source.bar — but the resolved binding is foo. That's a correctness bug, not just a stale snapshot.

Switching both getOwnPropertySlot and getValue calls to exportEntry.localName fixes both. With that change the hasOwn fallback also happens to cover the star-namespace case (the source can't own starNamespacePrivateName), so the explicit sentinel guard from the original suggestion becomes optional — though keeping it saves a pointless method-table dispatch on every export * as X read.

}
#endif

slot.setValueModuleNamespace(this, static_cast<unsigned>(PropertyAttribute::DontDelete), value, environment, scopeOffset);
return true;
}
Expand Down Expand Up @@ -452,6 +474,17 @@
putResult = moduleNamespaceObject->put(moduleNamespaceObject, globalObject, name, value, putter);
RETURN_IF_EXCEPTION(scope, {});
moduleNamespaceObject->m_isOverridingValue = false;

// Keep the live-exports backing object (if any) consistent with the env
// slot so spyOn / re-mock writes are observed by namespace reads that
// forward through it.
if (auto* synthetic = dynamicDowncast<SyntheticModuleRecord>(record)) {
if (JSObject* source = synthetic->liveExportsSource()) {
source->putDirect(vm, name, value, 0);
RETURN_IF_EXCEPTION(scope, {});
}
}

Check failure on line 486 in Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp

View check run for this annotation

Claude / Claude Code Review

overrideExportValue live-source write-through: wrong key and wrong primitive 

The write-through uses `name` (the export name on *this* namespace) instead of `resolution.localName` (the binding name in the target synthetic module — the same key the env-slot write on line 470 already uses). With `export { foo as bar } from './mocked'`, an override on B's namespace with `name='bar'` writes a stray `source.bar` while `source.foo` stays stale, so once the read side is fixed to use `localName` the override is silently lost for namespace reads. Separately, `putDirect` is a fragi
Comment on lines +495 to +500

@claude claude Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The write-through uses name (the export name on this namespace) instead of resolution.localName (the binding name in the target synthetic module — the same key the env-slot write on line 470 already uses). With export { foo as bar } from './mocked', an override on B's namespace with name='bar' writes a stray source.bar while source.foo stays stale, so once the read side is fixed to use localName the override is silently lost for namespace reads. Separately, putDirect is a fragile primitive for a user-supplied backing object: it asserts !parseIndex(propertyName) (numeric export names like "0" trip a debug ASSERT and desync named/indexed storage in release), it overwrites an accessor's GetterSetter in place and transitions it to a plain data property (destroying the live getter this feature exists to preserve), and it is non-virtual so a Proxy source's traps are bypassed and the write is invisible to the read side's source->get() — consider a method-table put (or skip the write-through when the existing own descriptor is an accessor) and handle the index case.

Extended reasoning...

What the bug is

overrideExportValue follows re-export chains via resolveExport, so record is resolution.moduleRecord — potentially a different module than the one whose namespace was passed in — and the binding key inside that target module is resolution.localName. The env-slot write on line 470 gets this right:

symbolTablePutTouchWatchpointSet(moduleEnvironment, globalObject, resolution.localName, value, ...);

but the new live-source write-through on line 483 uses name — the export name on this namespace — instead:

source->putDirect(vm, name, value, 0);

The source object's keys are the synthetic module's own export names (its export entries are createLocal(exportName, exportName), so localName == exportName == the property key on the factory object). When name != resolution.localName, the write lands on a key the read side never looks at.

Step-by-step: renamed re-export

  1. ./mocked is a SyntheticModuleRecord created via mock.module(id, () => ({ get foo() { ... } })) with a liveExportsSource that has foo.
  2. Module B contains export { foo as bar } from './mocked'.
  3. spyOn(nsB, 'bar') (or any Bun-side caller) invokes overrideExportValue(globalObject, 'bar', spy) on B's namespace.
  4. resolveExport walks B's Indirect entry and returns { moduleRecord: mockedSyntheticRecord, localName: 'foo' }.
  5. Line 470 correctly writes the env slot foo in the mocked module's environment.
  6. Line 483 executes source->putDirect(vm, 'bar', spy, 0) — a stray bar property is added to the factory object; source.foo is untouched.
  7. Once the read-side bug at line 216 is fixed to look up source[exportEntry.localName], reading nsB.bar forwards to source.foo — which still holds the original getter — and the spy is never observed via the namespace.

Even without the line-216 fix this is observable today: after step 6, reading nsMocked.foo on the mocked module's own namespace (where propertyName == 'foo') forwards to source.foo and returns the stale value, while the env slot already holds the spy — the two paths this hunk was added to keep consistent have diverged.

This is the write-side analogue of the read-side issue already flagged at line 216, but at a distinct code location and needing its own fix: use resolution.localName instead of name.

Secondary: putDirect is the wrong primitive here

While touching this line, note that putDirect(VM&, PropertyName, JSValue, unsigned) (JSObject.h:1231 → putDirectInternal<PutModeDefineOwnProperty>) makes assumptions the user-supplied backing object need not satisfy:

  • Numeric export names. putDirectInternal has ASSERT(!parseIndex(propertyName)) (JSObjectInlines.h:501) and the header documents "the property name is assumed to not be an index". A factory like mock.module(id, () => ({ '0': v })) or () => ['a','b'] yields export '0'; overrideExportValue on that key trips the debug ASSERT and, in release, adds '0' to named storage while the live value sits in indexed storage — the read side's source->get(globalObject, '0') parses the index and misses the write. Switching to resolution.localName does not help (it's still '0'). putDirectMayBeIndex (JSObject.cpp:3951) or a parseIndex branch handles this.

  • Accessor destruction. If source has get foo() { ... } — the exact shape this PR exists to support — putDirectInternal finds the existing offset with PropertyAttribute::Accessor, overwrites the GetterSetter cell via putDirectOffset, and (since newAttributes(0) != currentAttributes) takes Structure::attributeChangeTransition to a plain data property (JSObjectInlines.h:588–603). The live accessor is gone permanently; a later restore that writes the original snapshot back through the same path leaves ns.foo frozen — the pre-PR behavior this feature was meant to eliminate. If clobbering is the intended spyOn semantic that's fine, but if not, skip the write-through when the existing own descriptor is an accessor.

  • Proxy sources. liveExportsSource accepts any JSObject, so mock.module(id, () => new Proxy(target, { get(t,k){...} })) reaches here. putDirect is non-virtual and writes to the ProxyObject cell's own structure/butterfly, never invoking [[Set]]/[[DefineOwnProperty]] traps or touching the target. The read side's source->get() dispatches through ProxyObject::getOwnPropertySlot → the get trap → the target, so the value just written is invisible and the write-through is silently lost. A method-table-dispatched put (e.g. source->methodTable()->put(...) with a strict PutPropertySlot, or defineOwnProperty) would respect traps.

These three are edge cases and arguably nit-level on their own, but they all flow from the same choice of low-level define-own-property on an object whose shape the engine does not control, and can be addressed in the same one-line hunk as the localName fix.

(Minor: putDirect(VM&, ...) never throws a JS exception, so the RETURN_IF_EXCEPTION on line 484 is dead.)

Comment on lines +495 to +500

@claude claude Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The sibling batch API overrideExports() (lines 426-448) performs the same env-slot write via symbolTablePutTouchWatchpointSet but was not given the matching write-through to liveExportsSource() added here — so a bulk override on a live-source synthetic module updates only the env slot while namespace reads (lines 211-224) keep returning the source object's stale value. Before this PR both read and write agreed on the env slot, so this is a regression for that path; mirror this block inside the overrideExports() loop after line 441, keyed on pair.value.localName and dynamicDowncast<SyntheticModuleRecord>(pair.value.moduleRecord.get()). (Distinct from the wrong-key issue already noted on this hunk — here the write-through is absent from a separate function.)

Extended reasoning...

What the bug is

Commit 1ee717b added a write-through to the live-exports backing object in overrideExportValue() (lines 488-493) precisely because this PR redirected namespace reads on a SyntheticModuleRecord with a liveExportsSource away from the env slot and onto the source object (lines 211-224). But the sibling batch API overrideExports() at lines 426-448 — which performs the identical env-slot write via symbolTablePutTouchWatchpointSet at line 441 — was not given the same write-through. Its writes are now invisible to namespace reads whenever the target export resolves to a live-source synthetic module.

Step-by-step

  1. mock.module('m', () => ({ get foo() { return orig } })) creates a SyntheticModuleRecord whose m_liveExportsSource is the factory object with an own foo accessor, and whose env slot foo holds the first-read snapshot.
  2. Bun-side code calls ns->overrideExports(...) to bulk-replace exports (this Bun-specific API exists for re-mock / HMR paths). For the foo entry, pair.value.moduleRecord is the mocked SyntheticModuleRecord and pair.value.localName == 'foo'.
  3. Line 441 writes value into the env slot for foo. The source object's own foo accessor is untouched.
  4. A subsequent ns.foo read enters getOwnPropertySlotCommon, reaches line 211, dynamicDowncast<SyntheticModuleRecord> succeeds, liveExportsSource() is non-null, and source->getOwnPropertySlot(..., 'foo', ...) finds the old own accessor → hasOwn == true.
  5. Line 218 returns sourceSlot.getValue(...) — the original getter's result — and never consults the env slot. The override is silently lost.

Because pair.value.moduleRecord is the resolved target record (stored from resolution.moduleRecord in the constructor), this applies whether overrideExports() is called on the mocked module's own namespace or on a namespace that re-exports from it.

Why this is a regression introduced by this PR

Before this PR, both the write path (symbolTablePutTouchWatchpointSet → env slot) and the read path (getValue(environment, exportEntry.localName, scopeOffset) → env slot) agreed, so overrideExports() worked correctly on synthetic modules. This PR moved the read path to the source object but only taught overrideExportValue() to write there. The author's own commit message for 1ee717b ("overrideExportValue: write through to the live-exports source") states exactly the invariant being broken here for the batch variant.

Not a duplicate

The two existing comments on this PR are (a) line 223 — read side uses propertyName instead of exportEntry.localName, and (b) line 493 — overrideExportValue's write-through uses the wrong key / putDirect primitive. This finding is about a separate function where the write-through is absent entirely, and needs its own fix regardless of how (b) is resolved.

Fix

Mirror the block at 488-493 inside the overrideExports() loop, after line 441:

if (auto* synthetic = dynamicDowncast<SyntheticModuleRecord>(pair.value.moduleRecord.get())) {
    if (JSObject* source = synthetic->liveExportsSource())
        source->putDirect(vm, pair.value.localName, value, 0); // subject to the same putDirect caveats noted at line 493
}

(keyed on pair.value.localName, since that is the binding name in the target synthetic module — the same key line 441 already uses for the env-slot write).

Severity note

Marked nit rather than blocking because overrideExports() has no in-tree callers — it is invoked from the Bun side, and whether the companion change (oven-sh/bun#36677) exercises it against a live-source synthetic module cannot be confirmed from this repo. If it does, this is a user-visible regression and should be treated as blocking; either way the JSC-side inconsistency is real and cheap to fix alongside the line-493 change.


return putResult;
}

Expand Down
20 changes: 20 additions & 0 deletions Source/JavaScriptCore/runtime/SyntheticModuleRecord.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ void SyntheticModuleRecord::visitChildrenImpl(JSCell* cell, Visitor& visitor)
SyntheticModuleRecord* thisObject = uncheckedDowncast<SyntheticModuleRecord>(cell);
ASSERT_GC_OBJECT_INHERITS(thisObject, info());
Base::visitChildren(thisObject, visitor);
#if USE(BUN_JSC_ADDITIONS)
visitor.append(thisObject->m_liveExportsSource);
#endif
}

DEFINE_VISIT_CHILDREN(SyntheticModuleRecord);
Expand All @@ -92,7 +95,19 @@ SyntheticModuleRecord* SyntheticModuleRecord::tryCreateWithExportNamesAndValues(
VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);

#if USE(BUN_JSC_ADDITIONS)
// A trailing value with no matching name carries the live-exports backing
// object (Bun's mock.module / loader:"object" path).
JSObject* liveExportsSource = nullptr;
if (exportValues.size() == exportNames.size() + 1) {
JSValue extra = exportValues.at(exportNames.size());
if (extra.isObject())
liveExportsSource = asObject(extra);
}
ASSERT(exportNames.size() == exportValues.size() || liveExportsSource);
#else
ASSERT(exportNames.size() == exportValues.size());
#endif

auto* moduleRecord = create(globalObject, vm, globalObject->syntheticModuleRecordStructure(), moduleKey);
SymbolTable* exportSymbolTable = SymbolTable::create(vm);
Expand Down Expand Up @@ -121,6 +136,11 @@ SyntheticModuleRecord* SyntheticModuleRecord::tryCreateWithExportNamesAndValues(
ASSERT(putResult);
}

#if USE(BUN_JSC_ADDITIONS)
if (liveExportsSource)
moduleRecord->setLiveExportsSource(vm, liveExportsSource);
#endif

return moduleRecord;

}
Expand Down
9 changes: 9 additions & 0 deletions Source/JavaScriptCore/runtime/SyntheticModuleRecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,21 @@ class SyntheticModuleRecord final : public AbstractModuleRecord {

JS_EXPORT_PRIVATE static SyntheticModuleRecord* tryCreateWithExportNamesAndValues(JSGlobalObject*, const Identifier& moduleKey, const Vector<Identifier, 4>& exportNames, const MarkedArgumentBuffer& exportValues);

#if USE(BUN_JSC_ADDITIONS)
JSObject* liveExportsSource() const { return m_liveExportsSource.get(); }
void setLiveExportsSource(VM& vm, JSObject* source) { m_liveExportsSource.set(vm, this, source); }
#endif

private:
SyntheticModuleRecord(VM&, Structure*, const Identifier& moduleKey);

static SyntheticModuleRecord* tryCreateDefaultExportSyntheticModule(JSGlobalObject*, const Identifier& moduleKey, JSValue);

void finishCreation(JSGlobalObject*, VM&);

#if USE(BUN_JSC_ADDITIONS)
WriteBarrier<JSObject> m_liveExportsSource;
#endif
};

} // namespace JSC
Loading