Repository navigation
Conversation
A native [[Construct]] implementation must return a JSObject. The mock function reused its call handler for construct, so `new mockFn()` returned the implementation's result — which may be a primitive or undefined. The op_construct bytecode path tolerated that, but constructing through `Reflect.construct` / the C++ `JSC::construct` path hit `asObject()` on a non-object in `Interpreter::executeConstruct` and crashed. Give the mock a dedicated construct handler that enforces the contract: if the implementation returns a non-object, return a freshly constructed instance derived from the new.target, matching ordinary `new aFunctionReturningAPrimitive()` semantics.
|
Warning Review limit reached
More reviews will be available in 20 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:38 PM PT - May 30th, 2026
❌ @robobun, your commit 2afae3b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31621That installs a local version of the PR into your bun-31621 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #31386, which is the same fix but more complete: it also records the constructed instance in |
![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; the two inline nits are valid follow-ups but don't block.
Extended reasoning...
Overview
This PR fixes a SIGSEGV / assertion failure when a jest.fn() mock is constructed via Reflect.construct (or any path through JSC::construct). The mock previously reused its call handler for construct, which could return a primitive; JSC's native [[Construct]] contract requires an object, and Interpreter::executeConstruct ends in asObject(result), which crashes on non-cells. The fix adds a small dedicated construct handler (~20 lines) that delegates to the existing call/record logic and, if the result is not an object, allocates a fresh instance derived from newTarget. A portable regression test is added.
Files touched: src/jsc/bindings/JSMockFunction.cpp (declare + wire + define one host function) and test/js/bun/test/mock-fn.test.js (one new test).
Security risks
None. This is test-utility code (jest.fn() / bun:test mocks), not exposed to untrusted input in any meaningful sense. The new path uses the same JSC primitives (createSubclassStructure, constructEmptyObject) as a dozen other construct handlers in src/jsc/bindings/. Exception scope handling is correct (RETURN_IF_EXCEPTION after the delegated call and after createSubclassStructure). No new heap-stored fields, so no GC visitor changes needed.
Level of scrutiny
Low-to-medium. It's C++ in JSC bindings, but the change is tiny, mechanical, and a strict improvement over the prior behavior (crash / returning undefined from new). It's not in a CODEOWNER-covered path, and the existing 73-test mock suite plus the new regression test cover it.
Other factors
The bug-hunter raised two nits, both of which I agree are non-blocking:
getFunctionRealm(newTarget)for the fallback base structure — a cross-realm-newTarget-with-non-object-.prototypeedge case in a test utility; worth matching the rest of the directory but trivially deferrable.- Instance is allocated after the impl runs, so
this.x = …mutations are lost andmock.instancesstays empty. This is a real Jest-compat gap in the new path, but not a regression (construct never worked at all before), and threading a pre-allocatedthisthroughjsMockFunctionCallis a larger refactor that's reasonable as a follow-up.
Neither changes the verdict: the PR's stated goal is to stop the crash and satisfy the native [[Construct]] contract, and it does that correctly.
| JSObject* newTarget = asObject(callframe->newTarget()); | ||
| Structure* structure = JSC::InternalFunction::createSubclassStructure(globalObject, newTarget, globalObject->objectStructureForObjectConstructor()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| return JSValue::encode(JSC::constructEmptyObject(vm, structure)); |
There was a problem hiding this comment.
🟡 The fallback base structure passed to createSubclassStructure comes from the lexical globalObject directly; per OrdinaryCreateFromConstructor it should come from getFunctionRealm(globalObject, newTarget) instead. Every other construct handler in src/jsc/bindings/ (e.g. NapiClass.cpp:52-55, JSX509Certificate.cpp, NodeVMScript.cpp, JSHash.cpp, JSDOMFile.cpp) follows that pattern — it only matters for the cross-realm-newTarget-with-non-object-.prototype edge case, but it's a two-line change to stay consistent.
Extended reasoning...
What's diverging
jsMockFunctionConstruct builds the fallback instance like this:
JSObject* newTarget = asObject(callframe->newTarget());
Structure* structure = JSC::InternalFunction::createSubclassStructure(globalObject, newTarget, globalObject->objectStructureForObjectConstructor());The third argument — the base structure — is taken from the lexical globalObject. createSubclassStructure only consults this base when newTarget.prototype is not an object; in that case the constructed instance's [[Prototype]] falls back to the base structure's prototype. ECMA-262 OrdinaryCreateFromConstructor step 2 / GetPrototypeFromConstructor says that fallback prototype must come from GetFunctionRealm(newTarget), not the caller's realm.
Established codebase pattern
Every other native construct handler in src/jsc/bindings/ that calls createSubclassStructure resolves the realm first. NapiClass.cpp:52-55 (which uses the exact same objectStructureForObjectConstructor() base and explicitly cites JSC's ObjectConstructor.cpp as the reference):
JSGlobalObject* functionGlobalObject = getFunctionRealm(globalObject, asObject(newTarget));
RETURN_IF_EXCEPTION(scope, {});
Structure* baseStructure = functionGlobalObject->objectStructureForObjectConstructor();
Structure* objectStructure = InternalFunction::createSubclassStructure(globalObject, asObject(newTarget), baseStructure);The same shape appears in JSX509Certificate.cpp:188-190, NodeVMScript.cpp:122-124, JSDOMFile.cpp:78-80, NodeFSStatBinding.cpp:830-832, JSBakeResponse.cpp:207-209, JSSign.cpp:504-506, JSHash.cpp:303-305, JSHmac.cpp:249-251, NodeFSStatFSBinding.cpp:374-376, JSDiffieHellmanGroupConstructor.cpp:58-60, and JSDOMWrapperCache.h:222-226. This PR's new handler is the only one that skips it.
Step-by-step trigger
- Create a mock in realm A:
const m = jest.fn(); - Obtain a constructor
Cfrom realm B (e.g. vianode:vm/ ShadowRealm) and clobber its prototype:C.prototype = null; const inst = Reflect.construct(m, [], C);jsMockFunctionConstructruns with lexicalglobalObject= realm A andnewTarget=C(realm B).- The mock has no implementation → returns
undefined→ not an object → fallback path runs. createSubclassStructurereadsC.prototype, finds it's not an object, and uses the supplied base structure — realm A'sobjectStructureForObjectConstructor().insttherefore inherits from realm A'sObject.prototype. Spec (and the rest of the codebase) says it should inherit from realm B'sObject.prototype.
Why existing code doesn't prevent it
createSubclassStructure itself does not call getFunctionRealm; it trusts the base structure handed in. When newTarget.prototype is an object (the overwhelmingly common case) the base is ignored entirely, so this only surfaces in the non-object-prototype branch — which is exactly the branch the spec defines the realm rule for.
Impact / addressing the objection
One verifier argued this is not worth flagging because the trigger — cross-realm Reflect.construct of a jest.fn() mock with a newTarget whose .prototype is not an object — is implausible inside a unit test. That's fair: this is a test utility, not a web-platform object with cross-realm conformance requirements, and no user is likely to ever hit it. That's why this is filed as a nit, not a blocker. The reason it's still worth the two-line change is consistency: this is brand-new code that diverges from a pattern applied uniformly across a dozen other construct handlers in the same directory, and matching it costs effectively nothing.
Suggested fix
JSObject* newTarget = asObject(callframe->newTarget());
auto* functionGlobalObject = getFunctionRealm(globalObject, newTarget);
RETURN_IF_EXCEPTION(scope, {});
Structure* structure = JSC::InternalFunction::createSubclassStructure(globalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor());
RETURN_IF_EXCEPTION(scope, {});| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * globalObject, CallFrame* callframe)) | ||
| { | ||
| auto& vm = JSC::getVM(globalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
|
|
||
| EncodedJSValue encodedResult = jsMockFunctionCall(globalObject, callframe); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| // A native [[Construct]] implementation must return an object. The mock's | ||
| // implementation may return a primitive (or undefined), so fall back to a | ||
| // freshly constructed instance in that case, matching ordinary | ||
| // `new aFunctionReturningAPrimitive()` semantics and avoiding a crash when | ||
| // invoked via Reflect.construct / the C++ construct path. | ||
| JSValue result = JSValue::decode(encodedResult); | ||
| if (result.isObject()) | ||
| return encodedResult; | ||
|
|
||
| JSObject* newTarget = asObject(callframe->newTarget()); | ||
| Structure* structure = JSC::InternalFunction::createSubclassStructure(globalObject, newTarget, globalObject->objectStructureForObjectConstructor()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| return JSValue::encode(JSC::constructEmptyObject(vm, structure)); | ||
| } |
There was a problem hiding this comment.
🟡 The new construct handler creates the fresh instance after running the implementation, so this-mutations are lost: new (jest.fn(function(){ this.x = 5 }))().x is undefined here but 5 in Jest. Allocating the instance up front and passing it as thisValue would fix this — and would also be the natural place to push it onto fn->instances (which is wired up via getInstances() but never written to, so mock.instances is always []). Not a regression — the construct path never worked before — so feel free to defer to a follow-up, but the comment's claim of "ordinary new aFunctionReturningAPrimitive() semantics" isn't quite accurate as-is.
Extended reasoning...
What the bug is
jsMockFunctionConstruct invokes the implementation first (by delegating to jsMockFunctionCall, which calls Bun::call(globalObject, result, callData, thisValue, args) with thisValue = callframe->thisValue()), and only afterward — if the result is not an object — allocates a fresh instance via constructEmptyObject(vm, structure). For a native InternalFunction constructor in JSC, no this object is pre-allocated by the engine; callframe->thisValue() is not the freshly-constructed instance. So any this.x = … assignments in the implementation land on whatever thisValue() happened to be, and the brand-new object returned at the end never sees them.
The same root cause means the constructed instance is never pushed to fn->instances. The instances array is fully wired up — declared as a WriteBarrier<JSArray>, exposed via getInstances() at mockObjectStructure offset 2, cleared in clear(), visited in visitAdditionalChildrenInGCThread — but nothing in jsMockFunctionCall or jsMockFunctionConstruct ever writes to it, so mock.instances is always [].
Why existing code doesn't prevent it
The comment at lines 993–997 says this matches "ordinary new aFunctionReturningAPrimitive() semantics", but that's only half of the contract. In spec [[Construct]] (and in Jest, where the mock is a plain JS function that does mockImpl.apply(this, arguments)), the engine creates the this object before the body runs, the body mutates it, and that same object is what gets returned when the body's return value is non-object. Here the order is inverted: call → check → allocate. The crash is fixed, but the semantics diverge from both the spec step the comment cites and from Jest.
Step-by-step proof
const m = jest.fn(function () { this.x = 5; });
const inst = new m();jsMockFunctionConstructis entered;callframe->thisValue()is whatever JSC passes for a native construct (not a fresh instance derived fromnewTarget.prototype).- It calls
jsMockFunctionCall, which reaches theJSMockImplementation::Kind::Callbranch and doesBun::call(..., thisValue, args). The implementation runsthis.x = 5on that unrelatedthisValueand returnsundefined. - Back in
jsMockFunctionConstruct,result.isObject()is false, so it allocatesconstructEmptyObject(vm, structure)and returns it. instis the empty object from step 3 —inst.xisundefined. In Jest,inst.x === 5.- Nothing pushed to
fn->instances, som.mock.instancesis[]. In Jest it is[inst].
Impact
This is not a regression — before this PR new m() returned undefined (so .x threw) and Reflect.construct crashed outright; the PR strictly improves things. But it is a real Jest-compat gap in the code path this PR introduces, and the inline comment overstates the spec match. Tests that mock a constructor with jest.fn(function(){ this.field = … }) and inspect the returned instance, or that assert on mock.instances, will still fail under Bun.
The mock.instances half is pre-existing — fn->instances was never populated before this PR either (and in Jest it's actually pushed on every invocation, not just construct). It's mentioned here only because the fix for the this-mutation issue is exactly where it would be addressed.
How to fix
Allocate the instance up front and pass it through to the implementation, e.g.:
JSObject* newTarget = asObject(callframe->newTarget());
Structure* structure = InternalFunction::createSubclassStructure(globalObject, newTarget,
globalObject->objectStructureForObjectConstructor());
RETURN_IF_EXCEPTION(scope, {});
JSObject* instance = constructEmptyObject(vm, structure);
// push to fn->instances here (and ideally also record it in mock.contexts as 'this')
// invoke the recording/implementation logic with 'instance' as thisValue
// (either thread an explicit thisValue into jsMockFunctionCall, or inline the
// relevant parts here)
JSValue result = JSValue::decode(encodedResult);
return JSValue::encode(result.isObject() ? result.getObject() : instance);This requires threading an explicit thisValue into the shared call/record path, so it's a slightly larger change than the current crash fix — entirely reasonable to do as a follow-up.
What
Constructing a
jest.fn()/Bun.jest().mock()mock function viaReflect.construct(or any code path that goes through the C++JSC::constructhelper) could crash the process.Minimal repro:
In a debug/ASAN build this trips
ASSERTION FAILED: cell->isObjectSlow()inJSC::asObject. It was originally surfaced by a fuzzer as a flakySIGSEGV.Why
A native
[[Construct]]implementation in JavaScriptCore must return aJSObject. The mock reused its call handler (jsMockFunctionCall) for construct, so constructing a mock returned the implementation's result — which is frequently a primitive orundefined(e.g.jest.fn()with no implementation returnsundefined;jest.fn(() => 5)returns5).op_constructbytecode path (new m()) tolerated a non-object return, sonew m()merely returned the raw (often primitive) value — already not spec-correct.Reflect.construct/JSC::constructgo throughInterpreter::executeConstruct, which ends inreturn asObject(JSValue::decode(result)). With a non-object result this dereferences a non-cell value and crashes.Fix
Give the mock a dedicated construct handler. It runs the same recording + implementation logic, then enforces the native
[[Construct]]contract: if the implementation's result is not an object, it returns a freshly constructed instance derived from thenew.target's prototype — matching ordinarynew aFunctionReturningAPrimitive()semantics. When the implementation returns an object, that object is passed through unchanged.This also makes
new mockFn()spec-correct (it now returns an object instance instead of a primitive).Test
Added a portable (Jest/Vitest/Bun) regression test in
test/js/bun/test/mock-fn.test.jscoveringnew,Reflect.construct(m, []), an explicitnewTarget, and the primitive-vs-object implementation-return cases. It fails on the released build (typeof new jest.fn()was"undefined") and passes with this change. The existing mock/spyOn suite (73 tests) still passes.