diff --git a/changelog.d/7978-define-property-rooting.md b/changelog.d/7978-define-property-rooting.md new file mode 100644 index 0000000000..ec26028746 --- /dev/null +++ b/changelog.d/7978-define-property-rooting.md @@ -0,0 +1,76 @@ +**fix(gc): root `Object.defineProperty`'s receiver, key and descriptor fields across its own allocating calls (#7963)** + +A program that ran some allocating work and then installed properties with +`Object.defineProperty` faulted on a retired from-space address under the +quarantine (`PERRY_GC_SCHEDULE_SEED=1 PERRY_GC_SCHEDULE_RATE=1 +PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800`). This is the +window #6949's scope note names and defers, and the one #7949 deliberately left +open — it reproduces with a hand-written `defineProperty` loop, i.e. with +`Object.defineProperties` (the helper #7949 fixed) not on the path at all. + +`js_object_define_property` resolved the receiver's `ObjectHeader` and coerced +the key to a `StringHeader` once, near the top, and then carried both — plus the +three NaN-boxed words `obj_value` / `descriptor_value` / `key_value` — as bare +Rust locals to the end of the function, past `define_array_property`, +`enforce_define_property_invariants`, `obj_value_has_own_key`, +`ensure_key_in_keys_array`, `clone_closure_rebind_this`, +`define_property_force_store_value`, and every `desc_has_field` / +`desc_read_field`. Those last two allocate a field-name string per probe and, on +a descriptor whose fields are accessors, run **user JS** mid-define. A raw Rust +local is neither a shadow slot nor a temp root nor reachable from any registered +scanner, so an evacuating minor could neither keep those objects alive nor +rewrite the local — and `scripts/gc_root_dominance_check.py` reads emitted LLVM +IR, so it is structurally blind to the class. + +The stale receiver was the worse half: `obj as usize` is the OWNER KEY of the +per-property descriptor side tables, so a define that landed after a collection +filed its attributes and accessors under a dead address where the matching read +can never find them — a silent wrong answer rather than a crash. + +Three sites: + +* **`object_ops/define_property.rs`** — all five values are rooted in one scope, + and an `across!` macro is now the only way to name any of them across a call: + it runs the call first and rebinds all five from their roots afterwards, so a + pre-collection address is never nameable. The descriptor's `get`/`set` field + values, the existing accessor's closure bits (written back into the GC-scanned + accessor table when the redefining descriptor omits a field), and the + class-prototype mirror's method value are rooted for the same reason. The + three per-arm `RuntimeHandleScope`s collapse into one — an inner scope dropped + while an outer one is still taking handles truncates the outer container's + newest entries (the hazard documented on `gc::RootedValues`). +* **`object_ops/descriptor_helpers.rs`** — `DescView`'s six field values were + raw `JSValue`s read at decode time and handed back a dozen statements later; + the stale word was then *stored into the receiver*. Each present field is now + a `RuntimeHandle`, so `read` returns the post-collection address. + `validate_nonconfigurable_redefine`'s per-field arm likewise roots the + descriptor, the current value and the current accessor bits, and re-resolves + `desc_ptr` *after* the allocation that precedes each read. +* **`object/reflect_support.rs`** — `obj_value_has_own_key`'s final keys-array + walk held `keys` and `key_str` across `js_array_get`, which materializes a + lazy array and therefore allocates. Both are rooted and re-read per iteration. + +**Proof.** `crates/perry-runtime/src/gc/tests/rooted_define_property.rs`: +`define_property_lands_on_the_receiver_a_descriptor_getter_moved` drives the +real `#[no_mangle]` entry point with an accessor-backed descriptor whose getter +forces a copying minor, and asserts `copied_objects > 0`, that the **receiver's +and the key string's addresses changed**, that the property reads back the +getter's payload bytes, and that `get_property_attrs` finds the entry at the +**live** address. `desc_view_field_values_are_rooted` does the same for the +`DescView` fast path. `unrooted_receiver_copy_still_names_from_space` is the +sabotage arm: the identical address in a plain Rust `usize` keeps naming +from-space in the same cycle in which the rooted handle moves, which is what +makes the other two non-vacuous. + +**Compiled probe.** +`test-files/test_gap_gc_define_property_descriptor_rooting.ts` — an allocating +`Object.groupBy` arm, a hand-written `Object.defineProperty` loop, and a loop +whose descriptor bag carries allocating accessor getters. Under the witness +configuration it exits **138** with `[gc-fromspace-protect] FAULT` (`obj_type=3` += `GC_TYPE_STRING`, faulting at `user_ptr + 4`, which is +`StringHeader::byte_len` — i.e. the stale coerced key) on a pristine +`origin/main` build, and **0**, byte-exact against node 26.5.1, on this branch. + +`scripts/raw_handle_debt.py` falls by 5 (`define_property.rs` 3 → 2, +`reflect_support.rs` 4 → 3); the recorded baseline is deliberately left +unchanged so parallel debt-paying PRs do not collide. diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 1d0cb07ee0..56c254c0a6 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -35,6 +35,7 @@ mod os_tag; mod promote_in_place; mod root_words; mod rooted_container_values; +mod rooted_define_property; mod roots; mod runtime_roots; mod scan_fallback; diff --git a/crates/perry-runtime/src/gc/tests/rooted_define_property.rs b/crates/perry-runtime/src/gc/tests/rooted_define_property.rs new file mode 100644 index 0000000000..240cc35ff7 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/rooted_define_property.rs @@ -0,0 +1,255 @@ +//! #7963 — `Object.defineProperty`'s own receiver / key / descriptor-field +//! window (the one #6949's scope note names and defers, and the one #7949 +//! deliberately left open). +//! +//! ## The window +//! +//! `js_object_define_property` resolves the receiver's `ObjectHeader` and +//! coerces the key to a `StringHeader` once, near the top, and then keeps both +//! as bare Rust locals for the rest of the function — past +//! `enforce_define_property_invariants`, `obj_value_has_own_key`, +//! `ensure_key_in_keys_array`, `clone_closure_rebind_this`, +//! `define_property_force_store_value` and every `desc_has_field` / +//! `desc_read_field`. Those last two allocate a field-name string per probe +//! and, on a descriptor whose fields are accessors, run USER JS. A raw Rust +//! local is neither a shadow slot nor a temp root nor reachable from any +//! registered scanner, so an evacuating minor could neither keep it alive nor +//! rewrite it — and `scripts/gc_root_dominance_check.py` reads emitted LLVM IR, +//! so it is structurally blind to the whole class. +//! +//! The receiver is the worse half: `obj as usize` is the OWNER KEY of the +//! per-property descriptor side tables, so a stale receiver files the property +//! attributes and accessors under a dead address, where the matching read can +//! never find them. That is a silent wrong answer, not a crash. +//! +//! ## What these tests have to prove +//! +//! Not "the call didn't crash". Each test asserts, in this order, that the +//! cycle **actually moved the receiver** (`copied_objects > 0` AND the rooted +//! address changed) before believing anything about survival — a cycle that +//! moved nothing would satisfy the survival assertions vacuously, which is the +//! shape CLAUDE.md calls a presence check rather than a proof. +//! +//! `unrooted_receiver_copy_still_names_from_space` is the sabotage arm and is +//! what makes the rest non-vacuous: the identical address held in a plain Rust +//! `usize` — which is exactly what pre-fix `js_object_define_property` held — +//! keeps naming its pre-collection value in the same cycle in which the rooted +//! one moves. If the instrument could not tell the two apart, that test would +//! fail. + +use super::super::*; +use super::support::*; + +use crate::gc::RuntimeHandleScope; + +thread_local! { + /// Objects relocated by the collections forced from inside the descriptor + /// getter. A run that never moved anything proves nothing, so every test + /// gates on this being non-zero. + static GETTER_COPIED_OBJECTS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +fn register_handle_scanner() { + gc_register_mutable_root_scanner_with_source( + scan_runtime_handle_roots_mut, + MutableRootScannerSource::RuntimeHandles, + ); +} + +fn string_value(text: &str) -> f64 { + let ptr = crate::string::js_string_from_bytes(text.as_ptr(), text.len() as u32); + f64::from_bits(string_bits(ptr as usize)) +} + +unsafe fn string_ptr_of(value: f64) -> *const crate::StringHeader { + (value.to_bits() & POINTER_MASK) as *const crate::StringHeader +} + +fn object_value(obj: *mut crate::object::ObjectHeader) -> f64 { + f64::from_bits(ptr_bits(obj as usize)) +} + +fn addr_of(value: f64) -> usize { + (value.to_bits() & POINTER_MASK) as usize +} + +/// The descriptor's `value` getter: forces a copying minor — which relocates the +/// receiver `js_object_define_property` is holding — and then allocates the +/// payload string, so the retired from-space bytes are reused before the caller +/// reads its locals again. +extern "C" fn moving_value_getter(_closure: *const crate::closure::ClosureHeader) -> f64 { + let trace = collect_minor_trace(GcTriggerKind::Direct); + GETTER_COPIED_OBJECTS.with(|c| c.set(c.get() + trace.copying_nursery.copied_objects)); + string_value("payload") +} + +/// Build `{ get value() { …forces a moving minor…; return "payload" } }`. +/// +/// Installing the field as an ACCESSOR is what forces +/// `js_object_define_property` down its spec-general per-field path +/// (`try_decode_descriptor` refuses any descriptor carrying accessor-backed +/// fields), so `desc_read_field(descriptor, b"value")` runs the getter — user +/// JS, mid-define, exactly the window the issue names. +unsafe fn descriptor_bag_with_moving_value_getter(scope: &RuntimeHandleScope) -> f64 { + let bag = scope.root_nanbox_f64(object_value(crate::object::js_object_alloc(0, 0))); + let inner = scope.root_nanbox_f64(object_value(crate::object::js_object_alloc(0, 0))); + let getter = crate::closure::js_closure_alloc(moving_value_getter as *const u8, 0); + let getter_value = f64::from_bits(ptr_bits(getter as usize)); + + let get_key = crate::string::js_string_from_bytes(b"get".as_ptr(), 3); + crate::object::js_object_set_field_by_name( + addr_of(inner.get_nanbox_f64()) as *mut crate::object::ObjectHeader, + get_key, + getter_value, + ); + crate::object::js_object_define_property( + bag.get_nanbox_f64(), + string_value("value"), + inner.get_nanbox_f64(), + ); + bag.get_nanbox_f64() +} + +/// Read `target[key]` back through the ordinary `[[Get]]`. +unsafe fn read_property(target: f64, key: &str) -> f64 { + crate::value::js_get_property(target, key.as_ptr() as i64, key.len() as i64) +} + +#[test] +fn define_property_lands_on_the_receiver_a_descriptor_getter_moved() { + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + register_handle_scanner(); + GETTER_COPIED_OBJECTS.with(|c| c.set(0)); + + unsafe { + let scope = RuntimeHandleScope::new(); + let target = scope.root_nanbox_f64(object_value(crate::object::js_object_alloc(0, 0))); + let key = scope.root_nanbox_f64(string_value("moved_key")); + let bag = descriptor_bag_with_moving_value_getter(&scope); + let bag_handle = scope.root_nanbox_f64(bag); + + let target_before = addr_of(target.get_nanbox_f64()); + let key_before = addr_of(key.get_nanbox_f64()); + + crate::object::js_object_define_property( + target.get_nanbox_f64(), + key.get_nanbox_f64(), + bag_handle.get_nanbox_f64(), + ); + + // ---- the cycle has to have MOVED the receiver, or nothing below means + // anything. Both halves: something was copied, and this object's + // address changed. + assert!( + GETTER_COPIED_OBJECTS.with(|c| c.get()) > 0, + "the descriptor getter's collection moved nothing -- the assertions \ + below would be vacuous" + ); + let target_after = addr_of(target.get_nanbox_f64()); + assert_ne!( + target_after, target_before, + "the receiver was not relocated -- this run proves nothing about rooting" + ); + assert_ne!( + addr_of(key.get_nanbox_f64()), + key_before, + "the key string was not relocated -- this run proves nothing about rooting" + ); + + // ---- and the define has to have landed on the object that is alive + // NOW, not on the address the call started with. + let read_back = read_property(target.get_nanbox_f64(), "moved_key"); + assert_string_bytes(string_ptr_of(read_back), b"payload"); + + // The per-property attribute table is keyed by the receiver's ADDRESS. + // A stale receiver files the entry under the pre-collection address, so + // this lookup at the live address is what catches it. + assert!( + crate::object::descriptor_state::get_property_attrs(target_after, "moved_key") + .is_some(), + "property attributes were filed under a pre-collection receiver address" + ); + } +} + +#[test] +fn unrooted_receiver_copy_still_names_from_space() { + // The sabotage arm for both tests above. A receiver address copied into a + // plain Rust `usize` -- precisely what pre-fix `js_object_define_property` + // carried through its tail -- is invisible to the collector, so it keeps + // naming from-space across the very cycle in which the rooted handle to the + // SAME object is rewritten. This is what proves the assertions above are + // measuring rooting rather than an allocator that happened not to move + // anything. + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + register_handle_scanner(); + + unsafe { + let scope = RuntimeHandleScope::new(); + let rooted = scope.root_nanbox_f64(object_value(crate::object::js_object_alloc(0, 0))); + let unrooted_copy = addr_of(rooted.get_nanbox_f64()); + + let trace = collect_minor_trace(GcTriggerKind::Direct); + assert_copied_minor_trace(&trace, true, CopiedMinorFallbackReason::None, false); + assert!(trace.copying_nursery.copied_objects > 0); + + assert_ne!( + addr_of(rooted.get_nanbox_f64()), + unrooted_copy, + "the rooted receiver did not move -- this cycle cannot demonstrate the hazard" + ); + // And the plain copy is unchanged, by construction: nothing can rewrite + // a Rust local. If this ever fails, the collector grew a way to see the + // Rust stack and the `across!` discipline can be retired. + assert_eq!( + unrooted_copy, unrooted_copy, + "a plain usize cannot be rewritten by the collector" + ); + } +} + +#[test] +fn desc_view_field_values_are_rooted() { + // `try_decode_descriptor`'s fast path reads all six `ToPropertyDescriptor` + // fields ONCE and the caller reads them back much later, past several + // allocating calls. Before #7963 the six words were raw `JSValue`s in a + // Rust struct; now each present field is a runtime handle, so `read` + // returns the post-collection address. + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + register_handle_scanner(); + + unsafe { + let scope = RuntimeHandleScope::new(); + let descriptor = scope.root_nanbox_f64(object_value(crate::object::js_object_alloc(0, 0))); + let value_key = crate::string::js_string_from_bytes(b"value".as_ptr(), 5); + let payload = string_value("desc_view_payload"); + crate::object::js_object_set_field_by_name( + addr_of(descriptor.get_nanbox_f64()) as *mut crate::object::ObjectHeader, + value_key, + payload, + ); + + let view = crate::object::try_decode_descriptor(&scope, descriptor.get_nanbox_f64()) + .expect("a plain object literal descriptor must take the fast decode path"); + assert!(view.has(crate::object::DESC_VALUE)); + let before = addr_of(f64::from_bits(view.read(crate::object::DESC_VALUE).bits())); + + let trace = collect_minor_trace(GcTriggerKind::Direct); + assert_copied_minor_trace(&trace, true, CopiedMinorFallbackReason::None, false); + assert!( + trace.copying_nursery.copied_objects > 0, + "the cycle moved nothing -- the assertion below would be vacuous" + ); + + let after_value = f64::from_bits(view.read(crate::object::DESC_VALUE).bits()); + assert_ne!( + addr_of(after_value), + before, + "the descriptor's `value` was not relocated -- this run proves nothing" + ); + assert_string_bytes(string_ptr_of(after_value), b"desc_view_payload"); + } +} diff --git a/crates/perry-runtime/src/object/object_ops/define_property.rs b/crates/perry-runtime/src/object/object_ops/define_property.rs index 5a0d32d02d..5ba09e966e 100644 --- a/crates/perry-runtime/src/object/object_ops/define_property.rs +++ b/crates/perry-runtime/src/object/object_ops/define_property.rs @@ -296,6 +296,11 @@ unsafe fn define_property_on_handle( /// the descriptor — otherwise a `writable: false` descriptor would block its own /// initial value from being stored. #[no_mangle] +// `across!` (defined in the ordinary arm below) rebinds ALL FIVE roots on every +// use, by design: the shape must not depend on which of them the next statement +// happens to read. The final rebind of each is therefore dead, which is the +// point — nothing may name a pre-collection address. +#[allow(unused_assignments)] pub extern "C" fn js_object_define_property( obj_value: f64, key_value: f64, @@ -418,12 +423,20 @@ pub extern "C" fn js_object_define_property( let desc = describe_value_for_type_error(descriptor_value); throw_object_type_error_with_suffix("Property description must be an object: ", &desc); } + // #7963: ONE handle scope for everything below. The decoded descriptor's + // field values, the receiver, the coerced key string and the accessor + // closures are all live across calls that allocate, and this scope is + // what makes them GC roots. It is deliberately a single scope: an inner + // `RuntimeHandleScope` dropped while an outer one is still taking + // handles truncates the outer container's newest entries (see + // `gc::RootedValues`' module docs), so the arms below share this one. + let scope = crate::gc::RuntimeHandleScope::new(); // #6748 follow-up: decode the descriptor's 6 fields in ONE pass when it // is a plain default-prototype object (the overwhelming majority) — // the per-field `desc_has_field`/`desc_read_field` helpers each cost a // key-string alloc plus a HasProperty/[[Get]] walk. `None` keeps the // spec-general per-field path everywhere below. - let desc_view = try_decode_descriptor(descriptor_value); + let desc_view = try_decode_descriptor(&scope, descriptor_value); match &desc_view { Some(v) => validate_property_descriptor_view(v), None => validate_property_descriptor(descriptor_value), @@ -673,7 +686,6 @@ pub extern "C" fn js_object_define_property( // `closure_ptr` files the property under a dead address, where the // matching read can never find it. Root all three across the // coercion and read them back through their handles. - let scope = crate::gc::RuntimeHandleScope::new(); let obj_handle = scope.root_heap_word_u64(obj_value.to_bits()); let desc_handle = scope.root_nanbox_f64(descriptor_value); let closure_handle = scope.root_raw_mut_ptr(closure_ptr as *mut u8); @@ -868,7 +880,6 @@ pub extern "C" fn js_object_define_property( // the raw local at risk is `addr` — the TypedArray's heap address, // resolved from `obj_value` *before* the coercion and dereferenced // as a `TypedArrayHeader` after it. - let scope = crate::gc::RuntimeHandleScope::new(); let obj_handle = scope.root_heap_word_u64(obj_value.to_bits()); let desc_handle = scope.root_nanbox_f64(descriptor_value); let addr_handle = scope.root_raw_mut_ptr(addr as *mut u8); @@ -979,13 +990,29 @@ pub extern "C" fn js_object_define_property( } // Extract key string. // - // #6943: the ordinary arm's raw local is `obj` — the receiver's - // `ObjectHeader`, resolved above and dereferenced below (class-id - // probe, typed-array define, `define_array_property`, the keys_array - // walk). It, `obj_value` and `descriptor_value` are rooted across the - // GC-capable coercion; see the closure arm above for the full - // reasoning. - let scope = crate::gc::RuntimeHandleScope::new(); + // #6943 / #7963: the ordinary arm carries TWO raw heap pointers all the + // way to the end of this function — `obj`, the receiver's + // `ObjectHeader`, and `key_str`, the coerced key — plus three NaN-boxed + // words (`obj_value`, `descriptor_value`, `key_value`). Between here + // and the last use it runs a dozen calls that can allocate and + // therefore EVACUATE: `js_string_coerce` itself, + // `define_array_property`, `enforce_define_property_invariants`, + // `obj_value_has_own_key`, `ensure_key_in_keys_array`, + // `clone_closure_rebind_this`, `define_property_force_store_value`, and + // every `desc_has_field` / `desc_read_field` (each allocates a + // field-name string, and on a non-plain descriptor runs a USER GETTER). + // A raw Rust local is neither a shadow slot nor a temp root nor + // reachable from any registered scanner, so the collector can neither + // keep those objects alive nor rewrite the local. + // + // The stale receiver is worse than a stale read: `obj as usize` is the + // OWNER KEY of the per-property descriptor side tables, so a define + // that lands after a collection files its attributes and accessors + // under a dead address, where the matching read can never find them. + // + // `across!` below is the only way to name any of them across a call: it + // runs the call FIRST and rebinds every one from its root afterwards, + // so a pre-collection address is never nameable. let obj_handle = scope.root_raw_mut_ptr(obj); let obj_value_handle = scope.root_heap_word_u64(obj_value.to_bits()); let desc_handle = scope.root_nanbox_f64(descriptor_value); @@ -994,15 +1021,35 @@ pub extern "C" fn js_object_define_property( // object / BigInt key evacuated by the coercion on the next line would // be dereferenced again there. let key_handle = scope.root_nanbox_f64(key_value); - let key_str = crate::builtins::js_string_coerce(key_value); - let obj = obj_handle.get_raw_mut_ptr::(); - let obj_value = f64::from_bits(obj_value_handle.get_heap_word_u64()); - let descriptor_value = desc_handle.get_nanbox_f64(); - let key_value = key_handle.get_nanbox_f64(); + let (key_str, mut obj) = obj_handle + .across_mut::(|| crate::builtins::js_string_coerce(key_value)); + let mut obj_value = f64::from_bits(obj_value_handle.get_heap_word_u64()); + let mut descriptor_value = desc_handle.get_nanbox_f64(); + let mut key_value = key_handle.get_nanbox_f64(); if key_str.is_null() { return obj_value; } + let key_str_handle = scope.root_string_ptr(key_str); + let mut key_str = key_str; + // Run `$call` — which may allocate, and therefore may MOVE any of the + // five values above — then rebind all five from their roots. Never bind + // the pre-call address to anything that outlives the call. + macro_rules! across { + ($call:expr) => {{ + let (result, refreshed_obj) = obj_handle.across_mut::(|| $call); + let ((), refreshed_key) = + key_str_handle.across_mut::(|| ()); + obj = refreshed_obj; + key_str = refreshed_key; + obj_value = f64::from_bits(obj_value_handle.get_heap_word_u64()); + descriptor_value = desc_handle.get_nanbox_f64(); + key_value = key_handle.get_nanbox_f64(); + result + }}; + } // Extract the key as a Rust string for the descriptor side-table lookup. + // A `String` is on the Rust heap, so it is immune to evacuation and can + // safely be read after any of the calls below. let key_rust: Option = { let name_ptr = (key_str as *const u8).add(std::mem::size_of::()); let name_len = (*key_str).byte_len as usize; @@ -1018,20 +1065,26 @@ pub extern "C" fn js_object_define_property( super::super::class_registry::class_id_for_decl_prototype_object(obj as usize) { if let Some(ref name) = key_rust { - if desc_has_field(descriptor_value, b"value") { - let value_field = desc_read_field(descriptor_value, b"value"); - if !value_field.is_undefined() { + if across!(desc_has_field(descriptor_value, b"value")) { + let value_bits = across!(desc_read_field(descriptor_value, b"value").bits()); + if !crate::value::JSValue::from_bits(value_bits).is_undefined() { + // The method value must survive `descriptor_enumerable`, + // which reads two more descriptor fields. + let value_slot = scope.root_nanbox_u64(value_bits); // #5024 followup: defineProperty data descriptor is // non-enumerable unless it sets `enumerable: true`. Mark // it so the prototype-method enumeration mirror honours // the descriptor instead of defaulting to enumerable // (the `Class.prototype.m = fn` assignment default). + let enumerable = across!(descriptor_enumerable(descriptor_value)); super::super::class_registry::class_prototype_method_set_enumerable( + target_cid, name, enumerable, + ); + define_class_prototype_method( target_cid, name, - descriptor_enumerable(descriptor_value), + value_slot.get_nanbox_u64(), ); - define_class_prototype_method(target_cid, name, value_field.bits()); } } } @@ -1050,18 +1103,18 @@ pub extern "C" fn js_object_define_property( } return obj_value; } - if let Some(ok) = (!receiver_plain_object) - .then(|| { - super::super::define_array_property( - obj, - obj_value, - key_str, - key_rust.as_deref(), - descriptor_value, - ) - }) - .flatten() - { + let array_outcome = if receiver_plain_object { + None + } else { + across!(super::super::define_array_property( + obj, + obj_value, + key_str, + key_rust.as_deref(), + descriptor_value, + )) + }; + if let Some(ok) = array_outcome { if ok { return obj_value; } @@ -1076,18 +1129,17 @@ pub extern "C" fn js_object_define_property( // mutation, so a rejected definition leaves the object untouched and the // thrown TypeError matches Node. if let Some(ref k) = key_rust { - enforce_define_property_invariants( + across!(enforce_define_property_invariants( obj, key_str, k, descriptor_value, desc_view.as_ref(), - ); + )); } super::super::mark_object_dynamic_shape_unknown(obj); // Extract descriptor object - let desc_ptr = extract_obj_ptr(descriptor_value); - if desc_ptr.is_null() { + if extract_obj_ptr(descriptor_value).is_null() { return obj_value; } @@ -1097,12 +1149,15 @@ pub extern "C" fn js_object_define_property( // NOT reset to the new-property `false` default. Capture the current // attributes before any mutation below. `None` ⇒ the key is new, so the // historical all-`false` (writable defaults to `has_accessor`) applies. - let existing_attrs: Option = key_rust.as_ref().and_then(|k| { + let existing_attrs: Option = if let Some(ref k) = key_rust { // #6743: wide objects answer own-key presence via the O(1) sidecar // (repeated defines were O(N²) through this check); narrow or // non-indexable receivers keep the general path. - let present = own_key_present_via_index(obj, key_str) - .unwrap_or_else(|| super::super::obj_value_has_own_key(obj_value, key_value)); + let indexed = own_key_present_via_index(obj, key_str); + let present = match indexed { + Some(present) => present, + None => across!(super::super::obj_value_has_own_key(obj_value, key_value)), + }; if present { Some( super::super::get_property_attrs(obj as usize, k) @@ -1111,40 +1166,55 @@ pub extern "C" fn js_object_define_property( } else { None } - }); + } else { + None + }; // Detect accessor descriptor (has `get` and/or `set`) vs. data // descriptor (has `value`/`writable`) by `ToPropertyDescriptor` field // PRESENCE (HasProperty — own OR inherited) on the descriptor object, // not by `is_undefined`: `{ get: undefined }` is an explicit (present) // accessor field, and an *inherited* `value`/`get` counts as present. - let (desc_has_get, desc_has_set, get_field, set_field) = match &desc_view { - Some(v) => ( - v.has(DESC_GET), - v.has(DESC_SET), - v.read(DESC_GET), - v.read(DESC_SET), - ), - None => ( - desc_has_field(descriptor_value, b"get"), - desc_has_field(descriptor_value, b"set"), - desc_read_field(descriptor_value, b"get"), - desc_read_field(descriptor_value, b"set"), - ), + // + // The two field VALUES are rooted: `ensure_key_in_keys_array` and the + // first `clone_closure_rebind_this` both run before the second is read. + let get_field_slot = scope.root_nanbox_u64(crate::value::TAG_UNDEFINED); + let set_field_slot = scope.root_nanbox_u64(crate::value::TAG_UNDEFINED); + let (desc_has_get, desc_has_set) = match &desc_view { + Some(v) => { + get_field_slot.set_nanbox_u64(v.read(DESC_GET).bits()); + set_field_slot.set_nanbox_u64(v.read(DESC_SET).bits()); + (v.has(DESC_GET), v.has(DESC_SET)) + } + None => { + let has_get = across!(desc_has_field(descriptor_value, b"get")); + let has_set = across!(desc_has_field(descriptor_value, b"set")); + let get_bits = across!(desc_read_field(descriptor_value, b"get").bits()); + get_field_slot.set_nanbox_u64(get_bits); + let set_bits = across!(desc_read_field(descriptor_value, b"set").bits()); + set_field_slot.set_nanbox_u64(set_bits); + (has_get, has_set) + } }; let has_accessor = desc_has_get || desc_has_set; // The existing accessor (if the property is currently an accessor) — // used to retain `get`/`set` fields the redefining descriptor omits. + // Its two words are closure POINTERS that are written back into the + // (GC-scanned) accessor table below, past `ensure_key_in_keys_array` and + // two closure clones, so they are rooted rather than copied. let existing_accessor: Option = key_rust .as_ref() .and_then(|k| super::super::get_accessor_descriptor(obj as usize, k)); + let had_existing_accessor = existing_accessor.is_some(); + let prior_get_slot = scope.root_nanbox_u64(existing_accessor.map(|a| a.get).unwrap_or(0)); + let prior_set_slot = scope.root_nanbox_u64(existing_accessor.map(|a| a.set).unwrap_or(0)); if has_accessor { // Store the accessor closures in the side table. Ensure the key is present // in the object's keys_array so lookups (hasOwn, getOwnPropertyDescriptor, // keys) can see it. - ensure_key_in_keys_array(obj, key_str); + across!(ensure_key_in_keys_array(obj, key_str)); if let Some(k) = key_rust.clone() { // Issue #450: spec says the getter/setter runs with `this === obj` // (the property access target). The user's descriptor literal @@ -1162,32 +1232,44 @@ pub extern "C" fn js_object_define_property( // `set`) keeps the current accessor's `get` (or `set`). When the // current property is a data property being converted to an // accessor, omitted fields default to `undefined` (0). - let recv_box = crate::value::js_nanbox_pointer(obj as i64); - let prior = existing_accessor; - let get_bits = if desc_has_get { - if get_field.is_undefined() { - 0u64 + let get_out_slot = scope.root_nanbox_u64(0); + if desc_has_get { + let get_field = get_field_slot.get_nanbox_u64(); + if crate::value::JSValue::from_bits(get_field).is_undefined() { + get_out_slot.set_nanbox_u64(0); } else { - crate::closure::clone_closure_rebind_this(get_field.bits(), recv_box) + // `recv_box` is derived from the CURRENT receiver, one + // statement before the clone that can move it. + let recv_box = crate::value::js_nanbox_pointer(obj as i64); + let cloned = across!(crate::closure::clone_closure_rebind_this( + get_field, recv_box + )); + get_out_slot.set_nanbox_u64(cloned); } } else { - prior.map(|a| a.get).unwrap_or(0) - }; - let set_bits = if desc_has_set { - if set_field.is_undefined() { - 0u64 + get_out_slot.set_nanbox_u64(prior_get_slot.get_nanbox_u64()); + } + let set_out_slot = scope.root_nanbox_u64(0); + if desc_has_set { + let set_field = set_field_slot.get_nanbox_u64(); + if crate::value::JSValue::from_bits(set_field).is_undefined() { + set_out_slot.set_nanbox_u64(0); } else { - crate::closure::clone_closure_rebind_this(set_field.bits(), recv_box) + let recv_box = crate::value::js_nanbox_pointer(obj as i64); + let cloned = across!(crate::closure::clone_closure_rebind_this( + set_field, recv_box + )); + set_out_slot.set_nanbox_u64(cloned); } } else { - prior.map(|a| a.set).unwrap_or(0) - }; + set_out_slot.set_nanbox_u64(prior_set_slot.get_nanbox_u64()); + } set_accessor_descriptor( obj as usize, k, AccessorDescriptor { - get: get_bits, - set: set_bits, + get: get_out_slot.get_nanbox_u64(), + set: set_out_slot.get_nanbox_u64(), }, ); } @@ -1198,10 +1280,11 @@ pub extern "C" fn js_object_define_property( // while a generic descriptor on an existing accessor leaves it intact. let (desc_has_value, desc_has_writable) = match &desc_view { Some(v) => (v.has(DESC_VALUE), v.has(DESC_WRITABLE)), - None => ( - desc_has_field(descriptor_value, b"value"), - desc_has_field(descriptor_value, b"writable"), - ), + None => { + let has_value = across!(desc_has_field(descriptor_value, b"value")); + let has_writable = across!(desc_has_field(descriptor_value, b"writable")); + (has_value, has_writable) + } }; let is_data = desc_has_value || desc_has_writable; @@ -1219,58 +1302,41 @@ pub extern "C" fn js_object_define_property( .remove(&(obj as usize, k.clone())); clear_property_attrs(obj as usize, k); } - let value_field = match &desc_view { - Some(v) => v.read(DESC_VALUE), - None => desc_read_field(descriptor_value, b"value"), - }; // Ensure the key exists; store the (possibly `undefined`) value // via `[[DefineOwnProperty]]`, bypassing the `[[Set]]` writability // / frozen guard (invariants already enforced above). When // `value` is omitted (a `{ writable: ... }`-only descriptor on a // brand-new property) the value defaults to `undefined`. if desc_has_value { - define_property_force_store_value( + let value_bits = match &desc_view { + Some(v) => v.read(DESC_VALUE).bits(), + None => across!(desc_read_field(descriptor_value, b"value").bits()), + }; + across!(define_property_force_store_value( obj, key_str, - f64::from_bits(value_field.bits()), - ); - } else if existing_accessor.is_some() { + f64::from_bits(value_bits), + )); + } else if had_existing_accessor { // Accessor → data with no `value`: the value becomes the // data default `undefined`. - define_property_force_store_value( + across!(define_property_force_store_value( obj, key_str, f64::from_bits(crate::value::TAG_UNDEFINED), - ); + )); } else { - ensure_key_in_keys_array(obj, key_str); + across!(ensure_key_in_keys_array(obj, key_str)); } } else { // Generic descriptor: no value/writable/get/set. It only adjusts // enumerable/configurable and never converts the property kind. // Leave any existing accessor / data value untouched; just make // sure the key is present (for a brand-new generic define). - ensure_key_in_keys_array(obj, key_str); + across!(ensure_key_in_keys_array(obj, key_str)); } } - // Read attribute flags from descriptor. JS defaults when omitted in - // `Object.defineProperty` are `false` (NOT `true` like for direct assignment). - let read_bool = |name: &[u8]| -> Option { - let v = match &desc_view { - Some(view) => view.read(match name { - b"writable" => DESC_WRITABLE, - b"enumerable" => DESC_ENUMERABLE, - _ => DESC_CONFIGURABLE, - }), - None => desc_read_field(descriptor_value, name), - }; - if v.is_undefined() { - None - } else { - Some(crate::value::js_is_truthy(f64::from_bits(v.bits())) != 0) - } - }; // Omitted attributes default to the EXISTING property's value when // redefining (spec retention, see `existing_attrs` above), else to // `false` for a new property. Accessor descriptors don't carry @@ -1281,25 +1347,50 @@ pub extern "C" fn js_object_define_property( // Accessor → data conversion: the current property has no // [[Writable]], so an omitted `writable` defaults to FALSE (the // retained-attrs rule doesn't apply across the kind switch). - let accessor_to_data = existing_accessor.is_some() + let accessor_to_data = had_existing_accessor && !has_accessor && match &desc_view { Some(v) => v.has(DESC_VALUE) || v.has(DESC_WRITABLE), None => { - desc_has_field(descriptor_value, b"value") - || desc_has_field(descriptor_value, b"writable") + let has_value = across!(desc_has_field(descriptor_value, b"value")); + let has_writable = across!(desc_has_field(descriptor_value, b"writable")); + has_value || has_writable } }; - let writable = read_bool(b"writable").unwrap_or_else(|| { + // Read attribute flags from descriptor. JS defaults when omitted in + // `Object.defineProperty` are `false` (NOT `true` like for direct + // assignment). Each field is converted to a plain `bool` immediately + // after its read — `is_undefined` / `js_is_truthy` cannot allocate — so + // no NaN-boxed word survives the NEXT field's read. + let flag_of = |bits: u64| -> Option { + if crate::value::JSValue::from_bits(bits).is_undefined() { + None + } else { + Some(crate::value::js_is_truthy(f64::from_bits(bits)) != 0) + } + }; + let writable_flag = flag_of(match &desc_view { + Some(v) => v.read(DESC_WRITABLE).bits(), + None => across!(desc_read_field(descriptor_value, b"writable").bits()), + }); + let enumerable_flag = flag_of(match &desc_view { + Some(v) => v.read(DESC_ENUMERABLE).bits(), + None => across!(desc_read_field(descriptor_value, b"enumerable").bits()), + }); + let configurable_flag = flag_of(match &desc_view { + Some(v) => v.read(DESC_CONFIGURABLE).bits(), + None => across!(desc_read_field(descriptor_value, b"configurable").bits()), + }); + let writable = writable_flag.unwrap_or_else(|| { if accessor_to_data { false } else { existing_attrs.map(|a| a.writable()).unwrap_or(has_accessor) } }); - let enumerable = read_bool(b"enumerable") + let enumerable = enumerable_flag .unwrap_or_else(|| existing_attrs.map(|a| a.enumerable()).unwrap_or(false)); - let configurable = read_bool(b"configurable") + let configurable = configurable_flag .unwrap_or_else(|| existing_attrs.map(|a| a.configurable()).unwrap_or(false)); if let Some(k) = key_rust { diff --git a/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs b/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs index ea90f4a99c..79f5f046c7 100644 --- a/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs +++ b/crates/perry-runtime/src/object/object_ops/descriptor_helpers.rs @@ -135,20 +135,38 @@ pub(crate) const DESC_WRITABLE: usize = 3; pub(crate) const DESC_ENUMERABLE: usize = 4; pub(crate) const DESC_CONFIGURABLE: usize = 5; -pub(crate) struct DescView { +/// A decoded `ToPropertyDescriptor` result whose six field values are GC roots. +/// +/// #7963: the view is built ONCE near the top of `js_object_define_property` +/// and then read at a dozen points spread across the rest of that function — +/// past `ensure_key_in_keys_array`, `clone_closure_rebind_this`, +/// `define_property_force_store_value` and the own-key probes, every one of +/// which can allocate and therefore evacuate. Six raw `JSValue`s in a Rust +/// struct are neither shadow slots nor temp roots nor reachable from any +/// registered scanner, so an evacuating minor could neither keep those values +/// alive nor rewrite them — and the stale word was then *stored into* the +/// receiver (`define_property_force_store_value`) or into the accessor side +/// table. Holding each present field as a [`crate::gc::RuntimeHandle`] puts it +/// on the already-registered runtime-handle mutable root scanner, so `read` +/// hands back the post-collection address. +pub(crate) struct DescView<'scope> { present: [bool; 6], - vals: [crate::value::JSValue; 6], + handles: [Option>; 6], } -impl DescView { +impl DescView<'_> { #[inline] pub(crate) fn has(&self, f: usize) -> bool { self.present[f] } - /// Field value; `undefined` when absent (matching the per-field readers). + /// Field value, **re-read from its root**; `undefined` when absent + /// (matching the per-field readers). #[inline] pub(crate) fn read(&self, f: usize) -> crate::value::JSValue { - self.vals[f] + match &self.handles[f] { + Some(h) => crate::value::JSValue::from_bits(h.get_nanbox_u64()), + None => crate::value::JSValue::from_bits(crate::value::TAG_UNDEFINED), + } } } @@ -203,7 +221,10 @@ unsafe fn object_prototype_has_desc_field() -> bool { /// Single-pass decode of `descriptor_value`'s 6 `ToPropertyDescriptor` fields. /// `Some(view)` is exactly equivalent to running `desc_has_field` / /// `desc_read_field` per field; `None` means the caller must use those. -pub(crate) unsafe fn try_decode_descriptor(descriptor_value: f64) -> Option { +pub(crate) unsafe fn try_decode_descriptor<'scope>( + scope: &'scope crate::gc::RuntimeHandleScope, + descriptor_value: f64, +) -> Option> { let jv = crate::value::JSValue::from_bits(descriptor_value.to_bits()); if !jv.is_pointer() { return None; @@ -256,10 +277,9 @@ pub(crate) unsafe fn try_decode_descriptor(descriptor_value: f64) -> Option Option Option) { let has_get = view.has(DESC_GET); let has_set = view.has(DESC_SET); if (has_get || has_set) && (view.has(DESC_VALUE) || view.has(DESC_WRITABLE)) { @@ -503,7 +526,7 @@ pub(crate) unsafe fn enforce_define_property_invariants( key: *const crate::StringHeader, key_name: &str, descriptor_value: f64, - desc_view: Option<&DescView>, + desc_view: Option<&DescView<'_>>, ) { if obj.is_null() || (obj as usize) <= 0x10000 { return; @@ -567,13 +590,23 @@ pub(crate) unsafe fn validate_nonconfigurable_redefine( cur_accessor: Option, cur_value: f64, descriptor_value: f64, - desc_view: Option<&DescView>, + desc_view: Option<&DescView<'_>>, ) { const TAG_TRUE: u64 = 0x7FFC_0000_0000_0004; - let desc_ptr = extract_obj_ptr(descriptor_value); - if desc_ptr.is_null() && desc_view.is_none() { + if extract_obj_ptr(descriptor_value).is_null() && desc_view.is_none() { return; } + // #7963: the `desc_view.is_none()` arm allocates a field-name string per + // probe (and `desc_has_field` can run a user `HasProperty`), so the + // descriptor object, the CURRENT value being compared against, and the + // current accessor's closure bits are all live across a collection point. + // Root them and re-read at every use; `desc_ptr` in particular is + // re-resolved AFTER the allocation that precedes each read. + let scope = crate::gc::RuntimeHandleScope::new(); + let desc_handle = scope.root_nanbox_f64(descriptor_value); + let cur_value_handle = scope.root_nanbox_f64(cur_value); + let acc_get_handle = scope.root_nanbox_u64(cur_accessor.map(|a| a.get).unwrap_or(0)); + let acc_set_handle = scope.root_nanbox_u64(cur_accessor.map(|a| a.set).unwrap_or(0)); let reject = || throw_object_type_error_with_suffix("Cannot redefine property: ", key_name); let view_index = |name: &[u8]| -> usize { @@ -590,7 +623,7 @@ pub(crate) unsafe fn validate_nonconfigurable_redefine( let has_field = |name: &[u8]| -> bool { match desc_view { Some(v) => v.has(view_index(name)), - None => desc_has_field(descriptor_value, name), + None => desc_has_field(desc_handle.get_nanbox_f64(), name), } }; let read = |name: &[u8]| -> crate::value::JSValue { @@ -598,6 +631,8 @@ pub(crate) unsafe fn validate_nonconfigurable_redefine( Some(v) => v.read(view_index(name)), None => { let k = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + // Resolve the descriptor AFTER the allocation above. + let desc_ptr = extract_obj_ptr(desc_handle.get_nanbox_f64()); js_object_get_field_by_name(desc_ptr as *const ObjectHeader, k) } } @@ -651,6 +686,7 @@ pub(crate) unsafe fn validate_nonconfigurable_redefine( 0 } }; + let _ = acc; if desc_has_get { let want = read(b"get"); let want_fp = if want.is_undefined() { @@ -658,7 +694,9 @@ pub(crate) unsafe fn validate_nonconfigurable_redefine( } else { closure_func_ptr(want.bits()) }; - if want_fp != closure_func_ptr(acc.get) { + // `read` can allocate, so take the CURRENT accessor bits from the + // root rather than the pre-call copy captured in `cur_accessor`. + if want_fp != closure_func_ptr(acc_get_handle.get_nanbox_u64()) { reject(); } } @@ -669,7 +707,7 @@ pub(crate) unsafe fn validate_nonconfigurable_redefine( } else { closure_func_ptr(want.bits()) }; - if want_fp != closure_func_ptr(acc.set) { + if want_fp != closure_func_ptr(acc_set_handle.get_nanbox_u64()) { reject(); } } @@ -685,7 +723,8 @@ pub(crate) unsafe fn validate_nonconfigurable_redefine( } if desc_has_value { let new_value = f64::from_bits(read(b"value").bits()); - if js_object_is(new_value, cur_value).to_bits() != TAG_TRUE { + // `read` can allocate; `cur_value` is a pre-call copy. + if js_object_is(new_value, cur_value_handle.get_nanbox_f64()).to_bits() != TAG_TRUE { reject(); } } diff --git a/crates/perry-runtime/src/object/reflect_support.rs b/crates/perry-runtime/src/object/reflect_support.rs index c5594139df..448831e752 100644 --- a/crates/perry-runtime/src/object/reflect_support.rs +++ b/crates/perry-runtime/src/object/reflect_support.rs @@ -122,30 +122,41 @@ pub(crate) fn obj_value_has_own_key(value: f64, key: f64) -> bool { } // #6943: the ordinary arm dereferences `obj` for its `keys_array` // *after* the GC-capable coercion, so the receiver is rooted across it. - // An already-heap-string key — the common `Reflect.defineProperty(o, - // "x", …)` shape — keeps the pre-fix path: `js_string_coerce` returns - // that pointer unchanged without touching the allocator. - let (obj, key_str) = if crate::builtins::string_coerce_is_inert(key) { - (obj, crate::builtins::js_string_coerce(key)) - } else { - let scope = crate::gc::RuntimeHandleScope::new(); - let obj_handle = scope.root_raw_mut_ptr(obj); - let key_str = crate::builtins::js_string_coerce(key); - (obj_handle.get_raw_mut_ptr::(), key_str) - }; + // (#7963 dropped the `string_coerce_is_inert` shortcut around this + // scope: the keys walk below needs the same scope for its own roots + // whatever the key's shape, so skipping it here bought nothing.) + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_raw_mut_ptr(obj); + let (key_str, obj) = obj_handle + .across_mut::(|| crate::builtins::js_string_coerce(key)); if key_str.is_null() { return false; } if let Some(present) = crate::process::process_env_has_field(obj, key_str) { return present; } - let keys = (*obj).keys_array; + // #7963 (the second half of #6949's deferred scope note): `js_array_get` + // MATERIALIZES a lazy array, so it can allocate and therefore evacuate. + // `keys` and `key_str` were raw Rust locals walked across it — neither + // shadow slots nor temp roots nor reachable from any registered + // scanner, so the collector could neither keep them alive nor rewrite + // them, and the very next iteration compared a from-space string + // against a from-space slot. Root both and re-read each iteration; the + // pre-call addresses are never bound past the call. + let keys_handle = scope.root_raw_mut_ptr((*obj).keys_array); + let key_handle = scope.root_string_ptr(key_str); + let ((), mut keys) = keys_handle.across_mut::(|| ()); if keys.is_null() || (keys as usize) < 0x10000 { return false; } let key_count = crate::array::js_array_length(keys) as usize; for i in 0..key_count { - let stored = crate::array::js_array_get(keys, i as u32); + let (stored, refreshed_keys) = + keys_handle.across_mut::(|| { + crate::array::js_array_get(keys, i as u32) + }); + let ((), key_str) = key_handle.across_const::(|| ()); + keys = refreshed_keys; if crate::string::js_string_key_matches(stored, key_str) { return true; } diff --git a/gc-handoff/DEFPROP-NOTES.md b/gc-handoff/DEFPROP-NOTES.md new file mode 100644 index 0000000000..3034e48c1b --- /dev/null +++ b/gc-handoff/DEFPROP-NOTES.md @@ -0,0 +1,268 @@ +# #7963 — `Object.defineProperty`'s own receiver / key / descriptor-field window + +Working notes for the `gc/7963-define-property-rooting` branch. Written +incrementally; the PR body is the summary, this is the audit trail. + +## The class + +Raw NaN-boxed values and raw heap pointers held in ordinary Rust locals across +calls that can allocate. Neither shadow slots nor temp roots nor reachable from +any registered scanner, so an evacuating minor can neither keep them alive nor +rewrite them. `scripts/gc_root_dominance_check.py` reads emitted LLVM IR, so it +is structurally blind to the whole class. + +#7949/#7962 closed the *container* shape (`Vec` accumulators). This is the +`obj` / `key_str` / `DescView` shape #6949's scope note names and defers: + +> `js_object_define_property` also holds `obj` / `descriptor_value` and the six +> raw `JSValue`s inside `DescView` across its own later `js_string_from_bytes` +> calls, and `obj_value_has_own_key` holds `keys` / `key_str` across a +> `js_array_get` walk that can materialize a lazy array. + +## The pristine fault, reproduced and localized + +`test-files/test_gap_gc_define_property_descriptor_rooting.ts` under the witness +configuration, on a pristine `origin/main` release build (`a769fafc6`, +`PERRY_NO_AUTO_OPTIMIZE=1`, `PERRY_RUNTIME_DIR` pinned to that build's `.a` +pair): + +``` +exit 138, stdout stopped after "definePropertyOneAtATime ok" + +[gc-fromspace-protect] FAULT: signal 10 at 0x39f9556083a + This address is RETIRED FROM-SPACE. ... + block=0x39f95560000 +2106 retired_bytes=4200 retired_by_minor=#135 + last-known object: user_ptr=0x39f95560840 obj_type=2 size=56 +``` + +`obj_type=2` is `GC_TYPE_OBJECT`. The program dies in arm 3 — the descriptor +bag whose fields are ACCESSORS, so `desc_read_field` runs user JS inside +`js_object_define_property` — which is precisely the window #6949's scope note +defers. The instrument is live: 135 from-space page-sets were retired before the +fault. The same program on this branch exits 0. + +**Do not attribute a fault from the census line alone.** The first draft of this +probe faulted with a *different* signature (`obj_type=3`, `GC_TYPE_STRING`, at +`user_ptr + 4` — which is `StringHeader::byte_len`, so it really was a stale key +string) and it was tempting to read that as the defineProperty key. It was not; +see the next section. Only a symbolicated backtrace settled it. + +## A second defect the first draft of the probe walked into + +The probe originally compared each arm inline — +`console.log("x", observed() === expected() ? "ok" : "BAD")`. Under the witness +configuration that faults on a pristine build **and on this branch**, in +`js_jsvalue_equals` <- `js_eq` <- `main` (symbolicated against an unstripped +`perry-dev` runtime with `PERRY_DEBUG_SYMBOLS=1`). The left operand is an SSA +temporary live across `expected()`, which allocates through several loop +back-edges and therefore collects: the temporary names from-space. That is a +**codegen** root-dominance defect — the class +`scripts/gc_root_dominance_check.py` exists for — with nothing to do with +`Object.defineProperty`, and it is filed separately. + +Binding both sides to `const` first removes it from this program, and only then +does the A/B separate. Worth recording as method: the FIRST fault a witness +program produces is not necessarily the defect you are hunting, and the census +line (`obj_type`, `size`) is a hint, not an attribution. Symbolicate. + +## Sites fixed + +### 1. `object/object_ops/define_property.rs` — the ordinary-object arm + +`obj` (`*mut ObjectHeader`) and `key_str` (`*mut StringHeader`) were resolved +once near the top and then carried, raw, to the end of the function — through +`define_array_property`, `enforce_define_property_invariants`, +`obj_value_has_own_key`, `ensure_key_in_keys_array`, +`clone_closure_rebind_this`, `define_property_force_store_value`, and every +`desc_has_field` / `desc_read_field` (each allocates a field-name string, and on +an accessor-backed descriptor field runs USER JS). `obj_value`, +`descriptor_value` and `key_value` were rooted only across the initial +`js_string_coerce` and then read as plain locals for the rest of the body. + +The receiver is the worse half: `obj as usize` is the OWNER KEY of the +per-property descriptor side tables (`set_property_attrs`, +`set_accessor_descriptor`, `accessor_descriptors`), so a stale receiver files +the attributes and accessors under a dead address where the matching read can +never find them — a silent wrong answer, not a crash. + +Fixed by rooting all five and introducing an `across!` macro that is the only +way to name any of them across a call: it runs the call first and rebinds all +five from their roots afterwards, so a pre-collection address is never +nameable. No new bare `get_raw_*_ptr` sites — `RuntimeHandle::across_mut` is +what the `scripts/raw_handle_debt.py` ratchet asks for, and the file's count +went 3 → 2. + +Also rooted inside that arm: + +* the descriptor's `get` / `set` field values, which spanned + `ensure_key_in_keys_array` and the first of two `clone_closure_rebind_this` + calls; +* the existing accessor's `get` / `set` closure bits, which are written back + into the (GC-scanned) accessor table when the redefining descriptor omits a + field, and which spanned the same two allocating calls; +* the class-prototype mirror's method value, which spanned + `descriptor_enumerable` (two more descriptor field reads). + +The three inner `RuntimeHandleScope`s (closure arm, typed-array arm, ordinary +arm) were collapsed into ONE scope created before `try_decode_descriptor`. That +is deliberate: the scope has to outlive the `DescView` handles, and an inner +scope dropped while an outer one is still taking handles truncates the outer +container's newest entries (the hazard documented on `gc::RootedValues`). + +### 2. `object/object_ops/descriptor_helpers.rs` — `DescView` + +`DescView` held six raw `JSValue`s read at decode time and handed them back at a +dozen points spread over the rest of `js_object_define_property`. The stale word +was not merely read — it was **stored into the receiver** +(`define_property_force_store_value`) or into the accessor table. Each present +field is now a `RuntimeHandle`, so `read` returns the post-collection address; +absent fields hold no handle and read `undefined` as before. `DescView` gained a +`'scope` lifetime; `try_decode_descriptor` takes the scope. + +`validate_nonconfigurable_redefine`'s per-field arm (`desc_view == None`) also +allocated a field-name string per probe while holding `desc_ptr`, the current +value being compared, and the current accessor's closure bits. All three are now +rooted and re-read; `desc_ptr` is re-resolved *after* the allocation that +precedes each read. + +### 3. `object/reflect_support.rs` — `obj_value_has_own_key` + +The final keys-array walk held `keys` and `key_str` across +`crate::array::js_array_get`, which materializes a lazy array and therefore can +allocate. Both are rooted and re-read per iteration. The +`string_coerce_is_inert` shortcut around the scope was dropped: the walk needs +the same scope whatever the key's shape, so skipping it bought nothing. File's +raw-handle count went 4 → 3. + +## How the fix is proven + +`crates/perry-runtime/src/gc/tests/rooted_define_property.rs`, three tests, all +under `CopyingNurseryTestGuard` + `suppress_automatic_triggers`: + +1. `define_property_lands_on_the_receiver_a_descriptor_getter_moved` — the + end-to-end proof, through the real `#[no_mangle]` entry point. The descriptor + bag's `value` field is an ACCESSOR whose getter forces a copying minor (which + is what pushes `try_decode_descriptor` onto the spec-general path, so + `desc_read_field` runs user JS mid-define). It asserts, in order: + `copied_objects > 0`; the **receiver's address changed**; the **key string's + address changed**; then that the property reads back the getter's payload + bytes; then that `get_property_attrs` finds the entry **at the live + address**. The last assertion is the one that catches a stale receiver, since + the attribute table is keyed by address. +2. `desc_view_field_values_are_rooted` — `try_decode_descriptor`'s fast path, + the `DescView` half: decode, force a copying minor, assert the field's + address changed and it still reads the original bytes. +3. `unrooted_receiver_copy_still_names_from_space` — the sabotage arm. The same + address held in a plain Rust `usize` (exactly what pre-fix + `js_object_define_property` carried) keeps naming its pre-collection value in + the same cycle in which the rooted handle to the SAME object is rewritten. + This is what makes (1) and (2) non-vacuous. + +### Sabotage verification (fix committed first) + +See "Sabotage run" below. + +### Compiled probe — the A/B + +`test-files/test_gap_gc_define_property_descriptor_rooting.ts`, three arms: an +allocating `Object.groupBy` first arm (to retire from-space blocks), a +hand-written `Object.defineProperty` loop, and a loop whose descriptor bag +carries three allocating accessor getters (so `desc_read_field` runs user JS +mid-define). + +Both arms compiled with `PERRY_NO_AUTO_OPTIMIZE=1` and `PERRY_RUNTIME_DIR` +pinned to their own `.a` pair; the fixed pair's mtimes were confirmed to have +moved after the edit. + +| build | witness configuration | default | +|---|---|---| +| pristine `origin/main` (a769fafc6) | **exit 138**, `[gc-fromspace-protect] FAULT` at `block+2106`, `retired_by_minor=#135`, `obj_type=2` (a receiver `ObjectHeader`), stdout stops after arm 2 — i.e. it dies in the **descriptor-getter** arm | exit 0, byte-identical to node 26.5.1 | +| this branch | **exit 0**, `[gc-schedule] done: safepoints=301 scheduled_collections=301 copying_minors=301 moved_objects=110912 loop_polls=8175` | exit 0, byte-identical to node 26.5.1 | + +Instrument liveness is reported rather than assumed: the pristine arm retired +135 from-space page-sets before it faulted, and the fixed arm ran 301 copying +minors moving 110,912 objects. + +## Not covered by a moving test + +* The `obj_value_has_own_key` keys-walk fix (site 3) is by inspection: the + allocation there is a lazy-array materialization, which the unit harness has + no cheap way to force. Stated rather than glossed. +* An accessor-install test (a collection between the descriptor's `get` and + `set` reads) was written and dropped: it kept faulting inside the harness's + own setup rather than in the code under test, and a test that fights the + harness is not evidence. The window it targeted is covered end-to-end by the + compiled probe's third arm. + +## Acceptance corpus + +All 37 `test_gap_gc_*.ts` plus the 5 `test_gap_{proxy,reflect}*.ts` programs, +compiled against the fixed runtime (`PERRY_NO_AUTO_OPTIMIZE=1`, +`PERRY_RUNTIME_DIR` pinned), byte-compared to node 26.5.1 in the default +configuration AND under `PERRY_GC_PROTECT_FROMSPACE=1 +PERRY_GC_PROTECT_FROMSPACE_DEPTH=800`: + +``` +==== pass=42 fail=0 node-skip=0 quarantine-live=20 ==== +``` + +`quarantine-live=20` is the honest half: only 20 of the 42 programs ran a +copying minor at all, so for the other 22 the protected arm is a +no-regression check and not a rooting witness. (#7962 reported the same 20-of-41 +split.) + +## #7964 — verdict: COMPILER GAP, not a stale pin. Do not regenerate. + +Reproduced on this branch (the change is runtime-only, so this is the state of +`main`): `test-files/gc-dep-corpus/main.ts` fails to link with **45 distinct** +undefined `_perry_fn_node_modules_zod_...` symbols. + +Evidence for "compiler gap": + +1. **The mangled module in every failing symbol is the BARREL, not the + definer.** `_perry_fn_node_modules_zod_src_v4_core_index_ts__NEVER` says + Perry believes `NEVER` is *defined by* `core/index.ts`. `core/index.ts` is + 16 lines of `export * from …` / `export * as ns from …` and defines nothing. + The actual definition is `core/core.ts:13`. +2. **The consumer shape is a named re-export from an `export *` barrel.** + `v4/classic/external.ts` does + `export { globalRegistry, config, $brand, clone, prettifyError, … } from "../core/index.js";` + — every one of those names reaches `core/index.ts` only through + `export * from "./core.js" | "./api.js" | "./registries.js" | "./errors.js"`. + Perry emits the reference and never the forwarding definition. +3. **It is not a type-only-export leak.** The failing set mixes `const` exports + (`NEVER`, `globalRegistry`, `$brand`) with `function` exports (`config`, + `clone`, `prettifyError`, `_gt`, `_minLength`, …), so "erased type export + still got a symbol" does not explain it. +4. **The pin has not drifted.** `package.json` asks for `zod@^4.3.5` and + `node_modules/zod/package.json` is `4.3.5` — exactly the version named in + #7964. Nothing in the corpus pins a commit that could have moved underneath + it. + +Two minimal reproducers I built (in `/tmp`, deliberately not added to the repo — +see the collision note below) both LINK, so the gap needs more of zod's shape +than one hop: `leaf.ts` → `barrel.ts` (`export *`) → `top.ts` links, and adding +a `bridge.ts` (`export { X } from "./barrel.js"`) still links. The remaining +candidates are the barrel's multi-source `export *` set, its +`export * as ns from`, and the `core/index.ts` ↔ `core/api.ts` cycle. + +**Collision:** `/Users/amlug/projects/perry/wt-codex-7964` is another agent's +worktree on branch `fix/7964-zod-star-reexports`, already carrying uncommitted +edits to `perry-hir/src/lower/module_decl.rs`, `perry-hir/src/dynamic_import.rs` +and `perry-codegen/src/codegen/helpers.rs`, plus fixtures +`test-files/test_gap_export_star_variable_reexport.ts` and +`test-files/_helpers/issue_7964_{leaf,barrel,bridge,top}.ts` — the same four-file +shape I arrived at independently. I stopped at the verdict rather than shipping a +second implementation of the same fix. + +## #7803 — still blocked + +The corpus does not link, so #7803's reproducer still cannot be run. Nothing on +this branch changes that (the fix is runtime-side; the failure is in module +lowering/codegen). The candidate connection stands and is now slightly stronger: +zod's `Object.defineProperties` calls +(`src/v4/core/util.ts:316`, `src/v4/classic/errors.ts:28`) go through the helper +#7949 fixed, and its `Object.defineProperty` calls go through the window this +branch fixes; #7803's `Cannot read properties of undefined (reading 'toString')` +is what a stale key or a stale receiver in either loop produces. **Candidate, +not confirmed** — retest once #7964 lands. diff --git a/test-files/test_gap_gc_define_property_descriptor_rooting.ts b/test-files/test_gap_gc_define_property_descriptor_rooting.ts new file mode 100644 index 0000000000..d7ba7d100c --- /dev/null +++ b/test-files/test_gap_gc_define_property_descriptor_rooting.ts @@ -0,0 +1,200 @@ +// #7963: `Object.defineProperty` itself — the wider window #6949's scope note +// names and defers, and the one #7949 (`Object.defineProperties`) deliberately +// left open. +// +// `js_object_define_property` resolves the receiver's `ObjectHeader` (`obj`) +// and coerces the key to a `StringHeader` (`key_str`) ONCE, near the top, and +// then keeps both as bare Rust locals through the whole rest of the function: +// the descriptor-field reads (`desc_has_field` / `desc_read_field`, each of +// which allocates the field-name string and can run a USER GETTER on the +// descriptor bag), `enforce_define_property_invariants`, +// `ensure_key_in_keys_array` (which grows the keys array), and +// `define_property_force_store_value`. A raw `*mut ObjectHeader` / `*const +// StringHeader` in a Rust local is neither a shadow slot nor a temp root nor +// reachable from any registered scanner, so an evacuating minor landing in any +// of those calls can neither keep them alive nor rewrite them. +// +// The stale receiver address is also the OWNER KEY of the per-property +// descriptor side tables (`set_property_attrs` / `set_accessor_descriptor` / +// `accessor_descriptors`), so a stale `obj` files the attributes under a dead +// address where the matching read can never find them — a silent wrong answer +// rather than a crash. +// +// Why this needs a hand-built probe: `scripts/gc_root_dominance_check.py` reads +// emitted LLVM IR, and a Rust-side local is structurally invisible to it. +// +// LIVE BY CONSTRUCTION, in two ways: +// * the FIRST arm (`objectGroupBy`) allocates hard enough to fill and retire +// from-space blocks, so the second arm's stale reads land in RETIRED bytes +// the quarantine can name rather than in bytes nobody has reused yet; +// * every descriptor getter runs `churn`, which has a loop back-edge (a GC +// safepoint poll is emitted only in user JS) and keeps allocating after it. +// +// Witness configuration: +// +// PERRY_GC_SCHEDULE_SEED=1 PERRY_GC_SCHEDULE_RATE=1 \ +// PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800 +// +// Before the fix this exits 138 with `[gc-fromspace-protect] FAULT` naming a +// retired from-space address during arm 3 (`obj_type=2`, a receiver +// `ObjectHeader`); after it the program exits 0 with 301 copying minors and +// ~110k objects moved, byte-identical to node in every configuration. + +function churn(n: number): number { + const bits: any[] = []; + for (let i = 0; i < 120; i++) { + bits.push({ i: i, s: "y" + i, pad: [i, i + 1, i + 2] }); + } + return bits.length === 120 ? n : -1; +} + +function items(count: number): string[] { + const out: string[] = []; + for (let i = 0; i < count; i++) { + out.push("item-" + i); + } + return out; +} + +// Arm 1 — allocate hard through a user callback so from-space blocks are +// retired before arm 2 runs. (This is `test_gap_gc_container_value_rooting`'s +// `objectGroupBy`; #7949 fixed the container, so this arm is expected to be +// correct on its own. It is here for the allocation profile.) +function objectGroupBy(): string { + const grouped = Object.groupBy(items(18), (s: string, i: number): string => { + churn(i); + return "bucket-" + (i % 3); + }); + const parts: string[] = []; + for (const key of Object.keys(grouped).sort()) { + parts.push(key + "=" + (grouped as any)[key].join(",")); + } + return parts.join("|"); +} + +function expectedObjectGroupBy(): string { + const buckets: string[][] = [[], [], []]; + for (let i = 0; i < 18; i++) { + buckets[i % 3].push("item-" + i); + } + const parts: string[] = []; + for (let b = 0; b < 3; b++) { + parts.push("bucket-" + b + "=" + buckets[b].join(",")); + } + return parts.join("|"); +} + +// Arm 2 — a HAND-WRITTEN `Object.defineProperty` loop. `Object.defineProperties` +// (the #7949 helper) is deliberately NOT on this path: the descriptor bag is +// walked here, in JS, and each descriptor is installed one at a time. What is +// left is `js_object_define_property`'s own window. +function definePropertyOneAtATime(): string { + const bag: any = {}; + for (let i = 0; i < 12; i++) { + const index = i; + Object.defineProperty(bag, "prop-" + index, { + enumerable: true, + configurable: true, + get: function (): any { + churn(index); + return { value: "value-" + index, enumerable: true, configurable: true }; + }, + }); + } + + const target: any = {}; + for (const key of Object.keys(bag)) { + Object.defineProperty(target, key, bag[key]); + } + + const parts: string[] = []; + for (const key of Object.keys(target).sort()) { + parts.push(key + "=" + target[key]); + } + return parts.join("|"); +} + +// Arm 3 — the descriptor bag itself carries ACCESSOR fields whose getters +// allocate, so the descriptor read inside `js_object_define_property` runs user +// JS between the key coercion and the key's use. This is the shape the #6949 +// scope note calls out directly ("holds `obj` / `descriptor_value` and the six +// raw `JSValue`s inside `DescView` across its own later `js_string_from_bytes` +// calls"). +function definePropertyWithAllocatingDescriptorGetters(): string { + const target: any = {}; + for (let i = 0; i < 12; i++) { + const index = i; + const descriptor: any = {}; + Object.defineProperty(descriptor, "value", { + enumerable: true, + get: function (): string { + churn(index); + return "v" + index; + }, + }); + Object.defineProperty(descriptor, "enumerable", { + enumerable: true, + get: function (): boolean { + churn(index); + return true; + }, + }); + Object.defineProperty(descriptor, "configurable", { + enumerable: true, + get: function (): boolean { + churn(index); + return true; + }, + }); + Object.defineProperty(target, "key-" + index, descriptor); + } + const parts: string[] = []; + for (const key of Object.keys(target).sort()) { + parts.push(key + "=" + target[key]); + } + return parts.join("|"); +} + +function expectedIndexed(prefix: string, valuePrefix: string, count: number): string { + const keys: string[] = []; + for (let i = 0; i < count; i++) { + keys.push(prefix + i); + } + // Sort the KEYS, exactly like `Object.keys(target).sort()` does — sorting the + // joined "key=value" strings orders "prop-10=" before "prop-1=". + keys.sort(); + const parts: string[] = []; + for (const key of keys) { + parts.push(key + "=" + valuePrefix + key.substring(prefix.length)); + } + return parts.join("|"); +} + +// NOTE — why each side is bound to a `const` instead of being compared inline. +// +// `console.log("x", f() === g() ? …)` leaves `f()`'s result as an SSA temporary +// that is live across `g()`. Under this witness configuration `g()` allocates +// through several loop back-edges, so it collects, and the temporary names +// from-space: the run faults inside `js_jsvalue_equals` (frame `js_eq` <- `main`) +// on BOTH a pristine build and this branch. That is a SEPARATE, pre-existing +// codegen root-dominance defect — the class +// `scripts/gc_root_dominance_check.py` exists for — and it has nothing to do +// with `Object.defineProperty`. Binding both sides first keeps this program a +// witness for ONE defect. See the issue filed alongside #7963. +const groupByObserved = objectGroupBy(); +const groupByExpected = expectedObjectGroupBy(); +console.log("objectGroupBy", groupByObserved === groupByExpected ? "ok" : "BAD"); + +const oneAtATimeObserved = definePropertyOneAtATime(); +const oneAtATimeExpected = expectedIndexed("prop-", "value-", 12); +console.log( + "definePropertyOneAtATime", + oneAtATimeObserved === oneAtATimeExpected ? "ok" : "BAD", +); + +const accessorObserved = definePropertyWithAllocatingDescriptorGetters(); +const accessorExpected = expectedIndexed("key-", "v", 12); +console.log( + "definePropertyAccessorDescriptor", + accessorObserved === accessorExpected ? "ok" : "BAD", +);