Conversation
Web IDL converts a HeadersInit through the Symbol.iterator method of the
value. Bun copied the header list of any Headers object and never read
that method, so an override on a subclass, on the instance or on
Headers.prototype had no effect. A Headers subclass that keeps its
entries in its own storage (SuperHeaders in @mjackson/headers) lost every
header in new Response(body, { headers }). WPT covers the conversion in
fetch/api/headers/headers-basic.any.js ("Create headers with existing
headers with custom iterator").
JSFetchHeaders::toWrappedAsInit (FetchHeaders::cast_as_init in Rust)
returns the wrapped headers only when a side-effect-free lookup of
Symbol.iterator finds the built-in Headers.prototype.entries. The places
that took a Headers object as a HeadersInit now use it and fall back to
the generic conversion: new Headers(), Response and Request init,
fetch() headers and proxy.headers, server.upgrade() and the WebSocket
proxy.headers option.
server.upgrade() treated an empty conversion as an invalid value, so
{ headers: {} } threw. An empty HeadersInit now adds no header.
FetchHeaders::cast has no caller left and is removed. The WPT file is
vendored byte-identical with a driver next to textstream-wpt.test.ts.
|
Status Reproduced on main (1.4.3-canary,
CI (#116928, Review: the threads are resolved. Two findings from the review are on main already and have their own trackers (#41875, #43027). |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughChangesThe PR adds guarded HeadersInit conversion and consumers
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The changed HeadersInit paths have guarded fallback and error handling with no established merge-blocking regression. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked: the toWrappedAsInit walk really runs no user code (it bails on overridesGetOwnPropertySlot, static property tables, and overridesGetPrototype before every getDirect/getPrototypeDirect, and an accessor surfaces as a GetterSetter cell that fails the JSFunction cast); the new FFI entry matches parameter-for-parameter across headers.h, JSFetchHeaders.cpp, and FetchHeaders.rs; and the remaining cast_/dynamicDowncast<JSFetchHeaders> users (server.fetch() at server_body.rs:2313, the console formatter, BunInjectedScriptHost, deepEquals) are either not HeadersInit positions or explicitly left out in the description.
Extended reasoning...
Inline findings are being posted on the server.upgrade() sites in src/runtime/server/server_body.rs, so a human look is already signaled. This note only records what else was examined and ruled out from reading the diff: the prototype-chain walk in src/jsc/bindings/webcore/JSFetchHeaders.cpp (toWrappedAsInit) is gated so getDirect and getPrototypeDirect never reach an object that can answer from outside its Structure; the safe fn extern declaration in src/jsc/FetchHeaders.rs matches the C++ definition and the headers.h prototype; and the sibling-site grep shows the only HeadersInit-position shortcut not converted is server.fetch(), which the description explicitly defers to another PR. None of this is a guarantee of correctness of the change as a whole.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/server/server_body.rs— Callers of server.upgrade(req, { headers }) who pass their own Headers object with Sec-WebSocket-Protocol set have that header silently deleted from their object after the call. The copy path at server_body.rs:1727 hands the caller's live FetchHeaders to fast_remove at server_body.rs:1754 and :1763 (and :1963, :1967 at the second site), so a Headers reused across upgrades sends the protocol only on the first one. Fix: never mutate a user-owned FetchHeaders; clone the list (clone_this) when cast_as_init returns a borrowed pointer, or read the two values and skip them in to_uws_response instead of removing them.Extended reasoning...
The base branch does this too on the same lines (verified from the diff: the fast_remove lines are unchanged context), which is why it was dismissed. But this PR is the change that rewires which objects reach those lines and the base already carries the comment 'Copied out because fast_remove frees the entry' without addressing that the entry belongs to the user. Step 1: const h = new Headers({ 'Sec-WebSocket-Protocol': 'chat' }) at module scope, reused per connection. Step 2: server.upgrade(req, { headers: h }) — cast_as_init at :1727 returns the borrowed list. Step 3: fast_get then fast_remove at :1750-1754 removes Sec-WebSocket-Protocol from h. Step 4: the first client gets the protocol; every later client upgraded with the same h gets no Sec-WebSocket-Protocol header, and a browser client that requested a subprotocol fails the handshake. Population: every Bun.serve and node:http WebSocket server that reuses a Headers object; rate is once per connection. Remedy: clone_this before mutation, or filter the two names during to_uws_response without mutating.
Verification: pre-existing. Trigger: a caller passes a plain
Headersobject (built-in Symbol.iterator) containingSec-WebSocket-Protocol/Sec-WebSocket-Extensionstoserver.upgrade(req, { headers })and reuses that object afterwards. Mechanism verified:JSFetchHeaders::toWrappedAsInit(src/jsc/bindings/webcore/JSFetchHeaders.cpp, new code) returns&wrapper->wrapped()— the JS object's own live…
|
On the two findings of the review, both of which are on main already:
No change to this PR for either one. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 2:02 AM PT - Sep 17th, 2026
❌ @robobun, your commit b276ab8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43023That installs a local version of the PR into your bun-43023 --bun |
Backport the complete HeadersInit guard and caller cutover from oven-sh#43023 at b276ab8 onto the fb7c integration line. The older source keeps FetchSession proxy parsing inline in fetch.rs, so map that caller there. Apply the same guarded conversion to server.fetch and clone its native header list for Request ownership. This preserves the caller's Headers snapshot and incorporates the matching ownership correction from oven-sh#40888 without adopting the JS wrapper's borrowed reference.
Add Unreleased notes for the five-commit integration range from fb7c3a5 through 3ff0efc. Runtime code, tests and build inputs remain unchanged from 3ff0efc. Its Linux optimized and Debug/ASAN selections each passed 560 cases and four programs, with four existing skips and 184 filter exclusions. The canonical Rust cross-target check passed all 12 targets with no skips. Retain implementation credit for robobun's Headers iterator work in oven-sh#43023 and Jarred Sumner's server.fetch header-copy correction in oven-sh#40888, both incorporated by f5234e3.
Backport the complete HeadersInit guard and caller cutover from oven-sh#43023 at b276ab8 onto the fb7c integration line. The older source keeps FetchSession proxy parsing inline in fetch.rs, so map that caller there. Apply the same guarded conversion to server.fetch and clone its native header list for Request ownership. This preserves the caller's Headers snapshot and incorporates the matching ownership correction from oven-sh#40888 without adopting the JS wrapper's borrowed reference. (cherry picked from commit f5234e3)
Backport the complete HeadersInit guard and caller cutover from oven-sh#43023 at b276ab8 onto the fb7c integration line. The older source keeps FetchSession proxy parsing inline in fetch.rs, so map that caller there. Apply the same guarded conversion to server.fetch and clone its native header list for Request ownership. This preserves the caller's Headers snapshot and incorporates the matching ownership correction from oven-sh#40888 without adopting the JS wrapper's borrowed reference. (cherry picked from commit f5234e3)
This branch has not been deployed
1 participant
Problem
Headersobject given as aHeadersInitis copied from its header list. Bun never reads itsSymbol.iterator. Node and browsers call it. A subclass with its own storage loses every header: [headers]SuperHeadersnot compatible with Bun'sResponseconstructor remix-run/remix#10872 (SuperHeaders). WPT "Create headers with existing headers with custom iterator" fails.JSFetchHeaders.cpp:136,JSWebSocket.cpp:316,Response.rs:1278,fetch.rs:1135,fetch.rs:1164,FetchSession.rs:124,server_body.rs:1203.Fix
JSFetchHeaders::toWrappedAsInit(FetchHeaders::cast_as_initin Rust). It returns the header list only when a lookup ofSymbol.iterator, which runs no user code, finds the built-inHeaders.prototype.entries. Each shortcut uses it and keeps its fallback, the generic conversion.fetch()still sends names in the case the user wrote. Undici has the same rule. Cost: 10 to 25 ns per convertedHeadersobject.test/js/web/fetch/headers.test.ts(17 new tests, 15 fail before) and the vendoredwpt/headers-basic.any.js(1 of 23 fails before).cast_as a predicate, becauseserver.fetch()still needs the pointer.Background
HeadersInitis the Web IDL type of everyheadersvalue: a sequence of pairs or a record. For an object, Web IDL readsSymbol.iterator. If it is a method, the value converts through it.FetchHeadersis the C++ header list.JSFetchHeadersis its JS wrapper, theHeadersobject.Notes
Repro. With
@mjackson/headers@0.10.0(the class from the Remix report):Without a package:
[["x","y"]][["x","y"]][["own","1"]][["a","b"]][["a","b"]][["a","b"]][["x","y"]][["x","y"]][["own","1"]]Entry points.
new Headers(),ResponseandRequestinit (new Response,Response.json,Response.redirect,new Request),fetch()headersandproxy.headers(alsoBun.FetchSession),server.upgrade(), and theWebSocketproxy.headersoption. TheWebSocketheadersoption already used the generic conversion.The rule. Undici takes its copy path when
V[Symbol.iterator] === Headers.prototype.entriesandVis not a Proxy (lib/web/fetch/headers.js,webidl.converters.HeadersInit).toWrappedAsInitidentifies the built-in by its host function, so theentriesof another realm also passes. A Proxy of aHeadersis not aJSFetchHeaders, so it never reached the copy.The lookup. The function walks the prototype chain with
getDirect. It stops, and returns null, at an object that overridesgetOwnPropertySlotorgetPrototype, or that has a static property table. A Proxy in the chain is the practical case. The generic conversion then does the real[[Get]], so agettrap or an accessor runs exactly once. Two tests pin that. TogetDirectan accessor is aGetterSettercell, which is not aJSFunction, so the function returns null for it too.server.upgrade()and an empty init.create_from_jsreturnsNonefor an emptyHeadersInit, andserver.upgrade()treatedNoneas an invalid value. On mainserver.upgrade(req, { headers: {} })throwsupgrade options.headers must be a Headers or an object. With this PR aHeadersobject whose iterator yields nothing reaches the same code, so an empty conversion now adds no header, for{}and[]too. A test covers the three.Cost. One release binary with the walk switched on and off between rounds (a temporary flag, not in this PR). Best of 3 runs of 21 rounds, 1M calls per round, ns per call. The machine was busy, so the last digit is noise.
new Headers(h), 1 headernew Headers(h), 8 headersnew Headers(subclassInstance), 8 headersnew Response(null, { headers }), emptynew Response(null, { headers }), 1 headernew Response(null, { headers }), 8 headersnew Request(url, { headers }), 8 headersA plain object or an array does not reach the check. The walk costs more inside
new Responsethan in thenew Headersloop. It reads about ten cache lines (theHeaders.prototypestructure, its property table, the function, its executable), and they stay hot only in the small loop.The first version used a
PropertySlot::VMInquirylookup. In the same kind of one-binary comparison it cost 17 to 23 ns onnew Headers(h), against 10 ns for the walk, so this PR has the walk. A watchpoint onHeaders.prototype[Symbol.iterator]would make the common case a pointer compare. It needs per-realm state and adaptive-watchpoint plumbing that the bindings do not use anywhere today, so it is not part of this PR.Not changed:
server.fetch(). It has the same shortcut (server_body.rs:2310). #40888 rewrites that block to fix a double free, and a change here would conflict with it. The same one-token change applies after #40888 lands.Not changed: a
RequestorResponsegiven as the init object (new Response(body, response),new Request(url, request)). That shortcut (Init::init,Request::construct_into) copies the fields of the platform object and skips every getter of the dictionary,headersincluded. Node readsresponse.headersthere and converts it. It is a different shortcut with a different precondition, and it never looks at aHeadersobject.WPT.
headers-basic.any.jsis byte-identical to upstream (blobead1047645a1), with a driver that copiestextstream-wpt.test.ts. The other files offetch/api/headers/are not vendored here. They fail for reasons that this PR does not touch (forbidden header names,Set-Cookieiteration order), or they needsetup(), which the shim does not have.Dead code.
FetchHeaders::casthas no caller left, so this PR removes it.fetch_headers_from_jsinserver_body.rsloses its unusedglobalparameter. The new FFI entry point is inJSFetchHeaders.cpp, not inbindings.cpp.Self-review. Five concerns, four addressed:
server.fetch()(that site is now left out)server.upgrade()threw for aHeadersobject whose iterator yields nothing (fixed, with a test)FetchHeaders::cast_as a boolean predicate (not done:server.fetch()still uses the pointer)Suites run on the debug ASAN build.
headers.test.tsandwpt/headers-basic-wpt.test.ts(139 pass, also underBUN_JSC_validateExceptionChecks=1),headers.undici.test.ts,headers-case.test.ts,fetch_headers.test.js,fetch-args.test.ts,fetch-session.test.ts,response.test.ts,body.test.ts,body-clone.test.ts,request.test.ts,request-subclass.test.ts,test/js/deno/fetch/{headers,request,response}.test.ts,websocket-proxy.test.ts,websocket-custom-headers.test.ts,proxy.test.ts,test/js/first_party/ws/ws.test.ts: all pass.websocket-server.test.tsandbun-server.test.tshave tests that spawn a client and time out when the whole file runs on this loaded machine. The set changes between runs, and the tests pass in smaller runs with-t.[human-review] gate passed · iteration 0 · 12 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file