test: make mock functions construct a proper instance#31891
Conversation
JSMockFunction registered its call handler as the [[Construct]] implementation too, so constructing a mock returned whatever the implementation returned, undefined by default. Native constructors must return an object: Reflect.construct(mockFn, []) tripped the asObject() assertion in debug builds and is type confusion in release builds, and new mockFn() evaluated to undefined. Construct now follows ordinary function semantics, matching jest's mockConstructor: create the instance from newTarget's prototype, run the mock with it as this, record it in mock.instances, and return it unless the implementation returned an object.
|
Updated 11:11 AM PT - Jun 5th, 2026
❌ @robobun, your commit 0d8f0e6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31891That installs a local version of the PR into your bun-31891 --bun |
|
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)
WalkthroughThis PR adds constructor semantics to mock functions in Bun's JSC bindings. The 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/jsc/bindings/JSMockFunction.cppIn file included from src/jsc/bindings/JSMockFunction.cpp:1: ... [truncated 2200 characters] ... tend_decl.CFrontend_decl_funct.add_method in file "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52 Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #31386, which I opened earlier for the same fuzzer-found crash and didn't notice before opening this one. #31386 takes the same approach (a dedicated This report (fingerprint |
There was a problem hiding this comment.
I didn't find any issues — the construct handler follows the standard JSC pattern and the instances-array bookkeeping mirrors the existing calls/contexts code — but ~50 lines of new JSC binding C++ that fixes a release-build type confusion is worth a human glance.
Extended reasoning...
Overview
This PR fixes a Fuzzilli-found assertion failure / release-build type confusion reached by constructing a bun:test mock function (Reflect.construct(jest.fn(), [])). The root cause is that JSMockFunction registered jsMockFunctionCall as both the call and construct handler, so [[Construct]] could return undefined, which JSC's Interpreter::executeConstruct then passes to asObject(). The fix extracts the call body into jsMockFunctionCallImpl(... thisValue), adds a dedicated jsMockFunctionConstruct that creates an instance via getFunctionRealm + InternalFunction::createSubclassStructure, records it in mock.instances, runs the shared impl with the instance as this, and returns the instance unless the impl returned an object. Two files touched: src/jsc/bindings/JSMockFunction.cpp (~60 lines net) and 7 new tests in test/js/bun/test/mock-fn.test.js.
Security risks
The PR fixes a security-adjacent issue (type confusion on asObject(undefined) in release builds). The new code itself does asObject(callframe->newTarget()), which is safe because newTarget is always an object during a construct call. Exception checks (RETURN_IF_EXCEPTION) are placed after getFunctionRealm, createSubclassStructure, push, and the impl call. The instances array is already visited in visitAdditionalChildrenInGCThread. tryCreateUninitializedRestricted is unchecked for null-on-OOM, but that exactly matches the existing pattern for calls/contexts/returnValues in the same file. I don't see new risks introduced.
Level of scrutiny
Moderate-to-high. This is hand-written JSC binding C++ touching object construction, GC-tracked arrays, and exception scopes in a code path exercised by every jest.fn() / mock() user who uses new. The change is well-scoped and follows established JSC idioms (same pattern as ObjectConstructor; instances bookkeeping is byte-for-byte parallel to the existing calls bookkeeping), and the PR description documents debug+ASAN, exception-validation, and GC-stress verification. Still, native engine binding changes that resolve a type-confusion crash deserve a human reviewer's eyes rather than bot-only approval.
Other factors
The bug hunting system found nothing. Test coverage is solid: 7 new cases covering the default instance, Reflect.construct, instance-as-this, object-return override, primitive-return ignored, mockReturnValue with object, prototype honoring, and throw-still-records. The test file is designed to also run under real Jest/Vitest, so the asserted semantics are cross-checked against the reference implementations.
What does this PR do?
Fixes a Fuzzilli-found assertion failure (fingerprint
JSObject.h(1346),ASSERTION FAILED: cell->isObjectSlow()) reached by constructing abun:testmock function:Root cause:
JSMockFunctionpassedjsMockFunctionCallas both the call and the construct handler:so
[[Construct]]returned whatever the mock implementation returned,undefinedby default. JSC requires a native[[Construct]]to return an object:Reflect.constructgoes throughInterpreter::executeConstruct, which doesasObject(result)on the return value. Onundefinedthat asserts in debug builds and is type confusion in release builds. The bytecode path doesn't assert, sonew fn()silently evaluated toundefined, which also violates the spec (anewexpression always produces an object).Fix: add a dedicated
jsMockFunctionConstructthat follows ordinary function[[Construct]]semantics, which is exactly what jest'smockConstructor(a plain JS function) does:OrdinaryCreateFromConstructor(newTarget, "%Object.prototype%")(the samegetFunctionRealm+InternalFunction::createSubclassStructurepattern JSC'sObjectConstructoruses), so an assignedfn.prototypeis honored.mock.instances(the array already existed, was exposed onfn.mock, and was cleared bymockClear/mockReset, but nothing ever wrote to it).this, somock.contexts,mock.calls, andmock.resultsrecord the same things jest records fornew.The load-bearing change is the constructor list at
JSMockFunction(...):jsMockFunctionCall, jsMockFunctionCallbecomesjsMockFunctionCall, jsMockFunctionConstruct. The old handler body is unchanged, just extracted intojsMockFunctionCallImpltakingthisValueas a parameter (during native construct the callframe's this slot holds newTarget, not a this value).How did you verify your code works?
ASSERTION FAILED: cell->isObjectSlow()/isCell()).test/js/bun/test/mock-fn.test.jsundermock() > called as a constructor: 5 of the 7 fail on the unfixed build (new fn()isundefined,mock.instancesstays empty); the other 2 pin the "implementation returned an object wins" contract that already worked and must keep working. This file is written to also run under real Jest and Vitest, and the assertions match jest's documentedmock.instancesbehavior fornew.mock-fn.test.js(79 tests),mock-disposable.test.ts,mock/mock-module.test.ts,spyMatchers.test.ts,expect-toHaveReturnedWith.test.js, andjest-extended.test.jspass with the debug build.BUN_JSC_validateExceptionChecks=1is clean on the new path, including anewTarget.prototypeproxy trap that throws.new fn()iterations withBun.gc(true)keeps all recorded instances alive.Known remaining divergence from jest, unchanged by this PR: a bare mock function has no own
prototypeproperty (jest's mocks are plain functions, sonew jest.fn() instanceof fnworks there without assigningfn.prototypefirst).