Skip to content

Fix crash when using Reflect.construct on a mock function #29790

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 2 commits into from
Closed

Fix crash when using Reflect.construct on a mock function #29790

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cdac378
Fix crash when using Reflect.construct on a mock function
robobun Apr 27, 2026
2877eb3
Honor newTarget.prototype in mock construct fallback
robobun Apr 27, 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
18 changes: 17 additions & 1 deletion src/bun.js/bindings/JSMockFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
}

JSC_DECLARE_HOST_FUNCTION(jsMockFunctionCall);
JSC_DECLARE_HOST_FUNCTION(jsMockFunctionConstruct);
JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_protoImpl);
JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_mock);
JSC_DECLARE_HOST_FUNCTION(jsMockFunctionGetter_mockGetLastCall);
Expand Down Expand Up @@ -462,7 +463,7 @@
}

JSMockFunction(JSC::VM& vm, JSC::Structure* structure, CallbackKind wrapKind)
: Base(vm, structure, jsMockFunctionCall, jsMockFunctionCall)
: Base(vm, structure, jsMockFunctionCall, jsMockFunctionConstruct)
{
initMock();
}
Expand Down Expand Up @@ -981,6 +982,21 @@
return JSValue::encode(jsUndefined());
}

JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe))
{
auto& vm = JSC::getVM(lexicalGlobalObject);
auto scope = DECLARE_THROW_SCOPE(vm);

EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe);
RETURN_IF_EXCEPTION(scope, {});

JSValue result = JSValue::decode(encodedResult);
if (result.isObject()) [[likely]]
return encodedResult;

RELEASE_AND_RETURN(scope, JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject)));

Check warning on line 997 in src/bun.js/bindings/JSMockFunction.cpp

View check run for this annotation

Claude / Claude Code Review

Construct fallback ignores newTarget.prototype 

Nit: the fallback uses `constructEmptyObject(lexicalGlobalObject)`, which always inherits from `Object.prototype` and ignores `callframe->newTarget()`. So `Reflect.construct(jest.fn(), [], SomeClass)` returns an object that is *not* `instanceof SomeClass`, whereas Jest (ordinary `[[Construct]]`) would honor `newTarget.prototype`. Not blocking — the crash fix is a strict improvement — but you could use `InternalFunction::createSubclassStructure` (as in `NapiClass.cpp` / `JSDOMFile.cpp`) to derive
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +985 to +1003

@claude claude Bot Apr 27, 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.

🟡 Nit: the fallback object is allocated after the implementation runs (which is invoked as a plain [[Call]] with callframe->thisValue()), so for new (jest.fn(function(){ this.x = 42 }))() the this.x = 42 lands on the wrong object and the returned instance is an empty {} — Jest yields {x: 42}. Not blocking — this is still a strict improvement over the crash — but the "matching ordinary [[Construct]] semantics" claim doesn't hold for the constructor‑assigns‑to‑this pattern; a fuller fix would create the receiver from newTarget.prototype first, pass it as this when invoking the impl, and return it on a non‑object result.

Extended reasoning...

What the bug is

jsMockFunctionConstruct delegates to jsMockFunctionCall with the original callframe, and only after that call returns does it allocate the fallback object via constructEmptyObject(vm, structure). Inside jsMockFunctionCall, the implementation is invoked as

JSValue returnValue = Bun::call(globalObject, result, callData, thisValue, args);
// where thisValue = callframe->thisValue()

i.e. a plain [[Call]] with whatever the native construct frame's thisValue happens to be — not a freshly‑allocated receiver derived from newTarget.prototype. So when the mock body assigns to this, those mutations land on the wrong object, and the brand‑new fallback object created at line 1002 has none of them.

In ordinary ECMAScript [[Construct]] (and in Jest, where jest.fn() is a real JS function), the engine performs OrdinaryCreateFromConstructor(newTarget, "%Object.prototype%") before the body runs, the body executes with that object as this, and that same object is returned when the body's completion value is not an object.

Step‑by‑step proof

