Make bun_core::String own its WTF ref

Merged
DY
3mo ago
+3924-6236

What does this PR do?

bun_core::String (the Rust mirror of C++ BunString) was a Copy POD with no destructor, so every +1 producer — clone_utf8, create_format, to_bun_string, URL accessors, C++ toStringRef out-params — relied on the caller remembering .deref() or OwnedString. An audit found ~60 places that didn't (per-import ResolvedSource.source_url/specifier, whole HMR bundles in bake, Postgres error fields, valkey/UDP/S3 option strings, parseArgs argv, QUIC headers, …). This moves the +1/-1 rule into the type so those, and any not yet found, are released by Drop.

Ownership is the type now

// before: Copy, no Drop — caller must remember
let s = value.to_bun_string(global)?;   // +1
do_stuff(&s);
s.deref();                              // forgotten in ~60 places

// after: String owns one ref. Drop = deref, Clone = ref, moves are free.
let s = value.to_bun_string(global)?;
do_stuff(&s);                           // released at scope exit, incl. `?` paths

FFI signatures carry ownership. String has the same layout as C++ BunString, so a by-value String in an extern signature means the ref crosses (like Box<T> in FFI); &String / StringView<'_> mean borrow. C++ that only reads takes const BunString*.

safe fn URL__host(url: &URL) -> String;                        // C++: Bun::toStringRef (+1) → we own it
pub fn host(&self) -> String { URL__host(self) }

extern "C" fn BakeProdResolve(global: &JSGlobalObject, a: &String, b: &String) -> String {
    String::create_format(format_args!("..."))                 // C++: transferToWTFString()
}

safe fn ZigSourceProvider__getSourceSlice(p: &SourceProviderMap) -> StringView<'_>;   // C++: Bun::toStringView (+0)

Borrows are &String / StringView<'a> (a by-value borrow tied to whatever owns the characters: property-iterator names, substring/trunc, JsCell<String>::get), and consuming into JS is into_js(self) (was transfer_to_js(&mut self)):

return String::clone_utf8(name).into_js(global);   // ref moves into the JSString, no ref/deref pair

Reading a JS string's characters is a scoped borrow. JSValue::get_zig_string() returned a ZigString aliasing a JSString's buffer with no lifetime, no ref, and nothing keeping the cell alive. It is removed; callers either take an owned to_bun_string(), or:

let v = value.to_js_string_view(global)?;   // one FFI call: toString + JSString::view (no ref; substring ropes aren't flattened)
use(v.latin1(), v.to_utf8(), v.length());   // derefs to &String
// drop(v) keeps the JSString cell observable to the GC until here (like C++ GCOwnedDataScope)

