Repository navigation
Conversation
…caller's Blob
new Response([blob]), new Request(url, { body: [blob] }) and
server.fetch(url, { body: blob }) built their body through the MOVE
variant of Blob::get, which took the StoreRef out of the JS-visible Blob.
The caller's Blob (or File, Bun.file(), BuildArtifact) kept reporting its
size but read back empty afterwards, and a byte-backed File lost its name.
The single-Blob-part fast path now always returns blob.dupe(), which
shares the refcounted store, the same thing the direct new Response(blob)
path and the Blob constructor already do. With no caller left that moves,
the MOVE const generic and the from_js_move / from_js_clone /
from_js_clone_optional_array wrappers are removed.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: ready for review. Reproduced on bun 1.4.0 with The 12 new cases in test/js/web/fetch/body.test.ts fail on the unfixed build and pass with this branch; the related blob/body/FormData suites still pass. |
There was a problem hiding this comment.
LGTM — the fix replaces take_store() with the dupe() path already used everywhere else that derives a Blob from a JS-visible Blob, and deletes the now-dead MOVE generic.
What was reviewed:
- The one behavioral change (
blob.dupe()instead of the field-copy +take_store()) — refcount is balanced by the body Blob's eventualdeinit(). - The
might_only_be_one_thing || !MOVE→might_only_be_one_thingsimplification: for a multi-part arraytop_valueis still the array itself, so none of the fast-path match arms fired and the block fell through — behavior-preserving. - All five
Blob::get::<..>callers are updated to the single-generic form; no lingeringfrom_js_move/from_js_clone*references anywhere. - Tests cover Request/Response × plain Blob/File/
Bun.file()/BuildArtifact plusserver.fetch({ body }), and assert both the body's bytes and the source Blob's bytes/name after use.
Extended reasoning...
Overview
The PR fixes new Response([blob]) / new Request(url, { body: [blob] }) / server.fetch(url, { body: blob }) emptying the caller's Blob. In Blob::from_js_without_defer_gc's single-Blob-part fast path, the MOVE = true arm built the body Blob with store: blob.take_store(), stealing the store out of the JS-visible Blob the user still holds. The fix drops the MOVE const generic entirely and returns blob.dupe() unconditionally — the same shared-store path already used for new Blob([blob]), new Response(blob), Bun.write(dst, [blob]), and the BuildArtifact arm. With no caller left that moved, the from_js_move/from_js_clone/from_js_clone_optional_array wrappers and the now-unused Cell/JsCell imports are deleted.
Security risks
None. This is a Blob store refcount fix; no auth/crypto/permissions surface.
Level of scrutiny
Medium — this touches the store-refcount path, which is memory-safety adjacent. But the change moves from a dangerous pattern (mutating a JS-visible object's owning field out from under the user) to the well-trodden safe pattern (dupe() bumps the store refcount, released when the body's Value::Blob is dropped via Value::drop → b.deinit()). I traced the might_only_be_one_thing || !MOVE → might_only_be_one_thing collapse: when might_only_be_one_thing is false, top_value is still the original array (JSType Array/DerivedArray), which matches none of the fast-path arms (string-like / array-buffer-like / DOMWrapper) and fail_if_top_value_is_not_typed_array_like stays false, so the block was a no-op there — the removal is behavior-preserving as the description claims.
Other factors
- All five
Blob::get::<..>call sites (Blob constructor, File constructor,write_file_internal,Body::Value::from_js,Server::on_fetch) are updated; a repo grep confirms no remaining references to the deleted wrappers. - The
as_::<Blob>()→as_class_ref::<Blob>()swap removes one raw-pointer deref in favor of the safe accessor already used in the sibling BuildArtifact arm and inBody.rs:991. - Tests are thorough: they cover the whole variant matrix (Request AND Response × plain Blob / same Blob twice / File name+bytes /
Bun.file()/ BuildArtifact, plusserver.fetchbare Blob and[blob]), assert both the body's read-back and the source Blob's read-back, and the description confirms theUSE_SYSTEM_BUN=1-fails /bun bd test-passes discipline. - The dead
MOVEmachinery is deleted in the same PR, per the repo's "delete dead code in the same PR that makes it dead" rule.
|
Closing: #38503 now carries this change (the |
Problem
new Response([blob])the response reads"hello world"butblob.text()resolves to""whileblob.sizestill reports 11.new Request(url, { body: [blob] }), and forserver.fetch(url, { body: blob })even without the array (that path calls the helper directly). AFilepart additionally loses itsname, and aBun.file()orBuildArtifactpart reads back empty afterwards.Body::Value::from_js(src/runtime/webcore/Body.rs:1053) andServer::on_fetch(src/runtime/server/server_body.rs:2513) calledBlob::get::<MOVE = true, ..>. In the single-Blob-part fast path offrom_js_without_defer_gc(src/runtime/webcore/Blob.rs,JSType::DOMWrapperarm)MOVEbuilt the body blob withstore: blob.take_store(), which takes the store out of the JS-visible Blob the user still holds. The non-MOVEarm and the directnew Response(blob)path (Body.rs:991) share the store withdupe()instead.as_::<Blob>()also matches aBuildArtifact(it returns the artifact's inner blob), which is why artifacts were affected too.Fix
blob.dupe()unconditionally (the one behavioral change in the diff).dupe()creates a new view on the same refcounted store, so the body is still zero-copy; the only difference from the removed field copy is the extra store reference, which is exactly what keeps the caller's Blob valid.new Blob([blob]),new Response(blob),Bun.write(dst, [blob]),blob.slice()) takes its own reference, and Node/the spec never modify a Blob used as a body (Node stringifies the array to"[object Blob]"and leaves the Blob intact; Bun keeps its array-of-parts extension, it just stops mutating the part).MOVEconst generic and thefrom_js_move/from_js_clone/from_js_clone_optional_arraywrappers it selected between are dead and are deleted;getnow takes onlyREQUIRE_ARRAY. Themight_only_be_one_thing || !MOVEcondition becomesmight_only_be_one_thing: for a multi-part array the fast-path block matched nothing and fell through, so this is behavior preserving. The now unusedCell/JsCellimports go with it.array containing a Blob(run for both Request and Response: plain Blob, the same Blob backing two bodies, File name + bytes, Bun.file(), BuildArtifact) andserver.fetch() body option(bare Blob and[blob]). All 12 fail on the unfixed build (USE_SYSTEM_BUN=1) with the source reading back""(andname: undefinedfor the File), and pass withbun bd test.handle stack overflowtimes out locally under the debug build independently of this change).Background
Blobin bun is a small view (offset,size, metadata) onto aStore, the refcounted owner of the bytes (or of the file path / S3 key forBun.file()/ S3 blobs). Several Blobs can view one store;blob.dupe()makes another view and bumps the store's refcount,take_store()removes a Blob's view without touching the refcount, leaving that Blob with no store at all. A Blob without a store reads as empty but keeps its cachedsize, which is the11/""mismatch above.Blob::get::<REQUIRE_ARRAY>is the shared "blob parts to Blob" routine behindnew Blob(parts)/new File(parts, name)(REQUIRE_ARRAY = true) and behind body inits andBun.write()sources (false, a bare part is accepted as well). When the input is a single Blob part it takes a fast path that reuses the part's store instead of copying bytes; that fast path is the code changed here.new Response([...])) are a bun extension: the fetch spec would stringify the array. That extension is unchanged by this PR.Repro and outputs
Other entry points on bun 1.4.0, all fixed by this branch: