Skip to content

Fix crash when constructing a mock function whose implementation returns a primitive #31375

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 constructing a mock function whose implementation returns a primitive #31375

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
39786a7
fix(bun:test): return an object when constructing a mock function
robobun May 25, 2026
85f9f8b
Use jest.fn() in cross-runner mock construct test
robobun May 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
36 changes: 35 additions & 1 deletion src/jsc/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,39 @@
return JSValue::encode(jsUndefined());
}

// Native constructors must return an object, so behave like `new` on an ordinary JS function:
// run the mock with a freshly created `this` and return its result only when it is an object.
JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe))
{
auto& vm = JSC::getVM(lexicalGlobalObject);
auto scope = DECLARE_THROW_SCOPE(vm);

if (!dynamicDowncast<JSMockFunction>(callframe->jsCallee())) [[unlikely]] {
throwTypeError(lexicalGlobalObject, scope, "Expected callee to be mock function"_s);
return {};
}

JSValue newTarget = callframe->newTarget();
JSObject* thisObject = nullptr;
if (newTarget && newTarget.isObject()) {
JSValue prototype = asObject(newTarget)->get(lexicalGlobalObject, vm.propertyNames->prototype);
RETURN_IF_EXCEPTION(scope, {});
if (prototype.isObject())
thisObject = JSC::constructEmptyObject(lexicalGlobalObject, asObject(prototype));
}
if (!thisObject)
thisObject = JSC::constructEmptyObject(lexicalGlobalObject);

callframe->setThisValue(thisObject);
JSValue returnValue = JSValue::decode(jsMockFunctionCall(lexicalGlobalObject, callframe));
RETURN_IF_EXCEPTION(scope, {});

Check notice on line 1010 in src/jsc/bindings/JSMockFunction.cpp

View check run for this annotation

Claude / Claude Code Review

mock.instances not populated when constructing a mock 

Not a regression, but worth noting while we're here: `mock.instances` is still never populated — `fn->instances` is declared, cleared, GC-visited and exposed via `getInstances()`, but nothing ever pushes to it. In Jest, `new mockFn()` pushes the constructed `this` onto `mock.instances` (it's the canonical API for tracking instances), and the new construct path now has `thisObject` in hand, so this would be the natural place to wire it up. The new test asserts on `noImpl.mock.contexts[0]` rather
Comment on lines +1008 to +1010

@claude claude Bot May 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.

🟣 Not a regression, but worth noting while we're here: mock.instances is still never populated — fn->instances is declared, cleared, GC-visited and exposed via getInstances(), but nothing ever pushes to it. In Jest, new mockFn() pushes the constructed this onto mock.instances (it's the canonical API for tracking instances), and the new construct path now has thisObject in hand, so this would be the natural place to wire it up. The new test asserts on noImpl.mock.contexts[0] rather than noImpl.mock.instances[0], which sidesteps the gap.

Extended reasoning...

What the gap is. In Jest, mockFn.mock.instances is the documented API for tracking objects created by calling a mock with new — each new mockFn() pushes the constructed this onto that array. Bun declares the backing storage for this (mutable JSC::WriteBarrier<JSC::JSArray> instances), clears it in clear(), visits it in visitAdditionalChildrenInGCThread, lazily creates it in getInstances(), and wires it into the mock object structure at offset 2 — but nothing in the file ever pushes a value into it. Grepping the whole of JSMockFunction.cpp for instances shows only declarations, clears, visits, and reads; there is no push / initializeIndex / putDirectIndex targeting fn->instances.

Code path. jsMockFunctionCall records each invocation by pushing to fn->calls, fn->contexts, fn->invocationCallOrder, and fn->returnValues, but not fn->instances. The new jsMockFunctionConstruct creates thisObject (exactly the value Jest pushes onto mock.instances), then delegates to jsMockFunctionCall via callframe->setThisValue(thisObject). So after construction, mock.contexts contains the new instance but mock.instances remains empty.

