Fix crash when constructing a mock function whose implementation returns a primitive#31375
Fix crash when constructing a mock function whose implementation returns a primitive#31375robobun wants to merge 2 commits into
Conversation
Mock functions registered the call callback as their construct callback, so `new mockFn()` (or Reflect.construct) returned whatever the mock implementation produced. When that value was a primitive, the native construct contract was violated: Interpreter::executeConstruct treats the result as a JSObject*, which asserts in debug builds and is a type confusion in release builds. Give JSMockFunction a dedicated construct callback that creates the `this` object from newTarget.prototype, runs the mock with it, and returns the mock's result only when it is an object, matching ordinary JS constructor semantics.
|
Updated 9:29 PM PT - May 24th, 2026
❌ @robobun, your commit 85f9f8b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31375That installs a local version of the PR into your bun-31375 --bun |
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 45 minutes and 42 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds support for using 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
| callframe->setThisValue(thisObject); | ||
| JSValue returnValue = JSValue::decode(jsMockFunctionCall(lexicalGlobalObject, callframe)); | ||
| RETURN_IF_EXCEPTION(scope, {}); |
There was a problem hiding this comment.
🟣 Not a regression, but worth noting while we're here: mock.instances is still never populated — fn->instances is declared, cleared, GC-visited and exposed via getInstances(), but nothing ever pushes to it. In Jest, new mockFn() pushes the constructed this onto mock.instances (it's the canonical API for tracking instances), and the new construct path now has thisObject in hand, so this would be the natural place to wire it up. The new test asserts on noImpl.mock.contexts[0] rather than noImpl.mock.instances[0], which sidesteps the gap.
Extended reasoning...
What the gap is. In Jest, mockFn.mock.instances is the documented API for tracking objects created by calling a mock with new — each new mockFn() pushes the constructed this onto that array. Bun declares the backing storage for this (mutable JSC::WriteBarrier<JSC::JSArray> instances), clears it in clear(), visits it in visitAdditionalChildrenInGCThread, lazily creates it in getInstances(), and wires it into the mock object structure at offset 2 — but nothing in the file ever pushes a value into it. Grepping the whole of JSMockFunction.cpp for instances shows only declarations, clears, visits, and reads; there is no push / initializeIndex / putDirectIndex targeting fn->instances.
Code path. jsMockFunctionCall records each invocation by pushing to fn->calls, fn->contexts, fn->invocationCallOrder, and fn->returnValues, but not fn->instances. The new jsMockFunctionConstruct creates thisObject (exactly the value Jest pushes onto mock.instances), then delegates to jsMockFunctionCall via callframe->setThisValue(thisObject). So after construction, mock.contexts contains the new instance but mock.instances remains empty.
Why nothing prevents it. The only thing that could populate mock.instances is an explicit push, and there isn't one anywhere in the file. The new test in mock-fn.test.js asserts expect(noImpl.mock.contexts[0]).toBe(instance) rather than expect(noImpl.mock.instances[0]).toBe(instance), so the test suite doesn't catch the omission. Under real Jest (which this test file is documented to be runnable against), mock.instances[0] would equal instance — the cross-runner contract is satisfied only because the test avoids checking it.
Step-by-step proof.
const Fn = jest.fn(); const inst = new Fn();jsMockFunctionConstructruns, allocatesthisObject, sets it as the call frame'sthis, and callsjsMockFunctionCall.jsMockFunctionCallpushesthisObjectontofn->contexts(soFn.mock.contexts === [inst]), pushes[]ontofn->calls, pushes a result ontofn->returnValues, pushes an id ontofn->invocationCallOrder— and returns.- Nothing has touched
fn->instances.Fn.mock.instanceslazily resolves viagetInstances()to a fresh empty array. - Result:
Fn.mock.instances→[]in Bun,[inst]in Jest.
Impact. This is a pre-existing Jest-compat gap, not a regression — mock.instances was never populated before this PR either, and the PR's stated purpose (fixing the isCell() crash) is achieved correctly without it. But the PR explicitly adds the construct path "to match Jest, where mocks are plain JS functions", and mock.instances is the Jest API specifically for construct calls, so it's directly adjacent. User code that follows the Jest docs (expect(MockCtor.mock.instances[0]).toBe(...)) will still see an empty array under Bun.
Fix. In jsMockFunctionConstruct, after creating thisObject (and before or after delegating to jsMockFunctionCall), push thisObject onto fn->instances using the same pattern used for contexts in jsMockFunctionCall (push if the array exists, otherwise create a 1-element array and fn->instances.set(...)). Then the new test could assert noImpl.mock.instances[0] instead of (or in addition to) noImpl.mock.contexts[0]. Non-blocking — fine as a follow-up.
|
Closing as a duplicate of #30212, which fixes the same root cause (the mock/spy construct path returning a non-object from a native constructor) by giving The non-blocking review note here about populating |
1 participant
What does this PR do?
Fixes a crash found by fuzzing (assertion
isCell()inJSC::JSValue::asCell,JSCJSValue.h:1043).JSMockFunctionregisteredjsMockFunctionCallas both its call and construct callback, so constructing a mock returned whatever the mock implementation produced. Native constructors must always return an object:Interpreter::executeConstructdoesasObject(result)on the returned value, so a primitive result asserts in debug builds and is a type confusion in release builds. It was also observable from JS —new (mock())()evaluated toundefined, which should be impossible for anewexpression.Repro before this change (crashes debug builds,
Reflect.constructpath):This PR gives
JSMockFunctiona dedicated construct callback that follows ordinary JS constructor semantics (and matches Jest, where mocks are plain JS functions):thisobject fromnewTarget.prototype(falling back toObject.prototype)this, somock.contexts/mock.resultskeep recording as before and implementations that assign tothisworkthisHow did you verify your code works?
test/js/bun/test/mock-fn.test.jscoveringnew mock()with no implementation, primitive-returning, object-returning,this-mutating and throwing implementations,Reflect.constructwith a customnewTarget, and constructing a spy on a missing property (the fuzzer scenario). The new tests fail with theisCell()assertion on a build without the fix and pass with it.bun bd test test/js/bun/test/mock-fn.test.js(also withBUN_JSC_validateExceptionChecks=1), plusmock-disposable.test.ts,spyMatchers.test.ts, andmock/mock-module-non-string.test.ts— all pass.