Fix crash when Reflect.construct is used on mock functions returning non-objects - #28532
Fix crash when Reflect.construct is used on mock functions returning non-objects#28532robobun wants to merge 3 commits into
Conversation
|
Updated 8:05 PM PT - Mar 25th, 2026
❌ @robobun, your commit 56b2b37 has 7 failures in
🧪 To try this PR locally: bunx bun-pr 28532That installs a local version of the PR into your bun-28532 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDetect constructor calls in JSMockFunction via Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/JSMockFunction.cpp (1)
972-975:⚠️ Potential issue | 🔴 CriticalAdd
isConstructguards toKind::ReturnThisandKind::RejectedValuecases.The fallback and all other implementation cases (lines 953, 967, 989) explicitly check
thisValue.isObject()with a fallback toconstructEmptyObject(), proving thatthisValueis not guaranteed to be an object in construct mode.Kind::ReturnThisreturnsthisValuedirectly without this guard, which violates [[Construct]] semantics requiring an object return. Similarly,Kind::RejectedValueshould follow the defensive pattern used throughout this function.Current code (lines 972-981):
case JSMockImplementation::Kind::ReturnThis: { setReturnValue(createMockResult(vm, globalObject, "return"_s, thisValue)); return JSValue::encode(thisValue); } case JSMockImplementation::Kind::RejectedValue: { JSValue rejectedPromise = JSC::JSPromise::rejectedPromise(globalObject, impl->underlyingValue.get()); RETURN_IF_EXCEPTION(scope, {}); setReturnValue(createMockResult(vm, globalObject, "return"_s, rejectedPromise)); return JSValue::encode(rejectedPromise); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/JSMockFunction.cpp` around lines 972 - 975, The ReturnThis and RejectedValue branches must mirror the defensive construct-mode checks used elsewhere: for JSMockImplementation::Kind::ReturnThis, if isConstruct && !thisValue.isObject() create an object via constructEmptyObject(vm, globalObject) and use that as both the value passed to createMockResult(vm, globalObject, "return"_s, ...) and the encoded return value; otherwise keep current behavior. For JSMockImplementation::Kind::RejectedValue, compute the rejected promise with JSC::JSPromise::rejectedPromise(...), then if isConstruct && !thisValue.isObject() use constructEmptyObject(vm, globalObject) as the actual return object (and pass that into createMockResult), else return the rejectedPromise as now; ensure you still check RETURN_IF_EXCEPTION(scope, {}) after creating the promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bun.js/bindings/JSMockFunction.cpp`:
- Around line 972-975: The ReturnThis and RejectedValue branches must mirror the
defensive construct-mode checks used elsewhere: for
JSMockImplementation::Kind::ReturnThis, if isConstruct && !thisValue.isObject()
create an object via constructEmptyObject(vm, globalObject) and use that as both
the value passed to createMockResult(vm, globalObject, "return"_s, ...) and the
encoded return value; otherwise keep current behavior. For
JSMockImplementation::Kind::RejectedValue, compute the rejected promise with
JSC::JSPromise::rejectedPromise(...), then if isConstruct &&
!thisValue.isObject() use constructEmptyObject(vm, globalObject) as the actual
return object (and pass that into createMockResult), else return the
rejectedPromise as now; ensure you still check RETURN_IF_EXCEPTION(scope, {})
after creating the promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34a5ae12-3040-434c-bcc7-dba8ca115aa8
📥 Commits
Reviewing files that changed from the base of the PR and between e59a147 and 13a1fffa9b5eadd052cbe6ccbaef7b1b52502ba3.
📒 Files selected for processing (2)
src/bun.js/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn-construct.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| setReturnValue(createMockResult(vm, globalObject, "return"_s, jsUndefined())); | ||
| if (isConstruct) { | ||
| return JSValue::encode(thisValue.isObject() ? thisValue : JSC::constructEmptyObject(globalObject)); | ||
| } | ||
| return JSValue::encode(jsUndefined()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟣 Pre-existing issue: mock.mock.instances is always an empty array in Bun because fn->instances is never written to anywhere in jsMockFunctionCall. This PR adds isConstruct detection and makes Reflect.construct a first-class supported path, so users will now naturally reach this gap when inspecting m.mock.instances after constructor calls.
Extended reasoning...
What the bug is
mock.mock.instances is supposed to record the this value (the newly-constructed object) for every constructor invocation, matching Jest behavior. In Bun, the fn->instances write-barrier field is declared, GC-visited, lazily initialized to an empty array, and exposed on the mock.mock object — but it is never written to during invocation. The result is that m.mock.instances is permanently [] regardless of how many times the mock is called with new or Reflect.construct.
The specific code path
In jsMockFunctionCall (JSMockFunction.cpp), the function populates fn->calls, fn->contexts, fn->invocationCallOrder, and fn->returnValues for every call, but there is no corresponding block that pushes to fn->instances. The getInstances() accessor (lines 419-424) lazily constructs the array on first access but no code path ever calls instances->push(...) or putDirectIndex(...) on it.
Why existing code does not prevent this
Searching the entire file confirms fn->instances only appears in: the field declaration, clear(), getInstances(), the GC visitor, and the mock object structure initializer. There is no invocation-time write. The gap has always existed — before this PR, new m() also left instances empty — but the PR officially supports Reflect.construct as a working code path and adds the isConstruct flag, so users will now reasonably expect mock.mock.instances to be populated after Reflect.construct(m, []) succeeds.
Impact
Any code that inspects m.mock.instances[n] to verify what object was constructed will always see undefined (or find instances.length === 0). This is a divergence from Jest:
const m = jest.fn();
const obj = new m();
console.log(m.mock.instances[0] === obj); // Jest: true, Bun: false (empty array)How to fix
When isConstruct is true, push thisValue (or the fallback empty object) to fn->instances at the same point calls and contexts are recorded. The simplest fix mirrors the contexts tracking block:
JSC::JSArray* instances = fn->instances.get();
if (isConstruct) {
JSValue instanceValue = thisValue.isObject() ? thisValue : /* fallback constructed object */;
if (instances) {
instances->push(globalObject, instanceValue);
} else {
// initialize array with one entry
fn->instances.set(vm, fn, ...);
}
}Step-by-step proof
const m = jest.fn();— creates aJSMockFunction;fn->instancesis uninitialized (will be lazily created as[]).new m();—jsMockFunctionCallis invoked withcallframe->newTarget()set;isConstruct = true.- The function pushes to
fn->calls,fn->contexts,fn->invocationCallOrder. - No code touches
fn->instances. m.mock.instances→getInstances()returns the lazily-created empty[].m.mock.instances[0]→undefined. In Jest this would be the constructed object.
This is pre-existing behavior unrelated to this PR, but the PR directly interacts with the affected code area and increases the surface for this issue by making Reflect.construct succeed where it previously crashed.
13a1fff to
cbee3f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/test/mock-fn-construct.test.ts`:
- Around line 3-16: The tests "Reflect.construct on mock with non-object return
value does not crash" and "Reflect.construct on mock with mockReturnValue does
not crash" should also assert the constructed object's prototype to ensure we
don't silently return a fresh plain object: after calling Reflect.construct(m,
[]), check that Object.getPrototypeOf(result) === m.prototype (or the mock's
prototype) and add a third case using m.mockReturnThis() (and/or a bare
jest.fn() with no implementation) to cover the ReturnThis and no-implementation
branches touched by the native change; update assertions to verify both the
primitive-return value via m.mock.results[0].value and the correct prototype on
the constructed result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e104d7d1-0daf-4d99-84c5-86f2d4fe5d76
📥 Commits
Reviewing files that changed from the base of the PR and between 13a1fffa9b5eadd052cbe6ccbaef7b1b52502ba3 and 7db0842.
📒 Files selected for processing (2)
src/bun.js/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn-construct.test.ts
| test("Reflect.construct on mock with non-object return value does not crash", () => { | ||
| const m = jest.fn(() => 42); | ||
| const result = Reflect.construct(m, []); | ||
| expect(result).toBeObject(); | ||
| expect(m.mock.results[0].value).toBe(42); | ||
| }); | ||
|
|
||
| test("Reflect.construct on mock with mockReturnValue does not crash", () => { | ||
| const m = jest.fn(); | ||
| m.mockReturnValue(123); | ||
| const result = Reflect.construct(m, []); | ||
| expect(result).toBeObject(); | ||
| expect(m.mock.results[0].value).toBe(123); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert receiver/prototype semantics in the primitive-return cases.
toBeObject() would still pass if the construct fallback accidentally returned a fresh plain object. Adding prototype checks—and one mockReturnThis() or bare jest.fn() constructor case—would cover the ReturnThis and no-implementation branches touched by the native change.
🧪 Suggested coverage additions
test("Reflect.construct on mock with non-object return value does not crash", () => {
const m = jest.fn(() => 42);
const result = Reflect.construct(m, []);
expect(result).toBeObject();
+ expect(Object.getPrototypeOf(result)).toBe(m.prototype);
expect(m.mock.results[0].value).toBe(42);
});
@@
test("new on mock with non-object return value still works", () => {
const m = jest.fn(() => 42);
const result = new m();
expect(result).toBeObject();
+ expect(Object.getPrototypeOf(result)).toBe(m.prototype);
expect(m.mock.results[0].value).toBe(42);
});
+
+test("Reflect.construct preserves the supplied newTarget prototype", () => {
+ const m = jest.fn(() => 42);
+ function NewTarget() {}
+
+ const result = Reflect.construct(m, [], NewTarget);
+ expect(Object.getPrototypeOf(result)).toBe(NewTarget.prototype);
+});
+
+test("mockReturnThis under construction returns the receiver", () => {
+ const m = jest.fn().mockReturnThis();
+ const result = new m();
+
+ expect(Object.getPrototypeOf(result)).toBe(m.prototype);
+ expect(m.mock.results[0].value).toBe(result);
+});Also applies to: 25-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/test/mock-fn-construct.test.ts` around lines 3 - 16, The tests
"Reflect.construct on mock with non-object return value does not crash" and
"Reflect.construct on mock with mockReturnValue does not crash" should also
assert the constructed object's prototype to ensure we don't silently return a
fresh plain object: after calling Reflect.construct(m, []), check that
Object.getPrototypeOf(result) === m.prototype (or the mock's prototype) and add
a third case using m.mockReturnThis() (and/or a bare jest.fn() with no
implementation) to cover the ReturnThis and no-implementation branches touched
by the native change; update assertions to verify both the primitive-return
value via m.mock.results[0].value and the correct prototype on the constructed
result.
There was a problem hiding this comment.
LGTM — my previous inline concern was incorrect; the code correctly stores the original return value in mock.mock.results before returning the construct fallback, and the tests verify this.
Extended reasoning...
Overview
This PR modifies jsMockFunctionCall in JSMockFunction.cpp to add [[Construct]] semantics: when callframe->newTarget() is set, and the mock returns a non-object, the handler returns thisValue (if it is an object) or a fresh empty object rather than the primitive. A new test file covers Reflect.construct and new on mocks with non-object returns.
Security risks
No security concerns. This is entirely within the test-mocking subsystem, touches no auth, crypto, permissions, or network code.
Level of scrutiny
This is a targeted bugfix with low blast radius. The change adds an isConstruct flag and four small guard blocks; no existing logic paths are modified, only extended with early returns. The tests directly exercise the fixed crash case with assertions on both the returned value and mock.mock.results[n].value.
Prior review
I previously raised a 🔴 inline comment claiming returnValue was mutated before being stored in mock.mock.results, causing wrong values to be recorded. That analysis was wrong. In the Kind::Call path, putDirectIndex writes the original returnValue (e.g. 42) into the results array at line 954, and only after that does the isConstruct guard return the fallback object — returnValue itself is never reassigned. The tests assert expect(m.mock.results[0].value).toBe(42) and toBe(123) respectively, which would fail if the mutation I described actually occurred. The code is correct.
7db0842 to
2f14b9c
Compare
There was a problem hiding this comment.
LGTM — the construct-semantics fix is correct, my prior feedback was addressed (invalid tests removed in 56b2b37, and mock.results records the original value before the early-return path).
Extended reasoning...
Overview
This PR modifies jsMockFunctionCall in JSMockFunction.cpp to detect construct invocations via callframe->newTarget() and ensure all return paths satisfy the JS [[Construct]] contract (return value must be an object). A new test file mock-fn-construct.test.ts with two targeted crash-regression tests is added.
Security risks
None. This touches mock/test infrastructure only, with no auth, crypto, or privileged code paths involved.
Level of scrutiny
Moderate. The change is a focused crash fix with clear semantics — standard [[Construct]] fallback behavior. The isConstruct variable is read-only and computed once at the top of the function. Each return site in jsMockFunctionCall was individually reviewed for the correct interaction between result recording and the fallback return.
Other factors
mock.resultsrecording is correct: in theKind::Callpath,returnValuesArray->putDirectIndex(... returnValue)runs before theisConstructearly-return, somock.results[0].valuecorrectly captures the original primitive (42 or 123). My earlier inline comment flagging this as a bug was based on a misread of the diff ordering.- Tests are valid: both tests assert
toBeObject()andmock.results[0].value === original_primitive. These would crash/fail on system Bun (prior to this fix) sinceReflect.constructreturning a non-object from a native construct handler triggers a JSC assertion. Commit56b2b37removed the two tests that were already passing on system Bun, addressing my prior review comment. - Pre-existing issues (instances array never populated,
withImplementationsync restore tail bug) are unrelated to this PR and flagged as pre-existing in the inline comments.
| } | ||
|
|
||
| setReturnValue(createMockResult(vm, globalObject, "return"_s, jsUndefined())); | ||
| if (isConstruct) { | ||
| return JSValue::encode(thisValue.isObject() ? thisValue : JSC::constructEmptyObject(globalObject)); | ||
| } | ||
| return JSValue::encode(jsUndefined()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟣 Pre-existing bug: in jsMockFunctionWithImplementation, the synchronous restore path sets thisObject->tail to lastImpl instead of lastTail, corrupting the once-implementation queue when multiple mockImplementationOnce calls precede a synchronous withImplementation. This is unrelated to this PR, which only modifies jsMockFunctionCall.
Extended reasoning...
What the bug is
In jsMockFunctionWithImplementation (around line 1431 of JSMockFunction.cpp), the synchronous restore path has a copy-paste error: it uses lastImpl for both the implementation and tail write-barriers, but tail should be restored to lastTail:
auto lastImpl = thisObject->implementation.get(); // head of the once-queue
auto lastTail = thisObject->tail.get(); // tail of the once-queue
auto lastFallback = thisObject->fallbackImplmentation.get();
// ...
// synchronous restore:
thisObject->implementation.set(vm, thisObject, lastImpl); // correct
thisObject->tail.set(vm, thisObject, lastImpl); // BUG: should be lastTail
thisObject->fallbackImplmentation.set(vm, thisObject, lastFallback); // correctlastTail is captured but only ever passed to MockWithImplementationCleanupData::create for the async path. The synchronous path never uses it.
The specific code path
The async cleanup handler jsMockFunctionWithImplementationCleanup (line 1367) correctly restores fn->tail from ctx->internalField(2) which holds lastTail. The sync path is asymmetric: it skips the correct lastTail and erroneously writes lastImpl to tail instead.
Why existing code does not prevent this
The tail pointer is only ever validated implicitly through the correctness of the once-implementation linked list. When tail is wrong, the list appears intact until a new mockImplementationOnce call chains off the wrong node — at that point, any once-impls that were chained after the first are silently orphaned. There is no assertion or runtime check.
Impact
When a user has queued multiple once-implementations and then calls withImplementation synchronously, any once-impls beyond the first are lost. Subsequent mockImplementationOnce calls chain off the wrong node:
const m = jest.fn();
m.mockImplementationOnce(() => 1);
m.mockImplementationOnce(() => 2); // tail -> once(2), impl -> once(1)
m.withImplementation(() => 99, () => {});
// After sync restore: tail = once(1) (WRONG, should be once(2))
m.mockImplementationOnce(() => 3);
// pushImplOnce chains once(3) off once(1), but once(2) is now orphaned
// queue should be: once(1)->once(2)->once(3), but is: once(1)->once(3)Step-by-step proof
m.mockImplementationOnce(() => 1)—impl = once(1),tail = once(1)m.mockImplementationOnce(() => 2)—impl = once(1),once(1).next = once(2),tail = once(2)m.withImplementation(() => 99, () => {})— saveslastImpl = once(1),lastTail = once(2), replaces impl temporarily- Sync callback returns (non-promise) — restore path runs
implementation.set(lastImpl)→impl = once(1)✓tail.set(lastImpl)→tail = once(1)✗ (should beonce(2))m.mockImplementationOnce(() => 3)—pushImplOncereadstail = once(1), setsonce(1).next = once(3),tail = once(3)once(2)is now unreachable — the second once-impl is silently lost
Fix
Change the synchronous restore to use lastTail:
thisObject->tail.set(vm, thisObject, lastTail); // was: lastImpl
1 participant
jsMockFunctionCallserves as both the call and construct handler forJSMockFunction. When a mock returns a non-object value (e.g. a number frommockReturnValue), calling it viaReflect.constructcauses JSC to assertisCell()atJSCJSValue.h:1077because the construct path expects the handler to return a cell/object.Root cause:
JSMockFunctionregisters the same native function for both call and construct:When the mock's return value is not an object (number, undefined, etc.), it is returned directly from the construct handler.
newhas its own isObject check in the bytecode, butReflect.constructtrusts the handler and crashes.Fix: Follow standard JS
[[Construct]]semantics — when called as a constructor (newTargetis set) and the return value is not an object, returnthisValueor a new empty object instead.Repro: