fix(expect): propagate exception from custom asymmetric matcher in asymmetricMatch#29199
fix(expect): propagate exception from custom asymmetric matcher in asymmetricMatch#29199
Conversation
…ymmetricMatch ExpectCustomAsymmetricMatcher.execute() is a callconv(.c) function that swallows Zig errors via `catch false` while leaving the JSC exception pending. When asymmetricMatch() called execute() and then returned a non-zero jsBoolean, the host-fn wrapper tripped releaseAssertNoException() because a pending exception existed alongside a non-zero return value. Check for a pending exception after execute() and return error.JSError so the exception is properly propagated to JavaScript.
|
Updated 11:01 PM PT - Apr 11th, 2026
❌ @robobun, your commit 2592c43 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29199That installs a local version of the PR into your bun-29199 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors asymmetric matcher execution to propagate JS exceptions: the core implementation now returns Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re: duplicate detection pointing at #28404 — related but not a duplicate. Both stem from
Propagation matches the existing behavior of the C++ expect.extend({ bad() { throw new Error("boom"); } });
expect({a: 1}).toEqual({a: expect.bad()}); // already throws "boom" on main
expect.bad().asymmetricMatch({}); // crashed on main; now throws "boom"I also verified #28404's repro ( The two PRs are incompatible: if #28404 merges, this PR's test would fail because the exception gets cleared before |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean one-line fix for the pending-exception / stale-boolean assertion crash, consistent with existing hasException patterns in the file.
Extended reasoning...
Overview
The PR modifies two files: a one-line fix in src/bun.js/test/expect.zig and a new regression test in test/js/bun/test/expect-extend.test.js. Scope is narrow and self-contained.
Root cause & fix
ExpectCustomAsymmetricMatcher.execute() uses catch false / catch return false which silently swallows error.JSError but leaves the JSC exception pending on the VM. asymmetricMatch() then returned a non-zero JSValue while an exception was still live, violating the JSC host-function contract and triggering releaseAssertNoException(). The fix adds the missing if (globalThis.hasException()) return error.JSError; check immediately after execute(), which is the standard pattern already used in at least two other places in this file (lines 343 and 1575).
Security risks
None. This is test-framework infrastructure; no auth, crypto, or permission code is touched.
Level of scrutiny
Low. The change is a single line that follows an already-established pattern, addresses a confirmed crash, and is accompanied by a focused regression test. The logic is easy to verify locally against the codebase.
Other factors
No prior reviews on this PR. No outstanding reviewer comments. Bug hunting system found no additional issues. The regression test is well-written and directly exercises the fixed code path.
There was a problem hiding this comment.
LGTM — minimal one-line fix with a regression test, correctly propagates the pending JSC exception from instead of returning a boolean while the exception is still set.
Extended reasoning...
Overview
Two files changed: one line added to src/bun.js/test/expect.zig and a regression test added to test/js/bun/test/expect-extend.test.js. The fix targets the ExpectCustomAsymmetricMatcher.asymmetricMatch function.
Root Cause and Fix
execute() is a callconv(.c) function that uses catch false to swallow Zig errors, but this leaves any pending JSC exception on the VM. asymmetricMatch() previously returned JSValue.jsBoolean(matched) unconditionally — a non-null value — while a pending exception violated the JSC host-function wrapper's releaseAssertNoException() invariant. The fix adds a single globalThis.hasException() check before returning, matching the same pattern already used on line 1575 of the same file.
Security Risks
None. This is a bug fix in the test framework's expect API, not security-sensitive code.
Level of Scrutiny
Low. The change is one line of Zig plus a test case, following an established pattern in the codebase. The PR description clearly explains root cause, fix, and verification. CodeRabbit found no actionable comments.
Other Factors
The PR author also explains why this PR is not a duplicate of #28404: that PR proposes silently clearing the exception, while this PR propagates it — which is the correct behavior that matches the existing C++ toEqual path. The regression test directly validates the fix.
Split execute() into a Zig-native executeImpl() returning bun.JSError!bool. The callconv(.c) execute() wrapper preserves existing behavior for the C++ matchAsymmetricMatcher caller, while asymmetricMatch() now calls executeImpl directly with try so exceptions propagate via the error union instead of needing an explicit hasException() check.
| const matcher_name = try matcher_fn.getName(globalThis); | ||
|
|
||
| // retrieve the asymmetric matcher args | ||
| // if null, it means the function has not yet been called to capture the args, which is a misuse of the matcher | ||
| const captured_args: JSValue = js.capturedArgsGetCached(thisValue) orelse { | ||
| globalThis.throw("expect.{f} misused, it needs to be instantiated by calling it with 0 or more arguments", .{matcher_name}) catch {}; | ||
| return false; | ||
| return globalThis.throw("expect.{f} misused, it needs to be instantiated by calling it with 0 or more arguments", .{matcher_name}); | ||
| }; | ||
| captured_args.ensureStillAlive(); | ||
|
|
||
| // prepare the args array as `[received, ...captured_args]` | ||
| const args_count = captured_args.getLength(globalThis) catch return false; | ||
| const args_count = try captured_args.getLength(globalThis); | ||
| var allocator = std.heap.stackFallback(8 * @sizeOf(JSValue), globalThis.allocator()); | ||
| var matcher_args = std.array_list.Managed(JSValue).initCapacity(allocator.get(), args_count + 1) catch { | ||
| globalThis.throwOutOfMemory() catch {}; | ||
| return false; | ||
| return globalThis.throwOutOfMemory(); | ||
| }; | ||
| matcher_args.appendAssumeCapacity(received); | ||
| for (0..args_count) |i| { | ||
| matcher_args.appendAssumeCapacity(captured_args.getIndex(globalThis, @truncate(i)) catch return false); | ||
| matcher_args.appendAssumeCapacity(try captured_args.getIndex(globalThis, @truncate(i))); | ||
| } | ||
|
|
||
| return Expect.executeCustomMatcher(globalThis, matcher_name, matcher_fn, matcher_args.items, this.flags, true) catch false; | ||
| return Expect.executeCustomMatcher(globalThis, matcher_name, matcher_fn, matcher_args.items, this.flags, true); |
There was a problem hiding this comment.
🟣 This PR introduces executeImpl without fixing two pre-existing memory leaks from the old execute(): (1) matcher_name (a ref-counted bun.String from getName()) is never deref'd, leaking a WTFStringImpl reference on every matcher invocation; (2) matcher_args ArrayList is never deinit'd — when args_count+1 > 8 the stackFallback spills to heap and that memory is never freed. Both leaks existed before this PR but the refactor is a good opportunity to fix them: add defer matcher_name.deref() after line 1703, and defer matcher_args.deinit() after the initCapacity call.
Extended reasoning...
matcher_name leak (bug_001): matcher_fn.getName(globalThis) calls JSC__JSValue__getName which internally calls Bun::toStringRef() (see BunString.cpp lines 270/278/286). toStringRef explicitly calls wtfString.impl()->ref() to increment the WTFStringImpl refcount, transferring ownership to the caller. The returned bun.String is stored in matcher_name at line 1703 (executeImpl) and line 1146 (applyCustomMatcher), but neither site has a corresponding defer matcher_name.deref(). The existing codebase demonstrates the correct pattern in multiple places: ScopeFunctions.zig lines 316-317 uses var description_name = try description.getName(globalThis); defer description_name.deref();, and lines 1906-1907 of the same file show const matcher_name = try arguments[0].toBunString(globalThis); defer matcher_name.deref();. Both toBunString and getName have the same ownership semantics via toStringRef.
matcher_args leak (bug_002): In executeImpl lines 1714-1720, a std.array_list.Managed(JSValue) is created using a stackFallback allocator with an 8-element (64-byte) stack buffer. When args_count+1 > 8 (i.e., 8 or more captured matcher args), initCapacity falls back to globalThis.allocator() for heap allocation. globalThis.allocator() returns bunVM().allocator, a general-purpose VM allocator — not an arena — so the memory must be explicitly freed. Neither the success path (line 1723 passes items to executeCustomMatcher but the list is never freed) nor any early-return error path calls matcher_args.deinit().
Pre-existing nature: Both bugs existed in the old execute() function before this PR. The PR refactors the logic into executeImpl() without fixing either leak. The same missing deinit pattern also appears at lines 1165-1170 (the symmetric matcher path). Since these are pre-existing, the PR does not worsen them significantly — the new asymmetricMatch call path was previously calling execute() anyway.
Concrete proof — matcher_name: For any named custom matcher (e.g., expect.myMatcher()), each call to executeImpl calls getName() which calls toStringRef which bumps the WTFStringImpl refcount from N to N+1. On function exit, that reference is never released. After 1000 calls, the refcount is N+1000 and the string's backing memory cannot be freed.
Concrete proof — matcher_args: Suppose a matcher is registered as expect.extend({ myMatcher: function(a,b,c,d,e,f,g,h) { ... } }) with 8 captured args. Then args_count = 8, args_count+1 = 9 > 8, so initCapacity(9) spills to heap. matcher_args.deinit() is never called, so 9 * sizeof(JSValue) bytes are leaked per call.
Fix: After line 1703: add defer matcher_name.deref();. After the initCapacity call: add defer matcher_args.deinit();. This mirrors the patterns used elsewhere in the codebase.
There was a problem hiding this comment.
These are both pre-existing leaks (as noted) that also exist in applyCustomMatcher at lines 1146 and 1165-1170 which this PR doesn't touch. Fixing them only in executeImpl but not in the sibling applyCustomMatcher would be inconsistent.
Keeping this PR scoped to the assertion crash fix; the leaks are worth addressing but belong in a separate change that covers both call sites.
1 participant
What does this PR do?
Fuzzilli found a debug assertion crash (fingerprint
3c2ad38d8c96b67d) where calling.asymmetricMatch()on a custom asymmetric matcher whose implementation throws triggers:Root cause
ExpectCustomAsymmetricMatcher.execute()is acallconv(.c)function (shared with the C++ deep-equals path) that handles errors withcatch false/catch return false. This swallows the Zigerror.JSErrorbut leaves the JSC exception pending on the VM.asymmetricMatch()then returnedJSValue.jsBoolean(matched)— a non-zero value — while a JSC exception was still pending. The host-function wrapper asserts that a non-zero return implies no pending exception, and trippedreleaseAssertNoException().Fix
After calling
execute(), checkglobalThis.hasException()and returnerror.JSErrorso the exception is properly propagated to JavaScript instead of returning a stale boolean.Repro
How did you verify your code works?
build/debug-fuzz/bun-debug(before), throws a catchable JS error (after).test/js/bun/test/expect-extend.test.jswhich crashes the unfixed debug build and passes with the fix.expect-extend.test.jssuite passes (aside from pre-existing "intensive usage" timeout that also fails on the unfixed build due to debug-build slowness).