Fix crash when constructing jest.fn() mock that returns a non-object#29940
Fix crash when constructing jest.fn() mock that returns a non-object#29940robobun wants to merge 1 commit into
Conversation
JSMockFunction used the same native function for both [[Call]] and [[Construct]]. When the mock implementation returned a primitive (e.g. jest.fn(Symbol)), Reflect.construct() would receive that primitive and hit an asObject() assertion in Interpreter::executeConstruct. Add a dedicated construct handler that creates a fresh 'this' object, runs the mock with it, and substitutes it for any non-object return value — matching ordinary JS 'new' semantics.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #29790 (and #28532, #28315). Closing in favor of #29790 which already has CI green. One difference worth noting: this PR additionally set |
|
Caution Review failedPull request was closed or merged during review WalkthroughImplements a dedicated construct handler for Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 1/5 review remaining, refill in 38 minutes and 21 seconds. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JSObject* thisObject = JSC::constructEmptyObject(globalObject); | ||
| callframe->setThisValue(thisObject); |
There was a problem hiding this comment.
🔴 The new instance is created via constructEmptyObject(globalObject), which always uses Object.prototype and ignores callframe->newTarget(). This means Reflect.construct(fn, [], CustomCtor) and a user-assigned fn.prototype = {...} are silently ignored, so the PR description's claim of "matches ordinary JS new semantics" doesn't hold for the prototype chain. Consider reading newTarget().prototype and using it (e.g. via InternalFunction::createSubclassStructure as in NapiClass.cpp) to derive the instance's [[Prototype]].
Extended reasoning...
What the bug is
jsMockFunctionConstruct allocates the new this with:
JSObject* thisObject = JSC::constructEmptyObject(globalObject);This overload always builds a plain object whose [[Prototype]] is Object.prototype. It never consults callframe->newTarget(), so the standard ECMA‑262 OrdinaryCreateFromConstructor step — "derive the new object's [[Prototype]] from newTarget.prototype" — is skipped. In Jest, mock functions are ordinary JS functions, so the engine performs that step automatically; here we are reimplementing [[Construct]] natively and have to do it ourselves.
Code path that triggers it
const fn = jest.fn(function(){});class C {}(or any function with a.prototype)Reflect.construct(fn, [], C)→ JSC dispatches tojsMockFunctionConstructwithnewTarget === C.constructEmptyObject(globalObject)ignoresnewTarget, producing{}withObject.prototype.- The implementation returns
undefined, sothisObjectis returned.Object.getPrototypeOf(result) === Object.prototype, notC.prototype, andresult instanceof Cisfalse.
The same applies if the user explicitly sets fn.prototype = { foo: 'bar' } and then does new fn() — new fn().foo is undefined here but 'bar' in Jest. (Note: because JSMockFunction is an InternalFunction without an own .prototype, the variant fn.prototype.foo = ... would throw before reaching this code; that's a separate pre‑existing limitation. The Reflect.construct and direct‑assignment cases are the observable ones.)
Why nothing prevents it
The handler reads callframe->thisValue() indirectly via jsMockFunctionCall, but never reads callframe->newTarget(). There is no fallback path that inspects the constructor's prototype. Other native constructors in this repo do handle this — e.g. NapiClass.cpp reads callFrame->newTarget() and calls InternalFunction::createSubclassStructure before constructEmptyObject(vm, structure); JSX509Certificate.cpp, JSDOMFile.cpp, and NodeDirent.cpp follow the same pattern.
Impact
Any test that mocks a class/constructor and relies on the prototype chain of constructed instances will diverge from Jest. Concretely, Reflect.construct(mock, args, RealClass) produces an object that is not instanceof RealClass, and methods inherited via the mock's prototype are unreachable on the instance. The PR description explicitly says this "matches ordinary JS new semantics", which is misleading for the prototype chain.
Step‑by‑step proof
class Base { greet() { return 'hi'; } }
const fn = jest.fn(function() {});
const inst = Reflect.construct(fn, [], Base);
// ECMA‑262 §10.1.13: prototype should be Base.prototype
Object.getPrototypeOf(inst) === Base.prototype; // Jest: true | this PR: false
inst instanceof Base; // Jest: true | this PR: false
inst.greet(); // Jest: 'hi' | this PR: TypeErrorIn jsMockFunctionConstruct, callframe->newTarget() is Base, but constructEmptyObject(globalObject) returns an object with globalObject->objectPrototype() as its prototype, so all three assertions fail.
How to fix
Read newTarget and derive the structure from its .prototype, mirroring the existing pattern:
JSValue newTarget = callframe->newTarget();
JSObject* thisObject;
if (newTarget.isObject()) {
auto* targetFn = jsCast<JSObject*>(newTarget);
auto* base = globalObject->objectStructureForObjectConstructor();
auto* structure = InternalFunction::createSubclassStructure(globalObject, targetFn, base);
RETURN_IF_EXCEPTION(scope, {});
thisObject = JSC::constructEmptyObject(vm, structure);
} else {
thisObject = JSC::constructEmptyObject(globalObject);
}(or equivalently, look up newTarget.get(globalObject, vm.propertyNames->prototype) and pass it to constructEmptyObject(globalObject, proto) when it's an object). This keeps the crash fix intact while making the prototype chain match ordinary new semantics.
| JSObject* thisObject = JSC::constructEmptyObject(globalObject); | ||
| callframe->setThisValue(thisObject); | ||
|
|
||
| JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); |
There was a problem hiding this comment.
🟣 Nit (pre-existing): now that there's a dedicated [[Construct]] path that allocates thisObject, this would be the natural place to push it into fn->instances so mock.instances actually gets populated. Currently fn->instances is declared, exposed via mock.instances, cleared in clear(), and GC-visited, but nothing in this file ever pushes to it — so new fn(); fn.mock.instances is still [] (Jest gives [instance]). Not blocking since the array was already always empty before this PR and the scope here is the crash fix, but worth a follow-up.
Extended reasoning...
What's missing
JSMockFunction has an instances field (WriteBarrier<JSC::JSArray>), a getInstances() accessor, and exposes it as mock.instances (slot 2 of the mock object structure). It's cleared in clear() and visited in visitAdditionalChildrenInGCThread. But searching the entire file shows nothing ever calls instances->push(...) or otherwise populates it — unlike calls, contexts, invocationCallOrder, and returnValues, which are all pushed in jsMockFunctionCall.
This PR adds jsMockFunctionConstruct, which allocates a fresh thisObject via constructEmptyObject and uses it as the new instance — exactly the value Jest records in mock.instances for a new call. But the new handler doesn't push thisObject into fn->instances, so the array remains empty.
Step-by-step
const fn = jest.fn(); const inst = new fn();jsMockFunctionConstructruns, createsthisObject, sets it ascallframe->thisValue(), delegates tojsMockFunctionCall.jsMockFunctionCallpushes tofn->calls,fn->contexts(withthisObject),fn->invocationCallOrder, andfn->returnValues. It never touchesfn->instances.jsMockFunctionConstructreturnsthisObject(since the implementation returnedundefined).fn.mock.instances→getInstances()lazily creates an empty array →[].
In Jest the same code yields fn.mock.instances[0] === inst, so assertions like expect(fn.mock.instances[0]).toBe(inst) (a common pattern when mocking constructors) still fail under Bun after this PR.
Why nothing currently prevents it
There's simply no code path that writes to instances. Before this PR the [[Construct]] handler was jsMockFunctionCall itself, which also never populated it, so this is pre-existing — mock.instances has always been an empty array in Bun. Note that in Jest, mock.instances is actually pushed on every invocation (it records this, which for non-new calls is the receiver and for new calls is the new instance), so a fully correct fix would push in jsMockFunctionCall rather than only in the construct path.
Impact
Low / compat-only. No crash, no incorrect return value — just a Jest API surface (mock.instances) that continues to be a no-op. The PR's stated goal (fix the asObject() assertion crash) is unaffected.
Suggested fix (follow-up, non-blocking)
Either:
- In
jsMockFunctionCall, after pushingcontexts, also pushthisValueintofn->instances(matches Jest exactly —instancesmirrorscontextsfor plain calls and holds the new object fornew), or - At minimum, in
jsMockFunctionConstruct, after creatingthisObject, retrievefnfromcallframe->jsCallee()and pushthisObjectintofn->instancesso the construct case works.
Filed as a nit since it's a pre-existing gap and orthogonal to the crash fix, but the new construct handler is the obvious place to start recording instances.
1 participant
What
JSMockFunctionregisteredjsMockFunctionCallas both its[[Call]]and[[Construct]]handler. When the mock's implementation returns a primitive (e.g.jest.fn(Symbol)), constructing the mock viaReflect.constructreturned that primitive toInterpreter::executeConstruct, which then calledasObject()on it and hit:Native
[[Construct]]handlers must return an object.Fix
Add a dedicated
jsMockFunctionConstructthat:this,jsMockFunctionCallwith thatthis(somock.contexts,mockReturnThis, and implementations that readthisall see the new instance),This matches ordinary JS
newsemantics (and Jest, where mock functions are plain JS functions and non-object returns are replaced bythis).Repro
Found by Fuzzilli.