Repository navigation
Conversation
…eneration mode Two tests in test/cli/inspect/debugger-shared-code-generation-mode.test.ts, for the JSC fix in oven-sh/WebKit#830. A vm.Script compiles once and every context links that compile, so the host global object and a vm context share the unlinked executable of each function in the script. On main the code of whichever global object runs a function first is given to the other one: - an async generator suspended in a context crashes with SIGSEGV when it resumes after the host ran the body with a debugger attached, - a breakpoint in a function that a context ran first never pauses the host.
…cached function The test was a todo. A function decoded from the bytecode cache of a `bun build --compile --bytecode` executable has no debugger opcodes, and JSC handed that code to the debugged global object, so a `debugger;` statement in it did nothing. With oven-sh/WebKit#830 the debugged global object gets code generated with the opcodes and the decoded code stays for everyone else.
JSC gave the bytecode an unlinked function holds to every global object that links it, whatever the caller asked for. A debugger is attached to one global object, so a `vm` context or a ShadowRealm could be handed bytecode with debug hooks that the host generated, and the host bytecode without them. A generator suspended in one kind crashed when it resumed in the other, and a breakpoint in the host was never reached. The held bytecode now goes only to callers that ask for the same thing of the debugger. The other kind of caller gets bytecode of its own, and the held bytecode is never replaced. The pin is a preview build. It must be replaced by the commit on the fork's main before this merges.
The revision compares only the debugger part of the code generation mode and never replaces the bytecode an unlinked function holds: a caller that asks for the other kind gets bytecode of its own. Bytecode decoded from a `bun build --bytecode` cache therefore stays where a suspended generator can find it again.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Status: draft. The tests pass with the preview build of oven-sh/WebKit#837, which also has oven-sh/WebKit#836 and oven-sh/WebKit#830. This PR leaves draft when those merge and How I reproduced it on main (debug build with ASAN,
Two more cases fail on main. A generator that starts under a profiler resumes after the profiler is turned off, or after the inspector client disconnects: crash. A plain function that a context calls first: the host's breakpoint never pauses.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new harness's ws-URL regex for a split-chunk race — the inspector writes the Listening: href line in one small Bun.write to stderr (src/js/internal/debugger.ts:236), well under PIPE_BUF, so the URL cannot arrive truncated — and the unread stdout pipe: none of the three fixtures writes to stdout, so it cannot fill and stall the child.
Extended reasoning...
The diff pins WEBKIT_VERSION in scripts/build/deps/webkit.ts to an autobuild-preview-pr tag, flips one test.todo to a live test in test/cli/inspect/compile-bytecode-tooling.test.ts, and adds a 205-line test file with three test.concurrent inspector-protocol cases exercising vm.Script and ShadowRealm generator resumption and a breakpoint under --inspect. It touches no security-sensitive surface; the only non-test change is the dependency pin. The inline findings already cover the ephemeral pin and the harness's failure wiring, so this note only records two adjacent concerns examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "87ee10da087b63e011bd1ada64d1814a850a4395"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-830-4264b09e"; |
There was a problem hiding this comment.
🔴 Every build of main (CI and bun bd) will fail to download JSC once oven-sh/WebKit#830 merges or closes, because this pins an ephemeral preview tag. scripts/build/deps/webkit.ts:6 sets WEBKIT_VERSION to autobuild-preview-pr-830-4264b09e; GitHub deletes autobuild-preview-pr-* releases when the WebKit PR merges, which scripts/build/download.ts:320 documents as a 404 for every build at once. Fix: before merge, swap WEBKIT_VERSION to the merged oven-sh/WebKit main sha (40-hex or autobuild-<sha>). The description calls the PR blocked on the WebKit PR; that holds only if the bump happens before merge, which nothing in the diff enforces.
Why this was flagged
Trigger: the PR is merged (or any downstream branch builds) after oven-sh/WebKit#830 merges or closes, which deletes the GitHub release autobuild-preview-pr-830-4264b09e. scripts/build/deps/webkit.ts:6 feeds cfg.webkitVersion (scripts/build/config.ts:59) and the prebuilt download URL (scripts/build/deps/webkit.ts:81), so every configure+build of bun downloads from a URL that returns HTTP 404. scripts/build/download.ts:328-337 turns that into WebKit preview release is gone and the build aborts; bun sync-webkit-source (scripts/sync-webkit-source.ts:24) also fails to resolve the tag. On the base branch WEBKIT_VERSION is the permanent sha 87ee10da087b63e011bd1ada64d1814a850a4395, whose release never disappears. The process.versions and WEBKIT_BYTECODE_CACHE_HASH_KEY consumers also report a preview id rather than a commit. The author's note that the PR is blocked is a process claim, not a safeguard in code.
Verification: This branch is merged after oven-sh/WebKit#830 is merged or closed, which deletes the autobuild-preview-pr-830-4264b09e release. scripts/build/deps/webkit.ts:81-82 uses the version verbatim as the release tag, which every prebuilt-mode configure (the CI and bun bd default) fetches. No guard exists: nothing in the repo blocks merging a preview-pr pin.
There was a problem hiding this comment.
Correct. The preview tag is there so that CI runs the tests against the engine change, and it goes away when oven-sh/WebKit#830 merges or closes.
I converted this PR to a draft, so it cannot merge with the tag. It leaves draft when WEBKIT_VERSION is the merged commit of the WebKit PR. I keep this thread open until then.
… or a closed socket - A fourth case: the context resumes first, the host starts with the debugger on, breakpoints are turned off, and the host resumes. It crashes on main. - `send` rejects on an error reply and when the socket closes, with the child's stderr, so a debugger that did not activate fails the test and a dead child does not leave it to time out.
|
Updated 10:05 PM PT - Oct 21st, 2026
❌ @robobun, your commit 633e140 has some failures in 🧪 To try this PR locally: bunx bun-pr 45770That installs a local version of the PR into your bun-45770 --bun |
…ed-code-generation-mode # Conflicts: # scripts/build/deps/webkit.ts
…the profilers and a client that disconnects The preview has oven-sh/WebKit#836, oven-sh/WebKit#830 and oven-sh/WebKit#837 on top of the WebKit commit that main pins. New tests: a vm.Script that runs twice in one global object, with a debugger, the type profiler and the control flow profiler turned on while its first generator is suspended. A generator and an async function that start under a profiler and resume after the profiler is turned off, and after the inspector client is gone.
robobun
changed the title
…6270) ### What does this PR do? Hardening follow-up to #46021. It fixes regressions from #46021 and #44618 that were never released, includes more of the open pull requests that fix a crash, and fixes a few crashes that had no pull request. #### Regressions fixed (none was released) - `spyOn(Bun, "$")`, and the same for `"sql"`, `"postgres"` and `"SQL"`, threw "does not support accessor properties yet". - Async `fs.read`, `fs.write` and `zlib.brotliCompress` on a view over a `WebAssembly.Memory` could lose data or copy far more than they needed. - `node:vm` with `vm.constants.DONT_CONTEXTIFY`: `this` in host accessors, prototype handling, `util.types.isProxy`, `Bun.deepEquals`, `assert.deepEqual` and `console.log` of a context. - `zlib` `stream.flush()` threw for arguments it used to accept, and streams had a new enumerable property. - `Bun.Transpiler` `exports.replace` with the name `"default"`. - `process.env.X = 3000` could store the number once the function was JIT-compiled. - `node:sqlite` and `bun:sqlite` could lose rows at exit. - Dev server: stylesheets shared between pages after a build error. - Shell `cp` of a file onto itself exited 1. - `error.stack`: the frame of `require()` reads `at anonymous (unknown)` again. - `import * as process from "process"` followed by `process.on(...)` threw at load. The EventEmitter methods that the module exports now act on `process`. - `delete res.statusCode; res.end()` in a `node:http` handler threw. It sends 200, as in Node. (from #46128) - `node:tls`, `node:https` and `node:http2`: connection teardown and write callbacks behave as in 1.4.3 and Node again. #### Crash fixes Bundler, parser and CSS: #42286, #46092, #46097, #46177, #46239, #36334, #43024, #40769. `bun test`, `--hot` and `--watch`: #30776, #38206, #38974. Shell: #43183, #37743, #46168, #46207, #34900. `node:http`, `node:http2`, `Bun.serve`, streams: #43449, #45380, #44457, #42197, #42485, #38082, #38124, #46074, #45999, #46045. `node:vm`, native addons, workers: #46129, #46198, #45615, #33412, #38031. Dev server: #42790, #42850, #39488. Other: #35991, #38030, #38586, #40526, #41830, #46029, #46078, #46114, and the bug of #42215. #### Behavior changes - `crypto.randomFill(view, cb)` reports `ERR_CRYPTO_OPERATION_FAILED` if it could not fill the view. It used to report success. - A function registered twice on `process` runs twice, as in Node. (#41830) - Under `--hot`, a reload after an uncaught exception cancels the timers that the failed run created. The new run's timers fire, and so do timers and `fs.watchFile` watchers that earlier runs kept on `globalThis`. The reload also resets the exit code to 0. (#38206) - The parent of a Worker no longer gets an `'error'` event for a failed `Bun.serve` request in that Worker. The error is printed, as on the main thread. - `hash.write()` after `digest()` drops the chunk, as in Node. (#35991) - `http2` over TLS with `server.emit("connection", socket)`: `session.socket` is a `TLSSocket` and a `'connection'` listener fires, as in Node. (#38124) - A `slice()` with no type of a body `Blob` has `type === ""`, as in Node. (#46114) - `util.types.isProxy(globalThis)` is false, as in Node. - A write on a `node:net` or `node:tls` socket that is still pending when the connection closes calls back with `ECANCELED`, as in Node. - `HTMLRewriter.on()` throws for a selector that is nested too deeply. - `Bun.Image`: `maxPixels` also bounds PNG metadata, `metadata()` of a GIF reports the size that gets decoded, and a path with a NUL byte throws. (#40526) - `bun build --no-bundle` with the `base64` or `dataurl` loader prints an error. (#36334) - Bundled output for a default-exported function or class expression changes to `var file_default = function name() {}`, as in esbuild. `.name` is unchanged. (#46097) - A Proxy in the prototype chain of a `node:vm` global is allowed. jsdom 30 with `runScripts` needs this. - `bun:sqlite` commits a write that `iterate()` left open when the main thread exits. An explicit `BEGIN` is still rolled back. - When a Worker stops while a native addon's async work is still running, the addon's finalizers run after that work is done, as in Node. - Shell: `mv -n` and `cp -n` don't overwrite, and `cp` reads `R`, `v` and `n` anywhere in a flag group. #### Performance - Ordinary `node:vm` contexts run at the same speed. With `DONT_CONTEXTIFY`, stores to globals and reads of inherited names in a hot loop cost 10 to 20 % more than on `main`. - A `process.env` write costs 20 to 35 % less than after #46021. - An async read into a `WebAssembly.Memory` view looks through the live memories: about 1.5 µs more with one memory. #### Not included - Taken in part, so these should stay open for the rest: #36334, #38974, #39488, #40526, #40769, #42485, #45615, #45999, #46029, #46092, #46114, #46168, #46207, #46239. - Needs a WebKit change: #34655 #39204 #40170 #40253 #41587 #41900 #42699 #42897 #42903 #43084 #43813 #44518 #44949 #45770 #45804 #45827. - #44723 (covered by #46097), #41675, #42753, #38762, #41475, #46016. ### How did you verify your code works? - Every item was reproduced first on release builds of `main`, of its parent and of 1.4.3, and compared with Node v26 where Node has the API. - Every fix has a test that fails on `main` and passes on a debug + ASAN build of this branch, except where that isn't practical (huge inputs, code that only runs on Windows or macOS). - The regressions were checked on grids of calls against the build before #46021 and against 1.4.3. - Bun's own test suites and Node's `test-vm-*`, `test-http2-*` and `test-zlib-*` files for the areas touched.
…bKit main WebKit main (e8a8efc09c) is one commit past the WebKit commit that main pins (7d0c8a7d09): oven-sh/WebKit#862.
…nt is gone The test asserted that the first Bun.gc(true) after its helper returns destroys the statement. A collection can keep an unreferenced cell for a round. Bun.gc(true) walks the heap before it collects, the walk leaves a free-list end marker on the dead stack (the address of the newest cell plus one), and the frames of the collection are built over it. On the Release + ASAN lane with this WebKit that word keeps the statement's iterator through the first collection. The second collection takes it. The test now collects until the write is finished, at most 10 times. It still checks that no collection asks the authorizer, and it still fails on a build from before the feature (the process aborts).
Blocked on oven-sh/WebKit#836, oven-sh/WebKit#830 and oven-sh/WebKit#837. Draft until
WEBKIT_VERSIONis a merged commit.Problem
Segmentation fault at address 0x20inllint_op_put_to_scope. Avm.Scriptthat runs twice, or in anode:vmcontext, is enough.Fix
test/cli/inspect/debugger-shared-code-generation-mode.test.ts(10 tests, all fail on main). One GC test innode-sqlite.test.tsnow collects until its statement is gone (Notes).Background
vm.Script(vm.Script: compile the source once and link that compile in every context it runs in #38040) compiles once. Every run and context links that one "unlinked" function.Downsides
CodeBlock(17 realms: 68 generations against 4).Bun.shrink(): 2,004 cells against 4 for 2,000 generators.libJavaScriptCore.atext grows by 5,491 B.Notes
Repro
The 10 tests, on a debug build of main with ASAN (
ea6a2c7b7c, WebKit274ebb3cf6) and on this PR:vm.Scriptin the host and a context, a debugger activatesvm.Scriptthat ran twice in one global object, a debugger activatesNaNwhere32and66are rightThe fix is the pin in
scripts/build/deps/webkit.ts, so the "main" column is a build of this branch with theWEBKIT_VERSIONof main.Where the fix is
WEBKIT_VERSIONpins the preview build of [JSC] Do not inline a function that has debug hooks WebKit#837, which has all three PRs.UnlinkedFunctionExecutable::unlinkedCodeBlockForgives its held bytecode to every caller, whatever mode the caller asks for. JSC pins the mode of a started generator body, but the bytecode is looked up again for eachCodeBlock.FunctionExecutable::codeOfStartedBody(). The body lets go of it when unlinked code is deleted and none of its activations is suspended. The lookup compares the debugger bit and the type profiler bit.node:inspectorturns that profiler on and keeps the code that exists (src/jsc/bindings/JSInspectorProfiler.cpp). That behaves as on main.VM::disableTypeProfiler()andVM::disableControlFlowProfiler()destroyed a profiler that pinned bytecode still points into.e8a8efc09c, one commit ([JSC] Fix Yarr JIT abort when nested alternatives claim more than INT_MAX characters WebKit#862) past the commit that main pins. Their JSC tests fail on that commit: 21 of 21 runs.Measurements
libJavaScriptCore.atext, CI lanebun-webkit-linux-amd64(size): 34,264,855 B ate8a8efc09c, 34,270,346 B in the preview. Data and bss are equal.Bun.shrink(),Bun.gc(true), debug builds.UnlinkedFunctionCodeBlockcells left: finished generators 4 on main and 4 here. Suspended generators 4 on main and 2,004 here. Main generates the bytecode of a suspended body again when it resumes.sizeof(FunctionExecutable)goes from 120 to 128 B in a 128 B cell, and a deletion of unlinked code reads each started body (22 ns).Tests
debugger-shared-code-generation-mode.test.ts: the 10 tests of the table. 4 of 4 runs pass on the debug build with ASAN.compile-bytecode-tooling.test.ts: "debugger attached via inspector.open() pauses inside a never-called function" was a todo. A function decoded from the embedded bytecode cache has no debugger opcodes, and the debugged global object now gets bytecode with them.test/cli/inspect/(all 9 files),test/js/node/vm/(all 7 files),test/js/node/inspector/(inspector,inspector-profiler),test/cli/test/coverage.test.ts,test/js/bun/jsc/(shadow,bun-jsc, twowebkit-upgrade-*files),test/regression/issue/29519.test.ts,test/regression/issue/26298.test.ts.node-sqlite.test.tson the Release + ASAN lane"GC lifetime > a write statement that iterate() stopped after its first row > is finished by the next call on the connection after it's collected" failed on
x64-asanwith this pin:Expected to contain: "ft_data",Received: [ null ]. It passes there with the pin of main, and on every other lane. It needs the test before it in the file.The test asserted that the first
Bun.gc(true)after its helper returns destroys the statement. With this pin the first collection keeps the statement's iterator, and the second one takes it. No runtime code is wrong:Bun.gc(true)runs a collection, and a collection can keep an unreferenced cell for a round (#40966 ruled the same way for two other tests). The test now collects until the write is finished, at most 10 times. It still checks that no collection asks the authorizer, and it still fails on a build from before #46021 (the process aborts).Why the first collection keeps the iterator, from gdb on
bun run build:asan(Release + ASAN, 3 of 3 runs fail):MachineThreads::gatherConservativeRoots), one word in the scanned range points into the iterator cell:0x7fffffff5228holdsiterator + 1. The word is in the frame ofJSC::Heap::waitForCollection, 8 bytes above its lowest address. That function never writes the slot.JSC__VM__runGC->Heap::deleteAllUnlinkedCodeBlocks->MarkedSpace::willStartIterating->BlockDirectory::stopAllocating->LocalAllocator::stopAllocating->MarkedBlock::Handle::stopAllocating->BlockDirectory::didFinishUsingBlock, whose prologue pushes a register. The value is a free-list end marker:FreeCell::makeLastmarks the last interval with an offset of 1, soFreeList::m_nextIntervalis the address of the first cell of that interval plus one. That cell is the newest object of its type.JSC__VM__runGCcallsdeleteAllUnlinkedCodeBlocksand thencollectNowfrom one frame, so the collection's frames lie over the dead frames of the heap walk.ConservativeRootsaccepts a pointer into the middle of a cell.0written to that one word at the scan, the test passes. So the word is the only thing that keeps the iterator.generateHeapSnapshotForDebugging()runs its own collection from another stack, and that collection takes the iterator: the snapshot has only the class objects ofStatementSyncandStatementSyncIterator.The heap walk is not new (
Bun.gchas made it since #9540). oven-sh/WebKit#830 givesHeap::deleteAllUnlinkedCodeBlocksa larger frame, which moves where the marker lands. A fix for the marker itself would be a change in JSC of its own, and is not part of this PR.Self-review
The review of the first design (a mode compare in the lookup, the pinned mode kept) raised 33 concerns. It found a wrong closure binding and a crash with the control flow profiler, and asked for three PRs in place of one. 28 are addressed in the WebKit PRs. Not done:
vm.Script: the second linked copy then needs about 17 s of idle time.Related
vm.Script. That is the sharing site of the first repro.