diff --git a/changelog.d/10582-define-property-accessor-attrs.md b/changelog.d/10582-define-property-accessor-attrs.md new file mode 100644 index 0000000000..342b1b3f23 --- /dev/null +++ b/changelog.d/10582-define-property-accessor-attrs.md @@ -0,0 +1,19 @@ +### Fixed + +- **A generic `Object.defineProperty`/`defineProperties` descriptor (no + `get`/`set`/`value`/`writable`, e.g. `{ enumerable: true }`) against an + existing **class-declared** get/set accessor no longer breaks it.** (#10480) + A ClassBody accessor lives in the class vtable, not the address-keyed + descriptor tables `defineProperty` normally writes, so the generic-descriptor + branch could not see the class key: it appended a shadowing data property + with `writable: false`, which silenced the setter (assignment threw in + strict code, silently dropped in sloppy code) and never actually applied the + requested `enumerable`/`configurable` change. Every WebIDL-generated class + (whatwg-url, node-fetch, undici-style polyfills) marks its prototype + accessors enumerable exactly this way at module load — node-fetch's + `Object.defineProperties(Request.prototype, { method: { enumerable: true }, + … })` broke every later write to those accessors. A new per-accessor + side table (`class_registry/accessor_attrs.rs`) now records the overridden + attributes instead, so `getOwnPropertyDescriptor`, `Object.keys`/`values`/ + `entries`, `hasOwnProperty`/`propertyIsEnumerable`, and `delete` all see the + update while the getter/setter stay exactly where they already lived. diff --git a/crates/perry-runtime/src/object/class_registry.rs b/crates/perry-runtime/src/object/class_registry.rs index 6afe9c9bf7..db5f82e9b9 100644 --- a/crates/perry-runtime/src/object/class_registry.rs +++ b/crates/perry-runtime/src/object/class_registry.rs @@ -42,6 +42,7 @@ pub use super::class_handles::{ }; use super::*; +mod accessor_attrs; mod builtin_alias_construct; mod class_meta; mod construct; @@ -63,6 +64,14 @@ mod registration; mod state; mod vm_brand; +// ── accessor_attrs.rs ─────────────────────────────────────────────────────── +pub(crate) use accessor_attrs::{ + class_accessor_attrs, class_accessor_attrs_in_use, class_accessor_descriptor, + class_declared_accessor_ptrs, class_enumerable_accessor_names, + class_prototype_enumerable_accessor, class_set_accessor_attrs, + decl_prototype_enumerable_key_snapshot, decl_prototype_keys_with_enumerable_accessors, +}; + // ── state.rs ──────────────────────────────────────────────────────────────── #[cfg(test)] pub(crate) use state::class_decl_prototype_object_root_store; diff --git a/crates/perry-runtime/src/object/class_registry/accessor_attrs.rs b/crates/perry-runtime/src/object/class_registry/accessor_attrs.rs new file mode 100644 index 0000000000..89c970d026 --- /dev/null +++ b/crates/perry-runtime/src/object/class_registry/accessor_attrs.rs @@ -0,0 +1,327 @@ +//! Reflective attributes of DECLARED class accessors (#10480). +//! +//! A ClassBody `get x() {}` / `set x(v) {}` lives in the class vtable +//! (`CLASS_VTABLE_REGISTRY`, or `CLASS_STATIC_ACCESSORS` for `static`), not in +//! the address-keyed descriptor tables `Object.defineProperty` writes. The +//! vtable records only the two function pointers, so the accessor's +//! `[[Enumerable]]` / `[[Configurable]]` were pinned to the ClassBody defaults +//! (`false` / `true`) and nothing could change them. +//! +//! `Object.defineProperties(C.prototype, { x: { enumerable: true } })` is how +//! every WebIDL-generated class (whatwg-url, node-fetch, undici-style +//! polyfills) marks its accessors at module load. That generic descriptor fell +//! through to the ordinary define path, which could not see the class key: it +//! appended a keys-array placeholder with a NEW property's `writable: false`, +//! and that data property on the prototype then rejected every instance write +//! before the class setter could run — while the requested enumerability was +//! never reported. +//! +//! This table holds what a generic descriptor applied, keyed by +//! `(class_id, is_static, name)`. Absence means the ClassBody defaults, so the +//! table stays empty in a program that never redefines a class accessor, and +//! [`class_accessor_attrs_in_use`] lets the enumeration paths skip the lookup +//! with one load. The values are booleans: nothing here is a GC root. + +use super::*; +use std::collections::HashMap; +use std::sync::atomic::{AtomicBool, Ordering}; + +crate::perry_thread_local! { + static CLASS_ACCESSOR_ATTRS: std::cell::RefCell> = + std::cell::RefCell::new(HashMap::new()); +} + +/// Sticky: set by the first [`class_set_accessor_attrs`]. Only a hint that +/// the table may be non-empty — never cleared, so a stale `true` merely costs +/// a lookup. +static CLASS_ACCESSOR_ATTRS_IN_USE: AtomicBool = AtomicBool::new(false); + +/// ClassBody defaults for an accessor: `(enumerable, configurable)`. +const CLASS_ACCESSOR_DEFAULT_ATTRS: (bool, bool) = (false, true); + +#[inline] +pub(crate) fn class_accessor_attrs_in_use() -> bool { + CLASS_ACCESSOR_ATTRS_IN_USE.load(Ordering::Relaxed) +} + +/// `(enumerable, configurable)` of the declared accessor `name`. +pub(crate) fn class_accessor_attrs(class_id: u32, is_static: bool, name: &str) -> (bool, bool) { + if !class_accessor_attrs_in_use() { + return CLASS_ACCESSOR_DEFAULT_ATTRS; + } + CLASS_ACCESSOR_ATTRS.with(|table| { + table + .borrow() + .get(&(class_id, is_static, name.to_string())) + .copied() + .unwrap_or(CLASS_ACCESSOR_DEFAULT_ATTRS) + }) +} + +pub(crate) fn class_set_accessor_attrs( + class_id: u32, + is_static: bool, + name: &str, + enumerable: bool, + configurable: bool, +) { + CLASS_ACCESSOR_ATTRS_IN_USE.store(true, Ordering::Relaxed); + CLASS_ACCESSOR_ATTRS.with(|table| { + table.borrow_mut().insert( + (class_id, is_static, name.to_string()), + (enumerable, configurable), + ); + }); +} + +/// Raw `(getter, setter)` func_ptrs of a live own declared accessor — `None` +/// for a method, a field, an inherited accessor, or one `delete` removed. +pub(crate) fn class_declared_accessor_ptrs( + class_id: u32, + is_static: bool, + name: &str, +) -> Option<(usize, usize)> { + if class_is_key_deleted(class_id, name) { + return None; + } + if is_static { + class_own_static_accessor_ptrs(class_id, name) + } else { + class_own_accessor_ptrs(class_id, name) + } +} + +/// `Object.getOwnPropertyDescriptor` for a declared accessor. The getter value +/// is rooted across the setter value's allocation. +pub(crate) unsafe fn class_accessor_descriptor( + class_id: u32, + is_static: bool, + name: &str, + getter: usize, + setter: usize, +) -> f64 { + let scope = crate::gc::RuntimeHandleScope::new(); + let get = scope.root_nanbox_f64(class_accessor_function_value(getter, false, name)); + let set = class_accessor_function_value(setter, true, name); + let (enumerable, configurable) = class_accessor_attrs(class_id, is_static, name); + crate::object::descriptors::build_accessor_descriptor( + get.get_nanbox_f64(), + set, + enumerable, + configurable, + ) +} + +/// The class's own declared accessors that are currently enumerable, in +/// ClassBody order. Empty (without walking the class) unless some accessor of +/// this class was made enumerable. +pub(crate) fn class_enumerable_accessor_names(class_id: u32, is_static: bool) -> Vec { + if !class_accessor_attrs_in_use() { + return Vec::new(); + } + let any = CLASS_ACCESSOR_ATTRS.with(|table| { + table + .borrow() + .iter() + .any(|(&(cid, st, _), &(enumerable, _))| { + cid == class_id && st == is_static && enumerable + }) + }); + if !any { + return Vec::new(); + } + class_own_string_member_names(class_id, is_static) + .into_iter() + .filter(|name| { + class_declared_accessor_ptrs(class_id, is_static, name).is_some() + && class_accessor_attrs(class_id, is_static, name).0 + }) + .collect() +} + +/// `Object.keys(C.prototype)` when `physical` is the enumerable key list of the +/// declared-class prototype object of `class_id`: splice in the enumerable +/// declared accessors (which have no physical slot) in [[OwnPropertyKeys]] +/// order — `constructor`, then ClassBody members, then keys added later. +/// Returns `physical` itself when the class has no enumerable accessor. +pub(crate) unsafe fn decl_prototype_keys_with_enumerable_accessors( + class_id: u32, + physical: *mut crate::array::ArrayHeader, +) -> *mut crate::array::ArrayHeader { + let accessors = class_enumerable_accessor_names(class_id, false); + if accessors.is_empty() || physical.is_null() { + return physical; + } + // Copy the physical names out before the first allocation below moves + // anything; `physical` is not read again. + let mut physical_names = Vec::new(); + let mut scratch = [0u8; crate::value::SHORT_STRING_MAX_LEN]; + for i in 0..crate::array::js_array_length(physical) { + let key = crate::array::js_array_get(physical, i); + if let Some(bytes) = crate::string::js_string_key_bytes(key, &mut scratch) { + if let Ok(name) = std::str::from_utf8(bytes) { + physical_names.push(name.to_string()); + } + } + } + let mut names: Vec = Vec::new(); + let mut push = |name: &str| { + if !names.iter().any(|existing| existing == name) { + names.push(name.to_string()); + } + }; + if physical_names.iter().any(|name| name == "constructor") { + push("constructor"); + } + for name in class_own_string_member_names(class_id, false) { + if accessors.contains(&name) || physical_names.contains(&name) { + push(&name); + } + } + for name in &physical_names { + push(name); + } + crate::object::descriptors::sort_property_names_ecma(&mut names); + let scope = crate::gc::RuntimeHandleScope::new(); + let out = scope.root_raw_mut_ptr(crate::array::js_array_alloc(names.len() as u32)); + for name in names { + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + let updated = out.with_mut_ptr(|out| { + crate::array::js_array_push(out, crate::value::JSValue::string_ptr(key)) + }); + out.set_raw_mut_ptr(updated); + } + out.with_mut_ptr(|out: *mut crate::array::ArrayHeader| out) +} + +/// True when `name` is an enumerable declared accessor of the declared-class +/// prototype at `obj_addr` — the enumeration paths' test for a key with no +/// physical slot. Cheap (one atomic load) until some accessor is redefined. +pub(crate) fn class_prototype_enumerable_accessor(obj_addr: usize, name: &str) -> bool { + if !class_accessor_attrs_in_use() { + return false; + } + let Some(class_id) = class_id_for_decl_prototype_object(obj_addr) else { + return false; + }; + class_declared_accessor_ptrs(class_id, false, name).is_some() + && class_accessor_attrs(class_id, false, name).0 +} + +/// `Object.values` / `Object.entries` own-key snapshot for a declared-class +/// prototype with enumerable ClassBody accessors — those keys have no slot in +/// the physical keys array the ordinary snapshot walks. `None` for every other +/// receiver, which keeps the existing walk. +/// +/// The list is `Object.keys`', so (unlike the ordinary snapshot) enumerability +/// is settled here rather than re-read per key: a getter that flips a SIBLING +/// accessor's enumerability mid-enumeration is not modelled. Nothing else +/// changes — the per-key `[[Get]]` and its side effects are unaffected. +pub(crate) unsafe fn decl_prototype_enumerable_key_snapshot( + obj: *const ObjectHeader, +) -> Option>> { + if !class_accessor_attrs_in_use() { + return None; + } + let class_id = class_id_for_decl_prototype_object(obj as usize)?; + if class_enumerable_accessor_names(class_id, false).is_empty() { + return None; + } + let keys = crate::object::field_get_set::enumeration::js_object_keys(obj); + if keys.is_null() { + return None; + } + let mut snapshot = Vec::new(); + let mut scratch = [0u8; crate::value::SHORT_STRING_MAX_LEN]; + for i in 0..crate::array::js_array_length(keys) { + let key = crate::array::js_array_get(keys, i); + if let Some(bytes) = crate::string::js_string_key_bytes(key, &mut scratch) { + snapshot.push(bytes.to_vec()); + } + } + Some(snapshot) +} + +#[cfg(test)] +mod tests { + use super::*; + + extern "C" fn getter(_this: f64) -> f64 { + 0.0 + } + + extern "C" fn setter(_this: f64, _value: f64) -> f64 { + 0.0 + } + + unsafe fn register(class_id: u32, name: &str, with_setter: bool, order: i64) { + js_register_class_getter( + class_id as i64, + name.as_ptr(), + name.len() as i64, + getter as *const () as usize as i64, + ); + if with_setter { + js_register_class_setter( + class_id as i64, + name.as_ptr(), + name.len() as i64, + setter as *const () as usize as i64, + ); + } + js_register_class_string_member_order( + class_id as i64, + name.as_ptr(), + name.len() as i64, + 0, + order, + ); + } + + #[test] + fn unrecorded_accessor_keeps_classbody_defaults() { + assert_eq!( + class_accessor_attrs(0x7c48_0001, false, "never"), + (false, true) + ); + } + + #[test] + fn attrs_are_keyed_by_static_side_and_name() { + let cid = 0x7c48_0002; + class_set_accessor_attrs(cid, false, "x", true, false); + assert!(class_accessor_attrs_in_use()); + assert_eq!(class_accessor_attrs(cid, false, "x"), (true, false)); + assert_eq!(class_accessor_attrs(cid, true, "x"), (false, true)); + assert_eq!(class_accessor_attrs(cid, false, "y"), (false, true)); + } + + /// Only live declared accessors qualify: a deleted one, a name the class + /// never declared, and a non-enumerable one are all excluded, and the + /// survivors come back in ClassBody order rather than insertion order. + #[test] + fn enumerable_accessor_names_follow_classbody_order() { + let cid = 0x7c48_0003; + unsafe { + register(cid, "b", true, 10); + register(cid, "a", false, 20); + register(cid, "c", true, 30); + register(cid, "gone", true, 40); + } + class_set_accessor_attrs(cid, false, "a", true, true); + class_set_accessor_attrs(cid, false, "b", true, true); + class_set_accessor_attrs(cid, false, "c", false, true); + class_set_accessor_attrs(cid, false, "gone", true, true); + class_set_accessor_attrs(cid, false, "undeclared", true, true); + class_mark_key_deleted(cid, "gone"); + assert_eq!( + class_enumerable_accessor_names(cid, false), + vec!["b".to_string(), "a".to_string()] + ); + assert!(class_enumerable_accessor_names(cid, true).is_empty()); + assert_eq!(class_declared_accessor_ptrs(cid, false, "gone"), None); + assert!( + class_declared_accessor_ptrs(cid, false, "a").is_some_and(|(g, s)| g != 0 && s == 0) + ); + } +} diff --git a/crates/perry-runtime/src/object/delete_rest.rs b/crates/perry-runtime/src/object/delete_rest.rs index a9c0fb2e96..f22daa3667 100644 --- a/crates/perry-runtime/src/object/delete_rest.rs +++ b/crates/perry-runtime/src/object/delete_rest.rs @@ -70,6 +70,12 @@ pub extern "C" fn js_object_delete_field( if let Some(name) = super::has_own_helpers::str_from_string_header(key) { let class_id = obj as usize as u32; if super::class_registry::class_name_for_id(class_id).is_some() { + if super::class_registry::class_declared_accessor_ptrs(class_id, true, name) + .is_some() + && !super::class_registry::class_accessor_attrs(class_id, true, name).1 + { + return 0; + } super::class_registry::class_delete_own_dynamic_prop(class_id, name); super::class_registry::class_mark_key_deleted(class_id, name); super::class_registry::invalidate_class_string_member_order( @@ -294,6 +300,13 @@ pub extern "C" fn js_object_delete_field( super::class_registry::class_id_for_decl_prototype_object(obj as usize) { if let Some(name) = super::has_own_helpers::str_from_string_header(key) { + // #10480: a ClassBody accessor redefined non-configurable. + if super::class_registry::class_declared_accessor_ptrs(cid, false, name) + .is_some() + && !super::class_registry::class_accessor_attrs(cid, false, name).1 + { + return 0; + } if name != "constructor" && (super::class_registry::class_own_accessor_ptrs(cid, name).is_some() || super::native_module::class_has_own_method(cid, name) @@ -725,6 +738,11 @@ fn delete_receiver_is_pointer(obj_value: f64) -> bool { } fn delete_class_prototype_key(class_id: u32, name: &str) -> i32 { + if super::class_registry::class_declared_accessor_ptrs(class_id, false, name).is_some() + && !super::class_registry::class_accessor_attrs(class_id, false, name).1 + { + return 0; + } let has_own = name == "constructor" || super::native_module::class_has_own_method(class_id, name) || super::class_registry::class_own_accessor_ptrs(class_id, name).is_some() diff --git a/crates/perry-runtime/src/object/descriptors.rs b/crates/perry-runtime/src/object/descriptors.rs index 11b61e7f15..cef07e07f0 100644 --- a/crates/perry-runtime/src/object/descriptors.rs +++ b/crates/perry-runtime/src/object/descriptors.rs @@ -521,15 +521,13 @@ pub extern "C" fn js_object_get_own_property_descriptor(obj_value: f64, key_valu super::class_registry::class_own_static_accessor_ptrs(class_id, &method_name) }; if let Some((g, s)) = accessor { - return build_accessor_descriptor( - super::class_registry::class_accessor_function_value( - g, - false, - &method_name, - ), - super::class_registry::class_accessor_function_value(s, true, &method_name), - false, - true, + let is_static = super::class_prototype_ref_id(obj_value).is_none(); + return super::class_registry::class_accessor_descriptor( + class_id, + is_static, + &method_name, + g, + s, ); } if super::class_prototype_ref_id(obj_value).is_some() @@ -946,11 +944,8 @@ pub extern "C" fn js_object_get_own_property_descriptor(obj_value: f64, key_valu } else if let Some((g, s)) = super::class_registry::class_own_accessor_ptrs(cid, name) { - return build_accessor_descriptor( - super::class_registry::class_accessor_function_value(g, false, name), - super::class_registry::class_accessor_function_value(s, true, name), - false, - true, + return super::class_registry::class_accessor_descriptor( + cid, false, name, g, s, ); } } diff --git a/crates/perry-runtime/src/object/field_get_set/entries_shape.rs b/crates/perry-runtime/src/object/field_get_set/entries_shape.rs index 2e373ef769..6ae7c6a79d 100644 --- a/crates/perry-runtime/src/object/field_get_set/entries_shape.rs +++ b/crates/perry-runtime/src/object/field_get_set/entries_shape.rs @@ -166,6 +166,15 @@ pub(super) fn js_object_entries_shape(obj: *const ObjectHeader) -> *mut ArrayHea // that. Enumerability is likewise re-evaluated per key in the read phase // (an earlier getter can create a descriptor or flip a future key's // enumerability), so we deliberately do NOT filter it during the snapshot. + // #10480: a declared-class prototype's enumerable ClassBody accessors + // have no physical key, so the keys-array walk cannot see them. The + // probe runs `Object.keys`, which allocates, so the receiver is rooted + // across it and re-read. + let accessor_scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = accessor_scope.root_raw_const_ptr(obj); + let (class_accessor_keys, obj) = obj_handle.across_const::(|| { + super::super::class_registry::decl_prototype_enumerable_key_snapshot(obj) + }); let mut snapshot_keys: Vec> = Vec::with_capacity(count); let mut key_buf = [0u8; crate::value::SHORT_STRING_MAX_LEN]; for j in 0..count { @@ -181,6 +190,9 @@ pub(super) fn js_object_entries_shape(obj: *const ObjectHeader) -> *mut ArrayHea snapshot_keys.push(bytes.to_vec()); } } + if let Some(merged) = class_accessor_keys { + snapshot_keys = merged; + } for key_bytes in snapshot_keys { let key_str = @@ -194,11 +206,19 @@ pub(super) fn js_object_entries_shape(obj: *const ObjectHeader) -> *mut ArrayHea // hidden a key that was in the initial snapshot (test262 // entries/getter-removing-future-key, getter-making-future-key- // nonenumerable). - if !super::super::own_key_present(obj as *mut ObjectHeader, key_str) { - continue; - } - if descriptor_marks_non_enumerable(obj, JSValue::string_ptr(key_str)) { - continue; + let class_accessor = std::str::from_utf8(&key_bytes).is_ok_and(|name| { + super::super::class_registry::class_prototype_enumerable_accessor( + obj as usize, + name, + ) + }); + if !class_accessor { + if !super::super::own_key_present(obj as *mut ObjectHeader, key_str) { + continue; + } + if descriptor_marks_non_enumerable(obj, JSValue::string_ptr(key_str)) { + continue; + } } // Create a pair array [key, value]. let pair = crate::array::js_array_alloc(2); diff --git a/crates/perry-runtime/src/object/field_get_set/enumeration.rs b/crates/perry-runtime/src/object/field_get_set/enumeration.rs index 610c165113..246fc28b84 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -169,8 +169,11 @@ pub extern "C" fn js_object_keys_value(value: f64) -> *mut ArrayHeader { // `Object.keys(C)` / `for (k in C)` (test262 class/elements static-field-*). if let Some(class_id) = super::super::class_ref_id(value) { if super::super::class_prototype_ref_id(value).is_none() { + // Static accessors are defined before static fields, so an + // enumerable one (#10480) precedes them. let mut names = - super::super::class_registry::class_own_enumerable_field_names(class_id); + super::super::class_registry::class_enumerable_accessor_names(class_id, true); + names.extend(super::super::class_registry::class_own_enumerable_field_names(class_id)); super::super::descriptors::sort_property_names_ecma(&mut names); let arr = crate::array::js_array_alloc(names.len().max(1) as u32); let mut out = arr; @@ -1286,7 +1289,22 @@ pub extern "C" fn js_object_keys(obj: *const ObjectHeader) -> *mut ArrayHeader { } return shape_keys; } - js_object_keys_shape(obj) + // #10480: a declared-class prototype's enumerable ClassBody accessors have + // no physical key. Resolve the class before the walk can move `obj`. + let decl_class = super::super::class_registry::class_accessor_attrs_in_use() + .then(|| { + super::super::class_registry::class_id_for_decl_prototype_object(strip_nanbox_addr(obj)) + }) + .flatten(); + let keys = js_object_keys_shape(obj); + match decl_class { + Some(class_id) => unsafe { + super::super::class_registry::decl_prototype_keys_with_enumerable_accessors( + class_id, keys, + ) + }, + None => keys, + } } /// [`js_object_keys`] over the shape alone. @@ -1841,6 +1859,15 @@ fn js_object_values_shape(obj: *const ObjectHeader) -> *mut ArrayHeader { // read time, not cached up front: an earlier getter can create a // descriptor or flip a future key's enumerability, so we defer the // `descriptor_marks_non_enumerable` check to the read phase. + // #10480: a declared-class prototype's enumerable ClassBody accessors + // have no physical key, so the keys-array walk cannot see them. The + // probe runs `Object.keys`, which allocates, so the receiver is rooted + // across it and re-read. + let accessor_scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = accessor_scope.root_raw_const_ptr(obj); + let (class_accessor_keys, obj) = obj_handle.across_const::(|| { + super::super::class_registry::decl_prototype_enumerable_key_snapshot(obj) + }); let mut snapshot_keys: Vec> = Vec::with_capacity(count); let mut key_buf = [0u8; crate::value::SHORT_STRING_MAX_LEN]; for j in 0..count { @@ -1856,6 +1883,9 @@ fn js_object_values_shape(obj: *const ObjectHeader) -> *mut ArrayHeader { snapshot_keys.push(bytes.to_vec()); } } + if let Some(merged) = class_accessor_keys { + snapshot_keys = merged; + } for key_bytes in snapshot_keys { let key_str = crate::string::js_string_from_bytes(key_bytes.as_ptr(), key_bytes.len() as u32); @@ -1865,11 +1895,19 @@ fn js_object_values_shape(obj: *const ObjectHeader) -> *mut ArrayHeader { // Re-check own + enumerable at read time (a prior getter may have // removed/hidden the key, or created a descriptor) — see // `js_object_entries`. - if !super::super::own_key_present(obj as *mut ObjectHeader, key_str) { - continue; - } - if descriptor_marks_non_enumerable(obj, JSValue::string_ptr(key_str)) { - continue; + let class_accessor = std::str::from_utf8(&key_bytes).is_ok_and(|name| { + super::super::class_registry::class_prototype_enumerable_accessor( + obj as usize, + name, + ) + }); + if !class_accessor { + if !super::super::own_key_present(obj as *mut ObjectHeader, key_str) { + continue; + } + if descriptor_marks_non_enumerable(obj, JSValue::string_ptr(key_str)) { + continue; + } } let value = js_object_get_field_by_name(obj as *const ObjectHeader, key_str); crate::array::js_array_push_f64(result, f64::from_bits(value.bits())); diff --git a/crates/perry-runtime/src/object/object_ops.rs b/crates/perry-runtime/src/object/object_ops.rs index fd522d72da..f68017e7c2 100644 --- a/crates/perry-runtime/src/object/object_ops.rs +++ b/crates/perry-runtime/src/object/object_ops.rs @@ -10,6 +10,7 @@ use super::*; mod accessors; +mod define_class_accessor; mod define_get_accessor; mod define_properties; mod define_property; diff --git a/crates/perry-runtime/src/object/object_ops/define_class_accessor.rs b/crates/perry-runtime/src/object/object_ops/define_class_accessor.rs new file mode 100644 index 0000000000..d2ca5e16ef --- /dev/null +++ b/crates/perry-runtime/src/object/object_ops/define_class_accessor.rs @@ -0,0 +1,95 @@ +//! `Object.defineProperty` onto a DECLARED class accessor (#10480). +//! +//! A ClassBody accessor is an own property of `C.prototype` (or of `C` for +//! `static`) whose get/set live in the class vtable, so the ordinary define +//! path — which only consults the address-keyed descriptor tables — took it for +//! a brand-new key. See `class_registry/accessor_attrs.rs` for what that broke. +use super::*; + +/// ValidateAndApplyPropertyDescriptor for the declared accessor `name` of +/// `class_id` (`is_static` selects `C` over `C.prototype`). +/// +/// * Not a live declared accessor → `false`, the caller's path decides. +/// * Current accessor non-configurable → the spec's rejections throw +/// `Cannot redefine property: ` exactly as for any other property. +/// * Generic descriptor (none of `get`/`set`/`value`/`writable`) → only the +/// attributes change: an omitted field keeps its current value and the +/// getter/setter stay in place. Returns `true`. +/// * Anything else → `false`: replacing a vtable accessor half or converting it +/// to a data property is not modelled here (compiled receivers call the +/// declared get/set directly), so the caller's existing path is unchanged. +pub(super) unsafe fn define_declared_class_accessor( + class_id: u32, + is_static: bool, + name: &str, + descriptor_value: f64, + desc_view: Option<&super::descriptor_helpers::DescView<'_>>, +) -> bool { + let Some((getter, setter)) = + super::super::class_registry::class_declared_accessor_ptrs(class_id, is_static, name) + else { + return false; + }; + let (enumerable, configurable) = + super::super::class_registry::class_accessor_attrs(class_id, is_static, name); + // The per-field reads below allocate a field-name string (and may run a + // user getter on a non-plain descriptor), so the descriptor is re-read from + // its root at every use. + let scope = crate::gc::RuntimeHandleScope::new(); + let desc = scope.root_nanbox_f64(descriptor_value); + if !configurable { + // The validator compares accessor halves by closure `func_ptr`, which a + // reflected class accessor value carries. Root the getter value across + // the setter value's allocation; the validator roots both on entry. + let get = scope.root_nanbox_f64( + super::super::class_registry::class_accessor_function_value(getter, false, name), + ); + let set = super::super::class_registry::class_accessor_function_value(setter, true, name); + validate_nonconfigurable_redefine( + name, + PropertyAttrs::new(false, enumerable, false), + Some(AccessorDescriptor { + get: get.get_nanbox_u64(), + set: set.to_bits(), + }), + f64::from_bits(crate::value::TAG_UNDEFINED), + desc.get_nanbox_f64(), + desc_view, + ); + } + // `ToPropertyDescriptor` field presence is HasProperty (own or inherited). + let has = |index: usize, field: &[u8]| -> bool { + match desc_view { + Some(view) => view.has(index), + None => desc_has_field(desc.get_nanbox_f64(), field), + } + }; + if has(DESC_GET, b"get") + || has(DESC_SET, b"set") + || has(DESC_VALUE, b"value") + || has(DESC_WRITABLE, b"writable") + { + return false; + } + // A present field is `ToBoolean(value)` — `{ enumerable: undefined }` is + // an explicit `false`, not an omission. + let flag = |index: usize, field: &[u8]| -> Option { + has(index, field).then(|| { + let value = match desc_view { + Some(view) => view.read(index), + None => desc_read_field(desc.get_nanbox_f64(), field), + }; + crate::value::js_is_truthy(f64::from_bits(value.bits())) != 0 + }) + }; + let enumerable = flag(DESC_ENUMERABLE, b"enumerable").unwrap_or(enumerable); + let configurable = flag(DESC_CONFIGURABLE, b"configurable").unwrap_or(configurable); + super::super::class_registry::class_set_accessor_attrs( + class_id, + is_static, + name, + enumerable, + configurable, + ); + true +} 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 9bf54caca2..c7ea7e6dde 100644 --- a/crates/perry-runtime/src/object/object_ops/define_property.rs +++ b/crates/perry-runtime/src/object/object_ops/define_property.rs @@ -824,6 +824,19 @@ pub extern "C" fn js_object_define_property( return obj_value; } if let Some(name) = super::super::metadata_key_to_string(key_value) { + // #10480: a declared accessor — instance on the prototype ref, + // static on the class ref — keeps its get/set under a generic + // descriptor; only its attributes change. + if super::define_class_accessor::define_declared_class_accessor( + target_cid, + super::super::class_prototype_ref_id(obj_value).is_none(), + &name, + desc_handle.get_nanbox_f64(), + desc_view.as_ref(), + ) { + return obj_value; + } + let descriptor_value = desc_handle.get_nanbox_f64(); let has_get = desc_has_field(descriptor_value, b"get"); let has_set = desc_has_field(descriptor_value, b"set"); if super::super::class_prototype_ref_id(obj_value).is_none() && (has_get || has_set) @@ -1402,6 +1415,23 @@ 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 { + // #10480: the prototype's ClassBody accessors have no physical + // key, so the ordinary arm below would define a NEW property + // over them. A physical key (an expando that shadows the class + // member) keeps the ordinary arm. + if !own_key_present(obj, key_str) + && across!( + super::define_class_accessor::define_declared_class_accessor( + target_cid, + false, + name, + descriptor_value, + desc_view.as_ref(), + ) + ) + { + return obj_value; + } 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() { diff --git a/crates/perry-runtime/src/object/object_ops/has_own.rs b/crates/perry-runtime/src/object/object_ops/has_own.rs index eb07574f9f..7355eff40e 100644 --- a/crates/perry-runtime/src/object/object_ops/has_own.rs +++ b/crates/perry-runtime/src/object/object_ops/has_own.rs @@ -600,7 +600,24 @@ pub extern "C" fn js_object_property_is_enumerable(obj_value: f64, key_value: f6 class_id, key_name, ) .is_some(); - return f64::from_bits(if is_static_field { TAG_TRUE } else { TAG_FALSE }); + // #10480: a declared static accessor is non-enumerable by + // ClassBody default, but a generic descriptor can flip it + // (Object.defineProperty(C, "x", { enumerable: true })). + let is_enumerable_static_accessor = + super::super::class_registry::class_accessor_attrs_in_use() + && super::super::class_registry::class_declared_accessor_ptrs( + class_id, true, key_name, + ) + .is_some() + && super::super::class_registry::class_accessor_attrs( + class_id, true, key_name, + ) + .0; + return f64::from_bits(if is_static_field || is_enumerable_static_accessor { + TAG_TRUE + } else { + TAG_FALSE + }); } } } @@ -720,6 +737,12 @@ pub extern "C" fn js_object_property_is_enumerable(obj_value: f64, key_value: f6 if (*obj).class_id != 0 && super::super::field_get_set::is_internal_runtime_key(key_name) { return f64::from_bits(TAG_FALSE); } + // #10480: a ClassBody accessor is an own property of the prototype with + // no physical key; a generic `defineProperty` can make it enumerable. + if super::super::class_registry::class_prototype_enumerable_accessor(obj as usize, key_name) + { + return f64::from_bits(TAG_TRUE); + } if !own_key_present(obj, key_str) { return f64::from_bits(TAG_FALSE); } diff --git a/scripts/gc_runtime_root_holders.json b/scripts/gc_runtime_root_holders.json index 70d46c0e73..49f36c114c 100644 --- a/scripts/gc_runtime_root_holders.json +++ b/scripts/gc_runtime_root_holders.json @@ -760,6 +760,12 @@ "scanner": "gc::roots::visit_global_root_slots, reached by js_gc_register_global_root (gc/roots.rs:325 pushes the slot into GLOBAL_ROOTS; gc/roots.rs:1433 hands it to the mutable-root walk)", "why": "Caches the shared VM intrinsic realm's globalThis as NaN-boxed bits. The single site that writes the cell \u2014 fresh_intrinsic_global, node_vm.rs:1080 \u2014 calls js_gc_register_global_root(slot.as_ptr()) in the same `with` closure, immediately after the store and with no allocation in between; the early return on a non-zero cell means that store happens at most once per thread, so there is no path that populates the cell without registering it. GLOBAL_ROOTS is thread_local, exactly like the cell, and visit_mutable_root_slots feeds it to BOTH the marker and the post-evacuation rewrite (gc/tests/copying.rs:1272, test_copying_minor_rewrites_shadow_and_global_roots), so the cached pointer is marked and forwarded rather than left stale." }, + { + "file": "crates/perry-runtime/src/object/class_registry/accessor_attrs.rs", + "name": "CLASS_ACCESSOR_ATTRS", + "verdict": "not_a_gc_pointer", + "why": "Reflective enumerable/configurable overrides applied by a generic Object.defineProperty(/ies) descriptor to a DECLARED class accessor (#10480), keyed by (class_id: u32, is_static: bool, name: String) to (enumerable: bool, configurable: bool). Every field is a plain scalar or an owned String \u2014 no NaN-boxed JSValue, no heap ObjectHeader address, nothing for the collector to mark or rewrite. The accessor's own getter/setter function pointers stay exactly where they already lived, in the class vtable side tables (CLASS_VTABLE_REGISTRY / CLASS_STATIC_ACCESSORS); this table only remembers which two attribute bits a generic descriptor overrode, never the accessor's identity or value." + }, { "file": "crates/perry-runtime/src/object/class_registry/state.rs", "name": "CLASS_DECLARED_STATIC_GLOBAL_SLOTS", diff --git a/test-files/test_gap_10480_define_property_generic_descriptor_accessors.ts b/test-files/test_gap_10480_define_property_generic_descriptor_accessors.ts new file mode 100644 index 0000000000..e2466bc990 --- /dev/null +++ b/test-files/test_gap_10480_define_property_generic_descriptor_accessors.ts @@ -0,0 +1,306 @@ +// #10480: an attributes-only descriptor on an existing CLASS accessor must +// keep its getter and setter and change only the attributes. +// +// `Object.defineProperties(C.prototype, { p: { enumerable: true } })` is how +// every WebIDL-generated class (whatwg-url, node-fetch, undici-style +// polyfills) publishes its prototype accessors. Perry's define path only knew +// the address-keyed descriptor tables, which never hold a ClassBody accessor, +// so it filed the key as a brand-new property with a `writable: false` data +// slot on the prototype: the setter stopped running (assignment threw +// "Cannot assign to read only property" in strict code, and was dropped in +// sloppy code), and the requested `enumerable` / `configurable` never showed +// up in the descriptor or in `Object.keys` / `for...in`. +// +// Covered here: get/set pairs, getter-only, setter-only, static accessors, +// `defineProperty` vs `defineProperties`, subclass instances, the +// non-configurable rejections, and the object-literal / `defineProperty` / +// function-prototype accessors that always worked (controls). + +function describe(object: any, key: string): string { + const descriptor = Object.getOwnPropertyDescriptor(object, key); + if (!descriptor) return "absent"; + const kind = + "get" in descriptor + ? `get=${typeof descriptor.get} set=${typeof descriptor.set}` + : `value=${typeof descriptor.value} writable=${descriptor.writable}`; + return `${kind} enumerable=${descriptor.enumerable} configurable=${descriptor.configurable}`; +} + +function outcome(fn: () => string): string { + try { + return fn(); + } catch (error: any) { + return `${error.constructor.name}`; + } +} + +function forIn(object: any): string { + const keys: string[] = []; + for (const key in object) keys.push(key); + return keys.join(","); +} + +// ── the node-fetch / whatwg-url shape ─────────────────────────────────────── +class URLLike { + _p = ""; + get pathname() { + return this._p; + } + set pathname(value: string) { + this._p = "set:" + value; + } +} +Object.defineProperties(URLLike.prototype, { pathname: { enumerable: true } }); +console.log("url-desc", describe(URLLike.prototype, "pathname")); +const url = new URLLike(); +console.log( + "url-assign", + outcome(() => { + url.pathname = "/x"; + return url.pathname; + }), +); +console.log("url-keys", Object.keys(URLLike.prototype).join(",")); +console.log("url-for-in", forIn(url)); +console.log( + "url-enumerable", + URLLike.prototype.propertyIsEnumerable("pathname"), + Object.prototype.propertyIsEnumerable.call(URLLike.prototype, "pathname"), +); + +// A dynamic receiver takes the runtime dispatch path rather than a compiled +// direct call to the declared setter. +const dynamicUrl: any = new URLLike(); +const dynamicKey = "pathname"; +dynamicUrl[dynamicKey] = "/dyn"; +console.log("url-dynamic", dynamicUrl[dynamicKey]); + +// ── defineProperty, every generic descriptor shape ────────────────────────── +class Single { + _p = ""; + get p() { + return this._p; + } + set p(value: string) { + this._p = "set:" + value; + } +} +Object.defineProperty(Single.prototype, "p", { configurable: true }); +const single = new Single(); +console.log( + "single-configurable", + outcome(() => { + single.p = "y"; + return single.p; + }), + describe(Single.prototype, "p"), +); +Object.defineProperty(Single.prototype, "p", {}); +console.log("single-empty", describe(Single.prototype, "p")); +Object.defineProperty(Single.prototype, "p", { enumerable: true }); +console.log("single-enumerable", describe(Single.prototype, "p"), Object.keys(Single.prototype).join(",")); +Object.defineProperty(Single.prototype, "p", { enumerable: false }); +console.log("single-non-enumerable", describe(Single.prototype, "p"), Object.keys(Single.prototype).join(",")); +console.log( + "single-still-set", + outcome(() => { + single.p = "z"; + return single.p; + }), +); + +// ── getter-only and setter-only halves ────────────────────────────────────── +class Halves { + _v = 0; + get readOnly() { + return "read:" + this._v; + } + set writeOnly(value: number) { + this._v = value + 1; + } +} +Object.defineProperties(Halves.prototype, { + readOnly: { enumerable: true }, + writeOnly: { enumerable: true }, +}); +console.log("halves-read", describe(Halves.prototype, "readOnly")); +console.log("halves-write", describe(Halves.prototype, "writeOnly")); +const halves: any = new Halves(); +console.log("halves-get", halves.readOnly); +halves.writeOnly = 41; +console.log("halves-set", halves._v, halves.readOnly); +console.log("halves-keys", Object.keys(Halves.prototype).join(",")); +console.log("halves-for-in", forIn(halves)); +console.log("halves-enumerable", Halves.prototype.propertyIsEnumerable("readOnly")); + +// ── inheritance: the accessor is redefined on the BASE prototype ──────────── +class Base { + _b = ""; + get tag() { + return this._b; + } + set tag(value: string) { + this._b = "base:" + value; + } +} +class Derived extends Base {} +Object.defineProperties(Base.prototype, { tag: { enumerable: true } }); +const derived = new Derived(); +console.log( + "derived-assign", + outcome(() => { + derived.tag = "v"; + return derived.tag; + }), +); +console.log("derived-for-in", forIn(derived)); +console.log("derived-own", Object.keys(Derived.prototype).join(","), Object.keys(Base.prototype).join(",")); + +// ── ClassBody order is preserved when several accessors go enumerable ─────── +class Ordered { + get b() { + return "b"; + } + m() { + return "m"; + } + get a() { + return "a"; + } + get c() { + return "c"; + } +} +Object.defineProperties(Ordered.prototype, { c: { enumerable: true }, b: { enumerable: true } }); +console.log("ordered-keys", Object.keys(Ordered.prototype).join(",")); +console.log("ordered-names", Object.getOwnPropertyNames(Ordered.prototype).join(",")); +console.log("ordered-entries", JSON.stringify(Object.entries(Ordered.prototype))); + +// ── static accessors ──────────────────────────────────────────────────────── +class Statics { + static _v = 1; + static get sv() { + return Statics._v; + } + static set sv(value: number) { + Statics._v = value * 10; + } +} +Object.defineProperty(Statics, "sv", { enumerable: true }); +console.log("static-desc", describe(Statics, "sv")); +console.log( + "static-enumerable", + Statics.propertyIsEnumerable("sv"), + Object.prototype.propertyIsEnumerable.call(Statics, "sv"), +); +Statics.sv = 2; +console.log("static-read", Statics.sv, Object.keys(Statics).join(","), forIn(Statics)); + +// ── non-configurable rejections ───────────────────────────────────────────── +class Locked { + get q() { + return 1; + } + set q(_value: number) {} +} +Object.defineProperty(Locked.prototype, "q", { configurable: false }); +console.log("locked-desc", describe(Locked.prototype, "q")); +console.log( + "locked-configurable", + outcome(() => { + Object.defineProperty(Locked.prototype, "q", { configurable: true }); + return "ok"; + }), +); +console.log( + "locked-enumerable", + outcome(() => { + Object.defineProperty(Locked.prototype, "q", { enumerable: true }); + return "ok"; + }), +); +console.log( + "locked-getter", + outcome(() => { + Object.defineProperty(Locked.prototype, "q", { + get() { + return 2; + }, + }); + return "ok"; + }), +); +console.log( + "locked-data", + outcome(() => { + Object.defineProperty(Locked.prototype, "q", { value: 3 }); + return "ok"; + }), +); +console.log( + "locked-same", + outcome(() => { + Object.defineProperty(Locked.prototype, "q", { configurable: false, enumerable: false }); + return "ok"; + }), +); +let deleted: unknown = "unset"; +try { + deleted = delete (Locked.prototype as any).q; +} catch (error: any) { + deleted = error instanceof TypeError; +} +console.log("locked-delete", deleted, describe(Locked.prototype, "q")); +console.log("locked-read", new Locked().q); + +// ── controls: shapes that always worked ───────────────────────────────────── +const literal: any = { + _v: "", + get p() { + return this._v; + }, + set p(value: string) { + this._v = "set:" + value; + }, +}; +Object.defineProperties(literal, { p: { enumerable: true } }); +literal.p = "lit"; +console.log("literal", literal.p, describe(literal, "p"), Object.keys(literal).join(",")); + +const made: any = {}; +Object.defineProperty(made, "p", { + get() { + return this._v; + }, + set(value: string) { + this._v = "set:" + value; + }, + configurable: true, +}); +Object.defineProperties(made, { p: { enumerable: true } }); +made.p = "def"; +console.log("defined", made.p, describe(made, "p")); + +function Legacy(this: any) {} +Object.defineProperty(Legacy.prototype, "p", { + get() { + return this._v; + }, + set(value: string) { + this._v = "set:" + value; + }, + configurable: true, +}); +Object.defineProperties(Legacy.prototype, { p: { enumerable: true } }); +const legacy: any = new (Legacy as any)(); +legacy.p = "fn"; +console.log("function-prototype", legacy.p, describe(Legacy.prototype, "p"), forIn(legacy)); + +// A class METHOD is a data property; a generic descriptor must keep its value. +class WithMethod { + m() { + return "m"; + } +} +Object.defineProperty(WithMethod.prototype, "m", { enumerable: true }); +console.log("method", new WithMethod().m(), describe(WithMethod.prototype, "m")); diff --git a/test-files/test_gap_10480_define_property_generic_descriptor_sloppy.cts b/test-files/test_gap_10480_define_property_generic_descriptor_sloppy.cts new file mode 100644 index 0000000000..52badb4ed7 --- /dev/null +++ b/test-files/test_gap_10480_define_property_generic_descriptor_sloppy.cts @@ -0,0 +1,42 @@ +// #10480, sloppy half: in non-strict CJS the broken define did not throw — the +// write was silently dropped, which is how it reached node-fetch's +// `Request.prototype` and whatwg-url's `URL.prototype` without an error. +// An attributes-only descriptor must leave the setter in place, so the write +// still runs it here and the getter reports the setter's value. + +class Sloppy { + _p = ""; + get pathname() { + return this._p; + } + set pathname(value: string) { + this._p = "set:" + value; + } + get readOnly() { + return "ro"; + } +} + +Object.defineProperties(Sloppy.prototype, { + pathname: { enumerable: true }, + readOnly: { enumerable: true }, +}); + +const instance = new Sloppy(); +instance.pathname = "/sloppy"; +console.log("assigned", instance.pathname, instance._p); + +const dynamic: any = new Sloppy(); +const key = "pathname"; +dynamic[key] = "/dynamic"; +console.log("dynamic", dynamic[key]); + +console.log("read-only", (instance as any).readOnly); + +const descriptor = Object.getOwnPropertyDescriptor(Sloppy.prototype, "pathname"); +console.log("descriptor", typeof descriptor.get, typeof descriptor.set, descriptor.enumerable, descriptor.configurable); + +const keys: string[] = []; +for (const name in instance) keys.push(name); +console.log("for-in", keys.join(",")); +console.log("keys", Object.keys(Sloppy.prototype).join(","));