fix(gc): avoid closure side-table GC deadlock

crates/perry-runtime/src/closure/dynamic_props.rs

@@ -128,6 +128,25 @@ fn merge_closure_prop_map(
     }
 }
 
+fn forwarded_heap_owner(owner: usize) -> Option<usize> {
+    if owner == 0 {
+        return None;
+    }
+    if matches!(
+        crate::arena::classify_heap_generation(owner),
+        crate::arena::HeapGeneration::Unknown
+    ) {
+        return None;
+    }
+    unsafe {
+        let header = crate::value::addr_class::try_read_gc_header(owner)?;
+        if header.gc_flags & crate::gc::GC_FLAG_FORWARDED == 0 {
+            return None;
+        }
+        Some(crate::gc::forwarding_address(header as *const _) as usize)
+    }
+}
+
 pub(crate) fn closure_dynamic_props_owner_moved(old_owner: usize, new_owner: usize) {
     if old_owner == 0 || new_owner == 0 || old_owner == new_owner {
         return;
@@ -207,46 +226,64 @@ pub(crate) fn visit_closure_static_prototype_slot_mut(
 /// ajv's `validate.errors = [{ msg }]`) had its element objects freed
 /// behind the still-live array.
 pub fn scan_closure_dynamic_props_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) {
-    let mut moved = Vec::new();
-    if let Ok(mut props) = get_closure_props().lock() {
-        for (&owner, closure_props) in props.iter_mut() {
-            // Metadata key rewrite. Only fires in rewrite-phase modes;
-            // mark phases return `false` here without recording the key
-            // as a root (so the side-table entry doesn't itself keep
-            // the closure alive — same semantics as overflow-field
-            // metadata, see `visit_metadata_usize_slot`).
-            let mut new_owner = owner;
-            if visitor.visit_metadata_usize_slot(&mut new_owner) {
-                moved.push((owner, new_owner));
-            }
-            // #1802: trace every stored value in every phase. In `Mark`
-            // / `CopyingMark` this keeps `fn.errors = [...]` and its
-            // transitive contents reachable; in rewrite phases it
-            // updates the slot bits when a value was forwarded.
-            for value in closure_props.values_mut() {
-                visitor.visit_nanbox_f64_slot(value);
-            }
+    let prop_owners = get_closure_props()
+        .lock()
+        .ok()
+        .map(|props| props.keys().copied().collect::<Vec<_>>())
+        .unwrap_or_default();
+    for owner in prop_owners {
+        let Some(mut closure_props) = get_closure_props()
+            .lock()
+            .ok()
+            .and_then(|mut props| props.remove(&owner))
+        else {
+            continue;
+        };
+
+        // Metadata key rewrite. Only fires in rewrite-phase modes; mark phases
+        // return `false` here without recording the key as a root (so the
+        // side-table entry doesn't itself keep the closure alive).
+        let mut new_owner = owner;
+        visitor.visit_metadata_usize_slot(&mut new_owner);
+        // #1802: trace every stored value in every phase. In `Mark` /
+        // `CopyingMark` this keeps `fn.errors = [...]` and its transitive
+        // contents reachable; in rewrite phases it updates slot bits when a
+        // value was forwarded.
+        for value in closure_props.values_mut() {
+            visitor.visit_nanbox_f64_slot(value);
         }
-        for (old_owner, new_owner) in moved {
-            if let Some(old_props) = props.remove(&old_owner) {
-                merge_closure_prop_map(&mut props, new_owner, old_props);
-            }
+        if new_owner == owner {
+            new_owner = forwarded_heap_owner(owner).unwrap_or(owner);
+        }
+
+        if let Ok(mut props) = get_closure_props().lock() {
+            merge_closure_prop_map(&mut props, new_owner, closure_props);
         }
     }
-    let mut moved_prototypes = Vec::new();
-    if let Ok(mut prototypes) = get_closure_prototypes().lock() {
-        for (owner, proto_bits) in prototypes.iter_mut() {
-            let old_owner = *owner;
-            let mut new_owner = old_owner;
-            if visitor.visit_metadata_usize_slot(&mut new_owner) {
-                moved_prototypes.push((old_owner, new_owner));
-            }
-            visitor.visit_nanbox_u64_slot(proto_bits);
+
+    let prototype_owners = get_closure_prototypes()
+        .lock()
+        .ok()
+        .map(|prototypes| prototypes.keys().copied().collect::<Vec<_>>())
+        .unwrap_or_default();
+    for owner in prototype_owners {
+        let Some(mut proto_bits) = get_closure_prototypes()
+            .lock()
+            .ok()
+            .and_then(|mut prototypes| prototypes.remove(&owner))
+        else {
+            continue;
+        };
+
+        let mut new_owner = owner;
+        visitor.visit_metadata_usize_slot(&mut new_owner);
+        visitor.visit_nanbox_u64_slot(&mut proto_bits);
+        if new_owner == owner {
+            new_owner = forwarded_heap_owner(owner).unwrap_or(owner);
         }
-        for (old_owner, new_owner) in moved_prototypes {
-            if let Some(proto_bits) = prototypes.remove(&old_owner) {
-                prototypes.insert(new_owner, proto_bits);
-            }
+
+        if let Ok(mut prototypes) = get_closure_prototypes().lock() {
+            prototypes.insert(new_owner, proto_bits);
         }
     }
     // #3655: re-key the deleted-keys side table when a closure moves. The
@@ -316,6 +353,10 @@ pub extern "C" fn js_value_is_closure(value_bits: i64) -> i32 {
 /// Get a dynamic property stored on a closure.
 /// Returns TAG_UNDEFINED if not found.
 pub fn closure_get_dynamic_prop(ptr: usize, prop: &str) -> f64 {
+    if !is_closure_ptr(ptr) {
+        return f64::from_bits(crate::value::TAG_UNDEFINED);
+    }
+
     if let Some(acc) = crate::object::get_accessor_descriptor(ptr, prop) {
         if acc.get == 0 {
             return f64::from_bits(crate::value::TAG_UNDEFINED);
@@ -591,6 +632,8 @@ pub extern "C" fn js_closure_unbind_this(val: f64) -> f64 {
 mod tests_1802 {
     use super::*;
 
+    static SIDE_TABLE_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+
     /// #1802: the side-table values must be visited in mark phases, not
     /// only during the metadata-rewrite tail. Pre-fix
     /// `scan_closure_dynamic_props_roots_mut` early-returned unless
@@ -600,6 +643,7 @@ mod tests_1802 {
     /// removed, the adapter sees every stored value's bits.
     #[test]
     fn dyn_prop_values_are_visited_in_mark_phase() {
+        let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap();
         // A unique synthetic closure address (just an integer key — the
         // scanner doesn't deref it during value visitation; the
         // metadata-key visitor is a no-op for non-heap addresses).
@@ -631,6 +675,52 @@ mod tests_1802 {
         }
     }
 
+    #[test]
+    fn dyn_prop_scanner_visits_values_without_holding_props_lock() {
+        let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap();
+        let owner: usize = 0xC10C_AB1E_0000_1803;
+        let value_bits: u64 = 0x7FFD_AAAA_BBBB_CCCD;
+        closure_set_dynamic_prop(owner, "errors", f64::from_bits(value_bits));
+
+        let mut saw_value = false;
+        let mut lock_was_free = false;
+        {
+            let mut mark = |v: f64| {
+                if v.to_bits() == value_bits {
+                    saw_value = true;
+                    lock_was_free = get_closure_props().try_lock().is_ok();
+                }
+            };
+            let mut visitor = crate::gc::RuntimeRootVisitor::for_copy(&mut mark);
+            scan_closure_dynamic_props_roots_mut(&mut visitor);
+        }
+
+        assert!(
+            saw_value,
+            "scanner did not visit the stored closure prop value"
+        );
+        assert!(
+            lock_was_free,
+            "scanner must not hold CLOSURE_PROPS while visitor callbacks can move closures"
+        );
+
+        if let Ok(mut props) = get_closure_props().lock() {
+            props.remove(&owner);
+        }
+    }
+
+    #[test]
+    fn dyn_prop_get_ignores_non_closure_receivers() {
+        let _guard = SIDE_TABLE_TEST_LOCK.lock().unwrap();
+        let obj = crate::object::js_object_alloc(0, 0) as usize;
+
+        assert_eq!(
+            closure_get_dynamic_prop(obj, "payload").to_bits(),
+            crate::value::TAG_UNDEFINED,
+            "ordinary objects must not enter closure-only Function.prototype fallback"
+        );
+    }
+
     /// #4740: `is_closure_ptr` must NOT dereference an address in the
     /// `[0x10000, 0x100000)` native-handle band. Web Fetch response handles
     /// (`0x40000+`), node:http / axios / fastify ids live there and are not

crates/perry/tests/gc_write_barrier_stress.rs

@@ -19,7 +19,24 @@
 //! literals are avoided too (compile-time blowup, #4880).
 
 use std::path::PathBuf;
-use std::process::Command;
+use std::process::{Command, Output, Stdio};
+use std::time::{Duration, Instant};
+
+#[cfg(unix)]
+use std::os::unix::process::CommandExt;
+
+#[cfg(unix)]
+unsafe extern "C" {
+    fn kill(pid: i32, sig: i32) -> i32;
+    fn setsid() -> i32;
+}
+
+#[cfg(unix)]
+const SIGTERM: i32 = 15;
+#[cfg(unix)]
+const SIGKILL: i32 = 9;
+
+const COMPILED_BINARY_TIMEOUT: Duration = Duration::from_secs(120);
 
 fn perry_bin() -> PathBuf {
     PathBuf::from(env!("CARGO_BIN_EXE_perry"))
@@ -46,11 +63,61 @@ fn compile_and_run(source: &str) -> std::process::Output {
         String::from_utf8_lossy(&compile.stderr)
     );
 
-    Command::new(&output)
-        .env("PERRY_GC_FORCE_EVACUATE", "1")
-        .env("PERRY_GC_VERIFY_EVACUATION", "1")
-        .output()
-        .expect("run compiled binary")
+    let mut run = Command::new(&output);
+    run.env("PERRY_GC_FORCE_EVACUATE", "1")
+        .env("PERRY_GC_VERIFY_EVACUATION", "1");
+    run_compiled_binary_with_timeout(run, COMPILED_BINARY_TIMEOUT)
+}
+
+fn run_compiled_binary_with_timeout(mut command: Command, timeout: Duration) -> Output {
+    command.stdout(Stdio::piped()).stderr(Stdio::piped());
+    #[cfg(unix)]
+    unsafe {
+        command.pre_exec(|| {
+            if setsid() == -1 {
+                return Err(std::io::Error::last_os_error());
+            }
+            Ok(())
+        });
+    }
+
+    let mut child = command.spawn().expect("run compiled binary");
+    let start = Instant::now();
+    loop {
+        match child.try_wait().expect("poll compiled binary") {
+            Some(_) => {
+                return child
+                    .wait_with_output()
+                    .expect("collect compiled binary output")
+            }
+            None if start.elapsed() >= timeout => {
+                #[cfg(unix)]
+                unsafe {
+                    let pgid = -(child.id() as i32);
+                    kill(pgid, SIGTERM);
+                    std::thread::sleep(Duration::from_millis(250));
+                    if child.try_wait().expect("poll after SIGTERM").is_none() {
+                        kill(pgid, SIGKILL);
+                    }
+                }
+                #[cfg(not(unix))]
+                {
+                    child.kill().expect("kill timed out compiled binary");
+                }
+                let output = child
+                    .wait_with_output()
+                    .expect("collect timed out compiled binary output");
+                panic!(
+                    "compiled binary timed out after {:?}\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}",
+                    timeout,
+                    output.status,
+                    String::from_utf8_lossy(&output.stdout),
+                    String::from_utf8_lossy(&output.stderr)
+                );
+            }
+            None => std::thread::sleep(Duration::from_millis(50)),
+        }
+    }
 }
 
 fn assert_ok_output(run: &std::process::Output, expected: &str) {