diff --git a/src/bundler/transpiler.rs b/src/bundler/transpiler.rs index c5ef7f1b8fd3..4aa408386857 100644 --- a/src/bundler/transpiler.rs +++ b/src/bundler/transpiler.rs @@ -83,7 +83,7 @@ impl<'a> Transpiler<'a> { // resolver/lib.rs `// arena: dropped`), so nothing left to thread. } - /// VM-teardown: the owning `VirtualMachine` is raw-allocated and never `Drop`'d, + /// VM-teardown: the owning `VirtualMachine` is one that nothing drops (its storage stays), /// so free `BundleOptions` here. `log`/`fs`/`env` are aliased/singletons; left alone. /// `resolver` is a value field whose caches alias process-global BSSMaps, so the /// resolver itself stays put — only its owned `opts` projection (cloned in @@ -93,7 +93,7 @@ impl<'a> Transpiler<'a> { /// Calls `drop_in_place` on `options` / `result` / `resolver.opts` / /// `resolve_results`, leaving them logically uninitialized. After this /// returns, `self` must never be dropped (or `deinit`'d again) — every - /// caller holds a `Transpiler` that bypasses `Drop`: a raw-`dealloc`'d + /// caller holds a `Transpiler` that bypasses `Drop`: a never-freed /// `VirtualMachine` field, a `MaybeUninit` stack slot, or an arena-backed /// `&'static mut`. Owned `Transpiler`s from [`Self::for_worker`] must use /// normal `Drop` instead. diff --git a/src/jsc/RefString.rs b/src/jsc/RefString.rs index 3f63e3019f05..4bd76bb760ad 100644 --- a/src/jsc/RefString.rs +++ b/src/jsc/RefString.rs @@ -21,6 +21,37 @@ pub type Map = #[derive(Debug)] pub struct StringTooLong; +/// A VM's interned [`RefString`]s. Its `Drop` releases the ref that each entry holds on its own impl. +#[derive(Default)] +pub(crate) struct Table(Map); + +impl core::ops::Deref for Table { + type Target = Map; + fn deref(&self) -> &Map { + &self.0 + } +} + +impl core::ops::DerefMut for Table { + fn deref_mut(&mut self) -> &mut Map { + &mut self.0 + } +} + +impl Drop for Table { + fn drop(&mut self) { + for &entry in self.0.values() { + // SAFETY: the entry's own ref, released below, keeps it live. + let impl_ = unsafe { + // The finalizer would look for the entry in this table. + (*entry.as_ptr()).on_before_deinit = None; + (*entry.as_ptr()).impl_ + }; + drop(bun_core::String::adopt_wtf_impl(impl_)); + } + } +} + pub type Callback = unsafe fn(ctx: *mut c_void, str: *mut RefString); pub struct RefString { diff --git a/src/jsc/ScriptExecutionContext.rs b/src/jsc/ScriptExecutionContext.rs index 3482d009fb95..6d8a3510fe02 100644 --- a/src/jsc/ScriptExecutionContext.rs +++ b/src/jsc/ScriptExecutionContext.rs @@ -209,6 +209,11 @@ impl ScriptExecutionContext { .with_mut(|groups| groups.as_deref_mut().is_none_or(|groups| groups.is_empty())) } + /// For a context that outlives the thread's uSockets loop: its (emptied) groups leave the loop now. + pub(crate) fn detach_socket_groups_from_loop(&self) { + drop(self.socket_groups.take()); + } + /// The groups client sockets opened by a graph context's script join. pub(crate) fn socket_groups(&self) -> *mut crate::rare_data::SocketGroups { self.socket_groups diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index ff2511f8c87d..1717aaba691c 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -283,9 +283,8 @@ pub struct VirtualMachine { pub(crate) had_errors: bool, pub macros: MacroMap, - // LAYERING: values are `MacroEntryPoint` from `bun_bundler::entry_points` - // (forward dep); stored type-erased and cast back by the consumers. - pub macro_entry_points: bun_collections::ArrayHashMap, + pub macro_entry_points: + bun_collections::ArrayHashMap>, pub macro_mode: bool, /// Depth of live [`MacroModeGuard`]s on this thread. Nonzero exactly while /// macro JS may be executing — both `MacroContext::call` and `Macro::init` @@ -327,7 +326,7 @@ pub struct VirtualMachine { pub regular_event_loop: EventLoop, pub event_loop: *mut EventLoop, // BORROW_FIELD — points at sibling regular_event_loop/macro_event_loop - pub(crate) ref_strings: crate::ref_string::Map, + pub(crate) ref_strings: crate::ref_string::Table, pub(crate) ref_strings_mutex: bun_threading::Mutex, pub(crate) active_tasks: usize, @@ -2077,8 +2076,9 @@ impl VirtualMachine { ) -> crate::CrateResult<*mut JSInternalPromise> { use bun_bundler::entry_points::{Fs, MacroEntryPoint}; use bun_collections::hash_map::Entry; - let entry_point: *mut MacroEntryPoint = match self.macro_entry_points.entry(hash) { - Entry::Occupied(e) => (*e.get()).cast(), + // `path` points into the entry's own buffer, which its box keeps in place. + let path: &[u8] = match self.macro_entry_points.entry(hash) { + Entry::Occupied(e) => e.get().source.path.text, Entry::Vacant(v) => { let mut ep = Box::new(MacroEntryPoint::default()); // SAFETY: PathName stores slices with an artificial 'static @@ -2095,15 +2095,10 @@ impl VirtualMachine { hash, specifier, )?; - let raw = bun_core::heap::into_raw(ep); - v.insert(raw.cast()); - raw + v.insert(ep).source.path.text } }; - // SAFETY: `entry_point` was just inserted (heap-allocated) or fetched - // from the cache; it lives for the VM lifetime. - let path: &[u8] = unsafe { &*entry_point }.source.path.text; let promise = self.run_with_api_lock(|| { // SAFETY: per-thread VM; the API lock guarantees JSC is held. VirtualMachine::get().as_mut()._load_macro_entry_point(path) @@ -2419,11 +2414,11 @@ impl VirtualMachine { } /// Ends this thread's VM, which is not the main thread's: its exit handlers, - /// [`teardown`](Self::teardown), and the storage `init` put on the global heap. + /// [`teardown`](Self::teardown), its field destructors, and the storage `init` put on the global heap. /// Returns its exit code. /// /// # Safety - /// `this` is this thread's VM, made without a caller's `log`; this thread holds + /// `this` is this thread's VM as `init` returned it, made without a caller's `log`; this thread holds /// its API lock and abandons it; nothing else can reach the VM. pub unsafe fn exit_and_free(this: *mut Self) -> u8 { // SAFETY: fn contract (statement-scoped exclusive access: the handlers run script). @@ -2434,19 +2429,22 @@ impl VirtualMachine { }; // SAFETY: fn contract. unsafe { Self::teardown(this, Teardown::Worker) }; + // SAFETY: sole owner. Both are raw pointers, which no field destructor frees. + let (console, log) = unsafe { ((*this).console, (*this).log) }; + // SAFETY: sole owner. `teardown` left every field valid to drop, and no field destructor + // reads what is gone by now: the JSC VM, the loop, the runtime state. A `Strong` asks the + // thread's VM whether it is shutting down, so `set_vm(None)` comes after. + unsafe { core::ptr::drop_in_place(this) }; // SAFETY: sole owner; nothing past this point dereferences the VM. unsafe { - let console = core::mem::replace(&mut (*this).console, core::ptr::null_mut()); if !console.is_null() { bun_core::heap::destroy(console); } - if let Some(log) = (*this).log.take() { + if let Some(log) = log { bun_core::heap::destroy(log.as_ptr()); } VMHolder::set_vm(None); - // The VM was `alloc_zeroed(Layout::())` in - // `init`, NOT `Box::new` — dealloc the raw storage directly so - // field `Drop`s do not re-run on already-`deinit`'d state. + // `init` made the storage with `alloc_zeroed`, not `Box::new`. std::alloc::dealloc(this.cast::(), core::alloc::Layout::new::()); } exit_code @@ -2633,6 +2631,8 @@ impl VirtualMachine { if let Some(rare) = unsafe { (*this).rare_data.as_deref_mut() } { rare.detach_socket_groups_from_loop(); } + // The dead context is dropped with the VM's storage, after the loop. + vm.dead_context.detach_socket_groups_from_loop(); // SAFETY: this thread's loop; nothing ticks it any more. unsafe { (*vm.uws_loop()).internal_loop_data.jsc_vm = core::ptr::null_mut() }; bun_uws::free_thread_loop(); @@ -2644,7 +2644,7 @@ impl VirtualMachine { // ---- E. free owners -------------------------------------------------- // SAFETY: fn contract; last use of `this`. - unsafe { (*this).destroy() }; + unsafe { (*this).destroy_fields(matches!(kind, Teardown::Worker)) }; } } @@ -3255,7 +3255,7 @@ impl VirtualMachine { // SAFETY: VM is large + self-referential; allocate zeroed and fill in // place. The // allocation lives for the thread lifetime (never freed on the main - // thread; worker `destroy()` frees it explicitly). + // thread; `exit_and_free` frees a worker's). // // Note (validity): the zeroed bytes are NOT a valid // `VirtualMachine` — `origin_timer: Instant`, `on_unhandled_rejection: @@ -5693,8 +5693,13 @@ impl VirtualMachine { Ok(Ok(bun_core::String::clone_utf8(result.path))) } - /// Worker-thread teardown. + /// Teardown phase E for a VM whose storage stays allocated (bake's build VM). pub fn destroy(&mut self) { + self.destroy_fields(false); + } + + /// `drop_follows`: the field destructors run next (`exit_and_free`). + fn destroy_fields(&mut self, drop_follows: bool) { self.regular_event_loop.deinit(); self.macro_event_loop.deinit(); // The VM's own clone of its handle; the shared inner is freed when the @@ -5734,21 +5739,26 @@ impl VirtualMachine { // proxy strings; `ProxyEnvStorage: Default` so take()+drop suffices. drop(core::mem::take(&mut self.proxy_env_storage)); - // The VM box is `dealloc`'d raw by the worker (see `web_worker.rs` - // section 5) so field `Drop`s never run; reclaim the boxed - // `ModuleLoader` payloads explicitly. `eval_source.contents` may be + // `eval_source.contents` may be // mmap-backed (`MAPPED_CONTENTS_CACHE`) — `Source`'s `Drop` is a // no-op, so dropping the box just frees its own allocation. drop(core::mem::take(&mut self.module_loader)); - // Same raw-dealloc story: `preload` is cloned into the VM at spin() + // `preload` is cloned into the VM at spin() // time and `load_preloads` clears the boxes but keeps the Vec buffer, - // so reclaim it here or every Worker leaks it. + // so reclaim it here. drop(core::mem::take(&mut self.preload)); - // SAFETY: this VM is raw-`dealloc`'d (no field `Drop` runs), so - // `transpiler` is never auto-dropped after `deinit` clears its fields. - unsafe { self.transpiler.deinit() }; + if drop_follows { + // The field destructors drop the transpiler. Its macro context has no destructor. + if let Some(ctx) = self.transpiler.macro_context.take() { + ctx.deinit(); + } + } else { + // SAFETY: the storage of this VM is never freed, so nothing drops + // `transpiler` after `deinit` clears its fields. + unsafe { self.transpiler.deinit() }; + } drop(core::mem::take(&mut self.resolved_path_dups)); drop(core::mem::take(&mut self.main_resolved_path)); diff --git a/src/runtime/cli/lint_js.rs b/src/runtime/cli/lint_js.rs index e4a2a0943c2a..027627e8e706 100644 --- a/src/runtime/cli/lint_js.rs +++ b/src/runtime/cli/lint_js.rs @@ -401,7 +401,6 @@ fn end_vm(frees: bool) { // Never released: `exit_and_free` frees the VM, and the lock with it. let _lock = core::mem::ManuallyDrop::new(vm.global().vm().get_api_lock()); drop(crate::jsc_hooks::take_lint_vm()); - drop(core::mem::take(&mut vm.as_mut().argv)); // SAFETY: `start_vm` created it on this thread, which holds its lock. No other thread has a pointer to it. unsafe { VirtualMachine::exit_and_free(VirtualMachine::get_mut_ptr()) }; } diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index 920f9c462944..9147a0f66c1c 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -425,12 +425,11 @@ unsafe fn init_runtime_state( // `RuntimeState` (boxed above) so `deinit_runtime_state` reclaims it // alongside `timer`/`entry_point` on Worker teardown. The `Box` // payload address is stable, so a `'static` borrow is sound for the - // `Transpiler<'static>` field — both die in VM teardown - // (`vm.transpiler` is never dropped; see `ptr::write` note below). - // SAFETY: `state` is the unique freshly-boxed `RuntimeState`; the - // inner `Box` payload is heap-stable and outlives the - // `Transpiler` (reclaimed in `deinit_runtime_state` after the VM — - // and hence `vm.transpiler` — is done). + // `Transpiler<'static>` field: both die in VM teardown. + // SAFETY: `state` is the unique freshly-boxed `RuntimeState`. Its `Box` + // payload is heap-stable and outlives every use of the `Transpiler`: + // `deinit_runtime_state` reclaims it, and a worker's field destructors, + // which drop the transpiler after that, do not read `arena`. let arena: &'static bun_alloc::Arena = unsafe { &*(*state).transpiler_arena }; // Forward `opts.env_loader` so the VM // shares the caller's `DotEnv.Loader` (e.g. `bun test` writes @@ -3724,20 +3723,13 @@ fn __bun_fetch_builtin_module( } // ── `macro:` namespace ────────────────────────────────────────────── - // `vm.macro_entry_points` values are - // `*mut MacroEntryPoint` (gated `bun_bundler::entry_points` type); the - // map itself is keyed by `i32` hash of the specifier. + // `vm.macro_entry_points` is keyed by the `i32` hash of the specifier. if spec.starts_with(b"macro:") { use bun_bundler::entry_points::MacroEntryPoint; let id = MacroEntryPoint::generate_id_from_specifier(spec); - if let Some(&entry) = jsc_vm.macro_entry_points.get(&id) { - let entry = entry.cast::(); - // SAFETY: `entry` is the `heap::alloc`d `MacroEntryPoint` - // inserted by `load_macro_entry_point`; map ownership keeps it - // alive for the VM lifetime. - let contents = unsafe { &(*entry).source.contents }; + if let Some(entry) = jsc_vm.macro_entry_points.get(&id) { return Some(ResolvedSource { - source_code: bun_core::String::clone_utf8(contents), + source_code: bun_core::String::clone_utf8(&entry.source.contents), source_url: specifier.clone(), ..ResolvedSource::default() }); diff --git a/test/js/web/workers/worker-terminate-lifetime.test.ts b/test/js/web/workers/worker-terminate-lifetime.test.ts index 92c79f6db66d..52b396fbdf36 100644 --- a/test/js/web/workers/worker-terminate-lifetime.test.ts +++ b/test/js/web/workers/worker-terminate-lifetime.test.ts @@ -121,6 +121,150 @@ test( timeout, ); +// How ./worker.ts of the tests below ends, by its first argument: its loop +// drains, it calls process.exit(), or its parent terminates it (the interval +// keeps it alive until then). +const endsItsOwnWay = ` + const end = { + drains: () => {}, + exits: () => process.exit(0), + terminated: () => { postMessage("filled"); setInterval(() => {}, 1000); }, + }[process.argv[2]];`; +// In the parent: runs ./worker.ts until it has ended. +const runsAWorker = ` + const run = async ends => { + const worker = new Worker(new URL("./worker.ts", import.meta.url).href, { argv: [ends] }); + worker.onmessage = () => worker.terminate(); + await new Promise((resolve, reject) => { + worker.addEventListener("close", resolve); + worker.addEventListener("error", event => reject(event.error ?? event.message)); + }); + };`; + +// Regression: the storage of a worker's VM was freed without a run of its field +// destructors, and the teardown freed only the fields it named. A worker that +// filled one of the others leaked it. Each row fills one from a timer, the first +// row aside: the stack of a timer callback has no module-evaluation frame, which +// leaksan.supp suppresses. +describe.skipIf(!isASAN)("a worker frees what the fields of its VM own when it ends", () => { + const rows: [owned: string, fill: string, filled: string][] = [ + [ + "the macros its entry file called", + `import { one } from "./macro.ts" with { type: "macro" }; + console.log("macro returned", one()); + end();`, + "macro returned 1", + ], + [ + "the macros a Bun.Transpiler called", + `setTimeout(() => { + const source = 'import { one } from "./macro.ts" with { type: "macro" }; console.log("macro returned", one());'; + console.log(new Bun.Transpiler({ loader: "ts" }).transformSync(source).trim()); + end(); + }, 0);`, + `console.log("macro returned", 1);`, + ], + [ + "its require.extensions handlers", + `setTimeout(() => { + require.extensions[".custom"] = (module, filename) => module._compile("module.exports = 1;", filename); + console.log("handler is", typeof require.extensions[".custom"]); + end(); + }, 0);`, + "handler is function", + ], + [ + "the strings a Bun.FileSystemRouter interned", + `setTimeout(() => { + const router = new Bun.FileSystemRouter({ + style: "nextjs", + dir: import.meta.dir + "/pages", + origin: "https://example.com", + assetPrefix: "/assets/", + }); + console.log("matched", router.match("/")?.name); + end(); + }, 0);`, + "matched /", + ], + ]; + + test.concurrent.each(rows)( + "%s", + async (_, fill, filled) => { + using dir = tempDir("worker-end-frees", { + "macro.ts": `export function one() { return 1; }`, + "pages/index.ts": `export default 1;`, + "worker.ts": `${endsItsOwnWay}\n${fill}`, + "main.ts": ` + ${runsAWorker} + for (const ends of ["drains", "exits", "terminated"]) await run(ends); + console.log("every worker ended");`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.ts"], + cwd: String(dir), + env: { + ...bunEnv, + // The CI runner sets this for every test: the main thread tears its VM + // down at exit too. + BUN_DESTRUCT_VM_ON_EXIT: "1", + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), + LSAN_OPTIONS: `print_suppressions=0:suppressions=${join(import.meta.dirname, "../../../leaksan.supp")}`, + }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // A debug build also logs "[macro] call one" to stdout. + const lines = stdout.split("\n").filter(line => !line.startsWith("[macro]")); + expect({ stdout: lines, stderr, exitCode }).toEqual({ + stdout: [filled, filled, filled, "every worker ended", ""], + stderr: "", + exitCode: 0, + }); + }, + timeout, + ); +}); + +// The same regression, for what LeakSanitizer does not see: a mimalloc heap. The +// `macro` option has the VM's transpiler parse the table with its JSON cache, +// which makes one, and a worker's teardown left the caches of that transpiler. +test("a worker frees the mimalloc heap of its transpiler's JSON cache when it ends", async () => { + using dir = tempDir("worker-end-frees-heap", { + "worker.ts": ` + ${endsItsOwnWay} + setTimeout(() => { + new Bun.Transpiler({ loader: "ts", macro: { "some-package": { one: "./macro.ts" } } }); + end(); + }, 0);`, + "main.ts": ` + import { heapStats } from "bun:jsc"; + ${runsAWorker} + // The first worker also makes the heaps that stay for the process. + await run("drains"); + const before = heapStats().mimalloc.heaps.current; + for (const ends of ["drains", "exits", "terminated"]) await run(ends); + console.log("heaps that three workers left:", heapStats().mimalloc.heaps.current - before);`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.ts"], + cwd: String(dir), + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: "heaps that three workers left: 0\n", + stderr: "", + exitCode: 0, + }); +}); + // Regression: the per-VM c-ares channel was destroyed in deinit_runtime_state // (RuntimeState drop) AFTER JSC teardown and RareData.file_polls drop. // ares_destroy() synchronously fires EDESTRUCTION query callbacks and socket- @@ -164,6 +308,9 @@ test.skipIf(!isASAN)( ], env: { ...bunEnv, + // The CI runner sets this for every test; set it here too so the + // main thread's own per-VM state is not reported when run directly. + BUN_DESTRUCT_VM_ON_EXIT: "1", ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), LSAN_OPTIONS: `print_suppressions=0:suppressions=${join(import.meta.dirname, "../../../leaksan.supp")}`, }, diff --git a/test/leaksan.supp b/test/leaksan.supp index 06f00aa8c43d..a305f13bad62 100644 --- a/test/leaksan.supp +++ b/test/leaksan.supp @@ -78,7 +78,6 @@ leak:sys_jsc.error_jsc.errorToShellSystemError leak:lazyLoadSQLite leak:JSC::intlAvailableLocales leak:getaddrinfo -leak:runtime.api.filesystem_router.FileSystemRouter.constructor leak:JSC::intlSegmenterAvailableLocales leak:URL__getHref leak:runtime.dns_jsc.dns.Resolver.globalLookupService