Blob: wrap blobs for JS through JsClass::to_js only

[robobun]

Problem

• Several native Blob producers hand JS a wrapper that reports almost nothing to the GC: estimateShallowMemoryUsageOf returns 48 for a new File(), a parsed formData() file entry, a WebSocket binaryType = "blob" message and a Bun.Image#blob() result, against 65856 for the same 64 KiB in new Blob(). A parsed FormData, whose memoryCost reads the same field, is undercounted with them.
• Cause: the size was computed on only one of several wrapping routes (BlobExt::to_js and the Blob constructor). A blob wrapped through the by-value JsClass::to_js (Bun.Image) or wrapped directly by C++ (File, Blob__dupe, Blob__fromBytes*) never gets the field filled in, and the GC re-reads that same field for the blob's whole life.
• Separately, BlobExt::to_js(&self) handed C++ a mutable pointer cast from a shared reference (C++ refcounts through it and frees it), and it was a safe fn that was only correct when self was exactly the Blob::new allocation. Nothing in the signature enforced that; the pattern was repeated at 16 call sites. Harmless in practice today.

Fix

• JsClass::to_js(self) becomes the only Rust-side way to wrap a Blob: it heap-promotes and passes the Blob::new pointer straight to the plain wrapper, or to the S3File wrapper when the store is S3. BlobExt::to_js and the S3-specific helpers are deleted and every former caller passes the Blob by value; structured clone deserialization also drops its two scope guards, since the blob is now an ordinary local until the final to_js.
• The size is computed inside Blob::new. Property to check: every wrapper, whether Rust or C++ creates it, adopts a pointer minted by Blob::new, so no wrapping route can skip the bookkeeping, and taking the Blob by value removes the heap-receiver precondition instead of documenting it.
• Overlaps with Report File and S3File sizes to the GC when their wrappers are created #37667, blob: report the size of Blobs created by the native bindings to the GC #37697 and Make reference-receiver to_js adoption methods unsafe, fix DevServer stack Response #31987 are spelled out in the original; this branch leaves their lines alone, and their Blob-side size calls become redundant once this lands.
• Verification: new tests run a table of in-memory producers plus Bun.file(), Bun.stdin and the three S3 constructors through estimateShallowMemoryUsageOf, and check that only S3 blobs get the S3File wrapper. Without the fix the new File(), formData(), WebSocket and Bun.Image#blob() cases fail (each reports 48); with it the file and the neighbouring suites listed in the original pass on a debug build.

Background

• A JS Blob is a Rust Blob struct behind a C++ wrapper. The wrapper adopts a raw heap pointer, bumps and drops the refcount through it, and frees it from its finalizer, so wrapping a blob gives the allocation away. S3-backed blobs get a wrapper subclass whose prototype adds presign, stat and bucket.
• Blob::new is heap promotion: it boxes a by-value Blob, sets the refcount to 1 and returns the pointer a wrapper adopts. Before that a Blob is an ordinary value. Some producers (the JS constructors, Blob__dupe for FormData entries and MessageEvent, Blob__fromBytes* for WebSocket messages) return that pointer for C++ to wrap and never go through a Rust to_js.
• reported_estimated_size is a cached in-memory footprint (held bytes plus store, content type and name; not size on disk). JSC reads it when the wrapper is allocated and again from visitChildren on every GC; it is cached because GC marking threads read it. estimateShallowMemoryUsageOf from bun:jsc returns the same number, which is how the tests observe it.
• JsClass is the bun_jsc trait that gives a Rust type its from_js / to_js; Request already wraps itself this way. BlobExt is an extension trait in the runtime crate that layered more Blob methods, including the second to_js, on top of it.

---

no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/blob.test.ts

Original description

Symptom

Several native Blob producers hand JS a wrapper that reports no memory to the GC:

import { estimateShallowMemoryUsageOf } from "bun:jsc";
const bytes = new Uint8Array(64 * 1024);
estimateShallowMemoryUsageOf(new Blob([bytes]));                                      // 65856
estimateShallowMemoryUsageOf(await new Bun.Image(someBmp).png().blob());              // 48 (for a 14585 byte blob)
estimateShallowMemoryUsageOf(new File([bytes], "a.bin"));                             // 48
estimateShallowMemoryUsageOf((await new Response(formWithFile).formData()).get("f")); // 48
// a WebSocket binaryType = "blob" message: 48 as well

JSBlob reports Blob__estimatedSize(ptr) (the cached reported_estimated_size field) to JSC when the wrapper is created and again from visitChildren on every GC. If nothing computed the field before the blob was wrapped, those blobs, and containers such as a parsed FormData whose memoryCost reads the same number, stay invisible to the collector's memory accounting for their whole life.

Cause

Blob had two Rust-side ways of becoming a JS object, and the size bookkeeping was attached to one of them:

