node:vm: apply negative lineOffset/columnOffset to stack frames and the error header - #38248
node:vm: apply negative lineOffset/columnOffset to stack frames and the error header#38248robobun wants to merge 6 commits into
Conversation
…he arrow header JSC::SourceCode clamps the line and column a source starts at to 1, so code compiled with a negative node:vm lineOffset or columnOffset reported physical positions: the offset only worked when it was positive. The SourceProvider still carries the requested start, so the stack formatters (error.stack, Error.prepareStackTrace call sites, Bun.inspect and the uncaught error printer) now add the negative part back, which is what V8 reports for the same options. handleException computed the physical line for the arrow header by subtracting the unclamped (negative) start, so it looked up a line that does not exist (no source line or caret) or, with lineOffset -1, showed the line after the throw. It now subtracts only the part JSC actually added and reports the line with the whole offset, like Node. compileFunction's wrapper shift is now unconditional, so body line N reports as N + lineOffset for zero and negative offsets as well.
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit e697683 has some failures in 🧪 To try this PR locally: bunx bun-pr 38248That installs a local version of the PR into your bun-38248 --bun |
|
Reproduced on 1.4.0 and a debug build of main (b5afcac) with: const vm = require("node:vm");
try {
new vm.Script("1;\n2;\n3;\nthrow new Error('x')", { filename: "n.js", lineOffset: -2 }).runInThisContext();
} catch (e) { console.log(e.stack.split("\n").slice(0, 6)); }Before: Tests: CI state (head e697683, no code change since fbcfcb3): every job that ran passed (177/179 in build 95880, same in 95340). The two remaining jobs are the darwin 14 aarch64 test lanes, which expired without running in 95340 and have been waiting for an agent in 95880; the test files Buildkite lists as flaky in those builds passed on retry or alone and none involve this change (the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
adjustPositionBackwards walks the position back to the `new` keyword in the coordinates JSC reported and recounts the column from the source when it crosses a line, so the offset has to be added once it is done: added first, a negative column offset could trigger that recount (and be dropped by it), and a position walked back onto the first line missed the column offset that applies there.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Follow-ups pushed since the review started:
|
There was a problem hiding this comment.
Re-reviewed after 5a9d4a2/1daa1e2/e2c6cb4 — the ordering issue I flagged earlier is fixed (the negative-offset step now runs after the op_construct walk, with the line-crossing new\nError case added to the prepareStackTrace test), and no new issues were found this run. A human look would still be worthwhile given the overlap with #37396/#38240 and the neighbouring constructAnonymousFunction PRs the description calls out.
What was checked: getLineColumnForStackFrame is only reached behind frame.hasLineAndColumnInfo(), so frame.codeBlock() is non-null there; applyNegativeSourceStart early-returns on a null provider and is a no-op when the provider start is non-negative, so non-vm frames in formatStackTrace are unchanged; the INT_MIN guard on startLine-- avoids the signed-overflow the old clamp incidentally prevented.
Extended reasoning...
Overview
Adds Bun::applyNegativeSourceStart in ErrorStackFrame.cpp to re-apply the negative part of a node:vm lineOffset/columnOffset that JSC::SourceCode clamps away, and threads it through the three position-reading paths: getAdjustedPositionForBytecode (CallSites / Bun.inspect / uncaught printer), a new getLineColumnForStackFrame used by formatStackTrace (error.stack), and handleException's arrow-header in NodeVM.cpp. constructAnonymousFunction now decrements the wrapped start line unconditionally (with an INT_MIN guard) so compileFunction reports body line N as N + lineOffset for every sign. Tests: 23 new cases in test/js/node/vm/vm.test.ts plus updated Bun-branch expectations in test-vm-basic.js.
Security risks
None identified. The change is arithmetic on line/column ordinals for error-message formatting; inputs are already range-validated int32 by BaseVMOptions::fromJS. No allocation sizing, no untrusted-length indexing.
Level of scrutiny
Moderate-to-high. The helper itself is small, but it now runs inside formatStackTrace — the shared error.stack path for every error in the runtime. Its correctness depends on a JSC-internal invariant (only node:vm providers ever carry a negative startPosition) that I could not verify against the vendored WebKit sources in this checkout, though the reasoning in the PR description is sound and the null-provider / non-negative early-outs make it a no-op elsewhere by construction.
Other factors
- The ordering bug from my previous review was fixed in 5a9d4a2 and covered by the new
" new\nError('x')"test case, so theadjustPositionBackwardsrecompute now runs on JSC's clamped coordinates before the offset is folded in. - The comment-cop feedback was addressed in 1daa1e2/e2c6cb4.
- The finder-raised null-CodeBlock concern was checked:
getLineColumnForStackFrame's only caller guards onhasLineAndColumnInfo(). - The description flags textual overlap with #37396, #38240, #38228, #38236, #38239, #38245 in the same functions; sequencing those merges is the main reason to keep a human in the loop rather than auto-approve.
…bage Offsets larger than the line now produce frames whose line is 0 or negative. error.stack prints them as V8 does; the renderers that cannot: - the error printer's stack-string parser read line numbers as unsigned, so "at f (x.js:-6:9)" came back as url "x.js:-6" with the column as its line; it now parses them signed, and such lines count as no position - the printer wrote the ":" separator when only the column was valid, leaving "at f (x.js:)" - error.line / error.column are unsigned in ErrorInstance and wrapped around; they now read as 0 populateStackFramePosition's no-bytecode-index fallback goes through getLineColumnForStackFrame like formatStackTrace does.
|
Pushed fbcfcb3 after going through the change once more for inputs whose offset is larger than the line (lines 0 or below). The description now also spells out the one trade-off of keeping the bias in the provider's start position (JSC's debugger refuses breakpoints in sources with a negative start; previously already the case for |
There was a problem hiding this comment.
The reordering in 5a9d4a2 addresses the earlier finding — applyNegativeSourceStart now runs after the op_construct walk-back, and the new\nError test case covers the line-crossing path. I didn't find further issues this pass, but given this touches every stack-frame rendering path (formatStackTrace, ZigException / V8StackTraceIterator, the uncaught printer, CallSite positions), overlaps with several open PRs on the same lines, and Build #95043 is showing failures, a human look is still worthwhile.
What was reviewed:
getLineColumnForStackFramecall sites all reach it with a non-null code block (guarded byhasLineAndColumnInfo()/ the earliercodeBlock()check inpopulateStackFramePosition);applyNegativeSourceStarthandles a null provider.SourceURLFormatterdropping the|| column.is_valid()term: the print block below has no line-invalid branch, so this only removes a stray trailing:for below-zero positions — matches theat f (nn.js)test.toUnsignedOneBasedclamps negative one-based ordinals to 0 before the unsigned write-back, soerror.linereads as unset rather than wrapping.- V8StackTraceIterator now parses signed segments so re-parsed
nn.js:-6:9frames round-trip as invalid (below-zero) rather than as part of the URL.
Extended reasoning...
Overview
This PR makes negative lineOffset/columnOffset in node:vm behave like Node/V8 by re-applying the negative part of the provider start position that JSC::SourceCode clamps away. It introduces Bun::applyNegativeSourceStart and a shared getLineColumnForStackFrame in ErrorStackFrame.cpp, threads them through formatStackTrace (error.stack), ZigException.cpp (Bun.inspect / uncaught printer / V8 stack re-parsing), and ErrorStackTrace CallSites, fixes handleException's arrow-header line/caret arithmetic in NodeVM.cpp, removes the compileFunction lineOffset > 0 guard so body line N reports as N + lineOffset for every sign, and adjusts SourceURLFormatter so frames whose line ends up ≤ 0 render without a :pos suffix. ~200 lines of tests cover the header, frames in nested functions, prepareStackTrace CallSites (including the walk-back-onto-first-line case my earlier comment flagged), Bun.inspect, compileFunction across offset signs, and the uncaught printer in a subprocess.
Since the previous review
My earlier inline finding (applying the negative offset before adjustPositionBackwards let the physical-column recompute discard it) was fixed in 5a9d4a2 by moving the call after the switch, with a test where the construct's divot is on line 2 and gets walked back onto line 1. Comment-cop feedback was addressed in 1daa1e2/e2c6cb4. fbcfcb3 tightened SourceURLFormatter so a below-zero line no longer prints a dangling :.
Security risks
None identified. The change is arithmetic on already-computed source positions; inputs are int32-validated lineOffset/columnOffset. INT_MIN is explicitly guarded in constructAnonymousFunction before the decrement. Negative results are handled by treating them as "no position" (Rust side) or clamping to 0 for the unsigned error.line/error.column fields.
Level of scrutiny
Medium-high. Stack-frame position computation is shared by every error path in the runtime, so a mistake here would affect all error output, not just node:vm. The change is designed to be a no-op for non-vm sources (start position 0:0), and the first-physical-line check for columnOffset matches JSC's own semantics, but the number of call sites and the noted overlaps with #37396 / #38240 / #38228 / #38236 / #38239 / #38245 mean a maintainer should confirm the merge order and that Build #95043's failures are unrelated.
Other factors
Test coverage is thorough and asserts columns relative to an un-offset baseline rather than hard-coding JSC's divot columns, so it should be robust. The Bun-specific expectations in test-vm-basic.js were updated to Node's line numbers now that the compileFunction off-by-one is fixed. The V8StackTraceIterator change to signed parsing is a small semantic widening (a URL segment like -1 after a colon would now parse as a line number), which is the intended behaviour for the nn.js:-6:9 case and unlikely to collide with real URLs.
There was a problem hiding this comment.
I re-reviewed after the follow-up commits (5a9d4a2, 1daa1e2, e2c6cb4, fbcfcb3) and found no further bugs — the ordering issue I raised earlier is fixed and now covered by the line-crossing new\nError test. Given the reach of this change (every stack-trace rendering path), the documented debugger-breakpoint trade-off, and the coordination needed with #37396 / #38240 and neighbours, a maintainer look is still worthwhile.
What was reviewed:
applyNegativeSourceStartfirst-line check againststd::max(startLine, 0)for the mixed positive-lineOffset / negative-columnOffset case — matches the{ lineOffset: 5, columnOffset: -2 }test.getLineColumnForStackFramecall sites for nullcodeBlock()— both are guarded (!codereturn inpopulateStackFramePosition;hasLineAndColumnInfo()informatStackTrace).V8StackTraceIteratorsigned-int parse and theSourceURLFormatter:gate for below-zero positions — covered by thenn.jssubprocess assertions.toUnsignedOneBasedclamping soerror.linereads 0 rather than wrapping.
Extended reasoning...
Overview
The PR re-applies the negative part of lineOffset/columnOffset that JSC's SourceCode clamps away, so node:vm stack frames, the arrow header, Error.prepareStackTrace CallSites, Bun.inspect, and the uncaught-error printer all report Node-matching positions. It touches ErrorStackFrame.{cpp,h} (new applyNegativeSourceStart + getLineColumnForStackFrame), FormatStackTraceForJS.cpp (route error.stack through the helper; clamp negative one-based positions to 0 for JSC's unsigned error.line/error.column), NodeVM.cpp (handleException header arithmetic; constructAnonymousFunction unconditional -1 shift), ZigException.cpp (signed line/column parsing in V8StackTraceIterator; fallback path routed through the helper), and ZigStackFrame.rs (drop the trailing : when only the column is valid). ~200 lines of new tests in vm.test.ts and updated Bun-gated expectations in test-vm-basic.js.
Security risks
None identified. The change is arithmetic on already-computed stack positions and string formatting; no new user-controlled parsing, allocation, or trust boundaries. The INT_MIN guard in constructAnonymousFunction prevents the one signed-overflow case.
Level of scrutiny
High. applyNegativeSourceStart runs on every frame with a code block via formatStackTrace and populateStackFramePosition — it is a no-op for every provider whose start is ≥ 0 (all non-vm sources), but a mistake here would misreport positions for ordinary errors. The PR description also documents a real trade-off (JSC's debugger compares provider start unsigned, so breakpoints cannot be set in a source with a negative start — now also affects compileFunction at the default offset when a debugger attaches late) and an alternative design (record the bias on NodeVMScriptFetcher keyed by source id). That is a maintainer-level design call.
Other factors
My earlier finding (applying the negative start before the op_construct walk-back) was fixed in 5a9d4a2 with a regression test; the comment-cop feedback was addressed in 1daa1e2/e2c6cb4; fbcfcb3 added below-zero handling with tests. Test coverage is thorough (23 cases across six entry points, header, nested functions, both offset signs, prepareStackTrace, Bun.inspect, compileFunction, and a subprocess for the uncaught printer). The description calls out overlap with five open PRs (#37396, #38228, #38236, #38239, #38240, #38245) touching the same functions, with specific merge-order guidance — a human should coordinate that.
1 participant
Problem
lineOffsetis ignored at runtime byvm.Script,vm.runInThisContextandvm.runInContext.new vm.Script("1;\n2;\n3;\nthrow new Error('x')", { filename: "n.js", lineOffset: -2 }).runInThisContext()produces a stack startingn.js:4/Error: x/at n.js:4:16; Node printsn.js:2, the source line, a caret, thenat n.js:2:7. Positive offsets work. A negative offset is the usual way to compensate for wrapper lines a caller prepended to the source.lineOffset: -1it shows the line after the throw, caret included.columnOffset < 0is dropped from the first line's frame columns the same way, andvm.compileFunctionreports body line N as N + 1 for everylineOffset <= 0.JSC::SourceCode's constructor clamps the line and column a source starts at to 1 (vendor/WebKit/Source/JavaScriptCore/parser/SourceCode.h), and every position JSC reports isclamped start + physical position, so the negative part never reaches the frames.handleException(src/jsc/bindings/NodeVM.cpp) then subtracted the provider's unclamped start (-2) from JSC's line to locate the source line, landing two lines below the throw.Fix
Bun::applyNegativeSourceStart(src/jsc/bindings/ErrorStackFrame.cpp): theSourceProviderstill carries the requested start, so after JSC's line/column is read,min(startLine, 0)is added to the line and, on the first physical line,min(startColumn, 0)to the column. It runs as the last step ofgetAdjustedPositionForBytecode(call sites forError.prepareStackTrace,Bun.inspect, the uncaught error printer), after the walk that movesnew X()frames back to thenewkeyword, since that walk works in the coordinates JSC reported and can end on the first line; and informatStackTrace(error.stack) andpopulateStackFramePosition's fallback through the newgetLineColumnForStackFrame.physical + max(start, 0):CodeBlock::lineColumnForBytecodeIndexadds the executable's first line, andUnlinkedFunctionExecutable::linkedSourceCodederives nested functions' first line and column from the clamped program values while sharing the provider. Addingmin(start, 0)therefore givesphysical + start, which is what V8 reports and Node prints. Every provider outside node:vm starts at 0:0 (JSC's ownmakeSourcecallers, eval,new Function, Bun's providers), so this is a no-op for them; positive offsets are unchanged.handleExceptionremoves only what JSC added (max(start, 0)) to find the physical line and printsphysical + startas the header line, so the header matches Node (n.js:2,nn.js:-6,z.js:0) and the frames under it. The caret uses the clamped start column the same way instead of relying on an unsigned wraparound.constructAnonymousFunctionshifts the wrapped program's start line by one unconditionally; its comment gave the clamp as the reason not to, which no longer applies. Body line N now reports as N + lineOffset for every offset, which also removes the off-by-one at the defaultlineOffset: 0(INT_MINis left alone rather than overflowed). compileFunction'scolumnOffseton body line 1 was never applied and still is not (that line is physical line 2 of the wrapped program).error.stackandCallSite#toJSONcarry them as V8 would (at f (nn.js:-6:18)); Bun's own renderers treat a non-positive line as "no position", and three of them needed fixing for that to come out clean: the error printer's stack-string parser (ZigException.cpp,V8StackTraceIterator) read line numbers as unsigned and turnednn.js:-6:18into the urlnn.js:-6with line 18; the printer (src/jsc/ZigStackFrame.rs) wrote the:separator when only the column was valid (at f (nn.js:)); anderror.line/error.column, whichErrorInstancestores unsigned, wrapped to 4294967290 and now read 0. They render asat f (nn.js)now.InspectorDebuggerAgent::resolveBreakpointcompares it as unsigned, so breakpoints cannot be set in a source whose start is negative. That was already the case forvm.Scriptwith a negativelineOffset; with this PR it also covers compileFunction code at the default offset, which only matters when a debugger attaches after the functions were compiled (compileFunction programs are not reported to an already attached debugger at all). Recording the bias onNodeVMScriptFetcherinstead (keyed by source id so eval code inside the body stays unbiased, as node:vm: report compileFunction body lines relative to lineOffset when it is 0 #38240 does for its wrapper columns) would avoid that at the cost of threading it through the three creation sites; I kept the provider since it is the data JSC already keeps for exactly this purpose and eval/new Functionget fresh providers for free.test/js/node/vm/vm.test.ts: thecan specify lineOffset/columnOffsettodos are implemented for all six run entry points, and a newdescribecovers the header (line, source line, caret) throughScript#runInThisContext,Script#runInNewContextandvm.runInThisContext, the wrong-line case, frames in nested functions,columnOffsetin both directions, lines at or below zero (header,error.line, and the uncaught printer with and without the decorated stack),Error.prepareStackTracecall sites (including anewwhose callee is on the next line and gets walked back onto the first),Bun.inspect,compileFunctionwith offsets -1/0/3, and the uncaught printer in a subprocess. 23 tests fail on the released binary and on a debug build without thesrc/change; all pass with it. Columns are asserted relative to an un-offset run, so the tests do not depend on which column JSC attributes to a throw.test/js/node/vm/(script-leak.test.tstimes out on this debug+ASAN container with and without the change), all 95test/js/node/test/parallel/test-vm-*.js(the Bun-specific expectations intest-vm-basic.jsthat documented the compileFunction off-by-one now expect Node's lines, following the file's existingtypeof Bungating),test/js/bun/test/stack.test.ts,test/js/node/v8/capture-stack-trace.test.js,test/regression/issue/23022-stack-trace-iterator.test.ts,test/js/bun/sourcemap/,test/js/node/module/sourcemap.test.js,inspect.test.jsandinspect-error.test.js(its two minified-file snapshots fail on debug builds before this change as well; tracked separately). Output for non-vm errors is byte-identical to the released binary in the scripts I compared.columnOffseton body line 1. If its layout is kept, itsconstructAnonymousFunctionblock has to replace this PR's shift (keeping both reports one line too high), and its column step has to run in JSC's coordinates, i.e. beforeapplyNegativeSourceStart. node:vm: keep lineOffset/columnOffset from overflowing JSC parser positions #38228, node:vm: validate lineOffset/columnOffset by value like Node's validateInt32 #38236, node:vm: report compileFunction body errors as SyntaxErrors and reject bodies that close the function #38239 and node:vm: accept a hashbang line at the start of a compileFunction body #38245 touch neighbouring lines ofconstructAnonymousFunction. error.stack: report frames at new X(...) at the new keyword #37396 restructures the same spots offormatStackTrace/ErrorStackFrame.cpp/populateStackFramePosition; on top of it,applyNegativeSourceStartstays the last step of both of its branches, and its rewritten walk should add the clamped (max(start, 0)) start column when it lands on the first line so that this step still converts JSC coordinates; the line-crossing test case here catches a double application.Background
lineOffset/columnOffsettell node:vm where a snippet sits inside a larger file. They are zero-based and signed; Node addslineOffsetto every line andcolumnOffsetto the first line's columns only, and V8 reports the sums as-is, including 0 or negative results.SourceProviderholds the source text and theTextPositionit was created with. ASourceCodeis a range of a provider plus the one-based line and column JSC counts from, clamped to 1. Frames report positions relative to their code block'sSourceCode; functions defined in a program getSourceCodes derived from the program's, all pointing at the same provider.DecorateErrorStack: when an error escapes a vm run,handleExceptionprepends<url>:<line>, the offending source line and a caret toerror.stack. Compile-time SyntaxErrors get the same fromdecorateParseErrorStack, which already handled negative offsets.getAdjustedPositionForBytecodeis where Bun already post-processes JSC positions (it movesnew X()frames to thenewkeyword);formatStackTracebuilds theerror.stackstring and used to read JSC's raw line/column directly. Positions cross to the Rust printer asZigStackFramePosition(zero-basedints, where a negative value means "none"); once an error'sstackstring has been materialized, the printer gets its frames by re-parsing that string withV8StackTraceIterator.[review] gate passed · iteration 1 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file