Repository navigation
[JSC] addErrorInfo() drops what the host's stack formatter throws - #774
Merged
Merged
Conversation
With a host that formats stacks (VM::onComputeErrorInfoJSValue; Bun),
addErrorInfo() runs the formatter while the parser's error object is being
made. The formatter can run Error.prepareStackTrace, so it can throw, and its
ThrowScope asks its callers for an exception check either way. The Function
constructor makes no such check: constructFunctionSkippingEvalEnabledCheck()
gets the error back and throws it, so `new Function("(")` aborts under
--validateExceptionChecks=1, and a throw from Error.prepareStackTrace was
pending while the SyntaxError was thrown over it.
addErrorInfo() now catches around the formatter and clears what it threw,
short of a termination. The caller made the error to throw it: the
SyntaxError is what the program sees, as it already did, through the Function
constructor, eval and vm.Script alike.
![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 found no bugs; since it changes exception-scope handling in JSC's error-construction path (a CODEOWNERS-covered area with no in-tree test for the host-formatter path), a human look would still be worthwhile.
What was reviewed:
- The restructured branch:
hostComputes == falseand non-Bun builds still reach the samematerializeErrorInfoIfNeeded(vm)call and putDirect fallthrough as before — no behavior change there. - Choice of
DECLARE_TOP_EXCEPTION_SCOPE: this tree has noCatchScope;TopExceptionScopeis the fork's catch idiom (same ascreateTypeErrorCopyin the same file) and nests correctly inside callers' scopes. - Whether the new scope's constructor could itself trip
validateExceptionCheckson an earlier unchecked throw — ruled out, since the host formatter's ownThrowScopeconstructor already performed that same verification before this change. - Whether dropping a throwing
Error.prepareStackTracechanges what programs observe — ruled out: previously the SyntaxError was thrown over the pending exception anyway, so the observable result (SyntaxError) is the same.
Extended reasoning...
The change is a single-file, roughly ten-line edit in Source/JavaScriptCore/runtime/Error.cpp inside the USE(BUN_JSC_ADDITIONS) branch of addErrorInfo(), wrapping the host stack formatter call in a TopExceptionScope and clearing non-termination exceptions before returning. It touches no injection, auth, or data-exposure surface. The code is correct as far as I can verify against TopExceptionScope, ThrowScope, and ErrorInstance::materializeErrorInfoIfNeeded, and the hostComputes==false path is unchanged. Deferring rather than approving because /Source/JavaScriptCore is covered by .github/CODEOWNERS, the change alters exception-propagation semantics in a core engine path, and the only test coverage lives in the Bun repository rather than in this tree.
|
Preview build of 6bd59af: |
This was referenced Oct 7, 2026
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Oct 13, 2026
… a value (#45190) ### What does this PR do? Fixes a regression in Bun 1.4.3. When `,` `]` or `}` came right after a complete value: - `Bun.JSONL.parseChunk` reported a `read` offset in front of a value it had already returned. The streaming loop in the docs (`buffer = buffer.slice(result.read)`) then got the same value again on the next call. - `Bun.JSONL.parseChunk` and `Bun.JSONL.parse` returned the value once more for each blank line in front of it, in quadratic time. Strings and typed arrays were affected alike. The error for such input is intended and stays. Input with one value per line was never affected. | input | 1.4.2 and this PR | 1.4.3 | |---|---|---| | `'{"id":1},\n{"id":2}\n'` | `[{"id":1}]`, read 8 | `[{"id":1}]`, read 1 | | `"1,2\n"` | `[1]`, read 1 | `[1]`, read 0 | | `"[1]]\n[2]\n"` | `[[1]]`, read 3 | `[[1]]`, read 1 | | `"\n\n7,\n"` | `[7]`, read 3 | `[7,7,7]`, read 2 | | `"[\n7]}\n"` | `[[7]]`, read 4 | `[[7],7]`, read 2 | | 100,000 blank lines, then `7,` | `[7]` | 100,001 copies of `7`, 2.7 s | **Cause.** The streaming parser behind `Bun.JSONL` takes the end of each value from a position that only `Lexer::lex()` records. The WebKit upgrade in #44615 brought in `Lexer::nextAfterValue()`, which consumes `,` `}` `]` after a value without `lex()`. The position then still pointed in front of the value, or at the last key inside it. When only newlines lay between that position and the value, the parser took them for the end of the line, moved on one line, and parsed the same value again. **Fix.** oven-sh/WebKit#799 (merged): for these three tokens `tryStreamingParse()` walks back from the token over the whitespace in front of it. Only Bun's own function changes. The lexer and `JSON.parse` are not touched. This PR moves `WEBKIT_VERSION` from `0c06faad` to `3d325715`, the merge commit of #799, and adds the tests. **The bump also brings in** the two changes that merged to the fork's `main` after the current pin: - oven-sh/WebKit#774: `addErrorInfo()` drops what the host's stack formatter throws, so `new Function("(")` passes `validateExceptionChecks`. - oven-sh/WebKit#787: the heap checks for a collection once per block of reported extra memory, not on every report. ### How did you verify your code works? **Tests.** 46 new tests in `test/js/bun/jsonl/jsonl-parse.test.ts`. They run 17 values (numbers, strings, literals, objects, arrays, non-ASCII, multi-line) after 6 kinds of preceding lines, as a string, a UTF-16 string, a `Uint8Array` and a non-ASCII `Uint8Array`, and compare the whole `parseChunk` result and the `parse` result. There are also cases for 10,000 blank lines, an object whose shape the JSON key cache knows, nesting deeper than the native stack, a `start` offset, and slicing at `read`. | build | whole file | |---|---| | 1.4.2 | 315 pass | | 1.4.3 | 289 pass, 26 fail | | debug build of `main` (c69a636) | same failures as 1.4.3 (run on an earlier version of the tests) | | debug build of this PR | 315 pass | 20 of the 46 pass on 1.4.3 too: whitespace before the token, the token on a later line, and `x` in its place. They cover the walk over whitespace that the fix adds, and the path that still uses the recorded position. **Comparison with 1.4.2.** Every sequence of up to 5 tokens out of 18 (`[ ] { } , : "a" 1 \n space true x {"a":1} [1] " - 1.5 {"a":"b","c":2}`), 2,000,718 inputs. For each one the `parseChunk` and `parse` results, and up to 4 tokens also as UTF-16, as bytes, as non-ASCII bytes and with a `start` offset. | build | inputs that differ from 1.4.2 | |---|---| | 1.4.3 | 161,568 | | this PR | 0 | **`JSON.parse` cost.** `bin/jsc` of `bun-webkit-linux-amd64-lto`, an array of 20,000 integers and an array of 4,000 small objects parsed 400 times, `perf stat -e instructions:u`, three runs each. The first version of the WebKit fix recorded the position in `nextAfterValue()`. It gave the same 0 differences, but every value `JSON.parse` sees goes through there, so it was dropped. | WebKit build | instructions | |---|---| | fork `main` (5d2ceba2) | 4.405 to 4.411 G | | position recorded in `nextAfterValue()` (97c108d8) | 4.470 to 4.475 G | | oven-sh/WebKit#799 (dd3b20d5) | 4.404 to 4.408 G | All of the above ran on Linux x64. In CI the file also passes on macOS arm64 (315 pass, build 125228). **Memory tests and oven-sh/WebKit#787.** In build 125228 `structured-clone-fastpath.test.ts` ("constant amount of memory for simple object inputs", limit 1,024 KB) measured 1,776 KB once on macOS arm64, inside the parallel batch, and passed when the runner ran it alone. It is the only time that file shows up in the annotations of the last 100 builds, so I compared CI's own `bun-linux-x64` binaries: `main` at c69a636 (build 125192), which is the base of this branch, and this branch (build 125228). The WebKit bump is the only difference between them. Real test file, with the measured value printed, CI's environment variables, runs interleaved. RSS growth in the object test, the one that failed (limit 1,024 KB): | | main | this PR | |---|---|---| | alone, 150 runs: median | -36 KB | -36 KB | | alone, 150 runs: max | 512 KB | 512 KB | | after 20 other test files in the same process, 24 runs: max | 512 KB | 512 KB | | runs at or over the limit | 0 | 0 | RSS growth in the string test next to it (limit 8,192 KB): | | main | this PR | |---|---|---| | alone, 150 runs: median | 512 KB | 512 KB | | alone, 150 runs: max | 1,596 KB | 1,536 KB | | alone, 150 runs at 1,024 KB or more | 20 | 48 | So the bump does keep one more 512 KB string alive more often in the string test, which fits what #787 says about itself (a collection can start a little later). The maximum does not move and stays far under that test's limit. The object test does not change on Linux. I could not reproduce the macOS result and have no macOS machine, so it is not explained. One-off failures in the parallel batch are common: 65 different test and platform pairs in those 100 builds. **The failed job in build 125228** (1 of 181) was `darwin any aarch64 - test-bun`. `s3-list-objects.test.ts` failed all 4 attempts with `Secret not found: S3_R2_ACCESS_KEY`, because the agent had `no route to host` for agent.buildkite.com from 22:35:34 to 22:36:40 UTC. The test body never ran.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
With a host that formats stacks (
VM::onComputeErrorInfoJSValue; Bun), the fork'saddErrorInfo()runs the formatter while the parser's error object is being made. The formatter can runError.prepareStackTrace, so it can throw, and itsThrowScopeasks its callers for an exception check either way.The Function constructor makes no such check.
constructFunctionSkippingEvalEnabledCheck()gets the error back fromFunctionExecutable::fromGlobalCode()and throws it. So:new Function("(")aborts a build run with--validateExceptionChecks=1(Bun's ASAN lane setsBUN_JSC_validateExceptionChecks=1):when
Error.prepareStackTracethrows, that exception is still pending while the SyntaxError is thrown over it.eval and
vm.Scriptdo check after the error is made, which is why only the Function constructor trips.Fix
addErrorInfo()catches around the formatter and clears what it threw, short of a termination. The caller made the error to throw it, so the SyntaxError is what the program sees.Behaviour does not change. With an
Error.prepareStackTracethat throws,new Function("("),(0, eval)("(")andnew vm.Script("(")each throw the SyntaxError before and after this change (Node 22 does the same).Verification
Bun built against this tree (debug + ASAN, Linux x64), with
BUN_JSC_validateExceptionChecks=1:new Function("(")throws a SyntaxError and no longer aborts, with the default formatter and with anError.prepareStackTracethat throws.new Functionassertions that Upgrade WebKit to dbdca7545d bun#44615 keeps as atest.todointest/js/bun/jsc/webkit-upgrade-dbdca7545d.test.tspass.capture-stack-trace.test.js,inspect-error.test.jsandvm.test.tspass.There is no JSTests case: the
jscshell has no stack formatting hook, so this path does not exist there. The test is the one in Bun, which stops being a todo when Bun's WebKit pin includes this.