Repository navigation
Conversation
JSMockFunction used the same native function for both call and construct. When invoked via Reflect.construct with no implementation, it returned undefined, and JSC::Interpreter::executeConstruct then called asObject() on that value, tripping the isCell() assertion. Give JSMockFunction its own construct handler that allocates an instance, records it in mock.instances, passes it as this to the shared call path, and returns it whenever the implementation does not produce an object. This also makes mock.instances actually track constructed objects like Jest does.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesMock functions now have a dedicated construct callback. Construction creates and records an instance, invokes the mock with that instance as Mock Function Construction
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/jsc/bindings/JSMockFunction.cpp`:
- Around line 980-1021: Add a brief comment in jsMockFunctionConstruct
immediately before callframe->setThisValue(instance), documenting that newTarget
was captured earlier because setThisValue repurposes the constructor CallFrame’s
this slot for jsMockFunctionCall, consistent with CallFrame.rs.
In `@test/js/bun/test/mock-fn.test.js`:
- Around line 806-833: Add a construction-error case to the existing
“Reflect.construct on a mock” test using a mock implementation that throws, and
assert both new fn() and Reflect.construct(fn, []) synchronously throw the same
Error. Keep the assertions focused on propagating the implementation exception
rather than returning an allocated instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7991438c-900d-4ae3-b262-6cb5093897cb
📒 Files selected for processing (2)
src/jsc/bindings/JSMockFunction.cpptest/js/bun/test/mock-fn.test.js
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) | ||
| { | ||
| Zig::GlobalObject* globalObject = uncheckedDowncast<Zig::GlobalObject>(lexicalGlobalObject); | ||
| auto& vm = JSC::getVM(globalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| JSMockFunction* fn = dynamicDowncast<JSMockFunction>(callframe->jsCallee()); | ||
| if (!fn) [[unlikely]] { | ||
| throwTypeError(globalObject, scope, "Expected callee to be mock function"_s); | ||
| return {}; | ||
| } | ||
|
|
||
| JSValue newTarget = callframe->newTarget(); | ||
| JSObject* newTargetObject = newTarget.isObject() ? asObject(newTarget) : fn; | ||
| JSValue prototype = newTargetObject->get(globalObject, vm.propertyNames->prototype); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSObject* instance = prototype.isObject() | ||
| ? JSC::constructEmptyObject(globalObject, asObject(prototype)) | ||
| : JSC::constructEmptyObject(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| if (JSC::JSArray* instances = fn->instances.get()) { | ||
| instances->push(globalObject, instance); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| } else { | ||
| JSC::ObjectInitializationScope object(vm); | ||
| instances = JSC::JSArray::tryCreateUninitializedRestricted( | ||
| object, | ||
| globalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous), | ||
| 1); | ||
| instances->initializeIndex(object, 0, instance); | ||
| fn->instances.set(vm, fn, instances); | ||
| } | ||
|
|
||
| callframe->setThisValue(instance); | ||
| EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSValue result = JSValue::decode(encodedResult); | ||
| if (result.isObject()) | ||
| return encodedResult; | ||
| return JSValue::encode(instance); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the vendored JSC CallFrame this-slot/newTarget behavior used by this construct callback.
fd -a CallFrame.h | xargs -I{} rg -n -B2 -A2 'setThisValue|newTarget\(\)' {}Repository: oven-sh/bun
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CallFrame.h candidates ---'
fd -a -i 'CallFrame\.h$' . | head -20
printf '%s\n' '--- construct callback and callframe access ---'
rg -n -B4 -A8 'jsMockFunctionConstruct|setThisValue|newTarget\(\)|thisValue\(\)' \
src/jsc/bindings src 2>/dev/null | head -240Repository: oven-sh/bun
Length of output: 19972
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked CallFrame files and JSC source roots ---'
git ls-files | rg -i '(^|/)(CallFrame\.h|JavaScriptCore|jsc|webkit)(/|$)' | head -160
printf '%s\n' '--- CallFrame API references ---'
rg -n -S 'class CallFrame|newTarget\(\)|setThisValue\(|thisValue\(\) const|thisValue\(\)' \
--glob '*.{h,hpp,cpp,cc,mm,inc}' . 2>/dev/null | head -240
printf '%s\n' '--- exact mock constructor context ---'
sed -n '960,1030p' src/jsc/bindings/JSMockFunction.cppRepository: oven-sh/bun
Length of output: 38257
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CallFrame.rs slot definitions ---'
rg -n -B8 -A14 'pub fn (new_target|this_value|set_this_value)|newTarget|setThisValue|thisValue' \
src/jsc/CallFrame.rs | head -180
printf '%s\n' '--- analogous host-constructor slot reuse ---'
sed -n '1,95p' src/jsc/bindings/NapiClass.cpp
sed -n '1025,1065p' src/jsc/bindings/napi.h
printf '%s\n' '--- constructor implementations using both slots ---'
sed -n '320,350p' src/runtime/webview/JSWebViewConstructor.cpp
sed -n '165,195p' src/jsc/bindings/NodeDirent.cppRepository: oven-sh/bun
Length of output: 8808
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- generated CallFrame.rs contents ---'
wc -l src/jsc/CallFrame.rs
cat -n src/jsc/CallFrame.rs | head -220
printf '%s\n' '--- CallFrame generation and WebKit source references ---'
rg -n -S 'CallFrame\.rs|CallFrame|WebKit|oven-sh/WebKit|397dafc9721b8f8046f9448abb6dbc14efe096d3' \
build.zig build.zig.zon src package.json README.md 2>/dev/null | head -220Repository: oven-sh/bun
Length of output: 31334
Document the intentional CallFrame slot reuse.
At Line 1013, add a short comment that callframe->newTarget() was captured before setThisValue(instance) repurposes the constructor frame’s this slot for jsMockFunctionCall. src/jsc/CallFrame.rs documents this slot-sharing behavior.
🤖 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 980 - 1021, Add a brief
comment in jsMockFunctionConstruct immediately before
callframe->setThisValue(instance), documenting that newTarget was captured
earlier because setThisValue repurposes the constructor CallFrame’s this slot
for jsMockFunctionCall, consistent with CallFrame.rs.
Source: Coding guidelines
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Added the throw-during-construct coverage. Skipping the inline comment on On the duplicate flag: yes, #34575 is the superset fix but it currently has merge conflicts ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but deferring to a human reviewer — this adds a new native JSC construct callback that mutates the CallFrame's this-slot and changes user-visible mock.instances behavior, and it overlaps with the broader open #34575, so a maintainer should confirm the scope split.
What was reviewed:
- Exception-scope discipline in
jsMockFunctionConstruct: every fallible call (get(prototype),constructEmptyObject,push, delegatedjsMockFunctionCall) is followed byRETURN_IF_EXCEPTION. newTargetis captured beforesetThisValue(instance)overwrites the shared slot, matching the NapiClass/NodeDirent pattern.instancesarray creation vs. push interacts correctly with the lazymockobject (getInstances()reuses the array either way).- Test covers no-impl, object-returning impl, primitive-returning impl, custom
newTarget, and throwing impl.
Extended reasoning...
Overview
The PR adds jsMockFunctionConstruct in src/jsc/bindings/JSMockFunction.cpp and wires it as the construct callback for JSMockFunction (previously both call and construct used jsMockFunctionCall). The new handler allocates an instance from newTarget.prototype, records it in fn->instances, sets it as this on the CallFrame, delegates to the existing call path, and applies JS constructor return semantics (object result wins, otherwise return the allocated instance). A new test in test/js/bun/test/mock-fn.test.js covers Reflect.construct, new, custom newTarget, primitive/object-returning implementations, and a throwing implementation.
Security risks
None identified. This is test-runner mock machinery; no auth, crypto, filesystem, or untrusted-input parsing is involved. The construct path only handles values already reachable from the test's own code.
Level of scrutiny
Medium-high. This is hand-written C++ against JavaScriptCore internals, in the category REVIEW.md flags as most-blocked (native memory safety / exception checks). The change is small (~40 LOC) and follows established patterns from other host constructors in the repo, and the exception-scope handling looks complete. However, it is not a mechanical change: it mutates a live CallFrame (setThisValue) to reuse the call handler as a construct body, relies on the constructor-frame this/newTarget slot aliasing, and changes an observable Jest-compat behavior (mock.instances is now populated).
Other factors
- There is an open overlapping PR (#34575) that fixes the same JSMockFunction path plus JSFFIFunction and other native constructors. The author acknowledged it has merge conflicts and positioned this as the mergeable subset; a maintainer should decide whether to land this now or wait for the rebase of the broader fix.
- CodeRabbit's two nits were handled: the throw-during-construct test was added in f32c31e, and the author gave a reasonable justification for skipping the
setThisValuecomment (matches uncommented precedent in NapiClass.cpp / NodeDirent.cpp). - I did not identify any missing
RETURN_IF_EXCEPTION, GC-rooting gaps (the instance is on the conservatively-scanned stack and immediately stored through a WriteBarrier), or paths where the constructor could still return a non-object.
Given the native-bindings surface, the observable behavior change, and the pending scope decision vs. #34575, this warrants a human sign-off rather than auto-approval.
|
Closing this as a duplicate of #34575, which has been open since July 18 and covers a superset of this change. Same root cause and the same core fix (a dedicated
Fuzzilli has found this assertion seven times now; splitting review attention across two PRs is part of why it stayed unfixed, so keeping one. |
Fuzzer hit the
isCell()assertion inJSCJSValue.hwith:JSMockFunctionregisteredjsMockFunctionCallfor both call and construct. With no implementation that function returnsjsUndefined(), andJSC::Interpreter::executeConstructthen doesasObject()on the result (native constructors must return an object). In release builds this surfaced asReflect.construct(jest.fn(), [])evaluating toundefined.This adds a dedicated construct handler that allocates the instance (using
newTarget.prototypesoReflect.construct(mock, args, Sub)produces aSubinstance), records it inmock.instances, hands it to the shared call path asthis, and returns it whenever the implementation does not produce an object. As a side effectmock.instancesnow actually tracks constructed objects like Jest does (it was always empty before).Supersedes the closed #34656. #34575 is the broader fix (also covers JSFFIFunction and native module helpers) but has merge conflicts; this PR covers the JSMockFunction part and is cleanly mergeable.