Conversation
Blob.reported_estimated_size is a cache that Blob__estimatedSize only reads (it also runs on the GC thread). BlobExt::to_js and the Blob constructor fill it, but the exports the C++ WebCore::Blob wrapper uses to create Blobs did not, so multipart formData() entries, WebSocket binaryType = "blob" messages and the FormData / MessageEvent objects holding them reported 0 extra bytes. Blob__dupe, Blob__fromBytes, Blob__fromBytesWithType and Blob__fromMmapWithType now compute the size before heap-promoting the Blob they hand back.
WalkthroughChangesBlob memory accounting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on bun 1.4.0 and on main with the snippet in the description ( Related PRs for the other producers with the same symptom: #37667 ( CI is green on the final revision (Buildkite build 92978, ASAN lane included) and every review thread is resolved. Ready for a maintainer. |
There was a problem hiding this comment.
LGTM — focused GC accounting fix that fills reported_estimated_size at the four C++ binding entry points, with matching tests.
What was reviewed:
new_for_bindingscomputes the size on the stackBlobbeforeBlob::newheap-promotes it; theCellvalue moves with the struct, soBlob__estimatedSizeandreportExtraMemoryAllocatedsee it.- Content type is now set before size computation in the
*WithTypepaths, so it's included in the estimate; the two duplicated stamping blocks collapse intoset_content_type_from_cstrand the raw*blobderefs are gone. Blob__fromBytesempty/null-ptr and non-unixBlob__fromMmapWithTypefallback paths preserved.- Tests wire
onerror/oncloseto reject, useport: 0andusing, and bound the estimate at 1×–2× rather than an exact value.
Extended reasoning...
Overview
The PR touches two files: src/runtime/webcore/Blob.rs (four extern "C" exports refactored to route through a new new_for_bindings helper that calls calculate_estimated_byte_size() before Blob::new; two new private helpers blob_from_bytes and set_content_type_from_cstr extracted from duplicated inline code; a stale doc comment on Blob__fromBytesWithType corrected) and test/js/bun/util/heap-snapshot.test.ts (a new nested describe with four tests covering multipart Request/Response parsing, re-appending a parsed File, and WebSocket binaryType = "blob").
Security risks
None. The only observable change is the value written into reported_estimated_size, which JSC reads for GC scheduling, visitChildren, and heap-snapshot reporting. No user-facing API surface, parsing, or validation changes. The refactor actually reduces unsafe surface: the previous code heap-promoted first and then dereferenced the raw *mut Blob to set the content type; the new code sets it on the stack value via &Blob + Cell::set before promotion.
Level of scrutiny
Low-to-medium. calculate_estimated_byte_size is an existing method that just sums a handful of field lengths and writes a Cell<usize>; calling it on a freshly built, exclusively-owned stack Blob on the JS thread is safe. The refactored exports are behaviorally identical apart from that one extra call — I compared each old body against the new helper composition (empty/null-ptr short-circuit in blob_from_bytes, the #[cfg(not(unix))] fallback in Blob__fromMmapWithType, and the BlobContentType::Owned(mime_slice.into()) stamping) and found no dropped side effects. The reordering that sets content type before size computation only makes the estimate more accurate.
Other factors
The PR description enumerates every consumer of these exports (WebCore::Blob toJS, FormData multipart entries, Blob__dupeFromJS, WebSocket blob mode, webview screenshots) and explains why sibling paths (new File, Bun.Image#blob, BuildArtifact) are handled in separate PRs — the scope boundary is deliberate and doesn't overlap. Tests follow harness conventions (bounded assertions, port: 0, using disposal, failure events wired to reject, try/finally cleanup) and the author confirmed they fail under USE_SYSTEM_BUN=1 and pass on the debug build alongside the existing Blob/FormData/WebSocket suites. The bug hunting system found nothing.
|
Updated 1:00 AM PT - Aug 12th, 2026
✅ @robobun, your commit 90b67e050ad03d6b3c935e35e904a3d38c64b2fb passed in 🧪 To try this PR locally: bunx bun-pr 37697That installs a local version of the PR into your bun-37697 --bun |
…eenshot Blobs report their size
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 4257-4261: Update the Blob creation flow around new_for_bindings
and the toJSNewlyCreated path so the file configuration via Blob__setAsFile
occurs before Blob__create, or recalculate the estimated byte size after
stored_name is assigned. Add coverage using a file name whose length makes the
estimate change, and verify the returned wrapper reports the refreshed estimate.
🪄 Autofix
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: 098b7524-fa30-4f06-bc40-5d55e60702fa
📒 Files selected for processing (4)
src/runtime/webcore/Blob.rstest/js/bun/util/heap-snapshot.test.tstest/js/bun/webview/webview-chrome.test.tstest/js/bun/webview/webview.test.ts
…lone Blob::new computing the size already covers those producers; #37667 (create-time reporting for File and S3File wrappers) and #37697 (content type ordering in the Blob__from* exports) change those same lines and apply independently of this branch, so this branch does not touch them. calculate_estimated_byte_size stays pub for types that embed a Blob by value and report it themselves.
|
Heads-up on overlap with #37656: its latest push computes |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; the earlier doc-comment nit I raised was addressed in 90b67e0. Because it refactors the unsafe FFI Blob constructors and is one of three coordinated PRs with a planned follow-up consolidation, a human look would still be worthwhile.
What was reviewed:
new_for_bindingswritesreported_estimated_sizeon a stack-local Blob on the JS thread beforeBlob::newheap-promotes it — no marking-thread race.blob_from_bytes/set_content_type_from_cstrare byte-for-byte extractions of the previous inline code; content type is now set before the size is cached, so it's included.- The unresolved CodeRabbit note on
toJSNewlyCreatedordering is about pre-existingblob.cppcode and only affects a few bytes of filename, not the payload accounting this PR fixes. - New tests cover every touched export (Request/Response
.formData(), re-appended File, WebSocket blob, both webview screenshot backends) and wire error/close to reject.
Extended reasoning...
Overview
The PR fixes GC memory accounting for Blobs created by the native C++ bindings rather than by new Blob(). Four extern "C" exports in src/runtime/webcore/Blob.rs (Blob__dupe, Blob__fromBytes, Blob__fromBytesWithType, Blob__fromMmapWithType) plus the JS Blob constructor are routed through a new two-line helper new_for_bindings that calls calculate_estimated_byte_size() before Blob::new(). Two copies of the content-type-stamping block are deduplicated into set_content_type_from_cstr, and the byte-copying prelude of the two fromBytes* variants into blob_from_bytes. Four new test cases in heap-snapshot.test.ts and one assertion each in the two webview screenshot tests cover every touched export.
Security risks
None. No new unsafe operations are introduced — the extracted helpers carry the exact safety contracts of the code they replace, and the raw-pointer dereferences that were previously done through (*blob) on a fresh heap allocation now happen on a stack &Blob before heap promotion, which is strictly safer. The value written is only read by JSC for GC scheduling and heap-snapshot reporting; it does not affect user-visible behaviour.
Level of scrutiny
Moderate. The change is mechanically simple (compute a cached size before heap-promoting; dedupe two identical blocks) and cannot regress runtime behaviour — the only observable effect is that estimateShallowMemoryUsageOf and JSC's extra-memory accounting now report the payload bytes instead of 0. The write to reported_estimated_size happens on the JS thread on an exclusively-owned stack value before Blob::new publishes it, so there is no interaction with the GC marking thread that reads the cache. That said, the touched functions are the FFI boundary between the C++ WebCore::Blob wrapper (used by FormData, WebSocket, MessageEvent, WebView) and the Rust Blob, and the PR is one of three coordinated changes (#37656, #37667) with a stated follow-up to move the computation into Blob::new itself — a maintainer should confirm the staging is what they want.
Other factors
- My prior inline comment (drop the stale
'staticfromBlob__fromMmapWithType's safety doc) was applied in 90b67e0, and the comment-cop threads were resolved by shortening thenew_for_bindingsdoc to one line. - The first CI build (#92698) was green including ASAN; the pushes since then only touched doc comments and added the screenshot assertions.
- CodeRabbit left an unresolved note that
toJSNewlyCreatedinblob.cppcallsBlob__createbeforeBlob__setAsFile, so the filename bytes aren't in the cached estimate on that path. That ordering is pre-existing C++ this PR doesn't touch, the underlying Rust Blob on that path now has its payload counted (viaBlob__fromBytes→new_for_bindings), and the missed filename is a handful of bytes — the PR description already names moving the computation intoBlob::newas the follow-up that closes this. I don't consider it a blocker but flagging it as the one open thread. - Test quality is good:
port: 0,usingfor the server, error/close events wired to reject the awaited promise, bounds asserted as[1x, 2x)of a 1 MiB payload so they can't pass vacuously.
|
One correction to the above: review on #37656 pointed out the constructor's own |
|
Heads-up from #38562: |
Problem
RequestorResponsebody (and theFormDataholding it, 81), a WebSocket message received withbinaryType = "blob"(and itsMessageEvent, 56), andBun.WebView#screenshot().new Blob()of the same 1 MiB reports 1048896. Reproduces on 1.4.0 and main.Blobconstructor and the ordinary Rust to JS conversion fill in. The native entry points hand the Blob to JSC with the cache still 0, and duplicating a Blob copies its source's 0.estimateShallowMemoryUsageOfshow.Fix
Blobconstructor go through one helper that computes the estimate and then moves the Blob to the heap. The typed variants set the content type before the size is taken, so it is counted.FormData,MessageEvent) reads that same allocation. The value only goes to JSC, and it now matchesnew Blob(sameBytes)plus the stored name and content type.new File(...),Bun.Image#blob(), server and bundler accounting) are left to Report File and S3File sizes to the GC when their wrappers are created #37667, Blob: wrap blobs for JS through JsClass::to_js only #37656 and a separate issue. Moving the computation intoBlob::newitself is a follow-up once Blob: wrap blobs for JS through JsClass::to_js only #37656 lands.Background
Blob::newmoves it to a heap allocation whose pointer the C++ bindings wrap in a JS object. The cache has to be filled before that move and after every field it counts (name, content type) is set.extern "C"exports rather than the JS constructor, so a fix in the constructor never reaches them. Dupe copies an existing Blob, cache included.estimateShallowMemoryUsageOffrombun:jscreturns the size JSC currently holds for one object, which is how the tests observe the number.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/webview/webview-chrome.test.ts test/js/bun/webview/webview.test.ts
Original description
Repro
Reproduces on bun 1.4.0 and on main (
Request#formData()behaves the same asResponse#formData()).Cause
Blob.reported_estimated_sizeis a cache. The generatedBlob__estimatedSize(whatBlob__create'sreportExtraMemoryAllocated,JSBlob::visitChildren,JSBlob::memoryCostand thereforeWebCore::Blob::memoryCost()read) only returns it, because it is also called from the GC marking thread. Whoever creates a Blob that JSC will account for has to fill it first;BlobExt::to_jsand theBlobconstructor do.The Blobs above never pass through either of those. They are created by the exports behind the C++
WebCore::Blobwrapper (src/jsc/bindings/blob.h) and the webview backends, and none of them computed the size:Blob__dupe: everyWebCore::Blobto JS conversion (src/jsc/bindings/blob.cpp:Blob__setAsFile, thenBlob__dupe, thenBlob__create), every multipart file entry (FormData.rsbuilds a stack Blob andappendBlobdupes it) and everyformData.append(name, blob)from JS (Blob__dupeFromJS).dupe_with_content_typecopies the source's cached value, which for a multipart entry is the never-computed 0, so the 0 was copied into the FormData entry and again into the JS wrapper.Blob__createthen reported 0 bytes at allocation and the marking thread reported 0 at every GC.Blob__fromBytes: WebSocket binary messages in blob mode (WebSocket.cpp). The JS-visible Blob is aBlob__dupeof this one, andMessageEvent::memoryCost()reads this one directly.Blob__fromBytesWithType/Blob__fromMmapWithType:Bun.WebView#screenshot()on the Chrome and WebKit backends, which hand the pointer straight toBlob__create.Fix
The four exports, and the
Blobconstructor (which already did the same two steps inline), go through one helper,new_for_bindings, which callscalculate_estimated_byte_size()and thenBlob::new.Blob__fromBytes/Blob__fromBytesWithType/Blob__fromMmapWithTypeare restructured so the content type is set on the stack Blob before the size is taken; the two copies of the content type stamping becomeset_content_type_from_cstr. The'staticrequirement onmimein the two*WithTypedoc comments is dropped, since the string has been copied into an ownedcontent_typesince #29910.Why this is the right place: these exports are exactly the points where a Blob is handed to a consumer that reads the cache without going through
BlobExt::to_js, and the freshly built Blob is exclusively owned and on the JS thread there, so computing is safe and covers every consumer of that allocation at once (the JS wrapper,DOMFormData::memoryCost,MessageEvent::memoryCost). Computing inBlob__duperather than at thetoJScall site also means the estimate includes the file name thatBlob__setAsFilestores just before it, and it fixes the JSappend()path for a source whose own cache was never filled (a parsed entry appended to another FormData). The computation is a handful of field reads, negligible next to the heap allocation and wrapper creation in the same call. Behaviour is otherwise unchanged: the value is only reported to JSC, and it now reports whatnew Blob(sameBytes)reports (plus the stored name / content type). This does not produce unbounded retention on its own, since Bun's periodic event loop GC still collects these Blobs eventually; what was wrong is the accounting JSC uses to schedule collections and what heap snapshots andestimateShallowMemoryUsageOfshow.Sibling paths with the same symptom, intentionally not in this PR:
new File(...)(jsdom_file_construct+JSDOMFile.cpp): Report File and S3File sizes to the GC when their wrappers are created #37667.Bun.Image#blob(), the one Rust producer that used the by-valueJsClass::to_js(which also skips the computation): Blob: wrap blobs for JS through JsClass::to_js only #37656, which makes that path compute the size and removes the split. This PR does not touch the functions Blob: wrap blobs for JS through JsClass::to_js only #37656 or Report File and S3File sizes to the GC when their wrappers are created #37667 change, andnew_for_bindingskeeps working ifcalculate_estimated_byte_sizebecomes an inherent method there.BuildArtifactinline Blob andFileRoute::memory_cost()read the same cache for a Blob that is never computed; that is server / bundler accounting rather than a JS Blob and has been filed separately.Where this should end up:
Blob::newis the one heap-promotion point every reader of the cache goes through, so once #37656 has movedcalculate_estimated_byte_sizeintobun_jsc(it has to live next toBlob::newto be called from there), the computation belongs inBlob::newitself, andnew_for_bindings, the constructor's call and the explicit calls added by #37656 / #37667 all become deletable. Every currentBlob::newcaller either has its fields final at that point or recomputes inBlobExt::to_jsafterwards, so that move is mechanical. It is not done here because it needs the hoist that #37656 is already making; this PR keeps the fix inbun_runtimeso the three PRs stay independent, and the consolidation is a follow-up on top of whichever lands last.Verification
Four tests added at the end of the "Native types report their size correctly" group in
test/js/bun/util/heap-snapshot.test.ts, each asserting the estimate is between 1x and 2x a 1 MiB payload:Requestbody, and from a multipartResponsebody: theFileitself and the parsedFormData(the latter goes through the entry'sWebCore::Blob, i.e. theappendBlobdupe). Without the fix: 48 and 81.Blob__dupeFromJSwith an uncomputed source). Without the fix: 84.binaryType = "blob":event.data(theBlob__dupe) and theMessageEventitself (MessageEvent::memoryCost()reads theBlob__fromBytesBlob, so this assertion is the one that depends onBlob__fromBytescomputing). Without the fix: 48 and 56.The two
*WithTypeexports are covered by one added assertion in each existing screenshot test:test/js/bun/webview/webview-chrome.test.ts("screenshot returns a PNG Blob",Blob__fromBytesWithType, runs on the Linux lanes that have Chrome) andtest/js/bun/webview/webview.test.ts(same name,Blob__fromMmapWithType, macOS lanes). Each asserts the screenshot Blob reports at least whatnew Blob([itsBytes])reports. The Chrome one was run here through a real Chrome: 48 vs 690 for a 370 byte PNG without the fix, 1123 vs 1114 with it (the 9 extra bytes are the ownedimage/png); the WebKit one is the same code path on aBytesstore and is left to CI. The existingBlobconstructor coverage (blob.test.ts, theFormDatatest in the same group) still passes with the constructor routed through the helper.With the fix the debug build reports 1049325 for the parsed File (
new Blobof the same bytes: 1049320, the difference being the storedu.binname), 1049361 for the FormData, 1049320 for the WebSocket Blob and 1049336 for its MessageEvent.The heap-snapshot tests fail with
USE_SYSTEM_BUN=1 bun testand pass withbun bd test. Also run on the fixed debug build:test/js/web/html/FormData.test.ts,FormData-multipart-serialization.test.ts,FormData-file-error-leak.test.ts,test/js/web/fetch/blob.test.ts,test/js/web/websocket/websocket-blob.test.ts(all pass;blob-file-name-ownership.test.tsand two pre-existing tests inheap-snapshot.test.tsonly exceed the 5s default per-test timeout locally under debug+ASAN, which they also do without this change).cargo fmt --checkis clean.