diff --git a/changelog.d/9595-thenable-assimilation-wrapper-rooting.md b/changelog.d/9595-thenable-assimilation-wrapper-rooting.md new file mode 100644 index 0000000000..5faa641551 --- /dev/null +++ b/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. diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index 44869b2c89..fc67e7a3bf 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/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()) { diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/thenable_assimilation.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/thenable_assimilation.rs new file mode 100644 index 0000000000..72fef6878f --- /dev/null +++ b/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::(), + f64::from_bits(ptr_bits(then_handle.get_raw_mut_ptr::() 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::() 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); + } +} diff --git a/crates/perry-runtime/src/promise/assimilate.rs b/crates/perry-runtime/src/promise/assimilate.rs index 577c534edd..a41c1e2c6f 100644 --- a/crates/perry-runtime/src/promise/assimilate.rs +++ b/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::(), + reason_handle.get_nanbox_f64(), + ); + return crate::value::js_nanbox_pointer( + rejected_handle.get_raw_mut_ptr::() 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::() 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::() 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::() as i64); + let reject_f64 = crate::value::js_nanbox_pointer(reject_handle.get_raw_mut_ptr::() 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::() as i64) } #[cfg(test)] diff --git a/crates/perry-runtime/src/promise/combinators.rs b/crates/perry-runtime/src/promise/combinators.rs index c88c4429ca..3f30accfd8 100644 --- a/crates/perry-runtime/src/promise/combinators.rs +++ b/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::() 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::() 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::() as u64); + let reject_f64 = f64::from_bits(reject_handle.get_raw_mut_ptr::() 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::() 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::() as i64) } /// Assimilate an object-literal thenable whose `then` is an own/inherited DATA diff --git a/crates/perry-runtime/src/util_promisify.rs b/crates/perry-runtime/src/util_promisify.rs index 262d6bbcd1..56e85fb12c 100644 --- a/crates/perry-runtime/src/util_promisify.rs +++ b/crates/perry-runtime/src/util_promisify.rs @@ -702,6 +702,11 @@ extern "C" fn callbackify_outer_thunk(closure: *const ClosureHeader, rest_value: let data = (arr as *const u8).add(std::mem::size_of::()) as *const f64; crate::closure::js_native_call_value(fn_handle.get_nanbox_f64(), data, original_arg_len) }; + // #9539: `returned` is a heap value that outlives two closure allocations, + // an interned-key lookup and `js_assimilate_thenable` (which runs the + // thenable's own `then`). Root it so every later read sees the current + // address rather than a pre-collection one. + let returned_handle = scope.root_nanbox_f64(returned); // Build the onFulfilled / onRejected closures (bound to the user callback). let fulfilled = js_closure_alloc(callbackify_fulfilled_thunk as *const u8, 1); @@ -727,7 +732,7 @@ extern "C" fn callbackify_outer_thunk(closure: *const ClosureHeader, rest_value: ); // Native Perry Promise → attach our handlers directly. - let promise_ptr = promise_ptr_from_value(returned); + let promise_ptr = promise_ptr_from_value(returned_handle.get_nanbox_f64()); if !promise_ptr.is_null() { let promise_handle = scope.root_raw_mut_ptr(promise_ptr); js_promise_attach_handlers( @@ -740,8 +745,10 @@ extern "C" fn callbackify_outer_thunk(closure: *const ClosureHeader, rest_value: // Class-based thenable (e.g. a custom Promise subclass) → assimilate into a // real Promise wrapper, then attach. - let assimilated = crate::promise::js_assimilate_thenable(returned); - let assimilated_ptr = promise_ptr_from_value(assimilated); + let assimilated = scope.root_nanbox_f64(crate::promise::js_assimilate_thenable( + returned_handle.get_nanbox_f64(), + )); + let assimilated_ptr = promise_ptr_from_value(assimilated.get_nanbox_f64()); if !assimilated_ptr.is_null() { let promise_handle = scope.root_raw_mut_ptr(assimilated_ptr); js_promise_attach_handlers( @@ -756,14 +763,16 @@ extern "C" fn callbackify_outer_thunk(closure: *const ClosureHeader, rest_value: // as an own field, not a vtable method, so `js_assimilate_thenable` passes // it through unchanged. Probe for a callable `.then` field and invoke it // with the thenable as `this`, passing our fulfilled/rejected handlers. - if let Some(then_fn) = callable_then_field(returned) { + if let Some(then_fn) = callable_then_field(returned_handle.get_nanbox_f64()) { let then_handle = scope.root_nanbox_f64(then_fn); let on_fulfilled = nanbox_pointer(fulfilled_handle.get_raw_const_ptr::() as *const u8); let on_rejected = nanbox_pointer(rejected_handle.get_raw_const_ptr::() as *const u8); let args = [on_fulfilled, on_rejected]; - let prev_this = scope.root_nanbox_f64(crate::object::js_implicit_this_set(returned)); + let prev_this = scope.root_nanbox_f64(crate::object::js_implicit_this_set( + returned_handle.get_nanbox_f64(), + )); unsafe { crate::closure::js_native_call_value( then_handle.get_nanbox_f64(), @@ -777,7 +786,7 @@ extern "C" fn callbackify_outer_thunk(closure: *const ClosureHeader, rest_value: // Not a Promise or thenable — Node throws `TypeError` synchronously because // it attempts to call `.then` on the result. - throw_callbackify_not_thenable(returned); + throw_callbackify_not_thenable(returned_handle.get_nanbox_f64()); } /// Locate a callable own `then` field on a heap-object value (object-literal @@ -805,10 +814,15 @@ fn callable_then_field(value: f64) -> Option { if gc_header.obj_type != crate::gc::GC_TYPE_OBJECT { return None; } - let obj = addr as *const crate::object::ObjectHeader; - let key = js_string_from_bytes(b"then".as_ptr(), 4); - let then_value = - crate::object::js_object_get_field_by_name_f64(obj, key as *const crate::StringHeader); + // #9539: interning `"then"` allocates, so the receiver must be a root + // across it — a raw local would be read back at its pre-collection address. + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_raw_const_ptr(addr as *const crate::object::ObjectHeader); + let key_handle = scope.root_string_ptr(js_string_from_bytes(b"then".as_ptr(), 4)); + let then_value = crate::object::js_object_get_field_by_name_f64( + obj_handle.get_raw_const_ptr::(), + key_handle.get_raw_const_ptr::(), + ); if is_callable_closure(then_value) { Some(then_value) } else { diff --git a/test-files/test_gap_9539_callbackify_thenable_exit_gc.ts b/test-files/test_gap_9539_callbackify_thenable_exit_gc.ts new file mode 100644 index 0000000000..8ad3eb3dd2 --- /dev/null +++ b/test-files/test_gap_9539_callbackify_thenable_exit_gc.ts @@ -0,0 +1,56 @@ +// #9539 — callbackify must keep synchronously-resolved object thenables and +// their queued promise reactions valid across nursery collections. The +// callbacks all complete before the observable output; the regression was an +// exit-time SIGSEGV when the final microtask checkpoint read a stale pointer. + +import * as util from "node:util"; + +const N = 6000; + +function churn(): number { + const tmp: any[] = []; + for (let k = 0; k < 480; k++) { + tmp.push({ k, s: "t" + k, pad: [k, k + 1] }); + } + return tmp.length; +} + +function check(name: string, factory: (i: number) => any): void { + let bad = 0; + const notes: string[] = []; + for (let i = 0; i < N; i++) { + const c: any = factory(i); + const want = "c" + i + ":480"; + let got: any; + try { + got = c.run(); + } catch (error: any) { + got = "THREW:" + (error && error.message); + } + if (got !== want) { + bad++; + if (bad <= 2) notes.push("[" + i + " got=" + String(got) + "]"); + } + } + console.log(name + " bad=" + bad + notes.join("")); +} + +function host(i: number, run: (this: any) => string): any { + return { id: i, inner: { def: "c" + i }, run }; +} + +check("callbackify_object_thenable", function (i) { + return host(i, function (this: any) { + let n = 0; + const callbackified = util.callbackify(function () { + return { + then: function (resolve: any, _reject: any) { + n = churn(); + resolve(1); + }, + }; + } as any); + callbackified(function () {}); + return this.inner.def + ":" + n; + }); +});