HTMLRewriter: accept a Response whose body is a JS-created ReadableStream - #33310
HTMLRewriter: accept a Response whose body is a JS-created ReadableStream#33310robobun wants to merge 6 commits into
Conversation
|
Updated 5:58 AM PT - Jul 15th, 2026
❌ @robobun, your commit b1acf27 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33310That installs a local version of the PR into your bun-33310 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughThis PR propagates pending JS exceptions in HTMLRewriter, reworks JavaScript-backed ChangesJS ReadableStream buffering support
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/workerd/html-rewriter.test.js`:
- Around line 470-474: The test "reusing the transformed response's source
stream throws" is too broad because it uses a bare toThrow() and does not verify
the intended failure. Update the assertion around rewriter().transform(response)
to check the exact error message or error class used by this repo’s conventions,
specifically matching the "Response body already used" failure. Keep the test
focused on the transform(response) reuse path and assert the normalized error
value instead of any thrown exception.
🪄 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: a0f1afd2-c307-4af4-9814-5ef40de3d585
📒 Files selected for processing (3)
src/runtime/api/html_rewriter.rssrc/runtime/webcore/Body.rstest/js/workerd/html-rewriter.test.js
|
Checked both against this branch. Neither is fixed here, and the reasoning in the suggestion does not hold:
Both reproduce on unmodified // raw TCP upstream writes "<p>h" then stalls
const up = await fetch(url);
const out = new HTMLRewriter().on("p", { element: e => e.setInnerContent("bye") }).transform(up);
await out.body.getReader().read(); // resolves { value: undefined, done: true } -> #19305
// Bun.serve({ fetch: () => out }) // hangs forever -> #6068Verified on this branch: both still reproduce, byte for byte. What this PR does change is that a JS-backed source now reaches that same (still-broken) output path instead of throwing Not adding the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Both review findings were right, and both are fixed in b641742. Thanks — the second one is a bug I would not have found from the CI log alone. 1. Unchecked exception scope (the x64-asan failure)Correct diagnosis. Reproduced locally, byte for byte: Fixed with let promise_value = {
bun_jsc::validation_scope!(scope, global);
let value = global.readable_stream_to_array_buffer(stream.value);
scope.assert_exception_presence_matches(value.is_empty());
value
};Whole file now passes under 2. Rewriting out of a detachable user bufferAlso correct, and it is observable without even getting to the free. let chunk;
const body = new ReadableStream({
start(c) { chunk = new TextEncoder().encode("<a>x</a><zzz>y</zzz>"); c.enqueue(chunk); c.close(); },
});
const out = new HTMLRewriter()
.on("a", { element() { chunk.set(new TextEncoder().encode("qqq"), 9); } })
.transform(new Response(body));
await out.text();
// before: "<a>x</a><qqq>y</zzz>" <- the handler's write leaked into the unparsed tail
// after: "<a>x</a><zzz>y</zzz>"Adding Fixed by copying into the bufferer's existing The bare |
There was a problem hiding this comment.
b8c5ff6 addresses the last finding (the readable_stream_ref Strong is now released before the builtin returns, and the new leak test covers it) — nothing further from me, but given the JSC GC-root / refcount / exception-scope interactions this touches and the three memory-safety fixes already made during review, this warrants a human pass over the final state.
Extended reasoning...
Overview
This PR routes Source::JavaScript and Source::Direct ReadableStream bodies through readableStreamToArrayBuffer so HTMLRewriter.transform() accepts a JS-created stream instead of throwing ERR_STREAM_CANNOT_PIPE. It touches ~130 lines of native code across src/runtime/webcore/Body.rs (new buffer_js_readable_stream, reworked handle_resolve_stream, dead js_sink/ArrayBufferJSSink removal) and src/runtime/api/html_rewriter.rs (surface pending JS exceptions from the bufferer), plus ~280 lines of new tests in test/js/workerd/html-rewriter.test.js and un-skips two existing .todo payload tests.
What changed since my last review
Commit b8c5ff6 addresses the Strong-ref leak I flagged: buffer_js_readable_stream now calls self.readable_stream_ref.deinit() immediately after handing the stream to the builtin, with a comment explaining the native↔JS cycle it would otherwise create, and a new heapStats-based leak test (does not leak a transform whose source stream never settles) that abandons 200 default-stream transforms and asserts live Response count doesn't grow. All three prior findings (detachable-buffer UAF, unchecked exception scope, Strong-ref leak) now have both the fix and a targeted regression test in the diff. The current bug-hunting run found nothing new.
Security risks
No new externally-reachable attack surface — the input is a developer-supplied ReadableStream, not remote data. The memory-safety concerns (borrowed slice into a user-detachable ArrayBuffer fed to lol-html while it re-enters user JS; refcount balance on the abandoned-promise path; exception-scope validation) have each been addressed with a fix and a test. I did not find further issues on this pass.
Level of scrutiny
High. This is native runtime code at the intersection of JSC GC roots (Strong, NativePromiseContext), intrusive refcounting (BufferOutputSink), JSC exception-scope validation, and re-entry into user JS while holding borrowed native state — exactly the bug classes CLAUDE.md flags as most-blocked. Three separate memory-safety issues surfaced and were fixed during review; that history alone argues for a human confirming the final composition (particularly the readable_stream_ref.deinit() placement relative to the synchronous Fulfilled/Rejected arms and the is_empty() early return, and the stream_buffer copy on the resolve path).
Other factors
Test coverage is thorough (14 new cases spanning sync/async, direct/default, error paths, all consumption APIs, the detach case, the leak case, and the Bun.serve shape). The PR description is precise about what is and isn't fixed (output-side .body gap recorded as .todo). CI on the latest commit (build #68226) was still running when I reviewed.
|
Agreed that this wants a human pass. The two things named as worth checking are the right two, so e33494e pins both down in-tree rather than leaving them as prose.
|
Ready for review. The one red lane is not this diff.Build #68227 on That is a PTY test. This diff touches It is currently failing pipeline-wide, on unrelated branches:
So I'm not re-rolling — it would land on the same lane. What's here
90 pass / 1 todo / 0 fail on the file, clean under What I'd want a reviewer to look atThree of the five commits fix bugs in my own first revision — unobserved The code is right now, and the invariants are spelled out in this comment and encoded as tests. But that history is the honest argument for a careful pass over the final composition rather than taking my word for it — particularly the |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
The correct fix to make HTMLRewriter not buffered.
|
You're right, and digging into why has made the two problems collapse into one. This PR is the wrong fix. The buffering and the JS-stream rejection are the same bug
Source::JavaScript | Source::Direct => {
// this is broken right now
// return self.create_js_sink(stream);
return Err(bun_core::err!("UnsupportedStreamType"));
}Meanwhile the runtime already has the primitive this wants to be:
It is a two-method trait: pub trait ResumableSinkContext {
fn write_request_data(&mut self, bytes: &[u8]) -> ResumableSinkBackpressure;
fn write_end_request(&mut self, err: Option<JSValue>);
}
So HTMLRewriter buffers because it reimplemented this badly, and it rejects JS streams because that reimplementation never finished. Pointing The one real prerequisite
if let Some(promise) = result.as_any_promise() {
vm().wait_for_promise(promise); // nested drain, mid-writeThat is only safe today because ProposalThere are four open PRs in this area, all mine, all overlapping, none landing: #32956 (output streaming), #32988 (full streaming rework, bolts an Unless you say otherwise I will:
The test suite here (JS/direct sources, chunk boundaries, all the consumption paths, the error and abandoned-stream cases) carries over unchanged and is the regression net for step 2, so the work is not wasted. Say the word if you'd rather I keep them separate or sequence them differently. |
…ream
transform() threw ERR_STREAM_CANNOT_PIPE ("Failed to pipe stream") for any
Response built from `new ReadableStream({...})` or a `type: "direct"` stream.
ValueBufferer::buffer_locked_body_value rejected Source::JavaScript and
Source::Direct outright; the arm it returned from was commented "this is
broken right now".
Route those sources through readableStreamToArrayBuffer, which is the same
path `new Response(stream).arrayBuffer()` already takes, and hand the
resolved bytes to on_finished_buffering. A stream that is already complete
resolves synchronously and keeps transform()'s synchronous contract; a
pending one settles through the existing onResolveStream/onRejectStream
promise reactions, so an upstream error rejects the transformed body instead
of truncating it.
readableStreamToArrayBuffer can also throw synchronously (a chunk that is
neither a string nor a view). Propagate that pending exception instead of
masking it with ERR_STREAM_CANNOT_PIPE.
js_sink and its ArrayBufferJSSink alias were the dead remains of the
create_js_sink approach this replaces; nothing ever set the field.
Fixes #14216
Fixes #11758
…d bytes Two fixes to the JS-ReadableStream path. `readableStreamToArrayBuffer`'s C++ wrapper returns under a ThrowScope, whose destructor runs simulateThrow() when returning to native code. Checking the returned JSValue for emptiness is invisible to JSC's exception-check validation, so the unobserved need-check tripped the next scope (`JSC__JSValue__asArrayBuffer`'s ASSERT_NO_PENDING_EXCEPTION) and aborted the x64-asan lane. Wrap the call in a validation_scope! and assert the empty-value/pending-exception correspondence. handle_resolve_stream handed lol-html a slice borrowed straight from the resolved ArrayBuffer. toArrayBuffer()'s single-chunk fast path returns the user's own buffer verbatim, and lol-html tokenizes its input in place while dispatching handlers, so a handler could mutate the bytes still being parsed (or transfer the buffer and free the backing store). Copy into stream_buffer first, as the Source::Bytes path already does.
buffer_locked_body_value roots the source stream in readable_stream_ref before dispatching. On the JS-stream path that root also transitively reaches the NativePromiseContext cell (stream -> reader -> readRequests -> promise reactions -> cell), and the cell holds the sink's in-flight +1. So the cell was never collected, Bun__NativePromiseContext__destroy never fired, and a transform over a stream that never settles leaked its whole BufferOutputSink: the rewriter, its output buffer, and the Strong pinning the transformed Response. Release the root once readableStreamToArrayBuffer has taken the stream over through its own reader, exactly as set_promise already does for Response.arrayBuffer(). Whatever drives the source keeps the returned promise alive while the transform is live; when nothing does, the chain is collectable and the deferred deref runs. Measured over 200 abandoned transforms: +200 live Responses before, 0 after. Direct streams never hit this (readableStreamToArrayBufferDirect stores nothing on the stream that reaches the capability promise).
… unrooted sources The bufferer drops its GC root on the source once readableStreamToArrayBuffer owns it, so a live transform is kept alive only by whatever can still settle the stream. Pin that down: settling needs the controller, and the controller holds the stream, so the three shapes that hide the stream from userland (controller held by a timer, escaped to an outer scope, or reachable only from a pending direct pull) still complete after collecting hard mid-flight. Also debug_assert that handle_resolve_stream is the sole writer to stream_buffer, matching the assertions the Source::Bytes arm already carries.
e33494e to
c9fd390
Compare
The only construction site was the Source::JavaScript | Source::Direct arm of buffer_locked_body_value, which this PR replaces.
|
Superseded by #35324, which does the |
|
Superseded by #36697. |
2 participants
Fixes #14216
Fixes #11758
Repro
transform()itself throws, before anything is read. The same body without the rewriter reads fine (new Response(body).text()), and the same rewriter accepts string / Blob /blob.stream()/fetch()bodies. The broken composition is the documented streaming-SSR and middleware shape: build or wrap an HTML body in JS, rewrite it on the way out.Cause
ValueBufferer::buffer_locked_body_value(src/runtime/webcore/Body.rs) matches on the source stream's kind and rejects two of them outright:HTMLRewriter'sBufferOutputSinkis the bufferer's only caller, and it maps every error other thanStreamAlreadyUsedtoERR_STREAM_CANNOT_PIPE. So a bug class the rewriter never supported surfaces as a generic pipe failure that reads like user error.Source::Bytes(fetch/file/blob bodies) has a native pipe;Source::JavaScriptandSource::Directonly the JS runtime knows how to drive, which is what the removedcreate_js_sinkwas trying to work around with anArrayBufferSink.Fix
Route those two sources through
readableStreamToArrayBuffer, the same builtinnew Response(stream).arrayBuffer()already uses, and feed the resolved bytes toon_finished_buffering:transform()keeps its synchronous contract (including throwing a handler's error);Bun__BodyValueBufferer__onResolveStream/onRejectStream, the promise reactions that were already wired up and registered inpromiseHandlerIDbut unreachable. An upstream error therefore rejects the transformed body rather than truncating it, matching HTMLRewriter: reject the transformed body when the upstream body fails #32927's behavior for native bodies.The pending-promise context is a
NativePromiseContextcell, so a promise collected without ever settling releases the sink's ref throughBun__NativePromiseContext__destroyinstead of leaking.readableStreamToArrayBuffercan also throw synchronously (for example a chunk that is neither a string nor a view). That exception is already pending on the VM, so it is now propagated instead of being masked byERR_STREAM_CANNOT_PIPE.js_sinkand itsArrayBufferJSSinkalias were the dead remains of thecreate_js_sinkapproach being replaced; nothing ever assigned the field.Scope: this is the input side only
The rewriter still buffers the whole document before running lol-html, as it always has for every body type. The output side is untouched; #32988 and #32956 are the orthogonal output-side changes, and neither one fixes this arm (both still carry
// this is broken right now).One consequence is worth naming, because it is the one thing this PR does not get you. A JS stream that is already complete when
transform()returns (the common shape:start()enqueues and closes) resolves synchronously, so the transformed body is a plain blob and every consumer works, includingBun.serve. A JS stream that is still pending at that point now behaves exactly like a native body that is still mid-stream:.text()/.arrayBuffer()/.bytes()/.blob()all work, but reading.bodyyields""and returning the response straight fromBun.servehangs.That is not introduced here. Both reproduce on unmodified
mainwith a native source — afetch()whose body is still arriving whentransform()runs:So this PR brings JS-backed sources to parity with native ones rather than past them, and #32988 is what closes the remaining gap for both. A
.todoin the test file records it.Three follow-up fixes, each its own commit
The first revision was wrong in three ways. All three were caught by machinery, not by me, and each has a test that fails without its fix:
readableStreamToArrayBuffer's C++ wrapper returns under aThrowScope, whose destructor callssimulateThrow()when returning to native code. Checking the returnedJSValuefor emptiness is a value check, invisible to JSC's exception-check validation, so the outstanding need-check tripped the next scope (asArrayBuffer'sASSERT_NO_PENDING_EXCEPTION) and aborted the x64-asan lane. Now wrapped invalidation_scope!withassert_exception_presence_matches(value.is_empty()).handle_resolve_streamhanded lol-html a slice borrowed straight from the resolved ArrayBuffer.toArrayBuffer()'s single-chunk fast path returns the user's own buffer, and lol-html tokenizes in place while dispatching handlers, so a handler could reach into bytes not yet parsed (<a>x</a><zzz>came out as<a>x</a><qqq>) or transfer the buffer and free it mid-read. Now copied intostream_bufferfirst, as theSource::Bytespath already does.buffer_locked_body_valueroots the source stream, and on this path that root transitively reaches theNativePromiseContextcell holding the sink's refcount, so an abandoned transform leaked its wholeBufferOutputSink. Measured: 200 abandoned transforms → +200 liveResponseobjects. Released once the builtin owns the stream, mirroring whatset_promisealready does forResponse.arrayBuffer()(with a comment citing Memory leak if accessing body of fetch() response before consuming the stream #13678 for the same reason).Verification
Tests land in
test/js/workerd/html-rewriter.test.js(the existing HTMLRewriter file — this behavior was never correct, so it is not a regression test). 17 new cases cover: single binary chunk, single string chunk (the builtin returns aUint8Arrayview there, a different branch), an element split across chunk boundaries, mixed string/binary chunks, an empty stream, atype: "direct"stream, a stream that only produces aftertransform()returns, all seven consumption paths (.text(),.arrayBuffer(),.bytes(),.blob(),.json(),getReader(),Bun.readableStreamToText), element handlers observing the document, a stream that errors (before and aftertransform()returns), a bad chunk surfacing its realTypeError, reuse of a consumed source, theBun.serveshape from #11758, plus the detach and leak regressions above.Every one of them fails on
main. The twoit.todo("works with payload of type direct" / "default")cases in that file were todo for exactly this reason and are un-skipped; they pass now.Full suite
A note on the leak, since it is the kind of thing that hides:
Bun__NativePromiseContext__destroyschedules the deref on the event loop, soBun.gc(true)followed byheapStats()in the same synchronous turn never observes it. An earlier ad-hoc probe of this exact scenario looked clean for that reason and for the unrelated reason that it only watched for ASAN crashes rather than growth. The committed test drains the loop between collections, and fails withReceived: 200on the unfixed build.Rebased onto be77b65 (main). Conflicts were with #33909 (
bun_core::err!→ per-cratethiserror::Errorenums) and #33193 (webstreams rewrite in C++); both resolved mechanically — the newcrate::Errorvariants andcrate::Result<()>in my code, and thereadableStreamToArrayBufferFFI point is unchanged and still wraps aThrowScope. All 19 new tests pass unmodified and still fail on stock bun.This PR still awaits @Jarred-Sumner's steer per the analysis above — rebased to keep it reviewable in the meantime.
[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file