Conversation
…SSink Fixes #14216 Fixes #11758 Fixes #19305 BufferOutputSink was the only caller of ValueBufferer, a bespoke "buffer a whole body into one slice" helper that rejected Source::JavaScript / Source::Direct streams outright and surfaced as ERR_STREAM_CANNOT_PIPE from transform(). Restructure BufferOutputSink so the rewriter's output target is a ByteStream (a separate allocation, so the rewriter never re-enters its owner and feed/finish/fail take &self) and the input is driven per chunk by a new HTMLRewriterInputSink JsSinkType via assign_to_stream, the same readStreamIntoSink pump fetch and S3 already use. Materialised bodies (string / ArrayBuffer / in-memory Blob) keep a synchronous fast path so transform(String) still returns a value synchronously. Handler errors are latched under a HandlerErrorScope RAII guard and re-thrown from get_pending_error on the next write/end/flush so the pump aborts instead of reading a never-closing source forever. Delete ValueBufferer (~420 lines) and its FFI / NativePromiseContext tag / PromiseFunctions / SinkHandle::ValueBufferer / crate::Error surface; html_rewriter was its only consumer. Net -62 lines. Supersedes #35324, which did the same restructure on top of ResumableSink before #36087 deleted that abstraction.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // on the stack via ensure_still_alive above). String/ArrayBuffer | ||
| // input took the synchronous `feed` path, so the output ByteStream | ||
| // is complete and `to_any_blob` drains it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// RAII guard installing `vm.unhandled_pending_rejection_to_capture` so | ||
| /// `handler_callback` / `create_lolhtml_error` can recover the original JS | ||
| /// error a handler threw (sync or via a rejected promise awaited by | ||
| /// `wait_for_promise`). Restores the previous capture slot and rejection | ||
| /// handler on drop. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Heap-boxed rewriter; `Cell` so `feed`/`fail`/`finish` can take `&self`. | ||
| /// The rewriter's output sink is `SinkRef(*mut ByteStream)` (a separate | ||
| /// allocation), so driving it never re-enters `BufferOutputSink`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// GC root for the output `ByteStream`'s JS wrapper. `SinkRef` writes into | ||
| /// the `ByteStream` pointed at by this stream's `Source::Bytes` payload. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// First error latched by [`Self::fail`]; read back by `init()` so a | ||
| /// synchronous handler error still makes `transform()` throw. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Output: a `ByteStream`-backed native ReadableStream. `SinkRef` writes | ||
| // rewritten chunks here; the returned Response's body wraps it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Route the input body to the rewriter. Materialised bodies | ||
| /// (String/ArrayBuffer/InternalBlob, and Blobs that do not need a file | ||
| /// read) feed the rewriter synchronously; everything else becomes a | ||
| /// `ReadableStream` pumped through `HTMLRewriterInputSink` via the | ||
| /// standard `assign_to_stream` JS pump, which accepts every stream source | ||
| /// kind (including `JavaScript`/`Direct`). | ||
| /// |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Called with an in-flight +1 on `self`; that ref is consumed by | ||
| /// `on_input_end` on every return-`Ok(())` path. On `Err` the caller's | ||
| /// `ScopedRef` releases it instead. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Deliberately no native `SinkHandle` fast path: `feed` drives | ||
| // `HtmlRewriter::write`, which runs async handlers via | ||
| // `wait_for_promise` (nested event loop). A ByteStream/FileReader | ||
| // push-pipe could deliver the next chunk while `write()` is still on | ||
| // the stack; the `readStreamIntoSink` JS pump is call-return | ||
| // sequenced so it cannot. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Feed one chunk to the rewriter. Copies first: lol-html tokenizes the | ||
| /// first chunk in place, and a handler that mutates or transfers the | ||
| /// source buffer would corrupt tokens past the cursor. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Terminal: `end()` the rewriter on success (flushes the final chunk to | ||
| /// the output ByteStream via `SinkRef`), or propagate `err` via `fail`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Latch the first error: destroy the rewriter, store `err` in `failed` | ||
| /// (for `init()` to throw synchronously), and push it into the output | ||
| /// ByteStream so `.text()`/`.body` reject. Idempotent. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// End-of-input: run `finish` under a `HandlerErrorScope` (so an `end()` | ||
| /// handler that throws is captured), then release the in-flight +1 taken | ||
| /// in `init()`. `self` must not be touched after this call. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Writes chunks to the output `ByteStream` (a separate allocation), so the | ||
| /// rewriter never re-enters `BufferOutputSink` and `feed`/`finish`/`fail` can | ||
| /// take `&self`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// JSSink driving a `ReadableStream` body into `BufferOutputSink::feed` per | ||
| /// chunk via the standard `assign_to_stream` pump. Not user-constructible. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Non-owning; the owning `BufferOutputSink` carries a +1 intrusive ref | ||
| /// (taken in `init()`) while this is `Some`. Cleared by the | ||
| /// assign_to_stream-result path before it releases that ref via | ||
| /// `on_input_end`; `finalize` releases it as a fallback. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `fail()` destroyed the rewriter; the latched error surfaces on | ||
| // the pump's next `write`/`end`/`flush` via `get_pending_error`, | ||
| // which throws it so `rsisAbrupt` cancels the source. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked both against this branch:
Re the comment-cop annotations: the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…; reject on undefined error Three findings from automated review, all correct: 1. `Box::leak(HTMLRewriterInputSink)` was never freed on any path: `end()` nulls `m_sinkPtr` before the controller destructor so `__finalize` never fires on the normal path, and `finalize()` did not self-free. Store the pointer on `BufferOutputSink.input_sink` and free it via `clear_input_sink()` (detach + heap::take) from `on_input_end` and `Drop`, mirroring `FetchTasklet::clear_sink`. `finalize()` self-frees as the GC-without-end fallback. 2. `on_reject_rewriter_input` mapped an `undefined`/`null` rejection to `on_input_end(None)`, closing the output as a truncated success. Pass `Some(err)` unconditionally (matching `on_reject_request_stream`). 3. `init()` read `failed` via `try_swap()`, clearing it; a still- Pending pump whose first sync chunk made a handler throw would then see `get_pending_error() == None` and keep reading forever. Read non-destructively. New tests cover (2) and (3).
| /// First error latched by [`Self::fail`]; read back non-destructively by | ||
| /// `init()` (sync throw) and `get_pending_error()` (pump abort). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Owned Box from `start_reading_input`; freed by [`Self::clear_input_sink`] | ||
| /// (the `FetchTasklet::clear_sink` pattern). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Reclaim the `Box<HTMLRewriterInputSink>` leaked in | ||
| /// `start_reading_input`: null the controller's `m_sinkPtr` via | ||
| /// [`JSSink::detach`] so `__finalize` cannot later touch the freed | ||
| /// allocation, then drop the Box. Idempotent. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// End-of-input: run `finish` under a `HandlerErrorScope` (so an `end()` | ||
| /// handler that throws is captured), free the input sink, then release | ||
| /// the in-flight +1 taken in `init()`. `self` must not be touched after. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Reached only when the controller is collected with `m_sinkPtr` still | ||
| // set, i.e. `clear_input_sink` never ran. Null the owner's slot so its | ||
| // `Drop` cannot double-free, release the in-flight +1, then self-free | ||
| // (the `ArrayBufferSink::finalize` pattern). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Pass the rejection through unconditionally: `controller.error()` | ||
| // with no argument rejects with `undefined`, which must still fail | ||
| // the transform rather than close the output as a truncated success. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
CI status for baad918
Not re-rolling: the All three automated review findings (Box leak, |
There was a problem hiding this comment.
The three findings from the previous pass (Box leak, failed cleared by try_swap letting the pump spin, controller.error(undefined) closing as truncated success) look addressed in baad918 — clear_input_sink + self-freeing finalize, non-destructive failed.get().get() in init(), and unconditional Some(err) in the reject handler, each with a covering test. No new issues surfaced this pass.
That said, this restructures BufferOutputSink's ownership model end-to-end (new JSSink type, output → ByteStream, deletes ValueBufferer and its SinkHandle/NativePromiseContext wiring), so it's worth a human pass on the overall design and the refcount/GC-root balance.
What was reviewed
HTMLRewriterInputSinkBox lifecycle acrossassign_to_streamcompletion /.then()reactions / GC finalize /clear_input_sink— no double-free or leak path found.SinkRef(*mut ByteStream)liveness: rooted viaBufferOutputSink::outputStrong for the rewriter's lifetime.start_reading_inputin-flight +1 balanced on everyOk(())arm viaon_input_end, and byScopedRefdrop on the?path.HandlerErrorScoperestoresprev_captureand rejection handler on drop; nested scopes (init → write_utf8) stack correctly.
Extended reasoning...
Overview
This PR restructures how HTMLRewriter.transform() consumes a Response body. It replaces the bespoke ValueBufferer (~420 lines, HTMLRewriter's only caller) with a new HTMLRewriterInputSink JsSinkType driven through the standard assign_to_stream pump, and changes the output from a Locked(PendingValue) body to a ByteStream-backed ReadableStream from the start. Touches 14 files: the core html_rewriter.rs restructure (~824 lines changed), Body.rs (-429), C++ JSSink codegen and ZigGlobalObject wiring, NativePromiseContext tag removal, SinkHandle::ValueBufferer variant removal, and 391 lines of new tests.
Security risks
None identified. HTMLRewriter processes user-provided HTML through lol-html; this PR does not change parsing or add any trust boundary. The feed() chunk copy defends against a handler mutating the source buffer mid-scan (covered by a test).
Level of scrutiny
High. This is memory-safety-critical native code — the most-blocked category per the repo's review guidance. It introduces a new refcounted object with a Box::leak + external-owner reclaim pattern, a raw *mut ByteStream output sink whose liveness depends on a GC Strong root, a new HandlerErrorScope RAII guard that swaps VM-global rejection state, and rewires how the in-flight +1 on BufferOutputSink is balanced across sync/async/error paths. The previous review pass found three real bugs here (a leak, a pump-spin after a cleared error latch, and a silent-truncation on error(undefined)); baad918 addresses each with a matching test, but the density of ownership transitions warrants a human confirming the overall model.
Other factors
- Two prior attempts at this restructure (#33310, #35324) were superseded; this is the third iteration on post-#36087 architecture, so there is design history a maintainer will have context on.
- The
comment-copbot flagged several long comments; robobun explained they are SAFETY/doc comments required by clippy lints, and the check itself passes. - Test coverage is thorough (21 new cases + 2 un-todo'd), passes under ASAN and
validateExceptionChecks, and includes GC-stress and leak probes. - The deliberate skip of the native
SinkHandlefast path (to avoid re-entrancy underwait_for_promise) is a design trade-off a maintainer should sign off on.
|
@robobun conflicts, rebase |
|
@robobun nvm |
2 participants
Fixes #14216
Fixes #11758
Fixes #19305
Fixes #6068
Repro
transform()throws for any Response whose body is a JS-created ReadableStream. The same body without the rewriter reads fine, 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_valuematched on the stream source kind and rejectedSource::JavaScript | Source::Directoutright.ValueBuffererwas a bespoke "read a whole body into one slice" helper whose only caller was HTMLRewriter. More fundamentally,BufferOutputSinkowned the lol-html rewriter and was its output target:SinkRef(*mut BufferOutputSink)wrote toself.bytesand calledself.done(), so every driver of the rewriter had to holdBufferOutputSinkas a root*mutto avoid re-entering its own&mut. That is whyrun_output_sinkonmaintakes*mut Selfandinit()is peppered with "do not hold&mut *sink" notes.Fix
BufferOutputSinkis restructured so the rewriter's output target is a separate allocation and the input is driven per chunk:Output =
ByteStream. The outputResponsebody is aByteStream-backedReadableStreamfrom the start;SinkRef(*mut ByteStream)writes chunks to it viaon_data, never back intoBufferOutputSink. The self-reference is gone, sofeed/finish/failtake&selfand the raw-pointer field-access pattern ininit()is gone. This also fixesS3Clientwrites empty file forHTMLRewritertransformed fetch Response #19305:.body.getReader(),Bun.readableStreamToText(body)andBun.servereturning a transformed response all read the sameByteStreamregardless of whether the input has settled.Input =
HTMLRewriterInputSink. A newJsSinkTypealongsideFetchRequestBodySink/NetworkSink.start_reading_inputdoesto_readable_stream()+JSSink::<HTMLRewriterInputSink>::assign_to_streamfor stream bodies (includingSource::JavaScript/Source::Direct/Source::Bytes/ file-backed blobs), and a short synchronous path for materialised bodies sotransform(String | ArrayBuffer)still returns a value synchronously.Value::Erroris handled synchronously sotransform()of an already-failed body still throws. The nativeSinkHandlefast path is deliberately skipped:feeddrivesHtmlRewriter::write, which runs async handlers viawait_for_promise(nested event loop); a ByteStream/FileReader push-pipe could deliver the next chunk whilewrite()is still on the stack, whereas thereadStreamIntoSinkJS pump is call-return sequenced.Per-chunk
rewriter.write().feed()copies the chunk beforewrite()because lol-html tokenises the first chunk straight from the input slice and a handler could otherwise mutate or transfer bytes it has not yet parsed.Error handling.
HandlerErrorScopeis an RAII guard pointingvm.unhandled_pending_rejection_to_captureat a local cell and installing the quiet rejection handler. Everyfeed/finishruns under one, socreate_lolhtml_errorrecovers the original JS error a handler threw on both sync and async paths.fail()latches the error in astrong::Optionalso a sync handler error insideinit()still makestransform()throw, pushes it to the outputByteStreamso.text()/.bodyreject, andget_pending_error()rethrows it on the pump's nextwrite/endsorsisAbruptaborts instead of reading a never-closing source forever.Deletions
Net -62 lines.
ValueBufferer(~420 lines ofBody.rs) and itsBun__BodyValueBufferer__*FFI /NativePromiseContext::Tag::BodyValueBufferer/PromiseFunctionsentries /SinkHandle::ValueBufferervariant.BufferOutputSinkfieldsbytes,response,response_value,body_value_bufferer,tmp_sync_errorand methodson_finished_buffering,run_output_sink,done,write,write_tmp_sync_error.JSSink<ArrayBufferSink>::detach_self.crate::Error::{UnsupportedStreamType, StreamAlreadyUsed, InvalidStream}.Verification
Tests land in
test/js/workerd/html-rewriter.test.js. 21 new cases cover single/multi/mixed-chunk JS streams,type: "direct", a stream that only produces aftertransform()returns, every consumption path (.text(),.arrayBuffer(),.bytes(),.blob(),.json(),.body.getReader(),Bun.readableStreamToText), handlers observing the document, upstream errors before and aftertransform()returns, a bad chunk type surfacing itsTypeError, reuse of a consumed source, a handler mutating/transferring the source buffer mid-scan, aggressive GC while the source is in flight,.bodyof a transform whose source is still pending, stopping the pump once a handler throws, a handler-error leak probe, and theBun.serveshape from #11758. The twoit.todo("works with payload of type direct" / "default")cases were todo for this reason and are un-skipped.The
transform rejects when the upstream body fails > .body ...tests are adapted for streaming: chunks delivered before the failure now reach.body, so the assertions read to completion instead of expecting a single rejected.read().Relationship to #35324 / #33310
#33310 routed the same arm through
readableStreamToArrayBufferand was blocked on the grounds that HTMLRewriter should stop buffering. #35324 did this same restructure on top ofResumableSink, then #36087 deletedResumableSinkin favour of theJsSinkTypefamily. This PR is #35324 redone on the post-#36087 architecture; the test coverage from that PR is carried over.[review] gate passed · iteration 0 · 14 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file