fix(runtime): root the assimilated thenable wrapper across the user then (#9539)

changelog.d/9595-thenable-assimilation-wrapper-rooting.md

@@ -0,0 +1,43 @@
+### Fixed
+
+- **Thenable assimilation now returns the wrapper promise's post-collection
+  address; `util.callbackify` with an object-literal thenable SIGSEGV'd at exit
+  (#9539).** ~6,000 `util.callbackify(fn)(cb)` calls whose `fn` returns
+  `{ then(resolve) { …allocate…; resolve(v) } }` produced every callback's
+  correct result and then died with status 139 in
+  `promise::microtasks::pump_protected`, where node exits 0.
+
+  `assimilate_via_then_property` (and the class-vtable arm of
+  `js_assimilate_thenable`) allocate the wrapper `Promise`, hand the user's
+  `then` two resolving closures capturing it, run that `then`, and NaN-box the
+  wrapper out of a bare `*mut Promise` Rust local. The `then` body is user
+  code: with the moving nursery a loop safepoint inside it evacuates the young
+  generation. The wrapper survives and moves — the closures' raw-`i64` capture
+  words are pointer-bearing, so the collector rewrites them and `resolve(v)`
+  settles the promise at its NEW address — but the Rust local is not a GC root,
+  so the function returned the pre-collection address, i.e. retired from-space.
+  `callbackify_outer_thunk` then classified that word as a Promise, rooted it
+  and attached its reactions to it, so a dead promise reached the task queue and
+  the final microtask checkpoint dereferenced it.
+
+  Every value that outlives an allocation or a user-JS call in these functions
+  now lives in a `RuntimeHandleScope` handle — a mutable GC root the evacuating
+  minor rewrites in place — and each use re-reads through the handle: the
+  returned wrapper, the thenable receiver and its `then` action, the resolving
+  closures, `callbackify`'s `returned` across its closure allocations and
+  across `js_assimilate_thenable`, and `callable_then_field`'s receiver across
+  the `"then"` intern.
+
+  `PERRY_GC_PROTECT_FROMSPACE=1` names the stale object exactly
+  (`RETIRED FROM-SPACE … obj_type=5 size=80`, faulting on the
+  `obj_type == GC_TYPE_PROMISE` test inside `callbackify_outer_thunk`), and
+  `PERRY_GC_MOVING_LOOP_POLLS=0` makes even the unfixed build pass — the control
+  that names the moving minor inside `then` as the mechanism. Gap test
+  `test_gap_9539_callbackify_thenable_exit_gc.ts` is status 139 on 5/5 unfixed
+  runs and exits 0 on 10/10 fixed runs, plus 5/5 each under from-space
+  protection, a 1 MiB nursery, forced evacuation and
+  `PERRY_GC_MOVING_SAFEPOINT=0`. New unit test
+  `gc/tests/runtime_roots/thenable_assimilation.rs` drives a native `then` that
+  forces a copying minor before calling `resolve(1)`; unfixed it reports
+  `Pending != Fulfilled`. `perry-runtime` is 3011 passed / 0 failed
+  single-threaded.

crates/perry-runtime/src/gc/tests/runtime_roots.rs

@@ -18,6 +18,7 @@ mod side_table_scanners;
 mod string_normalize_form;
 mod string_slice;
 mod symbol_description;
+mod thenable_assimilation;
 mod transient_handles;
 
 fn assert_panics_with(expected: &str, f: impl FnOnce()) {

crates/perry-runtime/src/gc/tests/runtime_roots/thenable_assimilation.rs

@@ -0,0 +1,99 @@
+//! #9539 — thenable assimilation must hand back the wrapper promise's
+//! POST-collection address.
+//!
+//! `assimilate_via_then_property` / `js_assimilate_thenable` allocate a wrapper
+//! Promise, hand the user's `then` a pair of resolving closures that capture it,
+//! and then run that `then`. The body is arbitrary user code, and with the
+//! moving nursery a loop safepoint inside it evacuates the young generation.
+//! The wrapper survives — the closures' capture words are pointer-bearing and
+//! get rewritten — but the bare `*mut Promise` Rust local used to build the
+//! return value is not a GC root, so the pre-fix code returned the address the
+//! promise had *before* the callback collected. `util.callbackify` then
+//! classified that retired from-space word as a Promise, rooted it and attached
+//! reactions to it, and the exit-time microtask checkpoint faulted.
+
+use super::*;
+
+/// A native stand-in for a user `then(resolve, reject)` whose body allocates
+/// enough to collect — the `churn()` loop of the end-to-end fixture. It roots
+/// `resolve` the way a compiled JS frame does (its locals are precise roots),
+/// so the wrapper promise stays reachable through the closure capture and is
+/// RELOCATED rather than freed.
+extern "C" fn test_thenable_then_force_minor_gc(
+    _closure: *const crate::closure::ClosureHeader,
+    resolve: f64,
+    _reject: f64,
+) -> f64 {
+    let scope = RuntimeHandleScope::new();
+    let resolve_handle = scope.root_nanbox_f64(resolve);
+    let _ = crate::gc::gc_collect_minor();
+    let args = [1.0f64];
+    unsafe {
+        crate::closure::js_native_call_value(
+            resolve_handle.get_nanbox_f64(),
+            args.as_ptr(),
+            args.len(),
+        );
+    }
+    f64::from_bits(crate::value::TAG_UNDEFINED)
+}
+
+#[test]
+fn test_assimilated_thenable_wrapper_survives_then_callback_copied_minor_gc() {
+    let _guard = CopyingNurseryTestGuard::new(0);
+    let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers();
+    activate_malloc_registry_for_tests();
+    register_runtime_handle_root_scanner_for_tests();
+    gc_register_mutable_root_scanner(promise_mutable_root_scanner);
+    crate::closure::js_register_closure_arity(test_thenable_then_force_minor_gc as *const u8, 2);
+
+    let scope = RuntimeHandleScope::new();
+    let then_closure =
+        crate::closure::js_closure_alloc(test_thenable_then_force_minor_gc as *const u8, 0);
+    let then_handle = scope.root_raw_mut_ptr(then_closure);
+
+    // `{ then(resolve, reject) { … } }` — an object literal (class_id 0), so
+    // assimilation takes the `then`-as-data-property path.
+    let thenable_handle = scope.root_raw_mut_ptr(crate::object::js_object_alloc(0, 1));
+    let key_handle = scope.root_string_ptr(crate::string::js_string_from_bytes(b"then".as_ptr(), 4));
+    crate::object::js_object_set_field_by_name(
+        thenable_handle.get_raw_mut_ptr(),
+        key_handle.get_raw_const_ptr::<crate::StringHeader>(),
+        f64::from_bits(ptr_bits(then_handle.get_raw_mut_ptr::<u8>() as usize)),
+    );
+
+    let before = gc_collection_count();
+    let assimilated = crate::promise::js_assimilate_thenable(f64::from_bits(ptr_bits(
+        thenable_handle.get_raw_mut_ptr::<u8>() as usize,
+    )));
+    assert!(
+        gc_collection_count() > before,
+        "the thenable's `then` body must force a copying minor GC while the wrapper is live"
+    );
+
+    let bits = assimilated.to_bits();
+    assert_eq!(
+        bits & TAG_MASK,
+        POINTER_TAG,
+        "assimilation must return a heap pointer"
+    );
+    let promise = (bits & POINTER_MASK) as *mut crate::promise::Promise;
+    unsafe {
+        assert_eq!(
+            (*header_from_user_ptr(promise as *const u8)).obj_type,
+            GC_TYPE_PROMISE,
+            "the returned address must still be a live Promise after the callback's collection"
+        );
+        // The discriminating assertion: `resolve(1)` settled the promise at its
+        // POST-collection address (the capture word was rewritten). Returning
+        // the pre-collection address — the #9539 bug — hands back the retired
+        // from-space copy, which is still Pending.
+        assert_eq!(
+            (*promise).state,
+            crate::promise::PromiseState::Fulfilled,
+            "assimilation must return the wrapper the resolving closure settled, \
+             not its pre-collection address"
+        );
+        assert_eq!((*promise).value, 1.0);
+    }
+}

crates/perry-runtime/src/promise/assimilate.rs

@@ -408,31 +408,64 @@ pub(super) fn assimilate_via_then_property(value: f64) -> f64 {
     if super::then_probe::definitely_no_then(value) {
         return value;
     }
+    // #9539: `js_native_call_value` below runs the thenable's `then` body —
+    // arbitrary user code. With the moving nursery (default-on) a loop
+    // safepoint inside it evacuates the young generation, so every heap value
+    // this function still needs afterwards must live in a transient handle the
+    // collector rewrites; a bare `*mut Promise` local would name retired
+    // from-space by the time the call returns.
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let value_handle = scope.root_nanbox_f64(value);
+
     // `Get(value, "then")` (27.2.1.3.2 step 8). A throwing getter is an abrupt
     // completion → resolve-with-thenable rejects the wrapper promise with the
     // thrown value (step 9), rather than letting the exception unwind out of the
     // resolve path. Return that rejected wrapper so callers chain it.
     let then_val = match combinator_catch_js(|| unsafe {
-        crate::value::js_dynamic_object_get_property(value, b"then".as_ptr() as *const i8, 4)
+        crate::value::js_dynamic_object_get_property(
+            value_handle.get_nanbox_f64(),
+            b"then".as_ptr() as *const i8,
+            4,
+        )
     }) {
         Ok(v) => v,
         Err(reason) => {
-            let p = js_promise_new();
-            js_promise_reject(p, reason);
-            return crate::value::js_nanbox_pointer(p as i64);
+            let reason_handle = scope.root_nanbox_f64(reason);
+            let rejected_handle = scope.root_raw_mut_ptr(js_promise_new());
+            js_promise_reject(
+                rejected_handle.get_raw_mut_ptr::<Promise>(),
+                reason_handle.get_nanbox_f64(),
+            );
+            return crate::value::js_nanbox_pointer(
+                rejected_handle.get_raw_mut_ptr::<Promise>() as i64
+            );
         }
     };
-    if callable_closure_value(then_val).is_none() {
-        return value;
+    let then_handle = scope.root_nanbox_f64(then_val);
+    if callable_closure_value(then_handle.get_nanbox_f64()).is_none() {
+        return value_handle.get_nanbox_f64();
     }
 
-    let new_promise = js_promise_new();
-    let promise_i64 = new_promise as i64;
+    let promise_handle = scope.root_raw_mut_ptr(js_promise_new());
 
-    let resolve_closure = crate::closure::js_closure_alloc(promise_resolve_fn as *const u8, 1);
-    crate::closure::js_closure_set_capture_ptr(resolve_closure, 0, promise_i64);
-    let reject_closure = crate::closure::js_closure_alloc(promise_reject_fn as *const u8, 1);
-    crate::closure::js_closure_set_capture_ptr(reject_closure, 0, promise_i64);
+    let resolve_handle = scope.root_raw_mut_ptr(crate::closure::js_closure_alloc(
+        promise_resolve_fn as *const u8,
+        1,
+    ));
+    crate::closure::js_closure_set_capture_ptr(
+        resolve_handle.get_raw_mut_ptr(),
+        0,
+        promise_handle.get_raw_mut_ptr::<Promise>() as i64,
+    );
+    let reject_handle = scope.root_raw_mut_ptr(crate::closure::js_closure_alloc(
+        promise_reject_fn as *const u8,
+        1,
+    ));
+    crate::closure::js_closure_set_capture_ptr(
+        reject_handle.get_raw_mut_ptr(),
+        0,
+        promise_handle.get_raw_mut_ptr::<Promise>() as i64,
+    );
 
     // Pass the resolving functions as proper NaN-boxed function values (not the
     // raw closure-pointer-bits convention used internally by
@@ -441,20 +474,30 @@ pub(super) fn assimilate_via_then_property(value: f64) -> f64 {
     // so `typeof onFulfilled === "function"` must hold (test262
     // yield-star-async-* / yield-star-next-then-* check this). A NaN-boxed
     // closure is still invoked through the normal call path.
-    let resolve_f64 = crate::value::js_nanbox_pointer(resolve_closure as i64);
-    let reject_f64 = crate::value::js_nanbox_pointer(reject_closure as i64);
+    let resolve_f64 =
+        crate::value::js_nanbox_pointer(resolve_handle.get_raw_mut_ptr::<u8>() as i64);
+    let reject_f64 = crate::value::js_nanbox_pointer(reject_handle.get_raw_mut_ptr::<u8>() as i64);
     let args = [resolve_f64, reject_f64];
 
     // Bind `this` to the thenable so a non-arrow `then` body reads the right
     // receiver, then call `Get(value, "then")` as a value (own data property).
-    let this_scope = crate::gc::RuntimeHandleScope::new(); // #9445
-    let prev = this_scope.root_nanbox_f64(crate::object::js_implicit_this_set(value));
+    let prev = scope.root_nanbox_f64(crate::object::js_implicit_this_set(
+        value_handle.get_nanbox_f64(),
+    )); // #9445
     unsafe {
-        crate::closure::js_native_call_value(then_val, args.as_ptr(), args.len());
+        crate::closure::js_native_call_value(
+            then_handle.get_nanbox_f64(),
+            args.as_ptr(),
+            args.len(),
+        );
     }
     crate::object::js_implicit_this_set(prev.get_nanbox_f64());
 
-    crate::value::js_nanbox_pointer(new_promise as i64)
+    // Re-read the wrapper through its handle: the user `then` just ran and may
+    // have relocated it (#9539). Returning `new_promise`'s pre-call address is
+    // what handed callers (util.callbackify, await) a retired from-space
+    // pointer they then classified, rooted and attached reactions to.
+    crate::value::js_nanbox_pointer(promise_handle.get_raw_mut_ptr::<Promise>() as i64)
 }
 
 #[cfg(test)]

crates/perry-runtime/src/promise/combinators.rs

@@ -1261,24 +1261,49 @@ pub extern "C" fn js_assimilate_thenable(value: f64) -> f64 {
         };
 
     // Allocate the wrapper promise plus resolve/reject closures pointing at it.
-    let new_promise = js_promise_new();
-    let promise_i64 = new_promise as i64;
+    //
+    // #9539: the vtable `then` below is user code, and with the moving nursery
+    // (default-on) a loop safepoint inside it evacuates the young generation.
+    // The wrapper survives — the closures' capture words are rewritten — but a
+    // bare `*mut Promise` local is not a GC root, so reading it after the call
+    // returns a retired from-space address. Hold every value the call outlives
+    // in transient handles and re-read them through those handles.
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let this_handle = scope.root_raw_const_ptr(obj_ptr);
+    let promise_handle = scope.root_raw_mut_ptr(js_promise_new());
 
-    let resolve_closure = crate::closure::js_closure_alloc(promise_resolve_fn as *const u8, 1);
-    crate::closure::js_closure_set_capture_ptr(resolve_closure, 0, promise_i64);
-    let reject_closure = crate::closure::js_closure_alloc(promise_reject_fn as *const u8, 1);
-    crate::closure::js_closure_set_capture_ptr(reject_closure, 0, promise_i64);
+    let resolve_handle = scope.root_raw_mut_ptr(crate::closure::js_closure_alloc(
+        promise_resolve_fn as *const u8,
+        1,
+    ));
+    crate::closure::js_closure_set_capture_ptr(
+        resolve_handle.get_raw_mut_ptr(),
+        0,
+        promise_handle.get_raw_mut_ptr::<Promise>() as i64,
+    );
+    let reject_handle = scope.root_raw_mut_ptr(crate::closure::js_closure_alloc(
+        promise_reject_fn as *const u8,
+        1,
+    ));
+    crate::closure::js_closure_set_capture_ptr(
+        reject_handle.get_raw_mut_ptr(),
+        0,
+        promise_handle.get_raw_mut_ptr::<Promise>() as i64,
+    );
 
     // The user's `then(onFulfilled, onRejected)` reads each parameter as a
     // raw f64 closure pointer (matching the convention used by
     // `js_promise_new_with_executor`).
-    let resolve_f64 = f64::from_bits(resolve_closure as u64);
-    let reject_f64 = f64::from_bits(reject_closure as u64);
+    let resolve_f64 = f64::from_bits(resolve_handle.get_raw_mut_ptr::<u8>() as u64);
+    let reject_f64 = f64::from_bits(reject_handle.get_raw_mut_ptr::<u8>() as u64);
 
     // Invoke `value.then(resolve, reject)` via the vtable. Mirrors
     // `call_vtable_method` in object.rs: NaN-box `this` with POINTER_TAG so
     // the method body sees a real instance pointer.
-    let this_f64 = f64::from_bits(JSValue::pointer(obj_ptr as *mut u8).bits());
+    let this_f64 = f64::from_bits(
+        JSValue::pointer(this_handle.get_raw_mut_ptr::<crate::object::ObjectHeader>() as *mut u8)
+            .bits(),
+    );
     unsafe {
         match then_param_count {
             0 => {
@@ -1297,7 +1322,8 @@ pub extern "C" fn js_assimilate_thenable(value: f64) -> f64 {
         }
     }
 
-    crate::value::js_nanbox_pointer(new_promise as i64)
+    // Re-read the wrapper through its handle — see the #9539 note above.
+    crate::value::js_nanbox_pointer(promise_handle.get_raw_mut_ptr::<Promise>() as i64)
 }
 
 /// Assimilate an object-literal thenable whose `then` is an own/inherited DATA