• BlobExt::to_js(&self) (src/runtime/webcore/Blob.rs) computed the estimated size and picked the JSS3File wrapper for S3 stores, but built the pointer the wrapper adopts with ptr::from_ref(self).cast_mut(). That pointer is derived from a shared reborrow, yet it is what C++ later writes the refcount through (Blob__ref/Blob__deref; ref_count is a plain u32) and what the finalizer frees; under the aliasing model those are writes and a deallocation through a read-only pointer. It was also a safe fn whose correctness depended on self being exactly the Blob::new allocation, which nothing in the signature enforced (the shape Make reference-receiver to_js adoption methods unsafe, fix DevServer stack Response #31987 found crashing for a stack Response). Harmless in practice today, but a contract the compiler could not see, repeated at 16 call sites as Blob::new(x) + unsafe { to_js(&*ptr) }, and the comment claiming the cast kept the Blob::new provenance was wrong.
• JsClass::to_js(self) (src/jsc/webcore_types.rs) took the Blob by value and heap-promoted it, but skipped the bookkeeping and the S3 routing. Image.rs used it precisely to avoid the heap-receiver precondition of the other one, which is the Bun.Image symptom.
• The C++-wrapped producers went through neither: the File constructor, Blob__dupe (how FormData entries and MessageEvent blobs are wrapped) and Blob__fromBytes* (WebSocket messages, webview screenshots) return a Blob::new pointer that C++ wraps directly, and the only explicit size computations were in BlobExt::to_js and the Blob constructor.

Fix

1. JsClass::to_js becomes the only Rust-side wrapping path, the arrangement Request already uses for its JsClass impl: it heap-promotes and passes the Blob::new pointer straight to Blob__create or BUN__createJSS3FileUnsafely (whose extern moves next to its only caller). BlobExt::to_js, s3_file::to_js_unchecked and construct_s3_file_internal are deleted; every former caller (Blob.rs, Body.rs, Archive.rs, BunObject.rs, ObjectURLRegistry.rs, S3Client.rs, S3File.rs, Image.rs) passes the Blob by value, which removes the Blob::new + raw reborrow pair at each site. Structured clone deserialization no longer needs its two scope guards: the blob is a local until the final to_js, so an early ? drops it like any other value (Blob has no Drop impl; its store, content type and name release through their field drops, which is what deinit() did for a non-heap blob).
2. The size is computed in Blob::new itself (calculate_estimated_byte_size / estimated_size move to webcore_types.rs as inherent methods). Every wrapper, Rust- or C++-created, adopts a pointer minted there, so this covers the C++-wrapped producers as well, and the explicit calls in to_js and in the Blob constructor go away. Two open PRs change the neighbouring lines and are complementary to this one rather than conflicting with it, so this branch leaves those lines alone: Report File and S3File sizes to the GC when their wrappers are created #37667 makes JSDOMFile.cpp / JSS3File.cpp report the size when the wrapper is allocated (today the S3 wrapper only re-reports from visitChildren, so it never feeds the allocation budget), and blob: report the size of Blobs created by the native bindings to the GC #37697 reorders the Blob__fromBytes* exports so the content type is set before promotion. Once this lands, the explicit calculate_estimated_byte_size() calls those PRs add become redundant and can be removed (the constructor line, which blob: report the size of Blobs created by the native bindings to the GC #37697 also edits, is the one place the diffs touch the same lines); the method stays pub for them and for types that embed a Blob by value (Report BuildArtifact bytes to the GC and size FileRoute blobs in the constructor #37708).

Why this shape rather than marking the old function unsafe and adding the missing calls producer by producer: the precondition only existed because the function took a reference to an allocation it was about to give away, and the gaps only existed because the bookkeeping was attached to one of several wrapping routes. Taking the value removes the precondition instead of documenting it, and computing at heap promotion leaves no route around the bookkeeping, which is also why the producer-by-producer fixes in #37667 and #37697 reduce to their create-time reporting, field ordering and tests once this is in. #31987 (open) marks the old &self method unsafe; its Blob hunks are superseded by this change, the rest of that PR is unaffected.

Verification

New tests at the end of test/js/web/fetch/blob.test.ts: a table of in-memory Blob producers (new Blob, new File, slice(), structuredClone, resolveObjectURL, ReadableStream#blob() on a blob stream, buffered and streamed Response#blob(), Request#blob(), a natively parsed formData() entry, a WebSocket binaryType = "blob" message, the three Bun.Archive outputs, Bun.Image#blob()) asserting estimateShallowMemoryUsageOf(blob) >= blob.size on a 64 KiB incompressible payload; a table of store-backed producers (Bun.file(), Bun.stdin, the three S3 constructors) asserting the wrapper reports more than an empty in-memory blob does; and a check that exactly the S3 producers get the S3File wrapper. Without the fix the new File(), formData(), WebSocket and Bun.Image#blob() cases fail (each receives 48); the rest cover the call sites the change converts (every JS-reachable one: the bytes-to-bytes Bun.write branch is rejected earlier by argument validation, Bun.embeddedFiles is exercised by test/regression/issue/31575.test.ts, and the webview screenshot exports have no test here). Whole file: 96 pass on the fixed debug build (rebased on main, which added its own tests to the file in the meantime). Cross-check against the neighbouring PRs, run by applying only their test hunks to this branch: the four tests #37697 adds (parsed multipart files, a parsed file re-appended to another FormData, a WebSocket blob message, including the FormData and MessageEvent memory costs) pass against this branch as is, and #37667's File estimate test passes too, while its two allocation-time tests fail here exactly as they do on main, since reporting at allocation for the two wrappers C++ creates directly is that PR's own change.

Also run on the fixed debug build: test/js/web/fetch/{blob,body,body-clone,blob-file-name-ownership}.test.ts, test/js/web/html/FormData*.test.ts, test/js/web/structured-clone-blob-file.test.ts, test/js/node/buffer-resolveObjectURL.test.ts, test/js/web/websocket/websocket-client-short-read.test.ts, test/js/bun/archive.test.ts, test/js/bun/util/bun-stdin-slice.test.ts, test/js/web/workers/worker_blob.test.ts, test/regression/issue/31575.test.ts, the FormData/Request/Response/WebSocket cases of test/js/bun/util/heap-snapshot.test.ts, the local parts of test/js/bun/s3/s3.test.ts and the image .blob() test. cargo check for bun_bin on linux-x64 plus bun_runtime on x86_64-pc-windows-msvc (the jsc_abi_extern! sysv64 branch) and aarch64-apple-darwin, cargo clippy and cargo fmt --check on bun_jsc and bun_runtime, and test/internal/source-lints are clean.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7fe60607-4852-459a-bc8f-6b224423f518

📥 Commits

Reviewing files that changed from the base of the PR and between 6172d63 and 79dc483.

📒 Files selected for processing (10)

• src/jsc/webcore_types.rs
• src/runtime/api/Archive.rs
• src/runtime/api/BunObject.rs
• src/runtime/image/Image.rs
• src/runtime/webcore/Blob.rs
• src/runtime/webcore/Body.rs
• src/runtime/webcore/ObjectURLRegistry.rs
• src/runtime/webcore/S3Client.rs
• src/runtime/webcore/S3File.rs
• test/js/web/fetch/blob.test.ts

---

Walkthrough

Blob JavaScript conversion now uses owned Blob values, calculates estimated memory, and selects JSS3File for S3-backed blobs. Native producers and structured-clone paths remove intermediate heap allocations and unsafe pointer conversions. Tests cover memory accounting and S3 wrapper behavior.

Changes

Blob bridging and producer integration

Layer / File(s)	Summary
Blob JavaScript bridge and memory accounting src/jsc/webcore_types.rs	Blob::to_js now calculates estimated memory and selects JSBlob or JSS3File. Blob exposes cached size estimation methods.
Value-based Blob conversion src/runtime/webcore/Blob.rs	Slices, structured-clone deserialization, Bun files, byte writes, and action values now convert owned Blob values directly. Obsolete BlobExt methods and exports were removed.
Native producer integration src/runtime/api/Archive.rs, src/runtime/api/BunObject.rs, src/runtime/image/Image.rs, src/runtime/webcore/Body.rs, src/runtime/webcore/ObjectURLRegistry.rs, src/runtime/webcore/S3Client.rs, src/runtime/webcore/S3File.rs	Native Blob producers now use direct to_js conversion without intermediate heap allocation or unsafe pointer dereferencing.
Blob memory and wrapper validation test/js/web/fetch/blob.test.ts	Tests cover memory accounting across Blob producers and verify S3-only wrapper behavior.

Possibly related PRs

• oven-sh/bun#37625: Updates related Blob bridging references and S3 client comments.

Suggested reviewers: jarred-sumner, alii, dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The description identifies related issues and explains how this change overlaps with or supersedes their Blob-related work.
Out of Scope Changes check	✅ Passed	The implementation, cleanup, ownership fixes, and tests support the stated Blob wrapping and GC accounting objectives.
Title check	✅ Passed	The title clearly and concisely describes the primary change to use JsClass::to_js for Blob wrapping.
Description check	✅ Passed	The description explains the problem, implementation, verification, and test results, although it uses different headings from the template.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 11:05 PM PT - Aug 12th, 2026}

❌ @robobun, your commit f6451bd has some failures in Build #93851 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 37656

That installs a local version of the PR into your bun-37656 executable, so you can run:

bun-37656 --bun

[robobun]

Status: ready for review (rebased on main as a single commit, 2ef4ebd; description is current).

Reproduced on the released bun and on an unfixed debug build of main: estimateShallowMemoryUsageOf returns 48 (the bare JS cell) for blobs from Bun.Image#blob(), new File(), a natively parsed formData() entry and a WebSocket binaryType = "blob" message, while every other producer returns size plus a few hundred bytes. Those four cases in the tables at the end of test/js/web/fetch/blob.test.ts fail before this change and pass after it; the remaining cases lock every JS-reachable producer the change touches.

Scope: the Rust-side wrapping path (JsClass::to_js only) plus computing the size in Blob::new. The neighbouring lines belong to #37667 (allocation-time reporting for File / S3File wrappers) and #37697 (Blob__from* field ordering); this branch leaves them untouched so the three apply in any order (cross-check in the comments above). Automated review found no defects; its three nits are addressed and resolved. CI: build 93003 on the pre-rebase code passed all 190 jobs, including the macOS lanes. On the rebase (2ef4ebd, unchanged since), build 93585 passed 177 of 181 jobs; its two failing lanes both hit test/cli/install/migration/complex-workspace.test.ts, an install test that clones from bitbucket.org / gitlab.com (reported separately, unrelated to this diff), and the re-run, build 93851, finished with 179 of 181 jobs passed and no failing tests (those two lanes included, and the darwin 14 lanes once they got an agent); the remaining two jobs are the darwin 26 test lanes, which CI canceled in both rebase builds rather than ran, so the build is marked failed for CI capacity reasons only. The code is ready for a maintainer.

[robobun]

Note for the record: the FilesContext::run_from_js hunk in src/runtime/api/Archive.rs also closes a small error-path leak on main. Today the loop heap-promotes with Blob::new first and converts the name afterwards:

let blob_ptr = Blob::new(Blob::create_with_bytes_and_allocator(data, global, false));
...
let name_js = blob.name.get().to_js(global)?;   // returns before the wrapper adopts blob_ptr
let blob_js = blob.to_js(global);

bun_core::String::to_js throws ERR_STRING_TOO_LONG when clone_utf8 handed back a dead string (allocation failure or an over-long entry name), and the ? returns before blob.to_js hands the allocation to the JS wrapper, so that one entry's Blob and its store are never freed. With the blob kept by value until the final to_js, as in this PR, the early return drops it like any other local. Only reachable on OOM-class failures, so no separate test; mentioning it here so the Archive change is not read as a pure mechanical conversion and so nobody opens a second PR for the same lines.

[claude]

I reviewed this PR and didn't find any bugs. Because it's a substantial refactor of Blob's JS-wrapper ownership path across 10 files — moving code between crate layers, changing an FFI extern's home, and removing scope guards in structured-clone deserialization on the strength of field-drop equivalence — a human look at the architectural shape and memory-safety reasoning would still be worthwhile.

What was reviewed:

• Verified no remaining callers of the deleted BlobExt::to_js / s3_file::to_js_unchecked / construct_s3_file_internal / blob::js re-export.
• Confirmed JsClass is in scope at every converted call site (including Image.rs, which dropped the UFCS form).
• Checked the scope-guard removal in on_structured_clone_deserialize: Blob has no Drop impl and its store (StoreRef::drop), name (OwnedStringCell::drop), and content_type (Arc) fields release on plain drop, matching what deinit() did for a stack blob.
• Confirmed the moved calculate_estimated_byte_size preserves the original arithmetic (owned-content-type branch is equivalent to the old * is_owned as usize).

Extended reasoning...

Overview

This PR consolidates two divergent paths for wrapping a Rust Blob as a JS object into a single JsClass::to_js(self) implementation. The old BlobExt::to_js(&self) computed GC size and routed S3 blobs correctly but required an unsound-in-principle &self → *mut cast and an unenforced heap-allocation precondition; the old JsClass::to_js was safe but skipped both size bookkeeping and S3 routing (the observable bug: Bun.Image#blob() reported ~48 bytes to the GC regardless of payload). The fix moves calculate_estimated_byte_size/estimated_size to inherent methods on Blob in bun_jsc::webcore_types, has JsClass::to_js do the size computation and S3 dispatch, relocates the BUN__createJSS3FileUnsafely extern next to its sole caller, and converts ~16 call sites across 8 files to pass Blob by value. Structured-clone deserialization drops two scopeguards since a by-value local Blob releases its store/name/content-type via field drops on early ?.

Security risks

None identified. This is internal ownership/lifetime plumbing; no user-input parsing, auth, or crypto surface changes. The FFI extern moved crates but its call is now unsafe with a SAFETY comment (previously safe fn with the contract on the wrapper), which is a tightening.

Level of scrutiny

High. This is memory-safety-adjacent native code: Blob heap-promotion, FFI ownership transfer to a C++ wrapper that later writes the refcount and frees the allocation, and GC extra-memory reporting. The change is a net simplification and removes a genuinely questionable provenance cast, but it also moves code between crate layers (bun_runtime → bun_jsc) and relies on field-drop equivalence to deinit() for stack blobs — both are the kind of reasoning a maintainer should confirm.

Other factors

• Test coverage is solid: a table-driven test covers 11 Blob producers plus 3 S3 constructors, asserting both the GC size bookkeeping and the S3File wrapper selection. The PR description documents USE_SYSTEM_BUN=1 failure for the Image case.
• The PR notes it supersedes the Blob hunks of open PR #31987 — coordination with that PR's author is a human concern.
• I grep-verified no remaining references to the deleted symbols and that JsClass is in scope at every converted .to_js() site.
• The estimated_size codegen hook (generate-classes.ts:2250 emits ${T}::estimated_size(this)) now resolves to the new inherent method rather than the removed trait method; this matches the pattern the PR cites for Request.

Given the scope (10 files, architectural consolidation, cross-crate code movement in GC/FFI-sensitive territory), this exceeds the bar for auto-approval even though no defects were found.

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks Blob ownership handoff to the C++ wrapper across ~10 files, moves calculate_estimated_byte_size and the BUN__createJSS3FileUnsafely extern down a crate tier, and partially supersedes #31987, a human sign-off on the layering and scope would still be worthwhile.

What was checked:

• No remaining references to the deleted BlobExt::to_js / s3_file::to_js_unchecked / construct_s3_file_internal / blob::js re-export.
• Blob has no Drop impl; StoreRef, OwnedStringCell, and BlobContentType each release on field drop, so the removed scopeguards in on_structured_clone_deserialize are covered by plain drops.
• The one remaining calculate_estimated_byte_size caller in Blob.rs now resolves to the new inherent method; the codegen Blob::estimated_size hook likewise resolves inherent-first over the JsClass default.

Extended reasoning...

Overview

This PR consolidates two divergent Blob → JSValue paths into the single JsClass::to_js(self), fixing a real bug where Bun.Image#blob() handed JS a wrapper that reported ~48 bytes to the GC instead of the blob's actual size. The change moves calculate_estimated_byte_size/estimated_size to inherent methods on Blob in bun_jsc::webcore_types, moves the BUN__createJSS3FileUnsafely extern next to its only caller, deletes BlobExt::to_js / s3_file::to_js_unchecked / construct_s3_file_internal, and converts ~16 call sites from Blob::new(x) + unsafe { (*ptr).to_js() } to by-value x.to_js(global). It touches 9 source files plus a comprehensive test table covering every converted producer.

Security risks

None identified. This is internal ownership/refcounting plumbing with no user-input parsing or auth surface. The change actually tightens the aliasing story (removes ptr::from_ref().cast_mut() on a shared borrow that C++ later writes through) and removes ~16 unsafe blocks.

Level of scrutiny

High. Blob ownership handoff across the Rust↔C++ FFI boundary and JSC GC memory accounting are correctness-critical: a mistake here is a use-after-free or a leak on every Blob creation. The structured-clone deserialization change removes two scopeguards on the argument that Blob's field drops are equivalent to deinit() for a non-heap blob — I verified this holds (Blob has no Drop; StoreRef::drop calls Store::deref, OwnedStringCell::drop derefs the name, BlobContentType::Owned(Arc) drops naturally), but it's the kind of invariant a maintainer should confirm. The crate-layering move follows the Request precedent and only reads types already defined at that tier, so there's no upward dependency.

Other factors

The PR description is unusually thorough (mechanism, verification across three targets, interaction with open PR #31987), the bug hunter found nothing, and my grep sweep confirms no dangling references to any deleted item. Test coverage is strong — a test.each-style table asserts estimateShallowMemoryUsageOf(blob) >= blob.size for every converted producer plus S3-wrapper routing. The reasons to defer rather than approve are scope (10 files, FFI ownership), the layering decision (moving code from bun_runtime to bun_jsc), and the stated overlap with #31987, all of which merit a maintainer's eye rather than bot approval.

[robobun]

Changes in cf65644, after a self-review of the first two commits:

• The size bookkeeping moved from JsClass::to_js into Blob::new. to_js is only the Rust producers' funnel; the File constructor, Blob__dupe (FormData entries, MessageEvent blobs) and Blob__fromBytes* (WebSocket messages, webview screenshots) hand C++ a Blob::new pointer to wrap directly and were reporting 0 in exactly the same way Bun.Image#blob() was. Every wrapper adopts a pointer minted by Blob::new, so computing there covers all of them and lets the explicit calls in to_js and the Blob constructor go. new File(), a parsed formData() entry and a WebSocket blob message are now in the test table and fail without the fix like the image case does. (This supersedes the two follow-ups the previous description mentioned.)
• JSS3File::create now calls reportExtraMemoryAllocated like the generated Blob__create does; before, S3 wrappers only re-reported from visitChildren, so they never fed the allocation budget. The doc comment on to_js had claimed parity that did not exist.
• The doc comment on Blob::estimated_size described a resolution fallback that cannot happen (the generated thunk never sees JsClass); it now just says what reads the value.

Description updated to match.

[claude]

I reviewed this PR and didn't find any bugs; the earlier nit on the WebSocket test's onclose wiring is addressed in 4379276. Because this restructures Blob's ownership/heap-promotion path across the Rust↔C++ FFI boundary and touches GC memory accounting in ~8 call sites, a human look would still be worthwhile.

Checked: the by-value to_js funnel routes S3 blobs to JSS3File and computes the estimated size in Blob::new so C++-wrapped producers (Blob__dupe, Blob__fromBytes*, the File constructor) are covered; the scopeguard removal in structured-clone deserialization is sound because a stack Blob releases its store/content-type/name via field Drops (StoreRef, Arc, OwnedStringCell); jsdom_file_construct and the Blob__fromBytes* exports now set their fields before Blob::new so those fields are counted; the BUN__createJSS3FileUnsafely extern is now correctly unsafe (was safe fn before) and its sole caller passes the fresh Blob::new allocation.

Extended reasoning...

Overview

This PR consolidates all Rust-side Blob→JS wrapping through JsClass::to_js(self) (by value), deleting the parallel BlobExt::to_js(&self) path and the s3_file::to_js_unchecked / construct_s3_file_internal helpers. It moves calculate_estimated_byte_size/estimated_size from BlobExt to inherent methods on Blob in webcore_types.rs, computes the estimated size inside Blob::new so every wrapper (Rust- or C++-created) reports it, and adds reportExtraMemoryAllocated to JSS3File::create. Eight call sites across Blob.rs, Body.rs, Archive.rs, BunObject.rs, ObjectURLRegistry.rs, S3Client.rs, S3File.rs and Image.rs are converted from Blob::new(x) + unsafe { (*ptr).to_js() } to passing the Blob by value. Structured-clone deserialization drops its two scopeguards in favor of ordinary local drop. Tests add ~130 lines covering every JS-reachable producer.

Security risks

None identified. The change is internal to GC memory accounting and Blob ownership; no user-facing input validation, auth, or crypto paths are touched. The BUN__createJSS3FileUnsafely extern moves from safe fn to unsafe fn, which tightens rather than loosens the contract.

Level of scrutiny

High. This is native memory-safety code — heap promotion, refcounting, GC extra-memory reporting, and FFI ownership transfer — which REVIEW.md calls out as the most-blocked category. The change removes ~16 unsafe { &*ptr } reborrows and two scopeguards based on the reasoning that field Drops cover what deinit() did for a non-heap blob; that reasoning checks out (StoreRef::drop derefs the store, BlobContentType::Owned drops its Arc, OwnedStringCell releases the name), but it is exactly the kind of ownership-model change a maintainer should confirm. The PR also interacts with the open #31987 (which it partially supersedes).

Other factors

The PR description is unusually thorough (mechanism, verification across platforms, explicit list of test files run), the bug-hunting pass found no defects, and the one prior nit (WebSocket test onclose wiring) was addressed in the latest commit. Test coverage is comprehensive: a table of 15 in-memory producers, 5 store-backed producers, and an S3-wrapper-routing check. CI on the earlier commits was green per the author's notes. Nonetheless the breadth (11 files, cross-cutting refactor of a core type's JS wrapping) and the memory-safety surface put this outside what I'd approve without a human reviewer.

[robobun]

0256787 narrows the previous push. While writing it up I found that two PRs opened in the meantime already own the lines cf65644 had also touched: #37667 (the File constructor and allocation-time reporting in JSDOMFile.cpp / JSS3File.cpp, with a test that observes the allocation budget) and #37697 (the Blob__fromBytes* exports, with container-level tests). This branch now keeps only the part that is its own, computing the size in Blob::new, and leaves those lines untouched, so all three apply in any order; calculate_estimated_byte_size stays pub for their explicit calls and for #37708. Applying just their test hunks to this branch: #37697's four tests and #37667's File test pass here already, #37667's two allocation-time tests fail here as on main (that is its C++ change). The four new table cases in blob.test.ts still fail without this branch and pass with it. Description updated.

[claude]

I reviewed the latest push (9903397) and found no issues — both earlier nits are addressed (the redundant calculate_estimated_byte_size() in the constructor is gone, and the description now correctly attributes the JSS3File.cpp / Blob__fromBytes* changes to #37667/#37697). Because this reworks the Blob→JS ownership handoff and GC size accounting across ~16 native call sites plus the FFI extern move and structured-clone scopeguard removal, a maintainer look would still be worthwhile.

What was reviewed:

• JsClass::to_js now heap-promotes and routes S3 blobs to BUN__createJSS3FileUnsafely; verified every converted call site passes a by-value Blob whose store/name/content_type drop on early return.
• Blob::new computes reported_estimated_size before any wrapper reads it; the moved calculate_estimated_byte_size body is byte-equivalent to the deleted BlobExt version modulo the content-type branch shape.
• Structured-clone deserialize: confirmed Blob has no Drop impl and its JsCell<Option<StoreRef>>/OwnedStringCell fields release on an early ?, so dropping the scopeguards is sound.
• Test table covers every JS-reachable producer the diff touches; WebSocket case wires onclose to reject before the await.

Extended reasoning...

Overview

This PR consolidates two Rust-side Blob→JS wrapping paths into one (JsClass::to_js), moves GC size bookkeeping into Blob::new so C++-wrapped producers (File constructor, Blob__dupe, Blob__fromBytes*) report their bytes too, and mechanically converts ~16 call sites from Blob::new(x) + unsafe { (*ptr).to_js() } to by-value x.to_js(). It deletes BlobExt::to_js, s3_file::to_js_unchecked, construct_s3_file_internal, the structured-clone scopeguards, and moves the BUN__createJSS3FileUnsafely extern (now unsafe) next to its sole caller. A comprehensive producer-table test in blob.test.ts locks the fix.

Security risks

None identified. The structured-clone deserializer still clamps untrusted offset and validates tags; removing the scopeguards changes cleanup mechanics (field Drop instead of explicit deinit()) but not validation. The S3 extern is now correctly unsafe (it was safe fn before despite adopting a raw pointer). No new user-input parsing.

Level of scrutiny

High. This is native code touching the exact categories REVIEW.md flags as most-blocked: pointer ownership transfer to C++ wrappers, GC memory accounting, FFI extern signatures, and error-path cleanup. The individual conversions are mechanical and the reasoning is sound, but the blast radius (every Blob producer) and the subtlety of Blob having no Drop impl while relying on field drops make this worth a maintainer's eyes rather than bot-only approval.

Other factors

• Both prior nits from this review are addressed in 9903397 and the updated description.
• The bug-hunting system found nothing this run.
• Tests are thorough (per-producer table, store-backed producers, S3-wrapper check) and the PR description documents cross-verification against #37667/#37697's test hunks.
• CI build #93003 for the current HEAD is in progress per the robobun status comment; no result posted to the thread yet.

[robobun]

Rebased on main as one commit (2ef4ebd) after #32024, #37597, #37651 and #37681 landed; no changes beyond the two conflict resolutions: S3Client::file now uses main's construct_blob helper followed by the by-value to_js, and blob.test.ts keeps both the new "Blob from ArrayBuffer-like values" block from main and the tables from this PR. Build 93003 on the pre-rebase code was green on all 190 jobs; the rebased tree passes blob.test.ts (96), body, FormData, structured-clone, archive and resolveObjectURL suites locally, and the four fail-before cases still fail on the released binary.

[claude]

Re-reviewed after 2ef4ebd — the three earlier nits (WebSocket onclose wiring, stale description, redundant constructor-side calculate_estimated_byte_size) are all addressed and this run found nothing new. Because the change reworks Blob ownership at the FFI/GC boundary and rewires ~16 call sites plus the structured-clone deserialize path, a human look is still worthwhile.

What was reviewed:

• JsClass::to_js reads is_s3() before Blob::new(self) consumes the value; S3 routing matches the deleted BlobExt::to_js.
• calculate_estimated_byte_size is a semantic move — the new if content_type.is_owned() branch is equivalent to the old * (is_owned as usize) term.
• Structured-clone deserialize dropping its scope guards: no Drop impl exists for Blob, so an early ? releases the store/name/content_type via field drops.
• The Archive files() loop now drops the by-value blob if name.to_js throws, closing the error-path leak the author noted.

Extended reasoning...

Overview

Consolidates the two Rust-side Blob→JS wrapping paths into the by-value JsClass::to_js, moves size bookkeeping into Blob::new, and deletes BlobExt::to_js / s3_file::to_js_unchecked / construct_s3_file_internal plus the JSBlob re-export. Sixteen call sites across Blob.rs, Body.rs, Archive.rs, BunObject.rs, ObjectURLRegistry.rs, S3Client.rs, S3File.rs and Image.rs switch from Blob::new(x) + unsafe { (*ptr).to_js(...) } to passing the blob by value. The BUN__createJSS3FileUnsafely extern moves into webcore_types.rs next to its only remaining caller. Tests add a producer table over estimateShallowMemoryUsageOf covering every JS-reachable wrapping route plus the S3File-prototype check.

Security risks

None identified. No parsing of untrusted input is added; the structured-clone path already clamped offset and that logic is unchanged. The S3 credential handling is untouched beyond call-site shape.

Level of scrutiny

High — this is native memory-ownership code at the JSC FFI boundary. The old BlobExt::to_js derived a *mut from &self and handed it to C++ to refcount and free; the new path takes ownership by value and heap-promotes exactly once, which is strictly safer, but every converted site had to be checked for (a) no error-path between value construction and to_js that would need explicit cleanup the old raw-pointer shape lacked, and (b) no second use of the blob after the consuming call. The structured-clone rewrite drops two scopeguards in favour of ordinary local drop, which is correct only because Blob has no Drop impl and its owning fields (StoreRef, Arc<[u8]>, OwnedStringCell) release on field drop — verified by grep.

Other factors

All three of my earlier inline nits are resolved and the threads are marked so. The PR description was rewritten to match the narrowed scope after 0256787 split out the C++-side changes to #37667/#37697. Test coverage is thorough (14 in-memory producers + 5 store-backed producers + wrapper-class check), and the author cross-checked the neighbouring PRs' test hunks against this branch. Given the breadth of native ownership changes and the interaction with three other open PRs touching adjacent lines, I'm deferring rather than approving.

[claude]

I reviewed this PR and the bug hunt found no issues; the three nits from earlier runs are all addressed. Because it reworks Blob heap-promotion and FFI ownership across ten files, a maintainer look is still worthwhile.

What was reviewed:

• Field-drop of a by-value Blob matches deinit() for non-heap blobs (StoreRef::drop, OwnedStringCell::drop, BlobContentType Arc), so the removed scopeguards in on_structured_clone_deserialize don't leak on early ?.
• Every converted call site sets content_type/name on the local before to_js, so calculate_estimated_byte_size in Blob::new counts them; the Archive#files() conversion also closes the OOM-path leak noted in the thread.
• BUN__createJSS3FileUnsafely moved to an unsafe extern with a SAFETY comment; the S3 routing in JsClass::to_js matches what the deleted BlobExt::to_js did, and the new test asserts only S3 blobs get presign.

Extended reasoning...

Overview

This PR consolidates Blob-to-JS wrapping onto a single path (JsClass::to_js(self)) and moves calculate_estimated_byte_size into Blob::new, so every wrapper — whether created from Rust or from C++ (File constructor, Blob__dupe, Blob__fromBytes*) — reports its bytes to the GC. It deletes BlobExt::to_js(&self) (which cast &self to *mut and required an undocumented heap-receiver precondition) and s3_file::to_js_unchecked, converting ~16 call sites across Archive, BunObject, Image, Blob, Body, ObjectURLRegistry, S3Client and S3File from Blob::new(x) + unsafe { (*ptr).to_js(g) } to x.to_js(g). Structured-clone deserialization drops two scopeguards because the blob is now a plain local until the final to_js. Tests add a table of 17 in-memory producers plus file/S3 producers asserting estimateShallowMemoryUsageOf(blob) >= blob.size and that only S3 blobs get the S3File wrapper.

Security risks

None identified. The change removes unsafe blocks rather than adding them; the one new unsafe (BUN__createJSS3FileUnsafely call) is narrower than the deleted safe fn extern it replaces and carries a SAFETY comment naming the ownership transfer. No user-controlled input parsing changes.

Level of scrutiny

High. This is native memory-ownership code at the Rust/C++ FFI boundary — the most-blocked category in REVIEW.md. I verified: (1) Blob has no Drop impl and its owning fields (JsCell<Option<StoreRef>>, JsCell<BlobContentType>, OwnedStringCell) each release on field drop, so dropping a by-value Blob on early return is equivalent to the removed deinit() scopeguards; (2) at every converted site the content_type/name mutations happen on the local before to_js consumes it, so the size computation in Blob::new sees them; (3) is_s3() is checked before Blob::new moves self, and the S3 branch hands the same freshly-minted pointer to BUN__createJSS3FileUnsafely that Blob__create would have received.

Other factors

CI on the pre-rebase code was green on all 190 jobs; the rebase run's only failures were an unrelated network-bound install test. All three of my earlier inline nits were addressed. The test coverage is thorough (each converted producer plus the four that fail without the fix). Still, this is a 10-file refactor of heap-promotion and GC-accounting semantics that interacts with three neighbouring open PRs (#37667, #37697, #31987), so deferring to a maintainer familiar with the Blob/JSC ownership model rather than auto-approving.

[robobun]

Heads-up from #38562, which touches the same function: calculate_estimated_byte_size now delegates to a free estimate_in_memory_size(blob, include_store) in Blob.rs, and BlobExt gains newly_allocated_size() (the generated Blob__newlyAllocatedSize thunk resolves Blob::newly_allocated_size, and the creation sites report it instead of the estimate). If the computation moves into webcore_types.rs / Blob::new as planned here, both of those need to move along with it; the rebase is otherwise mechanical.