Normalize scope-object this in mock function calls to prevent leak/crash

[robobun]

What

Fixes a fuzzer-found deterministic crash (fingerprint signal:SIGNED_RIGHT_SHIFT, SIGSEGV in llint_op_typeof_is_function) caused by a JSC-internal scope object escaping to JavaScript through a mock function.

Repro

const fn = Bun.jest().mock().mockReturnThis();
const capture = () => fn;   // closure-capture → the call below resolves `fn` through a scope
const leaked = fn();        // returns the enclosing JSLexicalEnvironment
typeof leaked === "function"; // dereferences a null cell in the LLInt → SIGSEGV

On a release build the same script returns [native code: JSLexicalEnvironment] from fn() instead of undefined — the engine-internal environment record is observable from JS.

Root cause

For a call like f() where f resolves through a scope (a closure-captured variable, a module binding, or a global), JSC's bytecode passes the resolved JSScope — a JSLexicalEnvironment, module environment, with scope, or the raw global object — as the unconverted this argument and relies on the callee's ToThis to normalize it (FunctionCallResolveNode::emitBytecode). JS callees run op_to_this; host functions read the raw slot.

jsMockFunctionCall reads callframe->thisValue() and, for a mockReturnThis()-configured mock, returns it directly (and also pushes it into mock.contexts). When the mock is scope-resolved, that raw value is a scope object. Once it's in JS, reading a binding still in its temporal dead zone yields the empty JSValue (the TDZ marker), and a later typeof x === "function" dereferences a null cell in the LLInt.

Fix

Normalize the this value with toThis(globalObject, ECMAMode::strict()) right after reading it in jsMockFunctionCall, mirroring the existing pattern in CallSite.cpp and JSBuffer.cpp. Strict mode is the correct choice here:

• a normal object this (e.g. obj.fn()) passes through unchanged — mock.contexts and mockReturnThis still observe it;
• undefined/null stay as-is — bare fn() already returned undefined, and that's preserved;
• a JSScope becomes undefined — exactly what a strict JS callee would see, and what closes the leak.

Tests

Added a regression test in mock-fn.test.js covering the closure-captured mockReturnThis case. Verified it fails without the fix (returns the leaked JSLexicalEnvironment) and passes with it. All 72 existing mock-fn tests still pass.

[robobun]

^{Updated 1:10 AM PT - May 30th, 2026}

✅ @robobun, your commit 009d791fb243a72a1d243331473737f9038d034b passed in Build #59173! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 31605

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

bun-31605 --bun

[coderabbitai]

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: 2f8a7b0e-c5ca-4688-a60f-e1aa1fe62481

📥 Commits

Reviewing files that changed from the base of the PR and between 7069b30 and 00965e0.

📒 Files selected for processing (2)

• src/jsc/bindings/JSMockFunction.cpp
• test/js/bun/test/mock-fn.test.js

---

Walkthrough

This PR fixes a bug where mock functions could leak engine-internal scope objects as this values. The implementation in JSMockFunction.cpp normalizes callframe->thisValue() via toThis() for strict mode, and a regression test in mock-fn.test.js verifies that mockReturnThis() on closure-captured mocks returns the correct value without leaking scope objects.

Changes

Mock function this value normalization

Layer / File(s)	Summary
Mock function this normalization and regression test src/jsc/bindings/JSMockFunction.cpp, test/js/bun/test/mock-fn.test.js	jsMockFunctionCall normalizes thisValue via toThis(globalObject, ECMAMode::strict()) to prevent scope object leakage through mockReturnThis() and contexts arrays. A regression test verifies that closure-captured mocks do not leak internal scope objects when mockReturnThis() is invoked.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main change: normalizing scope-object 'this' to prevent a leak/crash, which directly addresses the primary bug fix in this PR.
Description check	✅ Passed	The description comprehensively covers both required template sections: explains what the PR does (fixes a scope object leak crash in mock functions) and verifies the fix with a regression test and verification of existing tests.
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.}

[claude]

