fix(runtime): root receivers and stored values across js_string_coerce property-key coercions (#6943)

changelog.d/6948-string-coerce-property-key-rooting.md

@@ -0,0 +1,60 @@
+**GC rooting: the `js_string_coerce`-as-property-key family (#6943).** Third and last known family in
+the unrooted-operand-across-GC-capable-coercion series (after #6934's dynamic arith and #6941's
+`ToPropertyKey`). A set of property entry points stringify the key with `js_string_coerce` directly,
+without an earlier `ToPropertyKey` — and held the receiver, and on the write paths the value about to
+be stored, as raw Rust locals across it. `js_string_coerce` allocates for every key shape except an
+already-heap `STRING_TAG` one (an SSO short key materializes onto the heap, a numeric key builds its
+stringification, an object key runs a user `toString`/`valueOf`), and an allocation can trigger a GC
+that **evacuates** live objects. A Rust local is neither a GC root nor a shadow slot, so a stale
+receiver dropped the write onto a forwarding stub and a stale stored value planted a dangling pointer
+inside a live object, where it outlived the call.
+
+Fixed with the established idiom — `crate::gc::RuntimeHandleScope` plus
+`root_heap_word_u64`/`root_raw_mut_ptr`/`root_nanbox_f64`/`root_string_ptr`, re-reading each operand
+through its handle after the coercion — across:
+
+- `object/object_ops/define_property.rs` — `Object.defineProperty`'s closure, typed-array and
+  ordinary arms (the receiver, the descriptor, and the already-dereferenced `closure_ptr` /
+  TypedArray address / `ObjectHeader`).
+- `object/descriptors.rs` — `getOwnPropertyDescriptor`'s class-object / typed-array / closure /
+  ordinary arms, `string_primitive_descriptor` (whose receiver is itself a movable heap string), and
+  `getOwnPropertyDescriptors` (result receiver + each stored descriptor).
+- `object/descriptor_state.rs` — `reflect_getter_closure_bits`, including the prototype-walk cursor.
+- `object/reflect_support.rs` — `obj_value_has_own_key`'s three arms plus `obj_value_attrs`, where a
+  stale receiver **address** silently misses the descriptor side table instead of crashing, so a
+  `Reflect.defineProperty` on a non-configurable property could slip through.
+- `object/array_object_ops.rs` — `array_length_reflect_define`.
+- `object/typed_array_define.rs` — `typed_array_own_index` and `typed_array_define_own_property`. The
+  issue named the shared `canonical_index_for_key` helper; reading it showed the helper is clean and
+  the hazard is at these two callers, which resolve the view address before the coercion and
+  dereference it after.
+- `proxy.rs` — the store fast path in `ordinary_set_with_receiver` (`obj.f = v`). The issue named the
+  class-instance arm; the plain-object arm reaches the same coercion transitively through
+  `object_proto_may_intercept_key` → `obj_value_has_own_key`, so one optional scope now covers both.
+  An inert-key check keeps the common already-heap-string key on the pre-fix code path verbatim.
+- `object/object_ops/has_own.rs`, `object/native_call_method/common_methods.rs` —
+  `hasOwnProperty` / `propertyIsEnumerable` in both their entry-point and method-call forms.
+- `object/object_ops/from_entries.rs` — `Object.fromEntries` (fresh result receiver **and** the entry
+  value written into it).
+- `symbol/properties.rs` — `js_class_register_static_symbol`'s non-symbol arm (the stored payload).
+- `object/with_env.rs`, `error.rs` — the `globalThis`-by-name helpers, whose window spans a read, a
+  `ToNumeric`/step, and a write-back.
+
+New shared predicate `builtins::string_coerce_is_inert(value)`, the `js_string_coerce` analogue of
+the `property_key_coercion_is_inert` predicate from #6941, justified by `js_string_coerce`'s own
+`is_string()` early return. Hot surfaces are gated on it so an already-heap-string key pays nothing.
+
+Also rooted, from review of the first pass: the property KEY itself at the `ordinary_set_with_receiver`
+store lane (an object key is a heap value and is exactly the shape whose user `toString` can evacuate
+it); `own_set_descriptor`'s receiver, whose raw address keys the descriptor side tables; the `obj_jv`
+tag view in `js_object_property_is_enumerable`; the enumerated receiver across both loops of
+`getOwnPropertyDescriptors`; and `key_value` where `js_object_define_property`'s fallback re-coerces
+it through `obj_value_has_own_key`.
+
+`proxy.rs`'s `target_set` was audited and is provably inert (its argument is always a
+`js_to_property_key` result, i.e. an already-heap string); a comment now records that so the next
+sweep doesn't re-examine it. `crates/perry/tests/gc_string_coerce_property_key_rooting_6943.rs` adds
+three forced-evacuation behavioral guards. As with both predecessors, no deterministic pre-fix
+failure is reachable from compiled code today — `gc()` runs a full mark-sweep and pins raw locals via
+the conservative stack scan (#6946), and `perry/gc`'s `minor()` engages the same scan (#6942) — so
+the suite is a guard, not a red-to-green regression test, and its module doc says so.

crates/perry-runtime/src/builtins/mod.rs

@@ -101,6 +101,11 @@ pub use globals::{
 
 pub(crate) use globals::{drain_queued_microtasks_count, queued_microtasks_pending};
 
+/// #6943: inertness predicate for the `js_string_coerce`-as-property-key
+/// rooting family. Crate-internal — callers use it to skip a
+/// `RuntimeHandleScope` when the coercion provably cannot GC.
+pub(crate) use numbers::string_coerce_is_inert;
+
 pub use numbers::{
     js_is_finite, js_is_nan, js_number_coerce, js_number_is_finite, js_number_is_integer,
     js_number_is_nan, js_number_is_safe_integer, js_parse_float, js_parse_int, js_string_coerce,

crates/perry-runtime/src/builtins/numbers.rs

@@ -673,6 +673,33 @@ pub extern "C" fn js_string_coerce(value: f64) -> *mut StringHeader {
     js_string_from_bytes(result.as_ptr(), result.len() as u32)
 }
 
+/// True when [`js_string_coerce`] provably neither allocates nor calls back
+/// into user JS for `value`, so a caller may hold a raw receiver / stored value
+/// across it without a [`RuntimeHandleScope`] (#6943).
+///
+/// Only an already-heap `STRING_TAG` value qualifies — that is the one arm of
+/// [`js_string_coerce`] that returns before touching the allocator (it hands
+/// the very same `StringHeader` pointer back). Every other shape allocates:
+/// `undefined` / `null` / booleans / numbers / BigInt build their
+/// stringification, an SSO short string (`SHORT_STRING_TAG`, a *different* tag)
+/// materializes onto the heap, and a `POINTER_TAG` object routes through
+/// `js_jsvalue_to_string`, which can invoke a user `toString` / `valueOf`. Any
+/// of those can trigger a GC that **evacuates** live objects — moving the
+/// caller's receiver and the value it is about to store — so callers must root
+/// across the coercion instead.
+///
+/// This is the `js_string_coerce` analogue of #6935's
+/// `object::property_key_coercion_is_inert`, which makes the same claim about
+/// `js_to_property_key`. The two predicates coincide today, but they are
+/// assertions about two different functions: this one is justified by
+/// [`js_string_coerce`]'s own `is_string()` early return, directly above.
+///
+/// [`RuntimeHandleScope`]: crate::gc::RuntimeHandleScope
+#[inline]
+pub(crate) fn string_coerce_is_inert(value: f64) -> bool {
+    crate::value::JSValue::from_bits(value.to_bits()).is_string()
+}
+
 /// `RequireObjectCoercible(this)` + `ToString(this)` for the inline-lowered
 /// `String.prototype` methods (`charAt` / `charCodeAt` / `codePointAt` /
 /// `split` / `toUpperCase` / …) when the receiver is NOT statically

crates/perry-runtime/src/error.rs

@@ -978,8 +978,16 @@ pub extern "C" fn js_global_get_or_throw_unresolved(name_value: f64) -> f64 {
     let g = crate::object::js_get_global_this();
     let gj = crate::value::JSValue::from_bits(g.to_bits());
     if gj.is_pointer() {
-        let gptr = (gj.bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
+        // #6943: `js_string_coerce` allocates for every non-heap-string name,
+        // so it can trigger a GC that **evacuates**. The global object's header
+        // was extracted into a raw Rust local *before* the coercion and
+        // dereferenced by `js_object_get_field_by_name` after it. Root the
+        // receiver and re-derive the header from the refreshed value.
+        let scope = crate::gc::RuntimeHandleScope::new();
+        let g_handle = scope.root_heap_word_u64(g.to_bits());
         let key = crate::builtins::js_string_coerce(name_value);
+        let g = f64::from_bits(g_handle.get_heap_word_u64());
+        let gptr = (g.to_bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
         if !gptr.is_null() && !key.is_null() {
             let v = unsafe { crate::object::js_object_get_field_by_name(gptr, key) };
             if !v.is_undefined() {
@@ -992,7 +1000,10 @@ pub extern "C" fn js_global_get_or_throw_unresolved(name_value: f64) -> f64 {
             // can't tell "absent" from "present, value undefined", so confirm
             // the property actually exists (as an OWN property — a global var
             // binding always is) before falling through to the throw.
-            let has = crate::object::js_object_has_own(g, name_value);
+            let has = crate::object::js_object_has_own(
+                f64::from_bits(g_handle.get_heap_word_u64()),
+                name_value,
+            );
             if crate::value::js_is_truthy(has) != 0 {
                 return f64::from_bits(crate::value::JSValue::undefined().bits());
             }
@@ -1027,15 +1038,35 @@ pub extern "C" fn js_global_update(name_value: f64, is_increment: f64, is_prefix
     let is_prefix = crate::value::js_is_truthy(is_prefix) != 0;
     let g = crate::object::js_get_global_this();
     let gj = crate::value::JSValue::from_bits(g.to_bits());
+    // #6943: `js_string_coerce` allocates for every non-heap-string name, and
+    // the read-modify-write below adds `js_object_get_field_by_name`,
+    // `js_object_has_own`, `js_to_numeric` and `js_numeric_step` — every one of
+    // them GC-capable. The global object (`g`, and the `gptr` header derived
+    // from the pre-coercion `gj`) and the coerced key string were raw Rust
+    // locals across all of it, and `gptr` is the receiver of the WRITE-BACK at
+    // the end. Root both and re-derive the header at each use.
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let g_handle = scope.root_heap_word_u64(g.to_bits());
     let key = crate::builtins::js_string_coerce(name_value);
+    let key_handle = scope.root_string_ptr(key);
     let mut present = false;
     let old = if gj.is_pointer() && !key.is_null() {
-        let gptr = (gj.bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
+        let g = f64::from_bits(g_handle.get_heap_word_u64());
+        let gptr = (g.to_bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
         if !gptr.is_null() {
-            let v = unsafe { crate::object::js_object_get_field_by_name(gptr, key) };
+            let v = unsafe {
+                crate::object::js_object_get_field_by_name(
+                    gptr,
+                    key_handle.get_raw_const_ptr::<crate::string::StringHeader>(),
+                )
+            };
             if !v.is_undefined()
                 || unsafe {
-                    crate::object::js_object_has_own(g, name_value).to_bits()
+                    crate::object::js_object_has_own(
+                        f64::from_bits(g_handle.get_heap_word_u64()),
+                        name_value,
+                    )
+                    .to_bits()
                         == crate::value::TAG_TRUE
                 }
             {
@@ -1055,10 +1086,23 @@ pub extern "C" fn js_global_update(name_value: f64, is_increment: f64, is_prefix
         let err_ptr = js_referenceerror_new(msg_str);
         return crate::exception::js_throw(crate::value::js_nanbox_pointer(err_ptr as i64));
     }
-    let numeric = unsafe { crate::value::js_to_numeric(old) };
-    let stepped = unsafe { crate::value::js_numeric_step(numeric, is_increment) };
-    let gptr = (gj.bits() & crate::value::POINTER_MASK) as *mut crate::object::ObjectHeader;
-    unsafe { crate::object::js_object_set_field_by_name(gptr, key, stepped) };
+    let old_handle = scope.root_nanbox_f64(old);
+    let numeric = unsafe { crate::value::js_to_numeric(old_handle.get_nanbox_f64()) };
+    let numeric_handle = scope.root_nanbox_f64(numeric);
+    let stepped =
+        unsafe { crate::value::js_numeric_step(numeric_handle.get_nanbox_f64(), is_increment) };
+    let stepped_handle = scope.root_nanbox_f64(stepped);
+    let g = f64::from_bits(g_handle.get_heap_word_u64());
+    let gptr = (g.to_bits() & crate::value::POINTER_MASK) as *mut crate::object::ObjectHeader;
+    unsafe {
+        crate::object::js_object_set_field_by_name(
+            gptr,
+            key_handle.get_raw_const_ptr::<crate::string::StringHeader>(),
+            stepped_handle.get_nanbox_f64(),
+        )
+    };
+    let numeric = numeric_handle.get_nanbox_f64();
+    let stepped = stepped_handle.get_nanbox_f64();
     if is_prefix {
         stepped
     } else {
@@ -1090,15 +1134,35 @@ static KEEP_JS_GLOBAL_ASSIGN_EXISTING_OR_THROW: extern "C" fn(f64, f64) -> f64 =
 pub extern "C" fn js_global_assign_existing_or_throw(name_value: f64, value: f64) -> f64 {
     let g = crate::object::js_get_global_this();
     let gj = crate::value::JSValue::from_bits(g.to_bits());
+    // #6943: the textbook shape of this family — a receiver AND the value being
+    // stored into it, both raw across a GC-capable `js_string_coerce`. The
+    // presence probe (`js_object_get_field_by_name`, `js_object_has_own`) and
+    // the not-defined path (`js_string_from_bytes`, `js_referenceerror_new`)
+    // allocate on top of that, and `gptr` is the receiver of the final write.
+    // Root the global, the coerced key and `value` for the whole helper.
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let g_handle = scope.root_heap_word_u64(g.to_bits());
+    let value_handle = scope.root_nanbox_f64(value);
     let key = crate::builtins::js_string_coerce(name_value);
+    let key_handle = scope.root_string_ptr(key);
     let mut present = false;
     if gj.is_pointer() && !key.is_null() {
-        let gptr = (gj.bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
+        let g = f64::from_bits(g_handle.get_heap_word_u64());
+        let gptr = (g.to_bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
         if !gptr.is_null() {
-            let v = unsafe { crate::object::js_object_get_field_by_name(gptr, key) };
+            let v = unsafe {
+                crate::object::js_object_get_field_by_name(
+                    gptr,
+                    key_handle.get_raw_const_ptr::<crate::string::StringHeader>(),
+                )
+            };
             if !v.is_undefined()
                 || unsafe {
-                    crate::object::js_object_has_own(g, name_value).to_bits()
+                    crate::object::js_object_has_own(
+                        f64::from_bits(g_handle.get_heap_word_u64()),
+                        name_value,
+                    )
+                    .to_bits()
                         == crate::value::TAG_TRUE
                 }
             {
@@ -1113,10 +1177,15 @@ pub extern "C" fn js_global_assign_existing_or_throw(name_value: f64, value: f64
         let err_ptr = js_referenceerror_new(msg_str);
         return crate::exception::js_throw(crate::value::js_nanbox_pointer(err_ptr as i64));
     }
-    let gptr = (gj.bits() & crate::value::POINTER_MASK) as *mut crate::object::ObjectHeader;
-    crate::object::js_object_set_field_by_name(gptr, key, value);
+    let g = f64::from_bits(g_handle.get_heap_word_u64());
+    let gptr = (g.to_bits() & crate::value::POINTER_MASK) as *mut crate::object::ObjectHeader;
+    crate::object::js_object_set_field_by_name(
+        gptr,
+        key_handle.get_raw_const_ptr::<crate::string::StringHeader>(),
+        value_handle.get_nanbox_f64(),
+    );
     // An assignment expression evaluates to its RHS.
-    value
+    value_handle.get_nanbox_f64()
 }
 
 /// Non-throwing variant of [`js_global_get_or_throw_unresolved`] for
@@ -1129,8 +1198,13 @@ pub extern "C" fn js_global_get_optional(name_value: f64) -> f64 {
     let g = crate::object::js_get_global_this();
     let gj = crate::value::JSValue::from_bits(g.to_bits());
     if gj.is_pointer() {
-        let gptr = (gj.bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
+        // #6943: root the global across the GC-capable coercion and re-derive
+        // its header afterwards — see `js_global_get_or_throw_unresolved`.
+        let scope = crate::gc::RuntimeHandleScope::new();
+        let g_handle = scope.root_heap_word_u64(g.to_bits());
         let key = crate::builtins::js_string_coerce(name_value);
+        let g = f64::from_bits(g_handle.get_heap_word_u64());
+        let gptr = (g.to_bits() & crate::value::POINTER_MASK) as *const crate::object::ObjectHeader;
         if !gptr.is_null() && !key.is_null() {
             let v = unsafe { crate::object::js_object_get_field_by_name(gptr, key) };
             return f64::from_bits(v.bits());

crates/perry-runtime/src/gc/mod.rs

@@ -135,7 +135,8 @@ pub(super) fn gc_collect_minor_with_trigger(trigger: GcTriggerSnapshot) -> GcCol
     // live bytes exceed K× the last full's live set (belt-and-suspenders for
     // callers that reach a minor outside the budgeted pressure path).
     if arena_growth_full_escalation_due() {
-        let outcome = gc_collect_full_mark_sweep_with_trigger(GcTriggerSnapshot::capture(trigger.kind));
+        let outcome =
+            gc_collect_full_mark_sweep_with_trigger(GcTriggerSnapshot::capture(trigger.kind));
         restore_minor_in_alloc(prev_in_alloc);
         return outcome;
     }

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

@@ -263,7 +263,17 @@ pub(crate) unsafe fn array_length_reflect_define(
     if obj.is_null() || !is_array_object(obj) {
         return None;
     }
+    // #6943: `js_string_coerce` allocates for every non-heap-string key and can
+    // run a user `toString` / `valueOf` for an object key, so it can trigger a
+    // GC that **evacuates**. `obj` (the array header that
+    // `array_set_length_from_descriptor` truncates through) and
+    // `descriptor_value` were raw Rust locals across the call.
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let obj_handle = scope.root_raw_mut_ptr(obj);
+    let desc_handle = scope.root_nanbox_f64(descriptor_value);
     let key_str = crate::builtins::js_string_coerce(key_value);
+    let obj = obj_handle.get_raw_mut_ptr::<ObjectHeader>();
+    let descriptor_value = desc_handle.get_nanbox_f64();
     if key_str.is_null() {
         return None;
     }