Fix assertion failure in Bun.plugin when target coercion throws#29589
Fix assertion failure in Bun.plugin when target coercion throws#29589robobun wants to merge 1 commit into
Conversation
When the target option passed to Bun.plugin() was a Symbol or an object whose string coercion throws, toStringOrNull() would return null with a pending exception. The code would then fall through to constructEmptyObject with the exception still pending, tripping assertNoException() in debug builds.
|
Updated 8:05 AM PT - Apr 22nd, 2026
❌ @robobun, your commit a6e3948 has some failures in 🧪 To try this PR locally: bunx bun-pr 29589That installs a local version of the PR into your bun-29589 --bun |
There was a problem hiding this comment.
LGTM — straightforward exception-scope fix following the established RETURN_IF_EXCEPTION pattern, with regression tests.
Extended reasoning...
Overview
This PR adds two RETURN_IF_EXCEPTION checks in setupBunPlugin() (src/bun.js/bindings/BunPlugin.cpp) to handle the case where toStringOrNull() on the target option returns null with a pending exception (e.g., Symbol or a throwing Symbol.toPrimitive). It also adds two regression tests to plugins.test.ts.
Security risks
None. This only adds early-return guards on an existing throw scope; no new inputs, no auth/crypto/permission surface.
Level of scrutiny
Low. The change is a 2-line mechanical application of the standard JSC RETURN_IF_EXCEPTION(throwScope, {}) pattern already used dozens of times in the same file. The fix is purely defensive — it converts a debug-only assertion failure into a properly propagated JS exception, with no behavior change on the happy path.
Other factors
- The bug hunting system found no issues.
- The added tests directly cover both the Symbol case and the throwing-coercion case, and the PR description confirms the full plugins.test.ts suite passes.
- The first inner check after
->value(globalObject)is slightly redundant in practice but harmless and consistent with surrounding code; the outer check is the actual fix and is correctly placed to catch the null-return-with-exception path beforeconstructEmptyObject().
|
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 (2)
WalkthroughAdded exception handling in Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
|
Duplicate of #28369 which has the identical fix. |
What does this PR do?
Fixes a debug assertion failure (
assertNoException) inBun.plugin()when thetargetoption is a value whose string coercion throws — e.g. aSymbol, or an object with a throwingSymbol.toPrimitive.toStringOrNull()returnsnullptrwith a pending exception in these cases. The surrounding code only handled the non-null branch and fell through toconstructEmptyObject()with the exception still pending, which tripsExceptionScope::assertNoException()in debug/ASAN builds.Found by Fuzzilli (fingerprint
15b3dd05628c2f45).How did you verify your code works?
Minimal repro that asserted before and now throws a clean
TypeError:Added regression tests to
test/js/bun/plugin/plugins.test.tscovering both theSymbolcase and a throwingSymbol.toPrimitive. Fullplugins.test.tssuite passes (31 pass, 1 todo).