ErrorInstance: mark the stack frames while the embedder keeps them alive - #510
ErrorInstance: mark the stack frames while the embedder keeps them alive#510robobun wants to merge 1 commit into
Conversation
Add VM::setKeepsErrorStackFramesAlive(bool). While it is set, ErrorInstance::visitChildren marks the captured frames under the cell lock, so a live error keeps them until materializeErrorInfoIfNeeded runs. V8 keeps the frames alive the same way. Bun sets the flag while a user Error.prepareStackTrace is installed. Without it, a dead frame makes reconcileWeakReferencesAtGCEnd render the stack string in the GC end phase, where the prepareStackTrace callback cannot run. The user formatter is then skipped for that error and error.stack is the default string.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a visitChildren override that runs on GC marking threads, a human look would still be worthwhile.
What was reviewed:
- New
ErrorInstance::visitChildrenImplmirrorsException::visitChildrenImpl(sameStackFrame::visitAggregateloop);visitor.vm()is an established accessor. - All mutator writes to
m_stackTracein this file already holdcellLock(), so the lock invisitChildrenmatches the existing concurrency contract. - Flag defaults to
false, so the added override is a no-op (Base::visitChildren+ early return) until the embedder opts in; relaxed atomic read is documented as safe-to-stale. - Only JSC-side subclass user is
JSDOMErrorWrapper(WebCore, not built in JSCOnly) and it has novisitChildrenof its own.
Extended reasoning...
Overview
This PR adds an opt-in mechanism for ErrorInstance to strongly mark its captured StackFrames during GC, instead of holding them weakly. It touches three files: ErrorInstance.cpp gains a visitChildrenImpl (guarded by USE(BUN_JSC_ADDITIONS)) that, when VM::keepsErrorStackFramesAlive() is true, takes the cell lock and calls frame.visitAggregate(visitor) on each captured frame. ErrorInstance.h declares the override. VM.h adds a relaxed std::atomic<bool> m_keepsErrorStackFramesAlive with getter/setter.
Security risks
None identified. This is GC marking logic — no parsing, auth, or external input. The change can only cause objects to be kept alive longer (when the flag is set), which is a memory-retention trade-off the PR description explicitly discusses (V8 parity while Error.prepareStackTrace is installed), not a safety issue. The relaxed atomic is read-only from marking threads and a stale read degrades to today's weak-frame behavior.
Level of scrutiny
Moderate-to-high. visitChildren runs on concurrent marking threads, so mistakes here (missing locks, unguarded pointer reads) can cause hard-to-reproduce crashes. That said, the implementation is a near-verbatim copy of Exception::visitChildrenImpl (Exception.cpp:60-70), with the addition of the cell lock — which is the correct addition, since ErrorInstance::m_stackTrace is a unique_ptr mutated under cellLock() throughout the file (setStackFrames, captureStackTrace, finishCreation, computeErrorInfo, materializeErrorInfoIfNeeded, estimatedSize). The runtime flag defaults to false, so without the matching Bun-side change the new code path is inert.
Other factors
CREATE_METHOD_TABLE(ErrorInstance)will now pick up the newvisitChildren; the only subclass in the tree (JSDOMErrorWrapperin WebCore) doesn't define its ownvisitChildrenand isn't built for the JSCOnly/Bun port.- The added comment in
reconcileWeakReferencesAtGCEndcorrectly notes that with the flag on, no frame will be found dead there. - No bugs surfaced from the automated bug-hunting pass.
I'm deferring rather than approving because GC marking is a critical path where subtle races are possible, and a second pair of eyes on any new visitChildren override is cheap insurance — even though this one follows the established pattern closely.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesError stack frame GC retention
Merge Risk: 🔵 Low · up to This change keeps error stack frames alive while custom formatting is enabled, but a stale synchronization read could allow frames to be collected before formatting and produce the default stack string. The PR is mergeable with explicit owner confirmation of the feature guard and GC synchronization behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Warning Git: Failed to clone repository. Please run the Comment |
Preview Builds
|
|
Closing in favor of #511, which marks the frames unconditionally instead of behind |
1 participant
Problem
ErrorInstanceholds its captured frames weakly. Once one frame's callee or CodeBlock is unmarked,ErrorInstance::reconcileWeakReferencesAtGCEndrenders the stack string in the GC end phase throughvm.onComputeErrorInfo()and drops the frames.Error.prepareStackTracesupport lives inonComputeErrorInfoJSValue, whichmaterializeErrorInfoIfNeededonly calls while the frames still exist. So an error whose frames die before the first.stackread skips the user formatter and gets the default string. Every error thrown from an async function hits this once a GC runs between the throw and the first read (the async body runs in a per-call closure).Fix
VM::setKeepsErrorStackFramesAlive(bool). While it is set, the newErrorInstance::visitChildrenmarks the frames under the cell lock, so a live error keeps them untilmaterializeErrorInfoIfNeededruns.Exception::visitChildrenalready marks its own copy of the frames the same way.Error.prepareStackTraceis installed, so other programs keep the weak frames and the GC end phase rendering.USE(BUN_JSC_ADDITIONS)only. The matching Bun change sets the flag from theError.prepareStackTracesetter and adds the tests.