ErrorInstance: keep the captured stack frames alive until the error info is materialized - #511
ErrorInstance: keep the captured stack frames alive until the error info is materialized#511robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 41 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ 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.
I reviewed this PR and didn't find any bugs. Because it changes GC retention semantics for ErrorInstance stack frames (weak → strong), a human look would still be worthwhile to confirm the memory-footprint tradeoff is acceptable.
Checked that the visitChildrenImpl shape matches Exception::visitChildrenImpl (Exception.cpp:60) and that the cell lock is held consistently with the mutator-side writes to m_stackTrace in setStackFrames, captureStackTrace, computeErrorInfo, and materializeErrorInfoIfNeeded.
Confirmed CREATE_METHOD_TABLE(ErrorInstance) picks up the new visitChildren and there are no JSC-side ErrorInstance subclasses whose method tables would need updating.
The ASSERT_NOT_REACHED() fallback in reconcileWeakReferencesAtGCEnd still calls computeErrorInfo in release builds, so a missed write barrier degrades to the old behavior rather than a use-after-sweep.
Extended reasoning...
Overview
This PR adds a visitChildren implementation to ErrorInstance under USE(BUN_JSC_ADDITIONS) that visits every captured StackFrame's callee and code block via frame.visitAggregate(visitor). This changes the frames from weakly-held (upstream behavior) to strongly-held until materializeErrorInfoIfNeeded clears them or the error itself is collected. The reconciliation loop in reconcileWeakReferencesAtGCEnd is kept as a release-build fallback with a debug ASSERT_NOT_REACHED(). Header change is the matching DECLARE_VISIT_CHILDREN under the same guard.
Security risks
None identified. This is a GC marking change, not an attack-surface change. Strengthening a reference from weak to strong cannot introduce use-after-free; the risk direction is memory retention, not safety. The cell lock is taken during visitation, matching the mutator-side lock discipline already in place, so there's no new race on m_stackTrace.
Level of scrutiny
High. This is core JavaScriptCore GC marking behavior and a deliberate reversal of an upstream design choice. Upstream keeps frames weak specifically so an unread error doesn't pin functions and their global objects; this PR accepts that retention in exchange for correct Error.prepareStackTrace and header behavior on lazy .stack reads (matching V8's CallSiteInfo retention). The implementation itself is small and follows the established Exception::visitChildrenImpl pattern exactly, plus a lock. I found no correctness issues. But the memory-footprint tradeoff — long-lived unread errors now keep callees, code blocks, and transitively their globals alive — is a design call that a maintainer should sign off on rather than an automated reviewer.
Other factors
- The change is fully guarded by
USE(BUN_JSC_ADDITIONS), so upstream/non-Bun builds are unaffected. - The PR supersedes #302 and is paired with tests in a companion Bun PR; those tests aren't in this repo, so I couldn't verify them directly.
- No JSC-side subclasses of
ErrorInstanceexist that would need their ownvisitChildrenchaining updated; embedder subclasses (e.g. WebCore'sDOMException) useStackTraceCapturePolicy::DoNotCaptureper the header comment and would have a nullm_stackTraceanyway.
Preview Builds
|
6923046 to
24ab6e3
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The new visitChildrenImpl mirrors Exception::visitChildrenImpl and takes the cell lock consistently with the mutator sites. Because this changes GC retention semantics for ErrorInstance (weak → strong frame references) and is an intentional design tradeoff over two closed alternatives, a human look at the retention/memory implications would still be worthwhile.
Checked that all mutator sites touching m_stackTrace (setStackFrames, captureStackTrace, computeErrorInfo, materializeErrorInfoIfNeeded, finishCreation) hold cellLock() before swapping the vector, so the locked iteration in visitChildren is safe.
Verified StackFrame::visitAggregate is the same call Exception uses to keep its frames alive.
Confirmed the ASSERT_NOT_REACHED() in reconcileWeakReferencesAtGCEnd still falls through to computeErrorInfo in release builds.
Extended reasoning...
Overview
This PR adds a visitChildren override to ErrorInstance under USE(BUN_JSC_ADDITIONS) that visits every captured StackFrame's callee and code block via frame.visitAggregate(visitor). The effect is that stack frames captured at error construction are now held strongly by the error until .stack is materialized or the error is collected, instead of upstream's weak retention with pre-rendering in reconcileWeakReferencesAtGCEnd. The reconciliation loop is kept as a fallback with a debug ASSERT_NOT_REACHED(). Two files touched: ErrorInstance.h (adds DECLARE_VISIT_CHILDREN) and ErrorInstance.cpp (adds the impl and the assert).
Security risks
None identified. This is a GC marking change; it does not touch parsing, auth, or user-controlled data paths. The memory-safety aspect (concurrent GC iterating a vector the mutator can replace) is handled by taking cellLock(), and I verified every mutator write to m_stackTrace in this file also holds that lock.
Level of scrutiny
High. This is a change to GC visitation in JavaScriptCore's core runtime. Even though the diff is small (~30 lines) and follows the existing Exception::visitChildrenImpl pattern closely, it deliberately reverses upstream WebKit's design choice (weak frames so unread traces don't pin functions and their global objects). The PR description explicitly frames this as a design decision made against two closed alternatives (#510 conditional pinning, #302 header-only fix), citing V8/Node parity as the rationale. That kind of retention-semantics tradeoff — correctness of Error.prepareStackTrace and stack headers vs. potential memory retention of otherwise-dead functions/globals — is exactly what a human maintainer should sign off on, not an automated reviewer.
Other factors
- The implementation itself looks correct: it matches
Exception.cpp:60-70line-for-line in structure, plus the necessarycellLock()sinceErrorInstance::m_stackTraceis a replaceableunique_ptr(unlikeException::m_stackwhich is set once infinishCreation). - The debug assert in
reconcileWeakReferencesAtGCEndis defensive-only; release builds still fall through to the pre-render path, so a missed write barrier would degrade rather than crash. - Tests exist on the Bun side (oven-sh/bun#40354) but not in this repo.
- No prior human or bot review comments beyond CodeRabbit's rate-limit notice and the preview-build comment.
JSC held the frames captured by an ErrorInstance weakly. When a callee or code block in the trace died before the first .stack read, the GC end phase pre-rendered the stack string through a callback that gets no error instance and cannot run JS. The cached string had a bare "Error" header with no message, Error.prepareStackTrace never ran, and no call sites existed. Any error created inside a function object that is collected before the read hit this: an IIFE, a .then callback, the prologue of an async function, or a module's top-level code before its first await. oven-sh/WebKit#511 makes ErrorInstance::visitChildren visit the frames under the cell lock, so the first .stack read always sees live frames. This takes the cell lock at the two places where Bun mutates the frame vector of an ErrorInstance, adds the write barrier that Error.appendStackTrace needs now that the destination keeps the appended frames alive, and points WEBKIT_VERSION at the preview build of that change.
24ab6e3 to
ac15ee5
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes GC retention semantics for ErrorInstance — a deliberate divergence from upstream's weak-frame policy — a human sign-off on the design tradeoff would still be worthwhile.
What was reviewed:
visitChildrenImplmatches theException::visitChildrenpattern and correctly takescellLock()since the mutator swapsm_stackTraceunder that lock.DECLARE_VISIT_CHILDREN/DEFINE_VISIT_CHILDRENwiring —CREATE_METHOD_TABLEpicks up the new override without aStructureFlagschange.- The
ASSERT_NOT_REACHED()fallback inreconcileWeakReferencesAtGCEndstill callscomputeErrorInfoin release, so a missed write barrier degrades rather than crashes.
Extended reasoning...
Overview
This PR adds a visitChildren override to ErrorInstance (gated on USE(BUN_JSC_ADDITIONS)) that visits every captured StackFrame's callee and code block via frame.visitAggregate(visitor). It also adds a debug ASSERT_NOT_REACHED() in reconcileWeakReferencesAtGCEnd for the now-unreachable dead-frame path. Two files touched: ErrorInstance.h (declaration) and ErrorInstance.cpp (implementation + comment).
Security risks
None identified. No user-controlled input flows into the new code; it only marks already-captured GC cells during the visiting phase. The change increases retention (callees, code blocks, and transitively their global objects stay alive until .stack is read or the error dies), which is a memory-footprint concern rather than a security one.
Level of scrutiny
High. This modifies GC visiting for a core runtime object and intentionally reverses upstream WebKit's design choice to hold stack frames weakly (upstream's comment: "we might end up keeping functions (and their global objects) alive"). The PR description shows this is the third iteration of the design (after #510's conditional pinning and #302's header-only fix), and the choice to pin unconditionally is a tradeoff a maintainer should ratify — it's correct for Bun's prepareStackTrace semantics but changes retention for every unread error.
Other factors
The implementation itself looks correct: it mirrors Exception::visitChildrenImpl (Exception.cpp:60), takes cellLock() to synchronize with captureStackTrace/setStackFrames/computeErrorInfo which swap m_stackTrace under that lock, and null-checks the unique_ptr. The companion Bun PR has tests. I'm deferring not on implementation correctness but because the retention-policy decision is the kind of design call that benefits from human review.
ac15ee5 to
769c01b
Compare
769c01b to
878072a
Compare
878072a to
9f7f5a6
Compare
|
Cherry-picked the Windows arm64 CI fix from #523 (29c4c68) so this PR gets a preview build: every Preview Build since this morning fails at the scoop install step, which blocks the release job. It is a CI-only change with no effect on the built artifacts. I drop it from this branch once #523 or #524 lands on main. |
JSC held the frames captured by an ErrorInstance weakly. When a callee or code block in the trace died before the first .stack read, the GC end phase pre-rendered the stack string through a callback that gets no error instance and cannot run JS. The cached string had a bare "Error" header with no message, Error.prepareStackTrace never ran, and no call sites existed. Any error created inside a function object that is collected before the read hit this: an IIFE, a .then callback, the prologue of an async function, or a module's top-level code before its first await. oven-sh/WebKit#511 makes ErrorInstance::visitChildren visit the frames under the cell lock, so the first .stack read always sees live frames. This takes the cell lock at the two places where Bun mutates the frame vector of an ErrorInstance, adds the write barrier that Error.appendStackTrace needs now that the destination keeps the appended frames alive, and points WEBKIT_VERSION at the preview build of that change.
29c4c68 to
529efc3
Compare
529efc3 to
a93e3a5
Compare
…nfo is materialized ErrorInstance held its captured frames weakly. When a callee or code block in the trace died before the first .stack read, reconcileWeakReferencesAtGCEnd pre-rendered the stack string from the GC end phase through VM::onComputeErrorInfo. That callback gets no error instance and cannot run JS, so the cached string had a bare "Error" header with no message, Error.prepareStackTrace never ran, and no call sites existed. The first .stack read then served that string. Under USE(BUN_JSC_ADDITIONS), ErrorInstance::visitChildren now visits every frame's callee and code block, the way Exception already does. It holds the cell lock because the mutator replaces the vector under that lock. The frames stay alive until materializeErrorInfoIfNeeded drops them or the error dies, which is what V8 does with CallSiteInfo. The first .stack read then always takes the normal path with live frames. The weak reconciliation loop stays as a fallback for a frame stored without a write barrier and asserts in debug builds. Needed for oven-sh/bun#34398.
a93e3a5 to
a427c97
Compare
1 participant
Problem
ErrorInstanceholds its captured frames weakly. When a callee or code block in the trace dies before the first.stackread,reconcileWeakReferencesAtGCEnd(ErrorInstance.cpp:376) pre-renders the stack string from the GC end phase throughVM::onComputeErrorInfo. That callback gets no error instance and cannot run JS.Errorheader with no message,Error.prepareStackTracenever runs, and no call sites exist. The first.stackread serves that string. In Bun this hits every error created inside a function object that is collected before the read: an IIFE, a.thencallback, the prologue of an async function, or a module's top-level code before its firstawait(Async-thrown Error loses its message from error.stack when GC runs before first .stack access bun#34398 and its siblings).Fix
USE(BUN_JSC_ADDITIONS),ErrorInstance::visitChildrenvisits every frame's callee and code block, the wayException::visitChildrenalready does. It takes the cell lock because the mutator replaces the vector under that lock (captureStackTrace,setStackFrames,computeErrorInfo).materializeErrorInfoIfNeededdrops them or the error dies. V8 does the same withCallSiteInfo, so this is also the retention behavior Node programs expect..stackread now always takes the normal path with live frames. The weak reconciliation loop stays as a fallback for a frame stored without a write barrier, and asserts in debug builds.onComputeErrorInfoandonComputeErrorInfoJSValuecallbacks keep their signatures.Background
reconcileWeakReferencesAtGCEndruns on every markedErrorInstanceafter marking and before sweeping. Upstream keeps the frames weak so an unread trace does not keep functions and their global objects alive. Bun already formats.stacklazily throughonComputeErrorInfoJSValue, which needs live frames to build call sites.VM::setKeepsErrorStackFramesAlive(Bun: while a userError.prepareStackTraceis installed). Error stack GC finalizer: render only frame lines, add the name/message header at materialization #302 kept the pre-render and restored only the header at materialization. This PR pins unconditionally, so the header, the hook and the call sites all come from the first-access path, including a formatter installed after the GC and errors from other realms.Bun side: oven-sh/bun#40354. Needed for oven-sh/bun#34398.