LGTM — surgical one-line fix using the established toThis(…, ECMAMode::strict()) pattern, with a regression test. The inline note about the same pattern in setSystemTime/useRealTimers is pre-existing and can be a follow-up.

Extended reasoning...

Overview

This PR is a one-line change to jsMockFunctionCall in src/jsc/bindings/JSMockFunction.cpp: it wraps callframe->thisValue() with .toThis(globalObject, JSC::ECMAMode::strict()) so that when a mock is called as a bare scope-resolved identifier, the engine-internal JSScope (e.g. JSLexicalEnvironment) is normalized to undefined instead of leaking to JS through mockReturnThis() / mock.contexts. A regression test is added to test/js/bun/test/mock-fn.test.js.

Security risks

None. This is test-runner mock infrastructure (bun:test / jest.fn()), not auth/crypto/network. The change closes an engine-internal object leak and a deterministic SIGSEGV; it doesn't introduce new surface area.

Level of scrutiny

Low-to-moderate. It's a single-expression change in JSC bindings, but it uses a well-established JSC API in exactly the way it's designed for — the identical .thisValue().toThis(&globalObject, ECMAMode::strict()) idiom already appears in JSBuffer.cpp, JSDOMOperation.h, JSEventTargetCustom.h, and JSEventEmitterCustom.h. The strict-mode semantics are correct: object this (e.g. obj.fn()) passes through unchanged, undefined/null stay as-is, and scope objects become undefined (matching what a strict JS callee would observe). Existing tests covering obj.fn() → obj and mock.contexts → [undefined, undefined] for bare calls confirm no behavioral regression.

Other factors

• The PR description includes a thorough root-cause analysis tracing the issue to FunctionCallResolveNode::emitBytecode and the host-function ToThis gap.
• All 72 existing mock-fn tests pass; the one CI failure (streams-leak.test.ts on x64-baseline) is unrelated to this change.
• No CODEOWNERS cover the touched files.
• The bug-hunter flagged a pre-existing instance of the same raw callframe->thisValue() return in JSMock__jsSetSystemTime / JSMock__jsUseRealTimers (and the Zig FakeTimers helpers). That's a valid "while you're here" — same root cause, same file — but it's not introduced or worsened by this PR and shouldn't gate this targeted crash fix. It can be addressed here or as a follow-up at the author's discretion.

[claude]

🟣 Pre-existing: the same raw callframe->thisValue() leak fixed here is still present ~600 lines down in JSMock__jsSetSystemTime (returns JSValue::encode(callframe->thisValue()) on both paths, lines ~1462/1467) and in JSMock__jsUseRealTimers (~1435). setSystemTime is exposed as a top-level bun:test export (jest.zig:224-225), so const { setSystemTime } = require("bun:test"); const c = () => setSystemTime; typeof setSystemTime(0) === "function" reproduces the same JSLexicalEnvironment leak / LLInt crash. The 7 return callframe.this() sites in FakeTimers.zig (useFakeTimers, useRealTimers, advanceTimersByTime, etc.) have the identical hazard. Since this is the same root cause and same fuzzer fingerprint being closed, it may be worth applying the same .toThis(globalObject, ECMAMode::strict()) (or returning jsUndefined()) to those while you're here.

Extended reasoning...

What the bug is

This PR correctly fixes jsMockFunctionCall by normalizing callframe->thisValue() via toThis(globalObject, ECMAMode::strict()) so that an engine-internal JSScope (e.g. JSLexicalEnvironment) cannot escape to JavaScript through mockReturnThis() or mock.contexts. However, the identical pattern remains unfixed in the same file:

BUN_DEFINE_HOST_FUNCTION(JSMock__jsSetSystemTime, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callframe))
{
    JSValue argument0 = callframe->argument(0);
    if (auto* dateInstance = dynamicDowncast<DateInstance>(argument0)) {
        ...
        return JSValue::encode(callframe->thisValue());   // <-- raw, no toThis()
    }
    ...
    return JSValue::encode(callframe->thisValue());       // <-- raw, no toThis()
}

