Fix crash when using Reflect.construct on a mock function#29790
Fix crash when using Reflect.construct on a mock function#29790robobun wants to merge 2 commits into
Conversation
JSC requires native constructors to return an object. jsMockFunctionCall was used for both [[Call]] and [[Construct]], but it returns jsUndefined() (or whatever primitive the mock implementation returns). When invoked via Reflect.construct, the interpreter's executeConstruct() passes the result through asObject(), which asserts isCell() on debug builds. Split out a dedicated construct handler that falls back to a fresh object when the mock would otherwise produce a non-object, matching ordinary [[Construct]] semantics.
|
Updated 3:06 PM PT - Apr 27th, 2026
❌ @robobun, your commit 2877eb3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29790That installs a local version of the PR into your bun-29790 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a dedicated construct handler Changes
🚥 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/JSMockFunction.cpp`:
- Around line 985-998: The construct fallback in jsMockFunctionConstruct
currently returns a plain object via constructEmptyObject which ignores
newTarget.prototype; update the fallback to create an instance using the target
function's subclass structure so subclassing and Reflect.construct(...,
newTarget) work correctly: after decoding result from jsMockFunctionCall in
jsMockFunctionConstruct, if the result is not an object obtain the appropriate
structure using InternalFunction::createSubclassStructure with the newTarget
(from callframe->newTarget or equivalent) and then allocate/initialize the
object from that structure instead of calling constructEmptyObject; ensure
exceptions and the existing RETURN_IF_EXCEPTION/RELEASE_AND_RETURN scope
handling remain intact.
🪄 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: cb59c3ff-579d-4029-82a5-e247b3d1c5ab
📒 Files selected for processing (2)
src/bun.js/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward crash fix that wraps the construct path to guarantee an object return; the two inline nits are non-blocking.
Extended reasoning...
Overview
This PR fixes a fuzzer-found assertion failure when Reflect.construct is used on a jest.fn() / spyOn() mock. JSMockFunction previously passed jsMockFunctionCall as both the call and construct native functions, but jsMockFunctionCall can return primitives (e.g. jsUndefined() for an empty mock), which violates JSC's invariant that native constructors must return an object — Interpreter::executeConstruct runs the result straight through asObject() and asserts. The fix adds a thin jsMockFunctionConstruct wrapper (~13 lines) that delegates to jsMockFunctionCall, propagates exceptions, and falls back to constructEmptyObject(globalObject) when the result is not an object. A regression test in mock-fn.test.js covers empty mocks, mockReturnValue(primitive), an object-returning implementation, and a property spy.
Security risks
None. This is test-runner mock infrastructure; no auth, crypto, network, or untrusted-input handling is involved. The change strictly tightens behavior (crash → defined object return) and uses standard JSC scope/exception macros (DECLARE_THROW_SCOPE, RETURN_IF_EXCEPTION, RELEASE_AND_RETURN).
Level of scrutiny
Low-to-moderate. The C++ change is small, mechanical, and follows the exact pattern used throughout the file and JSC host functions. It only adds a new code path for [[Construct]]; the existing [[Call]] path is untouched. The result-is-object check plus empty-object fallback is the canonical way native constructors satisfy the object-return invariant.
Other factors
Two inline nits were filed and I agree they are non-blocking: (1) the fallback ignores newTarget.prototype, so Reflect.construct(jest.fn(), [], SomeClass) won't be instanceof SomeClass — a parity gap, but a strict improvement over crashing and easy to refine later via InternalFunction::createSubclassStructure; (2) the new test's spyOn({foo:42}, "foo") line isn't guarded by if (isBun), which breaks the file's stated Jest/Vitest cross-runner compatibility convention — test-only, doesn't affect Bun CI. Neither warrants holding up a crash fix. Tests pass per the PR description (73 pass / 0 fail), and the diff is self-contained with no design decisions a human needs to weigh in on.
Use InternalFunction::createSubclassStructure so that Reflect.construct(mock, [], SomeCtor) produces an object that inherits from SomeCtor.prototype, matching ordinary [[Construct]] semantics. Also guard the spyOn-on-non-function assertion behind isBun since Jest and Vitest reject that form.
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) | ||
| { | ||
| auto& vm = JSC::getVM(lexicalGlobalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
|
|
||
| EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| JSValue result = JSValue::decode(encodedResult); | ||
| if (result.isObject()) [[likely]] | ||
| return encodedResult; | ||
|
|
||
| JSObject* newTarget = asObject(callframe->newTarget()); | ||
| JSGlobalObject* functionGlobalObject = getFunctionRealm(lexicalGlobalObject, newTarget); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| Structure* structure = InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| RELEASE_AND_RETURN(scope, JSValue::encode(JSC::constructEmptyObject(vm, structure))); | ||
| } |
There was a problem hiding this comment.
🟡 Nit: the fallback object is allocated after the implementation runs (which is invoked as a plain [[Call]] with callframe->thisValue()), so for new (jest.fn(function(){ this.x = 42 }))() the this.x = 42 lands on the wrong object and the returned instance is an empty {} — Jest yields {x: 42}. Not blocking — this is still a strict improvement over the crash — but the "matching ordinary [[Construct]] semantics" claim doesn't hold for the constructor‑assigns‑to‑this pattern; a fuller fix would create the receiver from newTarget.prototype first, pass it as this when invoking the impl, and return it on a non‑object result.
Extended reasoning...
What the bug is
jsMockFunctionConstruct delegates to jsMockFunctionCall with the original callframe, and only after that call returns does it allocate the fallback object via constructEmptyObject(vm, structure). Inside jsMockFunctionCall, the implementation is invoked as
JSValue returnValue = Bun::call(globalObject, result, callData, thisValue, args);
// where thisValue = callframe->thisValue()i.e. a plain [[Call]] with whatever the native construct frame's thisValue happens to be — not a freshly‑allocated receiver derived from newTarget.prototype. So when the mock body assigns to this, those mutations land on the wrong object, and the brand‑new fallback object created at line 1002 has none of them.
In ordinary ECMAScript [[Construct]] (and in Jest, where jest.fn() is a real JS function), the engine performs OrdinaryCreateFromConstructor(newTarget, "%Object.prototype%") before the body runs, the body executes with that object as this, and that same object is returned when the body's completion value is not an object.
Step‑by‑step proof
const m = jest.fn(function () { this.x = 42; });
const inst = new m();| Step | Jest (ordinary JS function) | Bun after this PR |
|---|---|---|
1. Create receiver from newTarget.prototype |
{} (proto = m.prototype) |
— (skipped) |
2. Run body with that receiver as this |
sets receiver.x = 42 |
runs Bun::call(impl, callframe->thisValue(), …); this.x = 42 lands on the construct frame's thisValue, not the eventual return value |
3. Body returns undefined (non‑object) |
return the receiver | result.isObject() is false → allocate a new constructEmptyObject(vm, structure) |
4. inst.x |
42 |
undefined |
A secondary consequence of the same ordering: createSubclassStructure reads newTarget.prototype after jsMockFunctionCall has already consumed any mockReturnValueOnce queue entry and pushed into mock.calls/mock.results. If newTarget.prototype is a throwing getter, the once‑queue is consumed and the call is recorded even though the construct ultimately throws — whereas spec/Jest read the prototype first, before the body ever runs.
Why nothing prevents it
jsMockFunctionConstruct never sets up a receiver before delegating; it just forwards the same callframe. jsMockFunctionCall has no construct‑aware path — it always passes callframe->thisValue() straight through to Bun::call. The createSubclassStructure work added in 2877eb3 fixed the prototype chain of the fallback (so Reflect.construct(empty, [], Base) instanceof Base), but didn't change when the object is created or what this the body sees.
Impact
Low. Before this PR the same path crashed (asObject(undefined) assertion), so any behavior here is a strict improvement, not a regression. The observable divergence affects the recognizable "mock a constructor that assigns to this" pattern (jest.fn(function(){ this.foo = … }) used with new), where Bun returns an empty object instead of the populated one Jest returns. This is distinct from the already‑addressed CodeRabbit/Claude comments on this PR, which only covered the newTarget.prototype chain.
How to fix (follow‑up)
Allocate the receiver up front and thread it through the implementation call, roughly:
JSObject* newTarget = asObject(callframe->newTarget());
auto* fnGlobal = getFunctionRealm(lexicalGlobalObject, newTarget);
RETURN_IF_EXCEPTION(scope, {});
Structure* structure = InternalFunction::createSubclassStructure(
lexicalGlobalObject, newTarget, fnGlobal->objectStructureForObjectConstructor());
RETURN_IF_EXCEPTION(scope, {});
JSObject* receiver = constructEmptyObject(vm, structure);
// invoke the mock with `receiver` as thisValue (requires plumbing into jsMockFunctionCall
// or factoring out a helper that takes an explicit thisValue)
return result.isObject() ? encodedResult : JSValue::encode(receiver);That's a larger refactor than this crash fix targets, so flagging as a known limitation / follow‑up rather than a blocker.
| JSObject* newTarget = asObject(callframe->newTarget()); | ||
| JSGlobalObject* functionGlobalObject = getFunctionRealm(lexicalGlobalObject, newTarget); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| Structure* structure = InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, functionGlobalObject->objectStructureForObjectConstructor()); |
There was a problem hiding this comment.
is that createSubclassStructure correct? shouldn't it be based on the mock functions structure or no?
There was a problem hiding this comment.
Yes, I believe so — the third argument to createSubclassStructure is the base structure for the instances this constructor produces, not the structure of the callee. It's used as:
- The fallback when
newTarget.prototypeisn't an object - The class-info/shape template even when
newTarget.prototypeis an object (the result is a structure with that prototype but the base's cell type)
A mock function acting as a constructor is emulating an ordinary user-defined function constructor, so its instances should be plain JSFinalObjects — there's no "mock instance" cell type. objectStructureForObjectConstructor() is the plain-object structure, which matches:
ObjectConstructor.cpp—new Object()itselfNapiClass.cpp:54-55— same pattern for N-API classes with a foreignnewTarget
JSMockFunction also doesn't expose a .prototype property (jest.fn().prototype === undefined), so when newTarget is the mock itself, createSubclassStructure falls back to the base structure → plain {} with Object.prototype, which is the OrdinaryCreateFromConstructor default. When newTarget is something else (Reflect.construct(mock, [], SomeClass)), it correctly picks up SomeClass.prototype.
|
Superseded by #30212, which additionally creates the |
2 participants
What does this PR do?
Fixes an assertion failure / crash when a
jest.fn()/spyOn()mock is invoked viaReflect.construct()and its implementation would return a non-object (e.g. no implementation,mockReturnValue(42), or a spy on a non-function property).JSC requires native constructors to return an object —
Interpreter::executeConstructpasses the result straight throughasObject().JSMockFunctionwas using the same host function for both[[Call]]and[[Construct]], which can returnjsUndefined()or other primitives.This adds a dedicated construct handler that falls back to a fresh empty object when the mock would otherwise produce a non-object, matching ordinary
[[Construct]]semantics (and Jest's behavior).How did you verify your code works?
test/js/bun/test/mock-fn.test.jscovering empty mocks,mockReturnValuewith a primitive, an implementation that returns an object, andspyOnof a non-function property.bun bd test test/js/bun/test/mock-fn.test.js— 73 pass, 0 failmain(assertion failure on debug, wrong return type on release).Found by Fuzzilli. Fingerprint:
JSCJSValue.h(1044)