Fix assertion failure constructing jest.fn() via Reflect.construct - #32667
Fix assertion failure constructing jest.fn() via Reflect.construct#32667robobun wants to merge 1 commit into
Conversation
JSMockFunction used the same native function for both call and construct. When constructed with no implementation (or one that returns a primitive), it returned jsUndefined(), which trips the asObject() assertion in Interpreter::executeConstruct. Add a dedicated construct path that allocates a this object up front and returns it when the implementation does not produce an object, matching standard [[Construct]] semantics.
|
Updated 8:09 AM PT - Jun 24th, 2026
❌ @robobun, your commit c593043 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32667That installs a local version of the PR into your bun-32667 --bun |
Walkthrough
ChangesJSMockFunction construct support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 995-999: The construct path in JSMockFunction::construct currently
creates thisObject and passes it through jsMockFunctionCall, but does not record
the constructed instance in fn->instances, leaving mock.mock.instances stale for
new mock() calls. Update the construct flow to append thisObject to the mock
instances collection before delegating to the shared call logic, keeping the
existing jsMockFunctionCall and callframe setup intact.
In `@test/js/bun/test/mock-fn.test.js`:
- Around line 800-809: The construction checks in the mock function tests only
verify typeof on the results from Reflect.construct and new calls, which still
passes for null. Update the assertions around noImpl, primitiveImpl, and
returnsPrimitive to first capture the constructed values, then assert each value
is not null before confirming it is an object/has the expected prototype shape;
use the existing jest.fn mock names to locate the affected cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4eb4ade-fd40-4521-85d2-0735a5bbf72d
📒 Files selected for processing (2)
src/jsc/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
| JSObject* thisObject = constructEmptyObject(vm, structure); | ||
| callframe->setThisValue(thisObject); | ||
|
|
||
| EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | ||
| RETURN_IF_EXCEPTION(scope, {}); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Record constructed mocks in mock.instances.
The new construct path creates thisObject and records it as the call context via jsMockFunctionCall, but never appends it to fn->instances, so new mock() still leaves mock.mock.instances stale. Add the instance update before delegating to the shared call logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jsc/bindings/JSMockFunction.cpp` around lines 995 - 999, The construct
path in JSMockFunction::construct currently creates thisObject and passes it
through jsMockFunctionCall, but does not record the constructed instance in
fn->instances, leaving mock.mock.instances stale for new mock() calls. Update
the construct flow to append thisObject to the mock instances collection before
delegating to the shared call logic, keeping the existing jsMockFunctionCall and
callframe setup intact.
| expect(typeof Reflect.construct(noImpl, [])).toBe("object"); | ||
| expect(typeof new noImpl()).toBe("object"); | ||
|
|
||
| const primitiveImpl = jest.fn(() => 42); | ||
| expect(typeof Reflect.construct(primitiveImpl, [])).toBe("object"); | ||
| expect(typeof new primitiveImpl()).toBe("object"); | ||
|
|
||
| const returnsPrimitive = jest.fn().mockReturnValue(42); | ||
| expect(typeof Reflect.construct(returnsPrimitive, [])).toBe("object"); | ||
| expect(typeof new returnsPrimitive()).toBe("object"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert a non-null constructed object.
typeof ... === "object" also passes for null, which is still an invalid construct result. Capture each constructed value and assert it is not null before checking the object shape/prototype.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/test/mock-fn.test.js` around lines 800 - 809, The construction
checks in the mock function tests only verify typeof on the results from
Reflect.construct and new calls, which still passes for null. Update the
assertions around noImpl, primitiveImpl, and returnsPrimitive to first capture
the constructed values, then assert each value is not null before confirming it
is an object/has the expected prototype shape; use the existing jest.fn mock
names to locate the affected cases.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused crash fix following the same construct pattern as NapiClass, with test coverage.
Extended reasoning...
Overview
This PR fixes a debug-build assertion failure (ASSERTION FAILED: cell->isObjectSlow()) when a jest.fn() mock is invoked as a constructor via Reflect.construct or new. Previously JSMockFunction registered jsMockFunctionCall as both its call and construct entrypoint; when the call path returns undefined (no implementation) or a primitive, JSC's executeConstruct calls asObject() on the result and asserts. The fix adds a dedicated jsMockFunctionConstruct (~20 lines) that allocates a this object via InternalFunction::createSubclassStructure, sets it on the callframe, delegates to the existing call logic, and returns the result if it's an object or the allocated this otherwise — standard [[Construct]] semantics. A new test in mock-fn.test.js exercises Reflect.construct and new against mocks with no impl, primitive-returning impls, mockReturnValue, and object-returning impls, plus verifies plain-call behavior is unchanged.
Security risks
None. This is test-runner-only code (jest.fn() / Bun.jest), not reachable from production app code paths, and touches no auth, crypto, network, or filesystem surfaces. The change strictly tightens behavior (returns an object instead of asserting/returning a primitive).
Level of scrutiny
Low-to-medium. The new function is a near-verbatim copy of the established pattern already used in NapiClass.cpp (getFunctionRealm → createSubclassStructure(..., objectStructureForObjectConstructor()) → constructEmptyObject → callFrame->setThisValue), so the JSC interaction has prior art in this codebase. Exception handling is correct (RETURN_IF_EXCEPTION after each fallible step), and asObject(callframe->newTarget()) is safe because newTarget is guaranteed to be an object on the construct path.
Other factors
The one inline note is explicitly flagged pre-existing and non-blocking: JSMockFunction lacks an own .prototype, so Object.getPrototypeOf(new fn()) falls back to Object.prototype — a Jest-compat gap that predates this PR and is orthogonal to the crash being fixed. The fix was found by Fuzzilli, is small and self-contained, and includes regression tests, so I'm comfortable approving without human review.
| Structure* structure = InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSObject* thisObject = constructEmptyObject(vm, structure); |
There was a problem hiding this comment.
🟣 Note (pre-existing, not blocking): JSMockFunction never installs an own .prototype property, so when newTarget is the mock itself createSubclassStructure reads fn.prototype as undefined and falls back to Object.prototype. The constructed instance therefore has Object.getPrototypeOf(new fn()) === Object.prototype (and new fn() instanceof fn throws), diverging from Jest where mocks are real JS functions. A follow-up adding a per-instance .prototype in JSMockFunction::create() would close the gap; the construct logic here would then work unchanged.
Extended reasoning...
Summary
jsMockFunctionConstruct derives the new instance's structure via InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor()). That helper reads newTarget.prototype and only falls back to the supplied base structure when the property is not an object. JSMockFunction is an InternalFunction and never defines an own .prototype data property — there is no putDirect(vm.propertyNames->prototype, ...) anywhere in this file, no entry for it in JSMockFunctionPrototypeTableValues, and InternalFunction::finishCreation does not auto-create one (only JSFunction does that lazily). So in the common new fn() case where newTarget === fn, fn.prototype is undefined, the fallback path is taken, and the freshly allocated thisObject has %Object.prototype% as its [[Prototype]].
Step-by-step
const fn = jest.fn();— creates aJSMockFunction. No ownprototypeis installed, sofn.prototype === undefined.new fn()entersjsMockFunctionConstructwithnewTarget = fn.createSubclassStructurecallsnewTarget->get(globalObject, vm.propertyNames->prototype)→undefined. Since that's not an object, it returns the base structure (objectStructureForObjectConstructor()).constructEmptyObject(vm, structure)produces a plain object whose[[Prototype]]isObject.prototype.- The mock has no implementation, so
jsMockFunctionCallreturnsundefined,result.isObject()is false, andthisObjectis returned. - Result:
Object.getPrototypeOf(new fn()) === Object.prototype, and(new fn()) instanceof fnthrowsTypeError: instanceof called on an object with an invalid prototype propertybecausefn.prototypeis not an object.
In Jest, mock functions are ordinary JS functions, so each mock has its own .prototype object, Object.getPrototypeOf(new fn()) === fn.prototype, and (new fn()) instanceof fn === true. The new test in this PR only asserts typeof === "object", so it does not exercise this divergence.
Why this is pre-existing / a nit
The construct code added here is spec-correct: OrdinaryCreateFromConstructor mandates falling back to the realm's %Object.prototype% when newTarget.prototype is not an object. The root cause is the pre-existing lack of a per-instance .prototype on JSMockFunction — e.g. ({}) instanceof jest.fn() already threw TypeError before this PR, and jest.fn().prototype was already undefined. This PR simply makes the constructed instance observable for the first time (previously construction asserted/returned undefined), so the prototype-chain divergence is now newly visible, but it was not introduced here. The Reflect.construct(fn, [], SomeClass) case where newTarget is a real class with a .prototype already works correctly with this implementation.
Suggested follow-up
In JSMockFunction::create(), allocate a fresh prototype object and install it:
JSObject* prototype = constructEmptyObject(globalObject);
prototype->putDirect(vm, vm.propertyNames->constructor, function, PropertyAttribute::DontEnum);
function->putDirect(vm, vm.propertyNames->prototype, prototype, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete);Once that exists, createSubclassStructure will pick it up automatically and jsMockFunctionConstruct needs no changes. This is a Jest-compat improvement worth tracking separately; it should not block this crash fix.
1 participant
What
JSMockFunctionusedjsMockFunctionCallas both its call and construct native function. When the mock has no implementation (or the implementation returns a primitive), the function returnsjsUndefined(). JSC'sInterpreter::executeConstructthen callsasObject()on that result and hitsASSERTION FAILED: cell->isObjectSlow()in debug builds (theReflect.constructpath goes throughexecuteConstruct).Minimal repro:
Fix
Add a dedicated
jsMockFunctionConstructthat:thisobject based onnewTargetviaInternalFunction::createSubclassStructurethisThis matches standard
[[Construct]]semantics (and what Jest does) where a constructor that returns a non-object yields the newly created instance.This also fixes
new jest.fn()()andnew jest.fn(() => 42)()previously returningundefined/42instead of an object on the bytecode path.Found by Fuzzilli.