fix(bun:test): return an object when a mock function is constructed with new#31386
fix(bun:test): return an object when a mock function is constructed with new#31386robobun wants to merge 10 commits into
Conversation
…ith new Mock functions registered jsMockFunctionCall as both their call and construct callback, so `new mock()` could return a non-object (undefined when the mock has no implementation, or a primitive return value). JSC requires native construct callbacks to return an object; Reflect.construct() on such a mock hit an isCell() assertion in debug builds and `new mock()` evaluated to undefined. Add a dedicated construct callback that mirrors `new` on an ordinary JS function: create `this` from newTarget.prototype, invoke the mock with it, and return the mock's result only when it is an object, otherwise the created `this`.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds explicit construct support to JSMockFunction: extracts shared call logic into jsMockFunctionCallImpl(thisValue, isConstruct), adds jsMockFunctionConstruct for new invocations, installs a writable ChangesMock function constructor support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 1:25 PM PT - May 31st, 2026
✅ @autofix-ci[bot], your commit 93736e3928db09fb32495f67d4e85f748bfb89a2 passed in 🧪 To try this PR locally: bunx bun-pr 31386That installs a local version of the PR into your bun-31386 --bun |
|
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}
|
CI status for dfc04ef (build #57920): all 72 platform build/test jobs are green on this commit — the only red status is the Buildkite roll-up, caused by a flaky first attempt of The fix itself has been verified against eight Fuzzilli reports of the |
…tors (#31393) ### What does this PR do? Fixes a fuzzer-found crash (`ASSERTION FAILED: isCell()` in `JSCJSValue.h`, `asCell()`) triggered by constructing `process.mainModule._compile`: ```js Reflect.construct(process.mainModule._compile, []); // ASSERTION FAILED: isCell() ``` `Module.runMain`, `Module._resolveFilename`, `module._compile` (in `src/jsc/modules/NodeModuleModule.cpp`) and `performance.now` (in `src/jsc/bindings/webcore/JSPerformance.cpp`) were created with their call function also registered as the **native construct callback**. JSC requires a native construct callback to return an object, but these functions return `undefined`, a string, or a number — so `new fn()` / `Reflect.construct(fn, …)` hit `asObject()` → `ASSERT(isCell())` in debug builds and produce an invalid `JSObject*` in release builds. ### Fix Create these functions without a native constructor (the `JSFunction::create` default, `callHostFunctionAsConstructor`), so constructing them throws a `TypeError` like other native helper functions — matching `performance.now` behavior in browsers/Node, while calling them is unchanged. Related: #31386 fixes the same assertion fingerprint for `bun:test` mock functions; this PR covers the remaining non-mock instances the fuzzer reached. ### How did you verify your code works? - The minimized repros (`Reflect.construct(module._compile, [])`, `new process.mainModule._compile()`, `Reflect.construct(Module._resolveFilename, ["fs"])`, `new Module.runMain()`, `Reflect.construct(performance.now, [])`) assert on current main (debug build) and now throw `TypeError` with this change. - Added regression tests: - `node-module-module > native module functions are not constructors` (`test/js/node/module/node-module-module.test.js`) - `performance.now is not a constructor` (`test/js/web/timers/performance.test.js`) Both fail on an unfixed build and pass with this change; the full files (29 and 7 tests) pass with the debug build, including the existing tests that call `module._compile` / `Module._resolveFilename`.
Populate fn->instances alongside fn->contexts in the shared call helper so `new`/Reflect.construct invocations appear in mock.instances, mirroring jest (which records each call's `this`). Verified identical output against jest for plain construct, object-returning impl, this-mutating impl, and regular non-construct calls.
There was a problem hiding this comment.
Thanks for addressing the mock.instances gap in 3b29284 — that resolves my earlier comment. I'm still deferring to a human since this is a native [[Construct]] callback in JSC bindings with GC/exception-scope interactions, plus a user-visible behavior change (mock.instances is now populated on every call where it was previously always []).
Extended reasoning...
Overview
This PR fixes a fuzzer-found assertion crash (isCell() in asObject()) when constructing a bun:test mock via new / Reflect.construct. It splits jsMockFunctionCall into a shared jsMockFunctionCallImpl(globalObject, callframe, thisValue) helper, registers a new jsMockFunctionConstruct native callback that synthesizes this from newTarget.prototype and applies ordinary [[Construct]] return-value semantics, and (in the latest commit, addressing my prior review) now pushes thisValue onto fn->instances on every invocation to match Jest. Test coverage in mock-fn.test.js exercises no-impl construct, Reflect.construct (with and without explicit newTarget), implementations writing to this, object vs primitive return values, prototype handling, and spyOn constructs.
Security risks
None identified. This is test-runner mock infrastructure; no auth/crypto/permissions surfaces are touched. The previous behavior was strictly worse (debug assert / release UB reinterpreting a non-cell as JSObject*).
Level of scrutiny
Medium-high. While scoped to bun:test mocks, the implementation lives in hand-written JSC bindings: it uses asObject(callframe->newTarget()), nested DECLARE_THROW_SCOPEs across the construct wrapper and the impl, ObjectInitializationScope + tryCreateUninitializedRestricted, and WriteBarrier updates. These patterns are copied verbatim from the adjacent calls/contexts handling and look correct to me, but JSC exception-check and GC discipline are subtle enough that a maintainer familiar with this file should sign off.
Other factors
- My earlier inline comment about
mock.instancesnever being written has been addressed by commit3b292844, which recordsthisValueintoinstanceson every call (matching Jest's actual behavior, not just the construct path). The new tests assertfn.mock.instances[i]accordingly. - This is a user-visible behavior change beyond the crash fix:
mock.instanceswas previously always[]and is now populated on every call. ExistingmockReset/mockCleartests still pass (they only assert emptiness post-clear or reference inequality), but it's worth a maintainer confirming this Jest-parity change is desired in the same PR. - CI was fully green on
dfc04efeacross all 72 platforms;3b292844(theinstancesfollow-up) is a mechanical copy of thecontextsblock and is currently building. - No CODEOWNERS entry covers this path.
|
CI status for 3b29284 (build #59409): the only red job is I already used my one retrigger earlier in this PR's history, so I'm not pushing another |
|
CI update for 3b29284 (build #59409): the earlier This check fails when a stripped release binary grows >0.5 MB vs the latest main build. This PR's only code delta is the Resolution is a maintainer decision — rebase onto current main to refresh the baseline, or add |
…nstruct-return-object
|
Merged latest Re-verified locally after the merge: the fuzzer repro ( |
![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 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 882-894: The helper jsMockFunctionCallImpl currently always
records thisValue into fn->instances, causing regular calls to populate
mock.instances; change the logic so fn->instances is only mutated when invoked
as a constructor. Concretely, add or use an "isConstruct" boolean parameter (or
detect construct-path) in jsMockFunctionCallImpl and move the instances
push/creation code so it runs only when isConstruct is true; leave
jsMockFunctionCall (regular call) calling the helper with isConstruct=false and
jsMockFunctionConstruct calling it with isConstruct=true so that fn->instances
(mock.instances) only contains values produced by the construct path. Ensure you
still use the existing initialization
(JSArray::tryCreateUninitializedRestricted, initializeIndex, fn->instances.set)
when creating the array in the construct case and preserve RETURN_IF_EXCEPTION
behavior.
🪄 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: 8d75d282-b995-4d4a-aa7d-2a3906b38057
📒 Files selected for processing (1)
src/jsc/bindings/JSMockFunction.cpp
mock.instances contains only instances created with new, matching jest; regular calls record this in mock.contexts instead. Thread an isConstruct flag through the shared call helper so the instances array is populated only on the construct path.
InternalFunction doesn't auto-create a .prototype like JSFunction, so jest.fn().prototype was undefined: new fn() instances inherited from Object.prototype and (new fn()) instanceof fn threw a TypeError. Install a fresh writable prototype with a constructor back-reference at creation, so mocks behave like ordinary JS functions under new (matches jest).
Both jest-mock and @vitest/spy push `this` onto mock.instances and mock.contexts unconditionally on every call (no new.target check), so the two arrays stay in lock-step. An earlier commit gated the instances push on construct based on jest's docs, but the runtime implementation records every call; verified against jest 29.7.0 and @vitest/spy. Revert the gating so mock.instances is drop-in compatible, and update the test to assert the real every-call behavior.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/jsc/bindings/JSMockFunction.cpp (1)
837-900:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
mock.instancesconstructor-only.
jsMockFunctionCallImpl()now pushesthisValueintofn->instancesfor every invocation, so plain calls and.call()/.apply()mutatemock.instances. Jest and Vitest both documentmock.instancesas the objects created vianew; per-callthisbelongs inmock.contextsinstead. This also means the new assertion intest/js/bun/test/mock-fn.test.jsLines 177-189 is codifying the wrong contract. (jestjs.io)Suggested fix
-static JSC::EncodedJSValue jsMockFunctionCallImpl(JSGlobalObject* lexicalGlobalObject, CallFrame* callframe, JSValue thisValue) +static JSC::EncodedJSValue jsMockFunctionCallImpl(JSGlobalObject* lexicalGlobalObject, CallFrame* callframe, JSValue thisValue, bool isConstruct) { ... - JSC::JSArray* instances = fn->instances.get(); - if (instances) { - instances->push(globalObject, thisValue); - RETURN_IF_EXCEPTION(scope, {}); - } else { - JSC::ObjectInitializationScope object(vm); - instances = JSC::JSArray::tryCreateUninitializedRestricted( - object, - globalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous), - 1); - instances->initializeIndex(object, 0, thisValue); - fn->instances.set(vm, fn, instances); + if (isConstruct) { + JSC::JSArray* instances = fn->instances.get(); + if (instances) { + instances->push(globalObject, thisValue); + RETURN_IF_EXCEPTION(scope, {}); + } else { + JSC::ObjectInitializationScope object(vm); + instances = JSC::JSArray::tryCreateUninitializedRestricted( + object, + globalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous), + 1); + instances->initializeIndex(object, 0, thisValue); + fn->instances.set(vm, fn, instances); + } } ... } JSC_DEFINE_HOST_FUNCTION(jsMockFunctionCall, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) { - return jsMockFunctionCallImpl(lexicalGlobalObject, callframe, callframe->thisValue()); + return jsMockFunctionCallImpl(lexicalGlobalObject, callframe, callframe->thisValue(), false); } JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) { ... - JSValue returnValue = JSValue::decode(jsMockFunctionCallImpl(lexicalGlobalObject, callframe, thisObject)); + JSValue returnValue = JSValue::decode(jsMockFunctionCallImpl(lexicalGlobalObject, callframe, thisObject, true)); ... }Also applies to: 1005-1026
🤖 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 837 - 900, The code in jsMockFunctionCallImpl is incorrectly pushing per-call thisValue into fn->instances; change the logic so fn->instances is only mutated when the call is a constructor invocation (use the CallFrame constructor check, e.g. callframe->isConstructCall() or equivalent) and for normal calls continue to push thisValue into fn->contexts only; update the identical instances-handling block later in the file (the section around the repeated code at lines ~1005-1026) the same way so instances remains constructor-only and contexts records per-call this values.
🤖 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.
Duplicate comments:
In `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 837-900: The code in jsMockFunctionCallImpl is incorrectly pushing
per-call thisValue into fn->instances; change the logic so fn->instances is only
mutated when the call is a constructor invocation (use the CallFrame constructor
check, e.g. callframe->isConstructCall() or equivalent) and for normal calls
continue to push thisValue into fn->contexts only; update the identical
instances-handling block later in the file (the section around the repeated code
at lines ~1005-1026) the same way so instances remains constructor-only and
contexts records per-call this values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0ef2b14-f721-406b-b3a9-857f0e4f9f62
📒 Files selected for processing (2)
src/jsc/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
There was a problem hiding this comment.
All three of my earlier inline comments have been addressed (mock.instances population, default .prototype install, and Jest-runtime-vs-docs semantics) and the bug hunter found nothing on the final revision — looks correct to me, but ~80 lines of new JSC construct/prototype binding code is worth a quick human pass.
Extended reasoning...
Overview
This PR fixes a fuzzer-found ASSERTION FAILED: isCell() crash when a bun:test mock function is constructed via new / Reflect.construct. It touches two files: src/jsc/bindings/JSMockFunction.cpp (adds a dedicated jsMockFunctionConstruct host callback, refactors the call path into a shared jsMockFunctionCallImpl(thisValue), installs a writable .prototype with a constructor back-ref on every mock, and populates mock.instances on each call to match jest-mock's actual runtime behavior) and test/js/bun/test/mock-fn.test.js (comprehensive coverage of new, Reflect.construct, newTarget.prototype, instanceof, primitive/object return values, mock.instances lock-step with mock.contexts, and spyOn construct cases).
Security risks
None identified. The surface is the bun:test mock-function object, which is test-only infrastructure. The new construct path reads newTarget.prototype (user-controlled but standard [[Construct]] semantics), allocates a plain object, and always returns an object — closing the prior path where a non-cell was reinterpreted as JSObject* in release builds. Exception checks (RETURN_IF_EXCEPTION) are placed after the get() and after the inner call, and the PR description reports BUN_JSC_validateExceptionChecks=1 passes.
Level of scrutiny
Medium. The change is well-scoped and follows existing patterns very closely (the instances push is a verbatim copy of the adjacent contexts push; the .prototype install mirrors how other InternalFunction subclasses in Bun do it). However, it is ~80 lines of new native JSC binding code involving a new construct host callback, throw-scope nesting, and per-mock object allocation in create(). JSC binding correctness (GC, exception propagation) is subtle enough that I'd prefer a human confirm rather than auto-approve.
Other factors
- I previously left three inline comments on earlier revisions (populate
mock.instances; install a default.prototypesoinstanceofworks; match jest-mock's implementation ofmock.instancesrather than its docs). All three were addressed in commits 8149cc3 → b32623a → 6e0babe, and the conversation threads are resolved. - The PR went through a semantic flip-flop on
mock.instances(construct-only vs. every-call) driven by a CodeRabbit comment citing Jest docs; the final state matches Jest/Vitest's actual runtime behavior and the test is written to pass under all three runners, consistent with the file's header contract. - CodeRabbit's latest pass returned no actionable findings; the multi-agent bug hunter found nothing on the final revision.
- CI: test/build jobs are green across platforms; the only red is an unrelated
-no-pielinker warning inbuild-rustjobs. - No CODEOWNERS cover the touched paths.
A bound mock (mock.bind(...)) forwards [[Construct]] to the mock's construct callback; fuzzing rediscovered the isCell() assertion through this path on unfixed builds. Assert the bound construct returns an object and runs the mock's this-mutating implementation.
1 participant
What does this PR do?
Supersedes #30212 (same fix, rebased onto current main with consolidated test coverage).
Fixes a fuzzer-found crash (
ASSERTION FAILED: isCell()inJSCJSValue.h,asCell()) triggered by constructing abun:testmock function.JSMockFunctionregisteredjsMockFunctionCallas both its call and construct callback. A native construct callback must return an object, but the call path can return non-objects:undefinedwhen the mock has no implementation, or whatever primitive the implementation /mockReturnValueproduced. As a result:Reflect.construct(mock(), [])hitasObject()→ASSERT(isCell())in debug builds (and would reinterpret a non-cell as aJSObject*in release builds).new mock()from bytecode evaluated toundefined, violatingnewsemantics.Fix
Register a dedicated
jsMockFunctionConstructcallback that behaves likenewon an ordinary JS function (matching Jest, whose mock constructor is a plain function):thisfromnewTarget.prototype(falling back toObject.prototype),this(somock.calls/mock.contexts/mock.resultsrecord the construct invocation, and implementations can assign tothis),this.The existing call behavior is unchanged;
jsMockFunctionCallnow forwards to a shared implementation that takes thethisvalue explicitly.How did you verify your code works?
new mock()/Reflect.construct(mock(), [])now return objects.mock() > are constructable with newtotest/js/bun/test/mock-fn.test.jscovering no-implementation constructs,Reflect.construct(including an explicitnewTarget), implementations assigning tothis, implementations returning objects, primitive return values, andnewTarget.prototypehandling, plusspyOntests for constructing a spied method and a spy on a missing property. It fails on current bun (new fn()isundefined) and passes with this change.mock-fn.test.js(73 tests),mock-disposable.test.ts, and themock/module-mocking tests pass with the debug build, including withBUN_JSC_validateExceptionChecks=1.