Fix crash constructing jest.fn() via Reflect.construct#30212
Conversation
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 12 minutes and 4 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)
WalkthroughA shared host-function helper unifies mock call and construct behavior; a dedicated ChangesMock Function Construct Support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 11:41 PM PT - May 24th, 2026
✅ @robobun, your commit daaa8d94a5f7a42bd7e395fd4d9c6b21b106afe3 passed in 🧪 To try this PR locally: bunx bun-pr 30212That installs a local version of the PR into your bun-30212 --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}
| if (isConstructCall) { | ||
| JSValue newTarget = callframe->newTarget(); | ||
| JSObject* newTargetObject = newTarget.getObject(); | ||
| if (newTargetObject && newTargetObject != fn) { | ||
| JSGlobalObject* functionGlobalObject = getFunctionRealm(globalObject, newTargetObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| Structure* structure = InternalFunction::createSubclassStructure(globalObject, newTargetObject, functionGlobalObject->objectStructureForObjectConstructor()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| thisValue = constructEmptyObject(vm, structure); | ||
| } else { | ||
| thisValue = constructEmptyObject(globalObject); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing nit (not introduced by this PR): now that the construct path allocates a fresh thisValue, this would be the natural place to also push it onto fn->instances so that mock.instances is populated on new fn() like Jest does — currently instances is exposed but never written to anywhere, so it always stays empty. Fine to leave for a follow-up since this PR is scoped to the crash fix.
Extended reasoning...
What the gap is
JSMockFunction declares a mutable JSC::WriteBarrier<JSC::JSArray> instances field, lazily creates it in getInstances(), clears it in clear(), visits it for GC, and exposes it as mock.instances on the mock object structure. However, nothing in jsMockFunctionCallOrConstruct (or anywhere else in the file) ever pushes into it. Grepping the file confirms the only references to instances are the declaration, clear(), getInstances(), the GC visitor, and the mockObjectStructure slot — none of them are writes during invocation. So mock.instances is always an empty array.
In Jest, mockFn.mock.instances is "an array that contains all the object instances that have been instantiated from this mock function using new" — every new fn() pushes the constructed this onto it.
Code path
In the new construct branch:
if (isConstructCall) {
...
thisValue = constructEmptyObject(...);
}the freshly allocated thisValue is later pushed onto fn->contexts (alongside calls, invocationCallOrder, and returnValues), but there is no corresponding push onto fn->instances. The fix would be to add the same lazy-create-then-push pattern for fn->instances inside the isConstructCall block (or right after contexts is pushed, gated on isConstructCall), pushing thisValue.
Why nothing currently prevents it
There simply is no write site. The infrastructure (storage, getter, GC visiting, mock-object slot) is all wired up, but the population step was never implemented.
Step-by-step proof
const fn = jest.fn();new fn();→ entersjsMockFunctionConstruct→jsMockFunctionCallOrConstruct(..., true).isConstructCallis true, sothisValue = constructEmptyObject(globalObject).calls,contexts,invocationCallOrderare pushed;returnValuesis pushed viasetReturnValue.instancesis not touched.fn.mock.instances→getInstances()lazily creates an empty array and returns it.expect(fn.mock.instances).toHaveLength(1)fails in Bun, passes in Jest.
Why this is pre-existing
Before this PR, instances was already never populated, and new (jest.fn(() => ({})))() already worked without crashing (the impl returned an object, so asObject() succeeded), so the empty mock.instances was already observable. This PR is a narrow Fuzzilli crash fix and doesn't change instances behavior at all — it just adds the construct branch where the missing push would naturally live, which is why it's worth flagging here as a follow-up rather than a blocker.
There was a problem hiding this comment.
LGTM — thanks for dropping the != fn guard and adding the fn.prototype test.
Extended reasoning...
Overview
Splits JSMockFunction's host function into separate jsMockFunctionCall / jsMockFunctionConstruct entry points that share a common body via an explicit isConstructCall flag. On construct, a fresh this is allocated through getFunctionRealm + InternalFunction::createSubclassStructure (honoring new.target.prototype), and any non-object return is replaced with that this so Interpreter::executeConstruct always receives an object. Six new tests in mock-fn.test.js cover no-impl, primitive-returning impl, mockReturnValue, object-returning impl, new.target prototype, user-assigned fn.prototype, and this propagation into the implementation.
Security risks
None. This is test-runner mock infrastructure; no auth, crypto, filesystem, or network surface. The new code paths only run when a jest.fn() mock is invoked via new/Reflect.construct, which previously crashed in debug builds.
Level of scrutiny
Moderate — C++ JSC bindings, but the change is small and idiomatic: the getFunctionRealm → createSubclassStructure → constructEmptyObject(vm, structure) sequence with RETURN_IF_EXCEPTION after each step is the standard pattern used throughout Bun's native constructors. The encodedReturnValue lambda enforcing object-or-this on construct is a textbook [[Construct]] substitution rule. The non-construct path is byte-identical to before (just routed through the shared helper with isConstructCall=false).
Other factors
I previously left two comments: a nit about the newTargetObject != fn fast-path skipping user-assigned fn.prototype, and a pre-existing observation about mock.instances never being populated. The author addressed the first in 6e3244f (the guard is gone and there's a covering test); the second was explicitly flagged as out-of-scope follow-up and is unchanged behavior. The bug-hunting pass on the latest revision found nothing. Strictly an improvement over the prior crash.
Jarred-Sumner
force-pushed
the
farm/7a85c992/fix-mock-construct-iscell
branch
from
6e3244f to
1df4eed
Compare
There was a problem hiding this comment.
LGTM — the != fn guard was dropped per the earlier nit and the construct path now follows the standard JSC createSubclassStructure idiom with good test coverage.
Extended reasoning...
Overview
Splits JSMockFunction's host function into separate [[Call]] and [[Construct]] entry points that share a common body via an isConstructCall flag. On construct, it allocates a fresh this via getFunctionRealm → InternalFunction::createSubclassStructure → constructEmptyObject, passes it to the implementation, and substitutes it for any non-object return value — standard ordinary-[[Construct]] semantics. The [[Call]] path is byte-for-byte unchanged aside from being routed through a thin wrapper. Six new tests in mock-fn.test.js cover no-impl construct, primitive returns, object passthrough, newTarget.prototype, user-assigned fn.prototype, and this propagation to the implementation.
Security risks
None. This is bun:test mock-function infrastructure; no auth, crypto, filesystem, or network surface is touched. The new code only allocates plain JS objects and reads newTarget.prototype via the standard JSC helper.
Level of scrutiny
Moderate-low. It's native JSC bindings C++, but the change is ~40 lines, confined to one function whose construct path previously crashed (so regression risk on that path is effectively zero), and follows the exact createSubclassStructure pattern used across Bun's other native constructors. Exception scopes are correctly threaded with RETURN_IF_EXCEPTION after both getFunctionRealm and createSubclassStructure.
Other factors
- My earlier 🟡 nit (the
newTargetObject != fnfast-path skipping user-assignedfn.prototype) was addressed in 6e3244f / 1df4eed and is now covered by the "new on mock uses assigned prototype" test. - The remaining 🟣 inline comment is an explicitly pre-existing
mock.instancesgap flagged as a follow-up, not a blocker. - No CODEOWNERS for the touched paths.
- The duplicate-PR bot flagged three prior attempts at the same fix; that's a triage decision for maintainers and doesn't affect correctness here.
robobun
force-pushed
the
farm/7a85c992/fix-mock-construct-iscell
branch
from
1df4eed to
1204fe7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/JSMockFunction.cpp (1)
844-901:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord constructor-created objects in
mock.instances.Line 844 allocates a constructor
thisValue, and Line 891 stores it incontexts, but this construct path never appends tofn->instances. That leaves constructor metadata incomplete for consumers readingmock.instances.Suggested fix
@@ JSC::JSArray* contexts = fn->contexts.get(); @@ fn->contexts.set(vm, fn, contexts); } + if (isConstructCall) { + 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); + } + } + auto invocationId = JSMockModule::nextInvocationId();🤖 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 844 - 901, The constructor path creates thisValue but never records it in fn->instances, leaving mock.instances incomplete; after the contexts handling add code that if isConstructCall then fetch fn->instances (like fn->contexts/get pattern), push thisValue with RETURN_IF_EXCEPTION(scope, {}) if the array exists, otherwise create a new JSArray via JSArray::tryCreateUninitializedRestricted, initialize index 0 with thisValue, and set it with fn->instances.set(vm, fn, instances).
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 844-901: The constructor path creates thisValue but never records
it in fn->instances, leaving mock.instances incomplete; after the contexts
handling add code that if isConstructCall then fetch fn->instances (like
fn->contexts/get pattern), push thisValue with RETURN_IF_EXCEPTION(scope, {}) if
the array exists, otherwise create a new JSArray via
JSArray::tryCreateUninitializedRestricted, initialize index 0 with thisValue,
and set it with fn->instances.set(vm, fn, instances).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4db13abc-f7d1-4476-ba38-bd2a03a85fb1
📒 Files selected for processing (2)
src/jsc/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
There was a problem hiding this comment.
LGTM — thanks for addressing the fn.prototype nit; the remaining mock.instances note is pre-existing and fine as a follow-up.
Extended reasoning...
Overview
This PR fixes a debug-build assertion crash (ASSERTION FAILED: isCell()) when a jest.fn() mock is invoked as a constructor via Reflect.construct or new. It touches src/jsc/bindings/JSMockFunction.cpp (splitting the shared host function into separate [[Call]]/[[Construct]] entry points, allocating a fresh this via InternalFunction::createSubclassStructure, and ensuring construct calls always return an object) plus six new test cases in test/js/bun/test/mock-fn.test.js.
Security risks
None. This is test-runner mock-function dispatch; no untrusted input parsing, auth, crypto, or filesystem/network surface. The change strictly tightens behavior (crash → spec-compliant [[Construct]] semantics) and follows the same getFunctionRealm / createSubclassStructure idiom used throughout Bun's other native constructors.
Level of scrutiny
Moderate. JSC bindings C++ warrants care, but the change is small (~50 lines), mechanical, and mirrors the standard JSC pattern for native constructors (allocate this from newTarget's prototype, substitute it for any non-object return). The encodedReturnValue lambda is applied consistently to every return site, exception checks (RETURN_IF_EXCEPTION) are placed after each fallible JSC call, and the call path (isConstructCall=false) is byte-for-byte unchanged in behavior.
Other factors
- I previously left two nits on this PR. The actionable one (honor a user-assigned
fn.prototypeonnew fn()by always going throughcreateSubclassStructure) was addressed in 6e3244f and now has a dedicated test. The other was an explicitly pre-existing gap (mock.instancesis never populated) flagged as a follow-up, not a blocker. - The bug-hunting system found no issues on the current revision.
- Test coverage is thorough: no-impl construct, primitive-returning impl, object-returning impl,
newTargetprototype, assignedfn.prototype, andthispropagation to the impl viamock.contexts. - No CODEOWNERS cover these paths.
- Pre-PR this code path crashed, so the change is strictly an improvement with no regression surface on the call path.
robobun
force-pushed
the
farm/7a85c992/fix-mock-construct-iscell
branch
from
1204fe7 to
5b8f855
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/JSMockFunction.cpp (1)
844-901:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack constructed receivers in
mock.instances.When
isConstructCallis true we allocatethisValueand record it inmock.contexts, but this path never appends that receiver tofn->instances.new fn()/Reflect.construct(fn, ...)will therefore still leavefn.mock.instancesempty even though constructor calls are now otherwise supported. Push the constructed receiver toinstancesonly for constructor calls, mirroring thecontextstracking pattern.Proposed fix
JSC::JSArray* contexts = fn->contexts.get(); if (contexts) { contexts->push(globalObject, thisValue); RETURN_IF_EXCEPTION(scope, {}); } else { JSC::ObjectInitializationScope object(vm); contexts = JSC::JSArray::tryCreateUninitializedRestricted( object, globalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous), 1); contexts->initializeIndex(object, 0, thisValue); fn->contexts.set(vm, fn, contexts); } + + if (isConstructCall) { + 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); + } + }🤖 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 844 - 901, The constructor call path (when isConstructCall is true) creates thisValue and records it in fn->contexts but never appends the constructed receiver to fn->instances; update the construct branch to push thisValue into fn->instances (mirror how contexts is handled): obtain or create JSC::JSArray* instances = fn->instances.get(), push(thisValue) if instances exists with RETURN_IF_EXCEPTION(scope, {}), otherwise create a new uninitialized restricted JSArray of length 1, initializeIndex(..., 0, thisValue) and set it via fn->instances.set(vm, fn, instances), matching the pattern used for fn->contexts and fn->calls so constructed receivers appear in mock.instances.
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 844-901: The constructor call path (when isConstructCall is true)
creates thisValue and records it in fn->contexts but never appends the
constructed receiver to fn->instances; update the construct branch to push
thisValue into fn->instances (mirror how contexts is handled): obtain or create
JSC::JSArray* instances = fn->instances.get(), push(thisValue) if instances
exists with RETURN_IF_EXCEPTION(scope, {}), otherwise create a new uninitialized
restricted JSArray of length 1, initializeIndex(..., 0, thisValue) and set it
via fn->instances.set(vm, fn, instances), matching the pattern used for
fn->contexts and fn->calls so constructed receivers appear in mock.instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98685672-b142-42a3-a07b-7ecca435f8a6
📒 Files selected for processing (2)
src/jsc/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
There was a problem hiding this comment.
LGTM — the earlier nit was addressed in 6e3244f/5b8f855, and the remaining mock.instances note is a pre-existing follow-up.
Extended reasoning...
Overview
This PR fixes a Fuzzilli-found ASSERTION FAILED: isCell() crash when a jest.fn() mock is invoked as a constructor via Reflect.construct / new. It touches two files: src/jsc/bindings/JSMockFunction.cpp (splitting the shared host function into separate [[Call]] / [[Construct]] entry points that delegate to a common helper with an isConstructCall flag, allocating a proper this via InternalFunction::createSubclassStructure, and substituting this for non-object return values on construct) and test/js/bun/test/mock-fn.test.js (six new tests covering no-impl construct, primitive returns, object returns, newTarget.prototype, assigned fn.prototype, and this propagation to the impl).
Security risks
None. This is bun:test mock-function infrastructure — no auth, crypto, permissions, filesystem, or network surface. The change only affects how jest.fn() behaves when invoked as a constructor, and the pre-PR behavior on this path was a debug-build crash.
Level of scrutiny
Moderate. While this is C++ JSC-bindings code, the scope is narrow and the implementation follows the established JSC idiom (getFunctionRealm → createSubclassStructure → constructEmptyObject, with RETURN_IF_EXCEPTION after each fallible step) used elsewhere in Bun's native constructors. The encodedReturnValue lambda correctly implements ordinary [[Construct]] semantics (return the impl's result if it's an object, else the allocated this). The isConstructCall=false path is byte-for-byte equivalent to the old code, so existing call behavior is unchanged.
Other factors
- My earlier 🟡 nit (the
newTargetObject != fnfast-path ignoring user-assignedfn.prototype) was addressed: the guard was dropped socreateSubclassStructurealways runs, and anew on mock uses assigned prototypetest was added. That inline thread is resolved. - My 🟣 comment about
mock.instancesnever being populated is explicitly pre-existing and flagged as a follow-up; it does not block this crash fix. - The bug-hunting system found no issues on the current revision.
- The duplicate-PR bot flagged three earlier PRs targeting the same crash; that's a process/triage matter for maintainers and doesn't bear on the correctness of this diff.
- Test coverage is solid: the six added cases exercise every new branch (no impl, primitive impl,
mockReturnValue, object impl, explicitnewTarget, assignedprototype, andthiscapture viamock.contexts).
|
CI status: the only failure on build #53591 so far is Across four CI builds (#50719, #50963, #51966, #53591) on successive rebases of this branch, Both code reviews approved. The fix has been verified against 30+ Fuzzilli reproductions of this fingerprint across 6 different main commits. Ready to merge once maintainers are satisfied the remaining CI noise is unrelated. |
|
Fuzzilli hit this same assertion again via a different entry point — a spy instead of const spy = Bun.jest(1002).spyOn(Date, 1002).mockRestore();
Reflect.construct(spy, []); // ASSERTION FAILED: isCell() in JSValue::asCell()Same root cause: the spy is a |
|
Fuzzilli reported this assertion again via the spy entry point (constructing a spy created by |
| if (isConstructCall) { | ||
| JSValue newTarget = callframe->newTarget(); | ||
| if (JSObject* newTargetObject = newTarget.getObject()) { | ||
| JSGlobalObject* functionGlobalObject = getFunctionRealm(globalObject, newTargetObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| Structure* structure = InternalFunction::createSubclassStructure(globalObject, newTargetObject, functionGlobalObject->objectStructureForObjectConstructor()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| thisValue = constructEmptyObject(vm, structure); | ||
| } else { | ||
| thisValue = constructEmptyObject(globalObject); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing (not introduced by this PR): even with isConstructCall true, the Kind::Call branch still invokes the implementation via Bun::call(...) — a regular [[Call]] — so spying on / mocking an ES class constructor and then new-ing it throws Class constructor Foo cannot be invoked without 'new', whereas Jest uses new impl(...args) on construct. The new "constructing a spy works" test only passes because it uses a plain function() {} rather than a class. Since this PR adds the isConstructCall flag and a proper thisValue right at this site, it'd be the natural place to branch to JSC::construct(globalObject, impl, args, newTarget) when isConstructCall and the impl is constructible — fine to leave for a follow-up alongside the mock.instances note since this PR is scoped to the crash fix.
Extended reasoning...
What the gap is
In the JSMockImplementation::Kind::Call branch of jsMockFunctionCallOrConstruct, the implementation is always invoked as:
JSC::CallData callData = JSC::getCallData(result);
...
JSValue returnValue = Bun::call(globalObject, result, callData, thisValue, args);Bun::call performs an ordinary [[Call]], never [[Construct]], regardless of isConstructCall. ES class constructors have valid CallData (so the Type::None guard doesn't trip), but per spec they throw TypeError: Class constructor X cannot be invoked without 'new' when invoked via [[Call]]. So a mock or spy whose implementation is an ES class can be called fine, but throws as soon as it's used with new / Reflect.construct.
In Jest, mock functions are ordinary JS functions whose body checks new.target and does new impl(...args) when invoked as a constructor, so spying on a class and then new-ing it works.
Concrete trigger / step-by-step
class Foo { constructor() { this.x = 1 } }
const obj = { Foo };
jest.spyOn(obj, 'Foo');
new obj.Foo();spyOn(obj, 'Foo')creates aJSMockFunctionand pushes aKind::Callimplementation whoseunderlyingValueis the originalclass Foo.new obj.Foo()entersjsMockFunctionConstruct→jsMockFunctionCallOrConstruct(..., /* isConstructCall */ true).isConstructCallis true, so a freshthisValueis allocated viacreateSubclassStructure— good so far.- The
Kind::Callbranch fetchesresult = class Foo, thengetCallData(result)returns a JS call data (classes are callable in thegetCallDatasense), so theType::Nonecheck passes. Bun::call(globalObject, result, callData, thisValue, args)performs[[Call]]onclass Foo→ throwsTypeError: Class constructor Foo cannot be invoked without 'new'.- The exception is recorded in
mock.resultsas athrowand re-thrown.
The same applies to jest.fn(class Foo {}) followed by new fn().
Why nothing currently prevents it
The new isConstructCall flag is consulted only for allocating thisValue and for the encodedReturnValue non-object fixup; it does not influence how the implementation is invoked. There is no JSC::getConstructData / JSC::construct path anywhere in this function. The "constructing a spy works" test added in this PR uses function () { this.ok = true }, which is happy to be [[Call]]ed with the allocated thisValue, so it passes — but swapping it for class { constructor() { this.ok = true } } would fail.
Why this is pre-existing
Before this PR the construct entry point was the same jsMockFunctionCall host function and hit the exact same Bun::call line, so new obj.Foo() on a class spy already threw the same TypeError (when it didn't crash earlier for the unrelated isCell() reason this PR fixes). The PR does not change this behavior; it refactors the surrounding code and adds the isConstructCall flag right next to where the fix would go, which is why it's worth flagging here as a follow-up rather than a regression.
Suggested fix
Inside Kind::Call, when isConstructCall and JSC::getConstructData(result).type != JSC::CallData::Type::None, use JSC::construct(globalObject, result, args, callframe->newTarget()) instead of Bun::call(...), then run the result through encodedReturnValue. Optionally, when the impl is constructible, prefer the constructor-allocated instance (the value construct returns) over the pre-allocated thisValue so fn.prototype of the implementation class is honored. This pairs naturally with the existing pre-existing note about populating mock.instances.
Severity
Pre-existing, follow-up. The PR is scoped to the Fuzzilli isCell() crash and doesn't claim to fix class-constructor spying, so this should not block merge — filing alongside the existing mock.instances pre-existing note.
There was a problem hiding this comment.
Agreed — good catch. Invoking the implementation with [[Construct]] (via JSC::getConstructData/JSC::construct) when isConstructCall and the impl is constructible is the right follow-up, and this is the natural spot for it now that the flag and thisValue exist here. Keeping this PR scoped to the isCell() crash fix; the class-constructor spying gap and the mock.instances population can go together in a follow-up.
JSMockFunction registered the same host function for both [[Call]] and [[Construct]]. When invoked as a constructor with no implementation (or one returning a primitive), it returned jsUndefined(). Native constructors must return an object; Interpreter::executeConstruct calls asObject() on the result, tripping the isCell() assertion. Split the host function into call/construct entry points. On construct, allocate a fresh this object (honoring new.target's prototype) and return it whenever the mock would otherwise produce a non-object.
robobun
force-pushed
the
farm/7a85c992/fix-mock-construct-iscell
branch
from
17566f2 to
0fb8200
Compare
|
CI status for build #57723 ( That HTTP-close timeout has been consistently red on Windows at this branch's old base (same failure as build #53591), while current main's Windows jobs are green (build #57709 passed). Rebased onto main |
|
CI status for build #57745 ( Retrying those two canceled Windows jobs from the Buildkite UI is all that's needed to complete the Windows lane — I've already used a re-run on this PR (the rebase), so leaving that to a maintainer rather than retriggering again. |
|
Fuzzilli hit this assertion again through another entry point (constructing a spy created by |
|
CI status for build #57842 ( |
|
Superseded by #31386, which carries the same fix rebased onto current main and folds in this PR's test coverage (spy construction, explicit |
1 participant
What
Fixes an
ASSERTION FAILED: isCell()crash when ajest.fn()mock is invoked as a constructor throughReflect.construct(or any path that goes throughInterpreter::executeConstruct).Why
JSMockFunctionregistered the same host function for both[[Call]]and[[Construct]]. When invoked as a constructor with no implementation (or an implementation/mockReturnValuethat produces a primitive), it returnedjsUndefined(). Native constructors must return an object —Interpreter::executeConstructcallsasObject()on the result, which trips theisCell()assertion in debug builds.Because
CallFrame::newTarget()is an alias forthisValue()(only meaningful in a construct context), the shared host function had no reliable way to tell call apart from construct.How
jsMockFunctionCall/jsMockFunctionConstructentry points that forward to a shared implementation with an explicitisConstructCallflag.thisobject — usingInternalFunction::createSubclassStructureto honornew.target's prototype when it differs from the mock itself — and pass it asthisto the implementation.thisinstead, matching ordinary[[Construct]]semantics and Jest's behavior.Found by Fuzzilli.