Use it for a scoped read; use to_bun_string() when the string must be kept. JSString::view() returns the same JSStringView guard; JSPropertyIterator::next() yields (StringView<'_>, JSValue) tied to the iterator. get_class_name/get_name_property return an owned String (they handed out views into C++ temporaries).

ResolvedSource is owned on both sides. Rust ResolvedSource/Errorable<T> own their strings, bytecode and module_info; C++ ErrorableResolvedSource gets a destructor, and Zig::SourceProvider::create takes what it keeps via transferToWTFString(). The unused specifier field is removed. Previously only ~SourceProvider released specifier/source_url, so every loader arm that doesn't build a provider (JSON/TOML/text/object exports, builtins, already-required CJS) leaked two strings per import. The LoaderHooks fn-pointer table between bun_jsc and bun_runtime is replaced by direct extern "Rust" calls; Errorable's error arm is just the JSValue (the code was never read).

Fixes

Leaks (a WTF::StringImpl ref — or the whole string — never released), per occurrence unless noted:

  • every import/require of JSON, JSONC, TOML, YAML, text, HTML, bun:main, internal/*, builtins and already-required CJS: ResolvedSource.specifier and source_url (and source_code for the JSON/object/builtin arms)
  • every async (await import) module fulfilment: source_url
  • bun --hot / bake dev server: the entire server HMR patch bundle on each reload; the initial server runtime; BakeProdResolve/BakeProdLoad/BakeToWindowsPath results
  • Bun.sql (Postgres): every field of every error/notice message; the JSON.stringify result for each json/jsonb bind parameter (#40102); inline column-name atoms in CachedStructure
  • Bun.sql (MySQL): JSON column values
  • new Bun.RedisClient(url): the URL string and each parsed component; every RESP map key
  • Bun.udpSocket({ hostname }) / connect address; Bun.serve({ unix }); the server URL passed to the inspector; error-path hostname strings
  • S3 error code/message
  • node:util parseArgs: one string per argv element and per short option
  • node:quic: header names/values, ALPN/SNI/status strings; node:http2 custom settings keys
  • HTMLRewriter element.attributes iterator name/value
  • node:module new SourceMap(payload) sources/names, findSourceMap path
  • expect(v, label), expect.extend matcher names, describe(fn)/test(fn) derived names, console.log table column names / class display names, console.trace() (built the exception twice)
  • ResolveMessage.code, Bun.$ escape, IPC cmd strings, process.title = …, Worker creation error message, WebSocket client close reason on error paths, Bun.Transpiler transform result when warnings reject, bun:ffi cc/linkSymbols strings on error, import.meta.url define in the bundler, bun install hosted-git URL parts, bun build --bytecode chunk source URL, code-coverage source URL, fs.Dirent name/path on early return, bundler plugin onResolve/onLoad strings when the plugin is tombstoned
  • worker VirtualMachine.main_resolved_path at teardown
  • the .jsc bytecode buffer and ModuleInfo for already-bundled modules on some paths
  • ZigException::Holder: strings of stack frames hidden by the frame filter

Use-after-free / lifetime:

  • Bun.dlopen read its path argument after walking the user symbols object (getters could drop it)
  • JSX tag name in console.log/expect formatting was reused after printing props/children
  • Bun.YAML.stringify kept anchor names past the property iterator that owned them
  • getClassName/getNameProperty returned views into function-local WTF::Strings
  • os.userInfo() options were ptr::read out of C++-owned memory (double free)
  • any toString()-created temporary JSString whose characters were read after a GC point (now pinned by JSStringView)

Other:

  • BakeLoadInitialServerCode copied the whole server HMR runtime into a new StringImpl at dev-server start; it now wraps the embedded bytes
  • non-ASCII paths in async-module load errors were formatted as Latin-1 (mojibake); encoding behaviour is otherwise unchanged
  • UDP socket address/remoteAddress getters swallowed a pending exception as undefined

Also removes Bun::toString(JSGlobalObject*, JSValue) (a +1 producer named like a borrow), StringCell/OwnedString, the duplicate bun_jsc::URL wrapper (now bun_url::whatwg::URL, held via the RAII whatwg::Parsed), and to_slice_or_null/to_slice_clone (identical to to_slice).

No extra refcount traffic or allocations vs main: Drop/Clone are the same tag-check + inc/dec the manual calls were; to_js_string_view is one FFI call like get_zig_string was; and a number of ref()/dupe_ref() pairs, a full transpiler-cache output copy, and per-module specifier refs are gone.

Struct size changes (nothing grows; shared layouts are asserted on both sides):

  • ResolvedSource (Rust ⇄ C++): 160 → 136 bytes — specifier and the unused allocator removed, needsDeref → bytecode_cache_owned. ErrorableResolvedSource follows (168 → 144).
  • Zig::SourceProvider (C++): no longer embeds a ResolvedSource; keeps only m_moduleInfo/m_tag/m_alreadyBundled (~144 bytes smaller per loaded module).
  • Rust-only: TranspilerJob drops its fetcher field; RuntimeTranspilerCache::Entry::output_code is a plain String instead of an enum; ZigException::Holder uses Option<ZigException> instead of MaybeUninit + loaded: bool.
  • Unchanged: bun_core::String (24, asserted), StringView/JsCell<String> (transparent), ZigStackTrace/ZigStackFrame, SystemError, generated class payloads.

Fixes #40102
Supersedes #32293.

How did you verify your code works?

cargo check/clippy on linux/macOS/windows targets; debug+ASAN build driven through module loading (ESM/CJS/JSON/TOML/text/?query, builtins, virtual modules), console.log/Bun.inspect (errors, JSX, boxed primitives, symbols, classes), node:path, node:module SourceMap, workers, process.title, os.userInfo, WebSocket send/publish, UDP/unix-socket serve, parseArgs, TOML/YAML/XML stringify, Bun.dlopen, bake dev server startup. Ran the module, resolve, path, parse-args, worker, inspect(-error), expect, shell, transpiler-cache, websocket-server, serve, zlib, udp, dns, http2 and serve-body-leak test files locally against main's debug build. New RSS regression tests (each repeats the operation with a ~256 KiB string and fails on 1.4.0 with the growth shown): Postgres jsonb bind (166 MiB) and ErrorResponse fields (125 MiB) via the wire-frames.ts mock, util.parseArgs (77), expect(v, label) (76), module.SourceMap (158), Bun.udpSocket({hostname}) (73), HTMLRewriter attributes (90), new RedisClient(url) (145), RESP map keys (150), S3 error (47). The use-after-free fixes are not observable on a release build of 1.4.0 without ASAN, so they have no test.