Give File its own prototype that inherits from Blob.prototype - #30328
Jarred-Sumner wants to merge 1 commit into
Conversation
|
Updated 11:02 PM PT - Aug 23rd, 2026
✅ @robobun, your commit 9b7d1ea5a517c4165290a1a58f0ffe80798701cf passed in 🧪 To try this PR locally: bunx bun-pr 30328That installs a local version of the PR into your bun-30328 --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:
WalkthroughJSDOMFile::customHasInstance was rewritten to perform a prototype-chain walk (using getPrototype/global getPrototypeOf semantics) that unwraps ProxyObject targets and consults JSDOMFile__hasInstance for unwrapped Blob-derived objects; a new test suite verifies instanceof File/Blob behavior across proxy and prototype edge cases. ChangesFile instanceof Check Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/JSDOMFile.cpp`:
- Around line 62-79: The current code unwraps proxies and then rejects any
unwrapped object that inherits<WebCore::JSBlob>(), but JSDOMFile__hasInstance is
only called on the original (possibly proxy) value, causing real File instances
wrapped in proxies to be rejected; fix by unwrapping proxies first (using
asObject + dynamicDowncast<ProxyObject> loop) before calling
JSDOMFile__hasInstance (or call JSDOMFile__hasInstance on both the original
value and the unwrapped object) so that proxy-wrapped real File objects are
recognized as instances, and keep the inherits<WebCore::JSBlob>() rejection only
for true Blob instances that are not File according to JSDOMFile__hasInstance.
In `@test/js/web/fetch/blob.test.ts`:
- Around line 358-371: Add a new test in the same suite that constructs a real
File (e.g., new File(["hi"], "a.txt")) wrapped in a transparent Proxy and
asserts the proxy is not instanceof File but is instanceof Blob; mirror the
existing pattern used for Blob (variables like blob/proxy/nested) so you cover
Proxy(new File(...)) and nested proxies to catch regressions in the native guard
for proxied File instances.
🪄 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: b5d8e986-d6e4-437a-948a-ebf3d56f8689
📒 Files selected for processing (2)
src/jsc/bindings/JSDOMFile.cpptest/js/web/fetch/blob.test.ts
1debbd3 to
90b01e8
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This isn't a great fix. @robobun rewrite this PR so that it does the class inheritance correctly instead.
|
Rewritten per review: 27 new tests in Diff is green at 7ff518e. CI build 85009 shows only unrelated parallel-batch flakes (spawn/socket/TLS timing, all passed on retry). Ready for review. |
90b01e8 to
1e6ee27
Compare
robobun
changed the title
instanceof File for proxies and prototype-chain objects
There was a problem hiding this comment.
No new issues found; all earlier threads (isNull vs isEmpty, the third DOMFormData filename producer, reportExtraMemoryAllocated, test onerror/onclose wiring, fd.set coverage, dead include) are addressed in 7b99d88. Deferring to a human because this rewires File's prototype/structure on ZigGlobalObject and changes which wrapper every WebCore::Blob toJS path allocates — worth a maintainer glance.
Checked: LazyClassStructure is in FOR_EACH_GLOBALOBJECT_GC_MEMBER so it's visited; reifyStaticProperties passes JSBlob::info() as classForThis, which matches the instance cell type; dupe_with_content_type/the sibling clone reset is_jsdom_file so file.slice() / new Blob([file]) / response.blob() stay plain Blobs (all covered by tests); toJS now calls Blob__setAsFile on the dupe rather than the shared impl, which is strictly safer.
Extended reasoning...
Overview
Gives File a real prototype chain: adds JSDOMFilePrototype (inheriting from Blob.prototype), replaces m_JSDOMFileConstructor (LazyProperty<JSObject>) with m_JSDOMFileClassStructure (LazyClassStructure) on ZigGlobalObject, deletes the custom [Symbol.hasInstance] and its Rust export, and routes every File-producing path (new File, structuredClone, FormData entries, toJS/toJSNewlyCreated, BlobExt::to_js when is_jsdom_file) through the new JSDOMFileStructure. blob.cpp toJS now branches on fileName().isNull() to keep WebSocket binaryType='blob' frames as plain Blobs, and DOMFormData::append/set normalize a null filename to emptyString() so all three FormData filename producers agree. dupe_with_content_type and the response-body clone site reset is_jsdom_file to false so slices/blob() results don't inherit File-ness. ~280 lines of new tests.
Security risks
None identified. No untrusted-input parsing changes; the DOMFormData edit only normalizes null → empty on an already-validated filename.
Level of scrutiny
High. This touches ZigGlobalObject.{h,cpp,lut.txt} (a header included nearly everywhere, with an explicit "be very cautious" warning), adds a GC-visited LazyClassStructure, hand-writes a JSC prototype class, and changes the wrapper-structure selection for every C++-side Blob that reaches JS. Several subtle bugs were found and fixed across seven review rounds (proxy unwrapping in the abandoned first approach, isEmpty vs isNull, the multipart filename="" producer, missing reportExtraMemoryAllocated), which is evidence the change is not mechanical.
Other factors
All 33 inline threads are resolved. A maintainer (Jarred) was engaged through the May iterations but has not signed off on the July refresh. Test coverage is thorough (prototype chain, proxies, Object.create, subclassing, structuredClone, FormData.append/set × empty/omitted filename, multipart parse, file.slice(), response.blob(), WebSocket blob frames, cross-Worker postMessage). The LazyClassStructure addition is exactly the kind of member the header comment says is acceptable, and it's placed in the visitor macro, so GC rooting looks correct — but given the scope and the number of subtle iterations it took to converge, I'd rather a human confirm the final shape than shadow-approve.
There was a problem hiding this comment.
No new issues found on this pass — all earlier threads (empty-vs-null filename, reportExtraMemoryAllocated, dead include, fd.set coverage, test error-wiring) look addressed. Deferring to a human for final sign-off since this is a non-trivial JSC binding change: new JSDOMFilePrototype class + LazyClassStructure wiring, a new BUN__createJSDOMFile FFI entry, and the fileName().isNull() File-vs-Blob discrimination in blob.cpp that took a few rounds to settle.
What was reviewed:
isNull()gate inblob.cppagainst all three filename producers (JSappend/set, multipart parser viaDOMFormData::append) — the null→empty normalization now covers each.dupe_with_content_type/ thenew Blob([file])fast-path resettingis_jsdom_fileso slices and re-wraps stay plain Blobs;structuredCloneand WorkerpostMessageround-trips.reportExtraMemoryAllocatedpresent at bothBUN__createJSDOMFileand the constructor path;LazyClassStructureinitialization order (Blob prototype/constructor are forced before File's).
Extended reasoning...
Overview
The PR gives File its own prototype object that inherits from Blob.prototype, replacing the previous hack where File.prototype === Blob.prototype and a custom [Symbol.hasInstance] was installed on the File constructor. Ten files touched: a new JSDOMFilePrototype C++ class with name/lastModified accessors and Symbol.toStringTag, m_JSDOMFileClassStructure on ZigGlobalObject (replacing the old m_JSDOMFileConstructor lazy property + custom getter/setter and .lut.txt CellProperty entry), a new BUN__createJSDOMFile extern-C allocator, blob.cpp's toJS/toJSNewlyCreated branching on fileName().isNull() to pick the File vs Blob structure, DOMFormData::append/set normalizing a null filename to emptyString(), Rust-side is_jsdom_file reset in dupe_with_content_type and the new Blob([file]) fast-path, removal of the JSDOMFile__hasInstance Rust export, and ~280 lines of new tests.
Security risks
None identified. No auth/crypto/permissions surface. The change is prototype-chain shape and wrapper-structure selection; user input reaches it only as filenames already flowing through existing FormData/multipart paths.
Level of scrutiny
Medium-high. This is hand-written JSC binding C++: a new prototype class, LazyClassStructure initialization that cross-references the Blob class structure, a new FFI entry point, and a semantic gate (isNull()) whose correctness depends on every filename producer agreeing on null-vs-empty. Two of those producers were found wrong in earlier review rounds and fixed, which is exactly why a maintainer should confirm the final shape. It also changes user-visible Web API behavior (instanceof, constructor, Symbol.toStringTag, Object.getPrototypeOf(File)), so compat sign-off matters.
Other factors
Test coverage is thorough (25+ new cases spanning proxies, subclassing, structuredClone, FormData append/set with empty/omitted filenames, multipart filename="", WebSocket binaryType='blob', Worker postMessage in a fresh realm, file.slice()/Response(file).blob() staying plain Blobs). Every prior inline thread — including several from a maintainer during the May iteration and the July null/empty and extra-memory-reporting rounds — is resolved, and the last two commits directly addressed the final nits (dead include, fd.set coverage, name setter). Given the scope and the GC-adjacent C++ involved, I'd rather a human give the final approval than auto-approve.
|
Rewritten per review: 27 new tests in Rebased onto current main (9b7d1ea). Ready for review. |
47a93f3 to
daee436
Compare
There was a problem hiding this comment.
I reviewed this PR again after the 08-12 rebase and didn't find any new issues. All prior feedback is addressed — the dead FunctionPrototype.h include is gone, BUN__createJSDOMFile and the constructor both call reportExtraMemoryAllocated, blob.cpp gates on .isNull() with DOMFormData::append/set normalizing null→empty, and the WebSocket/Worker tests wire onclose/onerror and use try/finally. Because this restructures the File/Blob prototype chain and structure caching across the JSC bindings, Rust FFI, and the global lut, a human sign-off is still worthwhile.
What was reviewed:
JSDOMFilePrototype/JSDOMFileConstructorsetup andLazyClassStructurewiring — prototype inherits fromBlob.prototype, constructor's[[Prototype]]isBlob, instance structure reusesJSBlob.blob.cpptoJS/toJSNewlyCreatedrouting onfileName().isNull()and the three FormData filename producers agreeing on null vs empty.dupe_with_content_typeand the single-Blob-part fast path now clearis_jsdom_filesofile.slice()/new Blob([file])/Response(file).blob()stay plain Blobs.- Deleted
JSDOMFile__hasInstanceexport has no remaining callers;BUN__createJSDOMFileFFI signature matches on both sides.
Extended reasoning...
Overview
This PR gives File its own prototype object that inherits from Blob.prototype, replacing the previous hack where File.prototype === Blob.prototype and a custom [Symbol.hasInstance] on the File constructor faked the distinction. Changes span 10 files: a new JSDOMFilePrototype/JSDOMFileConstructor pair backed by a LazyClassStructure on ZigGlobalObject, a new JSDOMFileStructure() accessor and BUN__createJSDOMFile extern, routing in blob.cpp's toJS/toJSNewlyCreated to pick File vs Blob prototype based on whether m_fileName is null, DOMFormData::append/set normalizing a null filename to the empty string, two Rust-side sites (dupe_with_content_type and the single-Blob-part fast path in Blob.rs) clearing is_jsdom_file on copies, a new is_jsdom_file branch in BlobExt::to_js, removal of the JSDOMFile__hasInstance export and the old CellProperty lut entry, and 27 new tests in blob.test.ts.
Security risks
None identified. The change is entirely about prototype/constructor identity and instance-structure selection for File wrappers. No user input parsing, no auth/crypto/permissions surface. The one place that touches untrusted-ish input — FormData multipart filenames — only changes null-vs-empty normalization, which affects which prototype the wrapper gets, not what bytes are read or written.
Level of scrutiny
Medium-high. This is core JSC binding machinery: adding a new LazyClassStructure, a new prototype class with its own ClassInfo and reifyStaticProperties (reusing the generated BlobPrototype__nameGetterWrap/lastModifiedGetterWrap with JSBlob::info() as the DOMAttribute class so the generated getter's jsDynamicCast<JSBlob*> still matches), and rerouting several allocation sites through a new extern. Mistakes here could manifest as GC issues, wrong prototype on cross-realm/Worker instances, or subtle FormData/WebSocket behavior changes. The PR has already been through several review rounds that caught real bugs (the isEmpty vs isNull conflation, the missing reportExtraMemoryAllocated, the third filename producer via WebCore__DOMFormData__appendBlob), which is a signal that the surface is subtle enough to warrant a maintainer look.
Other factors
- All 12+ inline findings from prior automated runs are marked resolved and I confirmed each fix is present in the current diff.
- Test coverage is thorough: prototype-chain identity, Proxy (transparent and with traps),
Object.create,setPrototypeOf(null), subclassing,structuredClone,Worker.postMessagein a fresh realm,FormData.getfor append AND set with empty/omitted filename, multipartfilename="", and negative cases (file.slice(),new Response(file).blob(),new Blob([file]), WebSocketbinaryType='blob'all stay plain Blobs). - I checked that
reifyStaticProperties(vm, WebCore::JSBlob::info(), ...)is intentional — the DOMAttribute getters are the generated Blob wrappers, whosejsDynamicCasttargetsJSBlob, and File instances areJSBlobcells, so passingJSBlob::info()keeps the receiver check consistent. - The
is_jsdom_file: Cell::new(false)change indupe_with_content_typelooks correct forslice()/Response.blob()/new Blob([file]), andstructuredClone(file)still preserves File-ness because deserialization goes throughBlobExt::to_json a Blob whoseis_jsdom_filewas set from the serialized flag, not throughdupe. - No outstanding human-reviewer requests; the last human-authored comments were the author's own resolution replies.
Given the breadth (JSC prototype/structure setup, global-object lut changes, cross-language FFI signature) and the number of subtle issues already found during review, I'm deferring rather than approving so a maintainer can give it a final pass.
File.prototype was aliased to Blob.prototype, which forced a custom [Symbol.hasInstance] on the File constructor to keep new Blob() instanceof File false. That override bypassed the normal prototype-chain walk, so Proxy getPrototypeOf traps, Object.create(File.prototype), and Object.setPrototypeOf(file, null) all gave the wrong answer. This adds a JSDOMFilePrototype object whose [[Prototype]] is Blob.prototype (with own name/lastModified accessors), a LazyClassStructure on the global object so prototype, structure and constructor initialize together, and Object.getPrototypeOf(File) is Blob. new File(...), structuredClone, Worker.postMessage, Archive entries and FormData file entries allocate with the File structure. Blob dupes (slice, Response.blob(), new Blob([file])) and WebSocket binaryType=blob frames stay plain Blobs. The custom hasInstance and its Rust helper are removed; OrdinaryHasInstance now gives the correct results. Fixes #25422 Fixes #26899
daee436 to
9b7d1ea
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. All prior review threads (including the maintainer's) are resolved and every earlier finding — the isNull() vs isEmpty() filename gate, the missing reportExtraMemoryAllocated, the fd.set coverage gap, the dead FunctionPrototype.h include, and the test error-wiring nits — has been addressed in the current diff. Because this is a non-trivial JSC bindings change (new LazyClassStructure, per-global cached Structure for File instances, and rerouted toJS paths for FormData/WebSocket/structuredClone), a maintainer look would still be worthwhile.
What was reviewed:
JSDOMFilePrototype/JSDOMFileConstructorshape,setConstructor's implicitprototype.constructorwiring, andObject.getPrototypeOf(File) === Blob.blob.cpp toJS/toJSNewlyCreatedisNull()gate against all three filename producers (append,set, multipart parser viaDOMFormData::append's new normalization).BUN__createJSDOMFileand the constructor both callreportExtraMemoryAllocated;dupe_with_content_typeand the single-Blob-part fast path now clearis_jsdom_filesofile.slice()/new Blob([file])stay plain Blobs.- Ruled out the finder claim that
jsDOMFilePrototypeNameSetterduplicates the generated setter — it's a distinct wrapper reifyingnameonFile.prototypewhile reusing the generated getter.
Extended reasoning...
Overview
This PR gives File a real prototype (JSDOMFilePrototype) whose [[Prototype]] is Blob.prototype, backed by a new LazyClassStructure (m_JSDOMFileClassStructure) on ZigGlobalObject. The old customHasInstance hack and its JSDOMFile__hasInstance Rust export are deleted; instanceof File is now ordinary OrdinaryHasInstance. It touches: JSDOMFile.cpp/.h (new prototype/constructor classes, BUN__createJSDOMFile extern), ZigGlobalObject.{cpp,h,lut.txt} (LazyClassStructure init/visitation, global File binding moved from CellProperty to ClassStructure), blob.cpp (toJS/toJSNewlyCreated now branch on fileName().isNull() to pick Blob vs File structure), DOMFormData.cpp (normalize null filename → empty so all FormData blob entries wrap as Files), webcore_types.rs/Blob.rs (dupe clears is_jsdom_file; BlobExt::to_js routes File-flagged blobs through BUN__createJSDOMFile; the old JSDOMFile__hasInstance export removed), plus 27 new tests in blob.test.ts.
Security risks
None. This is prototype-chain / structure-selection plumbing for a Web API type; no auth, crypto, filesystem, or network parsing changes. The FormData filename normalization is a null→empty coercion at a layer that already sanitizes surrogates.
Level of scrutiny
High. New JSC cell classes with hand-written HashTableValue tables and reifyStaticProperties, a new cached per-global Structure, GC memory-reporting calls, and rerouted allocation paths across C++↔Rust FFI are exactly the class of change REVIEW.md flags as most-blocked (memory safety, GC rooting, structure caching). It has already been through many iterations with a maintainer and prior automated review, and every raised issue is resolved in the current diff — but the surface area (structuredClone, Worker postMessage into a fresh realm, FormData, WebSocket binaryType='blob', Archive entries, subclassing) is broad enough that this is not a mechanical change I can approve without a human.
Other factors
- All six Jarred-Sumner inline threads and every prior
claude[bot]finding are marked resolved; I verified each fix landed in the current diff (dead include removed,fd.settest cases added,w.onerror/ws.onclosewired,reportExtraMemoryAllocatedadded at both allocation sites,isNull()gate withDOMFormData::append/setnormalizing null→empty). - The two
is_jsdom_file: Cell::new(false)changes indupe_with_content_typeand the single-Blob-part constructor path are what keepfile.slice()/new Blob([file])/new Response(file).blob()as plain Blobs — covered by tests. - Finder agents raised (and verifiers refuted) that
jsDOMFilePrototypeNameSetterduplicates the generatedBlobPrototype__nameSetterWrap; it does not — it's a separate DOMAttribute setter onFile.prototypethat forwards toBlobPrototype__setNamewhile reusing the generated getters, soname/lastModifiedare own accessors onFile.prototypeper the File API spec. - 27 new tests cover the prototype chain, proxies, subclassing, structuredClone, cross-realm Worker, FormData append/set with empty/omitted filenames, multipart
filename="", WebSocket blob frames, and the negative cases (slice/Response.blob/new Blob([file]) stay plain Blobs).
This branch has not been deployed
2 participants
Closes #25422.
Closes #26899.
What does this PR do?
File.prototypewas the same object asBlob.prototype, with a comment saying "This is not quite right. But we'll fix it if someone files an issue about it." To keepnew Blob() instanceof Filefalse despite that, theFileconstructor installed a custom[Symbol.hasInstance]that checked a native flag on the underlying cell instead of walking the prototype chain.That broke every case where
instanceofis supposed to consult the prototype chain:It also meant
new File(...).constructor === Blob,Object.prototype.toString.call(file) === "[object Blob]", andFile.prototype === Blob.prototype, none of which match browsers or Node.This gives
Filea real prototype chain:JSDOMFilePrototypeis a new prototype object whose[[Prototype]]isBlob.prototype. It carriesconstructorandSymbol.toStringTag; everything else is inherited fromBlob.prototype.m_JSDOMFileStructureis a cachedStructureon the global object forFileinstances (sameJSBlobcell type, different prototype).new File(...),structuredClone(file),FormDatafile entries, andBun.Archivefile entries all allocate with that structure.hasInstanceand itsJSDOMFile__hasInstanceRust export are deleted.instanceof Fileis now ordinaryOrdinaryHasInstance.File.prototype === Blob.prototypetruefalsefalseObject.getPrototypeOf(File.prototype) === Blob.prototypetruetruenew File([], "x").constructorBlobFileFileObject.prototype.toString.call(new File([], "x"))"[object Blob]""[object File]""[object File]"new Proxy(new File([], "x"), {}) instanceof FilefalsetruetrueObject.create(File.prototype) instanceof FilefalsetruetrueObject.create(Blob.prototype) instanceof Filefalsefalsefalsenew Blob() instanceof FilefalsefalsefalsestructuredClone(file).constructorBlobFileFileBun.file()is unchanged (it is aBlob, not aFile).How did you verify your code works?
27 new tests in
test/js/web/fetch/blob.test.tscovering the prototype chain,constructor,Symbol.toStringTag,Object.getPrototypeOf(File) === Blob, proxies (transparent and with traps),Object.create,Object.setPrototypeOf, subclassing,structuredClone,Worker.postMessageinto a fresh realm,FormDataappend/set with named, empty, and omitted filenames, multipartfilename="", WebSocketbinaryType="blob", andfile.slice()/Response(file).blob()/new Blob([file])staying plain Blobs. Verified fail-before on the unfixed build and pass-after withbun bd test.Also ran the existing blob, FormData, structured-clone, Archive, globals, inspect, body, and response suites locally with no regressions.
Rebased twice onto main. The second rebase moved the
m_JSDOMFileClassStructureinit into main's newlazyClassStructureInitstable inZigGlobalObject.cppand dropped the stalem_JSDOMFileConstructorentry fromlazyObjectInits. The.lut.txtentry follows main's renamedm_generatedLazyClassesneighbours.no test proof · iteration 11 · 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