const m = jest.fn(function () { this.x = 42; });
const inst = new m();
Step Jest (ordinary JS function) Bun after this PR
1. Create receiver from newTarget.prototype {} (proto = m.prototype) — (skipped)
2. Run body with that receiver as this sets receiver.x = 42 runs Bun::call(impl, callframe->thisValue(), …); this.x = 42 lands on the construct frame's thisValue, not the eventual return value
3. Body returns undefined (non‑object) return the receiver result.isObject() is false → allocate a new constructEmptyObject(vm, structure)
4. inst.x 42 undefined

A secondary consequence of the same ordering: createSubclassStructure reads newTarget.prototype after jsMockFunctionCall has already consumed any mockReturnValueOnce queue entry and pushed into mock.calls/mock.results. If newTarget.prototype is a throwing getter, the once‑queue is consumed and the call is recorded even though the construct ultimately throws — whereas spec/Jest read the prototype first, before the body ever runs.

Why nothing prevents it

jsMockFunctionConstruct never sets up a receiver before delegating; it just forwards the same callframe. jsMockFunctionCall has no construct‑aware path — it always passes callframe->thisValue() straight through to Bun::call. The createSubclassStructure work added in 2877eb3 fixed the prototype chain of the fallback (so Reflect.construct(empty, [], Base) instanceof Base), but didn't change when the object is created or what this the body sees.

Impact

Low. Before this PR the same path crashed (asObject(undefined) assertion), so any behavior here is a strict improvement, not a regression. The observable divergence affects the recognizable "mock a constructor that assigns to this" pattern (jest.fn(function(){ this.foo = … }) used with new), where Bun returns an empty object instead of the populated one Jest returns. This is distinct from the already‑addressed CodeRabbit/Claude comments on this PR, which only covered the newTarget.prototype chain.

How to fix (follow‑up)

Allocate the receiver up front and thread it through the implementation call, roughly:

JSObject* newTarget = asObject(callframe->newTarget());
auto* fnGlobal = getFunctionRealm(lexicalGlobalObject, newTarget);
RETURN_IF_EXCEPTION(scope, {});
Structure* structure = InternalFunction::createSubclassStructure(
    lexicalGlobalObject, newTarget, fnGlobal->objectStructureForObjectConstructor());
RETURN_IF_EXCEPTION(scope, {});
JSObject* receiver = constructEmptyObject(vm, structure);

// invoke the mock with `receiver` as thisValue (requires plumbing into jsMockFunctionCall
// or factoring out a helper that takes an explicit thisValue)

return result.isObject() ? encodedResult : JSValue::encode(receiver);

That's a larger refactor than this crash fix targets, so flagging as a known limitation / follow‑up rather than a blocker.


void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
{
Base::finishCreation(vm);
Expand Down
15 changes: 15 additions & 0 deletions test/js/bun/test/mock-fn.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,21 @@

expect(bar()()).toBe(true);
});

it("Reflect.construct on a mock returns an object", () => {
const empty = jest.fn();
expect(typeof Reflect.construct(empty, [])).toBe("object");
expect(empty).toHaveBeenCalledTimes(1);

const returnsPrimitive = jest.fn().mockReturnValue(42);
expect(typeof Reflect.construct(returnsPrimitive, [])).toBe("object");

const returnsObject = jest.fn(() => ({ x: 1 }));
expect(Reflect.construct(returnsObject, [])).toEqual({ x: 1 });

const spied = spyOn({ foo: 42 }, "foo");
expect(typeof Reflect.construct(spied, [])).toBe("object");

Check warning on line 810 in test/js/bun/test/mock-fn.test.js

View check run for this annotation

Claude / Claude Code Review

spyOn non-function property breaks Jest/Vitest compat 

nit: `spyOn({ foo: 42 }, "foo")` on a non-function property is Bun-specific — Jest throws `Cannot spy the foo property because it is not a function`. Since this file is meant to be runnable in Jest/Vitest (per the header), these two lines should be wrapped in `if (isBun) { ... }`, matching the existing `spyOn works on object` test which is guarded for the same reason.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
});
});

describe("spyOn", () => {
Expand Down
Loading