mirror of
https://github.com/oven-sh/bun.git
synced 2026-10-25 21:40:30 +08:00
robobun
86d32c8bb6
Precursor to #32988, at @Jarred-Sumner's direction there: > This code uses far too much unsafe. Instead, let's refactor our bindings to use the Rust LOLHTML crate directly instead of going through their C API. ### Problem Bun's runtime is Rust, but its lol-html bindings still went through lol-html's **C API**: a 1,299-line FFI layer (`src/lolhtml_sys/`, 72 `extern "C"` declarations) wrapping opaque handles, plus `extern "C"` output-sink and content-handler trampolines that manufacture aliased `&mut` references from userdata pointers, and a take-once thread-local last-error buffer (the direct cause of two correctness findings in #32988's review). That arrangement is a leftover from the Zig runtime, which had no other option; from Rust it is a Rust -> `extern "C"` -> Rust round trip for no benefit, and it is the structural reason HTMLRewriter code has been so `unsafe`-heavy. `lol_html` (v2.7.2) is already vendored at `vendor/lolhtml` and was already in the cargo dependency graph (the c-api crate we consumed is a thin wrapper over it), so using it directly is a dependency-graph no-op. ### What changed - `src/lolhtml_sys/` is deleted entirely: 1,340 lines, 74 `unsafe` blocks, all 72 `extern "C"` declarations. `lol_html = { path = "vendor/lolhtml" }` becomes a direct workspace dependency of `bun_runtime` and `bun_bundler`; the vendor fetch step is unchanged. - `src/runtime/api/html_rewriter.rs` builds a `lol_html::Settings` from the selectors and handlers accumulated on `LOLHTMLContext` and constructs `HtmlRewriter` directly. Handler registration is safe closures capturing raw `NonNull<Handler>` pointers into the `Rc<RefCell<LOLHTMLContext>>` that already outlives the rewriter (the same invariant the C userdata relied on, now visible in Rust). The output sink is a safe `impl lol_html::OutputSink for SinkRef(*mut BufferOutputSink)`; lol-html signals completion with a zero-length final chunk. `HtmlRewriter::end(self)` consumes, so the sink holds the rewriter behind a raw heap pointer read into a local before each call (the same re-entrancy discipline the file already uses) and never calls `end` after a failed `write`. - The eight JS wrapper classes keep their exact shape: a `Cell<*mut X>` handle that is only valid inside its content handler and is nulled before the handler returns. The handle is now a real `lol_html::html_content` type behind one documented lifetime-erasure helper (`cell_get`), and every wrapper method is a safe call into it, because the `lol_html` content operations are safe Rust. - The take-once thread-local error channel is gone. `lol_html`'s methods return typed errors (`RewritingError`, `SelectorError`, `TagNameError`, `AttributeNameError`, `CommentTextError`), so error text comes from the error the failing call returned, never a global consumed as a side channel. - `src/bundler/HTMLScanner.rs` and `src/bundler/linker_context/generateCompileResultForHtmlChunk.rs` migrate the same way. - The two `patches/lolhtml/*` patches only served the now-unused c-api crate and are removed, as is the orphaned `DISABLE_LOLHTML` feature flag (its only reader was the deleted FFI crate). - `HTMLRewriterLoader` (239 lines) is deleted outright rather than migrated. It is a `pub` Zig-port carryover with zero callers: nothing constructs one, it is absent from the JSSink codegen registry (`src/codegen/generate-jssink.ts`), and no `.rs`/`.ts`/`.cpp`/`.h`/`.zig` file references it outside its own definition. Being `pub` is what kept it off the `dead_code` lint. Review (claude, then CodeRabbit) found three distinct latent memory-safety bugs in it, none reachable; deleting the class fixes all three at the root instead of hardening code that cannot run. Net: 26 files, +877 / -2489. ### One behavioral fix: `onEndTag()` no longer leaks its callback This is the only user-visible behavior change, and it falls out of the model change rather than being a goal. The C API binds an element's end-tag handler as a raw `(function pointer, userdata)` pair, so Bun heap-allocated a per-call handler holding a `JSValue::protect()` GC root on the callback and handed the raw pointer to lol-html. Nothing ever freed it on the success path (the only free was the registration-error branch), so **every `element.onEndTag(fn)` call permanently leaked one protected JS function**, plus everything its closure captured, for the life of the process. A long-running `Bun.serve` + `HTMLRewriter` + `onEndTag` grows without bound. `lol_html`'s Rust API instead takes an owning `Box<dyn FnOnce(..)>`. The closure owns the protection; lol-html owns the closure and drops it (releasing the root) whether or not the end tag is ever reached. The leak is structurally impossible. Covered by an exact-count regression test in `test/js/workerd/html-rewriter-leak.test.ts` using `bun:jsc`'s `heapStats().protectedObjectTypeCounts` (no RSS threshold, stable on debug builds): two passes of 400 `onEndTag` registrations after a baseline leave **800** extra protected functions on `main` and **0** with this change. Every other HTMLRewriter and bundler-HTML test passes. Besides the new leak file, the only test change is four added cases in `html-rewriter.test.js` covering the `element.tagName`, `endTag.name`, and `comment.text` setters, which had no coverage anywhere in the tree; no existing test is modified. ### Verification - `cargo check -p bun_runtime -p bun_bundler`, `cargo clippy -p bun_runtime -p bun_bundler`, and `cargo fmt --all --check`: clean. - All five HTMLRewriter test files, 80 pass / 0 fail (75 pre-existing, unmodified, plus the new leak test and four new setter tests): `bun bd test test/js/workerd/html-rewriter.test.js test/js/workerd/html-rewriter-end-error.test.ts test/js/workerd/html-rewriter-leak.test.ts test/js/web/html/html-rewriter-doctype.test.ts test/regression/issue/htmlrewriter-additional-bugs.test.ts` - The new leak test on `main` (fail-before): `expect(after - before).toBe(0)` receives **800**. - All three bundler HTML test files, 30 pass / 0 fail: `bun bd test test/bundler/bundler_html.test.ts test/bundler/bundler_html_server.test.ts test/bundler/html-import-manifest.test.ts` - `grep -rn bun_lolhtml_sys --include='*.rs' --include='*.toml' .` outside `vendor/`: nothing. - `grep 'extern "C"' src/runtime/api/html_rewriter.rs src/bundler/HTMLScanner.rs`: nothing. ### Honest unsafe accounting The `unsafe` that was *about* the lol-html boundary is gone: the 74-block FFI layer, every trampoline, every opaque-handle call. `src/bundler/HTMLScanner.rs` drops from 13 `unsafe` blocks to 3. `html_rewriter.rs` itself still contains `unsafe`, and it is worth being precise about what that is, because almost none of it is about lol-html anymore: 1. One documented lifetime-erasure helper (`cell_get`) at the handler-to-JS-wrapper boundary. A JS object cannot carry a Rust borrow lifetime, so a handler's `&mut Element<'r, 't>` has to be parked as a raw pointer for the duration of the JS callback and nulled before the handler returns; that is inherent to exposing lol-html handles to JS at all, and it is now one place instead of being baked into 72 FFI signatures. Every call site obeys its ordering rule: a setter given a raw `JSValue` runs the value's ToString (arbitrary user JS, which can re-enter `cell_get` on the same wrapper) before the borrow is taken, through one shared `setter_utf8_arg` helper that carries the rule. 2. The pre-existing `JsClass` refcount and `*mut Response` plumbing, which is independent of which lol-html API is used and is unchanged by this PR. The practical payoff lands in #32988: its streaming change was fighting the C boundary (four review rounds found five real bugs, two of them caused directly by the take-once error buffer and the opaque-handle poisoning model). Rebased on this, that diff sheds the classes of unsafe that made it hard to get right. ### Relationship to #32988 #32988 (make HTMLRewriter actually stream) stays open and will be rebased on top of this once it lands. The two are deliberately separate so each is reviewable on its own: this one is the binding migration, and that one is the streaming behavior change.
17 lines
985 B
TOML
17 lines
985 B
TOML
# rustfmt config for the Cargo workspace. Formatting is the toolchain default
|
|
# (rust-toolchain.toml's pinned nightly); this file only carries the ignore list
|
|
# so `cargo fmt --all` (CI's `Format` job) never reformats out-of-scope code.
|
|
#
|
|
# `cargo fmt --all` formats every workspace member's crate root and follows
|
|
# `mod` declarations from there. `vendor/lolhtml` (the `lol_html` crate) is
|
|
# reachable as a direct path dependency of `bun_runtime`/`bun_bundler`; `test/`
|
|
# has Rust test fixtures (e.g. the tonic gRPC server) with their own
|
|
# out-of-workspace `Cargo.toml`; `build/` is the gitignored ninja output dir
|
|
# (codegen .rs, cargo target dir). None should be reformatted by CI even if a
|
|
# future change makes them reachable.
|
|
#
|
|
# Patterns use gitignore semantics: a bare name like `test` matches at *any*
|
|
# depth (it would also exclude `src/runtime/cli/test/`). Anchor with a leading
|
|
# `/` to scope each entry to the workspace root only.
|
|
ignore = ["/vendor", "/test", "/build"]
|