Repository navigation
Conversation
JSMockFunction used the same native function for both call and construct. When invoked via Reflect.construct with no implementation, it returned undefined, and JSC::Interpreter::executeConstruct then called asObject() on that value, tripping the isCell() assertion. Give JSMockFunction its own construct handler that allocates an instance, records it in mock.instances, passes it as this to the shared call path, and returns it whenever the implementation does not produce an object. This also makes mock.instances actually track constructed objects like Jest does.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesMock function construction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 991-995: Update the mock instance construction logic near the
construct frame to use the construct call’s newTarget when selecting the
prototype, preserving the existing fallback for non-object prototypes and
ensuring Reflect.construct(fn, ..., Subclass) inherits from NewTarget.prototype;
add a regression test in test/js/bun/test/mock-fn.test.js covering this
Reflect.construct or subclass behavior.
In `@test/js/bun/test/mock-fn.test.js`:
- Around line 807-826: Extend the “Reflect.construct on a mock returns an object
and records the instance” test with a non-default newTarget case, using a
distinct constructor or subclass whose prototype is observable. Call
Reflect.construct with that newTarget and assert the returned instance inherits
from NewTarget.prototype while preserving the mock’s recording expectations.
🪄 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: 0251daf0-e5c5-4bfa-b499-a850b386f239
📒 Files selected for processing (2)
src/jsc/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
|
Updated to look up |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #34575, which fixes the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JSValue prototype = fn->get(globalObject, vm.propertyNames->prototype); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSObject* instance = prototype.isObject() | ||
| ? JSC::constructEmptyObject(globalObject, asObject(prototype)) | ||
| : JSC::constructEmptyObject(globalObject); |
There was a problem hiding this comment.
🟡 The instance's [[Prototype]] is derived from fn (the callee) rather than callframe->newTarget(). Per ES OrdinaryCreateFromConstructor semantics — and matching every other InternalFunction construct handler in this directory (JSDOMFile.cpp, JSX509Certificate.cpp, NodeFSStatBinding.cpp, etc.) — the prototype lookup should happen on newTarget, so Reflect.construct(jest.fn(), [], C) should yield an instance whose prototype is C.prototype. In Jest, mocks are plain JS functions and honor newTarget naturally.
Extended reasoning...
What the bug is
jsMockFunctionConstruct allocates the newly-constructed instance by reading .prototype from callframe->jsCallee():
JSMockFunction* fn = dynamicDowncast<JSMockFunction>(callframe->jsCallee());
...
JSValue prototype = fn->get(globalObject, vm.propertyNames->prototype);Per the ES [[Construct]] internal method (specifically OrdinaryCreateFromConstructor), the [[Prototype]] of the new instance is derived from newTarget.prototype, not from the directly-called function. When invoked as new fn() these are the same object, but when invoked via Reflect.construct(fn, args, NewTarget) or via super() in a subclass, newTarget differs from the callee and must be the source of the prototype.
Code path that triggers it
class C {}
const m = jest.fn();
const x = Reflect.construct(m, [], C);
Object.getPrototypeOf(x) === C.prototype; // should be trueStep-by-step:
Reflect.construct(m, [], C)invokesm.[[Construct]]([], C). JSC dispatches tojsMockFunctionConstructwithcallframe->jsCallee() == mandcallframe->newTarget() == C.- The handler reads
m->get(globalObject, vm.propertyNames->prototype).JSMockFunctionis anInternalFunctionwith no own.prototypeproperty, so this returnsundefined. prototype.isObject()is false, so the handler falls through toconstructEmptyObject(globalObject), producing an instance whose[[Prototype]]isObject.prototype.Object.getPrototypeOf(x) === C.prototypeevaluates tofalse.
In Jest, jest.fn() returns a plain JS function, so the engine's ordinary [[Construct]] runs OrdinaryCreateFromConstructor(newTarget, ...) and the same expression evaluates to true.
Why existing code doesn't prevent it
Nothing in the new handler consults callframe->newTarget(). The common case new fn() happens to work only because newTarget === callee there. Every other native construct handler in src/jsc/bindings/ (e.g. JSDOMFile.cpp:70, JSX509Certificate.cpp:181, NodeFSStatBinding.cpp:824, JSStringDecoder.cpp, NodeVMScript.cpp) reads callFrame->newTarget() for exactly this reason.
Impact
Observable divergence from both the ES spec and Jest, but only on the 3-arg Reflect.construct / subclass-super() path with a mock as the target — an uncommon shape. It is not a regression: before this PR, constructing a mock either asserted (debug) or returned undefined (release). The primary crash fix and mock.instances tracking are unaffected.
Fix
Read the prototype from newTarget instead of the callee:
JSObject* newTarget = asObject(callframe->newTarget());
JSValue prototype = newTarget->get(globalObject, vm.propertyNames->prototype);
RETURN_IF_EXCEPTION(scope, {});(or equivalently use InternalFunction::createSubclassStructure / the pattern from the neighboring handlers).
Fuzzer hit the
isCell()assertion inJSCJSValue.hwith:JSMockFunctionregisteredjsMockFunctionCallfor both call and construct. With no implementation that function returnsjsUndefined(), andJSC::Interpreter::executeConstructthen doesasObject()on the result (native constructors must return an object). In release builds this surfaced asReflect.construct(jest.fn(), [])evaluating toundefined.This adds a dedicated construct handler that allocates the instance, records it in
mock.instances, hands it to the shared call path asthis, and returns it whenever the implementation does not produce an object. As a side effectmock.instancesnow actually tracks constructed objects like Jest does (it was always empty before).