ErrorInstance: pass the instance to the finalizer-time error-info hook (onComputeErrorInfoWithErrorInstance) - #445
Conversation
… (onComputeErrorInfoWithErrorInstance) ErrorInstance keeps its captured stack frames weakly. When a GC finds that one of them died (a callee closure that is no longer reachable, a CodeBlock jettisoned for old age) before .stack was ever read, finalizeUnconditionally() flushes the frames to a string through vm.onComputeErrorInfo(). That hook receives only the frames, not the error, so the embedder cannot render the error's name and message into the header line of the string. Bun's V8-style formatter therefore hardcodes "Error" there, and an error that was kept around across a GC ends up with a .stack of "Error\n at ..." instead of "TypeError: <message>\n at ..." (the same error's .stack is right whenever no GC runs in between). onComputeErrorInfoJSValue(), the hook used when the frames are still alive, already receives the instance. Add onComputeErrorInfoWithErrorInstance(), which is the same as onComputeErrorInfo() plus the ErrorInstance, and prefer it in computeErrorInfo() when it is set. The old hook is left in place and still used as the fallback so that a Bun that has not switched over yet keeps building and behaving as before; it can be removed once Bun only installs the new one.
|
Warning Review limit reached
Next review available in: 4 minutes 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small additive hook that mirrors the existing ErrorInfoFunctionJSValue pattern.
What was reviewed:
- New hook is preferred only when set;
onComputeErrorInfo()fallback and the no-hookstackTraceAsStringpath are unchanged, so current Bun builds behave identically. - Passing
thison thefinalizeUnconditionallypath is fine — the instance is live there (same path already passesthis->bunErrorData()and holdsthisinBunErrorInstanceFinalizer); the no-JS-heap-allocation constraint is documented on the typedef. - Getter/setter/member additions in
VM.hare copy-paste of the sibling hooks and stay underUSE(BUN_JSC_ADDITIONS).
Extended reasoning...
Overview
Adds a second variant of the finalizer-time error-info hook, VM::onComputeErrorInfoWithErrorInstance(), whose signature is identical to the existing onComputeErrorInfo() plus a JSObject* errorInstance parameter. ErrorInstance::computeErrorInfo prefers the new hook when set and falls back to the old one otherwise. Touches two files: a typedef + getter/setter/member in VM.h (all under USE(BUN_JSC_ADDITIONS)), and a 4-line branch in ErrorInstance.cpp.
Security risks
None. This is embedder plumbing: a WTF::Function slot on VM and one extra branch at the existing call site. No parsing, no untrusted input, no allocation-pattern changes. The JSObject* passed is this, which is already in scope and already partially exposed on this path via this->bunErrorData().
Level of scrutiny
Low-to-moderate. The touchpoint (finalizeUnconditionally → computeErrorInfo) is GC-adjacent, which normally warrants care, but the change does not alter what runs there — it only threads an extra pointer to the same hook shape that already exists (ErrorInfoFunctionJSValue already takes JSObject*). The ErrorInstance is guaranteed live during its own unconditional finalizer, and the no-JS-heap-allocation constraint that already applied to onComputeErrorInfo is restated on the new typedef. Because the new hook is unset until the companion Bun PR lands, this change is behaviorally a no-op on its own.
Other factors
- Purely additive and backward-compatible by design (PR description calls this out; verified in the diff that the
fnfallback and them_stackPropertyAlreadyMaterializedshort-circuit are preserved). - Follows the exact pattern of the three sibling hooks (
onComputeErrorInfo,onComputeErrorInfoJSValue,onAppendStackTrace) — same const/non-const accessor pair, sameWTF::movesetter, same member placement. - The upstream CODEOWNERS entry for
Source/JavaScriptCorenames an Apple reviewer team that does not apply in this fork; recent history shows Bun-specific JSC changes merged routinely.
Preview Builds
|
|
Closing: this duplicates #302, which adds the same callback ( |
1 participant
Problem
ErrorInstanceholds its captured stack frames weakly. When a GC finds that one of them died before.stackwas read (a callee closure nothing references anymore, a CodeBlock jettisoned for old age),ErrorInstance::finalizeUnconditionallyflushes the frames to a string right there, throughvm.onComputeErrorInfo()(ErrorInstance.cpp,computeErrorInfo).Errorthere, so an error kept alive across a GC reads back.stackas"Error\n at ..."instead of"TypeError: <message>\n at ...". The same error formats correctly if no GC happens in between, which is what makes it show up as flaky (first seen in bun's expressres.sendFiletest, where finalhandler renderserr.stackinto the response body).onComputeErrorInfoJSValue(), the hook used when the frames are still alive, already receives the instance; only the finalizer-time hook lacks it.Fix
VM::onComputeErrorInfoWithErrorInstance()/setOnComputeErrorInfoWithErrorInstance(): same signature asonComputeErrorInfo()plus theErrorInstance(asJSObject*, mirroringErrorInfoFunctionJSValue).ErrorInstance::computeErrorInfoprefers it when set and otherwise falls back toonComputeErrorInfo()as before.onComputeErrorInfo()can be deleted.name/messageoff the instance's property storage withStructure::getConcurrently+JSString::tryGetValueWithoutGC, the same way it already reads function names on this path.The bun side (installing the new hook and the regression test) is in a separate oven-sh/bun PR that bumps
WEBKIT_VERSIONto this change.