diff --git a/src/jsc/JSPromise.rs b/src/jsc/JSPromise.rs index 72b9b2cfcd18..7fd0346c0629 100644 --- a/src/jsc/JSPromise.rs +++ b/src/jsc/JSPromise.rs @@ -378,14 +378,13 @@ impl JSPromise { /// Safe `status()` for the common `*mut JSPromise`-stored case /// (`vm.pending_internal_promise` etc.). `JSPromise` is a GC-managed JSC - /// heap cell; pointers to it are kept alive by the VM's strong-ref slots, - /// not by Rust ownership. Centralizes the per-call-site - /// `unsafe { (*p).status() }` deref so callers don't open-code it. + /// heap cell; the caller must hold a GC root for `p` (a stack local, + /// `JSValue::protect`, or a `Strong`) for the duration of the call. + /// Centralizes the per-call-site `unsafe { (*p).status() }` deref so + /// callers don't open-code it. #[inline] pub fn status_ptr(p: *mut JSPromise) -> Status { - // `p` is a non-null GC-managed cell tracked by the VM (caller obtained - // it from a strong-ref VM field or a fresh - // `JSInternalPromise__resolvedPromise` return value). + // `p` is a non-null GC-managed cell the caller holds a root for. JSPromise::opaque_ref(p).status() } diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 2dc03fbd4246..285ac59d0c23 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -288,8 +288,10 @@ pub struct VirtualMachine { pub proxy_env_storage: crate::rare_data::ProxyEnvStorage, pub resolved_path_dups: Vec>, pub is_us_loop_entered: bool, + /// Entry-point evaluation promise. Always GC-protected while `Some`; + /// every write goes through [`set_pending_internal_promise`] which + /// balances the previous `protect()`. pub pending_internal_promise: Option<*mut JSInternalPromise>, - pub pending_internal_promise_is_protected: bool, pub pending_internal_promise_reported_at: u32, pub hot_reload_deferred: bool, pub entry_point_result: EntryPointResult, @@ -2285,6 +2287,22 @@ impl VirtualMachine { } } + /// Store (or clear) the entry-point evaluation promise. The promise is + /// polled across event-loop ticks by the `--hot` run loop + /// ([`report_exception_in_hot_reloaded_module_if_needed`] and [`reload`]), + /// and the module loader drops its own reference once the promise settles, + /// so it must be a GC root while stored here. Balances the previous + /// `protect()` before taking the new one. + pub fn set_pending_internal_promise(&mut self, promise: Option<*mut JSInternalPromise>) { + if let Some(prev) = self.pending_internal_promise { + JSValue::from_cell(prev).unprotect(); + } + self.pending_internal_promise = promise; + if let Some(p) = promise { + JSValue::from_cell(p).protect(); + } + } + /// `reloadEntryPoint(entry_path)` — set `main`, generate the synthetic /// `bun:main` entry, run preloads, and kick off module evaluation. pub fn reload_entry_point( @@ -2352,10 +2370,7 @@ impl VirtualMachine { // SAFETY: hook contract. let p = unsafe { (hooks.load_preloads)(self) }?; if !p.is_null() { - JSValue::from_cell(p).ensure_still_alive(); - JSValue::from_cell(p).protect(); - self.pending_internal_promise = Some(p); - self.pending_internal_promise_is_protected = true; + self.set_pending_internal_promise(Some(p)); return Ok(p); } } @@ -2363,8 +2378,7 @@ impl VirtualMachine { // Check if Module.runMain was patched. if self.has_patched_run_main { bun_core::hint::cold(); - self.pending_internal_promise = None; - self.pending_internal_promise_is_protected = false; + self.set_pending_internal_promise(None); let global_ref = self.global(); let argv1 = jsc::bun_string_jsc::create_utf8_for_js(global_ref, MAIN_FILE_NAME) .map_err(|_| crate::CrateError::JSError)?; @@ -2378,8 +2392,7 @@ impl VirtualMachine { return Ok(stored); } let resolved = JSC__JSInternalPromise__resolvedPromise(global_ref, ret); - self.pending_internal_promise = Some(resolved); - self.pending_internal_promise_is_protected = false; + self.set_pending_internal_promise(Some(resolved)); return Ok(resolved); } } @@ -2403,9 +2416,7 @@ impl VirtualMachine { p }; - self.pending_internal_promise = Some(promise); - self.pending_internal_promise_is_protected = false; - JSValue::from_cell(promise).ensure_still_alive(); + self.set_pending_internal_promise(Some(promise)); Ok(promise) } else { let global = self.global; @@ -2414,9 +2425,7 @@ impl VirtualMachine { jsc::JSModuleLoader::load_and_evaluate_module_ptr(global, Some(&main_str)) .map(NonNull::as_ptr) .ok_or(crate::CrateError::JSError)?; - self.pending_internal_promise = Some(promise); - self.pending_internal_promise_is_protected = false; - JSValue::from_cell(promise).ensure_still_alive(); + self.set_pending_internal_promise(Some(promise)); Ok(promise) } } @@ -3556,13 +3565,8 @@ impl VirtualMachine { // the JSC module loader registry. self.global().reload().expect("Failed to reload"); self.hot_reload_counter += 1; - if self.pending_internal_promise_is_protected { - if let Some(p) = self.pending_internal_promise { - JSValue::from_cell(p).unprotect(); - } - self.pending_internal_promise_is_protected = false; - } - // reload_entry_point() stores into pending_internal_promise on every return path. + // reload_entry_point() stores into pending_internal_promise on every + // return path; that store unprotects the outgoing promise. let main = self.main; // Note: reshaped for borrowck — copy the `RawSlice` first to avoid // overlapping `&self`/`&mut self` borrows. @@ -4572,10 +4576,7 @@ impl VirtualMachine { // SAFETY: hook contract. let p = unsafe { (hooks.load_preloads)(self) }?; if !p.is_null() { - JSValue::from_cell(p).ensure_still_alive(); - self.pending_internal_promise = Some(p); - JSValue::from_cell(p).protect(); - self.pending_internal_promise_is_protected = true; + self.set_pending_internal_promise(Some(p)); return Ok(p); } } @@ -4587,9 +4588,7 @@ impl VirtualMachine { let promise = jsc::JSModuleLoader::load_and_evaluate_module_ptr(global, Some(&main_str)) .map(NonNull::as_ptr) .ok_or(crate::CrateError::JSError)?; - self.pending_internal_promise = Some(promise); - self.pending_internal_promise_is_protected = false; - JSValue::from_cell(promise).ensure_still_alive(); + self.set_pending_internal_promise(Some(promise)); Ok(promise) } @@ -4759,13 +4758,7 @@ impl VirtualMachine { self.overridden_main.deinit(); self.entry_point_result.value.deinit(); self.entry_point_result.cjs_set_value = false; - if let Some(promise) = self.pending_internal_promise { - if self.pending_internal_promise_is_protected { - JSValue::from_cell(promise).unprotect(); - self.pending_internal_promise_is_protected = false; - } - self.pending_internal_promise = None; - } + self.set_pending_internal_promise(None); self.has_patched_run_main = false; self.set_main(b""); self.main_hash = 0; diff --git a/src/runtime/hw_exports.rs b/src/runtime/hw_exports.rs index dd343b1a658c..8d4c4d16a3ab 100644 --- a/src/runtime/hw_exports.rs +++ b/src/runtime/hw_exports.rs @@ -102,8 +102,7 @@ pub fn set_override_module_run_main_promise( promise: *mut JSInternalPromise, ) { if vm.pending_internal_promise.is_none() { - vm.pending_internal_promise = Some(promise); - vm.pending_internal_promise_is_protected = false; + vm.set_pending_internal_promise(Some(promise)); } } diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index deafdd440bff..cf8d033672ae 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -661,8 +661,8 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS // SAFETY: per fn contract — `vm` is the live per-thread VM. unsafe { (*vm).is_in_preload = true }; // Note: copy the raw ptr into a guard-owned local so the defer body - // doesn't borrow the fn param — later `(*vm).pending_internal_promise = …` - // would otherwise alias the guard's capture. + // doesn't borrow the fn param — later `(*vm)` accesses would otherwise + // alias the guard's capture. let vm_for_guard = vm; scopeguard::defer! { // SAFETY: per fn contract. @@ -774,8 +774,14 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS } }; - // SAFETY: per fn contract. - unsafe { (*vm).pending_internal_promise = Some(promise) }; + // SAFETY: per fn contract — short-lived `&mut *vm` for the field store; + // `promise` is a live JSC heap cell from `import_ptr` just above. + unsafe { (*vm).set_pending_internal_promise(Some(promise)) }; + // The wait loop below tolerates `pending_internal_promise` being + // swapped by HMR inside `tick()`/`auto_tick()`; that swap unprotects + // this local. Keep an independent scoped protect so the + // `.unwrap_or(promise)` fallback and the post-loop `status()` read + // stay rooted across a swap (JSC protect is refcounted). let _protected = JSValue::from_cell(promise).protected(); // ── wait ──────────────────────────────────────────────────────── @@ -825,7 +831,6 @@ unsafe fn load_preloads(vm: *mut VirtualMachine) -> bun_jsc::CrateResult<*mut JS if unsafe { &*promise }.status() == PromiseStatus::Rejected { return Ok(promise); } - // `_protected` drops here → unprotect. } // Under --isolate each test file gets diff --git a/test/cli/hot/hot.test.ts b/test/cli/hot/hot.test.ts index df1af3e01bd2..824c7b8d5ff8 100644 --- a/test/cli/hot/hot.test.ts +++ b/test/cli/hot/hot.test.ts @@ -766,3 +766,80 @@ ${Buffer.alloc(counter * 2, " ").toString()}throw new Error(${counter});`, }, longTimeout, ); + +// JSModuleLoader::loadModule's returned promise is not retained by JSC once +// it settles. pending_internal_promise holds it as a raw cell pointer and is +// polled every tick by the --hot run loop (via +// report_exception_in_hot_reloaded_module_if_needed and reload()). Without a +// GC root, a collection between ticks can free the cell and the next poll +// reads a dead JSPromise (Sentry BUN-38CT: SIGSEGV in JSPromise::status -> +// WriteBarrierBase::get). Conservative stack scanning usually keeps it alive +// on the caller's stack, which is why the crash is rare and the race is not +// deterministically reproducible in-process; this test instead asserts the +// invariant the fix establishes: the stored promise is always GC-protected. +it( + "roots the entry-point evaluation promise so per-tick status reads can't see a freed cell", + async () => { + const root = join(cwd, "pip-root.js"); + // reload_entry_point stores pending_internal_promise before module + // evaluation runs, so heapStats() inside the entry module observes whether + // that store took a protect(). A later setImmediate snapshot confirms the + // root persists once the promise has settled and nothing in JSC references + // it anymore. + writeFileSync( + root, + ` + const { heapStats } = require("bun:jsc"); + function protectedPromises() { + const counts = heapStats().protectedObjectTypeCounts; + let n = 0; + for (const [k, v] of Object.entries(counts)) { + if (k.includes("Promise")) n += v; + } + return { n, counts }; + } + const during = protectedPromises(); + setImmediate(() => { + const after = protectedPromises(); + console.log(JSON.stringify({ during, after })); + process.exit(0); + }); + `, + ); + + await using proc = spawn({ + cmd: [bunExe(), "--hot", "--no-clear-screen", "run", root], + env: bunEnv, + cwd, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const line = stdout.split("\n").find(l => l.startsWith("{")); + if (!line) { + throw new Error(`no JSON line in stdout.\nstdout: ${JSON.stringify(stdout)}\nstderr: ${stderr}`); + } + const { during, after } = JSON.parse(line); + // At minimum the entry-point evaluation promise must be protected while + // stored in pending_internal_promise, both during evaluation and on the + // next tick (the window the run loop reads it in). Assert on a combined + // object so the full protectedObjectTypeCounts map appears in the failure + // diff when either >=1 check fails. + expect({ + duringProtectedPromises: during.n >= 1, + afterProtectedPromises: after.n >= 1, + duringCounts: during.counts, + afterCounts: after.counts, + }).toEqual({ + duringProtectedPromises: true, + afterProtectedPromises: true, + duringCounts: expect.any(Object), + afterCounts: expect.any(Object), + }); + expect(exitCode).toBe(0); + }, + timeout, +);