Why nothing prevents it. The only thing that could populate mock.instances is an explicit push, and there isn't one anywhere in the file. The new test in mock-fn.test.js asserts expect(noImpl.mock.contexts[0]).toBe(instance) rather than expect(noImpl.mock.instances[0]).toBe(instance), so the test suite doesn't catch the omission. Under real Jest (which this test file is documented to be runnable against), mock.instances[0] would equal instance — the cross-runner contract is satisfied only because the test avoids checking it.

Step-by-step proof.

  1. const Fn = jest.fn(); const inst = new Fn();
  2. jsMockFunctionConstruct runs, allocates thisObject, sets it as the call frame's this, and calls jsMockFunctionCall.
  3. jsMockFunctionCall pushes thisObject onto fn->contexts (so Fn.mock.contexts === [inst]), pushes [] onto fn->calls, pushes a result onto fn->returnValues, pushes an id onto fn->invocationCallOrder — and returns.
  4. Nothing has touched fn->instances. Fn.mock.instances lazily resolves via getInstances() to a fresh empty array.
  5. Result: Fn.mock.instances → [] in Bun, [inst] in Jest.

Impact. This is a pre-existing Jest-compat gap, not a regression — mock.instances was never populated before this PR either, and the PR's stated purpose (fixing the isCell() crash) is achieved correctly without it. But the PR explicitly adds the construct path "to match Jest, where mocks are plain JS functions", and mock.instances is the Jest API specifically for construct calls, so it's directly adjacent. User code that follows the Jest docs (expect(MockCtor.mock.instances[0]).toBe(...)) will still see an empty array under Bun.

Fix. In jsMockFunctionConstruct, after creating thisObject (and before or after delegating to jsMockFunctionCall), push thisObject onto fn->instances using the same pattern used for contexts in jsMockFunctionCall (push if the array exists, otherwise create a 1-element array and fn->instances.set(...)). Then the new test could assert noImpl.mock.instances[0] instead of (or in addition to) noImpl.mock.contexts[0]. Non-blocking — fine as a follow-up.


if (returnValue && returnValue.isObject())
return JSValue::encode(returnValue);

return JSValue::encode(thisObject);
}

void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
{
Base::finishCreation(vm);
Expand Down
36 changes: 36 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,35 @@ describe("mock()", () => {

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

test("can be invoked with new", () => {
const noImpl = jest.fn();
const instance = new noImpl();
expect(instance).toBeInstanceOf(Object);
expect(noImpl.mock.contexts[0]).toBe(instance);
expect(Reflect.construct(noImpl, [])).toBeInstanceOf(Object);

const primitiveImpl = jest.fn(() => 42);
expect(new primitiveImpl()).toBeInstanceOf(Object);
expect(primitiveImpl.mock.results[0]).toEqual({ type: "return", value: 42 });

const usesThis = jest.fn(function () {
this.x = 1;
});
expect(new usesThis()).toEqual({ x: 1 });

const returnsObject = jest.fn(() => ({ y: 2 }));
expect(new returnsObject()).toEqual({ y: 2 });

const throws = jest.fn(() => {
throw new Error("boom");
});
expect(() => new throws()).toThrow("boom");

class Target {}
const withNewTarget = jest.fn();
expect(Object.getPrototypeOf(Reflect.construct(withNewTarget, [], Target))).toBe(Target.prototype);
});
Comment thread
claude[bot] marked this conversation as resolved.
});

describe("spyOn", () => {
Expand Down Expand Up @@ -1011,6 +1040,13 @@ describe("spyOn", () => {
expect(arr[14]()).toBe(456);
expect(fn).not.toHaveBeenCalled();
});

test("constructing a spy on a missing property returns an object", () => {
const target = {};
const fn = spyOn(target, "missing");
expect(Reflect.construct(fn, [])).toBeInstanceOf(Object);
expect(new fn()).toBeInstanceOf(Object);
});
}

// spyOn does not work with getters/setters yet.
Expand Down
Loading