Skip to content

Error.captureStackTrace: lazily compute .stack header on non-Error targets - #35640

Open
robobun wants to merge 6 commits into
mainfrom
farm/2631d62a/capture-stack-trace-non-error-header
Open

Error.captureStackTrace: lazily compute .stack header on non-Error targets#35640
robobun wants to merge 6 commits into
mainfrom
farm/2631d62a/capture-stack-trace-non-error-header

restore captured frames when a name/message getter throws

9b1d1eb
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 25, 2026 in 25m 57s

Code review found 2 potential issues

Found 2 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/jsc/bindings/FormatStackTraceForJS.cpp:799-808 Proto-chain walk continues past the jsNull() re-entrancy sentinel to an ancestor's capture
🟡 Nit src/jsc/bindings/FormatStackTraceForJS.cpp:827-830 Exception-path restore leaves callSites pinned when prepareStackTrace threw

Annotations

Check warning on line 808 in src/jsc/bindings/FormatStackTraceForJS.cpp

See this annotation in the file changed.

@claude claude / Claude Code Review

Proto-chain walk continues past the jsNull() re-entrancy sentinel to an ancestor's capture

The proto-chain walk and the `jsNull()` re-entrancy sentinel (both added in this PR's follow-up commits) interact incorrectly: when `v` is `jsNull()`, `v ? dynamicDowncast<JSArray>(v) : nullptr` yields `nullptr` and the loop *continues* to the prototype instead of stopping, so a re-entrant `.stack` read on a child whose own slot holds the sentinel walks past it and finds/formats an ancestor's unresolved CallSite array. Fix: break (and return `jsUndefined()`) when `v.isNull()`; only continue on e

Check warning on line 830 in src/jsc/bindings/FormatStackTraceForJS.cpp

See this annotation in the file changed.

@claude claude / Claude Code Review

Exception-path restore leaves callSites pinned when prepareStackTrace threw

The exception-path restore (`errorObject->putDirect(vm, privateName, callSites, 0)`) is only useful when `.stack` is still the CustomAccessor — but if the throw came from `Error.prepareStackTrace` itself, `formatStackTraceToJSValue(..., prepareStackTrace)` has already replaced `.stack` with a data property (the temp string) *before* invoking the callback, so the restored `JSArray<CallSite>` is never consumed and stays pinned to the target for its lifetime. This is the same bounded-retention shap