diff --git a/src/ast/ast_memory_allocator.rs b/src/ast/ast_memory_allocator.rs index 322e5921eb76..32b648bdf6b0 100644 --- a/src/ast/ast_memory_allocator.rs +++ b/src/ast/ast_memory_allocator.rs @@ -266,9 +266,8 @@ impl ASTMemoryAllocator { /// Per-iteration reset for hot reuse paths (`initialize_mini_store`'s /// per-workspace-child re-entry). Thin delegate to - /// [`bun_alloc::Arena::reset_retain_with_limit`]; the cold init paths - /// (`bundler::ThreadPool::Worker::init`, `BundleThread::generate_in_new_ - /// thread`) keep calling [`Self::reset`]. + /// [`bun_alloc::Arena::reset_retain_with_limit`]; the cold init path + /// (`bundler::ThreadPool::Worker::init`) keeps calling [`Self::reset`]. pub fn reset_retain_with_limit(&mut self, limit: usize) { if self.arena_dirty { debug_assert!( diff --git a/src/bundler/BundleThread.rs b/src/bundler/BundleThread.rs index 3733ae7d87ad..559b43cc7040 100644 --- a/src/bundler/BundleThread.rs +++ b/src/bundler/BundleThread.rs @@ -57,9 +57,8 @@ pub(crate) struct BundleThread { /// The trait accessors keep the generic `BundleThread` /// layout-agnostic. The concrete impl lives in T6 (`bun_bundler_jsc`). pub trait CompletionStruct: Node + Send + 'static { - /// `bump` is the per-build mimalloc heap that backs `transpiler`, so the - /// two share lifetime `'a` (option fields like `optimize_imports: &'a - /// StringSet` borrow from `bump`). + /// `bump` is the per-build mimalloc heap the transpiler borrows from, so + /// the two share lifetime `'a`. fn configure_bundler<'a>( &mut self, transpiler: &mut Transpiler<'a>, @@ -83,18 +82,12 @@ pub trait CompletionStruct: Node + Send + 'static { /// struct. fn as_js_bundle_completion_task(&mut self) -> dispatch::CompletionHandle; - /// `Transpiler<'a>` has borrow-carrying fields (`arena: &'a Arena`, - /// `resolver: Resolver<'a>`) that cannot be zero-init'd, so the allocate + - /// configure pair is folded into one trait call returning the - /// arena-allocated, fully-configured transpiler. - // The returned `&'a mut Transpiler<'a>` is arena-allocated via `bump.alloc(...)` - // (bumpalo `Bump`), which hands out `&mut` from `&self` through interior - // mutability — the standard arena pattern `mut_from_ref` cannot see through. - #[allow(clippy::mut_from_ref)] + /// Builds and configures the per-build transpiler. The box drops it on + /// every path. fn create_and_configure_transpiler<'a>( &mut self, bump: &'a Arena, - ) -> Result<&'a mut Transpiler<'a>, crate::Error>; + ) -> Result>, crate::Error>; /// Constructs the `BundleV2`, wires `plugins`/`completion`/`file_map`, /// and runs the bundle. @@ -272,22 +265,18 @@ impl BundleThread { let heap = Arena::new(); let bump = &heap; - let ast_memory_store: &mut bun_ast::ASTMemoryAllocator = - bump.alloc(bun_ast::ASTMemoryAllocator::new(bump)); - ast_memory_store.reset(); - ast_memory_store.push(); + let mut ast_memory_store = bun_ast::ASTMemoryAllocator::default(); + let _ast_scope = ast_memory_store.enter(); // Allocate + configure folded — see `create_and_configure_transpiler` doc. - let transpiler = completion.create_and_configure_transpiler(bump)?; + let mut transpiler = completion.create_and_configure_transpiler(bump)?; transpiler.resolver.generation = generation; - // Construction + run delegated — see - // `init_and_run` doc. Reborrow `transpiler` through a raw ptr so - // `completion` can be borrowed again below. - let transpiler_ptr: *mut Transpiler<'_> = transpiler; + let transpiler_ptr: *mut Transpiler<'_> = &raw mut *transpiler; let run = completion.init_and_run( - // SAFETY: `transpiler` lives in `bump` for the duration of `heap`. + // SAFETY: `init_and_run` wants `&'a mut Transpiler<'a>`; the box + // outlives the call and is not touched again until it drops. unsafe { &mut *transpiler_ptr }, bump, // `WorkPool::get()` returns `&'static ThreadPool`; pass as raw so @@ -301,9 +290,10 @@ impl BundleThread { // `deinitWithoutFreeingArena` + wait-group drain live inside `init_and_run` // (it owns `this`). let mut out_log = bun_ast::Log::init(); - // SAFETY: `transpiler.log` is the arena-allocated `*mut Log` set up by - // `configure_bundler`; valid for the lifetime of `heap`. Raw deref so the - // `&'a mut Transpiler` consumed by `init_and_run` above is not reborrowed. + // SAFETY: `transpiler.log` points at the completion task's own `Log`, + // which its owner keeps alive until `complete_on_bundle_thread`. Raw + // deref so the `&'a mut Transpiler` given to `init_and_run` is not + // reborrowed. let _ = unsafe { (*(*transpiler_ptr).log).append_to_with_recycled(&mut out_log, true) }; // logger OOM-only completion.set_log(out_log); @@ -311,33 +301,6 @@ impl BundleThread { completion.complete_on_bundle_thread(); } - ast_memory_store.pop(); - - // `transpiler` / `ast_memory_store` are arena-allocated, but their - // containers (`Resolver` caches, `BundleOptions` strings, the AST - // allocator's own `mi_heap` handle, …) live on the global heap as - // `Vec`/`Box`/`HashMap`, so dropping `heap` (`mi_heap_destroy`) reclaims - // the struct bytes but never runs `Transpiler::drop` / - // `ASTMemoryAllocator::drop` — leaking the resolver's directory/file - // caches and an entire `mi_heap` per `Bun.build()` call. LSan does not - // flag the latter (mimalloc bypasses the ASAN `malloc` interceptor), so - // the symptom is RSS-only: ~32 MB/build linear growth in the - // bun-build-api "does not leak sourcemap JSON" test. - // - // SAFETY: both pointers are the unique `&'a mut` slots returned by - // `bump.alloc(...)` above; nothing else holds a reference to either - // past `init_and_run` (`set_transpiler` was cleared by - // `deinit_without_freeing_arena`, `pop()` restored the AST-allocator - // thread-local). The arena bytes themselves are bulk-freed afterwards - // by `heap`'s `Drop` — `drop_in_place` only releases the *embedded - // global-heap* state, so there is no double free. - unsafe { - core::ptr::drop_in_place(transpiler_ptr); - core::ptr::drop_in_place(std::ptr::from_mut::( - ast_memory_store, - )); - } - run } } diff --git a/src/bundler/transpiler.rs b/src/bundler/transpiler.rs index cf0c9a3bcd57..f1fc92a112b1 100644 --- a/src/bundler/transpiler.rs +++ b/src/bundler/transpiler.rs @@ -146,14 +146,12 @@ pub struct Transpiler<'a> { } impl<'a> Transpiler<'a> { - /// Takes `*mut Log` (not `&'a mut`) because the same - /// `*Log` is aliased into `linker.log` / `resolver.log`; the struct - /// field is a raw pointer for that reason. + /// Takes `*mut Log` (not `&'a mut`) because the same `*Log` is aliased + /// into `options.log` / `linker.log` / `resolver.log`. pub fn set_log(&mut self, log: *mut bun_ast::Log) { self.log = log; + self.options.log = log; self.linker.log = log; - // SAFETY: caller (`ThreadPool::Worker::create`) passes the per-worker - // arena-allocated `Log`, which outlives this `Transpiler<'a>`. self.resolver.log = core::ptr::NonNull::new(log).expect("set_log: log is non-null"); } diff --git a/src/runtime/api/js_bundle_completion_task.rs b/src/runtime/api/js_bundle_completion_task.rs index f449fa2cdb8e..9f5ac4189ce8 100644 --- a/src/runtime/api/js_bundle_completion_task.rs +++ b/src/runtime/api/js_bundle_completion_task.rs @@ -1242,7 +1242,7 @@ impl CompletionStruct for JSBundleCompletionTask { fn create_and_configure_transpiler<'a>( &mut self, bump: &'a Arena, - ) -> bun_bundler::Result<&'a mut Transpiler<'a>> { + ) -> bun_bundler::Result>> { let config = &self.config; let opts = api::TransformOptions { define: if config.define.count() > 0 { @@ -1277,19 +1277,10 @@ impl CompletionStruct for JSBundleCompletionTask { }; let log: *mut bun_ast::Log = &raw mut self.log; - let t = Transpiler::init(bump, log, opts, Some(self.env))?; - let transpiler: &'a mut Transpiler<'a> = bump.alloc(t); - - // Post-init field wiring. - // Reborrow through a raw ptr so `&mut self` is usable - // again after handing `&'a mut Transpiler` (which is tied to `bump`, - // not `self`) to the trait method. - let tp: *mut Transpiler<'a> = transpiler; - // SAFETY: `tp` aliases nothing in `self`; lives in `bump`. - self.configure_bundler(unsafe { &mut *tp }, bump)?; - // SAFETY: `tp` was the unique `&'a mut` slot from `bump.alloc`; the - // reborrow above has ended. - Ok(unsafe { &mut *tp }) + // `configure_linker` stores field addresses: box first, then configure. + let mut transpiler = Box::new(Transpiler::init(bump, log, opts, Some(self.env))?); + self.configure_bundler(&mut transpiler, bump)?; + Ok(transpiler) } fn init_and_run<'a>( @@ -1342,8 +1333,7 @@ impl CompletionStruct for JSBundleCompletionTask { let run = bv2.run_from_js_in_new_thread(&entry_points); - // The AST-allocator pop lives in `generate_in_new_thread`; the - // source-map wait-group waits run only on the error path. + // The source-map wait-group waits run only on the error path. match run { Ok(build) => { self.set_result(BundleV2Result::Value(build)); diff --git a/test/bundler/bun-build-api.test.ts b/test/bundler/bun-build-api.test.ts index a909f51c655c..5df3a72cdc9c 100644 --- a/test/bundler/bun-build-api.test.ts +++ b/test/bundler/bun-build-api.test.ts @@ -1873,6 +1873,51 @@ export { greeting };`, }); }); +// `Bun.build()` and `new Bun.Transpiler()` create a `Transpiler` before they +// validate every option. When validation fails (here: a `define` value that is +// not valid JSON) the error path must still drop it. +test.skipIf(!isASAN)("Bun.build / Bun.Transpiler option error path does not leak the transpiler", async () => { + using dir = tempDir("bun-build-configure-err-leak", { "e.js": "0;\n" }); + const iters = 3; + const script = ` + const entry = ${JSON.stringify(join(String(dir), "e.js"))}; + const bad = { X: '{"a":' }; + let rejected = 0; + for (let i = 0; i < ${iters}; i++) { + try { await Bun.build({ entrypoints: [entry], define: bad }); } catch { rejected++; } + try { new Bun.Transpiler({ define: bad }); } catch { rejected++; } + } + console.log("rejected", rejected); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: { + ...bunEnv, + // Bun's built-in ASAN defaults turn LSan off. Destructing the VM on exit + // frees what JS still referenced, so only lost allocations get reported. + // abort_on_error=0 so a leak exits 1 instead of raising SIGABRT. + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1", "abort_on_error=0"].filter(Boolean).join(":"), + // verbosity=1 makes the exit-time check announce itself on stderr. The + // binary's built-in suppressions cover the detached bundler thread; the + // shared test/leaksan.supp is not used so a new entry there cannot hide + // this leak. + LSAN_OPTIONS: "verbosity=1", + BUN_DESTRUCT_VM_ON_EXIT: "1", + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Both calls must reach the option error, or the test measures nothing. + expect(stdout.trim()).toBe(`rejected ${iters * 2}`); + expect(stderr).toContain("LeakSanitizer: checking for leaks"); + // LSan prints one blank-line-separated block per leaking allocation stack. + // Before the fix each failed `Bun.build` left about 200 of them. + const leaks = stderr.split(/\n\s*\n/).filter(block => /^(?:Direct|Indirect) leak of /.test(block)); + expect(leaks).toEqual([]); + expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); +}); + // On release builds mimalloc's large-allocation arenas make RSS growth too // non-deterministic to draw a clean line between "leaking" and "not leaking" // for this path. Under debug/ASAN the allocator behaviour is stable enough to diff --git a/test/bundler/transpiler/transpiler.test.js b/test/bundler/transpiler/transpiler.test.js index 2ec82493d4ff..2164be1b04ec 100644 --- a/test/bundler/transpiler/transpiler.test.js +++ b/test/bundler/transpiler/transpiler.test.js @@ -2534,6 +2534,31 @@ export default <>hi ).toThrow(); }); + it("invalid define value surfaces the JSON parser diagnostic", () => { + // `configure_defines` writes the parse error into `options.log`, which + // `set_log` used to leave pointing at the constructor's moved-from stack + // slot — so `config.log` stayed empty and the generic `throw_error` path + // was taken instead of `log.to_js`. + let err; + try { + new Bun.Transpiler({ define: { X: '{"a":' } }); + } catch (e) { + err = e; + } + expect(err).toBeDefined(); + expect(err.message).not.toContain("ParserError"); + expect(err.message).toContain("Unexpected end of file"); + expect(err.position).toEqual({ + lineText: '{"a":', + file: "defines.json", + namespace: "internal", + line: 1, + column: 5, + length: 0, + offset: 5, + }); + }); + it("define with an empty-string key is ignored without leaving uninitialized slots", async () => { // `JSPropertyIterator` skips empty-name properties, but `names`/`values` were being // indexed by the property position instead of a dense counter, leaving garbage in the