and likewise JSMock__jsUseRealTimers at line ~1435. Additionally, src/runtime/test_runner/timers/FakeTimers.zig has 7 host functions (useFakeTimers, useRealTimers, advanceTimersToNextTimer, advanceTimersByTime, runOnlyPendingTimers, runAllTimers, clearAllTimers) that return callframe.this() raw with the same hazard.

Code path that triggers it

JSMock__jsSetSystemTime is registered directly on the bun:test module object in src/runtime/test_runner/jest.zig:224-225:

const setSystemTime = jsc.JSFunction.create(globalObject, "setSystemTime", JSMock__jsSetSystemTime, 0, .{});
module.put(globalObject, "setSystemTime", setSystemTime);

So const { setSystemTime } = require("bun:test") (or import { setSystemTime } from "bun:test") gives JS a bare host-function binding. When that binding is captured by a closure and called as a bare identifier, JSC's FunctionCallResolveNode::emitBytecode passes the resolved JSScope as the unconverted this argument. Native callees never run op_to_this, so callframe->thisValue() is the raw JSLexicalEnvironment, which is then returned straight to JS.

Why existing code doesn't prevent it

The PR description itself notes that ordinary JSFunction-wrapped host natives (citing CallSite.cpp / JSBuffer.cpp as precedent) need explicit toThis() normalization — the wrapper type doesn't shield them. JSMock__jsSetSystemTime is wrapped via jsc.JSFunction.create, which is exactly that case. There is no dynamicDowncast guard on thisValue in this function (unlike e.g. jsMockFunctionMockClear, which requires this to be a JSMockFunction and so can't leak a scope object).

Step-by-step proof

1. const { setSystemTime } = require("bun:test"); — setSystemTime is now a binding in the local lexical environment.
2. const capture = () => setSystemTime; — closure-capture forces setSystemTime to live in a heap-allocated JSLexicalEnvironment rather than a register.
3. const leaked = setSystemTime(0); — bytecode emits resolve_scope → finds the JSLexicalEnvironment → emits call with that scope object in the this slot. The native callee reads it unmodified and returns JSValue::encode(callframe->thisValue()), handing the JSLexicalEnvironment back to JS.
4. typeof leaked === "function" — llint_op_typeof_is_function on a JSLexicalEnvironment cell dereferences a null cell (or, in release builds, the scope object stringifies as [native code: JSLexicalEnvironment]) — the exact fuzzer fingerprint (signal:SIGNED_RIGHT_SHIFT, SIGSEGV) this PR is closing for mockReturnThis.

Impact

Same impact class as the bug being fixed: an engine-internal scope object becomes JS-observable, and reading a TDZ binding from it surfaces the empty JSValue, crashing the LLInt on a subsequent typeof. It's reachable from any test file that destructures setSystemTime (or the FakeTimers helpers) at module level into a closure-captured binding.

How to fix

Apply the same one-liner: replace callframe->thisValue() with callframe->thisValue().toThis(globalObject, JSC::ECMAMode::strict()) in JSMock__jsSetSystemTime (both return paths) and JSMock__jsUseRealTimers. For the Zig sites in FakeTimers.zig, either normalize via the equivalent toThis binding or simply return .js_undefined when callframe.this() isn't an object — the chaining return value isn't load-bearing for a bare call.

Severity

This is pre-existing — the PR does not touch these functions, add callers, or change their reachability. It's flagged because it's the same root cause, in the same file, producing the same fuzzer fingerprint the PR is meant to close, so the fuzzer will likely rediscover it. Not blocking, but a reasonable "while you're here".

[robobun]

Superseded by #32172, which contains this same jsMockFunctionCall fix plus the sibling sites the review here pointed out (JSMock__jsSetSystemTime, the Rust fake timer methods in FakeTimers.rs, and the now-dead C++ JSMock__jsUseRealTimers, which it deletes), along with the same pattern found in the Bun.plugin builder returns and the StringDecoder called-without-new path, with regression tests for each.