diff --git a/changelog.d/10564-implicit-this-scope-rooting.md b/changelog.d/10564-implicit-this-scope-rooting.md new file mode 100644 index 0000000000..ef42c7ea66 --- /dev/null +++ b/changelog.d/10564-implicit-this-scope-rooting.md @@ -0,0 +1,116 @@ +Fixed a stale caller `this` after a moving minor in every runtime guard that +binds the implicit-`this` cell for a callback (#10490). + +`js_native_call_method`'s prototype-override early path (#9247) bound the +callee's receiver with a private `ImplicitThisScope` guard that kept the +DISPLACED value — the caller's receiver — in a plain `f64` field and wrote it +back in `Drop`. The callee is user code, so a copying minor inside it relocates +that receiver; a struct field is not a root, and the restore reinstalled a +retired from-space address. The caller's next `this.x` then read `undefined`: +`Object.setPrototypeOf(o, proto); o.run()` where `run` calls an allocating +method threw `Cannot read properties of undefined`, deterministically and with +no GC env knobs, and cheerio 1.2.0 crashed in `_findBySelector` on the 3rd of +200 `load()` iterations. The #9445 sweep rooted every +`let prev = js_implicit_this_set(..)` pair, but a save/restore split across a +guard's constructor and its `Drop` does not have that shape — and the same +shape sat behind the `Array.prototype` callback engines (`DenseThisGuard`, +11 dense methods; `ThisGuard`, 9 `js_arraylike_*` methods), where the caller's +`this` was equally corrupted by an allocating callback. + +One shared `object::ImplicitThisScope<'scope>` now replaces all four private +guards. It roots the displaced value in a borrowed `RuntimeHandleScope` and +re-reads that slot in `Drop`, so the restore follows the object through an +evacuation; the borrow forces the scope to outlive the guard. The +prototype-override path also re-reads its receiver after +`clone_closure_rebind_this` (that clone allocates). The accompanying audit of +every implicit-`this` save/restore in the runtime and stdlib fixed six more +displaced values held unrooted across user code: the accessor-receiver override +in the handle-method prototype walk, `new.target` in the Intl and Temporal +subclass `super()` bridges, and ten stdlib sites (domain, events, process +warnings, net, web streams, tls ALPNCallback, worker_threads). The remaining +save/restores — including the 121 rooted by #9445 — were verified rooted. + +Validation: `test-files/test_gap_10490_implicit_this_scope_rooting.ts` (17 +shapes: `setPrototypeOf`, `Object.create`, `__proto__` literals, class +instances with a swapped prototype, `call`/`apply`, a per-evaluation subclass, +and the dense / array-like callback engines) prints a non-zero `bad=` count on +12 of 17 cases before the fix and is byte-identical to node after it, in both +the default and `PERRY_NO_AUTO_OPTIMIZE=1` pipelines. Four runtime unit tests +in `gc/tests/runtime_roots/implicit_this_scope.rs` plant a callback that runs a +forced-evacuation copying minor and assert the restored cell holds the +receiver's relocated address; the three that exist pre-fix fail on it. The +issue's repro passes under `PERRY_GC_SCHEDULE_SEED=1..5 +PERRY_GC_SCHEDULE_RATE=1 PERRY_GC_PROTECT_FROMSPACE=1` (baseline fails all +five), and a scaled copy matches node with `PERRY_GC_SCHEDULE_ALLOC_KB=0` over +120,516 copying minors. cheerio 1.2.0 compiled from source now completes +200 × load + 3 queries with node's exact output. Gap suite: 814/820, the same +6 snapshot entries as the baseline, no new failures. Cost on the changed +dispatch path is one handle slot: +0.71 % instructions on a 5M-call +swapped-receiver microbenchmark, +0.36 % on 5M array-callback engine calls. + +Follow-up review finding (same PR): the rooting fix above makes every displaced +value survive a moving collection, but four sites still save/restore +IMPLICIT_THIS and `new.target` with a bare statement pair, not a guard -- +`fetch_globals.rs`'s Temporal/Intl subclass `super()` bridges, the +prototype-walk accessor dispatch in `handle_methods.rs`, and the stdlib +listener/getter dispatchers (`streams.rs` and its `net`/`tls`/`worker_threads` +siblings, `domain.rs`, `events.rs`, `events/warnings.rs`). When the bracketed +call throws, the restore statement textually follows it, so neither `longjmp` +nor a system unwind ever runs it -- both cells stay pinned at whatever the +failed call set them to for every later read. + +`exception.rs` already keeps a `catch_savepoints!` family of exactly this +shape (shadow stack, runtime handles, call-method depth, ...): one member per +piece of state a transport-skipped cleanup would otherwise leak, captured at +every `try` and replayed by `js_throw` before it transports the exception -- +uniformly for both `js_try_push`/`HandlerKind::Setjmp` and the generated-code +`js_eh_try_push`/`HandlerKind::Unwind`, which already funnel into one +`try_push_with_kind` -> `CatchSavepoint::capture()`, with `js_throw` calling +`.restore()` unconditionally before it branches on transport. `implicit_this` +and `new_target` now join that family. The captured bits are a second root for +whatever heap value they hold while a `try` is open, invisible to the live +cell's own scanner, so `scan_exception_roots_mut` now also walks the live +prefix of the per-thread savepoints slab and rewrites both fields across a +moving collection -- registered in the perex GC test harness too, which +clears the production scanner registry and had only restored the live-cell +scanner. + +Proven: a unit test that reproduces the bare save/call/restore shape using +only pre-existing public entry points (`js_implicit_this_set`/ +`js_new_target_set`/`catch_js_throw`/`js_throw`, none of them touched by this +fix, so the same test body runs unmodified on both trees) -- an inner +`js_throw` crossing the bare site leaves both cells stuck at the inner value +instead of the enclosing `try`'s baseline. Fails on the pre-fix tree with +`assertion left == right failed: the try open around the bare site must have +restored IMPLICIT_THIS` (left the inner sentinel, right the outer baseline); +passes after, along with the macro's own auto-generated nested-throw witness +for both new members. + +NOT proven: that any of the four named sites is reachable the way the finding +assumes. Targeting the most tractable one -- `handle_methods.rs`'s +prototype-walk accessor dispatch -- a temporary `eprintln!` placed directly on +that path never fired for a getter-throws test case built to exercise it, so +something else resolves that call first. The other three sites were not +probed at all. This is recorded, not fixed, in a follow-up issue. + +Validation: `cargo test --release -p perry-runtime --tests` +(`RUST_TEST_THREADS=1`): 3975 passed, 1 pre-existing failure +(`a_free_or_move_outside_every_scope_is_caught_in_debug_builds`, a +`debug_assert!` funnel that cannot fire under `--release`, already documented +in this PR's own validation table), 4 ignored. `cargo test --release +-p perry-stdlib --tests`: 138 passed, 1 pre-existing failure unrelated to this +change (`readline::stdin_data_listener_flows_without_raw_mode`; this fix +touches no file in `perry-stdlib`). `scripts/run_lint_gates.sh` +(`SKIP_COMPILE_GATES=1`, compile tier known-red on this host): 76 of 77 pass, +the one red (`Public benchmark evidence freshness`) pre-existing and +unrelated. 10 of 11 targeted exception/try-catch gap tests pass; the one +failure (`test_issue_7302_thread_throws`) reproduces identically on the +unmodified baseline (a Node-side environment artifact in this sandbox, not a +Perry regression). Instruction-count A/B (`perf stat`, 3 runs/arm, spread +<0.1%) on a 5,000,000-iteration try/catch loop that never throws: 895.28M +(baseline) vs 965.34M (fixed) instructions, +7.83% (~14 instructions per +`try`-push) -- the cost of two more TLS reads folded into the one savepoint +write every `try` already performs. Measured on a deliberately adversarial +microbenchmark (nothing but the `try`/`catch` itself); the original PR's own +dispatch-call microbenchmarks, which mix in real work, show proportionally +smaller deltas for comparable per-call additions. diff --git a/crates/perry-runtime/src/array/generic.rs b/crates/perry-runtime/src/array/generic.rs index 00c1add7c5..a470d0a77b 100644 --- a/crates/perry-runtime/src/array/generic.rs +++ b/crates/perry-runtime/src/array/generic.rs @@ -662,20 +662,6 @@ fn object_has_property_chain(obj_ptr: usize, key_val: f64) -> bool { false } -/// RAII-ish guard binding the callback `this` (the optional `thisArg`) for the -/// duration of a generic iteration, restoring the previous binding on drop. -struct ThisGuard(f64); -impl ThisGuard { - fn new(this_arg: f64) -> Self { - ThisGuard(crate::object::js_implicit_this_set(this_arg)) - } -} -impl Drop for ThisGuard { - fn drop(&mut self) { - crate::object::js_implicit_this_set(self.0); - } -} - // --------------------------------------------------------------------------- // Callback iteration methods. The callback receives `(value, index, O)` with // `O` the *original* receiver value; `this_arg` binds the callback's `this`. @@ -700,7 +686,7 @@ pub extern "C" fn js_arraylike_forEach(recv: f64, cb: f64, this_arg: f64) -> f64 return undef(); } let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -726,7 +712,7 @@ pub extern "C" fn js_arraylike_forEach(recv: f64, cb: f64, this_arg: f64) -> f64 #[no_mangle] pub extern "C" fn js_arraylike_map(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -774,7 +760,7 @@ pub extern "C" fn js_arraylike_map(recv: f64, cb: f64, this_arg: f64) -> f64 { #[no_mangle] pub extern "C" fn js_arraylike_filter(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -816,7 +802,7 @@ pub extern "C" fn js_arraylike_filter(recv: f64, cb: f64, this_arg: f64) -> f64 #[no_mangle] pub extern "C" fn js_arraylike_some(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -845,7 +831,7 @@ pub extern "C" fn js_arraylike_some(recv: f64, cb: f64, this_arg: f64) -> f64 { #[no_mangle] pub extern "C" fn js_arraylike_every(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -877,7 +863,7 @@ pub extern "C" fn js_arraylike_every(recv: f64, cb: f64, this_arg: f64) -> f64 { #[no_mangle] pub extern "C" fn js_arraylike_find(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -905,7 +891,7 @@ pub extern "C" fn js_arraylike_find(recv: f64, cb: f64, this_arg: f64) -> f64 { #[no_mangle] pub extern "C" fn js_arraylike_findIndex(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -931,7 +917,7 @@ pub extern "C" fn js_arraylike_findIndex(recv: f64, cb: f64, this_arg: f64) -> f #[no_mangle] pub extern "C" fn js_arraylike_findLast(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) @@ -961,7 +947,7 @@ pub extern "C" fn js_arraylike_findLast(recv: f64, cb: f64, this_arg: f64) -> f6 #[no_mangle] pub extern "C" fn js_arraylike_findLastIndex(recv: f64, cb: f64, this_arg: f64) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); - let _g = ThisGuard::new(this_arg); + let _g = crate::object::ImplicitThisScope::bind(&scope, this_arg); let cb_h = scope.root_nanbox_f64(cb); let recv_h = scope.root_nanbox_f64(to_object(recv)); // Spec order: LengthOfArrayLike(O) is read *before* the IsCallable(cb) diff --git a/crates/perry-runtime/src/array/iter_methods.rs b/crates/perry-runtime/src/array/iter_methods.rs index ae54e8b858..93372fe32f 100644 --- a/crates/perry-runtime/src/array/iter_methods.rs +++ b/crates/perry-runtime/src/array/iter_methods.rs @@ -159,18 +159,15 @@ mod rooted_iter_array_tests { /// Explicit-`thisArg` call sites route through the `js_arraylike_*` engine /// instead of these helpers. Arrow callbacks capture `this` lexically and /// are unaffected. -struct DenseThisGuard(f64); -impl DenseThisGuard { - fn bind_undefined() -> Self { - DenseThisGuard(crate::object::js_implicit_this_set(f64::from_bits( - crate::value::TAG_UNDEFINED, - ))) - } -} -impl Drop for DenseThisGuard { - fn drop(&mut self) { - crate::object::js_implicit_this_set(self.0); - } +/// +/// The displaced receiver is the caller's `this`, held across every callback +/// in the loop, so it is restored from a root in the iteration's `scope` +/// (#10490 — this guard used to keep it in a plain field). +#[inline] +fn bind_undefined_this( + scope: &crate::gc::RuntimeHandleScope, +) -> crate::object::ImplicitThisScope<'_> { + crate::object::ImplicitThisScope::bind(scope, undefined_value()) } /// #5989/#8117: `.forEach` on a receiver codegen could not prove is a @@ -286,7 +283,7 @@ pub extern "C" fn js_array_forEach(arr: *const ArrayHeader, callback: *const Clo Some(h) => h.get_nanbox_f64(), None => rooted.receiver(), }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); if crate::array::array_iteration_is_exotic(arr) { for i in 0..length as usize { let arr = rooted.arr(); @@ -362,7 +359,7 @@ pub extern "C" fn js_array_map( // conservative scan; that knob was deleted in #7611, so there is no // longer a configuration in which this rooting is optional. See gh #6206. let cb_handle = scope.root_raw_const_ptr(callback); - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); // ECMA-262 §23.1.3.20 step 5: ArraySpeciesCreate(O, len) runs BEFORE // the iteration — it reads `O.constructor` / `@@species` (firing any @@ -485,7 +482,7 @@ pub extern "C" fn js_array_map_discard(arr: *const ArrayHeader, callback: *const let current_callback = || { crate::value::js_nanbox_get_pointer(cb_handle.get_nanbox_f64()) as *const ClosureHeader }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); if crate::array::array_iteration_is_exotic(arr) { for i in 0..length as usize { let arr = rooted.arr(); @@ -548,7 +545,7 @@ pub extern "C" fn js_array_filter( let cb_site = crate::closure::DirectCall3::resolve(callback); // Root the callback across the loop — see js_array_map / gh #6206. let cb_handle = scope.root_raw_const_ptr(callback); - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); // ECMA-262 §23.1.3.7 step 5: ArraySpeciesCreate(O, 0) runs before the // iteration (validates `O.constructor` / `@@species`, throwing on a @@ -644,7 +641,7 @@ pub extern "C" fn js_array_find(arr: *const ArrayHeader, callback: *const Closur let current_callback = || { crate::value::js_nanbox_get_pointer(cb_handle.get_nanbox_f64()) as *const ClosureHeader }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); let exotic = crate::array::array_iteration_is_exotic(arr); for i in 0..length as usize { @@ -714,7 +711,7 @@ pub extern "C" fn js_array_findIndex( let current_callback = || { crate::value::js_nanbox_get_pointer(cb_handle.get_nanbox_f64()) as *const ClosureHeader }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); let exotic = crate::array::array_iteration_is_exotic(arr); for i in 0..length as usize { @@ -769,7 +766,7 @@ pub extern "C" fn js_array_find_last( let current_callback = || { crate::value::js_nanbox_get_pointer(cb_handle.get_nanbox_f64()) as *const ClosureHeader }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); let exotic = crate::array::array_iteration_is_exotic(arr); for i in (0..length).rev() { let element = if exotic { @@ -821,7 +818,7 @@ pub extern "C" fn js_array_find_last_index( let current_callback = || { crate::value::js_nanbox_get_pointer(cb_handle.get_nanbox_f64()) as *const ClosureHeader }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); let exotic = crate::array::array_iteration_is_exotic(arr); for i in (0..length).rev() { let element = if exotic { @@ -930,7 +927,7 @@ pub extern "C" fn js_array_some(arr: *const ArrayHeader, callback: *const Closur let current_callback = || { crate::value::js_nanbox_get_pointer(cb_handle.get_nanbox_f64()) as *const ClosureHeader }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); let exotic = crate::array::array_iteration_is_exotic(arr); for i in 0..length as usize { @@ -1091,7 +1088,7 @@ pub extern "C" fn js_array_every(arr: *const ArrayHeader, callback: *const Closu let current_callback = || { crate::value::js_nanbox_get_pointer(cb_handle.get_nanbox_f64()) as *const ClosureHeader }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); let exotic = crate::array::array_iteration_is_exotic(arr); for i in 0..length as usize { @@ -1160,7 +1157,7 @@ pub extern "C" fn js_array_flatMap( crate::value::JSValue::pointer(result as *const u8).bits(), )); }; - let _tg = DenseThisGuard::bind_undefined(); + let _tg = bind_undefined_this(&scope); for i in 0..length as usize { let Some(element) = rooted.present(i) else { diff --git a/crates/perry-runtime/src/exception.rs b/crates/perry-runtime/src/exception.rs index 53459f49fc..4d93dd9667 100644 --- a/crates/perry-runtime/src/exception.rs +++ b/crates/perry-runtime/src/exception.rs @@ -692,6 +692,9 @@ pub fn scan_exception_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) if (*s).has_exception { visitor.visit_nanbox_f64_raw_slot(&raw mut (*s).current_exception); } + // #10564 review finding: every open `try`'s captured implicit-`this`/ + // `new.target` snapshot is a second root for whatever it holds. + savepoints::scan_pending_trap_roots_mut(&mut (*s).savepoints, (*s).try_depth, visitor); }); } @@ -787,6 +790,54 @@ mod tests { assert_eq!(RuntimeHandleScope::active_len_for_tests(), base_handles); } + /// PR #10564 review finding, reproduced without touching the new + /// `implicit_this`/`new_target` savepoints directly: a bare + /// save/call/restore pair — no RAII guard — around a call that throws. + /// This is exactly the shape at the 4 named sites (fetch_globals.rs's + /// Temporal/Intl subclass bridges, handle_methods.rs's prototype-walk + /// accessor dispatch, the stdlib listener/getter dispatchers) before this + /// fix: the restore statement textually follows the call, so neither + /// `longjmp` nor a system unwind ever reaches it once the call throws. + /// + /// Deliberately uses only pre-existing public entry points + /// (`js_implicit_this_set`/`js_new_target_set`/`catch_js_throw`/ + /// `js_throw`) that this fix does not change, so the same test body + /// fails on the unfixed tree (nothing restores either cell across the + /// inner `js_throw`) and passes once `implicit_this`/`new_target` join + /// `catch_savepoints!` — the fail-before/pass-after proof for the + /// mechanism every one of the 4 sites shares. + #[test] + fn a_bare_save_call_restore_site_is_made_exception_safe_by_the_savepoint() { + let base_this = crate::object::js_implicit_this_get().to_bits(); + let base_nt = crate::object::js_new_target_get().to_bits(); + + let outcome: Result<(), f64> = catch_js_throw(|| { + // The caller's open `try` around the guarded call, e.g. user code + // wrapping `obj.method()` in `try {}`. + let inner: Result<(), f64> = catch_js_throw(|| { + // The bare save/call/restore pair itself: set, call something + // that throws, restore — except the restore is unreachable. + let _prev_this = crate::object::js_implicit_this_set(11.0); + let _prev_nt = crate::object::js_new_target_set(22.0); + js_throw(99.0) + }); + assert_eq!(inner, Err(99.0)); + assert_eq!( + crate::object::js_implicit_this_get().to_bits(), + base_this, + "the try open around the bare site must have restored IMPLICIT_THIS" + ); + assert_eq!( + crate::object::js_new_target_get().to_bits(), + base_nt, + "the try open around the bare site must have restored new.target" + ); + }); + assert_eq!(outcome, Ok(())); + assert_eq!(crate::object::js_implicit_this_get().to_bits(), base_this); + assert_eq!(crate::object::js_new_target_get().to_bits(), base_nt); + } + #[test] fn try_push_pop_beyond_old_limit_does_not_panic() { // Regression for #5065: old fixed limit was 128 and js_try_push panicked diff --git a/crates/perry-runtime/src/exception/savepoints.rs b/crates/perry-runtime/src/exception/savepoints.rs index b8410821c5..a0d1cdee71 100644 --- a/crates/perry-runtime/src/exception/savepoints.rs +++ b/crates/perry-runtime/src/exception/savepoints.rs @@ -188,6 +188,24 @@ catch_savepoints! { capture: crate::object::call_method_depth_savepoint, restore: crate::object::call_method_depth_restore, latch: catch_subsystem::ALWAYS, idle: 0; + // PR #10564 review finding: the runtime guards that displace IMPLICIT_THIS + // around a `super()`/accessor/listener call they don't own (Temporal/Intl + // subclass bridges, the handle-method prototype-walk accessor dispatch, + // the stdlib listener/getter dispatchers) are a bare save/call/restore + // pair, not `ImplicitThisScope` — neither transport runs the restore + // statement that follows the call. The captured value is a second root + // for the object the live cell's own scanner already protects; see + // `scan_pending_trap_roots_mut` below. + implicit_this: u64, + capture: crate::object::implicit_this_trap_savepoint, + restore: crate::object::implicit_this_trap_restore, + latch: catch_subsystem::ALWAYS, idle: crate::value::TAG_UNDEFINED; + // Same shape as `implicit_this`, for `new.target` (the Temporal/Intl + // subclass `super()` bridges save/restore both together). + new_target: u64, + capture: crate::object::new_target_trap_savepoint, + restore: crate::object::new_target_trap_restore, + latch: catch_subsystem::ALWAYS, idle: crate::value::TAG_UNDEFINED; // Includes removal of the process-wide outer-pump contribution. pump: u32, capture: crate::stdlib_pump::pump_depth_savepoint, @@ -235,5 +253,32 @@ catch_savepoints! { latch: catch_subsystem::DYN_EVAL, idle: 0; } +/// Root + rewrite `implicit_this`/`new_target` in every OPEN `try`'s captured +/// savepoint (PR #10564 review finding). +/// +/// `capture()` copies the live cells' bits into this per-depth slab +/// precisely so `js_throw` can put them back after a bare save/call/restore +/// site (see `object::this_binding::implicit_this_trap_savepoint`) gets +/// longjmp'd or unwound past. That copy is a second root for the same value +/// the live cell's own scanner (`object::this_binding:: +/// scan_implicit_this_roots_mut`) already protects, and it is invisible to +/// that scanner. A moving minor that runs while a `try` is open — before any +/// throw crosses it — must rewrite this copy too, or a later throw restores a +/// from-space address. Bounded by `try_depth <= MAX_TRY_DEPTH`, same as every +/// other read of this slab. +pub(super) fn scan_pending_trap_roots_mut( + savepoints: &mut [std::mem::MaybeUninit], + try_depth: usize, + visitor: &mut crate::gc::RuntimeRootVisitor<'_>, +) { + for entry in &mut savepoints[..try_depth] { + // SAFETY: every slot below `try_depth` was written by `capture()` in + // `try_push_with_kind` before `try_depth` advanced past it. + let entry = unsafe { entry.assume_init_mut() }; + visitor.visit_nanbox_u64_slot(&mut entry.implicit_this); + visitor.visit_nanbox_u64_slot(&mut entry.new_target); + } +} + #[cfg(test)] mod tests; diff --git a/crates/perry-runtime/src/exception/savepoints/tests.rs b/crates/perry-runtime/src/exception/savepoints/tests.rs index 5b463d711f..e4fd866d76 100644 --- a/crates/perry-runtime/src/exception/savepoints/tests.rs +++ b/crates/perry-runtime/src/exception/savepoints/tests.rs @@ -89,6 +89,18 @@ pub(super) fn call_method(_: u32) { crate::object::test_enter_catch_method(); } +/// A plain finite double round-trips through the nanbox bit pattern +/// unchanged (no tag rewriting to worry about), so a distinct marker per call +/// is enough to make `capture()` observe a change — mirrors how +/// `static_private_owner`'s witness below reuses `marker as f64`. +pub(super) fn implicit_this(marker: u32) { + crate::object::js_implicit_this_set(marker as f64); +} + +pub(super) fn new_target(marker: u32) { + crate::object::js_new_target_set(marker as f64); +} + pub(super) fn pump(_: u32) { crate::stdlib_pump::test_enter_catch_pump(); } diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index 0a4f3e5294..0eda3a2f4b 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots.rs @@ -8,6 +8,7 @@ mod fs_options_object; mod generator_attach_prototype; mod handle_stack; mod hook_dispatch_handles; +mod implicit_this_scope; mod interned_string_caches; mod iter_result_keys; mod json_construction; diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/implicit_this_scope.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/implicit_this_scope.rs new file mode 100644 index 0000000000..1275d81e81 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/implicit_this_scope.rs @@ -0,0 +1,148 @@ +//! Moving-GC regression for the runtime's implicit-`this` guards (#10490). +//! +//! A runtime entry point that binds `IMPLICIT_THIS` for a callback displaces +//! the CALLER's receiver and writes it back when the callback returns. The +//! callback is user code, so a copying minor inside it can move that receiver; +//! a displaced value kept in a plain Rust field is not a root and is restored +//! as a retired from-space address — the caller's next `this.x` reads +//! `undefined`. `js_native_call_method`'s prototype-override early path and +//! the dense/array-like `Array.prototype` callback engines each carried such a +//! guard. Every test here plants a callback that runs a copying minor, then +//! asserts the restored cell is the caller's RELOCATED receiver — and that the +//! receiver actually moved, so a green run cannot be vacuous. + +use super::super::super::*; +use super::super::support::*; + +extern "C" fn collect_arity0(_closure: *const crate::closure::ClosureHeader) -> f64 { + crate::gc::gc_collect_minor(); + f64::from_bits(crate::value::TAG_UNDEFINED) +} + +extern "C" fn collect_arity3( + _closure: *const crate::closure::ClosureHeader, + _value: f64, + _index: f64, + _recv: f64, +) -> f64 { + crate::gc::gc_collect_minor(); + f64::from_bits(crate::value::TAG_UNDEFINED) +} + +fn boxed(ptr: *const u8) -> f64 { + f64::from_bits(crate::JSValue::pointer(ptr).bits()) +} + +/// Install a young object as the caller's `this`, run `call`, and assert the +/// cell afterwards holds that object's post-collection address. +fn assert_caller_this_survives(what: &str, call: impl FnOnce()) { + let _guard = CopyingNurseryTestGuard::new(0); + let _scan = ConservativeScanDisabledGuard::new(); + let _triggers = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let _force = ForcedEvacuationTestGuard::on(); + register_runtime_handle_root_scanner_for_tests(); + + let scope = crate::gc::RuntimeHandleScope::new(); + let caller = scope.root_nanbox_f64(boxed(crate::object::js_object_alloc(0, 0) as *const u8)); + let caller_before = caller.get_nanbox_u64(); + let outer = crate::object::js_implicit_this_set(caller.get_nanbox_f64()); + + let cycles_before = copying_minor_cycles(); + call(); + assert!( + copying_minor_cycles() > cycles_before, + "{what}: the callback must run a copying minor" + ); + assert_ne!( + caller.get_nanbox_u64(), + caller_before, + "{what}: the collection must actually move the caller's receiver" + ); + let restored = crate::object::js_implicit_this_set(outer); + assert_eq!( + restored.to_bits(), + caller.get_nanbox_u64(), + "{what}: the displaced `this` must be restored at its relocated address, \ + not the pre-collection one ({caller_before:#x})" + ); +} + +#[test] +fn implicit_this_scope_restores_a_relocated_receiver() { + assert_caller_this_survives("ImplicitThisScope", || { + let scope = crate::gc::RuntimeHandleScope::new(); + let _bound = crate::object::ImplicitThisScope::bind( + &scope, + f64::from_bits(crate::value::TAG_UNDEFINED), + ); + crate::gc::gc_collect_minor(); + }); +} + +#[test] +fn prototype_override_method_call_restores_a_relocated_caller_this() { + assert_caller_this_survives("js_native_call_method prototype override", || { + crate::closure::js_register_closure_arity(collect_arity0 as *const u8, 0); + let scope = crate::gc::RuntimeHandleScope::new(); + let method = scope + .root_nanbox_f64(boxed( + crate::closure::js_closure_alloc(collect_arity0 as *const u8, 0) as *const u8, + )); + let proto = scope.root_nanbox_f64(boxed(crate::object::js_object_alloc(0, 1) as *const u8)); + let key = crate::string::js_string_from_bytes(b"run".as_ptr(), 3); + crate::object::js_object_set_field_by_name( + crate::value::js_nanbox_get_pointer(proto.get_nanbox_f64()) as *mut crate::ObjectHeader, + key, + method.get_nanbox_f64(), + ); + let receiver = + scope.root_nanbox_f64(boxed(crate::object::js_object_alloc(0, 0) as *const u8)); + crate::object::object_ops::js_object_set_prototype_of( + receiver.get_nanbox_f64(), + proto.get_nanbox_f64(), + ); + assert!( + crate::object::prototype_chain::object_has_individual_class_prototype( + crate::value::js_nanbox_get_pointer(receiver.get_nanbox_f64()) as usize + ), + "the receiver must take the #9247 prototype-override early path" + ); + unsafe { + crate::object::js_native_call_method( + receiver.get_nanbox_f64(), + b"run".as_ptr().cast(), + 3, + std::ptr::null(), + 0, + ); + } + }); +} + +#[test] +fn dense_array_for_each_restores_a_relocated_caller_this() { + assert_caller_this_survives("js_array_forEach", || { + let scope = crate::gc::RuntimeHandleScope::new(); + let arr = crate::array::js_array_push_f64(crate::array::js_array_alloc(0), 1.0); + let arr = scope.root_raw_const_ptr(arr); + let cb = crate::closure::js_closure_alloc_singleton(collect_arity3 as *const u8); + // `js_array_forEach` roots the receiver itself, so a scoped argument is + // the right shape here (#7341). + arr.with_const_ptr(|ptr| crate::array::js_array_forEach(ptr, cb)); + }); +} + +#[test] +fn arraylike_for_each_restores_a_relocated_caller_this() { + assert_caller_this_survives("js_arraylike_forEach", || { + let scope = crate::gc::RuntimeHandleScope::new(); + let arr = crate::array::js_array_push_f64(crate::array::js_array_alloc(0), 1.0); + let arr = scope.root_nanbox_f64(boxed(arr as *const u8)); + let cb = crate::closure::js_closure_alloc_singleton(collect_arity3 as *const u8); + crate::array::js_arraylike_forEach( + arr.get_nanbox_f64(), + boxed(cb as *const u8), + f64::from_bits(crate::value::TAG_UNDEFINED), + ); + }); +} diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/perex_public.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/perex_public.rs index 26232afff6..b368f6c518 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots/perex_public.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/perex_public.rs @@ -34,6 +34,11 @@ pub(super) fn register_host_roots() { crate::object::regex_proto_thunks::scan_canonical_test_site_roots_mut, ); gc_register_mutable_root_scanner(crate::object::scan_implicit_this_roots_mut); + // PR #10564 review finding: implicit_this/new_target savepoints in + // exception.rs are a second root for whatever these tests displace + // IMPLICIT_THIS to across a throw. gc_init registers this in + // production; the isolation guard clears that registry too. + gc_register_mutable_root_scanner(crate::exception::scan_exception_roots_mut); gc_register_mutable_root_scanner(crate::closure::scan_singleton_closure_roots_mut); gc_register_mutable_root_scanner(crate::closure::scan_closure_dynamic_props_roots_mut); gc_register_mutable_root_scanner(crate::string::scan_intern_table_roots_mut); diff --git a/crates/perry-runtime/src/intl/subclass.rs b/crates/perry-runtime/src/intl/subclass.rs index d2f397e50e..94fee140bf 100644 --- a/crates/perry-runtime/src/intl/subclass.rs +++ b/crates/perry-runtime/src/intl/subclass.rs @@ -152,9 +152,10 @@ pub(crate) unsafe fn intl_subclass_super( let this_scope = crate::gc::RuntimeHandleScope::new(); let this_h = this_scope.root_nanbox_f64(this_box); let prev_this = this_scope.root_nanbox_f64(crate::object::js_implicit_this_set(this_box)); - let prev_nt = crate::object::js_new_target_set(parent_val); + // #10490: the displaced `new.target` crosses the same call. + let prev_nt = this_scope.root_nanbox_f64(crate::object::js_new_target_set(parent_val)); let instance = crate::closure::js_native_call_value(parent_val, args_ptr, args_len); - crate::object::js_new_target_set(prev_nt); + crate::object::js_new_target_set(prev_nt.get_nanbox_f64()); crate::object::js_implicit_this_set(prev_this.get_nanbox_f64()); // Re-home the freshly-built instance's brand + bound methods onto `this`. let this_bits = this_h.get_nanbox_f64().to_bits(); diff --git a/crates/perry-runtime/src/object/global_this/fetch_globals.rs b/crates/perry-runtime/src/object/global_this/fetch_globals.rs index 1bdc321a4d..2230073e09 100644 --- a/crates/perry-runtime/src/object/global_this/fetch_globals.rs +++ b/crates/perry-runtime/src/object/global_this/fetch_globals.rs @@ -461,9 +461,10 @@ pub(crate) unsafe fn temporal_subclass_super( let this_scope = crate::gc::RuntimeHandleScope::new(); let this_h = this_scope.root_nanbox_f64(this_box); let prev_this = this_scope.root_nanbox_f64(crate::object::js_implicit_this_set(this_box)); - let prev_nt = crate::object::js_new_target_set(parent_val); + // #10490: the displaced `new.target` crosses the same call. + let prev_nt = this_scope.root_nanbox_f64(crate::object::js_new_target_set(parent_val)); let cell = crate::closure::js_native_call_value(parent_val, args_ptr, args_len); - crate::object::js_new_target_set(prev_nt); + crate::object::js_new_target_set(prev_nt.get_nanbox_f64()); crate::object::js_implicit_this_set(prev_this.get_nanbox_f64()); if crate::temporal::is_temporal_value(cell) { attach_temporal_cell_to_this(this_h.get_nanbox_f64(), cell); diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index 7e74e525ce..d23eab11e6 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -302,15 +302,16 @@ pub(crate) use field_get_set::{ pub(crate) use this_binding::js_derived_super_scope_push; pub(crate) use this_binding::{ derived_super_binding_stack_restore, derived_super_binding_stack_savepoint, - scan_implicit_this_roots_mut, static_private_owner_current, static_private_owner_pop, - static_private_owner_push, static_private_owner_stack_restore, + implicit_this_trap_restore, implicit_this_trap_savepoint, new_target_trap_restore, + new_target_trap_savepoint, scan_implicit_this_roots_mut, static_private_owner_current, + static_private_owner_pop, static_private_owner_push, static_private_owner_stack_restore, static_private_owner_stack_savepoint, static_this_arm, static_this_arm_if_unarmed, static_this_disarm, IMPLICIT_THIS, }; pub use this_binding::{ js_implicit_this_get, js_implicit_this_get_sloppy, js_implicit_this_set, js_new_target_get, js_new_target_set, js_static_this_arm_classref, js_static_this_arm_value, - js_static_this_resolve, + js_static_this_resolve, ImplicitThisScope, }; pub use to_string_tag::js_object_to_string; pub(crate) use to_string_tag::typed_array_to_string_tag_name; diff --git a/crates/perry-runtime/src/object/native_call_method.rs b/crates/perry-runtime/src/object/native_call_method.rs index 41cc0945c7..2a9c7bbfa1 100644 --- a/crates/perry-runtime/src/object/native_call_method.rs +++ b/crates/perry-runtime/src/object/native_call_method.rs @@ -1189,29 +1189,6 @@ pub unsafe extern "C-unwind" fn js_native_call_method_nullsafe( js_native_call_method(object, method_name_ptr, method_name_len, args_ptr, args_len) } -/// Bind `IMPLICIT_THIS` for the duration of one call and restore the previous -/// value on the way out — including when the callee unwinds, which this -/// `extern "C-unwind"` dispatch surface makes an ordinary outcome rather than an -/// exotic one. A plain set/restore pair would leak the receiver into every later -/// implicit-`this` read once a method throws. #9244. -struct ImplicitThisScope { - previous: f64, -} - -impl ImplicitThisScope { - fn bind(receiver: f64) -> Self { - Self { - previous: crate::object::js_implicit_this_set(receiver), - } - } -} - -impl Drop for ImplicitThisScope { - fn drop(&mut self) { - crate::object::js_implicit_this_set(self.previous); - } -} - #[no_mangle] // Dynamic native calls may synchronously throw from the selected module // implementation. Keep this bridge unwind-capable so a generated caller's JS @@ -1369,10 +1346,9 @@ pub unsafe extern "C-unwind" fn js_native_call_method( ) as usize); if resolved { let method_handle = root_scope.root_nanbox_f64(f64::from_bits(method.bits())); - let receiver = object(); let bound = crate::closure::clone_closure_rebind_this( method_handle.get_nanbox_f64().to_bits(), - receiver, + object(), ); let args = refreshed_args(); // `clone_closure_rebind_this` only rewrites a closure that @@ -1385,8 +1361,11 @@ pub unsafe extern "C-unwind" fn js_native_call_method( // saw no `this` ("called on null or undefined") and // `Object(true).valueOf()` saw the wrong one ("called on // incompatible receiver"). #9244. Restored on the way out, - // including when the callee throws. - let _this_scope = ImplicitThisScope::bind(receiver); + // including when the callee throws — from a ROOT: the + // displaced `this` is the caller's receiver and the callee + // is user code that can move it (#10490). The receiver is + // re-read here, after the clone above allocated. + let _this_scope = crate::object::ImplicitThisScope::bind(&root_scope, object()); return crate::closure::js_native_call_value( f64::from_bits(bound), args.as_ptr(), diff --git a/crates/perry-runtime/src/object/native_call_method/handle_methods.rs b/crates/perry-runtime/src/object/native_call_method/handle_methods.rs index d4f306c499..f0684d317c 100644 --- a/crates/perry-runtime/src/object/native_call_method/handle_methods.rs +++ b/crates/perry-runtime/src/object/native_call_method/handle_methods.rs @@ -1156,16 +1156,19 @@ pub(super) unsafe fn dispatch_handle( let prev_this_h = prev_this_scope.root_nanbox_u64( IMPLICIT_THIS.with(|c| c.replace(receiver_f64.to_bits())), ); + // #10490: the displaced accessor receiver rides + // through the same getter call — root it too. let prev_override = super::super::field_get_set::accessor_receiver_override_begin( receiver_f64, - ); + ) + .map(|value| prev_this_scope.root_nanbox_f64(value)); let field_val = js_object_get_field_by_name( proto_obj as *const _, method_key as *const crate::StringHeader, ); super::super::field_get_set::accessor_receiver_override_end( - prev_override, + prev_override.map(|handle| handle.get_nanbox_f64()), ); IMPLICIT_THIS.with(|c| c.set(prev_this_h.get_nanbox_u64())); if !field_val.is_undefined() && !field_val.is_null() { diff --git a/crates/perry-runtime/src/object/native_call_method/primitive_methods.rs b/crates/perry-runtime/src/object/native_call_method/primitive_methods.rs index 5b5a174680..e628f08fc7 100644 --- a/crates/perry-runtime/src/object/native_call_method/primitive_methods.rs +++ b/crates/perry-runtime/src/object/native_call_method/primitive_methods.rs @@ -350,7 +350,10 @@ pub(super) unsafe fn dispatch_primitive( let receiver = object_handle.get_nanbox_f64(); let bound = crate::closure::clone_closure_rebind_this(method.get_nanbox_u64(), receiver); - let _this = ImplicitThisScope::bind(object_handle.get_nanbox_f64()); + let _this = crate::object::ImplicitThisScope::bind( + root_scope, + object_handle.get_nanbox_f64(), + ); let args = refreshed_args(); return Some(crate::closure::js_native_call_value( f64::from_bits(bound), @@ -389,7 +392,11 @@ pub(super) unsafe fn dispatch_primitive( let receiver = object_handle.get_nanbox_f64(); let bound = crate::closure::clone_closure_rebind_this(method.get_nanbox_u64(), receiver); - let _this = ImplicitThisScope::bind(receiver); + // Re-read: the clone above allocated (#10490). + let _this = crate::object::ImplicitThisScope::bind( + root_scope, + object_handle.get_nanbox_f64(), + ); let args = refreshed_args(); return Some(crate::closure::js_native_call_value( f64::from_bits(bound), diff --git a/crates/perry-runtime/src/object/this_binding.rs b/crates/perry-runtime/src/object/this_binding.rs index a53ad4fa50..2e48e72f92 100644 --- a/crates/perry-runtime/src/object/this_binding.rs +++ b/crates/perry-runtime/src/object/this_binding.rs @@ -233,6 +233,9 @@ pub extern "C" fn js_implicit_this_get_sloppy() -> f64 { /// js_implicit_this_set(prev.get_nanbox_f64()); /// ``` /// +/// or, when the restore must also run as the callee unwinds, the +/// [`ImplicitThisScope`] guard, which is that idiom with the restore in `Drop`. +/// /// This is longjmp-safe: `exception.rs` saves and restores the handle stack at /// trap boundaries, so a throw through the window truncates the scope exactly /// as a normal drop would. Inside a loop, open the scope PER ITERATION (or @@ -247,6 +250,45 @@ pub extern "C" fn js_implicit_this_set(value: f64) -> f64 { f64::from_bits(implicit_this_cell().replace(value.to_bits())) } +/// Bind `IMPLICIT_THIS` for the lifetime of the guard and restore the +/// displaced value on the way out — including when the callee unwinds, which +/// the runtime's `extern "C-unwind"` dispatch surfaces make an ordinary +/// outcome: a plain set/restore pair leaks the receiver into every later +/// implicit-`this` read once a callback throws (#9244). +/// +/// The displaced value is the CALLER's receiver, held across the user code the +/// guard brackets, so it lives in a slot of the borrowed `RuntimeHandleScope` +/// — marked, and rewritten when an evacuating minor moves it — and `Drop` +/// re-reads that slot. A guard that keeps it in a plain field is the #9445 +/// shape with the restore moved into `Drop`, where a sweep for +/// `let prev = js_implicit_this_set(..)` cannot see it: the private guards this +/// replaced reinstalled a retired from-space address as the caller's `this` +/// after `Object.setPrototypeOf(o, proto); o.run()` ran an allocating method +/// (#10490), and after every `Array.prototype` callback method. +/// +/// The borrow is what makes the order safe: the scope must be declared before +/// the guard, so the guard's `Drop` runs while its slot is still on the handle +/// stack. +pub struct ImplicitThisScope<'scope> { + previous: crate::gc::RuntimeHandle<'scope>, +} + +impl<'scope> ImplicitThisScope<'scope> { + #[inline] + pub fn bind(scope: &'scope crate::gc::RuntimeHandleScope, receiver: f64) -> Self { + Self { + previous: scope.root_nanbox_f64(js_implicit_this_set(receiver)), + } + } +} + +impl Drop for ImplicitThisScope<'_> { + #[inline] + fn drop(&mut self) { + js_implicit_this_set(self.previous.get_nanbox_f64()); + } +} + /// Read the current `new.target` value for ordinary function bodies. #[no_mangle] pub extern "C" fn js_new_target_get() -> f64 { @@ -259,6 +301,41 @@ pub extern "C" fn js_new_target_set(value: f64) -> f64 { NEW_TARGET.with(|c| f64::from_bits(c.replace(value.to_bits()))) } +/// `catch_savepoints!` capture/restore for `IMPLICIT_THIS` (PR #10564 review +/// finding). Several runtime guards displace `IMPLICIT_THIS` around a call +/// they don't control — a `super()` bridge, a prototype-walk accessor +/// dispatch, a stdlib listener/getter dispatcher — with a bare +/// save/call/restore statement sequence, not `ImplicitThisScope`. Neither a +/// `longjmp` nor a system unwind runs the restore statement that follows the +/// call, so a throw crossing one of those sites leaves the callee's receiver +/// installed for every later implicit-`this` read. This closes that gap the +/// same way `runtime_handles`/`call_method` already do: captured at every `try`, +/// replayed by `js_throw` before the exception transports, regardless of +/// transport. It is an unconditional `set`, so it composes safely with a +/// `ImplicitThisScope::drop` that also fires on the unwind path: whichever +/// runs last for a given frame reproduces the same locally-correct value. +#[inline] +pub(crate) fn implicit_this_trap_savepoint() -> u64 { + implicit_this_cell().get() +} + +pub(crate) fn implicit_this_trap_restore(bits: u64) { + implicit_this_cell().set(bits); +} + +/// `catch_savepoints!` capture/restore for `NEW_TARGET`. Same rationale as +/// [`implicit_this_trap_savepoint`]: the Temporal/Intl subclass `super()` +/// bridges (`fetch_globals.rs`, `intl/subclass.rs`) save/restore `new.target` +/// with a bare statement pair around the parent constructor call. +#[inline] +pub(crate) fn new_target_trap_savepoint() -> u64 { + NEW_TARGET.with(|c| c.get()) +} + +pub(crate) fn new_target_trap_restore(bits: u64) { + NEW_TARGET.with(|c| c.set(bits)); +} + /// GC mutable-root scanner for the implicit-`this` cell (issue #1813). /// /// `IMPLICIT_THIS` holds the NaN-boxed receiver for the duration of a diff --git a/crates/perry-runtime/src/regex/site_test.rs b/crates/perry-runtime/src/regex/site_test.rs index 2642553003..af6e75aea8 100644 --- a/crates/perry-runtime/src/regex/site_test.rs +++ b/crates/perry-runtime/src/regex/site_test.rs @@ -307,24 +307,6 @@ pub(crate) fn active_factory_stack_restore(depth: usize) { ACTIVE_FACTORY_SITES.with(|stack| stack.borrow_mut().truncate(depth)); } -struct ImplicitThisGuard<'scope> { - previous: crate::gc::RuntimeHandle<'scope>, -} - -impl<'scope> ImplicitThisGuard<'scope> { - fn bind(scope: &'scope crate::gc::RuntimeHandleScope, receiver: f64) -> Self { - Self { - previous: scope.root_nanbox_f64(crate::object::js_implicit_this_set(receiver)), - } - } -} - -impl Drop for ImplicitThisGuard<'_> { - fn drop(&mut self) { - crate::object::js_implicit_this_set(self.previous.get_nanbox_f64()); - } -} - fn call_value_at_site(site_key: usize, callee: f64, this_value: Option) -> f64 { let scope = crate::gc::RuntimeHandleScope::new(); let callee = scope.root_nanbox_f64(callee); @@ -338,7 +320,7 @@ fn call_value_at_site(site_key: usize, callee: f64, this_value: Option) -> let this_value = this_value.map(|value| scope.root_nanbox_f64(value)); let this_guard = this_value .as_ref() - .map(|value| ImplicitThisGuard::bind(&scope, value.get_nanbox_f64())); + .map(|value| crate::object::ImplicitThisScope::bind(&scope, value.get_nanbox_f64())); let active = ActiveFactoryGuard::push(site_key, expected_identity); let result = unsafe { crate::closure::js_native_call_value(callee.get_nanbox_f64(), std::ptr::null(), 0) @@ -492,7 +474,7 @@ fn site_test_dispatch_impl(receiver: f64, method: f64, argument: f64) -> f64 { } let method = scope.root_nanbox_f64(method); - let _this_guard = ImplicitThisGuard::bind(&scope, receiver.get_nanbox_f64()); + let _this_guard = crate::object::ImplicitThisScope::bind(&scope, receiver.get_nanbox_f64()); let args = [argument.get_nanbox_f64()]; unsafe { crate::closure::js_native_call_value(method.get_nanbox_f64(), args.as_ptr(), 1) } } diff --git a/crates/perry-stdlib/src/domain.rs b/crates/perry-stdlib/src/domain.rs index e3b5a746a0..3252effdcd 100644 --- a/crates/perry-stdlib/src/domain.rs +++ b/crates/perry-stdlib/src/domain.rs @@ -270,10 +270,14 @@ unsafe fn emit_domain_event(handle: Handle, event: &str, args: &[f64]) -> bool { return false; } let receiver = nanbox_handle(handle); + // #10490: the displaced `this` is the caller's receiver and every listener + // is user code that can move it; restore it from a root, taken once. + let this_scope = perry_runtime::gc::RuntimeHandleScope::new(); + let previous_this = this_scope.root_nanbox_f64(perry_runtime::object::js_implicit_this_get()); for listener in listeners { - let previous_this = perry_runtime::object::js_implicit_this_set(receiver); + perry_runtime::object::js_implicit_this_set(receiver); let _ = perry_runtime::closure::js_native_call_value(listener, args.as_ptr(), args.len()); - perry_runtime::object::js_implicit_this_set(previous_this); + perry_runtime::object::js_implicit_this_set(previous_this.get_nanbox_f64()); } true } diff --git a/crates/perry-stdlib/src/events.rs b/crates/perry-stdlib/src/events.rs index f385681958..97ca999b41 100644 --- a/crates/perry-stdlib/src/events.rs +++ b/crates/perry-stdlib/src/events.rs @@ -969,10 +969,13 @@ unsafe fn call_emitter_listener( arr_handle.get_raw_mut_ptr::() as i64, ); } - let previous_this = perry_runtime::object::js_implicit_this_set(receiver); + // #10490: root the displaced `this` across the listener (user code). + let this_scope = perry_runtime::gc::RuntimeHandleScope::new(); + let previous_this = + this_scope.root_nanbox_f64(perry_runtime::object::js_implicit_this_set(receiver)); let result = perry_runtime::closure::js_native_call_value(callback_value, args.as_ptr(), args.len()); - perry_runtime::object::js_implicit_this_set(previous_this); + perry_runtime::object::js_implicit_this_set(previous_this.get_nanbox_f64()); result } diff --git a/crates/perry-stdlib/src/events/warnings.rs b/crates/perry-stdlib/src/events/warnings.rs index 9c9603b4d4..350ee91f92 100644 --- a/crates/perry-stdlib/src/events/warnings.rs +++ b/crates/perry-stdlib/src/events/warnings.rs @@ -49,9 +49,13 @@ unsafe fn emit_warning(warning: f64) { let emit_warning = js_object_get_field_by_name_f64(process_obj, key_ptr); if closure_ptr_from_value(emit_warning).is_some() { let args = [warning]; - let previous_this = perry_runtime::object::js_implicit_this_set(process); + // #10490: root the displaced `this` across the (user-replaceable) + // `process.emitWarning`. + let this_scope = perry_runtime::gc::RuntimeHandleScope::new(); + let previous_this = + this_scope.root_nanbox_f64(perry_runtime::object::js_implicit_this_set(process)); perry_runtime::closure::js_native_call_value(emit_warning, args.as_ptr(), args.len()); - perry_runtime::object::js_implicit_this_set(previous_this); + perry_runtime::object::js_implicit_this_set(previous_this.get_nanbox_f64()); return; } } diff --git a/crates/perry-stdlib/src/net/mod.rs b/crates/perry-stdlib/src/net/mod.rs index b062adf049..b65072cd20 100644 --- a/crates/perry-stdlib/src/net/mod.rs +++ b/crates/perry-stdlib/src/net/mod.rs @@ -1777,13 +1777,16 @@ pub unsafe extern "C" fn js_net_socket_upgrade_tls( /// steady-state allocation). unsafe fn emit_socket_no_arg(handle: i64, event: &str) { let receiver = f64::from_bits(0x7FFD_0000_0000_0000 | (handle as u64 & 0x0000_FFFF_FFFF_FFFF)); - let previous_this = perry_runtime::object::js_implicit_this_set(receiver); + // #10490: root the displaced `this` across the listeners (user code). + let this_scope = perry_runtime::gc::RuntimeHandleScope::new(); + let previous_this = + this_scope.root_nanbox_f64(perry_runtime::object::js_implicit_this_set(receiver)); for callback in listeners_for(handle, event) { if callback != 0 { js_closure_call0(callback as *const ClosureHeader); } } - perry_runtime::object::js_implicit_this_set(previous_this); + perry_runtime::object::js_implicit_this_set(previous_this.get_nanbox_f64()); } #[cfg(feature = "tls")] diff --git a/crates/perry-stdlib/src/streams.rs b/crates/perry-stdlib/src/streams.rs index 02ab520ae1..98621b8d81 100644 --- a/crates/perry-stdlib/src/streams.rs +++ b/crates/perry-stdlib/src/streams.rs @@ -91,6 +91,14 @@ extern "C" { ) -> f64; #[link_name = "js_implicit_this_set"] fn provider_js_implicit_this_set(value: f64) -> f64; + #[link_name = "js_ffi_root_scope_enter"] + fn provider_js_ffi_root_scope_enter() -> usize; + #[link_name = "js_ffi_root_push_nanbox"] + fn provider_js_ffi_root_push_nanbox(bits: u64) -> usize; + #[link_name = "js_ffi_root_get_nanbox"] + fn provider_js_ffi_root_get_nanbox(index: usize) -> u64; + #[link_name = "js_ffi_root_scope_exit"] + fn provider_js_ffi_root_scope_exit(base: usize); #[link_name = "js_promise_new"] fn provider_js_promise_new() -> *mut Promise; #[link_name = "js_promise_all"] @@ -225,6 +233,23 @@ fn js_implicit_this_set(value: f64) -> f64 { provider_call!(provider_js_implicit_this_set(value)) } +/// Call `f` with `IMPLICIT_THIS` bound to `receiver`, restoring the displaced +/// value from a transient ROOT afterwards (#10490): it is the caller's +/// receiver, and `f` runs user code that an evacuating minor can move it +/// across. Uses the provider's root stack like every other runtime-owned +/// operation in this file. +fn with_implicit_this(receiver: f64, f: impl FnOnce() -> f64) -> f64 { + let base = provider_call!(provider_js_ffi_root_scope_enter()); + let previous = js_implicit_this_set(receiver); + let slot = provider_call!(provider_js_ffi_root_push_nanbox(previous.to_bits())); + let result = f(); + js_implicit_this_set(f64::from_bits(provider_call!( + provider_js_ffi_root_get_nanbox(slot) + ))); + provider_call!(provider_js_ffi_root_scope_exit(base)); + result +} + fn js_promise_new() -> *mut Promise { provider_call!(provider_js_promise_new()) } @@ -1482,9 +1507,7 @@ unsafe fn call_symbol_async_iterator(value: f64) -> Option { if !is_callable_value(method) { return None; } - let prev_this = js_implicit_this_set(value); - let iterator = js_native_call_value(method, std::ptr::null(), 0); - js_implicit_this_set(prev_this); + let iterator = with_implicit_this(value, || js_native_call_value(method, std::ptr::null(), 0)); if iterator.to_bits() == TAG_UNDEFINED { None } else { @@ -1538,9 +1561,8 @@ unsafe fn call_iterator_next(iterator: f64) -> Option { let next_val = js_object_get_field_by_name(iter_obj, next_key); let next = f64::from_bits(next_val.bits()); if is_callable_value(next) { - let prev_this = js_implicit_this_set(iterator); - let result = js_native_call_value(next, std::ptr::null(), 0); - js_implicit_this_set(prev_this); + let result = + with_implicit_this(iterator, || js_native_call_value(next, std::ptr::null(), 0)); Some(result) } else { Some(perry_runtime::object::js_native_call_method( diff --git a/crates/perry-stdlib/src/tls.rs b/crates/perry-stdlib/src/tls.rs index 4aebd0c960..cb07f061ec 100644 --- a/crates/perry-stdlib/src/tls.rs +++ b/crates/perry-stdlib/src/tls.rs @@ -1199,12 +1199,15 @@ pub unsafe extern "C" fn js_tls_client_preflight( nanbox_handle(alpn_callback), callback_socket, ); - let previous_this = perry_runtime::object::js_implicit_this_set(callback_socket); + // #10490: root the displaced `this` across the ALPNCallback (user code). + let this_scope = perry_runtime::gc::RuntimeHandleScope::new(); + let previous_this = this_scope + .root_nanbox_f64(perry_runtime::object::js_implicit_this_set(callback_socket)); let selected = js_closure_call1( rebound_callback as *const ClosureHeader, js_nanbox_pointer(argument as i64), ); - perry_runtime::object::js_implicit_this_set(previous_this); + perry_runtime::object::js_implicit_this_set(previous_this.get_nanbox_f64()); let selected = value_to_string(selected).map(String::into_bytes); sockets().lock().unwrap().remove(&socket_id); listeners().lock().unwrap().remove(&socket_id); diff --git a/crates/perry-stdlib/src/worker_threads.rs b/crates/perry-stdlib/src/worker_threads.rs index 0a2285c9eb..08b7e5cf8e 100644 --- a/crates/perry-stdlib/src/worker_threads.rs +++ b/crates/perry-stdlib/src/worker_threads.rs @@ -722,9 +722,13 @@ fn call_callback1(callback_bits: u64, this_bits: u64, arg: f64) { if closure.is_null() { return; } - let prev_this = perry_runtime::object::js_implicit_this_set(f64::from_bits(this_bits)); + // #10490: root the displaced `this` across the callback (user code). + let this_scope = perry_runtime::gc::RuntimeHandleScope::new(); + let prev_this = this_scope.root_nanbox_f64(perry_runtime::object::js_implicit_this_set( + f64::from_bits(this_bits), + )); perry_runtime::closure::js_closure_call1(closure, arg); - perry_runtime::object::js_implicit_this_set(prev_this); + perry_runtime::object::js_implicit_this_set(prev_this.get_nanbox_f64()); } fn object_event_handler(target_bits: u64, name: &str) -> Option { diff --git a/crates/perry-stdlib/src/worker_threads/worker_surface.rs b/crates/perry-stdlib/src/worker_threads/worker_surface.rs index c97e8af227..a050dda006 100644 --- a/crates/perry-stdlib/src/worker_threads/worker_surface.rs +++ b/crates/perry-stdlib/src/worker_threads/worker_surface.rs @@ -389,14 +389,18 @@ fn stream_emit_event(event: f64, arg: f64) -> f64 { }; let args = [arg]; let len = perry_runtime::array::js_array_length(arr); + // #10490: the displaced `this` crosses every listener (user code); restore + // it from a root, taken once. + let this_scope = perry_runtime::gc::RuntimeHandleScope::new(); + let prev_this = this_scope.root_nanbox_f64(perry_runtime::object::js_implicit_this_get()); for i in 0..len { let callback = perry_runtime::array::js_array_get_f64(arr, i); - let prev_this = perry_runtime::object::js_implicit_this_set(this); + perry_runtime::object::js_implicit_this_set(this); unsafe { let _ = perry_runtime::closure::js_native_call_value(callback, args.as_ptr(), args.len()); } - perry_runtime::object::js_implicit_this_set(prev_this); + perry_runtime::object::js_implicit_this_set(prev_this.get_nanbox_f64()); } js_bool(len > 0) } diff --git a/test-files/test_gap_10490_implicit_this_scope_rooting.ts b/test-files/test_gap_10490_implicit_this_scope_rooting.ts new file mode 100644 index 0000000000..711a85b138 --- /dev/null +++ b/test-files/test_gap_10490_implicit_this_scope_rooting.ts @@ -0,0 +1,337 @@ +// #10490: a dynamic method call on a receiver whose prototype was replaced +// (`Object.setPrototypeOf(o, proto); o.run()`) restored a STALE caller `this` +// after an evacuating minor ran inside the callee. +// +// `js_native_call_method`'s early path for such receivers bound the callee's +// `this` with a private `ImplicitThisScope` guard that kept the DISPLACED +// value — the caller's receiver — in a plain struct field and wrote it back in +// `Drop`. A copying minor inside the call relocates that object and rewrites +// every slot the collector can see; a Rust field is not one, so the restore +// reinstalled a retired from-space address and the caller's next `this.tag` +// read `undefined` (`Cannot read properties of undefined (reading 'length')`, +// cheerio's `this.options` in `_findBySelector`). The #9445 sweep rooted every +// `let prev = js_implicit_this_set(..)`, but not the same save hidden in a +// guard's `Drop` — nor the two identical guards behind the `Array.prototype` +// callback methods (`DenseThisGuard`, `ThisGuard`). +// +// Every case builds a fresh (young) receiver whose `run` reads `this` +// dynamically, calls something that allocates past the nursery, then reads +// `this` again. Deterministic without GC env knobs: pre-fix the affected cases +// print a non-zero `bad=` count; node prints `bad=0` for every line. + +const N = 500; + +function churn(): number { + const tmp: any[] = []; + for (let k = 0; k < 2000; k++) tmp.push({ k: k, s: "t" + k, pad: [k, k + 1] }); + return tmp.length; +} + +function check(name: string, make: (i: number) => any, invoke?: (o: any) => any): void { + let bad = 0; + const notes: string[] = []; + for (let i = 0; i < N; i++) { + const o: any = make(i); + const want = "tag" + i + ":2000"; + let got: any; + try { + got = invoke ? invoke(o) : o.run(); + } catch (e: any) { + got = "THREW:" + (e && e.message); + } + if (got !== want) { + bad++; + if (bad <= 2) notes.push(" [" + i + " got=" + String(got) + "]"); + } + } + console.log(name + " bad=" + bad + notes.join("")); +} + +// Plain-function methods: `this` is read off the implicit-`this` cell. +function run(this: any): string { + const n = this.churn(); // dynamic method call; a moving minor runs inside it + return this.tag + ":" + n; // `this` must still be the (moved) receiver +} +function runViaCall(this: any): string { + const n = this.churn.call(this); + return this.tag + ":" + n; +} +function runViaApply(this: any): string { + const n = this.churn.apply(this, []); + return this.tag + ":" + n; +} +const proto: any = { + churn: function (this: any): number { + return churn(); + }, + run: run, + runViaCall: runViaCall, + runViaApply: runViaApply, +}; + +// --- receivers whose prototype was replaced ------------------------------- + +check( + "setPrototypeOf_object", + (i) => { + const o = { tag: "tag" + i }; + Object.setPrototypeOf(o, proto); + return o; + }, +); + +check( + "object_create", + (i) => { + const o = Object.create(proto); + o.tag = "tag" + i; + return o; + }, +); + +check( + "proto_literal", + (i) => ({ __proto__: proto, tag: "tag" + i }), +); + +class Box { + tag: string; + constructor(i: number) { + this.tag = "tag" + i; + } + run(): string { + return "box"; + } +} + +check( + "class_instance_swapped_to_object", + (i) => { + const b = new Box(i); + Object.setPrototypeOf(b, proto); + return b; + }, +); + +class Other { + tag = ""; + churn(): number { + return churn(); + } + run(): string { + const n = this.churn(); + return this.tag + ":" + n; + } +} + +check( + "class_instance_swapped_to_class", + (i) => { + const b: any = new Box(i); + Object.setPrototypeOf(b, Other.prototype); + return b; + }, +); + +// --- the outer or inner call through Function.prototype.call / apply ------- + +check( + "outer_call", + (i) => { + const o = { tag: "tag" + i }; + Object.setPrototypeOf(o, proto); + return o; + }, + (o) => o.run.call(o), +); + +check( + "outer_apply", + (i) => { + const o = { tag: "tag" + i }; + Object.setPrototypeOf(o, proto); + return o; + }, + (o) => o.run.apply(o, []), +); + +check( + "inner_call", + (i) => { + const o = { tag: "tag" + i }; + Object.setPrototypeOf(o, proto); + return o; + }, + (o) => o.runViaCall(), +); + +check( + "inner_apply", + (i) => { + const o = { tag: "tag" + i }; + Object.setPrototypeOf(o, proto); + return o; + }, + (o) => o.runViaApply(), +); + +// --- a class evaluated per factory call, methods mixed into its base -------- +// (cheerio's `load()` declares `class LoadedCheerio extends Cheerio` per call +// and installs its API with `Object.assign(Cheerio.prototype, ...)`.) + +class Base { + tag: string; + constructor(i: number) { + this.tag = "tag" + i; + } +} +Object.assign(Base.prototype, { churn: proto.churn, run: run }); + +function makeLoaded(): any { + class Loaded extends Base {} + return Loaded; +} + +check( + "per_evaluation_subclass_mixin", + (i) => { + const Loaded = makeLoaded(); + return new Loaded(i); + }, +); + +// --- Array.prototype callback methods, called from a `this`-reading method -- +// The same unrooted save sat in the dense (`DenseThisGuard`) and array-like +// (`ThisGuard`) callback engines. The host method is deliberately NOT named +// like any class method above: a class-declared `run` routes `o.run()` through +// a dispatch that rebinds `this` into the callee's captures, which hides the +// stale cell from the body. + +function host(i: number, body: (this: any) => string): any { + return { tag: "tag" + i, hostRun: body }; +} + +function callHost(o: any): any { + return o.hostRun(); +} + +check( + "array_forEach", + (i) => + host(i, function (this: any) { + let n = 0; + [1].forEach(function () { + n = churn(); + }); + return this.tag + ":" + n; + }), + callHost, +); + +check( + "array_map", + (i) => + host(i, function (this: any) { + const out = [1].map(function () { + return churn(); + }); + return this.tag + ":" + out[0]; + }), + callHost, +); + +check( + "array_filter_some_every_find", + (i) => + host(i, function (this: any) { + let n = 0; + [1].filter(function () { + n = churn(); + return true; + }); + [1].some(function () { + n = churn(); + return false; + }); + [1].every(function () { + n = churn(); + return true; + }); + [1].find(function () { + n = churn(); + return false; + }); + return this.tag + ":" + n; + }), + callHost, +); + +check( + "array_forEach_thisArg", + (i) => + host(i, function (this: any) { + const arr: number[] = [1, 2].slice(1); + let n = 0; + const target = { k: 1 }; + arr.forEach(function (this: any) { + n = churn() + (this === target ? 0 : 1000); + }, target); + return this.tag + ":" + n; + }), + callHost, +); + +check( + "arraylike_forEach_thisArg", + (i) => + host(i, function (this: any) { + let n = 0; + const target = { k: 1 }; + Array.prototype.forEach.call( + { length: 1, 0: 1 }, + function (this: any) { + n = churn() + (this === target ? 0 : 1000); + }, + target, + ); + return this.tag + ":" + n; + }), + callHost, +); + +check( + "arraylike_map_thisArg", + (i) => + host(i, function (this: any) { + const target = { k: 1 }; + const out: any = Array.prototype.map.call( + { length: 1, 0: 1 }, + function (this: any) { + return churn() + (this === target ? 0 : 1000); + }, + target, + ); + return this.tag + ":" + out[0]; + }), + callHost, +); + +// --- the swapped receiver is itself the caller of a second swapped call ---- + +const outerProto: any = { + run: function (this: any): string { + const inner: any = { tag: "inner" }; + Object.setPrototypeOf(inner, proto); + const r = inner.run(); // nested early-path call on another swapped receiver + return this.tag + ":" + r.slice(r.indexOf(":") + 1); + }, +}; + +check( + "nested_swapped_receivers", + (i) => { + const o = { tag: "tag" + i }; + Object.setPrototypeOf(o, outerProto); + return o; + }, +);