Fix crash when constructing a jest.fn() mock#30641
Conversation
JSMockFunction used the same native entry point for [[Call]] and [[Construct]], and that entry point could return primitives (undefined, the mockReturnValue, or the implementation's return value). When invoked via Reflect.construct, JSC's Interpreter::executeConstruct unconditionally does asObject() on the result of a native construct, triggering an assertion when the result was not an object. Split the entry point into separate call and construct variants. On construct, allocate a fresh this object derived from new.target, record it in mock.instances, pass it as this to the implementation, and return it whenever the implementation result is not an object. This matches normal [[Construct]] semantics and Jest's behaviour for mock.instances.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30212 which has already been through review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JSGlobalObject* functionGlobalObject = getFunctionRealm(globalObject, newTargetObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| Structure* baseStructure = functionGlobalObject->objectStructureForObjectConstructor(); | ||
| Structure* structure = newTargetObject == fn ? baseStructure : InternalFunction::createSubclassStructure(globalObject, newTargetObject, baseStructure); |
There was a problem hiding this comment.
🔴 The newTargetObject == fn fast-path skips createSubclassStructure and hard-codes objectStructureForObjectConstructor() (i.e. Object.prototype), so a user-assigned fn.prototype is ignored when constructing directly via new fn() — yet honored via Reflect.construct(fn, [], Other). Since createSubclassStructure already falls back to baseStructure when newTarget.prototype isn't an object, you can drop the ternary and call it unconditionally.
Extended reasoning...
What the bug is
Line 850 uses the standard JSC fast-path idiom:
Structure* structure = newTargetObject == fn
? baseStructure
: InternalFunction::createSubclassStructure(globalObject, newTargetObject, baseStructure);This pattern is correct for built-in constructors like Array or Map, where it relies on two invariants: (a) the constructor's .prototype is non-writable/non-configurable, and (b) baseStructure already has that .prototype baked in as its stored prototype. Neither invariant holds for JSMockFunction. As an InternalFunction, it has no default .prototype own-property (so users can freely assign one), and baseStructure here is functionGlobalObject->objectStructureForObjectConstructor(), whose stored prototype is Object.prototype — not anything the user set.
How it manifests
Assigning .prototype to a mock is a documented Jest pattern for mocking ES6 classes (see Jest's ES6 class mocks docs). In Jest, mocks are plain JS functions, so new fn() follows OrdinaryCreateFromConstructor and honors fn.prototype. With this PR's fast-path, Bun ignores it.
Why nothing prevents it
InternalFunction::createSubclassStructure does the right thing — it performs newTarget->get(vm.propertyNames->prototype) and uses the result as the instance's [[Prototype]], falling back to baseStructure only when .prototype is not an object. The bug is precisely that the newTargetObject == fn branch skips this lookup. The new test at mock-fn.test.js:842 ("Reflect.construct respects new.target prototype") only exercises the newTarget !== fn branch, so it passes; there's no test that sets fn.prototype and constructs directly.
Step-by-step proof
const fn = jest.fn();
fn.prototype = { greet() { return "hi"; } };
const inst = new fn();new fn()entersjsMockFunctionCallImpl<true>withcallframe->newTarget() === fn.newTargetObject == fnis true, sostructure = baseStructure = objectStructureForObjectConstructor().thisValue = constructEmptyObject(vm, structure)→ an object whose [[Prototype]] isObject.prototype.- No implementation returns an object, so
constructorResultreturnsthisValue. - Result:
Object.getPrototypeOf(inst) === Object.prototype, andinst.greetisundefined.
Contrast with Reflect.construct(fn, [], class C {}): newTargetObject !== fn, so createSubclassStructure reads C.prototype and the instance correctly inherits from it — the asymmetry confirmed by the new test.
Impact
Users mocking classes by assigning Mock.prototype.method = jest.fn() and then doing new Mock() will get instances that don't inherit those methods, diverging from Jest. instanceof checks against fn will also fail (inst instanceof fn → false even though fn.prototype is set).
Fix
Drop the ternary:
Structure* structure = InternalFunction::createSubclassStructure(globalObject, newTargetObject, baseStructure);createSubclassStructure already returns baseStructure when newTarget.prototype is not an object (the default case for a fresh jest.fn()), so this is both a correctness fix and a simplification with negligible cost.
| JSC::JSArray* instancesArray = fn->instances.get(); | ||
| if (instancesArray) { | ||
| instancesArray->push(globalObject, thisValue); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| } else { | ||
| JSC::ObjectInitializationScope object(vm); | ||
| instancesArray = JSC::JSArray::tryCreateUninitializedRestricted( | ||
| object, | ||
| globalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous), | ||
| 1); | ||
| instancesArray->initializeIndex(object, 0, thisValue); | ||
| fn->instances.set(vm, fn, instancesArray); | ||
| } |
There was a problem hiding this comment.
🟡 In Jest, mock.instances is pushed on every invocation (in lockstep with mock.calls/mock.contexts), not only on [[Construct]] — see jest-mock's wrapper, which runs mockState.instances.push(this) unconditionally. With this change, mixing new fn() and fn() produces arrays where instances[i] no longer corresponds to calls[i], and the new test "calling as a function does not populate instances" will fail when this file is run under Jest (the file header says it's meant to be cross-runtime). Consider pushing thisValue to instances unconditionally to match jest-mock, or at minimum gate that last test with if (isBun).
Extended reasoning...
What the bug is
Jest's mock-function wrapper (in packages/jest-mock/src/index.ts) executes, on every invocation:
mockState.instances.push(this);
mockState.contexts.push(this);
mockState.calls.push(args);All three arrays therefore grow in lockstep and fn.mock.instances.length === fn.mock.calls.length === fn.mock.contexts.length always holds. After this PR, Bun pushes to instances only inside the if constexpr (isConstructCall) block in jsMockFunctionCallImpl, while calls and contexts are still pushed unconditionally. So a regular (non-construct) call grows calls/contexts but leaves instances untouched.
How it manifests / step-by-step proof
const fn = jest.fn();
new fn(); // call 0: construct
fn(); // call 1: regular
new fn(); // call 2: constructJest:
fn.mock.calls.length === 3fn.mock.contexts.length === 3fn.mock.instances.length === 3→[obj0, undefined, obj2]fn.mock.instances[2]corresponds tofn.mock.calls[2].
Bun after this PR:
fn.mock.calls.length === 3fn.mock.contexts.length === 3fn.mock.instances.length === 2→[obj0, obj2]fn.mock.instances[2]isundefinedandfn.mock.instances[1]is the instance from call 2, not call 1.
Any Jest-compatible test that indexes these arrays in parallel (e.g. expect(fn.mock.instances[i]).toBe(fn.mock.contexts[i])) will see misaligned data.
Why existing code doesn't prevent it
The instances push is guarded by if constexpr (isConstructCall) (JSMockFunction.cpp:845–867), whereas the contexts push a few lines below runs in both branches. Nothing else in the function adds an entry to instances for non-construct calls, so the arrays diverge by design.
Impact
- Index misalignment between
mock.instancesandmock.calls/mock.contextswhen construct and regular calls are mixed — a behavioral divergence from Jest that can break ported test suites. - The new test breaks the file's cross-runtime contract.
mock-fn.test.jsopens with "This file is meant to be runnable in Jest, Vitest, and Bun", but the new test "calling as a function does not populate instances" assertsexpect(fn.mock.instances).toHaveLength(0)after a regular call. Under real Jest that array has length 1 (containingundefined), so running this file in Jest will fail.
Note that pre-PR, instances was always empty, which was also divergent from Jest — so the C++ change here is a strict improvement. The issue is that it goes only halfway, and the accompanying test codifies the remaining divergence as expected behavior.
How to fix
Either:
- Match jest-mock fully: move the
instancespush out of theif constexpr (isConstructCall)block and pushthisValueunconditionally (just likecontexts). Then change the last test to expecttoHaveLength(1)/[undefined]. - Or, keep the current C++ but fix the test file: wrap "calling as a function does not populate instances" in
if (isBun) { … }so the file remains green when run under Jest/Vitest, consistent with how other Bun-specific behavior in this file is gated.
1 participant
What
Reflect.construct(jest.fn(), [])(and any[[Construct]]that goes through the C++Interpreter::executeConstructpath) tripped a debug assertion in JSC:JSMockFunctionregistered the same native entry point for[[Call]]and[[Construct]], and that entry point returns primitives in several cases —undefinedwhen there's no implementation, themockReturnValue, or the implementation's return value.executeConstructunconditionally callsasObject()on the result of a native construct, so a primitive result aborts.How
Split the native entry point into separate call and construct variants via a
booltemplate parameter. On construct:thisobject usingnew.target's prototype (viaInternalFunction::createSubclassStructure, falling back to the plain object structure).mock.instancesand use it as thethispassed to the implementation and stored inmock.contexts.thisinstead — standard[[Construct]]semantics.Regular calls are unchanged.
As a side effect,
mock.instancesis now populated when a mock is used as a constructor (it was always empty before), matching Jest.Repro