Conversation
These nodes speculate that the index is in bounds, and the abstract interpreter uses that speculation for their result type (Int32 for StringCharCodeAt and StringCodePointAt, String for an in-bounds StringAt). That lets it fold a compare such as `codePointAt(i) === undefined` to false. The node then has no users. In the FTL pipeline MovHint removal drops the hint that kept it alive, and DCE removes the node together with the speculation inside it. The optimized code treats every index as in bounds, and never exits, so it never learns that the intrinsic is wrong for this site. Mark the three nodes NodeMustGenerate, the same as ArithAdd and GetByVal, which also carry a speculation inside the node. Fixup clears the flag for StringAt with an OutOfBounds array mode, which returns undefined instead of exiting. Reported in oven-sh/bun#41609: @stylexjs/babel-plugin's media query parser started to fail after about 20 compilations in one process, because @csstools/css-tokenizer's endOfFile() is `void 0 === source.codePointAt(cursor)`.
![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 DFG node semantics (DCE eligibility) in a CODEOWNER-covered path, a JSC reviewer should still sign off.
What was reviewed:
- Verified in
DFGAbstractInterpreterInlines.hthatStringCharCodeAt/StringCodePointAtreportSpecInt32Onlyand in-boundsStringAtreportsSpecStringResolved, so the result types do depend on the in-bounds exit surviving —NodeMustGenerateis warranted. - Verified the fixup clearing
NodeMustGenerateforOutOfBoundsStringAtis sound: AI already reportsSpecStringResolved | SpecOtherthere and clobberize treats it as aPureValue, so it restores pre-PR DCE behavior for that case only. - Confirmed
StringCharAt(same fixup case, untouched) doesn't need the flag: its AI typeSpecStringResolvedis sound with or without the exit sincecharAtreturns""OOB. - Stress test follows JSTests conventions (
testLoopCount, throws on failure, no logging).
Extended reasoning...
Overview
The PR marks three DFG node types (StringAt, StringCharCodeAt, StringCodePointAt) as NodeMustGenerate in DFGNodeType.h so that DCE cannot remove them when their result is unused. These nodes carry an in-bounds speculation inside their own codegen, and the abstract interpreter's result types (SpecInt32Only / SpecStringResolved) are only sound if that speculation actually runs and can OSR-exit. DFGFixupPhase.cpp is updated to clear the flag for StringAt in OutOfBounds array mode, where the node returns undefined rather than exiting and its AI type already includes SpecOther. A new JSTests/stress regression test exercises all three intrinsics with dead results, then feeds out-of-bounds indexes and asserts correct undefined/NaN behavior.
Security risks
None identified. This is a correctness fix that makes the optimizer strictly more conservative for the affected nodes (preventing an unsound elimination). The one place it becomes less conservative — clearing the flag for OOB StringAt — merely restores the exact pre-PR behavior for that specific mode, and I confirmed via DFGAbstractInterpreterInlines.h and DFGClobberize.h that the OOB form is pure with a sound result type independent of any exit. There is no attacker-controlled surface, injection, or auth concern here.
Level of scrutiny
Moderate-to-high. The diff is tiny (3 flag bits, one conditional clear, one test), follows the established JSC pattern of NodeMustGenerate on speculating nodes (e.g. ArithAdd, GetByVal), and the reasoning in the PR description matches what the code actually does. However, DFG node flags govern optimizer behavior across all tiered-up JavaScript, and both Source/JavaScriptCore/ and JSTests/ are covered by .github/CODEOWNERS (@ WebKit/jsc-reviewers). Per the approval guidelines, CODEOWNER-covered critical-path changes should get a human sign-off even when the automated review finds nothing wrong.
Other factors
The stress test is well-constructed: it uses testLoopCount to reach the upper tiers, throws (rather than prints) on failure, and covers both 8-bit and 16-bit strings. I also checked that StringCharAt, which shares the same fixup case but was not touched, does not need the same treatment — its AI result type is SpecStringResolved regardless of bounds because charAt returns "" on OOB, so removing its exit cannot make downstream type assumptions unsound. No prior review comments or objections exist on the PR.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesThe DFG now preserves required string indexing operations during dead-code elimination while handling out-of-bounds String index bounds preservation
Merge Risk: ⚪ Minimal · up to The change preserves required bounds-check behavior for optimized string indexing while retaining StringAt’s undefined-returning out-of-bounds path. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed problem statement, fix, background, affected components, and verification results. It does not follow the repository template because it lacks a Bugzilla URL, a Reviewed by line, and an explicit changed-file/function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
|
Verified through oven-sh/bun#41713, which pins this PR's head (caa5d80). Its regression test fails on bun 1.4.3 and passes on a debug build with this commit. The stylex repro from oven-sh/bun#41609 passes as well. |
|
Closing: upstream fixed the same defect in another way, and the fork has that fix.
|
Merge upstream Bun through `ba3f27d1d1ce359d4eed842c135f0f6fba1acb00` into fork main, retaining upstream and contributor history. Land with a merge commit to retain upstream ancestry. All fork fixes #15, #7, #14, and #27 are incorporated and qualified. The five conflict resolutions preserve the fork’s behavior while integrating upstream changes: - Keep macOS `ProcessRetry` alongside upstream’s `Tty` event-loop flag. - Keep the richer worker `execArgv` record and parser for preloads, TLS trust, CPU profiling, and addon/FFI restrictions; incorporate upstream’s invalid process-only flag reporting and its C++ error channel. - Keep literal `?` file-URL handling in the module loader alongside upstream’s string-code-generation guards. - Retain one upstream-positioned `ERR_WORKER_INVALID_EXEC_ARGV` mapping; both histories added it, and the automatic merge initially duplicated it. The fork fixes landed since the previous sync `ee83b78b18` remain present: file-URL preloads (#22), response-finish diagnostics (#23), and macOS silent-run signal/stdio handling (#24). Earlier fork changes are retained by the merge. Upstream’s addon/FFI worker restrictions overlap the fork’s broader implementation; the shared behavior is retained in one parser, with upstream’s new invalid-flag handling added. WebKit remains `f20ce7744553c910bcf16a33faf976af208de091` on both sides. Direct source inspection confirms that `DFGSSALoweringPhase.cpp` lowers `StringAt`, `StringCharCodeAt`, and `StringCodePointAt` to independent `CheckInBounds` nodes, and the pin includes `JSTests/stress/string-index-dce-bounds-check.js`. This is the upstream replacement for closed oven-sh/WebKit#578, so no pin rollback is needed. The release documentation now reflects that fix. Final integration also retained both sides of the changelog conflict and added contributor credit for @SebTardif and @RomneyDa. The runtime plugin cache format remains version 34; the frozen upstream uses 33. Native qualification exposed a pre-existing fork mismatch with two new upstream idle-sweep tests: cached idle state stays false until message timing clears, even after a bodyless response ends inside its handler. Explicit Node HTTP idle sweeps now derive response availability while preserving incomplete TLS handshakes, application-owned parser-error sockets, request bodies, partial heads, queued responses, and tunnels. Both upstream one-read/split-head tests pass unchanged, as does the existing parser-error ownership test. A TLS 1.2 relay control proves a sweep cannot close a handshake in progress. Node 26.10.0 confirms the bodyless close behavior. Qualified head: `41dffc47212456d5ac80fc6fdddda08c4e4bfccc`, macOS arm64 release build (Bun 1.4.3, WebKit `f20ce7744553c910bcf16a33faf976af208de091`). No local SDK signpost patch was needed. Binary, hardening, duplicate-symbol, formatting, and applicable hosted checks passed. Codex review has no remaining accepted/actionable P0–P2 findings; the rejected non-regular-copy concern is ruled out by unchanged regular-file guards and eight matching baseline/candidate FIFO/device controls. | File | Pass | Skip | Todo | Fail | | --- | ---: | ---: | ---: | ---: | | `test/js/bun/terminal/terminal.test.ts` | 97 | 2 | 0 | 0 | | `test/js/bun/terminal/terminal-spawn.test.ts` | 23 | 2 | 0 | 0 | | `test/js/bun/spawn/spawn.test.ts` | 165 | 24 | 0 | 0 | | `test/js/node/child_process/child-process-stdio.test.js` | 9 | 0 | 0 | 0 | | `test/js/node/http/node-http.test.ts` | 277 | 1 | 0 | 0 | | `test/js/node/diagnostics_channel/diagnostics_channel.test.ts` | 26 | 0 | 3 | 0 | | `test/js/node/sqlite/node-sqlite.test.ts` | 143 | 4 | 0 | 0 | | `test/js/node/worker_threads/worker_threads.test.ts` | 172 | 0 | 0 | 0 | | `test/cli/run/preload-test.test.js` | 2 | 0 | 3 | 0 | | `test/cli/run/no-orphans.test.ts` | 15 | 9 | 0 | 0 | | `test/cli/install/bun-install-lifecycle-scripts.test.ts` | 70 | 0 | 0 | 0 | | `test/js/node/fs/fs.test.ts` | 660 | 16 | 0 | 0 | | `test/js/node/fs/cp.test.ts` | 54 | 6 | 0 | 0 | | `test/cli/run/transpiler-cache.test.ts` | 27 | 0 | 0 | 0 | | `test/cli/test/isolation.test.ts` | 41 | 0 | 0 | 0 | | `test/js/bun/plugin/plugins.test.ts` | 47 | 0 | 0 | 0 | | `test/js/bun/plugin/plugin-namespace-drive-letter.test.ts` | 1 | 0 | 0 | 0 | | `test/js/node/disallow-code-generation-from-strings.test.ts` | 28 | 0 | 0 | 0 | | `test/js/node/http/node-http-server-abort-events.test.ts` | 105 | 0 | 0 | 0 | | `test/js/node/http/node-http-server-close-drain.test.ts` | 31 | 0 | 0 | 0 | | `test/js/node/http/node-http-connect.test.ts` | 80 | 1 | 2 | 0 | | `test/js/node/http/node-http-upgrade-body.test.ts` | 11 | 2 | 0 | 0 | | `test/js/node/http/node-http-req-socket-pause.test.ts` | 40 | 0 | 0 | 0 | | `test/js/node/tls/node-tls-server.test.ts` | 99 | 0 | 0 | 0 | | `test/js/node/tls/node-tls-wrapped-socket-close.test.ts` | 15 | 0 | 0 | 0 | | `test/cli/install/bun-install-patch.test.ts` | 32 | 0 | 0 | 0 | | `test/js/bun/io/bun-write.test.js (stream fallback only)` | 1 | 0 | 0 | 0 | | `test/bundler/compile-argv.test.ts (compiled code-generation flags)` | 3 | 0 | 0 | 0 | | **Total** | **2274** | **67** | **8** | **0** | Filesystem and isolation suites used outer concurrency 2; nested stress work, assertions, and timeouts were unchanged. Lifecycle, TLS, and compiled-argument tests enabled the existing internal-test API at process startup (`BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING=1`, `BUN_GARBAGE_COLLECTOR_LEVEL=0`). Initial lifecycle/TLS invocations without those startup flags could not load `bun:internal-for-testing`. Earlier runs encountered recursive-readdir and copy-stress timeouts; the same readdir workload exceeded its deadline on the pre-sync baseline. Both full filesystem suites passed on the final head. The terminal ESRCH test missed its real kernel race window once (its path-reached assertion failed); the unchanged complete file passed on retry. The known broader Bun.write Response timeout was reproduced on a separately built pre-change baseline during #27 qualification and is not claimed as a passing full-suite run here; the added stream-fallback regression passes. Additional proof: normal/strict data/blob imports and Workers behaved as expected in all 16 probes; the compiled-argument tests preserve quoted strict-mode floors. OpenClaw smoke with the built runtime passed: `OpenClaw 2026.9.6 (23ad3a5)`. No tag or release is part of this PR.
1 participant
Problem
@stylexjs/babel-pluginfails withInvalid media query syntaxafter about 20 compilations in one process. The tokenizer's end test isvoid 0 === source.codePointAt(cursor). Once the FTL compiles the caller,codePointAt(length)no longer reports the end of the string.String.prototype.athas the same defect.BUN_JSC_useFTLJIT=falsehides it.StringCodePointAt,StringCharCodeAtand an in-boundsStringAtspeculate inside the node that the index is in bounds (DFGSpeculativeJIT.cpp:1898,FTLLowerDFGToB3.cpp:12240). The abstract interpreter relies on that speculation and types the resultSpecInt32Only(DFGAbstractInterpreterInlines.h:2745), so it folds the compare againstundefinedtofalse. The node has no users left. MovHint removal drops the hint that kept it alive in the FTL pipeline, and DCE (DFGDCEPhase.cpp:140) removes the node and the exit with it. The code never exits, so the bytecode parser never learns to stop using the intrinsic.Fix
StringAt,StringCharCodeAtandStringCodePointAtasNodeMustGenerateinDFGNodeType.h. This is the convention for a node that carries its own speculation:ArithAddandGetByValhave the flag too. A node that is proven in bounds still folds to a constant, which clears the flag.StringAtwith anOutOfBoundsarray mode. That node returnsundefinedinstead of exiting, so DCE can still drop it.StringCharCodeAtalready gets a separateCheckInBoundsin the FTL (298202@main), so it only depended on a MovHint in the DFG tier. The flag makes the three nodes uniform.JSTests/stress/string-index-dead-result-keeps-bounds-check.js(new). Bun's regression testtest/regression/issue/41609.test.tsfails on bun 1.4.3 and passes with this commit, see the bun PR.Background
NodeMustGeneratecan be removed by DCE when nothing uses its result.Node::removekeeps the edge checks as aChecknode, but a check inside the node's own code generation is lost.