Skip to content

fix(bun:test): return an object when constructing a mock function#31372

Closed
robobun wants to merge 1 commit into
mainfrom
farm/4e0236f3/fix-mock-construct
Closed

fix(bun:test): return an object when constructing a mock function#31372
robobun wants to merge 1 commit into
mainfrom
farm/4e0236f3/fix-mock-construct

Conversation

@robobun

@robobun robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a Fuzzilli-found crash (assertion isCell() in JSCJSValue.h:1043, reached from JSC::Interpreter::executeConstructasObject).

JSMockFunction (the object behind jest.fn() / mock() / spyOn()) registered jsMockFunctionCall as both its call and construct callbacks. That callback returns whatever the mock's implementation produces — undefined when there is no implementation, or any primitive from mockReturnValue/a primitive-returning implementation. A native [[Construct]] callback must return an object, so:

  • Reflect.construct(mockFn, []) hit the isCell() assertion in debug builds (and in release builds asObject() fabricates a bogus JSObject* from a primitive encoding — type confusion).
  • new mockFn() silently evaluated to undefined, which ordinary JS constructor semantics never allow.

Minimal repro (crashes a debug build before this change):

import { mock } from "bun:test";
Reflect.construct(mock(), []); // ASSERTION FAILED: isCell()

Fix

Give JSMockFunction a dedicated construct callback that follows ordinary constructor semantics:

  1. Create this from newTarget.prototype (falling back to a plain object).
  2. Run the existing mock body (call recording, implementations, etc.) with that this.
  3. Return the body's result if it is an object, otherwise return the created this.

This matches how Jest mock functions behave, since they are ordinary JS functions there (new jest.fn()() yields the new instance, and an object-returning implementation passes through).

How did you verify your code works?

  • The original Fuzzilli crash script and the minimized repro no longer crash a debug build (clean exit).
  • Added tests in test/js/bun/test/mock-fn.test.js covering new/Reflect.construct on jest.fn() (no implementation, primitive-returning, object-returning, mockReturnValue) and on a spy over a missing property (the fuzzer's exact path). They fail on the previous build and pass with this change.
  • bun bd test test/js/bun/test/mock-fn.test.js (74 pass), plus mock-disposable, mock-module*, spyMatchers, expect-toHaveReturnedWith, and describe suites all pass.

Mock functions registered jsMockFunctionCall as both their call and
construct callbacks, so `new mockFn()` / `Reflect.construct(mockFn, [])`
could return a non-object (e.g. undefined), violating the [[Construct]]
contract and failing the isCell() assertion in
JSC::Interpreter::executeConstruct.

Add a dedicated construct callback that creates `this` from
newTarget.prototype, runs the mock body with it, and returns the body's
result only when it is an object, matching ordinary constructor
semantics.
@robobun

robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:36 PM PT - May 24th, 2026

@robobun, your commit 1252053 has 1 failures in Build #57815 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31372

That installs a local version of the PR into your bun-31372 executable, so you can run:

bun-31372 --bun

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0c418137-3557-46b6-89c1-7739c7dbf3d7

📥 Commits

Reviewing files that changed from the base of the PR and between 7bd0861 and 1252053.

📒 Files selected for processing (2)
  • src/jsc/bindings/JSMockFunction.cpp
  • test/js/bun/test/mock-fn.test.js

Walkthrough

This PR adds explicit constructor support to Bun's mock functions. The implementation refactors shared call logic into a helper, wires a new construct handler into JSMockFunction, implements standard JS constructor semantics (creating a this object and returning it unless the mock body returns an object), and validates the behavior with tests covering primitive and object return values.

Changes

Mock Function Constructor Support

Layer / File(s) Summary
Shared call implementation and construct declaration
src/jsc/bindings/JSMockFunction.cpp
jsMockFunctionCallImpl(...) is extracted as a shared internal helper to encapsulate mock-call behavior, and jsMockFunctionConstruct host function is declared to handle constructor paths.
Construct wiring and implementation
src/jsc/bindings/JSMockFunction.cpp
JSMockFunction constructor wiring routes [[Construct]] to jsMockFunctionConstruct. The refactored jsMockFunctionCall delegates to shared logic; jsMockFunctionConstruct creates a thisObject from newTarget.prototype, invokes the shared call logic, and returns the mock result only if it is an object, otherwise returns the thisObject.
Constructor behavior tests
test/js/bun/test/mock-fn.test.js
Tests verify mock functions are constructible with new for no implementation, primitive returns, and object returns; a spy test ensures constructing a spy on a missing property returns a non-null object.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing mock function construction to return an object, matching the changeset which adds construct callback support to JSMockFunction.
Description check ✅ Passed The description comprehensively covers both required sections: it clearly explains what the PR does (the crash, root cause, and the fix) and thoroughly documents verification (crash reproduction, new tests, and test suite results).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix crash constructing jest.fn() via Reflect.construct #30212 - Also fixes the isCell() assertion crash when constructing mock functions by splitting jsMockFunctionCall into separate call/construct callbacks

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any issues — the construct callback correctly mirrors ordinary [[Construct]] semantics and the refactor is clean — but since this is native JSC bindings code fixing a type-confusion path, it's worth a human glance.

Extended reasoning...

Overview

This PR splits jsMockFunctionCall into a shared jsMockFunctionCallImpl (parameterized on thisValue) and adds a dedicated jsMockFunctionConstruct callback for JSMockFunction. Previously the call handler was reused for construct, which could return primitives — violating the [[Construct]] invariant and triggering an isCell() assertion (debug) / type confusion via asObject() on a primitive encoding (release). The new construct path creates this from newTarget.prototype, runs the mock body, and returns the body's result only if it is an object — standard ordinary-constructor semantics. Tests cover new/Reflect.construct for empty mocks, primitive-returning impls, object-returning impls, mockReturnValue, and spyOn over a missing property.

Security risks

The bug being fixed is itself the security-relevant part: in release builds the old code fabricated a JSObject* from a non-cell encoding. The fix removes that hazard. The new code's asObject(callframe->newTarget()) is safe because newTarget is always an object in a native construct callback, and exception checks bracket the .prototype lookup and the body call. I don't see new risks introduced.

Level of scrutiny

Medium-high. The diff is small (~35 lines of new C++ plus a mechanical refactor) and follows the well-known JSC pattern for native constructors, but it lives in src/jsc/bindings and changes how every jest.fn()/mock()/spyOn() behaves under new. Given it's native code on a memory-safety-adjacent path, a human reviewer familiar with Bun's JSC bindings should sign off.

Other factors

No CODEOWNERS cover this file. Test coverage for the new behavior is solid and the existing mock test suites reportedly pass. The refactor of the call path is behavior-preserving (only thisValue sourcing moved to the wrapper). No outstanding reviewer comments.

@robobun

robobun commented May 25, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #30212, which fixes the same isCell() assertion (mock functions registering the same host function for [[Call]] and [[Construct]]) with an equivalent approach, broader test coverage (including newTarget.prototype handling and this propagation), and CI already green on a rebase against current main.

The entry point from this fuzzer report — constructing a spy created over a missing property, e.g. Reflect.construct(spyOn(target, "doesNotExist"), []) — funnels into the same Kind::ReturnValue-with-a-non-object path that #30212 already covers (jest.fn().mockReturnValue("hello") + Reflect.construct), so no changes need to be carried over.

@robobun robobun closed this May 25, 2026
@robobun robobun deleted the farm/4e0236f3/fix-mock-construct branch May 25, 2026 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant