Pass the error instance to the stack string callback#302
Conversation
ErrorInstance::finalizeUnconditionally computes and caches the stack string through VM::onComputeErrorInfo, which does not receive the ErrorInstance, so the embedder cannot include the error's name and message in the cached string. A later .stack access serves that cached string and the "Name: message" head line is lost. Add an optional VM::onComputeErrorInfoWithInstance callback that also receives the instance and is preferred over onComputeErrorInfo when set. The existing callback keeps its signature and behavior, so current embedders are unaffected until they opt in. Needed for oven-sh/bun#34398.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward additive callback that mirrors the existing onComputeErrorInfoJSValue pattern.
Checked that the new fnWithInstance branch in computeErrorInfo is byte-for-byte the same as the existing fn branch plus this, so behavior is unchanged when the callback is unset.
Verified all additions are inside USE(BUN_JSC_ADDITIONS) guards and the VM.h accessor/setter/member follow the same shape as m_onComputeErrorInfoJSValue.
Considered whether passing this from the finalizeUnconditionally path is unsafe — the instance is live there (finalizer runs on live objects to snapshot before frame cells die), and the existing JSValue callback already receives this from the materialize path.
Extended reasoning...
Overview
Adds a new optional VM::onComputeErrorInfoWithInstance callback (typedef, member, const/non-const accessors, setter) in VM.h and prefers it over onComputeErrorInfo in ErrorInstance::computeErrorInfo. The new callback has the same signature as the existing one plus a JSObject* for the error instance, exactly mirroring the pre-existing ErrorInfoFunctionJSValue shape. ~20 lines net, all inside #if USE(BUN_JSC_ADDITIONS).
Security risks
None. No parsing, no external input, no auth/crypto. The callback is embedder-registered (Bun), same trust level as the existing onComputeErrorInfo / onComputeErrorInfoJSValue hooks. The only new data flowing to the callback is this, which the JSValue variant already exposes.
Level of scrutiny
Low-to-moderate. It touches ErrorInstance::computeErrorInfo, which can run from the GC finalizeUnconditionally path — normally a sensitive area — but the change is purely additive: a new if (fnWithInstance) branch ahead of the existing if (fn) branch, with an identical body except for passing this. When the new callback is not set (the default), control flow is unchanged. The safety of what the callback does with the instance is deferred to the Bun-side PR, which is the right layering.
Other factors
- Follows an established pattern precisely: the typedef, accessor pair, setter, and member declaration are copy-paste of the neighboring
onComputeErrorInfo/onComputeErrorInfoJSValuelines with the type swapped. - No behavior change for any caller until
setOnComputeErrorInfoWithInstanceis invoked, which nothing in this repo does. - The bug hunting system found no issues.
finalizeUnconditionallyruns on liveErrorInstances (to snapshot the stack string before captured frame cells become unmarked), sothisis a valid pointer there; this is not the destroy path.
Preview Builds
|
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
We cannot access the object after it's been finalized.
2 participants
Problem
ErrorInstance::finalizeUnconditionallycomputes and caches the stack string throughVM::onComputeErrorInfowhen any captured stack frame is no longer marked. That callback does not receive theErrorInstance, so Bun cannot read the error'sname/messagethere, and the cached string begins with a bareErrorinstead ofError: the message. A later.stackaccess serves the cached string as-is.User-visible effect in Bun (oven-sh/bun#34398): an async-thrown error whose
.stackis first read after a GC cycle loses its message from the stack string.Change
Add an optional
VM::onComputeErrorInfoWithInstancecallback with the same shape asonComputeErrorInfoplus theJSObject*instance (mirroringonComputeErrorInfoJSValue).ErrorInstance::computeErrorInfoprefers it when set. The existing callback keeps its signature and priority, so embedders that do not set the new callback see no behavior change.The Bun-side change that registers the new callback follows in oven-sh/bun (it reads name/message via the VMInquiry-based
sanitizedNameString/sanitizedMessageString, which do not run user code, so it is safe from the finalizer).