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
+43 −1
Closed

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

Show file tree
Hide file tree
Changes from all commits
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
23 changes: 22 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,26 @@
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;

JSObject* newTarget = asObject(callframe->newTarget());
JSGlobalObject* functionGlobalObject = getFunctionRealm(lexicalGlobalObject, newTarget);
RETURN_IF_EXCEPTION(scope, {});
Structure* structure = InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor());

@Jarred-Sumner Jarred-Sumner Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is that createSubclassStructure correct? shouldn't it be based on the mock functions structure or no?

@robobun robobun Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I believe so — the third argument to createSubclassStructure is the base structure for the instances this constructor produces, not the structure of the callee. It's used as:

  1. The fallback when newTarget.prototype isn't an object
  2. The class-info/shape template even when newTarget.prototype is an object (the result is a structure with that prototype but the base's cell type)

A mock function acting as a constructor is emulating an ordinary user-defined function constructor, so its instances should be plain JSFinalObjects — there's no "mock instance" cell type. objectStructureForObjectConstructor() is the plain-object structure, which matches:

JSMockFunction also doesn't expose a .prototype property (jest.fn().prototype === undefined), so when newTarget is the mock itself, createSubclassStructure falls back to the base structure → plain {} with Object.prototype, which is the OrdinaryCreateFromConstructor default. When newTarget is something else (Reflect.construct(mock, [], SomeClass)), it correctly picks up SomeClass.prototype.

RETURN_IF_EXCEPTION(scope, {});
RELEASE_AND_RETURN(scope, JSValue::encode(JSC::constructEmptyObject(vm, structure)));
}

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

View check run for this annotation

Claude / Claude Code Review

Construct fallback object created after impl runs, losing this-mutations 

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 fulle
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
21 changes: 21 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,27 @@ describe("mock()", () => {

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 });

class Base {}
expect(Reflect.construct(empty, [], Base)).toBeInstanceOf(Base);

if (isBun) {
// Jest doesn't allow spying on non-function properties
const spied = spyOn({ foo: 42 }, "foo");
expect(typeof Reflect.construct(spied, [])).toBe("object");
}
});
});

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