fix: return empty resolved promise when ByteBlobLoader store is null#28551
fix: return empty resolved promise when ByteBlobLoader store is null#28551
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughModified Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the fix correctly replaces the contract-violating .zero return with a resolved empty-Blob promise, matching the established ByteStream pattern.
Extended reasoning...
Overview
Two files changed: a 2-line fix in src/bun.js/webcore/ByteBlobLoader.zig and a new regression test file. The fix targets toBufferedValue when this.store is null (already consumed).
Security risks
None. This touches stream buffering logic, not auth, crypto, or permission-sensitive paths.
Level of scrutiny
Low — the change is a targeted, mechanical fix to a clearly-identified crash. The root cause (returning .zero without a pending JS exception) is well-documented in the PR description and the fix mirrors how ByteStream.toBufferedValue behaves after toAnyBlob() returns null. Cross-referencing ByteStream.zig:431-434 confirms the pattern is idiomatic.
Other factors
The two inline bug reports are style nits (missing expect(stderr).toBe("") assertions and bare test() instead of test.concurrent()). Neither affects the correctness of the regression tests or the fix itself. The tests do verify the essential behavior: no crash, correct empty output, exit code 0.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun.js/webview/JSWebViewConstructor.cpp (1)
189-238:⚠️ Potential issue | 🟠 MajorSpawn-mode options don't disable auto-detect.
When
backend.pathorbackend.argvare set, the code still passeschromeSkipAutoDetect = falsetocreateChrome, allowing DevToolsActivePort discovery to override the spawn mode. Bun can silently attach to an unrelated running Chrome instead of spawning the requested executable and flags.Set
chromeSkipAutoDetect = truewhen spawn-only options are populated. Add before thecreateChromecall (lines 343–347):if (!chromePath.isEmpty() || !chromeArgv.isEmpty()) { chromeSkipAutoDetect = true; }Also applies to: 306–325, 343–347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/webview/JSWebViewConstructor.cpp` around lines 189 - 238, The spawn-mode options (chromePath and chromeArgv) don't currently set chromeSkipAutoDetect, so code still allows DevToolsActivePort auto-detection and may attach to an unrelated Chrome; before calling createChrome (and anywhere spawn-mode is being validated, e.g., the block handling backend.path/backend.argv and the code just before createChrome), set chromeSkipAutoDetect = true when either chromePath or chromeArgv is non-empty to force spawn-only behavior (i.e., check if !chromePath.isEmpty() || !chromeArgv.isEmpty() and then set chromeSkipAutoDetect = true).packages/bun-types/bun.d.ts (1)
7981-8049:⚠️ Potential issue | 🟠 MajorMake
urlexclusive with the spawn-only Chrome options.As declared, TypeScript will still accept object literals that mix the two branches, e.g.
{ type: "chrome", url: "ws://...", path: "..." }, because excess-property checks evaluate properties across the whole union. That weakens the "mutually exclusive withpath/argv" contract and leaves invalid configs type-safe.🛠️ Suggested typing fix
+ type ChromeBackend = + { type: "chrome" } & Bun.__internal.XOR< + { + url: string; + }, + { + url?: false; + path?: string; + argv?: string[]; + stdout?: "inherit" | "ignore"; + stderr?: "inherit" | "ignore"; + } + >; + type Backend = | "webkit" | "chrome" - | { - type: "chrome"; - /** - * Connect to an existing Chrome's DevTools WebSocket directly. - * ... - */ - url: string; - } - | { - type: "chrome"; - /** - * Controls the connect-vs-spawn choice: - * ... - */ - url?: false; - path?: string; - argv?: string[]; - stdout?: "inherit" | "ignore"; - stderr?: "inherit" | "ignore"; - } + | ChromeBackend | { type: "webkit"; stdout?: "inherit" | "ignore"; stderr?: "inherit" | "ignore"; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bun-types/bun.d.ts` around lines 7981 - 8049, The two chrome-option union branches currently both use type: "chrome" so callers can illegally mix properties (e.g. {type:"chrome", url:"...", path:"..."}); make the branches mutually exclusive by updating the URL-connecting variant (the branch with url: string) to explicitly ban spawn-only fields (set path, argv, stdout, stderr to never) and update the spawn/auto-detect variant (the branch with url?: false) to explicitly forbid a connecting url (set url?: false | never or url: false), or use an XOR-style utility type; update the union for the discriminated chrome options (the two blocks with type: "chrome", url: string vs url?: false) so TypeScript rejects mixed-property objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bun-types/bun.d.ts`:
- Around line 8253-8262: The screenshot overloads on the screenshot method only
accept literal encoding signatures so calls with a variable union (e.g. const
enc: "blob" | "buffer") fail type-checking; add a final fallback overload for
screenshot that accepts encoding?: "blob" | "buffer" | "base64" | "shmem" (or
the union of all valid encodings) and returns Promise<Blob | Buffer | string | {
name: string; size: number }> so TypeScript can match variable-driven encodings;
update the declaration for the screenshot method to include this final
union-return overload while keeping the existing literal overloads.
- Around line 8318-8327: Add a matching generic removeEventListener overload for
CDP-style events and add the missing this binding to the corresponding
addEventListener overload: change the existing addEventListener<T =
unknown>(type: `${string}.${string}`, listener: (event: MessageEvent<T>) =>
void, ...) signature so the listener is typed as (this: WebView, event:
MessageEvent<T>) => void, and add removeEventListener<T = unknown>(type:
`${string}.${string}`, listener: (this: WebView, event: MessageEvent<T>) =>
void, options?: boolean | EventListenerOptions): void; keep the existing non-CDP
overloads for both addEventListener and removeEventListener as-is so strict
TypeScript users can add/remove typed CDP listeners without casts.
In `@src/bun.js/bindings/BunPlugin.cpp`:
- Around line 514-521: The argument was already verified with
callframe->argument(0).isString(), so avoid the redundant toString(globalObject)
call; use callframe->argument(0).asString() to obtain a JSC::JSString* (e.g.,
specifierString) and then get the WTF::String via
specifierString->value(globalObject). Remove the now-unnecessary
RETURN_IF_EXCEPTION(scope, {}) after toString since asString() does not invoke
VM exceptions. Update references around callframe->argument(0), asString(),
specifierString, and specifier accordingly.
In `@src/bun.js/bindings/NodeVMScriptFetcher.h`:
- Around line 27-33: The setter owner(JSC::VM&, JSC::JSValue) takes an unused vm
parameter; remove the VM parameter and change the signature to
owner(JSC::JSValue value), then adjust the body to construct m_owner from value
(using JSC::Weak<JSC::JSCell>(value.asCell()) when value.isCell(), else
m_owner.clear()). Update all call sites that currently pass a VM plus value to
call the new owner(value) overload (replace owner(vm, value) with owner(value))
and ensure m_owner and JSC::Weak<JSC::JSCell> usage remain unchanged.
In `@src/bun.js/bindings/webcore/EventListenerMap.h`:
- Around line 75-85: Make m_threadUID an atomic to eliminate the data race:
change its declaration to an std::atomic of the same type returned by
Thread::currentSingleton().uid() (e.g. std::atomic<Thread::UIDType>
m_threadUID{}), include <atomic>, and update accesses in
releaseAssertOrSetThreadUID() to use atomic operations (use load() to read and
store() to set, or a compare_exchange if you want a race-free set-if-zero).
Specifically modify the member m_threadUID and the function
releaseAssertOrSetThreadUID() so it reads m_threadUID via m_threadUID.load() and
writes via m_threadUID.store(...), ensuring proper memory semantics for these
checks before acquiring m_lock.
In `@src/bun.js/webview/ChromeBackend.cpp`:
- Around line 915-917: The static uint32_t shmSeq used to build the
shared-memory name (see shmSeq, name, snprintf and getpid) can wrap and collide
with an existing segment; update the naming to include additional entropy such
as a timestamp or random/monotonic value (e.g., append time(NULL) or a uint64_t
from clock_gettime or /dev/urandom) or promote shmSeq to a 64-bit counter to
avoid wraparound, then regenerate the name with snprintf; alternatively add a
short comment documenting the wraparound limitation if you intentionally accept
the risk—ensure the change is applied where the name is created before shm_open
with O_EXCL.
In `@src/bun.js/webview/ChromeProcess.zig`:
- Around line 395-425: Replace the raw bun.getenvZ calls used to compute root
with the cached, type-safe accessors (bun.env_var.HOME /
bun.env_var.LOCALAPPDATA) and replace the stack-allocated bun.PathBuffer
path_buf with a pooled buffer from bun.path_buffer_pool.get(); use that pooled
buffer when calling bun.path.joinAbsStringBufZ in the loop over candidates and
ensure you return the buffer via bun.path_buffer_pool.put(buf) on all control
paths (including early returns and after successful use). Target the code that
computes root (currently using bun.getenvZ) and the path_buf variable and its
use in bun.path.joinAbsStringBufZ so the change is localized to that helper.
In `@src/bun.js/webview/WebKitBackend.cpp`:
- Around line 270-295: The success flag (`ok`) is being set before the JS
wrappers are fully constructed (Blob__create, JSBuffer__fromMmap, jsString),
which can leave the mmap stranded if those operations throw; move the `ok =
true` assignment to after each wrapper creation succeeds and ensure the mmap
(`map`, `byteLen`) is unmapped on any failure/exception path so
handleReply()/m_pendingScreenshot only marks success when ownership has been
transferred by Blob__create/JSBuffer__fromMmap/jsString; reference and adjust
the Blob__fromMmapWithType/Blob__create branch, the JSBuffer__fromMmap branch,
and the Base64 branch (jsString + munmap) accordingly so that ok is set only
after the decode/creation returns without throwing and cleanup happens on
errors.
In `@src/glob/GlobWalker.zig`:
- Around line 1420-1427: In evalDir, avoid the out-of-bounds access when bump ==
2 by adding a bounds check before consulting comps[idx + 2].syntax_hint: only
access comps[idx + 2] if idx + 2 < comps.len; i.e., change the condition that
currently reads `if (bump == 2 and comps[idx + 2].syntax_hint != .Double)` to
require `idx + 2 < comps.len` as well, and keep the child.set(idx) behavior
unchanged; use the existing normalizeIdx/child.set logic around matchPatternDir
to ensure indices are clamped correctly.
---
Outside diff comments:
In `@packages/bun-types/bun.d.ts`:
- Around line 7981-8049: The two chrome-option union branches currently both use
type: "chrome" so callers can illegally mix properties (e.g. {type:"chrome",
url:"...", path:"..."}); make the branches mutually exclusive by updating the
URL-connecting variant (the branch with url: string) to explicitly ban
spawn-only fields (set path, argv, stdout, stderr to never) and update the
spawn/auto-detect variant (the branch with url?: false) to explicitly forbid a
connecting url (set url?: false | never or url: false), or use an XOR-style
utility type; update the union for the discriminated chrome options (the two
blocks with type: "chrome", url: string vs url?: false) so TypeScript rejects
mixed-property objects.
In `@src/bun.js/webview/JSWebViewConstructor.cpp`:
- Around line 189-238: The spawn-mode options (chromePath and chromeArgv) don't
currently set chromeSkipAutoDetect, so code still allows DevToolsActivePort
auto-detection and may attach to an unrelated Chrome; before calling
createChrome (and anywhere spawn-mode is being validated, e.g., the block
handling backend.path/backend.argv and the code just before createChrome), set
chromeSkipAutoDetect = true when either chromePath or chromeArgv is non-empty to
force spawn-only behavior (i.e., check if !chromePath.isEmpty() ||
!chromeArgv.isEmpty() and then set chromeSkipAutoDetect = true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bffab57-f3b0-4555-9712-6061276a4b2e
📒 Files selected for processing (39)
packages/bun-types/bun.d.tssrc/bun.js/VirtualMachine.zigsrc/bun.js/api/TOMLObject.zigsrc/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/bindings/NodeVMScriptFetcher.hsrc/bun.js/bindings/webcore/BroadcastChannel.cppsrc/bun.js/bindings/webcore/EventListenerMap.cppsrc/bun.js/bindings/webcore/EventListenerMap.hsrc/bun.js/bindings/webcore/EventTargetFactory.cppsrc/bun.js/bindings/webcore/EventTargetHeaders.hsrc/bun.js/bindings/webcore/EventTargetInterfaces.hsrc/bun.js/bindings/webcore/JSAbortController.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cppsrc/bun.js/bindings/webcore/WebSocket.cppsrc/bun.js/bindings/webcore/WebSocket.hsrc/bun.js/node/dir_iterator.zigsrc/bun.js/test/expect.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/webview/ChromeBackend.cppsrc/bun.js/webview/ChromeBackend.hsrc/bun.js/webview/ChromeProcess.zigsrc/bun.js/webview/JSWebView.cppsrc/bun.js/webview/JSWebView.hsrc/bun.js/webview/JSWebViewConstructor.cppsrc/bun.js/webview/JSWebViewPrototype.cppsrc/bun.js/webview/ObjCRuntime.hsrc/bun.js/webview/WebKitBackend.cppsrc/bun.js/webview/WebKitBackend.hsrc/bun.js/webview/WebViewEventTarget.hsrc/bun.js/webview/WebViewHost.cppsrc/bun.js/webview/WebViewHost.hsrc/bun.js/webview/host_main.cppsrc/bun.js/webview/ipc_protocol.hsrc/collections/bit_set.zigsrc/glob/GlobWalker.zigsrc/js/node/assert.tssrc/shell/braces.zigtest/js/web/streams/blob-stream-consumed.test.ts
| screenshot(options?: { encoding?: "blob"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Blob>; | ||
| screenshot(options: { encoding: "buffer"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Buffer>; | ||
| screenshot(options: { encoding: "base64"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<string>; | ||
| screenshot(options: { encoding: "shmem"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<{ | ||
| /** POSIX shm name (pass to `shm_open(2)` or Kitty `t=s`). */ | ||
| name: string; | ||
| /** Encoded image size in bytes. */ | ||
| size: number; | ||
| }>; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does TypeScript overload resolution work when an argument property is a union of string literals, like encoding: "blob" | "buffer", and the overloads are split into separate signatures for "blob"and"buffer"?
💡 Result:
TypeScript overload resolution processes call signatures in declaration order, from most specific to least specific. It selects the first signature where all arguments are assignable to the corresponding parameters. For an argument that is a union of string literals like encoding: "blob" | "buffer", with separate overloads for each literal: - If the argument is a single literal (e.g., "blob"), it matches the corresponding overload exactly, as the literal is assignable to that parameter. - If the argument is the full union ("blob" | "buffer"), it fails because the union is not assignable to either individual literal parameter (e.g., "blob" | "buffer" is not assignable to "blob"). TypeScript does not distribute union arguments across overloads or select multiple matching overloads; it requires exact match to one signature. This behavior is by design to prevent ambiguity and unintended calls, as documented in the TypeScript Handbook. It is a longstanding limitation tracked in microsoft/TypeScript#14107, with no distribution over unions. Workarounds include: - Adding a catch-all overload signature accepting the union and returning the union of return types. - Using a single generic signature with conditional return types, e.g., <T extends "blob" | "buffer">(encoding: T) => T extends "blob" ? BlobResult : BufferResult. - Preferring union parameters over overloads when possible, per official guidance.
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/2/functions.html
- 2: https://www.typescriptlang.org/docs/handbook/functions.html
- 3: Support overload resolution with type union arguments microsoft/TypeScript#14107
- 4: Overload resolution microsoft/TypeScript#32325
- 5: https://stackoverflow.com/questions/77211797/pass-a-union-type-to-an-overloaded-function
- 6: https://stackoverflow.com/questions/73729274/how-to-make-function-overloading-with-string-literal-types
- 7: https://stackoverflow.com/questions/66507585/overload-signatures-union-types-and-no-overload-matches-this-call-error
🏁 Script executed:
# Find and check the screenshot method in bun.d.ts
fd -t f 'bun.d.ts' | head -5Repository: oven-sh/bun
Length of output: 82
🏁 Script executed:
# Search for screenshot method overloads
rg -n 'screenshot' --type ts packages/bun-types/bun.d.ts | head -20Repository: oven-sh/bun
Length of output: 722
🏁 Script executed:
# Get the exact lines mentioned in the review
sed -n '8250,8270p' packages/bun-types/bun.d.tsRepository: oven-sh/bun
Length of output: 1166
Add a fallback overload for variable-driven encoding values.
The current four literal-keyed overloads will fail type-checking for calls like const enc: "blob" | "buffer" = ...; await view.screenshot({ encoding: enc }) even though the runtime supports them. TypeScript's overload resolution requires an exact match to one signature; it doesn't distribute union arguments across overloads. Add a final fallback signature accepting the union of all valid encodings and returning the union of all possible return types.
🛠️ Suggested fix
screenshot(options?: { encoding?: "blob"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Blob>;
screenshot(options: { encoding: "buffer"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Buffer>;
screenshot(options: { encoding: "base64"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<string>;
screenshot(options: { encoding: "shmem"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<{
/** POSIX shm name (pass to `shm_open(2)` or Kitty `t=s`). */
name: string;
/** Encoded image size in bytes. */
size: number;
}>;
+ screenshot(options: {
+ encoding?: "blob" | "buffer" | "base64" | "shmem";
+ format?: "png" | "jpeg" | "webp";
+ quality?: number;
+ }): Promise<
+ Blob |
+ Buffer |
+ string |
+ {
+ name: string;
+ size: number;
+ }
+ >;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| screenshot(options?: { encoding?: "blob"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Blob>; | |
| screenshot(options: { encoding: "buffer"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Buffer>; | |
| screenshot(options: { encoding: "base64"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<string>; | |
| screenshot(options: { encoding: "shmem"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<{ | |
| /** POSIX shm name (pass to `shm_open(2)` or Kitty `t=s`). */ | |
| name: string; | |
| /** Encoded image size in bytes. */ | |
| size: number; | |
| }>; | |
| screenshot(options?: { encoding?: "blob"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Blob>; | |
| screenshot(options: { encoding: "buffer"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<Buffer>; | |
| screenshot(options: { encoding: "base64"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<string>; | |
| screenshot(options: { encoding: "shmem"; format?: "png" | "jpeg" | "webp"; quality?: number }): Promise<{ | |
| /** POSIX shm name (pass to `shm_open(2)` or Kitty `t=s`). */ | |
| name: string; | |
| /** Encoded image size in bytes. */ | |
| size: number; | |
| }>; | |
| screenshot(options: { | |
| encoding?: "blob" | "buffer" | "base64" | "shmem"; | |
| format?: "png" | "jpeg" | "webp"; | |
| quality?: number; | |
| }): Promise< | |
| Blob | | |
| Buffer | | |
| string | | |
| { | |
| name: string; | |
| size: number; | |
| } | |
| >; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bun-types/bun.d.ts` around lines 8253 - 8262, The screenshot
overloads on the screenshot method only accept literal encoding signatures so
calls with a variable union (e.g. const enc: "blob" | "buffer") fail
type-checking; add a final fallback overload for screenshot that accepts
encoding?: "blob" | "buffer" | "base64" | "shmem" (or the union of all valid
encodings) and returns Promise<Blob | Buffer | string | { name: string; size:
number }> so TypeScript can match variable-driven encodings; update the
declaration for the screenshot method to include this final union-return
overload while keeping the existing literal overloads.
| addEventListener<T = unknown>( | ||
| type: `${string}.${string}`, | ||
| listener: (event: MessageEvent<T>) => void, | ||
| options?: boolean | AddEventListenerOptions, | ||
| ): void; | ||
| addEventListener( | ||
| type: string, | ||
| listener: EventListenerOrEventListenerObject, | ||
| options?: boolean | AddEventListenerOptions, | ||
| ): void; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TypeScript with strictFunctionTypes, is (event: MessageEvent) => voidassignable toEventTarget.removeEventListener/EventListener without a matching typed overload?
💡 Result:
No, (event: MessageEvent) => void is not assignable to EventTarget.removeEventListener or EventListener without a matching typed overload when strictFunctionTypes is enabled. EventTarget's removeEventListener (and addEventListener) has overloads. Specific EventTarget implementations like Window, MessagePort, etc., have typed overloads using their *EventMap (e.g., MessagePortEventMap["message"] is MessageEvent). These expect (this: MessagePort, ev: MessageEvent) => any. However, plain EventTarget only has the untyped overload: removeEventListener(type: string, callback: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions): void, where EventListener is (evt: Event) => void. With strictFunctionTypes, function parameters are checked contravariantly. Event (super type) is not assignable to MessageEvent (subtype), so (event: MessageEvent) => void fails to assign to (evt: Event) => void. A type assertion like as EventListener is needed for plain EventTarget, but typed overloads on specific targets accept it without assertion if this matches. This behavior is unchanged as of 2026 per latest lib.dom.d.ts sources.
Citations:
- 1: https://stackoverflow.com/questions/59940863/how-to-properly-type-event-handler-in-wrapped-addeventlistener-call-in-typescrip
- 2: https://stackoverflow.com/questions/47166369/argument-of-type-e-customevent-void-is-not-assignable-to-parameter-of-ty/47171216
- 3: TypeScript doesn't allow event : CustomEvent in addEventListener microsoft/TypeScript#28357
- 4: https://typhonjs-typedoc.github.io/ts-lib-docs/2023/dom/interfaces/EventTarget.html
- 5: https://raw.githubusercontent.com/microsoft/TypeScript/main/src/lib/dom.generated.d.ts
- 6: http://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-6.html
- 7: https://www.typescriptlang.org/docs/handbook/type-compatibility.html
🏁 Script executed:
# First, check the context around lines 8318-8327 in the file
sed -n '8300,8350p' packages/bun-types/bun.d.tsRepository: oven-sh/bun
Length of output: 2120
🏁 Script executed:
# Search for WebView class/interface definition
rg -n 'interface WebView|class WebView' packages/bun-types/bun.d.tsRepository: oven-sh/bun
Length of output: 97
🏁 Script executed:
# Look for removeEventListener overloads within WebView
rg -n 'removeEventListener' packages/bun-types/bun.d.ts -A 3 | head -80Repository: oven-sh/bun
Length of output: 1744
🏁 Script executed:
# Search for other addEventListener/event handler definitions with 'this:' binding in the same file
rg -n 'listener:.*this:\s*WebView' packages/bun-types/bun.d.tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Search for WebView's removeEventListener overloads specifically
sed -n '8148,8600p' packages/bun-types/bun.d.ts | grep -n 'removeEventListener' -A 3Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Also check what event handlers WebView currently has
sed -n '8148,8600p' packages/bun-types/bun.d.ts | grep -n 'addEventListener\|on[A-Z]' | head -20Repository: oven-sh/bun
Length of output: 370
🏁 Script executed:
# Get the full WebView class definition to see all addEventListener and event-related methods
sed -n '8148,8600p' packages/bun-types/bun.d.tsRepository: oven-sh/bun
Length of output: 17064
🏁 Script executed:
# Check if there are any other event handlers with 'this' binding in WebView context
rg -n 'class WebView' packages/bun-types/bun.d.ts -A 500 | head -400Repository: oven-sh/bun
Length of output: 17379
Add matching removeEventListener overload for CDP events and include this binding.
The new addEventListener<T> overload for CDP events ("Domain.Method" patterns) lacks a corresponding removeEventListener overload, preventing removal of typed listeners without a type cast in strict TypeScript projects. The contravariance of function parameters means (event: MessageEvent<T>) => void cannot be assigned to the generic EventListener type. Additionally, the listener parameter should include this: WebView to match the typing pattern used by other EventTarget subclasses in this file (e.g., EventSource, Worker).
Suggested implementation
addEventListener<T = unknown>(
type: `${string}.${string}`,
- listener: (event: MessageEvent<T>) => void,
+ listener: (this: WebView, event: MessageEvent<T>) => void,
options?: boolean | AddEventListenerOptions,
): void;
addEventListener(
type: string,
listener: EventListenerOrEventListenerObject,
options?: boolean | AddEventListenerOptions,
): void;
+ removeEventListener<T = unknown>(
+ type: `${string}.${string}`,
+ listener: (this: WebView, event: MessageEvent<T>) => void,
+ options?: boolean | EventListenerOptions,
+ ): void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bun-types/bun.d.ts` around lines 8318 - 8327, Add a matching generic
removeEventListener overload for CDP-style events and add the missing this
binding to the corresponding addEventListener overload: change the existing
addEventListener<T = unknown>(type: `${string}.${string}`, listener: (event:
MessageEvent<T>) => void, ...) signature so the listener is typed as (this:
WebView, event: MessageEvent<T>) => void, and add removeEventListener<T =
unknown>(type: `${string}.${string}`, listener: (this: WebView, event:
MessageEvent<T>) => void, options?: boolean | EventListenerOptions): void; keep
the existing non-CDP overloads for both addEventListener and removeEventListener
as-is so strict TypeScript users can add/remove typed CDP listeners without
casts.
| if (!callframe->argument(0).isString()) { | ||
| scope.throwException(lexicalGlobalObject, JSC::createTypeError(lexicalGlobalObject, "mock(module, fn) requires a module name string"_s)); | ||
| return {}; | ||
| } | ||
|
|
||
| JSC::JSString* specifierString = callframe->argument(0).toString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| WTF::String specifier = specifierString->value(globalObject); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using asString() directly after the string type check.
After confirming the argument is a string at line 514, the toString(globalObject) call at line 519 is slightly redundant since it performs unnecessary work for a value already known to be a JSString. You can use asString() directly.
♻️ Minor optimization
if (!callframe->argument(0).isString()) {
scope.throwException(lexicalGlobalObject, JSC::createTypeError(lexicalGlobalObject, "mock(module, fn) requires a module name string"_s));
return {};
}
- JSC::JSString* specifierString = callframe->argument(0).toString(globalObject);
- RETURN_IF_EXCEPTION(scope, {});
+ JSC::JSString* specifierString = callframe->argument(0).toString(globalObject)->asString();
WTF::String specifier = specifierString->value(globalObject);
RETURN_IF_EXCEPTION(scope, {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/BunPlugin.cpp` around lines 514 - 521, The argument was
already verified with callframe->argument(0).isString(), so avoid the redundant
toString(globalObject) call; use callframe->argument(0).asString() to obtain a
JSC::JSString* (e.g., specifierString) and then get the WTF::String via
specifierString->value(globalObject). Remove the now-unnecessary
RETURN_IF_EXCEPTION(scope, {}) after toString since asString() does not invoke
VM exceptions. Update references around callframe->argument(0), asString(),
specifierString, and specifier accordingly.
| void owner(JSC::VM&, JSC::JSValue value) | ||
| { | ||
| if (value.isCell()) | ||
| m_owner = JSC::Weak<JSC::JSCell>(value.asCell()); | ||
| else | ||
| m_owner.clear(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of the owner setter to verify if VM param is needed for call-site consistency.
rg -n "->owner\s*\(" --type cpp --type h
rg -n "\.owner\s*\(" --type cpp --type h | grep -v "\.owner\s*\(\s*\)"Repository: oven-sh/bun
Length of output: 459
🏁 Script executed:
# Search for owner() setter calls - fix regex escaping
rg "owner\s*\(" --type cpp --type h -n | head -30Repository: oven-sh/bun
Length of output: 541
🏁 Script executed:
# First, examine the file to get full context
cat -n src/bun.js/bindings/NodeVMScriptFetcher.h | head -50Repository: oven-sh/bun
Length of output: 2200
🏁 Script executed:
# Search for other Weak<> setters to see if they follow similar patterns
rg "Weak<" --type cpp --type h -A 5 -B 2 | grep -E "(void|owner|m_owner)" | head -30Repository: oven-sh/bun
Length of output: 2928
Remove unused vm parameter in setter.
The JSC::VM& vm parameter is not used in the function body. The JSC::Weak<JSC::JSCell> constructor used here only requires the cell value, not the VM reference.
Call sites in NodeVMScript.cpp:139 and NodeVM.cpp:1306 would also need updating to remove the VM argument.
🔧 Suggested simplification
- void owner(JSC::VM&, JSC::JSValue value)
+ void owner(JSC::JSValue value)
{
if (value.isCell())
m_owner = JSC::Weak<JSC::JSCell>(value.asCell());
else
m_owner.clear();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/NodeVMScriptFetcher.h` around lines 27 - 33, The setter
owner(JSC::VM&, JSC::JSValue) takes an unused vm parameter; remove the VM
parameter and change the signature to owner(JSC::JSValue value), then adjust the
body to construct m_owner from value (using
JSC::Weak<JSC::JSCell>(value.asCell()) when value.isCell(), else
m_owner.clear()). Update all call sites that currently pass a VM plus value to
call the new owner(value) overload (replace owner(vm, value) with owner(value))
and ensure m_owner and JSC::Weak<JSC::JSCell> usage remain unchanged.
| void releaseAssertOrSetThreadUID() | ||
| { | ||
| if (!m_threadUID) { | ||
| ASSERT(!Thread::mayBeGCThread()); | ||
| m_threadUID = Thread::currentSingleton().uid(); | ||
| return; | ||
| } | ||
| if (m_threadUID == Thread::currentSingleton().uid()) [[likely]] | ||
| return; | ||
| RELEASE_ASSERT(Thread::mayBeGCThread()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify helper/data-field pattern and pre-lock call ordering.
rg -n -C2 'releaseAssertOrSetThreadUID|m_threadUID|Locker locker \{ m_lock \};' \
src/bun.js/bindings/webcore/EventListenerMap.h \
src/bun.js/bindings/webcore/EventListenerMap.cpp
# Expected:
# 1) m_threadUID is currently non-atomic in EventListenerMap.h.
# 2) releaseAssertOrSetThreadUID() accesses m_threadUID.
# 3) Mutators call releaseAssertOrSetThreadUID() before taking m_lock.Repository: oven-sh/bun
Length of output: 4542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all m_threadUID accesses to confirm they're only in releaseAssertOrSetThreadUID()
echo "=== All m_threadUID accesses ==="
rg -n 'm_threadUID' src/bun.js/bindings/webcore/EventListenerMap.h src/bun.js/bindings/webcore/EventListenerMap.cpp
echo ""
echo "=== Checking if Thread, Lock, Locker, and atomics are available ==="
head -50 src/bun.js/bindings/webcore/EventListenerMap.h | grep -E '#include|using|namespace'Repository: oven-sh/bun
Length of output: 868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for existing std::atomic usage in WebCore bindings
echo "=== Existing std::atomic usage in WebCore bindings ==="
rg -l 'std::atomic' src/bun.js/bindings/webcore/*.cpp src/bun.js/bindings/webcore/*.h | head -5
# Check if WTF has atomic wrapper helpers
echo ""
echo "=== Looking for atomic patterns in WTF or similar headers ==="
rg 'std::atomic|compare_exchange' src/bun.js/bindings/webcore/ -A1 | head -30Repository: oven-sh/bun
Length of output: 1382
Make m_threadUID atomic to prevent a data race.
At lines 77, 79, and 82, m_threadUID is read and written without synchronization. Since all mutators call releaseAssertOrSetThreadUID() before acquiring m_lock, concurrent threads can interleave these accesses, causing undefined behavior.
🔧 Proposed fix
private:
void releaseAssertOrSetThreadUID()
{
- if (!m_threadUID) {
- ASSERT(!Thread::mayBeGCThread());
- m_threadUID = Thread::currentSingleton().uid();
- return;
- }
- if (m_threadUID == Thread::currentSingleton().uid()) [[likely]]
+ auto currentUID = Thread::currentSingleton().uid();
+ uint32_t expected = 0;
+ if (m_threadUID.compare_exchange_strong(expected, currentUID, std::memory_order_relaxed)) {
+ ASSERT(!Thread::mayBeGCThread());
+ return;
+ }
+ if (expected == currentUID) [[likely]]
return;
RELEASE_ASSERT(Thread::mayBeGCThread());
}
@@
- uint32_t m_threadUID { 0 };
+ std::atomic<uint32_t> m_threadUID { 0 };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/webcore/EventListenerMap.h` around lines 75 - 85, Make
m_threadUID an atomic to eliminate the data race: change its declaration to an
std::atomic of the same type returned by Thread::currentSingleton().uid() (e.g.
std::atomic<Thread::UIDType> m_threadUID{}), include <atomic>, and update
accesses in releaseAssertOrSetThreadUID() to use atomic operations (use load()
to read and store() to set, or a compare_exchange if you want a race-free
set-if-zero). Specifically modify the member m_threadUID and the function
releaseAssertOrSetThreadUID() so it reads m_threadUID via m_threadUID.load() and
writes via m_threadUID.store(...), ensuring proper memory semantics for these
checks before acquiring m_lock.
| const root = if (comptime bun.Environment.isWindows) | ||
| bun.getenvZ("LOCALAPPDATA") orelse return null | ||
| else | ||
| bun.getenvZ("HOME") orelse return null; | ||
| const candidates: []const []const u8 = if (comptime bun.Environment.isMac) &.{ | ||
| "Library/Application Support/Google/Chrome/DevToolsActivePort", | ||
| "Library/Application Support/Google/Chrome Canary/DevToolsActivePort", | ||
| "Library/Application Support/Google/Chrome Beta/DevToolsActivePort", | ||
| "Library/Application Support/Chromium/DevToolsActivePort", | ||
| "Library/Application Support/BraveSoftware/Brave-Browser/DevToolsActivePort", | ||
| "Library/Application Support/Microsoft Edge/DevToolsActivePort", | ||
| } else if (comptime bun.Environment.isLinux) &.{ | ||
| ".config/google-chrome/DevToolsActivePort", | ||
| ".config/google-chrome-beta/DevToolsActivePort", | ||
| ".config/google-chrome-unstable/DevToolsActivePort", | ||
| ".config/chromium/DevToolsActivePort", | ||
| ".config/BraveSoftware/Brave-Browser/DevToolsActivePort", | ||
| ".config/microsoft-edge/DevToolsActivePort", | ||
| } else if (comptime bun.Environment.isWindows) &.{ | ||
| // Windows installer layout: <vendor>\<channel>\User Data\ | ||
| "Google\\Chrome\\User Data\\DevToolsActivePort", | ||
| "Google\\Chrome SxS\\User Data\\DevToolsActivePort", // Canary | ||
| "Google\\Chrome Beta\\User Data\\DevToolsActivePort", | ||
| "Chromium\\User Data\\DevToolsActivePort", | ||
| "BraveSoftware\\Brave-Browser\\User Data\\DevToolsActivePort", | ||
| "Microsoft\\Edge\\User Data\\DevToolsActivePort", | ||
| } else &.{}; | ||
|
|
||
| var path_buf: bun.PathBuffer = undefined; | ||
| for (candidates) |rel| { | ||
| const path = bun.path.joinAbsStringBufZ(root, &path_buf, &.{rel}, .auto); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Bun's cached env accessors and the pooled path buffer in this helper.
This adds raw bun.getenvZ(...) lookups and a stack bun.PathBuffer on a cross-platform path. The Zig guidelines prefer bun.env_var.* here, and bun.path_buffer_pool.get()/put() is specifically called out to avoid the large Windows path buffer living on the stack.
♻️ Suggested cleanup
- var path_buf: bun.PathBuffer = undefined;
+ const path_buf = bun.path_buffer_pool.get();
+ defer bun.path_buffer_pool.put(path_buf);
for (candidates) |rel| {
- const path = bun.path.joinAbsStringBufZ(root, &path_buf, &.{rel}, .auto);
+ const path = bun.path.joinAbsStringBufZ(root, path_buf, &.{rel}, .auto);Switch the HOME / LOCALAPPDATA lookup to the corresponding bun.env_var.* accessor in the same pass.
As per coding guidelines, "Use type-safe, cached environment variable accessors via bun.env_var.*..." and "Use bun.PathBuffer for path buffers. For pooled path buffers that avoid 64KB stack allocations on Windows, use bun.path_buffer_pool.get() and bun.path_buffer_pool.put(buf)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/webview/ChromeProcess.zig` around lines 395 - 425, Replace the raw
bun.getenvZ calls used to compute root with the cached, type-safe accessors
(bun.env_var.HOME / bun.env_var.LOCALAPPDATA) and replace the stack-allocated
bun.PathBuffer path_buf with a pooled buffer from bun.path_buffer_pool.get();
use that pooled buffer when calling bun.path.joinAbsStringBufZ in the loop over
candidates and ensure you return the buffer via bun.path_buffer_pool.put(buf) on
all control paths (including early returns and after successful use). Target the
code that computes root (currently using bun.getenvZ) and the path_buf variable
and its use in bun.path.joinAbsStringBufZ so the change is localized to that
helper.
| if (this.matchPatternDir(pattern, next_pattern, entry_name, idx, is_last, &add_this)) |bump| { | ||
| child.set(this.normalizeIdx(idx + bump)); | ||
| // At `**/X` boundaries, keep the outer `**` alive unless | ||
| // idx+2 is itself `**` (whose recursion already covers it). | ||
| if (bump == 2 and comps[idx + 2].syntax_hint != .Double) { | ||
| child.set(idx); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential out-of-bounds access when bump == 2 and idx + 2 >= comps.len.
In evalDir, when matchPatternDir returns bump == 2, the code accesses comps[idx + 2] without verifying that idx + 2 < len. The normalizeIdx call on line 1421 handles clamping for the child set, but the check on line 1424 directly accesses comps[idx + 2].syntax_hint which could be out of bounds.
This can happen if the pattern ends with **/X (where X is the last component at index len - 1) and idx is len - 2.
🐛 Proposed fix to add bounds check
if (this.matchPatternDir(pattern, next_pattern, entry_name, idx, is_last, &add_this)) |bump| {
- child.set(this.normalizeIdx(idx + bump));
+ const target = idx + bump;
+ if (target < len) {
+ child.set(this.normalizeIdx(target));
+ }
// At `**/X` boundaries, keep the outer `**` alive unless
// idx+2 is itself `**` (whose recursion already covers it).
- if (bump == 2 and comps[idx + 2].syntax_hint != .Double) {
+ if (bump == 2 and target < len and comps[target].syntax_hint != .Double) {
child.set(idx);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/glob/GlobWalker.zig` around lines 1420 - 1427, In evalDir, avoid the
out-of-bounds access when bump == 2 by adding a bounds check before consulting
comps[idx + 2].syntax_hint: only access comps[idx + 2] if idx + 2 < comps.len;
i.e., change the condition that currently reads `if (bump == 2 and comps[idx +
2].syntax_hint != .Double)` to require `idx + 2 < comps.len` as well, and keep
the child.set(idx) behavior unchanged; use the existing normalizeIdx/child.set
logic around matchPatternDir to ensure indices are clamped correctly.
robobun
force-pushed
the
farm/673a370c/fix-blob-stream-text-crash
branch
from
f36eb06 to
35c1fd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/streams/blob-stream-consumed.test.ts`:
- Around line 13-36: Add a new concurrent test mirroring the existing
.text/.bytes/.blob cases: call Response.json({ a: 1 }), grab resp.body!, consume
the buffered store via await resp.text(), then call await body.json() and assert
the result is null (i.e., expect(parsed).toBe(null)); reference the existing
test pattern using test.concurrent, Response.json, resp.text, and body.json to
place and implement the new case.
- Around line 3-12: Trim the long inline bug-context comments at the top of
test/js/web/streams/blob-stream-consumed.test.ts (the multi-line comment
describing the regression scenario) and the additional inline comment referenced
around the later comment, replacing them with a short one-line summary of
behavior under test or moving the detailed explanation to the PR description;
keep the test file focused on the behavior and assertions rather than
historical/diagnostic context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2170ac4-37e3-4617-a649-b8adc7a741da
📒 Files selected for processing (2)
src/bun.js/webcore/ByteBlobLoader.zigtest/js/web/streams/blob-stream-consumed.test.ts
| // Regression test: calling .text()/.bytes()/.blob() on a | ||
| // BlobInternalReadableStreamSource body after the backing store has been consumed | ||
| // used to crash with "Expected an exception to be thrown" debug assertion because | ||
| // toBufferedValue returned .zero without setting a JS exception. | ||
| // | ||
| // The scenario: Response API consumption (resp.text()) detaches the ByteBlobLoader | ||
| // store via toBlobIfPossible (Path A) without marking the ReadableStream as | ||
| // disturbed. A subsequent body.text() enters the buffered fast path and calls | ||
| // toBufferedValue on a null store. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider trimming inline bug-context comments in this test file.
The long explanatory block can be moved to PR context to keep tests focused on behavior.
Based on learnings, "In oven-sh/bun regression test files ... do not suggest adding inline comments that explain the bug context or issue number being tested ... test code is kept clean without such explanatory comments."
Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/web/streams/blob-stream-consumed.test.ts` around lines 3 - 12, Trim
the long inline bug-context comments at the top of
test/js/web/streams/blob-stream-consumed.test.ts (the multi-line comment
describing the regression scenario) and the additional inline comment referenced
around the later comment, replacing them with a short one-line summary of
behavior under test or moving the detailed explanation to the PR description;
keep the test file focused on the behavior and assertions rather than
historical/diagnostic context.
| test.concurrent("ReadableStream body .text() after store consumed returns empty", async () => { | ||
| const resp = Response.json({ a: 1 }); | ||
| const body = resp.body!; | ||
| expect(await resp.text()).toBe('{"a":1}'); | ||
| // body's ByteBlobLoader store is now detached but stream is undisturbed | ||
| const result = await body.text(); | ||
| expect(result).toBe(""); | ||
| }); | ||
|
|
||
| test.concurrent("ReadableStream body .bytes() after store consumed returns empty", async () => { | ||
| const resp = Response.json({ a: 1 }); | ||
| const body = resp.body!; | ||
| await resp.text(); | ||
| const bytes = await body.bytes(); | ||
| expect(bytes.byteLength).toBe(0); | ||
| }); | ||
|
|
||
| test.concurrent("ReadableStream body .blob() after store consumed returns empty", async () => { | ||
| const resp = Response.json({ a: 1 }); | ||
| const body = resp.body!; | ||
| await resp.text(); | ||
| const blob = await body.blob(); | ||
| expect(blob.size).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a consumed-store .json() case for parity with buffered action coverage.
toBufferedValue is action-tag based; adding .json() here would close the remaining regression surface.
➕ Suggested test addition
test.concurrent("ReadableStream body .blob() after store consumed returns empty", async () => {
const resp = Response.json({ a: 1 });
const body = resp.body!;
await resp.text();
const blob = await body.blob();
expect(blob.size).toBe(0);
});
+
+test.concurrent("ReadableStream body .json() after store consumed rejects normally", async () => {
+ const resp = Response.json({ a: 1 });
+ const body = resp.body!;
+ await resp.text();
+ await expect(body.json()).rejects.toThrow();
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.concurrent("ReadableStream body .text() after store consumed returns empty", async () => { | |
| const resp = Response.json({ a: 1 }); | |
| const body = resp.body!; | |
| expect(await resp.text()).toBe('{"a":1}'); | |
| // body's ByteBlobLoader store is now detached but stream is undisturbed | |
| const result = await body.text(); | |
| expect(result).toBe(""); | |
| }); | |
| test.concurrent("ReadableStream body .bytes() after store consumed returns empty", async () => { | |
| const resp = Response.json({ a: 1 }); | |
| const body = resp.body!; | |
| await resp.text(); | |
| const bytes = await body.bytes(); | |
| expect(bytes.byteLength).toBe(0); | |
| }); | |
| test.concurrent("ReadableStream body .blob() after store consumed returns empty", async () => { | |
| const resp = Response.json({ a: 1 }); | |
| const body = resp.body!; | |
| await resp.text(); | |
| const blob = await body.blob(); | |
| expect(blob.size).toBe(0); | |
| }); | |
| test.concurrent("ReadableStream body .text() after store consumed returns empty", async () => { | |
| const resp = Response.json({ a: 1 }); | |
| const body = resp.body!; | |
| expect(await resp.text()).toBe('{"a":1}'); | |
| // body's ByteBlobLoader store is now detached but stream is undisturbed | |
| const result = await body.text(); | |
| expect(result).toBe(""); | |
| }); | |
| test.concurrent("ReadableStream body .bytes() after store consumed returns empty", async () => { | |
| const resp = Response.json({ a: 1 }); | |
| const body = resp.body!; | |
| await resp.text(); | |
| const bytes = await body.bytes(); | |
| expect(bytes.byteLength).toBe(0); | |
| }); | |
| test.concurrent("ReadableStream body .blob() after store consumed returns empty", async () => { | |
| const resp = Response.json({ a: 1 }); | |
| const body = resp.body!; | |
| await resp.text(); | |
| const blob = await body.blob(); | |
| expect(blob.size).toBe(0); | |
| }); | |
| test.concurrent("ReadableStream body .json() after store consumed rejects normally", async () => { | |
| const resp = Response.json({ a: 1 }); | |
| const body = resp.body!; | |
| await resp.text(); | |
| await expect(body.json()).rejects.toThrow(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/web/streams/blob-stream-consumed.test.ts` around lines 13 - 36, Add a
new concurrent test mirroring the existing .text/.bytes/.blob cases: call
Response.json({ a: 1 }), grab resp.body!, consume the buffered store via await
resp.text(), then call await body.json() and assert the result is null (i.e.,
expect(parsed).toBe(null)); reference the existing test pattern using
test.concurrent, Response.json, resp.text, and body.json to place and implement
the new case.
There was a problem hiding this comment.
LGTM — minimal, correct fix with concurrent regression tests.
Extended reasoning...
Overview
This PR modifies src/bun.js/webcore/ByteBlobLoader.zig (2 lines) and adds a new test file test/js/web/streams/blob-stream-consumed.test.ts. The Zig change replaces a bare .zero return with a resolved promise wrapping an empty Blob, fixing a contract violation that triggered a debug assertion crash when toBufferedValue was called after the backing store was already consumed.
Security risks
None. This is a pure defensive fix in stream buffering logic with no auth, crypto, or permission surfaces involved.
Level of scrutiny
Low. The change is a 2-line targeted fix in a single function. The replacement pattern (var empty: Blob.Any = .{ .Blob = Blob.initEmpty(globalThis) }; return empty.toPromise(globalThis, action);) mirrors what the function already does in the success path just above it. There is no ambiguity about the intent or correctness.
Other factors
Previous inline comments from this reviewer about test.concurrent() were already addressed — the tests in the diff correctly use test.concurrent(). The CI build failures on the PR commit appear to have been formatting-related and were resolved by the subsequent [autofix.ci] commit. No bugs were found by the automated bug hunting system. The coderabbit comments are all about unrelated files not touched by this PR.
|
CI failures appear to be infrastructure-related (Darwin jobs expired/timed out, not test failures). My test passes locally under ASAN. Retrying CI. |
robobun
force-pushed
the
farm/673a370c/fix-blob-stream-text-crash
branch
from
2047a13 to
d6764e7
Compare
There was a problem hiding this comment.
LGTM — minimal fix for a clear contract violation, tests already use test.concurrent(), and CI failures are infra-related.
Extended reasoning...
Overview
Two files changed: a 2-line fix in src/bun.js/webcore/ByteBlobLoader.zig and a new regression test file test/js/web/streams/blob-stream-consumed.test.ts. The fix replaces a bare return .zero (which signals "exception thrown" to the JSC host function contract) with a proper empty.toPromise(globalThis, action) when toAnyBlob returns null (store already consumed). This matches the pattern used by ByteStream for the same condition.
Security Risks
None. This is a pure correctness fix in a stream buffering path with no auth, crypto, or permission logic involved.
Level of Scrutiny
Low. The change is a straightforward one-liner bug fix with clear intent: avoid returning .zero without a pending exception. The fix mirrors an existing pattern in the codebase. The regression tests are concise, use test.concurrent() per the style guide, and directly reproduce the crash scenario.
Other Factors
My prior inline review comments (missing expect(stderr).toBe("") and test() vs test.concurrent()) are both marked resolved — and the diff confirms the tests already use test.concurrent(), making those concerns moot. The CI failures noted by robobun are infrastructure-related (Darwin timeout/expiry), not caused by this change.
| var empty: Blob.Any = .{ .Blob = Blob.initEmpty(globalThis) }; | ||
| return empty.toPromise(globalThis, action); |
There was a problem hiding this comment.
🟡 The crash fix in toBufferedValue is correct, but no regression test was included in the final merge. The file test/js/web/streams/blob-stream-consumed.test.ts was referenced in review comments but is absent from the repository, leaving the fixed crash path unguarded against future regressions. CLAUDE.md explicitly requires all changes to be tested.
Extended reasoning...
The PR fixes a real crash: when ByteBlobLoader.toBufferedValue was called after the store had already been consumed (set to null), it returned .zero without setting a JavaScript exception, violating the host function contract and triggering a debug assertion. The fix — returning a resolved promise from an empty Blob.initEmpty instead of .zero — is correct and mirrors how ByteStream handles the same case.
However, the final merged commit (d6764e7) only contains changes to src/bun.js/webcore/ByteBlobLoader.zig. No test file was added. Searching the repository confirms that test/js/web/streams/blob-stream-consumed.test.ts does not exist anywhere in the codebase.
The PR review comments from both claude[bot] and coderabbitai make inline comments on test/js/web/streams/blob-stream-consumed.test.ts, including specific line references (lines 9, 31, 55 for three test() calls) and style details like missing expect(stderr).toBe("") assertions and bare test() instead of test.concurrent(). This strongly indicates the test file existed in an earlier version of the PR but was dropped before the final commit.
The project CLAUDE.md, at point 2, states explicitly: "All changes must be tested - if you're not testing your changes, you're not done." The reproduction scenario from the PR description maps directly to a trivial regression test: create a Response.json(...), consume the body via .text(), then call .text() again and assert it resolves to an empty string rather than crashing.
Step-by-step proof of the regression gap:
- A future contributor modifies
toBufferedValue, perhaps cleaning up thevar emptybinding or changingBlob.initEmpty. - They accidentally reintroduce the
.zero-without-exception return path. - No test exists to catch this; CI passes.
- The crash is silently reintroduced until a user reports it in production.
The three test cases referenced in review (body.text(), body.bytes(), body.blob() after store consumption) cover the full surface of the action tag parameter in toBufferedValue and are trivial to implement. Adding them as test.concurrent in test/js/web/streams/blob-stream-consumed.test.ts would complete the fix.
When toBufferedValue was called on a ByteBlobLoader whose store had already been consumed (null), it returned .zero without setting a JavaScript exception. This violated the host function contract that .zero means an exception was thrown, triggering an assertion crash. Return a resolved promise from an empty Blob instead.
robobun
force-pushed
the
farm/673a370c/fix-blob-stream-text-crash
branch
from
d6764e7 to
98c93ae
Compare
1 participant
When
toBufferedValuewas called on aByteBlobLoaderwhose store had already been consumed (null), it returned.zerowithout setting a JavaScript exception. This violated the host function contract that returning.zeromeans an exception was thrown, triggering a debug assertion crash:Expected an exception to be thrown.The fix returns a resolved promise from an empty Blob instead of
.zero, matching howByteStreamhandles the equivalent case.Reproduction:
Crash fingerprint:
a136028432ab06ee