Error.captureStackTrace: keep frames when the callee frame was elided by a tail call - #32136
Error.captureStackTrace: keep frames when the callee frame was elided by a tail call#32136robobun wants to merge 4 commits into
Conversation
|
Updated 1:37 AM PT - Jul 14th, 2026
✅ @robobun, your commit ee01e06106171fe051d62db4126144085966514f passed in 🧪 To try this PR locally: bunx bun-pr 32136That installs a local version of the PR into your bun-32136 --bun |
WalkthroughAdds tail-call elision detection and updates JSCStackTrace::getFramesForCaller to use an optional removeCount; when the caller isn't found, the stack is only cleared if the caller could not have been tail-call-elided. Tests for various missing-caller scenarios are added. ChangesStack trace tail-call elision fix
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs and the heuristic looks sound, but since this is a deliberate divergence from V8's clear-everything semantics with a JSC-internals-based tradeoff (e.g., a strict function that was called once but is genuinely off-stack will now keep frames), it's worth a human signing off on the design choice.
Extended reasoning...
Overview
This PR changes JSCStackTrace::getFramesForCaller in src/jsc/bindings/ErrorStackTrace.cpp so that Error.captureStackTrace(err, fn) no longer wipes the entire trace when fn is absent from the stack, if fn's frame could plausibly have been elided by a JSC proper tail call. A new static helper callerCouldBeTailCallElided encodes the conditions under which JSC's bytecode generator emits tail calls (strict-mode JS function with call-codegen; bound functions and callable proxies treated conservatively). Four new tests pin both the fix (zod-shaped tail call, bound function) and the preserved V8 wipe behavior (host function, sloppy-mode function). I verified the existing not-in-stack tests still hold because their callee functions are never invoked, so isGeneratedForCall() is false and the wipe path is taken.
Security risks
None. This only affects which frames appear in a string stack trace; no auth, crypto, permissions, or untrusted-input parsing is touched. The worst case is a stack trace with extra frames.
Level of scrutiny
Medium-high. The implementation is small (~40 LOC) and well-commented, but it is a semantic design decision about Node.js compat in the presence of JSC tail calls, encoded as a heuristic that reaches into engine internals (FunctionExecutable::isInStrictContext, isGeneratedForCall, JSBoundFunction, InternalFunction). The PR explicitly accepts a tradeoff — "extra frames beat an empty stack" — meaning a strict-mode function that was called earlier but is legitimately not on the stack now keeps the full trace where Node would clear it. That's reasonable, but it's the kind of compat-vs-usability call a maintainer should ratify rather than a bot.
Other factors
The PR is thoroughly tested and documented, CodeRabbit had no findings, and the bug-hunting pass found nothing. No CODEOWNERS cover these paths. My deferral is purely about the design tradeoff, not a correctness concern.
|
CI across every run of this diff has been green except for a rotating set of Buildkite-flagged flakes on lanes this change does not touch:
A different unrelated test flakes in each build, none of them executes |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Remove all the comments
|
Done in 8029835. |
8029835 to
2f6a8a8
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this changes Error.captureStackTrace's caller-filtering semantics via a JSC-internals heuristic (strict-mode + isGeneratedForCall()), which is a Node-compat design tradeoff worth a maintainer's eyes.
Extended reasoning...
Overview
This PR modifies JSCStackTrace::getFramesForCaller in src/jsc/bindings/ErrorStackTrace.cpp so that when the caller passed to Error.captureStackTrace(err, caller) is not found on the stack, the trace is only cleared if the caller provably could not have been tail-call-elided. A new static helper callerCouldBeTailCallElided classifies the caller based on JSC internals: bound functions and strict-mode JS functions that have generated call code are treated as possibly-elided (keep frames); host functions, InternalFunctions, sloppy-mode functions, and never-called functions still get the V8 wipe. Four new tests pin both the new keep-frames cases and the preserved wipe cases.
Security risks
None identified. The change only affects which stack frames appear in an error's .stack string/CallSite array — no auth, crypto, permissions, or untrusted-input parsing is touched. The added code performs type checks (dynamicDowncast) on a JSObject* already validated as non-null by the surrounding code.
Level of scrutiny
Medium-high. This is a small diff but it lives in core runtime stack-trace machinery and deliberately diverges from V8/Node semantics for a class of callers. The heuristic leans on FunctionExecutable::isGeneratedForCall(), meaning observable behavior depends on whether the passed function has ever been invoked as a regular call — a subtle invariant that keeps the existing "not in stack → clear" tests passing only because their sentinel functions are never called. Whether "extra frames beat an empty stack" is the right compat tradeoff (and whether the bound-function / fallthrough-true cases are correctly scoped) is a judgment call a maintainer with JSC/PTC expertise should confirm.
Other factors
The PR is well-motivated (fixes the real-world zod #13904 empty-stack problem), well-described, and adds targeted regression tests for both directions of the behavior. CI is effectively green per the robobun analysis (only unrelated infra/flake failures). No prior human review or CODEOWNERS coverage on this path. Given the design-tradeoff nature and the JSC-internal subtlety, deferring rather than auto-approving.
2f6a8a8 to
054511e
Compare
|
Rebased onto main and resolved the conflict. It was in |
There was a problem hiding this comment.
I didn't find any bugs, but this intentionally diverges from V8's captureStackTrace semantics via a JSC-internals heuristic (strict-mode + isGeneratedForCall), so a maintainer should sign off on the compat tradeoff.
Extended reasoning...
Overview
Adds callerCouldBeTailCallElided() in src/jsc/bindings/ErrorStackTrace.cpp and changes getFramesForCaller to use std::optional<size_t> removeCount: when the callee passed to Error.captureStackTrace(err, fn) isn't found on the stack, the trace is now only cleared if fn provably could not have been tail-call-elided (host function, InternalFunction, sloppy-mode, or never generated code for a call). Otherwise the collected frames are kept. Four new tests pin both sides of the new behavior.
Security risks
None. Purely diagnostic — affects which frames appear in .stack strings. No new user-controlled input reaches allocation or control flow; jsExecutable() is only called after isHostFunction() returns false, which guarantees a FunctionExecutable.
Level of scrutiny
Moderate-to-high. The C++ itself is small and safe, but this is a deliberate behavioral divergence from Node/V8 in a core compat API, gated on a heuristic that inspects JSC-internal state (isInStrictContext, isGeneratedForCall). The design question — "keep possibly-extra frames vs. wipe everything when tail calls hide the callee" — is a maintainer judgment call, not a mechanical fix. It also relies on the observation that a strict function that has generated call code might have tail-called, which is conservative but means any strict function that was ever invoked will now keep frames when passed as an unfindable callee (a change from prior behavior even in non-tail-call cases).
Other factors
- Existing not-in-stack tests still pass because their callee functions are never invoked, so
isGeneratedForCall()is false and the wipe path is taken. - The fallthrough
return truefor arbitrary non-function objects (e.g.Error.captureStackTrace(e, {})) now keeps frames; the existing edge-case test only assertsstack !== undefinedso it doesn't catch this, but it is a semantic change. - No prior human review on the PR; CI is green on the touched test file across all lanes.
Given the compat-policy nature of the change, deferring to a human.
|
Good catch on the non-callable-object case. I verified it against Node 24, and the change there is actually a move toward V8 parity, not away from it:
Added a regression test for it in 6438922 ( The strict-mode + |
…was elided by a tail call JSC implements ES2015 proper tail calls, so a strict-mode function whose body ends in a tail call has its frame replaced by its callee's. When such a function is passed as the second argument to Error.captureStackTrace, it cannot be found on the stack, and the previous behavior (remove every frame, matching V8's skip-until-seen semantics) destroyed the entire trace. zod v4's .parse() does exactly this on every failed parse, leaving ZodErrors with zero stack frames. Keep the collected frames when the callee's frame could have been tail-elided. Still clear the trace, matching Node, when the callee provably never had an elidable call frame: host functions, native constructors, sloppy-mode functions, and functions that never generated code for a regular call. Bound functions keep the trace too, matching Node, where V8 never matches bound functions against stack frames.
|
Rebased onto main again. Same trivial conflict shape as before: main appended a new test (#34095 lazy error-info materialization) at the end of |
6438922 to
ee01e06
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this changes Node-compat semantics for Error.captureStackTrace via a heuristic tied to JSC compilation state, and the author explicitly flagged the keep-vs-wipe tradeoff as a maintainer policy call — worth a human look.
Checked: the std::optional<size_t> refactor preserves the found-caller path exactly; existing not-in-stack tests (notInStack, f9, S.constructor) still wipe because never-called strict functions return false from isGeneratedForCall() and Function is an InternalFunction. The non-callable-object fallthrough now matches Node's "only filter when callable" rule (previously inconsistent). jsExecutable() is safe to dereference after the isHostFunction() guard.
Extended reasoning...
Overview
Two files: src/jsc/bindings/ErrorStackTrace.cpp adds a static callerCouldBeTailCallElided helper and changes getFramesForCaller to use std::optional<size_t> removeCount so that when the caller isn't found on the stack, frames are only wiped if the caller provably couldn't have been tail-call-elided. test/js/node/v8/capture-stack-trace.test.js adds five tests covering the tail-call case (zod shape), host function, sloppy-mode function, bound function, and non-callable object.
Security risks
None. This only affects the diagnostic content of error.stack strings — no auth, crypto, permissions, or untrusted-input parsing paths are touched.
Level of scrutiny
Medium-high. The mechanical change (optional removeCount, conditional wipe) is small and the code itself is safe — jsExecutable() is only called after !isHostFunction() so it's non-null, and all downcasts are dynamicDowncast. But the behavior change is a deliberate divergence from V8/Node semantics for a subset of cases: strict-mode functions that have been called at least once (isGeneratedForCall() true) but aren't currently on the stack will now keep the full trace instead of wiping it. The heuristic is tied to JSC's lazy-compilation state, which means observable behavior can differ depending on whether a function has been invoked before. The PR author explicitly noted in the thread that "the strict-mode + isGeneratedForCall() heuristic and the keep-vs-wipe tradeoff are still a compat-policy call for a maintainer."
Other factors
- I traced the existing tests that assert wipe behavior (
captureStackTrace with constructor function not in stack,capture stack trace second argumentwithf9/S.constructor, and the asynccaller not in stack clears async frames too) — they all still wipe under the new logic because the functions are either never called (soisGeneratedForCall()is false) or areInternalFunctions. - The non-callable-object case (
{},[]) changing from wipe→keep is a genuine improvement toward Node parity, verified in the thread against Node 24. - Test coverage is good: both the new keep-frames cases and the preserved wipe cases are pinned.
- CI has been green on the touched test file across multiple rebases; the bug hunting system found nothing.
Given this is a Node-compat behavior heuristic that the author flagged for maintainer review, and it touches how every Error.captureStackTrace(err, fn) call with an unfindable fn behaves, I'm deferring rather than auto-approving.
|
Re-checked against current main (165dc9f) since #13904 was closed: this PR is still needed. #13904 was closed on its original repro; the tail-call case from #32132 (merged into it) still reproduces on main: |
2 participants
What does this PR do?
Fixes #13904.
Error.captureStackTrace(err, fn)cleared the entire stack trace wheneverfncould not be found on the stack. That matches V8's skip-until-seen semantics, but JSC implements ES2015 proper tail calls: a strict-mode function whose body ends in a tail call has its frame replaced by its callee's, so a function that is logically mid-execution can be unfindable. "Not on the stack" no longer implies "never called", and the V8 behavior degrades into destroying the whole trace.zod v4 hits this on every failed
.parse(): the callee it passes isinst.parse = (data, params) => parse.parse(inst, data, params, { callee: inst.parse }), an implicit-return tail call, so every ZodError arrived with an empty stack:Before: the ZodError has zero
atlines. After, the trace keeps the frames that exist, including the user's call site:The fix
JSCStackTrace::getFramesForCallernow only clears the trace when the callee's frame provably could not have been tail-elided, mirroring the conditions under which the bytecode generator emits tail calls (m_allowTailCallOptimization):InternalFunction), and sloppy-mode functions never make tail calls, and a function that never generated code for a regular call was never on the stack (construct invocations are never tail calls): these still clear the trace, matching Node.Frames that JSC's tail calls elide stay elided; per #26001 tail calls stay enabled for performance, so exact Node frame-for-frame parity in those traces is out of scope. This PR removes the failure mode where a trimming heuristic deletes every frame that does exist.
How did you verify your code works?
New tests in
test/js/node/v8/capture-stack-trace.test.js:captureStackTrace keeps frames when the caller frame was elided by a tail call(the zod shape) andcaptureStackTrace keeps frames for a bound function not in the stackboth fail on the unfixed build (stack is bareError: ...) and pass with the fix.captureStackTrace still clears frames for a host function not in the stackand... for a sloppy-mode function not in the stackpin the preserved V8/Node wipe behavior (output verified against Node 24).All 42 tests in the file pass, including the existing caller-not-in-stack tests from #27017 and #28651, which keep asserting the Node-matching cleared trace. Also ran the jsc-stress suite (83 pass), assert suites, and the sourcemap suites.
[review] gate passed · iteration 8 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 8
evidence per changed file
root cause · written by the author bot
The bug was in Bun's implementation of Error.captureStackTrace frame filtering in ErrorStackTrace.cpp, which trimmed one frame beyond the matched callee and, when the second argument never matched a frame on the stack (as with zod's arrow-function callee or a non-callable value), defaulted the removal count to the entire stack, wiping every frame. V8's semantics are to drop only the frames above and including the given function and to leave the trace untouched when that function is not found. The fix rewrites the filtering to stop removal exactly at the matched callee frame and to keep all …