Skip to content

Fix crash when Reflect.construct is used on mock functions returning non-objects #28532

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

Closed
robobun wants to merge 3 commits into from
Closed

Fix crash when Reflect.construct is used on mock functions returning non-objects #28532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21c11ab
Fix crash when Reflect.construct is used on mock functions returning …
robobun Mar 25, 2026
2f14b9c
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 25, 2026
56b2b37
Remove tests that pass on system bun
robobun Mar 25, 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
13 changes: 13 additions & 0 deletions src/bun.js/bindings/JSMockFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,7 @@ JSC_DEFINE_HOST_FUNCTION(jsMockFunctionCall, (JSGlobalObject * lexicalGlobalObje
return {};
}

const bool isConstruct = !callframe->newTarget().isUndefined();
JSC::ArgList args = JSC::ArgList(callframe);
JSValue thisValue = callframe->thisValue();
JSC::JSArray* argumentsArray = nullptr;
Expand Down Expand Up @@ -954,15 +955,24 @@ JSC_DEFINE_HOST_FUNCTION(jsMockFunctionCall, (JSGlobalObject * lexicalGlobalObje
fn->returnValues.set(vm, fn, returnValuesArray);
}

if (isConstruct && !returnValue.isObject()) {
return JSValue::encode(thisValue.isObject() ? thisValue : JSC::constructEmptyObject(globalObject));
}
return JSValue::encode(returnValue);
}
case JSMockImplementation::Kind::ReturnValue: {
JSValue returnValue = impl->underlyingValue.get();
setReturnValue(createMockResult(vm, globalObject, "return"_s, returnValue));
if (isConstruct && !returnValue.isObject()) {
return JSValue::encode(thisValue.isObject() ? thisValue : JSC::constructEmptyObject(globalObject));
}
return JSValue::encode(returnValue);
}
case JSMockImplementation::Kind::ReturnThis: {
setReturnValue(createMockResult(vm, globalObject, "return"_s, thisValue));
if (isConstruct && !thisValue.isObject()) {
return JSValue::encode(JSC::constructEmptyObject(globalObject));
}
return JSValue::encode(thisValue);
}
case JSMockImplementation::Kind::RejectedValue: {
Expand All @@ -978,6 +988,9 @@ JSC_DEFINE_HOST_FUNCTION(jsMockFunctionCall, (JSGlobalObject * lexicalGlobalObje
}

setReturnValue(createMockResult(vm, globalObject, "return"_s, jsUndefined()));
if (isConstruct) {
return JSValue::encode(thisValue.isObject() ? thisValue : JSC::constructEmptyObject(globalObject));
}
return JSValue::encode(jsUndefined());
}

Comment on lines 988 to 996

@claude claude Bot Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟣 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

  1. const m = jest.fn(); — creates a JSMockFunction; fn->instances is uninitialized (will be lazily created as []).
  2. new m(); — jsMockFunctionCall is invoked with callframe->newTarget() set; isConstruct = true.
  3. The function pushes to fn->calls, fn->contexts, fn->invocationCallOrder.
  4. No code touches fn->instances.
  5. m.mock.instances → getInstances() returns the lazily-created empty [].
  6. 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.

Comment on lines 988 to 996

@claude claude Bot Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟣 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); // correct

lastTail 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

  1. m.mockImplementationOnce(() => 1) — impl = once(1), tail = once(1)
  2. m.mockImplementationOnce(() => 2) — impl = once(1), once(1).next = once(2), tail = once(2)
  3. m.withImplementation(() => 99, () => {}) — saves lastImpl = once(1), lastTail = once(2), replaces impl temporarily
  4. Sync callback returns (non-promise) — restore path runs
  5. implementation.set(lastImpl) → impl = once(1) ✓
  6. tail.set(lastImpl) → tail = once(1) ✗ (should be once(2))
  7. m.mockImplementationOnce(() => 3) — pushImplOnce reads tail = once(1), sets once(1).next = once(3), tail = once(3)
  8. 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

Expand Down
16 changes: 16 additions & 0 deletions test/js/bun/test/mock-fn-construct.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { expect, jest, test } from "bun:test";

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);
});
Comment on lines +3 to +16

@coderabbitai coderabbitai Bot Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 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.

Loading