Repository navigation
Conversation
|
Updated 1:28 PM PT - Oct 27th, 2026
❌ @robobun, your commit 9b01338 has 1 failures in
The baseline build crashed or failed tests under 🧪 To try this PR locally: bunx bun-pr 41630That installs a local version of the PR into your bun-41630 --bun |
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Rejection detection and handling src/jsc/bindings/RejectedPromiseQueue.h, src/jsc/bindings/RejectedPromiseQueue.cpp, src/jsc/bindings/ZigGlobalObject.h, src/jsc/bindings/bindings.cpp |
The rejected-promise queue records its size before a property read. Custom-property inspection uses the size and returned value to identify a matching new rejection. |
Stream accessor rejection metadata src/jsc/bindings/webcore/streams/*.cpp |
Invalid-receiver paths for reader closed and writer closed and ready accessors use createRejectedPromiseWithTypeError with NativeGetter metadata. |
Inspection coverage test/cli/inspect/inspect.test.ts, test/js/bun/util/inspect.test.js |
Tests cover stream property previews, rejection reporting, queue pressure, and subprocess outcomes. The inspector helper separates event messages from replies. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
Low · up to 9b013
-
When debugging, previewing an event containing a rejected stream-getter promise can hide its rejection report. This is a narrow risk to be aware of when merging.
Pre-merge checks: 
4
-
Full details: Description check
-
Explanation
The description provides detailed problem context, implementation details, verification results, trade-offs, limitations, and test coverage. It does not use the template headings exactly, but it contains the required information and is complete.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
There was a problem hiding this comment.
LGTM — targeted fix that swaps the wrong-receiver rejection to the same createRejectedPromiseWithTypeError(..., NativeGetter) helper the generated WebIDL bindings already use.
What was reviewed:
- Verified
createRejectedPromiseWithTypeErrorreturnsEncodedJSValueand still usescreateTypeErrorwith the same message, only addingsetNativeGetterTypeError()— no observable behavior change outside the inspector path. - Checked all four call sites match the existing pattern in
JSDOMExceptionHandling.cpp:265(rejectPromiseWithGetterTypeError). - Test follows harness conventions:
tempDir,--inspect=127.0.0.1:0/, awaits stderr for the URL, wires WebSocketcloseto reject pending sends, asserts liveness viaRuntime.evaluateand controlledexit(42)before checking exit code — fails on the unfixed build via the rejectedsendpromise / wrong exit code, not just the stderr string check.
Extended reasoning...
Overview
Four hand-written promise-returning custom getters (closed on JSReadableStreamDefaultReader/JSReadableStreamBYOBReader, closed and ready on JSWritableStreamDefaultWriter) previously rejected wrong-receiver calls with a plain createTypeError wrapped in promiseRejectedWith. The inspector's InjectedScriptSource.js preview generator invokes these getters on the prototype object and only silences the resulting rejection when the error carries the native-getter flag (ErrorInstance::isNativeGetterTypeError()). Without the flag the rejection escapes as unhandled and Bun exits the debuggee. The fix swaps to createRejectedPromiseWithTypeError(..., RejectedPromiseWithTypeErrorCause::NativeGetter), the exact helper rejectPromiseWithGetterTypeError in JSDOMExceptionHandling.cpp already routes generated bindings through. A regression test in test/cli/inspect/inspect.test.ts exercises Runtime.getProperties with generatePreview on all three object types and asserts the debuggee stays alive.
Security risks
None. This only sets an internal flag on a TypeError that is already being created and rejected on the same wrong-receiver path. No new inputs are parsed, no auth/crypto/permissions surface is touched, and the error message is unchanged.
Level of scrutiny
Low-to-moderate. The C++ change is a four-line mechanical substitution to an established in-tree helper; I confirmed the helper's signature (JSC::EncodedJSValue) matches JSC_DEFINE_CUSTOM_GETTER's return type so dropping the outer JSValue::encode is correct, and that its NativeGetter branch still constructs a createTypeError (same error class and message as before) before calling setNativeGetterTypeError(). The include of JSDOMPromiseDeferred.h is added in each file. This is the same pattern already shipping for every generated promise attribute getter.
Other factors
The test is well-structured against the repo's review rules: it uses tempDir/bunExe/bunEnv, port: 0, await using on the subprocess, Promise.withResolvers for event bridging, and wires the WebSocket close event to reject every pending request so a debuggee crash surfaces as a test failure at the send() await rather than a hang. The load-bearing assertions (getProperties responses arrive, 1+1 still evaluates, exit code is 42) would all fail on the unfixed build independent of the supplementary stderr string check. No CODEOWNERS entries cover the changed paths and there are no outstanding third-party review objections on the timeline.
|
Status: rebased onto main and reworked. The title and the description say what this PR closes today. How I reproduced it on main:
The new tests in |
4eacdac to
6779a2f
Compare
… errors The closed getters of ReadableStreamDefaultReader, ReadableStreamBYOBReader and WritableStreamDefaultWriter, and the ready getter of WritableStreamDefaultWriter, return a rejected promise for a receiver that is not an instance. Build that promise with createRejectedPromiseWithTypeError(..., NativeGetter), so the error carries the flag that the inspector's injected script reads before it attaches a handler to the rejection of a getter that it called for a preview.
…ad produced The property walkers behind console.log, Bun.inspect and the bun test output call native getters with the printed object as the receiver. A getter that returns a rejected promise for a receiver that is not an instance left that rejection unhandled, and the process exited with code 1. The walk now marks a promise as handled when the getter rejected it during the read and returned it. A promise that was rejected before the read stays reported. Tests: the formatter cases in test/js/bun/util/inspect.test.js, and an inspector preview of objects that inherit from the stream reader and writer prototypes in test/cli/inspect/inspect.test.ts.
Formatter: the error message of a caught validation error, bun test (console.log, snapshots and a test.each title), a print inside an unhandledRejection listener, a rejection made during the read of a native getter but not returned, and Subprocess.exited after a failed wait. Inspector: assert which getters each request read, so the test fails when a request stops reading them, and add console.log with the Console domain on.
The walk compared the newest queued rejection before and after a native getter read. A getter that runs program code can handle the newest entry and return an older one. The older one is then the newest entry, and the walk marked it as handled, so it was never reported. The walk now compares the length of the queue. Entries enter at the end and never move up, so the queue is longer after the read only if its newest entry entered during the read. Before the read the walk makes room in a full queue, because an append to a full queue can drop handled entries and leave the queue no longer.
There was a problem hiding this comment.
I re-reviewed the PR after the latest commits and found no bugs; the queue-length rule in handleRejectionOfRead now covers the older-returned/newer-handled case from the earlier inline comment, and the new "leaves an older rejection reported" test pins it. Because this changes what the formatter and inspector silently mark as handled in the unhandled-rejection tracker, and the rule is a heuristic with stated limits, a human look at the design would still be worthwhile.
What was reviewed:
- The "entries never move up" invariant behind the length comparison:
makeRoomcompacts toward the front anddidHandleonly pops from the end, so an entry at index >= sizeBefore must have entered during the read. sizeBeforeReadmaking room under the cell lock before the getter call, so a single append cannot shrink the queue;handleRejectionOfReadonly reads the vector on the JS thread and marks the promise, not the vector.- The four stream getters switching to
createRejectedPromiseWithTypeError(..., NativeGetter)(same message, same rejected-promise shape) and all fourslot.isCustom()arms in both property walkers routing through the new helper. - Tests: subprocess pipes are drained with
Promise.all, the pin test asserts exact rejection message lists, the 64/128 boundary cases match thecapacity >= 64constant inmakeRoom, and the Linux-onlySubprocess.exitedcase usesskipIfwith a reason.
Extended reasoning...
The change touches the promise rejection tracking queue (RejectedPromiseQueue.h/.cpp, ZigGlobalObject.h), the console formatter's two property walkers in bindings.cpp, and four custom getters on three stream prototypes; it adds about 40 lines of native code plus roughly 290 lines of tests. It touches no auth, crypto, injection, or data-exposure surface. The prior inline concern about an older queued rejection being swallowed was addressed by replacing the newest-entry comparison with a length comparison plus a make-room step, and a dedicated test now pins that case. Deferral rather than approval is because the fix deliberately marks promises as handled from inside the formatter based on queue bookkeeping, with documented edge cases where a user-held rejection can still be hidden, which is a design trade-off a maintainer should weigh.
00ad975 to
9b01338
Compare
|
#47276 changes the same 4 getter lines as this PR: The line that serves both is the same in either merge order: auto* error = static_cast<ErrorInstance*>(Bun::createInvalidThisErrorForType(lexicalGlobalObject, "ReadableStreamDefaultReader"_s));
error->setNativeGetterTypeError();
return JSValue::encode(promiseRejectedWith(lexicalGlobalObject, error));If #47276 lands first, this PR takes that line at the 4 getters, and the two expectations at |
Problem
console.log(ReadableStreamDefaultReader.prototype)prints the object, then bun exits with code 1:TypeError: The 'closed' getter can only be used on a ReadableStreamDefaultReader. 1.3.14 and Node exit with code 0. A debugger preview ofObject.create(ReadableStreamDefaultReader.prototype)does the same.JSReadableStreamDefaultReader.cpp:695). Nothing holds that promise, so bun reports it.bun testoutput use the formatter too.Fix
createRejectedPromiseWithTypeError). The debugger handles a flagged rejection.getNativeGetterValueForDisplay). Other rejections stay reported.test/js/bun/util/inspect.test.jsandtest/cli/inspect/inspect.test.tsfail on main.Background
Subprocess.exited.Downsides
.textgrows 768 bytes.CustomEvent.detail. The formatter hides one that a replacedprocess.stdoutgetter rejects and returns.console.table,toEqual()and REPL tab completion read the getter themselves and still report.Notes
Scope of this PR before and now. The first version only flagged the four getters, for a debugger preview of a real reader or writer. On 1.4.3 that preview ended the debuggee. On 1.3.14 the reader and the BYOB reader lived and the writer did not (its
readygetter had no flag). Upstream WebKit 323271@main ("Web Inspector: don't automatically invoke native getters on functions and prototype objects") arrived with #45303 in 1.4.4. Since then the injected script does not call native getters on functions and prototype objects, so that preview lives and the first test of this PR passed without the fix. The injected script still calls the getters on an object that only inherits from the prototype, and it handles a rejection only whenInjectedScriptHost.isPromiseRejectedWithNativeGetterTypeErroris true. That is what the flag serves today.Why the formatter regressed. Until #33193 the four getters were JS builtin accessors. The formatter prints
[Getter]for those and never calls them. The C++ rewrite made them custom accessors. The property walkers inbindings.cppcall a custom accessor with the printed object as the receiver. They drop a thrown exception. They did nothing with a returned rejected promise.Designs weighed. The flag alone fixes the debugger preview and none of the formatter cases. A formatter that handles only a flagged rejection misses
Subprocess.exitedand ties the printer to a flag that #33193 dropped once without notice. Plain accessors in place of the custom accessors stop the formatter read, as in 1.3.14. But eachreader.closedandwriter.readyread on a real instance then becomes a getter function call, and the debugger still needs the flag. A change to the injected script needs a second repository.The formatter rule. Bun queues each promise that is rejected with no handler. After the microtasks drain, it reports each entry that is still not handled, and the process exits with code 1.
getNativeGetterValueForDisplayreads the length of that queue before and after the getter call. Entries enter at the end, and a removal never moves an entry up. So the queue is longer after the call only if its newest entry entered during the call. If the getter returned the promise of that entry, the walk marks it as handled. Before the call the walk makes room in a full queue, because an append to a full queue can drop handled entries, and the queue is then not longer. The rule does not read the flag. So it also coversSubprocess.exited, which returns a new rejected promise on each read after the wait for the child failed: printing such a Subprocess reportedECHILDtwice on main and reports nothing here, and a read that the program makes still reports.The rule before review. The first version compared the newest entry of the queue before and after the call. A review comment found a hole. With two unhandled rejections in the queue, a native getter that runs program code can handle the newer one and return the older one. The older one was then a new newest entry, and the walk handled it: main reports it, that version did not. Only
console._stdoutandconsole._stderrare known to run program code (they readprocess.stdoutandprocess.stderr, and a program can replace those getters), once for each stream in a process. No issue reports this, and the comment was marked optional. The length rule cannot handle a promise that was in the queue before the call. A length rule without the room step fails programs that replace nothing: a print with exactly 64 or 128 rejections in the queue exited with code 1, and a loop that handles each rejection one step late reported 18 of 1,000 prints. With the room step those report nothing. Two limits stay. A getter that runs program code can reject a promise in the call and return it, and the walk handles it (the second Downsides bullet). It can also make the walk miss its own rejection, which is then reported as on main. A sequence number for each queue entry would also be exact. It adds a field to each entry and a store on the path that queues each rejection. The second self-review was on this rule change. Its one concern that is not taken: keep the first version and state the hole as a limit.Tests.
inspect.test.js, eleven cases. Ten fail on main: prints of the prototypes, of objects that inherit from them and of a subclass prototype (console.error,Bun.inspect, sorted, and the message of a caught error), the exit code ofconsole.log(ReadableStreamDefaultReader.prototype),bun test(aconsole.log, two snapshots and atest.eachtitle), a print inside anunhandledRejectionlistener,Subprocess.exited(Linux), 200 prints in one turn, a print with 64 and with 128 handled rejections in the queue, a loop that handles each rejection one step late, and the review case above followed by one more print. One passes on both sides and pins what must stay reported.inspect.test.tsasserts which getters each inspector request read, then that no rejection is reported, then the same forconsole.logwith the Console domain on. It reports 16 rejections on main.readyreverted alone, the inspector test fails and names that getter. Without the room step, four cases fail (64, 128, 200 prints, the loop). Without the length comparison, the pin and the review case fail. With the first version of the rule, the review case fails. Without the check that the getter returned the promise, the pin fails. With the rule keyed on the flag, theSubprocess.exitedcase fails.console-log.test.ts,inspect-error.test.js,reportError.test.ts,test/js/node/console/console.test.ts,console-write.test.ts,streams.test.js. The new tests also pass withBUN_JSC_validateExceptionChecks=1.Measurements. Release LTO builds, linux-x64, of main c643ea8 and of this diff on that commit, built in the same tree. The diff under
src/is the same after the rebase onto 9b8347a (a WebKit upgrade). The tests ran again there, the measurements did not. Instruction counts are user-mode instructions (perf_event_open), JIT off, the slope between 2,000 and 12,000 iterations, median of 7 runs.closed(reader) /closed(BYOB) /closed(writer) /ready= 440.0 / 439.4 / 438.0 / 448.9 on main and 438.7 / 438.9 / 438.7 / 449.8 here (noise 4). Delta 0.CustomEventwith 14 native getter reads: 70,944 to 71,243, so +21 per read (noise 5)..text1,536 bytes more than main. An earlier body of the helper, inlined by the compiler, added 2 instructions to each property of a plain object (42,668 to 42,709 per print). So these numbers depend on what the compiler inlines.llvm-size):.text83,484,521 to 83,485,289 bytes. The stripped binary has the same file size. No new host function and no new field..catch: 2,715 to 2,734 instructions.bun:jscheapStats): 1 Promise + 1 Error per getter in both builds.Other getters. A script called every native getter on 131 prototypes that the globals and
Bunreach (320 getters) with anObject.create(prototype)receiver. Only these four return a promise. A scan of the 404JSC_DEFINE_CUSTOM_GETTERbodies finds the same four. This does not cover every class:console.log(Object.create(require("node:vm").Script.prototype))does not return on 1.3.14, on main or here. #41776 fixes that at the getters.The debugger downside in detail. The injected script handles any flagged rejection that it reads through a native getter. With a debugger attached and the Console domain on,
console.log(new CustomEvent("event", { detail: writer.ready })), wherewriteris not an instance, reported the rejection on main and does not report it here. Without the Console domain, and for a rejection with another error, nothing changes. The change that is not done would make the injected script handle a flagged rejection only when its own read made the promise. It belongs to oven-sh/WebKit. The case needs a program that makes the wrong read itself, keeps the promise behind another native getter, and is previewed in the same turn.Not covered.
console.tableandBun.inspect.tableread cell values through another iterator. Node also exits with code 1 forconsole.table([ReadableStreamDefaultReader.prototype]).Bun.deepEquals,expect().toEqual()andassert.notDeepEqualread a property to compare it.ReadableStreamDefaultReader.prototype.closedwhen the user presses Tab after it. The REPL prints oneTypeErrorafter the next evaluation and continues. That reader also runs user getters, so it is separate work.closed: Promise { <rejected> }. Node printsclosed: [Getter].Overlap. #38218 and #46156 change the same walkers in
bindings.cpp. #41776 and #46156 add tests at the end oftest/js/bun/util/inspect.test.js, and #41281 changes that file too. #46199 changesRejectedPromiseQueue. #46744 makesconsole._stdoutandconsole._stderrreal accessors, which the formatter does not call. Two cases here (the pin and the review case) use a replacedprocess.stdoutgetter behindconsole._stdout. If #46744 lands first, they fail and need another native getter that runs program code. None is known.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/util/inspect.test.js