fix(gc): bind the catch parameter and the closure this/new.target slots, and snapshot the Object.assign string source

changelog.d/7216-catch-closure-this-assign-source-roots.md

@@ -0,0 +1,14 @@
+### Fixed
+
+- **codegen/runtime: three more root-store holes from #7202's enumeration and #7207's review (#7209, #7208, #7215)**. Same invariant as #7207 — a GC-managed value's storage must be one the collector actually rewrites — reached three ways it had not been reached before: a slot that was *reserved and never bound*, a slot that was never reserved at all, and a raw borrow into a heap payload.
+
+  - **The `catch (e)` parameter (#7209)**. `collect_pointer_typed_locals` assigns the catch parameter a shadow-slot index — it is implicitly `Any`, i.e. pointer-possible — so `js_shadow_frame_enter`'s count already included it. `stmt/try_stmt.rs` allocated the storage with `alloca_entry`, stored the exception into it, and never emitted `js_shadow_slot_bind`: **the frame was sized for a root that did not exist**, `active[idx]` stayed false forever, and the collector never dereferenced the alloca. Sharper than an ordinary missing root because `js_clear_exception()` runs two lines earlier and drops the RUNTIME's own reference — from there the exception is reachable only through unrooted stack memory while arbitrary user code runs, so a precise-roots collection can **sweep** it rather than merely relocate it. Fixed with `emit_shadow_slot_bind_for_local`, which reuses the reserved index rather than growing the frame; emitted after the store rather than hoisted to entry setup, because on the non-throwing path the alloca is never written and an entry-hoisted bind would make the slot active over uninitialized stack bytes.
+
+  - **A closure's captured `this` / `new.target` (#7208)**. `codegen/method.rs:316` and `:1344` size their frames `m.len() + 1` and bind that extra slot to the `this` alloca; `codegen/closure.rs:589` used `m.len()` and reserved nothing, so there was no index to bind and both capture slots — plain `blk.alloca(DOUBLE)` holding a heap receiver for the whole body, read by every `ctx.this_stack.last()` consumer — were invisible to the collector. The in-tree comment exempting them ("the capture reads … run in the entry-block prologue, ahead of any statement that could collect") justifies the timing of the READ and says nothing about the lifetime of the SLOT. Now reserves one index per captured `this` / `new.target` and binds each inline in the entry block immediately after its store.
+
+  - **The `Object.assign` string source (#7215)**, from CodeRabbit's review of #7207. `str_bytes_from_jsvalue` returns a pointer INTO the source `StringHeader`'s data region for any string past the 5-byte SSO limit — header and payload are one contiguous `arena_alloc_gc` block and `GC_TYPE_STRING` is `movable` — and its own safety note forbids holding that pointer across a GC cycle. `object_assign_string_source` held it across three allocation points per character. #7207 opened a `RuntimeHandleScope` in that very function and rooted the target, the key and the value; it missed the source because the source is a *borrow*, not a JSValue in that frame. Snapshotted once before the loop, matching the `expandos` precedent a few lines down.
+
+### Testing
+
+- `test-files/test_gap_gc_catch_param_rooting.ts` and `test_gap_gc_closure_this_capture_rooting.ts`, both registered in `test-parity/gc_repsel_corpus.txt`. At `origin/main` (`1679e22b4`), compiled **and** run with `PERRY_GC_MOVING_LOOP_POLLS=1`: the closure one is `exit=139` (SIGSEGV) 3/3 and `bad 4` on the evacuating arm; the catch one is `message 8 field 8` of 400, 3/3 deterministic. Both clean 5/5 under polls with this change, 3/3 on the evacuating arm, 3/3 under the shipped default, byte-exact against Node 26.5.1. The corpus entry notes that `catch_param_rooting` is the one member that can also fail under a **non-moving** precise-roots collection — the sweep case — so it belongs on `cons_scan_off`, not only on the `requires=move` arms.
+- `test_gap_gc_assign_string_source_rooting.ts` ships **green at base**, and the fix comment says so rather than implying a witness. An instrumented build rooted both the source and the target inside the copy loop and counted relocations: `chars=26001 src_moves=0 tgt_moves=1`. The target moved — so a moving collection genuinely occurs inside that function, which is what makes #7207's target rooting load-bearing — but that source could not move, because at 26 KB it exceeds `LARGE_OBJECT_THRESHOLD_BYTES` (16 KiB) and `arena_alloc_gc` births it `GC_FLAG_TENURED` in the non-moving old generation. Under the threshold it is a movable nursery string, but one call then allocates too little to reliably span a collection; four shapes across four arm configurations, up to 120 observed GC cycles, all stayed clean. The exposure is the band just under 16 KiB, and the safety margin rests entirely on a tunable constant — which is the reason to fix it rather than document it.

crates/perry-codegen/src/codegen/closure.rs

@@ -586,7 +586,21 @@ pub(super) fn compile_closure(
         let flat_const_ids: std::collections::HashSet<u32> =
             cross_module.flat_const_arrays.keys().copied().collect();
         let m = crate::collectors::collect_pointer_typed_locals(params, body, &flat_const_ids);
-        lf.enable_shadow_frame(m.len() as u32);
+        // #7208: reserve one slot per CAPTURED `this` / `new.target`, exactly
+        // as `codegen/method.rs:316` and `:1344` do with their `+ 1`.
+        //
+        // Both are `blk.alloca(DOUBLE)` holding a heap receiver for the WHOLE
+        // closure body, read by every `ctx.this_stack.last()` consumer. Without
+        // a reserved index there is nothing to bind them to, so an evacuating
+        // minor neither marked nor rewrote them and every load below a
+        // collection point named from-space. The in-tree note further down
+        // ("the `this` / `new.target` capture reads ... run in the entry-block
+        // prologue, ahead of any statement that could collect") justifies the
+        // timing of the READ; it says nothing about the lifetime of the SLOT,
+        // which spans the body.
+        let capture_root_slots =
+            u32::from(captures_this || enclosing_class.is_some()) + u32::from(captures_new_target);
+        lf.enable_shadow_frame(m.len() as u32 + capture_root_slots);
         m
     } else {
         std::collections::HashMap::new()
@@ -681,6 +695,13 @@ pub(super) fn compile_closure(
     // Arrow-in-class leftover path (`enclosing_class.is_some()` without
     // the object-literal patch) keeps the old 0.0 sentinel — reads
     // return a bogus value but don't crash.
+    // #7208: the two reserved indices sit immediately above the local slots,
+    // mirroring `method.rs`'s single `this_shadow_slot_idx`. Bound INLINE right
+    // after each store, in the same entry block, so the store dominates the
+    // bind — an entry-setup hoist would make the slot active while the alloca
+    // still held stack garbage.
+    let capture_root_base = shadow_slot_map.len() as u32;
+    let bind_capture_slot = super::helpers::shadow_stack_enabled();
     let new_target_stack = if captures_new_target {
         let new_target_cap_idx = auto_captures.len() as u32;
         let blk = lf.block_mut(0).unwrap();
@@ -693,6 +714,12 @@ pub(super) fn compile_closure(
         );
         let v = blk.bitcast_i64_to_double(&bits);
         blk.store(DOUBLE, &v, &slot);
+        if bind_capture_slot {
+            blk.call_void(
+                "js_shadow_slot_bind",
+                &[(I32, &capture_root_base.to_string()), (PTR, &slot)],
+            );
+        }
         vec![slot]
     } else {
         Vec::new()
@@ -714,6 +741,14 @@ pub(super) fn compile_closure(
         } else {
             blk.store(DOUBLE, "0.0", &slot);
         }
+        if bind_capture_slot {
+            // The `new.target` slot took `capture_root_base` when it existed.
+            let idx = capture_root_base + u32::from(captures_new_target);
+            blk.call_void(
+                "js_shadow_slot_bind",
+                &[(I32, &idx.to_string()), (PTR, &slot)],
+            );
+        }
         vec![slot]
     } else {
         Vec::new()

crates/perry-codegen/src/stmt/try_stmt.rs

@@ -114,6 +114,28 @@ pub(crate) fn lower_try(
             let slot = ctx.func.alloca_entry(DOUBLE);
             ctx.locals.insert(*id, slot.clone());
             ctx.block().store(DOUBLE, &exc, &slot);
+            // #7209: BIND the slot the frame is already sized for.
+            //
+            // `collect_pointer_typed_locals` assigns the catch parameter an
+            // index — it is implicitly `Any`, i.e. pointer-possible — so
+            // `js_shadow_frame_enter`'s count already includes it. Nothing ever
+            // bound it, so `active[idx]` stayed false and the collector never
+            // dereferenced this alloca: the frame was sized for a root that did
+            // not exist.
+            //
+            // Sharper than an ordinary missing root, because
+            // `js_clear_exception()` two lines up has already dropped the
+            // RUNTIME's reference. From here the exception is reachable only
+            // through this alloca, and the catch body is arbitrary user code —
+            // so a precise-roots collection can SWEEP it, not merely move it.
+            //
+            // Emitted here rather than hoisted to entry setup precisely because
+            // the slot must not go active before the store: on the non-throwing
+            // path this alloca is never written, and an entry-hoisted bind
+            // would hand the root-word decoder uninitialized stack bytes. After
+            // the store is what `Stmt::Let` does for every ordinary local, and
+            // it reuses the RESERVED index rather than growing the frame.
+            crate::expr::emit_shadow_slot_bind_for_local(ctx, *id);
         }
         if let Some(f) = finally {
             // Per spec TryStatement : try Block Catch Finally — a throw

crates/perry-runtime/src/object/alloc.rs

@@ -1141,12 +1141,42 @@ unsafe fn object_assign_string_source(
     let Ok(s) = std::str::from_utf8(bytes) else {
         return;
     };
+    // #7214: SNAPSHOT the source before allocating anything.
+    //
+    // `str_bytes_from_jsvalue` returns a pointer INTO the source
+    // `StringHeader`'s data region for any string past the SSO limit (header
+    // and payload are one contiguous `arena_alloc_gc` block), and its own
+    // safety note says so: "Callers must not hold this pointer past a
+    // subsequent `scratch` modification or a GC cycle that could sweep the
+    // heap-backed `StringHeader`." The loop below holds it across three
+    // allocation points per character.
+    //
+    // MEASURED, because the size argument that makes this survivable is not one
+    // to rely on. An instrumented build rooted both the source and the target
+    // and counted relocations across the loop: on a 26 001-character source,
+    // `src_moves=0 tgt_moves=1` — collections DO happen inside this function
+    // (which is what makes the #7200 target rooting above load-bearing), but
+    // that source could not move because at 26 KB it is over
+    // `LARGE_OBJECT_THRESHOLD_BYTES` and `arena_alloc_gc` births it TENURED in
+    // the non-moving old generation. Shrink it under the threshold and it
+    // becomes a movable nursery string — but then one call allocates too little
+    // to reliably span a collection, and none was observed.
+    //
+    // So the exposure is real and narrow: a source in the band just under
+    // 16 KiB is both movable and long enough to allocate ~32 000 times. There
+    // is NO runtime witness for it and I am not implying otherwise; what there
+    // is, is a documented callee contract this violated and a safety margin
+    // that rests entirely on a tunable constant. One owned copy on a path that
+    // is already O(n) removes the dependence.
+    let owned: String = s.to_string();
+
     // #7200: three allocations per iteration (`key_ptr`, `value_ptr`, and the
-    // write funnel's interning/growth) with `target` and `key_ptr` live across
-    // them. The second `js_string_from_bytes` alone can move the first.
+    // write funnel's interning / keys-array growth) with `target` and `key_ptr`
+    // live across them. The probe above measured `tgt_moves=1`, so the target
+    // half of this is not hypothetical.
     let scope = crate::gc::RuntimeHandleScope::new();
     let tgt_h = scope.root_raw_mut_ptr(target);
-    for (idx, ch) in s.chars().enumerate() {
+    for (idx, ch) in owned.chars().enumerate() {
         let iter_scope = crate::gc::RuntimeHandleScope::new();
         let key = idx.to_string();
         let key_ptr = crate::string::js_string_from_bytes(key.as_ptr(), key.len() as u32);

test-files/test_gap_gc_assign_string_source_rooting.ts

@@ -0,0 +1,55 @@
+// #7214: `Object.assign(t, str)` / `{ ...str }` decoded the SOURCE string once,
+// into a raw `(*const u8, len)` view, and then held that borrow across every
+// allocation in the copy loop.
+//
+// `str_bytes_from_jsvalue` returns a pointer INTO the source `StringHeader`'s
+// data region for any string past the SSO limit, and says so in its own safety
+// note: "Callers must not hold this pointer past a subsequent `scratch`
+// modification or a GC cycle that could sweep the heap-backed `StringHeader`."
+// `object_assign_string_source` built a `&str` on that pointer and then hit
+// THREE allocation points per character — two `js_string_from_bytes` calls and
+// the write funnel's key interning / keys-array growth.
+//
+// #7207 opened a `RuntimeHandleScope` in that very function and rooted the
+// target, the key and the value. It did not root the source, because the source
+// is not a JSValue in that frame at all — it is a borrow. So an evacuating
+// minor moved the string and `chars()` walked from-space for every remaining
+// character.
+//
+// LIVE BY CONSTRUCTION AND ONLY ON THE MOVING ARMS. The source is a FRESH heap
+// string per iteration (built with `repeat`, well past the SSO limit), reachable
+// only from a shadow-bound local — so it survives the minor, which means it
+// MOVES. A non-moving collection leaves the bytes where they are and the borrow
+// stays accidentally valid.
+
+const ALPHA = "abcdefghijklmnopqrstuvwxyz";
+const WIDTH = 208;
+
+function run(): string {
+  let badChar = 0;
+  let badCount = 0;
+  for (let r = 0; r < 300; r++) {
+    // Past SHORT_STRING_MAX_LEN, so this is a real heap `StringHeader` in the
+    // nursery rather than an inline short string copied into the caller's
+    // scratch buffer.
+    const src: string = ALPHA.repeat(8);
+    const out: any = Object.assign({}, src);
+    let seen = 0;
+    for (let i = 0; i < WIDTH; i++) {
+      const got = out[i];
+      if (got !== undefined) {
+        seen++;
+      }
+      if (got !== ALPHA[i % 26]) {
+        badChar++;
+        break;
+      }
+    }
+    if (seen !== WIDTH) {
+      badCount++;
+    }
+  }
+  return "char " + badChar + " count " + badCount;
+}
+
+console.log("bad", run());

test-files/test_gap_gc_catch_param_rooting.ts

@@ -0,0 +1,49 @@
+// #7209: the `catch (e)` parameter has a shadow slot RESERVED for it and never
+// BOUND, so the exception object has no root at all for the whole catch body.
+//
+// `collectors/pointer_locals.rs` assigns the catch parameter a slot index (it
+// is implicitly `Any`, i.e. pointer-possible), so `js_shadow_frame_enter`'s
+// count already includes it. `stmt/try_stmt.rs` then allocates the parameter's
+// storage with `alloca_entry` and stores the exception into it — and never
+// emits `js_shadow_slot_bind`, so `active[idx]` stays false forever and the
+// collector never dereferences the alloca. The frame is sized for a root that
+// does not exist.
+//
+// What makes it sharp rather than merely untidy: `js_clear_exception()` runs
+// BEFORE the catch body is lowered, dropping the runtime's own reference. From
+// that point the only thing referring to the exception is the unrooted alloca.
+// Under a precise-roots collection the object can be SWEPT, not merely moved.
+//
+// LIVE BY CONSTRUCTION. `churn()` allocates hard enough to reach the collector,
+// and `e` is read AFTER it — both the message string and a field stored on the
+// error, so a relocated `Error` and a reclaimed one both show up.
+
+function churn(): number {
+  const a: any[] = [];
+  for (let i = 0; i < 600; i++) {
+    a.push({ i: i, s: "e" });
+  }
+  return a.length;
+}
+
+function run(): string {
+  let badChurn = 0;
+  let badMessage = 0;
+  let badField = 0;
+  for (let r = 0; r < 400; r++) {
+    try {
+      const err: any = new Error("boom" + r);
+      err.tag = r;
+      throw err;
+    } catch (e: any) {
+      // The exception is live across this call and nothing else refers to it.
+      const n = churn();
+      if (n !== 600) badChurn++;
+      if (e.message !== "boom" + r) badMessage++;
+      if (e.tag !== r) badField++;
+    }
+  }
+  return "churn " + badChurn + " message " + badMessage + " field " + badField;
+}
+
+console.log("bad", run());

test-files/test_gap_gc_closure_this_capture_rooting.ts

@@ -0,0 +1,55 @@
+// #7208: a closure's captured `this` lives in a plain `alloca_entry` that no
+// `js_shadow_slot_bind` names, so the collector neither marks nor rewrites it.
+//
+// `codegen/closure.rs` sized its shadow frame `enable_shadow_frame(m.len())`
+// where `codegen/method.rs` uses `m.len() + 1` — and that `+1` IS the `this`
+// slot, which `method.rs` then binds. Closures reserved nothing, so there was
+// no index to bind and the receiver was unrooted for the whole body.
+//
+// The in-tree comment claiming the capture reads are "exempt ... they run in
+// the entry-block prologue, ahead of any statement that could collect"
+// justifies the timing of the READ. It says nothing about the lifetime of the
+// SLOT, which spans every statement in the body — including the ones that
+// collect.
+//
+// LIVE BY CONSTRUCTION. The receiver is a temporary: after `make()` returns it
+// is reachable ONLY from the closure's capture cell, which IS a traced root —
+// so an evacuating minor MOVES it rather than freeing it, rewrites the capture
+// cell, and leaves the prologue's alloca copy naming from-space. `churn()` runs
+// between the prologue read and the `this.tag` read.
+
+function churn(): number {
+  const a: any[] = [];
+  for (let i = 0; i < 600; i++) {
+    a.push({ i: i, s: "c" });
+  }
+  return a.length;
+}
+
+class Holder {
+  tag: number;
+  label: string;
+  constructor(t: number) {
+    this.tag = t;
+    this.label = "h";
+  }
+  make(): () => string {
+    return () => {
+      const n = churn();
+      return this.label + ":" + (this.tag + (n - 600));
+    };
+  }
+}
+
+function run(): number {
+  let bad = 0;
+  for (let r = 0; r < 400; r++) {
+    const f = new Holder(r).make();
+    if (f() !== "h:" + r) {
+      bad++;
+    }
+  }
+  return bad;
+}
+
+console.log("bad", run());

test-parity/gc_repsel_corpus.txt

@@ -190,3 +190,22 @@ test_gap_repsel_gc_stress
 test_gap_gc_spread_accessor_rooting
 test_gap_gc_static_block_this_rooting
 test_gap_gc_inline_ctor_this_rooting
+
+# --- The catch parameter and the closure `this` capture (#7209, #7208) -------
+# Two more sites from the #7202 bare-alloca enumeration, both reserved-or-
+# missing shadow slots rather than late root stores.
+#
+# Measured on `origin/main` (1679e22b4), compiled AND run with
+# `PERRY_GC_MOVING_LOOP_POLLS=1`:
+#   catch_param_rooting          `message 8 field 8` of 400, 3/3 deterministic
+#   closure_this_capture_rooting exit=139 (SIGSEGV) 3/3; `bad 4` on the
+#                                evacuating arm
+#
+# `catch_param_rooting` is the one file in this corpus that can go wrong under a
+# NON-moving precise-roots collection as well: `js_clear_exception()` drops the
+# runtime's own reference before the catch body runs, so with the slot unbound
+# the exception is reachable from nothing the collector scans and can be SWEPT,
+# not merely relocated. Keep it exercised on `cons_scan_off`, not only on the
+# `requires=move` arms.
+test_gap_gc_catch_param_rooting
+test_gap_gc_closure_this_capture_rooting