Repository navigation
HTMLRewriter: hold handlers in a visited slot instead of protecting them - #43210
Conversation
on() and onDocument() called gcProtect() on every callback and handler object, and kept them protected for as long as the rewriter or any transform made from it was alive. A handler that reached its own rewriter, or the Response of its own transform, closed a cycle through a GC root, so none of it was ever collected. The callbacks and handler objects now live in a JS array held by a visited slot of the HTMLRewriter wrapper and of each transform cell. The native handler structs keep only their positions in that array. drive_rewriter() keeps the transform cell alive across each lol-html call. A handler can cancel the output reader, which drops every other path to the cell, while lol-html still has handlers to run for the rest of the chunk. A collection in that window swept the cell, and the next handler to throw recorded its error on a null cell. handler_callback() makes the native wrapper after its early returns, so a handler that is not called does not strand the wrapper's initial ref.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughHTMLRewriter now stores callbacks and handler objects in shared slots. Transforms retain the handler list while active and release it after completion, failure, or cancellation. Tests cover callback lifetime, garbage collection, array interception, and cancellation behavior. ChangesHTMLRewriter handler retention
Suggested reviewers: Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains after the lifecycle and regression coverage changes. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:32 PM PT - Sep 17th, 2026
⏳ @robobun, your commit 1b61630 is still building in
|
|
Status: the fix is pushed. CI runs now. How to reproduce the leak (1.4.2, 1.4.3 canary and main before this PR): let fired = 0;
const fr = new FinalizationRegistry(() => fired++);
class Counter {
count = 0;
constructor() {
this.rw = new HTMLRewriter().on("p", { element: () => void this.count++ });
}
}
async function once() {
const c = new Counter();
await c.rw.transform(new Response("<p>x</p>")).text();
fr.register(c.rw, 0);
}
for (let i = 0; i < 5000; i++) await once();
for (let i = 0; i < 6; i++) { Bun.gc(true); await Bun.sleep(50); }
const { protectedObjectTypeCounts } = require("bun:jsc").heapStats();
console.log("retained", 5000 - fired, "of 5000, protected Function", protectedObjectTypeCounts.Function ?? 0);
How to reproduce the crash: run the last test of Tests: One suspicion, not part of this diff and not reproduced: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked that HandlerSlot indexes cannot resolve against the wrong array when one handler synchronously drives another rewriter's pipe — handler_callback reads the list from the sink whose lol-html call is on the stack, and that cell's handlers slot is the same array the handler was appended to. I also checked the HTMLRewriterTransform cell liveness that drive_rewriter now relies on: every entry into the pipe holds the cell (source sinkOwner, output owner, pump promise, NativePromiseContext, or the Response on the stack in init), so the is_cell() check in handler_list() is not the only guard.
Extended reasoning...
Two verified findings are already posted inline (the remaining onEndTag leak through the output Response, and transform cells retaining the handler array after finish()), so a human look is needed regardless. This note only records two adjacent concerns examined and ruled out: nested-pipe slot confusion in handler_callback (the list is read from the active pipe's own cell, which received the same array from begin_transform) and the cell-liveness assumption in drive_rewriter/handler_list() (every path into the pipe holds the cell before the lol-html call starts). The temporaries held on the Rust stack between callback_property and HandlerList::append are ordinary native-stack JSValues, which is the standard rooting for values within a single call, so that was not treated as a defect.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
A transform cell holds the handler list so that the handlers outlive a collected rewriter. The cell kept the list after its rewrite finished, failed or was cancelled, so an output Response that script keeps (a cache, a route table) retained every handler object and whatever its callbacks close over. The cell's slot is cleared once the rewrite is over and no lol-html call is on the stack. A handler that cancels the output mid-chunk defers the release to the end of that call, because lol-html still runs the handlers for the rest of the chunk.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Each new comment is one line, and two existing comments are back to their original text.
|
Review follow-up:
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The follow-up push clears the transform cell's handlers slot on the finish/fail/cancel paths, which covers the retention I raised on the first version. Beyond the inline nit, I also checked the new release_handlers deferral against the async-handler resume path: resume_rewrite re-enters lol-html through drive_rewriter, so driving is set while the remaining handlers of that chunk run and the list is only dropped after the call returns — no premature clear there.
Extended reasoning...
Since the prior review, commit 726f080 added release_handlers() (src/runtime/api/html_rewriter.rs:1596) and wired it into cancel_from_output, finish, fail, and the post-drive check in drive_rewriter. The one case worth ruling out was a Done transition that happens while lol-html still has handlers to run for the current chunk but outside drive_rewriter's driving window; the suspended-then-resumed path is the candidate, and it goes through drive_rewriter(|r| r.resume()) at :1734, so the deferral applies. The onEndTag ProtectedJSValue remains as before, which the PR's notes explicitly scope out. One verified finding was not posted this run and the change is a non-trivial GC-lifetime refactor, so a human look is still warranted.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
The case waited on a promise that only the second handler call resolves, so a regression would time out. It now polls a bounded number of event loop turns and asserts the call count. reader.closed cannot signal the failure: it settles inside the first handler call, before the second one runs.
|
Second review round:
|
There was a problem hiding this comment.
I re-reviewed the latest push (the bounded poll in the handler-cancel case) and found no new bugs; since this replaces the GC rooting model for every HTMLRewriter handler in a re-entrant native pipe, a human look at the lifetime design is still worthwhile.
What was reviewed this run:
- Handler lookup path: the list is read from the active pipe's cell, which
drive_rewriternow pins across the lol-html call;release_handlersis deferred whiledrivingand every driver (write,resume,resume_rewrite,end_from_stream) gates onphase == Done, so no handler runs after the slot is cleared. - Nested
transform()from inside a handler: both transform cells share the rewriter's array, so the sameHandlerSlotindices resolve on whichever pipe is active. - Array access:
put_index/get_direct_indexareputDirectIndex/getDirectIndex(own properties) andget_lengthreadsJSArray::lengthdirectly, soArray.prototypeaccessors cannot intercept a handler; a partialappendfailure ininitleaves orphan entries but never a mismatched index.
Extended reasoning...
Overview
The PR replaces ProtectedJSValue (gcProtect roots) on ElementHandler/DocumentHandler with HandlerSlot(u32) indexes into a JS array held in a new visited handlers WriteBarrier slot on both the HTMLRewriter wrapper and each HTMLRewriterTransform cell (src/runtime/api/html_rewriter.classes.ts, src/runtime/api/html_rewriter.rs). handler_callback resolves callbacks through sink.handler_list() on the active pipe's cell, the wrapper Z::init was moved after the early returns so the first ref only ever goes to to_js, drive_rewriter captures the cell and ensure_still_alives it after the drive, and release_handlers clears the transform cell's slot on every terminal path once no lol-html call is on the stack. EndTagHandler keeps its ProtectedJSValue. The test file adds two describe blocks of leak-shape tests plus an Array.prototype accessor test and a spawned crash repro.
Security risks
None specific to this change: no new user-facing input parsing, no network or filesystem paths. The one hostile-userland angle (an Array.prototype indexed accessor observing the handler array) is closed by using putDirectIndex/getDirectIndex and a direct JSArray::length read, and the PR includes a test for it.
Level of scrutiny
High. This is a GC-lifetime redesign in a re-entrant native pipe: handlers now stay alive only by edges from wrapper cells, and the correctness of every path depends on the transform cell being reachable whenever lol-html can still call a handler. I traced the drivers (write, resume, resume_rewrite, end_from_stream, end_rewrite) and confirmed each gates on phase == Done or a null rewriter, that release_handlers defers while driving and is re-run by drive_rewriter, and that the suspension path (cancel during a pending handler promise) clears the slot before resume_rewrite would run and then bails on phase == Done. The nested-transform case is handled because both cells hold the same array object. The remaining reasoning (that every entry into the pipe comes from something holding the cell) is an argument, not a compiler-enforced invariant, and is the kind of thing a maintainer familiar with the pipe should weigh; the PR's own notes list it as an unverified-by-construction assumption.
Other factors
This is the third pass of this review; the earlier inline notes (handlers retained by a long-lived output Response; the onEndTag protect) were addressed by commit 726f080 and explicitly scoped out respectively, and the last push (78c3243) addressed the bounded-wait nit. The new tests use bounded setImmediate polling rather than sleeps, drain subprocess pipes concurrently, and use heapStats object/protected counts with N/4 bounds well below the unfixed N-of-N leak. No CODEOWNERS entry covers the changed files. The bug hunt exited on a dry streak with no findings.
HandlerList::get() returns the JsResult. handler_callback() stops the rewrite on Err and leaves the pending termination in place, the same as when a termination stops the callback itself. Fixes the jsresult-swallow source lint.
…ng the streams of a collected rewrite (#43379) ### Problem - `element.onEndTag(fn)` makes `fn` a GC root (`ProtectedJSValue` in `EndTagHandler`, `src/runtime/api/html_rewriter.rs`). If `fn` reaches the output `Response` and the rewrite stops early, 200 of 200 Responses stay. - Main also uses the streams of a collected rewrite, in four places. `cancel_from_output()` and `abandon_suspension()` close a dead JS input (`SEGV` in `JSReadStreamIntoSinkOperation::result()`, or `ASSERTION FAILED: status() == Status::Pending`). `abandon_suspension()` writes to a freed output (`heap-use-after-free` in `ByteStream::on_data`). `Bun.ModuleGraph.dispose()` cancels a stream source that is dead, or that is freed under the call (a segfault on a release build with no options). The leak hid the first. ### Fix - `onEndTag` callbacks live in a JS array in `endTagHandlers`, a new visited slot of the transform cell. The lol-html handler keeps an index. - `cancel_from_output()` cuts the output's edge to the transform cell last, as `fail()` and `finish()` already do. - The pipe's reference to its own cell is a `JsRef` instead of a bare `JSValue`, so a dead, unswept cell reads as `None`. - The two entries that nothing reachable makes, the `abandon_suspension()` task and `NewSource`'s `on_abort` (`dispose()`), hold the wrapper that they read for the call, and leave a collected one to its finalizer. - Verified: 23 new tests (`test/js/workerd/html-rewriter-leak.test.ts`, `module-graph-gc.test.ts`). 11 fail on a debug build of main, 13 on release ASAN. Other suites: Notes. Self-reviewed: 3 concerns raised, 3 addressed. ### Background - `gcProtect` makes a value a GC root, so a cycle through it is never garbage. A visited slot is an edge that the collector traces. - The transform cell (`HTMLRewriterTransform`) keeps the input stream of one `transform()` alive. The native pipe holds that stream as a raw pointer. - Considered the cell in the `PipePin` guard of every entry point (an earlier version of this PR), and a `Strong` on the input (a root per rewrite). Notes say why not. ### Downsides - `Element` grows from 40 to 48 bytes, `RewriterPipe` from 272 to 304, the release binary by 8,960 bytes. - A handler without `onEndTag()` costs the same within noise (150 ns per element, base-to-base difference up to 1.2 ns). With it, 41 to 48 ns less. - Two leaks without `onEndTag()` remain (Notes). <details><summary>Notes</summary> **Report.** There is no GitHub issue. #43210 measured the leak and left it for a follow-up, and its review asked for it: #43210 (comment). This is the last `ProtectedJSValue` in `html_rewriter.rs`. **Why a visited slot.** `REVIEW.md` ("Root or copy every JSValue held beyond the current call") asks for WriteBarrier members declared in `.classes.ts`, and keeps `protect` for a justified self-keepalive. #43210 applied that to the `handlers` slot. The third shape below also needs it: a rewrite whose input never ends reaches no terminal state, so only collection of the transform cell can free it, and a root prevents that collection. **Repro of the leak.** `MODE` is `ok`, `throw` or `cancel`. The `</div>` end tag never arrives. ```js const { heapStats } = require("bun:jsc"); let fired = 0; const fr = new FinalizationRegistry(() => fired++); const N = 200; const mode = process.env.MODE ?? "throw"; async function once() { const holder = {}; const rw = new HTMLRewriter() .on("div", { element(el) { el.onEndTag(() => holder.res); } }) .on("p", { element() { if (mode === "throw") throw new Error("boom"); } }); let ctrl; const res = rw.transform(new Response(new ReadableStream({ start(c) { ctrl = c; } }))); holder.res = res; fr.register(res, 0); const chunk = new TextEncoder().encode("<div><p>x</p>"); if (mode === "cancel") { const reader = res.body.getReader(); ctrl.enqueue(chunk); await reader.read(); await reader.cancel(); } else { ctrl.enqueue(chunk); ctrl.close(); try { await res.text(); } catch {} } } for (let i = 0; i < N; i++) await once(); for (let i = 0; i < 6; i++) { Bun.gc(true); await Bun.sleep(20); } console.log(mode, "retained", N - fired, "of", N, "protected Function", heapStats().protectedObjectTypeCounts.Function ?? 0); ``` **Measurements of the leak** (debug builds, retained Responses and protected Functions of 200, graphs of 15): | shape | main (367d939) | this PR | | --- | --- | --- | | rewrite completes | 0, 0 | 0, 0 | | a later handler throws, the body is read | 200, 200 | 0, 0 | | the output reader cancels | 200, 200 | 0, 0 | | the input never ends, all dropped, nothing pending on the output | 200, 200 | 0, 0 | | disposed `Bun.ModuleGraph` whose code left such a rewrite | 15 graphs | 1 graph | | the same graph code without `onEndTag()` (control) | 1 graph | 1 graph | Release builds of the merge base 37471e5 and of this PR give the same result for `throw` and `cancel`: 200 of 200, and 0 of 200. On main the heap snapshot of the new ModuleGraph test names the retainer: `root(ProtectedValues) Function -> JSLexicalEnvironment -> JSModuleEnvironment -> JSLexicalEnvironment -Variable:moduleGraph-> ModuleGraph`. **The dead input, case 1: `cancel_from_output()`.** An earlier version of this PR failed on the x64 ASAN lane: the new test "when the output reader is cancelled" aborted with `ASSERTION FAILED: status() == Status::Pending`. The cause is on main. `cancel_from_output()` starts with `detach_output()`, which clears the `owner` slot of the output stream. If script holds only the reader, that slot was the last GC path to the transform cell. The next step allocates the abort reason, so a collection can finish there. `detach_input_source()` then closes the sink controller of the JS input through `SourceHandle::JSController`, a raw pointer to a cell that only the transform cell kept alive. On main the protected `onEndTag` callback kept such a rewrite alive forever, so the window was closed for exactly the rewrites that the leak tests make. Main has the bug with no `onEndTag()` call. This script requests a collection before each `reader.cancel()`: ```js const encoder = new TextEncoder(); const N = 200; let cancelled = 0; for (let i = 0; i < N; i++) { let controller; const input = new ReadableStream({ start: c => void (controller = c), cancel() { cancelled++; } }); const reader = new HTMLRewriter().on("div", { element() {} }).transform(new Response(input)).body.getReader(); controller.enqueue(encoder.encode("<div><p>x</p>")); controller = undefined; if ((await reader.read()).done) throw new Error("done early"); Bun.gc(false); await reader.cancel(); } Bun.gc(true); console.log(JSON.stringify({ rewrites: N, cancelled })); ``` | release ASAN build | main | this PR | | --- | --- | --- | | plain run, 3 runs | `cancelled` is 195, 194, 200 of 200 | 200 of 200 | | `BUN_JSC_collectContinuously=1` | `SEGV on unknown address 0x000000000010` | 200 of 200 | The stack of the SEGV: `JSC::ClassInfo::isSubClassOf` < `JSReadStreamIntoSinkOperation::result()` < `Bun::WebStreams::rsisFinish` < `jsWebStreamsHandler_onReadStreamIntoSinkClose` < `JSC::runInternalMicrotask`. The failure needs a collection that finishes between `detach_output()` and the close of the input. It did not occur on a debug build or on a release build without ASAN. The regression test runs the script under `BUN_JSC_collectContinuously=1` on release builds (not on Windows, where that option is very slow), so it fails on main only on the release ASAN lanes. **The fix for case 1.** `detach_output()` moves to the end of `cancel_from_output()`, next to `release_input_roots()`. The reader that cancels then reaches the cell through the `owner` slot until the input is closed. This is the order of `fail()` and `finish()`, and the rule that the doc comment of `release_input_roots()` already states for the input's edges. Alternatives: - Keep the cell on the stack in the `PipePin` of `write`, `end_from_stream`, `resume`, `cancel_from_output` and `fail` (an earlier version of this PR). Each of those is entered by a peer whose own cell has an internal edge to the transform cell (`owner`, `sinkOwner`, the Response's `transform` slot, the context of a promise reaction). The guard mattered where the pipe itself cut that edge too early (this case), and where the first caller of the chain held nothing (cases 3 and 4). It does nothing for a cell that is already dead at the entry, so that version still crashes in case 4. It also made `EnsureStillAlive` a struct field. Everywhere else in the tree it is a local. - A `Strong` on the controller in `SourceHandle::JSController`. That is one more root per rewrite with a JS input, and a root is what made the leak. **The dead input, case 2: `abandon_suspension()`.** A review of this PR found it. It is on main too. A handler returns a promise that never settles, and script drops everything. The collector then frees the promise, and the destructor of its native context queues `abandon_suspension`. That task asked `cell.is_cell()` to learn if the transform cell is alive. The pipe's `cell` field was a bare `JSValue` that the cell's finalizer clears, and the finalizer runs at the sweep. A cell that is dead but not swept yet still passes `is_cell()`, so the task called `fail()`, and `fail()` closed the dead sink controller. That field is a hand-written weak reference. `JsRef` is the one the rest of the runtime uses for a native object's own wrapper (the stream sources next to this pipe do), and since #39334 its `try_get()` gives `None` for a dead, unswept cell, which is the answer of `JSC::Weak::get()`. The field is now a `JsCell<JsRef>`, and every read of it goes through `try_get()`. The task clears the input and output handles without a call into them when that is `None`, as it already did for a swept cell. ```js const N = 50; const encoder = new TextEncoder(); const tick = () => new Promise(resolve => setImmediate(resolve)); let cancelled = 0; for (let i = 0; i < N; i++) { let controller; const input = new ReadableStream({ start: c => void (controller = c), cancel: () => void cancelled++ }); new HTMLRewriter().on("p", { element: () => new Promise(() => {}) }).transform(new Response(input)); controller.enqueue(encoder.encode("<p>x</p>")); } for (let i = 0; i < 5; i++) await tick(); for (let round = 0; round < 10; round++) { Bun.gc(false); const junk = []; for (let j = 0; j < 500; j++) junk.push({ j }); await tick(); } process.stdout.write(JSON.stringify({ rewrites: N, cancelled })); ``` Run it as a file. A release build of main exits with `panic(main thread): Segmentation fault at address 0x0` in 10 of 10 runs. A debug build of main stops at `ASSERTION FAILED: decontaminate()`. A release ASAN build of main reports the same `SEGV` as case 1, with this stack: `JSReadStreamIntoSinkOperation::result()` < `rsisFinish` < `pumpOnClose` < `sinkControllerOnClose` < `SourceHandle::cancel` < `RewriterPipe::fail` < `RewriterPipe::abandon_suspension`. This PR prints `{"rewrites":50,"cancelled":0}` on all three builds. Bun 1.4.2 does not crash on this script. #37108 fixed the opposite direction, a controller destructor that reached a freed pipe. **The freed output, case 3: `abandon_suspension()` again.** It is on main too. The handler's promise is collected while script still holds the `Response`, so the task is queued for a reachable rewrite. Script drops the `Response` before the task runs. The cell is then unreachable, but no collection has found that out, so it reads as alive. `fail()` allocates (the error, the abort of the input), and a collection in there sweeps the cell and both streams. `fail()` then writes the error through `output`, a raw pointer to the freed `ByteStream`. This entry is a task that a destructor queued, so nothing on its stack reaches the cell. It keeps the cell that it read in a local `EnsureStillAlive` until it returns. ```js const encoder = new TextEncoder(); const tick = () => new Promise(resolve => setImmediate(resolve)); const responses = [], promises = []; function start() { let controller; const input = new ReadableStream({ start: c => void (controller = c) }); const response = new HTMLRewriter() .on("p", { element() { const p = new Promise(() => {}); promises.push(p); return p; } }) .transform(new Response(input)); response.body; responses.push(response); controller.enqueue(encoder.encode("<p>x</p>")); } for (let round = 0; round < 20; round++) { for (let i = 0; i < 4; i++) start(); await tick(); promises.length = 0; Bun.gc(true); responses.length = 0; await tick(); } ``` With `BUN_JSC_slowPathAllocsBetweenGCs=25` (a full collection at every 25th slow-path allocation) a release ASAN build of main reports the `heap-use-after-free` in 10 of 10 runs, and also for each of 7, 13, 20, 33, 50, 64, 80, 100 and 150. This PR exits with 0. A debug build and a release build without ASAN of main do not report it. **The dead or freed source, case 4: `Bun.ModuleGraph.dispose()`.** It is on main too, and needs no `onEndTag()`. `dispose()` cancels the source of every stream that the graph's script was given (`NewSource`'s `on_abort`, #42590), from native code. The JS wrapper owns the source, and nothing on that stack reaches a wrapper that script has dropped. `AbortHandleOwner` has a `KeepAlive` type for what keeps the owner alive while `on_abort` runs. `NewSource` declared none. - A collection that finishes inside `cancel()` frees the source under the call, together with the rewrite that feeds it. - After a collection that finished before, the wrapper is dead and waits for its sweep. `cancel()` then tells the rewrite, which closes its dead input. `NewSource` now keeps its wrapper (`this_jsvalue.try_get()`) alive for the call. For a wrapper that is collected and not swept yet it does nothing: the peers are collected too, and the finalizer releases the source. A source whose wrapper is finalized while native refs remain (a child's pipe that nobody reads) is cancelled as before. ```js const TICKS = 0; // or 1 const encoder = new TextEncoder(); const tick = () => new Promise(resolve => setImmediate(resolve)); const rewrite = input => new HTMLRewriter().on("div", { element() {} }).transform(input); function start(chained) { for (let i = 0; i < 20; i++) { const input = new ReadableStream({ start: c => void c.enqueue(encoder.encode("<div>x")), cancel() {} }); const output = rewrite(new Response(input)); (chained ? rewrite(output) : output).body; } } for (const chained of [false, true]) { for (let i = 0; i < 20; i++) { const graph = new Bun.ModuleGraph(); graph.run(() => start(chained)); await tick(); Bun.gc(false); for (let t = 0; t < TICKS; t++) await tick(); graph.dispose(); } } ``` Run it as a file, with no options. 5 runs for each value of `TICKS`: | build | `TICKS = 0` | `TICKS = 1` | | --- | --- | --- | | main, release | segfault, 5 of 5 | segfault or abort, 5 of 5 | | main, debug | `ASSERTION FAILED: decontaminate()`, 5 of 5 | the same, 5 of 5 | | main, release ASAN | `decontaminate()` or `SEGV`, 5 of 5 | `SEGV`, 5 of 5 | | the version of this PR with the cell in `PipePin`, release ASAN | `ASSERTION FAILED: result`, 5 of 5 | `SEGV` or `decontaminate()`, 5 of 5 | | this PR, release ASAN | passes, 5 of 5 | passes, 5 of 5 | **Each of the changes is needed.** Release ASAN builds, the repros of cases 1 to 3: | build | case 1 (`slowPathAllocsBetweenGCs=1`, 10 rewrites) | case 2 | case 3 | | --- | --- | --- | --- | | main | `cancelled` is 0 of 10 | `SEGV` | `heap-use-after-free` | | this PR with only the `JsRef` | `cancelled` is 0 of 10 | passes | `heap-use-after-free` | | this PR | 10 of 10 | passes | passes | **Why `Element` has a back reference.** `on()` handlers find their list through the pipe whose lol-html call is on the stack. `onEndTag()` cannot: after an `await` in an async handler no lol-html call is on the stack, and inside a handler of a nested, synchronous rewrite the pipe on the stack is the inner one. Both cases work on main and have a test here. `handler_callback` gives the pipe to the wrapper when it makes it. `Element::invalidate()` clears it together with the lol-html pointer. That happens when the handler returns, or when the pipe drops the wrapper it parked, so the reference never outlives the pipe. **Replacement.** A second `onEndTag()` on the same element replaces the first callback, in one handler or across two `on()` handlers. That is the behavior of main (`handlers.clear()` then push). lol-html gives every matching handler the same `Element`, and a suspension moves the user data with the parked copy. So the first call pushes the one lol-html handler and records its slot as the element's user data, and a later call only stores the new callback into that slot. **A parked element that outlives its transform cell.** A handler returns a promise, the element is parked, then the promise and the cell are collected together. Until the queued `abandon_suspension` task runs, script can still call `onEndTag()` on the parked element. The cell is then swept, or dead and not swept yet. In both states `try_get()` is `None`, and `hold_end_tag_callback` stores nothing. It also stores nothing for a parked element whose rewrite was cancelled or failed. In all of these the end tag can never come. The last test in the file covers the collected case. **Return value.** Unchanged: the element while it is attached, `null` once it is detached. **Sizes.** `size_of` in the release profile, merge base then this PR: `Element` 40, 48 (one per element handler call, freed when the handler returns). `RewriterPipe` 272, 304 (one per `transform()`, alignment 16). `EndTagHandler` 24, 16. The transform cell gets one more `WriteBarrier` (8 bytes). The first `onEndTag()` call on an element now boxes a 4-byte index as the lol-html user data, and no longer adds an entry to the VM's table of protected values. `size` of the linux-x64 release binaries: text 80,681,674, then 80,690,634. Data and bss do not change. **Speed.** Release builds of the merge base and of this PR, linux-x64, pinned to one core. One process rewrites a document of 20,000 elements with one element handler, and reports the best of 40 runs in nanoseconds per element. 25 rounds run the processes in turn: base, PR, base again. The table gives the minimum of the rounds, and the median in parentheses. The third column is the same base binary, which shows the noise. | case | base | this PR | base again | | --- | --- | --- | --- | | `<p>x</p>` x 20,000, handler only | 150.1 (154.9) | 150.9 (155.7) | 151.3 (157.7) | | `<p>x</p>` x 20,000, handler calls `onEndTag()` | 298.3 (306.2) | 257.6 (266.8) | 298.0 (306.7) | | `<div>` nested 50 deep x 400, handler only | 150.1 (154.2) | 150.5 (154.4) | 150.1 (154.5) | | `<div>` nested 50 deep x 400, handler calls `onEndTag()` | 322.1 (331.4) | 274.6 (280.6) | 326.8 (332.7) | `try_get()` is one call into C++ per handler call. The version of this PR without it measured 151.0, 259.8, 151.9 and 273.8 in the same rounds. **Not changed.** Each of these keeps a rewrite alive on main and on this PR with no `onEndTag()` call at all (100 rewrites each): - A failed rewrite keeps the handler's error in the output body as a `Strong` until the body is read (`fail()`). If that error references the output `Response` (for example `error.response = res`) and nothing reads the body, 100 of 100 Responses stay. With the body read, 0 stay. An error that does not reference the Response retains nothing. - A `read()` that is pending on the output while the JS input stream can never produce again keeps 100 of 100 Responses, through the protected promise and buffer of the pending pull. A pending `.text()` leaves 100 protected Promises (`promise_value.protect()`, `src/runtime/webcore/Body.rs:453`). - A rewrite that fails or is cancelled keeps its lol-html rewriter (the parser arena) until the transform cell is collected. Only `finish()` frees it earlier. A parked wrapper can still point into it, so an earlier free needs its own change. **Tests.** 23 new tests. On a debug build of main 11 fail: the three leak shapes, four of the five kept-Response shapes, the parked rewrite that dies with its handler promise, the two `dispose()` tests of case 4, and the ModuleGraph test. On a release ASAN build of main the cancel test under continuous collection and the test of case 3 fail too. The other tests guard the new storage and pass before the fix: the callback stays alive until its end tag with nothing else referencing it (GC churn in between, half of the outputs dropped), slot reuse under nesting, one end tag that closes several elements, callbacks that run after a handler cancelled the output earlier in the same chunk, replacement (one handler, two handlers, across a suspension), `onEndTag()` after an `await`, an outer element used in a nested rewrite, an indexed accessor on `Array.prototype`, the parked element whose rewrite was collected, and the kept-Response case of a rewrite that completes. **Suites run on the debug (ASAN) build:** `test/js/workerd/html-rewriter-leak.test.ts` (49 pass, 1 skipped in debug), `test/js/bun/module-graph/module-graph-gc.test.ts` (33 pass), `test/js/workerd/html-rewriter.test.js` (186 pass), `test/js/workerd/html-rewriter-end-error.test.ts`, `test/js/web/html/html-rewriter-doctype.test.ts`, `test/regression/issue/htmlrewriter-additional-bugs.test.ts`, `test/regression/issue/text-chunk-null-access.test.ts`, `test/js/bun/http/serve-stream-body-error.test.ts`, and the HTMLRewriter cases of `test/js/web/workers/worker-terminate-lifetime.test.ts` and `test/js/bun/module-graph/module-graph-isolation.test.ts`. On the release ASAN build: the leak file with the CI environment of the ASAN lane (`BUN_JSC_validateExceptionChecks=1`, `BUN_GARBAGE_COLLECTOR_LEVEL=1`) and `html-rewriter.test.js`, 3 runs. Also run by hand, on the earlier version with the cell in `PipePin`: a nested document with `Bun.gc(true)` in handlers and callbacks under `BUN_JSC_collectContinuously=1`, and `terminate()` of workers that hold pending callbacks in each state. **A test of this file that is slow on main.** "HTMLRewriter does not leak element/document handler allocations" passes 15 s on a loaded release ASAN build, on main and on this PR. #44359 and #44377 change that test. **Self-review.** The review of the first commit asked for the ModuleGraph test, for the narrower wording of the measured shapes, and for the "Not changed" list. All three are in. </details> <!-- robobun:evidence:begin --> --- **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/module-graph/module-graph-gc.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
2 participants
Problem
HTMLRewriterthat one of its own handlers can reach is never collected. 5,000 rewriters shaped likethis.rw = new HTMLRewriter().on("p", { element: () => this.count++ })leave 5,000 rewriters and 5,000 protected functions alive. A handler that reaches the transformedResponseleaks the same way.ElementHandler::initandDocumentHandler::init(src/runtime/api/html_rewriter.rs)gcProtecteach callback and handler object for the life of the rewriter and of each transform.Segmentation fault at address 0x40. A handler cancels the output reader, a collection sweeps the transform cell, and a later handler of that chunk throws.Fix
handlersslot on the rewriter's wrapper and on each transform cell holds it. Native structs keep indexes, as inJSSocketHandlers(socket: hold Bun.listen/Bun.connect callbacks in a GC-visited internal-fields cell #31859).Responsedoes not retain the handlers.drive_rewriter()keeps the transform cell alive across each lol-html call. Handlers are read from it and record errors on it.test/js/workerd/html-rewriter-leak.test.ts(13 new tests, 10 fail without the fix) and all oftest/js/workerd/. Self-reviewed: 16 concerns, 11 addressed, 5 explained in Notes.Background
gcProtectmakes a value a GC root. A cycle through a root is never garbage.values: [...]in a.classes.tsfile) is a wrapper field thatvisitChildrenreports to the collector.HTMLRewriterTransform) is the GC owner of onetransform()call. The outputResponseor the input source keeps it alive.Notes
Report. Found by a leak fuzzer (reference-cycle round, no GitHub issue). The result is the same on 1.4.2 and on main. Retainer chain from the report:
<root> -> Function -> JSLexicalEnvironment -> Counter -> HTMLRewriter.Measurements (N = 100 per shape,
heapStats()afterBun.gc(true), retained objects and protected functions):ResponseResponsekept after its rewrite is over (5 cases, N = 60)The last shape has no cycle through the rewriter. The pipe of each transform shares the native handler registry, so the callbacks stayed protected for as long as the transform cell lived, and the callbacks reached the cell through the
Response.The crash.
cancel_from_output()clears the output stream'sownerslot and the input source'ssinkOwnerslot. If script dropped the outputResponseand kept only the reader, only the native stack reaches the cell after that.PipePinalready keeps the native pipe alive for this case (see the test "cancelling the output from a handler mid-chunk does not free the pipe under its caller"), butrecord_handler_errorstill wrote toself.cell, which the finalizer had set to zero. Release build:Segmentation fault at address 0x40. Debug build:JSCast.h:182:38: runtime error: member call on null pointer of type 'JSC::JSCell'. The leak fix needs the cell alive across the lol-html call in any case, because the handler list is read from the cell.When a transform lets go. A transform cell clears its
handlersslot oncephase == Doneand no lol-html call is on the stack (release_handlers(), fromfinish(),fail(),cancel_from_output()and the end ofdrive_rewriter()). From then on nothing can start a lol-html call on that pipe, so no handler runs again. If a handler cancels the output mid-chunk, the release waits for the end of that call, because lol-html still runs the handlers for the rest of the chunk. This came from review: before it, a cached outputResponseretained every handler object and closure for as long as it lived (the same as on main, where they were protected for the life of the pipe).Why both wrappers hold the array.
new HTMLRewriter().on(...).transform(res)drops the rewriter at once, and the rewrite continues as input arrives. The transform cell keeps the handlers alive then. A rewriter that is kept for a latertransform()keeps them alive through its own slot. Both slots hold the same array, so a handler added after a transform started is also kept alive by that transform. The native registry is shared the same way.Handler lookup.
handler_callbackreads the list from the transform cell of the pipe whose lol-html call is on the stack. Rust keeps no unrooted copy of a callback. If a load fails (a pending termination), the handler reportsStop, the same as a terminated handler. The lookup runs before the native wrapper (Element,TextChunk, ...) is made, because onlyto_jshands the wrapper's first ref to an owner. Cost per handler call: one slot read on the cell and two own-index reads on a dense array. An empty element handler costs about 215 ns per call on a release build today.Not changed.
element.onEndTag(fn)still protectsfn. The callback is one-shot, and lol-html drops it when it runs or when the lol-html rewriter is dropped. A related leak remains there: iffnreaches the outputResponseand the rewrite throws or is cancelled before the end tag, the lol-html rewriter lives until the pipe is dropped, the pipe lives until the cell is collected, andfnroots the cell (200 of 200 retained in a probe, before and after this change). That callback has a different owner and lifetime (one rewrite, registrations that come and go), so it needs its own change.Self-review, the 5 concerns left as they are.
Stopandtake_handler_errorthen reads a null cell. No path does this: each entry into the pipe comes from something that holds the cell (the source'ssinkOwnerslot, the output stream'sownerslot, the pump promise reaction, theNativePromiseContext, theprotect()of a background pull, or theResponseon the stack ininit), anddrive_rewriternow holds it to the end of the call. Debug builds assert it.handler_list()usesis_cell(), which is also true for a cell that is dead but not swept yet. The same entry analysis applies.is_live_cell()is one more FFI call per handler call.onEndTagcallbacks stay protected (see "Not changed").abandon_suspensiondecides "cell still alive" withis_cell(). It predates this change and does not drive the rewriter.<p>elements in one write. They are 16 bytes in one HTTP chunk over loopback. A split would give{"calls":1}, a false failure and never a false pass.Tests that pass before the fix. The two "handlers that nothing else references stay alive" tests and the
Array.prototypetest guard the new design. They cannot fail on a build where the handlers are GC roots. The other 10 fail before the fix: the four leak shapes, the five "output Response that outlives its rewrite" cases, and the crash test.Suites run on the debug (ASAN) build:
test/js/workerd/html-rewriter-leak.test.ts(26 pass, 3 runs),test/js/workerd/html-rewriter.test.js(186 pass),test/js/workerd/html-rewriter-end-error.test.ts,test/js/web/html/html-rewriter-doctype.test.ts,test/js/bun/http/serve-stream-body-error.test.ts, and the HTMLRewriter cases oftest/js/web/workers/worker-terminate-lifetime.test.ts. Fail-before was checked on a release build and on the debug build withsrc/at main: the leak shapes report exactly N retained, and the crash test fails with the messages above.[human-review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file