WebKit upgrade to 46525dbf2abb#29897
Conversation
Adopts upstream's libpas Windows is_symmetric design (WebKit/WebKit#63762) while keeping Bun's TLC suspender / shutdown-guard / lazy-commit patches. Binding updates: - JSType.zig: PromiseReaction split into Slim/Full (+1 enum shift) - bindings.cpp: JSPromiseReaction::context() -> tryGetContext() - NodeVM.cpp: promiseEmptyOnRejectedFunction() removed -> jsUndefined() - NodeVMSourceTextModule.cpp / BunAnalyzeTranspiledModule.cpp: status()/hasTLA() setters renamed to setStatus()/setHasTLA() Unrelated fixes surfaced while validating: - dns.zig: c-ares can flip a TCP socket between writable/readable; FilePoll is single-direction, so unregister before re-registering instead of stacking both flags (tripped unregister assertion on conn error) - test/harness.ts: expectMaxObjectTypeCount treated 'undefined' count as not-yet-collected (undefined <= n is false), looping the full wait - sourcetextmodule-leak.test: scale iterations on debug builds
|
Updated 10:07 AM PT - May 1st, 2026
❌ @Jarred-Sumner, your commit 039dd63 has 4 failures in
DetailsDetails🧪 To try this PR locally: bunx bun-pr 29897That installs a local version of the PR into your bun-29897 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates the pinned WebKit commit used for prebuilt downloads; makes DNS FilePoll registrations single-directional; splits Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/api/bun/dns.zig`:
- Around line 2557-2559: The conditional that flips directions (checking
have_readable/want_readable and have_writable/want_writable) currently calls
poll.unregister(vm.event_loop_handle.?, false) which allows
FilePoll.unregisterWithFd’s fast path to skip kernel unregister and leaves
.poll_writable set; change the call to poll.unregister(vm.event_loop_handle.?,
true) (force unregister) so the kernel unregister always runs and poll direction
state is cleared when flipping from writable to readable; update the call site
in the block that uses have_readable/want_readable/have_writable/want_writable
to pass true.
In `@test/js/node/vm/sourcetextmodule-leak.test.ts`:
- Line 3: Replace the runtime require call with a module-scope import: change
the top-level "const { isDebug } = require('harness');" to a static import
statement "import { isDebug } from 'harness';" so the test uses module-scope
imports instead of require; locate the require for isDebug and update it
accordingly (ensure there are no other require usages in the file that should
also be converted).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b9314c0-e887-4af8-be9e-ce770b0b23a3
📒 Files selected for processing (9)
scripts/build/deps/webkit.tssrc/bun.js/api/bun/dns.zigsrc/bun.js/bindings/BunAnalyzeTranspiledModule.cppsrc/bun.js/bindings/JSType.zigsrc/bun.js/bindings/NodeVM.cppsrc/bun.js/bindings/NodeVMSourceTextModule.cppsrc/bun.js/bindings/bindings.cpptest/harness.tstest/js/node/vm/sourcetextmodule-leak.test.ts
| @@ -1,30 +1,40 @@ | |||
| const vm = require("vm"); | |||
| const { describe, it, expect } = require("bun:test"); | |||
| const { isDebug } = require("harness"); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/js/node/vm/sourcetextmodule-leak.test.ts | head -50Repository: oven-sh/bun
Length of output: 1593
Use module-scope import syntax instead of require in this test file.
Lines 1–3 use require() statements when the test is not specifically testing dynamic import/require behavior. Per coding guidelines, always use module-scope imports in such cases.
Suggested change
-const vm = require("vm");
-const { describe, it, expect } = require("bun:test");
-const { isDebug } = require("harness");
+import * as vm from "node:vm";
+import { describe, it, expect } from "bun:test";
+import { isDebug } from "harness";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { isDebug } = require("harness"); | |
| import * as vm from "node:vm"; | |
| import { describe, it, expect } from "bun:test"; | |
| import { isDebug } from "harness"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/vm/sourcetextmodule-leak.test.ts` at line 3, Replace the runtime
require call with a module-scope import: change the top-level "const { isDebug }
= require('harness');" to a static import statement "import { isDebug } from
'harness';" so the test uses module-scope imports instead of require; locate the
require for isDebug and update it accordingly (ensure there are no other require
usages in the file that should also be converted).
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const ITERATIONS = isDebug ? 2_000 : 50_000; | ||
| const THRESHOLD_MB = isDebug ? 300 : 3000; |
There was a problem hiding this comment.
🟡 nit: the debug-mode threshold (300MB) is looser per-iteration than release — 2,000 × ~50KB ≈ 100MB of source text, so even a 100% source-string leak wouldn't trip it. If the debug variant is intended to be a leak gate (vs. just a smoke/timeout fix), consider lowering THRESHOLD_MB to ~150 for isDebug; otherwise a comment that debug is intentionally just a crash/assert check would clarify.
Extended reasoning...
What this is
The new debug-mode constants are:
const ITERATIONS = isDebug ? 2_000 : 50_000;
const THRESHOLD_MB = isDebug ? 300 : 3000;The comment immediately above explains the test's intent: "50k×50KB ≈ 2.5 GB of source text — if module records leak their source we blow past the threshold." In release that holds (2.5GB of source vs a 3GB threshold, ratio ~0.83). In debug it doesn't: 2,000 × ~50KB ≈ 100MB of total source text against a 300MB threshold (ratio ~0.33). Put another way, the per-iteration budget went from ~60KB/iter (release) to ~150KB/iter (debug) — about 2.5× more permissive.
How it manifests
Concretely, walk one debug iteration:
sourceis a ~50,000-byte string (Buffer.alloc(50_000, ...)plus a few bytes of wrapper).- Each
go(i)constructs avm.SourceTextModulewith a unique copy of that source (source + "//" + i), links and evaluates it. - After 2,000 iterations and a forced GC, RSS delta is compared to
THRESHOLD_MB.
If the regression this test guards against returned — i.e. each module's SourceProvider/source string is never released — the raw source bytes alone total ~100MB. That sits comfortably below 300MB, so the assertion would still pass and the leak would be invisible on debug runs. The release variant (unchanged) remains the actual gate.
Why nothing else catches it
There's no other check in the debug path: the test computes (rss after − rss before) and asserts < THRESHOLD_MB. With the threshold at 3× the worst-case raw-source footprint, the only way the debug assertion fires is if a leak retains substantially more than the source string per module (record + executable + provider + ASAN redzones may or may not push it over, but the test no longer guarantees it the way the comment implies).
Why this is only a nit
- The PR's stated goal for the debug branch is to make a previously-timing-out test runnable ("50k iterations on debug ASAN can't fit in 5s"). It's a strict improvement over a test that never finished on debug; release CI is the real leak signal and is unchanged.
- Debug+ASAN has real RSS overhead (shadow memory, redzones, quarantine) that justifies extra headroom — a tight 100–120MB threshold could flake even with no leak.
- A real leaked module retains more than the 50KB source string (JSModuleRecord, ModuleProgramExecutable, identifier, etc.), so a full leak would likely still exceed 300MB under ASAN — it's just no longer guaranteed by the arithmetic the comment relies on.
This is the same objection the refutation raises, and it's fair: the change is intentional and net-positive. The point being flagged is narrower — the threshold and the comment are now inconsistent on debug, which can mislead a future reader into thinking the debug assertion bounds the source-leak case when it doesn't.
Suggested fix
Either lower the debug threshold so it still bounds a pure source leak with reasonable ASAN headroom, e.g.:
const THRESHOLD_MB = isDebug ? 150 : 3000;or, if 300MB is deliberately chosen for ASAN noise, add a one-line note that the debug variant is primarily a crash/assert smoke check and the leak gate is the release run. Non-blocking either way.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The current WEBKIT_VERSION value mixes a prebuilt tag with what tools
expect to be a commit hash; split it into two constants (e.g., WEBKIT_COMMIT for
the canonical git commit hash and WEBKIT_PREBUILT_TAG for the autobuild download
tag), update the export in webkit.ts to expose both symbols, and change
consumers: have scripts/sync-webkit-source.ts use WEBKIT_COMMIT for git checkout
and have scripts/build/depVersionsHeader.ts (and any process.versions.WEBKIT
reporting) read WEBKIT_COMMIT for version metadata while using
WEBKIT_PREBUILT_TAG only when resolving/download prebuilt binaries. Ensure names
match exactly (WEBKIT_COMMIT, WEBKIT_PREBUILT_TAG) so updates are
straightforward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bff775b-2b03-40cd-aa8e-630ba1679684
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "137c59666cfb02c45d04dcfd4b3852e27f78cffc"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-203-f7b52b08"; |
There was a problem hiding this comment.
Split source commit from prebuilt tag; current value breaks commit-based consumers.
Line 6 changes WEBKIT_VERSION from a commit-like value to an autobuild-* tag, but downstream tooling still treats it as a git commit (scripts/sync-webkit-source.ts does git checkout ${expectedCommit}) and version metadata logic expects a clean commit hash (scripts/build/depVersionsHeader.ts). This will break local source sync and muddies process.versions.WEBKIT.
Please keep a commit-hash source-of-truth (for checkout/version reporting) and introduce a separate prebuilt release-tag field for download resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/deps/webkit.ts` at line 6, The current WEBKIT_VERSION value
mixes a prebuilt tag with what tools expect to be a commit hash; split it into
two constants (e.g., WEBKIT_COMMIT for the canonical git commit hash and
WEBKIT_PREBUILT_TAG for the autobuild download tag), update the export in
webkit.ts to expose both symbols, and change consumers: have
scripts/sync-webkit-source.ts use WEBKIT_COMMIT for git checkout and have
scripts/build/depVersionsHeader.ts (and any process.versions.WEBKIT reporting)
read WEBKIT_COMMIT for version metadata while using WEBKIT_PREBUILT_TAG only
when resolving/download prebuilt binaries. Ensure names match exactly
(WEBKIT_COMMIT, WEBKIT_PREBUILT_TAG) so updates are straightforward.
Jarred-Sumner
force-pushed
the
jarred/webkit-upgrade-46525dbf2abb
branch
from
94ed419 to
e17dbd3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/build/deps/webkit.ts (1)
6-6:⚠️ Potential issue | 🟠 MajorSplit commit identity from prebuilt tag; current constant conflates both.
Line 6 changes
WEBKIT_VERSIONto anautobuild-*tag even though this constant is documented as the checkout commit/hash source-of-truth. That can break commit-based local source sync/version metadata flows. Keep a commit field and a separate prebuilt tag field, then use each in the appropriate consumer path.Proposed direction
- export const WEBKIT_VERSION = "autobuild-preview-pr-203-a5bf2e31"; + export const WEBKIT_COMMIT = "a5bf2e31"; // or full commit hash used for checkout/version metadata + export const WEBKIT_PREBUILT_TAG = "autobuild-preview-pr-203-a5bf2e31";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build/deps/webkit.ts` at line 6, The current WEBKIT_VERSION constant conflates the source commit and an autobuild prebuilt tag; split these into two constants: restore WEBKIT_VERSION (or introduce WEBKIT_COMMIT) to contain the actual checkout commit/hash as the source-of-truth, and add a new WEBKIT_PREBUILT_TAG (or WEBKIT_BUILD_TAG) set to "autobuild-preview-pr-203-a5bf2e31"; then update any consumers to use WEBKIT_COMMIT/WEBKIT_VERSION for checkout/sync logic and WEBKIT_PREBUILT_TAG for prebuilt artifact resolution (search for usages of WEBKIT_VERSION to ensure each consumer references the correct new symbol).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The current WEBKIT_VERSION constant conflates the source commit and an
autobuild prebuilt tag; split these into two constants: restore WEBKIT_VERSION
(or introduce WEBKIT_COMMIT) to contain the actual checkout commit/hash as the
source-of-truth, and add a new WEBKIT_PREBUILT_TAG (or WEBKIT_BUILD_TAG) set to
"autobuild-preview-pr-203-a5bf2e31"; then update any consumers to use
WEBKIT_COMMIT/WEBKIT_VERSION for checkout/sync logic and WEBKIT_PREBUILT_TAG for
prebuilt artifact resolution (search for usages of WEBKIT_VERSION to ensure each
consumer references the correct new symbol).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae641ccb-269d-4043-b143-19a67bb65031
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/bun.js/api/bun/dns.zig (1)
2566-2567:⚠️ Potential issue | 🟠 MajorForce unregister when flipping poll direction.
Line 2567 still uses
poll.unregister(..., false). In the non-forced path,FilePollcan retain the previous direction flag, so the subsequentregister()may leave stale poll state and violate the single-direction invariant this block is trying to enforce.Suggested change
- if ((have_readable and !want_readable) or (have_writable and !want_writable)) { - _ = poll.unregister(vm.event_loop_handle.?, false); - } + if ((have_readable and !want_readable) or (have_writable and !want_writable)) { + _ = poll.unregister(vm.event_loop_handle.?, true); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/api/bun/dns.zig` around lines 2566 - 2567, The unregister call in the direction-flip branch still passes false causing FilePoll to possibly retain previous direction; change the call to force-unregister so the old poll state is removed (i.e., call poll.unregister(vm.event_loop_handle.?, true)) before re-registering to ensure register() doesn't inherit stale direction flags (refer to poll.unregister, vm.event_loop_handle, FilePoll and the subsequent register() call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/bun.js/api/bun/dns.zig`:
- Around line 2566-2567: The unregister call in the direction-flip branch still
passes false causing FilePoll to possibly retain previous direction; change the
call to force-unregister so the old poll state is removed (i.e., call
poll.unregister(vm.event_loop_handle.?, true)) before re-registering to ensure
register() doesn't inherit stale direction flags (refer to poll.unregister,
vm.event_loop_handle, FilePoll and the subsequent register() call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1dd1ddf-6f9e-44f9-b7a5-7bb6efcdc8cc
📒 Files selected for processing (2)
scripts/build/deps/webkit.tssrc/bun.js/api/bun/dns.zig
| if ((heapStats().objectTypeCounts[type] ?? 0) <= count) return; | ||
| gc(true); | ||
| for (const wait = 20; maxWait > 0; maxWait -= wait) { | ||
| if (heapStats().objectTypeCounts[type] <= count) break; | ||
| if ((heapStats().objectTypeCounts[type] ?? 0) <= count) break; | ||
| await Bun.sleep(wait); | ||
| gc(); | ||
| } | ||
| expect(heapStats().objectTypeCounts[type] || 0).toBeLessThanOrEqual(count); | ||
| expect(heapStats().objectTypeCounts[type] ?? 0).toBeLessThanOrEqual(count); |
There was a problem hiding this comment.
🟡 Pre-existing nit: test/js/web/abort/abort.ts:5-20 has an inlined copy of expectMaxObjectTypeCount with the same undefined <= count pattern this fixes (lines 12/15/19 — line 19 lacks even the old || 0, so a fully-collected type would fail the assertion outright). Might be worth giving it the same ?? 0 treatment, or just replacing it with import { expectMaxObjectTypeCount } from "harness". Non-blocking — test-perf only and not touched by this PR.
Extended reasoning...
What this is
This PR's drive-by fix to test/harness.ts:145-152 adds ?? 0 to the three heapStats().objectTypeCounts[type] lookups in expectMaxObjectTypeCount, so that an absent type (lookup returns undefined) is treated as 0 rather than falling through the comparison. The PR description explicitly calls out the bug being fixed: "expectMaxObjectTypeCount treated undefined as not-yet-collected (undefined <= n is false) → looped 50 sync GCs even when fully collected."
test/js/web/abort/abort.ts:5-20 contains an inlined private copy of the same helper with the identical defect:
async function expectMaxObjectTypeCount(expect, type, count, maxWait = 1000) {
gc(true);
if (heapStats().objectTypeCounts[type] <= count) return; // line 12
gc(true);
for (const wait = 20; maxWait > 0; maxWait -= wait) {
if (heapStats().objectTypeCounts[type] <= count) break; // line 15
await new Promise(resolve => setTimeout(resolve, wait));
gc(true);
}
expect(heapStats().objectTypeCounts[type]).toBeLessThanOrEqual(count); // line 19
}None of the three lookups have a ?? 0 guard. Line 19 is actually slightly worse than the pre-PR harness.ts version, which at least had || 0 on the final expect — abort.ts has no fallback at all.
How it manifests
Walk through one call with type = "AbortSignal" and count = 10, assuming the GC has fully collected every AbortSignal so the key is absent from heapStats().objectTypeCounts:
- Line 12:
undefined <= 10→ coerces toNaN <= 10→false. The early-return is skipped even though 0 ≤ 10. - Loop runs ~50 iterations (1000ms / 20ms). Line 15 evaluates
undefined <= 10→falseevery time, so the loop never breaks early. Each iteration does a syncgc(true)plus a 20ms sleep. - Line 19:
expect(undefined).toBeLessThanOrEqual(10)— this fails the assertion (Bun's matcher does not coerceundefinedto 0), so a fully-collected type would actually make the test fail rather than pass.
The harness.ts fix in this PR addresses exactly steps 1-3 for the shared helper; the abort.ts copy retains all three.
Why nothing else catches it
abort.ts defines its own private expectMaxObjectTypeCount rather than importing from harness, so the harness.ts fix doesn't reach it. There's no shared abstraction — it's a copy-paste duplicate. The two callers in abort.ts (lines ~175-176, for "AbortController" and "AbortSignal") hit this local copy directly.
Why this is only a nit / pre-existing
- Not introduced by this PR: abort.ts is untouched in the diff; the bug predates this change.
- Unlikely to fire in practice:
AbortControllerandAbortSignalare essentially always present inheapStats().objectTypeCounts(the test harness/runtime keeps some alive), so theundefinedcase probably never triggers in CI — which is why line 19's missing|| 0hasn't been failing. - Test-perf only when it does trigger (lines 12/15): ~1s of wasted
gc(true)+ sleep per call, not a correctness issue for the leak-check itself. - The PR is a WebKit upgrade, not a "fix all expectMaxObjectTypeCount copies" PR; the harness.ts change was a drive-by surfaced during validation. The author isn't obligated to grep for copy-pasted duplicates.
That said, since the PR explicitly identifies and documents this exact pattern as a bug worth fixing, flagging the identical inlined duplicate is reasonable review feedback — the author is already in the mindset and may want to apply it (or de-duplicate) in the same pass.
Suggested fix
Either apply the same ?? 0 to abort.ts:12/15/19, or (better) delete the local copy and import { expectMaxObjectTypeCount } from "harness". Non-blocking either way.
…s both directions
…directional FilePoll support
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟣
src/bun.js/api/bun/dns.zig:2572-2574— Re: CodeRabbit's suggestion to passforce_unregister=truehere — that's unnecessary for DNS polls:.needs_rearmis only ever set when.one_shotis set (posix_event_loop.zig:338-339), and dns.zig registers withone_shot=false, so thisunregister()call can never reach theneeds_rearmfast path. That said, looking at that fast path did surface a real pre-existing copy-paste typo atsrc/async/posix_event_loop.zig:1067-1070:.poll_processis removed twice and.poll_writableis never removed (compare lines 1211-1214 which correctly remove all four). Not introduced or reachable from this PR's change — just worth a one-line fix while in the area.Extended reasoning...
What this is
CodeRabbit's inline comment on this line claims that
poll.unregister(vm.event_loop_handle.?, false)can hitFilePoll.unregisterWithFd'sneeds_rearmfast path, which "skips kernel unregister and currently does not clear.poll_writable", and suggests passingtrueto force the kernel unregister. Investigating that claim surfaces a real copy-paste typo insrc/async/posix_event_loop.zig:1067-1070:if (this.flags.contains(.needs_rearm) and !force_unregister) { log("unregister: {s} ({f}) skipped due to needs_rearm", .{ @tagName(flag), fd }); this.flags.remove(.poll_process); this.flags.remove(.poll_readable); this.flags.remove(.poll_process); // <-- duplicate; should be .poll_writable this.flags.remove(.poll_machport); return .success; }
.poll_processis removed twice and.poll_writableis never removed. Compare to the full unregister path at lines 1211-1214, which correctly removes.poll_readable,.poll_writable,.poll_process,.poll_machport.Why the dns.zig change cannot reach this path
.needs_rearmis inserted in exactly one place —FilePoll.onUpdateat posix_event_loop.zig:338-339:if (poll.flags.contains(.one_shot) and !poll.flags.contains(.needs_rearm)) { poll.flags.insert(.needs_rearm); }
It is gated on
.one_shot. The dns.zigregister()calls at lines 2577/2580 passone_shot=false(third arg), so DNS polls never have.one_shotset, never get.needs_rearmset, and therefore theif (this.flags.contains(.needs_rearm) and !force_unregister)branch at line 1065 is always false for them. The newunregister(..., false)call falls through to the full kernel-unregister path (lines 1076+ / 1207-1214), which correctly clears all four direction flags. CodeRabbit's suggestedforce_unregister=trueis therefore a no-op for dns.zig correctness.Step-by-step proof (hypothetical one-shot writable poll, not DNS)
For a poll that is one-shot — e.g. some other FilePoll consumer that calls
register(loop, .writable, true):register(.writable, one_shot=true)sets.poll_writableand.one_shot.- The fd becomes writable;
onUpdateruns. Line 338-339:.one_shotis set, so.needs_rearmis inserted. - Caller invokes
unregister(loop, false). Line 1065:.needs_rearmis set andforce_unregister=false, so the fast path runs. - Lines 1067-1070 remove
.poll_process,.poll_readable,.poll_process(again),.poll_machport..poll_writableremains set. - A subsequent
register(loop, .readable, ...)would set.poll_readable, leaving the poll holding both.poll_readableand.poll_writable, which trips thebun.assert(!(... .poll_readable and ... .poll_writable))at line 1210 the next time the full unregister path runs.
This is a real latent bug for one-shot writable polls elsewhere in the codebase; it just isn't the dns.zig case.
Why nothing else catches it
The fast path is specifically the "kernel already auto-removed this one-shot registration, just clear our bookkeeping" branch, so there's no kernel state to disagree with — the only thing wrong is the in-memory
.poll_writableflag lingering. It only manifests when the same FilePoll is reused with a different direction after a one-shot writable cycle, which is exactly the pattern this dns.zig change introduces (direction flips on a reused poll) — but dns.zig is non-one-shot, so it dodges the bug entirely.Suggested fix
One-line, in
src/async/posix_event_loop.zig:this.flags.remove(.poll_process); this.flags.remove(.poll_readable); - this.flags.remove(.poll_process); + this.flags.remove(.poll_writable); this.flags.remove(.poll_machport);Pre-existing and not in a file this PR touches; non-blocking, but trivially fixable while you're already looking at the FilePoll direction-flag invariant.
… call_indirect fix)
…ss_is_shutting_down)
…e>() (WebKit 23248aa3)
… assertion) (#29955) c-ares can request both readable and writable on the same TCP DNS fd — writable while connecting or when a send hits EAGAIN, readable for the response. `dns.zig:onDNSSocketState` registered both directions on a single `FilePoll`, which set both `.poll_readable` and `.poll_writable`; on close, `unregisterWithFd` tripped `bun.assert(!(poll_readable && poll_writable))`. On epoll the second `register` was also a `CTL_MOD` that silently overwrote the first direction's mask. ### Changes `src/async/posix_event_loop.zig`: - **`registerWithFd` (epoll):** when the other direction is already registered on this poll, OR its event mask into the `CTL_MOD` so both stay armed. `debugAssert` against bidirectional + one-shot (EPOLLONESHOT disarms the whole fd on the first event in either direction — not hit by the DNS path, which passes one_shot=false). - **`registerWithFd` (kqueue):** unchanged. kqueue keys on `(ident, filter)`, so the second `register()` call's `EV_ADD` creates a separate knote for the new filter without touching the existing one — there is no "already registered → modify" branch on kqueue. - **`unregisterWithFd` (kqueue mac/freebsd):** when both directions are set, submit two `EV_DELETE` changes (`EVFILT_READ` + `EVFILT_WRITE`) in one kevent call. epoll's `CTL_DEL` keys on fd alone so already removes both. Dropped the both-directions assertion. - macOS: changelist is `[2]kevent64_s`. The rc<0 global-error check now runs before per-entry `EV_ERROR` checks, which are guarded on `rc >= N` (with `KEVENT_FLAG_ERROR_EVENTS` error events are packed from index 0, not positionally matched to changes). - FreeBSD: `nevents=0`, so per-entry errors surface as rc=-1/errno for the first failing change; a silent miss on the second `EV_DELETE` (ENOENT) is harmless. - `needs_rearm` early-return now also clears `.poll_writable` (was only clearing readable/process/machport). `src/bun.js/api/bun/dns.zig:onDNSSocketState` (POSIX): when c-ares's desired direction set differs from what's registered: - **Adding only** (e.g. W → R+W): just `register()` the new direction — one `CTL_MOD` on epoll, one `EV_ADD` on kqueue. - **Dropping a direction** (e.g. W → R): `unregister()` then re-`register()` the remaining direction(s). FilePoll has no per-direction unregister, and leaving writable armed on a level-triggered connected socket would busy-loop. c-ares DNS fds are short-lived so the extra syscall is acceptable. `activate`/`deactivate` are already idempotent via `has_incremented_poll_count`, so registering twice keeps the loop refcount correct. ### Tests - `bun run zig:check-all`: all platforms compile (Linux/macOS/Windows × debug/release, x64/arm64). - `test/js/node/dns/` + `test/js/bun/dns/`: 137 pass / 2 fail (both pre-existing env-dependent: `dns.prefetch` cache-hit timing, `dns.getServers` resolv.conf — fail identically on parent of this commit). - `test/js/node/async_hooks/`: 110 pass / 3 todo / 1 fail (pre-existing `http-clientrequest` timeout, unrelated). - `async-context-dns-resolveTxt.js` fixture (original repro): exit 0. Split out of #29897 per review. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… assertion) (oven-sh#29955) c-ares can request both readable and writable on the same TCP DNS fd — writable while connecting or when a send hits EAGAIN, readable for the response. `dns.zig:onDNSSocketState` registered both directions on a single `FilePoll`, which set both `.poll_readable` and `.poll_writable`; on close, `unregisterWithFd` tripped `bun.assert(!(poll_readable && poll_writable))`. On epoll the second `register` was also a `CTL_MOD` that silently overwrote the first direction's mask. ### Changes `src/async/posix_event_loop.zig`: - **`registerWithFd` (epoll):** when the other direction is already registered on this poll, OR its event mask into the `CTL_MOD` so both stay armed. `debugAssert` against bidirectional + one-shot (EPOLLONESHOT disarms the whole fd on the first event in either direction — not hit by the DNS path, which passes one_shot=false). - **`registerWithFd` (kqueue):** unchanged. kqueue keys on `(ident, filter)`, so the second `register()` call's `EV_ADD` creates a separate knote for the new filter without touching the existing one — there is no "already registered → modify" branch on kqueue. - **`unregisterWithFd` (kqueue mac/freebsd):** when both directions are set, submit two `EV_DELETE` changes (`EVFILT_READ` + `EVFILT_WRITE`) in one kevent call. epoll's `CTL_DEL` keys on fd alone so already removes both. Dropped the both-directions assertion. - macOS: changelist is `[2]kevent64_s`. The rc<0 global-error check now runs before per-entry `EV_ERROR` checks, which are guarded on `rc >= N` (with `KEVENT_FLAG_ERROR_EVENTS` error events are packed from index 0, not positionally matched to changes). - FreeBSD: `nevents=0`, so per-entry errors surface as rc=-1/errno for the first failing change; a silent miss on the second `EV_DELETE` (ENOENT) is harmless. - `needs_rearm` early-return now also clears `.poll_writable` (was only clearing readable/process/machport). `src/bun.js/api/bun/dns.zig:onDNSSocketState` (POSIX): when c-ares's desired direction set differs from what's registered: - **Adding only** (e.g. W → R+W): just `register()` the new direction — one `CTL_MOD` on epoll, one `EV_ADD` on kqueue. - **Dropping a direction** (e.g. W → R): `unregister()` then re-`register()` the remaining direction(s). FilePoll has no per-direction unregister, and leaving writable armed on a level-triggered connected socket would busy-loop. c-ares DNS fds are short-lived so the extra syscall is acceptable. `activate`/`deactivate` are already idempotent via `has_incremented_poll_count`, so registering twice keeps the loop refcount correct. ### Tests - `bun run zig:check-all`: all platforms compile (Linux/macOS/Windows × debug/release, x64/arm64). - `test/js/node/dns/` + `test/js/bun/dns/`: 137 pass / 2 fail (both pre-existing env-dependent: `dns.prefetch` cache-hit timing, `dns.getServers` resolv.conf — fail identically on parent of this commit). - `test/js/node/async_hooks/`: 110 pass / 3 todo / 1 fail (pre-existing `http-clientrequest` timeout, unrelated). - `async-context-dns-resolveTxt.js` fixture (original repro): exit 0. Split out of oven-sh#29897 per review. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
# WebKit upgrade: aac4aed489d1 → 46525dbf2abb (264 commits, 40 in JSC/WTF/bmalloc) This upgrade pulls in Yusuke's libpas Windows cleanup (which we consolidate with our local Windows patches — see PR body), a `JSPromiseReaction` → `JSSlimPromiseReaction`/`JSFullPromiseReaction` split that shifts `JSType` enum values, further module-loader spec alignment, Map/Set fast iteration in DFG/FTL, and several Intl/String/Array micro-optimizations. ##⚠️ Bun-affecting - **61a88753e9e4** `JSPromiseReactionType` split into `JSSlimPromiseReactionType` + `JSFullPromiseReactionType`. Shifts every `JSType` value after it by +1. **`src/bun.js/bindings/JSType.zig` must be regenerated.** Any Bun binding referencing `JSPromiseReaction` directly needs updating. - **8af021fef543** More module-loader spec alignment (touches `JSModuleLoader.cpp`, `ModuleGraphLoadingState.h`). Conflicted with our recent module-loader perf work; merged. - **78682e818cf8** Avoid per-edge `ModuleLoaderPayload` allocation in graph loading — overlaps the same area as our `loadRequestedModules` memoization drop. - **0e74ad95db8e** libpas Windows cleanup — see "libpas consolidation" section below. - **99779db9ef0c** libpas: remove partial views. Renames `kind_and_role` → `kind` across segregated-view types; auto-merged into our TLC code. - **723db2e10e02** CMake: match Xcode's bmalloc/WTF/JSC build approach. May affect `build.ts` if it relied on old target layout. ## Runtime / builtins - 433704897995 Map/Set fast iteration (new DFG/FTL path) - aa6c00c696e6 Inline `String#localeCompare` tight loop in FTL - f6d1b6eb589d `padStart`/`padEnd` produce flat strings for short results - bf720cf23f89 More truncate-double-to-int adoption in `ArrayPrototype` - 370fcab2a837 Remove `@isArraySlow` - e14f98504927 Move `Symbol.hasInstance` to C++ - d1ec7b5323ad Remove `createSuppressedError` builtin - 10c13e90401d Refine `clobberize` rule for `ObjectCreate` - bfab8953b9ed `emitBytecodeInConditionContext` for optional chaining ## Intl - 5da32d944f90 Lazily resolve default calendar/numberingSystem in Intl constructors - 3d3f4763409e Delay `intlAvailableTimeZoneIndex` init - a60e76055b01 Initialize IANA TimeZone data at VM creation ## Parser / Yarr - 1088aa8bd998 YARR: reject dangling hyphen in `/v` class set - d4029d5d9a79 Build-speed fixes in JSC headers (forward decls / include hygiene — caused several trivial conflicts) ## Wasm - b05d16f9ce83 Save source offset for Wasm const-expr evaluation (better error locations) - 008ce40466bc OMG: only emit `WasmBoundsCheck` overflow guard for Int64 pointers - 475a14fe4091 JSPI: change return-PC signing in `EvacuatedStackSlices` ## Heap / GC - 5ade7c0d6d20 Remove `printInternal(ConstraintVolatility)` overload - 0d7053c9c316 Introduce `UnbarrieredMonotonicTime` ## libpas / bmalloc - **0e74ad95db8e** Windows cleanup: `is_symmetric` commit/decommit flag, `virtual_query_checked`, hardened `pthread_cond_timedwait`, proper `pthread_create`/`detach` split. **We adopt this wholesale and re-layer our `is_exiting` flag, `RtlDllShutdownInProgress` guard, `pas_thread_suspender`, compact-heap lazy-commit, and TLC-decommit OS-gate widening on top — none subsumed.** - 99779db9ef0c Remove partial views (segregated-view simplification) - 0e0e65cf0c2e Upgrade libpas to gnu++23 ## WTF - f69c4f5d8453 `WTF::Liveness` uses worklist - fd0db67fd877 Fix `utf8ForCharacters` crash on trailing unpaired surrogate - 9b4359d195c9 Adopt `LIFETIME_BOUND` in `downcast` family - f55db8dff653 Web-content hang fix (timer-related; touches WTF run-loop) - 41ae266227a4 Codegen: emit basenames not abs paths in generated comments (reproducible builds) ## Build / misc - b1b0e820cdf6 Fix `LLIntAssembly.h` `jsCast` undeclared (regression fix) - 21ce77b3d34b / 466ee713fc9c / a24949c80905 UnifiedBuild grouping churn (landed, reverted, relanded) --- WebKit PR: oven-sh/WebKit#203 WebKit pin: `f7b52b080538` ## Binding adaptations - `JSType.zig`: PromiseReaction → Slim/Full split (+1 enum shift) - `bindings.cpp`: `reaction->context()` → `JSPromiseReaction::tryGetContext()` - `NodeVM.cpp`: `promiseEmptyOnRejectedFunction()` removed → `jsUndefined()` - `NodeVMSourceTextModule.cpp` / `BunAnalyzeTranspiledModule.cpp`: `status()`/`hasTLA()` setters → `setStatus()`/`setHasTLA()` ## Drive-by fixes (pre-existing, surfaced during validation) - **dns.zig**: c-ares can flip a TCP socket between writable↔readable; FilePoll is single-direction, so unregister before re-registering instead of stacking both flags (asserts on close after conn error) - **test/harness.ts**: `expectMaxObjectTypeCount` treated `undefined` as not-yet-collected (`undefined <= n` is `false`) → looped 50 sync GCs even when fully collected - **sourcetextmodule-leak.test**: 50k iterations on debug ASAN can't fit in 5s — scale to 2k/60s on debug
2 participants
WebKit upgrade: aac4aed489d1 → 46525dbf2abb (264 commits, 40 in JSC/WTF/bmalloc)
This upgrade pulls in Yusuke's libpas Windows cleanup (which we consolidate with our local Windows patches — see PR body), a
JSPromiseReaction→JSSlimPromiseReaction/JSFullPromiseReactionsplit that shiftsJSTypeenum values, further module-loader spec alignment, Map/Set fast iteration in DFG/FTL, and several Intl/String/Array micro-optimizations.JSPromiseReactionTypesplit intoJSSlimPromiseReactionType+JSFullPromiseReactionType. Shifts everyJSTypevalue after it by +1.src/bun.js/bindings/JSType.zigmust be regenerated. Any Bun binding referencingJSPromiseReactiondirectly needs updating.JSModuleLoader.cpp,ModuleGraphLoadingState.h). Conflicted with our recent module-loader perf work; merged.ModuleLoaderPayloadallocation in graph loading — overlaps the same area as ourloadRequestedModulesmemoization drop.kind_and_role→kindacross segregated-view types; auto-merged into our TLC code.build.tsif it relied on old target layout.Runtime / builtins
String#localeComparetight loop in FTLpadStart/padEndproduce flat strings for short resultsArrayPrototype@isArraySlowSymbol.hasInstanceto C++createSuppressedErrorbuiltinclobberizerule forObjectCreateemitBytecodeInConditionContextfor optional chainingIntl
intlAvailableTimeZoneIndexinitParser / Yarr
/vclass setWasm
WasmBoundsCheckoverflow guard for Int64 pointersEvacuatedStackSlicesHeap / GC
printInternal(ConstraintVolatility)overloadUnbarrieredMonotonicTimelibpas / bmalloc
is_symmetriccommit/decommit flag,virtual_query_checked, hardenedpthread_cond_timedwait, properpthread_create/detachsplit. We adopt this wholesale and re-layer ouris_exitingflag,RtlDllShutdownInProgressguard,pas_thread_suspender, compact-heap lazy-commit, and TLC-decommit OS-gate widening on top — none subsumed.WTF
WTF::Livenessuses worklistutf8ForCharacterscrash on trailing unpaired surrogateLIFETIME_BOUNDindowncastfamilyBuild / misc
LLIntAssembly.hjsCastundeclared (regression fix)WebKit PR: oven-sh/WebKit#203
WebKit pin:
f7b52b080538Binding adaptations
JSType.zig: PromiseReaction → Slim/Full split (+1 enum shift)bindings.cpp:reaction->context()→JSPromiseReaction::tryGetContext()NodeVM.cpp:promiseEmptyOnRejectedFunction()removed →jsUndefined()NodeVMSourceTextModule.cpp/BunAnalyzeTranspiledModule.cpp:status()/hasTLA()setters →setStatus()/setHasTLA()Drive-by fixes (pre-existing, surfaced during validation)
expectMaxObjectTypeCounttreatedundefinedas not-yet-collected (undefined <= nisfalse) → looped 50 sync GCs even when fully collected