diff --git a/crates/perry-runtime/src/gc/heap_snapshot.rs b/crates/perry-runtime/src/gc/heap_snapshot.rs index e6f69d7ae9..b9405d178b 100644 --- a/crates/perry-runtime/src/gc/heap_snapshot.rs +++ b/crates/perry-runtime/src/gc/heap_snapshot.rs @@ -229,17 +229,10 @@ pub fn gc_build_v8_heap_snapshot_json() -> String { // Approximate the live set: collect first so unreachable malloc // objects are freed and fully-dead nursery blocks are reset before // the walk picks the population (Node's writeHeapSnapshot also - // forces a full GC). Force the conservative native-stack scan for - // this collection: in the default `auto` mode a full collect can - // miss top-level locals held only on the native stack and reclaim - // live objects (pre-existing explicit-`gc()` bug, see #4977 — - // repro: `const k = {nested:{deep:"s"}}; gc();` corrupts - // `k.nested.deep`). A diagnostics API must never make that worse. - let prev = super::roots::set_conservative_stack_scan_override(Some( - super::roots::ConservativeStackScanMode::Full, - )); + // forces a full GC). `js_gc_collect` forces the conservative + // native-stack scan when no per-thread override is pinned (#4977), + // so top-level locals held only on the native stack survive. js_gc_collect(); - super::roots::set_conservative_stack_scan_override(prev); // Free-list slots are dead-but-unreclaimed space inside live // blocks; exclude them from the dump. diff --git a/crates/perry-runtime/src/gc/policy.rs b/crates/perry-runtime/src/gc/policy.rs index cfcd427492..687283e3bd 100644 --- a/crates/perry-runtime/src/gc/policy.rs +++ b/crates/perry-runtime/src/gc/policy.rs @@ -552,10 +552,7 @@ pub(super) fn flush_deferred_gc_request() { if manual_gc_blocked_by_unsafe_zone() { return; } - crate::weakref::clear_pending_finalization_jobs(); - gc_collect_inner_with_trigger(GcTriggerSnapshot::capture(GcTriggerKind::Manual)) - .emit_after_current(); - crate::weakref::queue_pending_finalization_callbacks_after_gc(); + manual_gc_collect_now(); } DeferredGcRequest::Collect(kind) => { if gc_blocked_by_unsafe_zone() { @@ -1551,6 +1548,14 @@ pub extern "C" fn js_gc_collect() { if defer_gc_request(DeferredGcRequest::Collect(GcTriggerKind::Manual)) { return; } + manual_gc_collect_now(); +} + +/// Run an explicit (`gc()`) full collection. The `gc()` callsite may hold live +/// module-init/top-level locals only on the native stack, so the collection +/// forces the conservative native-stack scan (#4977); see `ManualGcScanGuard`. +fn manual_gc_collect_now() { + let _scan = super::roots::ManualGcScanGuard::force_full_scan(); crate::weakref::clear_pending_finalization_jobs(); gc_collect_inner_with_trigger(GcTriggerSnapshot::capture(GcTriggerKind::Manual)) .emit_after_current(); diff --git a/crates/perry-runtime/src/gc/roots.rs b/crates/perry-runtime/src/gc/roots.rs index f5e2e4e496..7d1bcd538a 100644 --- a/crates/perry-runtime/src/gc/roots.rs +++ b/crates/perry-runtime/src/gc/roots.rs @@ -178,7 +178,7 @@ thread_local! { /// set this to `Auto` (skip) inside their controlled-root scopes so a forced /// collection still reclaims the objects they hold only as native-stack /// locals; see `ScopedRootScannerRegistryGuard`. - static CONSERVATIVE_STACK_SCAN_OVERRIDE: std::cell::Cell> = + pub(super) static CONSERVATIVE_STACK_SCAN_OVERRIDE: std::cell::Cell> = const { std::cell::Cell::new(None) }; } @@ -190,6 +190,42 @@ pub(crate) fn set_conservative_stack_scan_override( CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.replace(mode)) } +/// Scoped guard forcing the conservative native-stack scan for an explicit +/// `gc()` collection (#4977). In the default `Auto` mode a full collection +/// skips the native scan, but at a `gc()` callsite live module-init/top-level +/// locals may be held only on the native stack — neither the precise +/// shadow-stack roots nor the module-var scanners cover them — so the +/// collector reclaimed live object graphs and later field reads returned +/// dangling-pointer garbage. An already-pinned per-thread override wins (the +/// GC unit tests pin `Auto` so a forced collection still reclaims objects they +/// hold only as native-stack locals), and an explicit +/// `PERRY_CONSERVATIVE_STACK_SCAN` env value beats any override either way, +/// so the bisection escape hatch keeps working. +pub(super) struct ManualGcScanGuard { + engaged: bool, +} + +impl ManualGcScanGuard { + pub(super) fn force_full_scan() -> Self { + let engaged = CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| { + if c.get().is_some() { + return false; + } + c.set(Some(ConservativeStackScanMode::Full)); + true + }); + Self { engaged } + } +} + +impl Drop for ManualGcScanGuard { + fn drop(&mut self) { + if self.engaged { + CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.set(None)); + } + } +} + pub(super) fn conservative_stack_scan_mode() -> ConservativeStackScanMode { // Explicit env var wins (so the dedicated mode tests and ops bisection keep // working), then a per-thread override, then the build-time default. diff --git a/crates/perry-runtime/src/gc/tests/roots.rs b/crates/perry-runtime/src/gc/tests/roots.rs index c01decb41b..5bf82162d5 100644 --- a/crates/perry-runtime/src/gc/tests/roots.rs +++ b/crates/perry-runtime/src/gc/tests/roots.rs @@ -805,3 +805,38 @@ fn test_shadow_stack_restore_drops_orphaned_bound_slots() { js_shadow_frame_pop(run_frame); assert_eq!(shadow_stack_depth(), 0); } + +#[test] +fn manual_gc_scan_guard_forces_full_scan_only_when_unpinned() { + use crate::gc::roots::{ManualGcScanGuard, CONSERVATIVE_STACK_SCAN_OVERRIDE}; + + let prev = set_conservative_stack_scan_override(None); + + // Unpinned: the guard engages a Full override for its lifetime (#4977 — + // explicit gc() must see top-level locals held only on the native stack). + { + let _scan = ManualGcScanGuard::force_full_scan(); + assert_eq!( + CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.get()), + Some(ConservativeStackScanMode::Full) + ); + } + assert_eq!(CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.get()), None); + + // Pinned (the GC unit-test guards pin Auto so forced collections still + // reclaim native-stack locals): the guard must not replace the override. + set_conservative_stack_scan_override(Some(ConservativeStackScanMode::Auto)); + { + let _scan = ManualGcScanGuard::force_full_scan(); + assert_eq!( + CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.get()), + Some(ConservativeStackScanMode::Auto) + ); + } + assert_eq!( + CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.get()), + Some(ConservativeStackScanMode::Auto) + ); + + set_conservative_stack_scan_override(prev); +} diff --git a/test-files/test_issue_4977_gc_toplevel_locals.ts b/test-files/test_issue_4977_gc_toplevel_locals.ts new file mode 100644 index 0000000000..2a81592920 --- /dev/null +++ b/test-files/test_issue_4977_gc_toplevel_locals.ts @@ -0,0 +1,16 @@ +// #4977: explicit gc() in default auto stack-scan mode reclaimed live +// top-level locals — string fields read back as dangling-pointer garbage. +class Widget { + x = 1; + constructor(public name: string) {} +} + +const keep = { nested: { deep: "leaf-string-4916" } }; +const w = new Widget("widget-name-4977"); + +gc(); + +console.log(keep.nested.deep.length); +console.log(keep.nested.deep); +console.log(w.name); +console.log(w.x);