fix(runtime): make iterator own properties readable; present-undefined .next throws (follow-up to #9066)

changelog.d/9075-iterator-own-prop-reads.md

@@ -0,0 +1,7 @@
+### Fixed: iterator own properties were write-only through the by-name GET; `it.next = undefined` silently ran the builtin (follow-up to #9066, PR #9075)
+
+The Map/Set-iterator arm in the by-name GET tail returned `undefined` for every non-`next` key without consulting own fields, so the #9066 reserved-floor storage was write-only through that lane — user properties stored past the floor (and hole-squeeze survivors) read back `undefined` while their values sat intact in the overflow spill. The arm moved to `accessors::map_set_iterator_property` with own-field shadowing first (ordinary [[Get]] order, so an own `return` patch also shadows the synthetic bound method).
+
+Also per review: an own `next` explicitly assigned `undefined` is present-but-non-callable and now throws per IteratorNext (bytes-based presence scan, no allocation on the unpatched hot path), and a failed reserved-floor seed drops the write instead of proceeding unseeded onto the backing-collection field.
+
+Validated byte-for-byte against the pinned Node 26.5.1 oracle (16 gap cases, including the reviewer's 12-add/10-delete survivor shape) plus 6 `reserved_floor` unit tests.

crates/perry-runtime/src/object/field_get_set/accessors.rs

@@ -136,6 +136,46 @@ pub(crate) unsafe fn own_data_field_by_name(
     None
 }
 
+/// #2856 synthetic method reads + #9019 own-field shadowing for a Map/Set
+/// iterator receiver's property GET (extracted from
+/// `get_field_by_name_tail.rs`, which sits at the file-size cap).
+///
+/// Ordinary [[Get]] order: an OWN property — user code can store one past
+/// the reserved floor since #9019 — shadows every synthetic method. This is
+/// also what makes user data properties on iterators readable at all: the
+/// old arm returned `undefined` for every non-`next` key without consulting
+/// own fields, so a stored value was write-only. Then the legacy bound
+/// synthetic methods (`return`/`throw`/`@@iterator`); `next` deliberately
+/// resolves through the caller's generic scans (`None`) so
+/// `iterator.next.call(other)` receives `other` and brand-checks; any other
+/// key is absent (`Some(undefined)`).
+pub(crate) unsafe fn map_set_iterator_property(
+    obj: *const ObjectHeader,
+    key: *const crate::StringHeader,
+) -> Option<JSValue> {
+    if let Some(v) = own_data_field_by_name(obj, key) {
+        return Some(v);
+    }
+    let key_ptr = (key as *const u8).add(std::mem::size_of::<crate::StringHeader>());
+    let key_len = (*key).byte_len as usize;
+    let key_bytes = std::slice::from_raw_parts(key_ptr, key_len);
+    let bind_name: Option<&'static [u8]> = match key_bytes {
+        b"return" => Some(b"return"),
+        b"throw" => Some(b"throw"),
+        b"@@iterator" => Some(b"@@iterator"),
+        _ => None,
+    };
+    if let Some(name) = bind_name {
+        let this_f64 = f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits());
+        let result = super::super::js_class_method_bind(this_f64, name.as_ptr(), name.len());
+        return Some(JSValue::from_bits(result.to_bits()));
+    }
+    if key_bytes == b"next" {
+        return None;
+    }
+    Some(JSValue::undefined())
+}
+
 crate::perry_thread_local! {
     static OBJECT_PROTOTYPE_LOOKUP_DEPTH: std::cell::Cell<u32> = const { std::cell::Cell::new(0) };
 }

crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs

@@ -1227,36 +1227,17 @@ pub(crate) fn get_field_by_name_object_tail(
             }
         }
 
-        // #2856: a property READ (not a call) of `next` on a Map/Set
-        // iterator object must yield a callable (so `typeof it.next ===
-        // "function"` and `const n = it.next; n()` work). The iterators
-        // dispatch via class id and store no `next` field, so bind the
-        // method to the receiver. Also bind the self-iterator methods.
+        // #2856 synthetic method reads + #9019 own-field shadowing for
+        // Map/Set iterator receivers — body in
+        // `accessors::map_set_iterator_property`. `None` means the key is
+        // `next` with no own patch: the generic scans below resolve the
+        // prototype thunk.
         if !key.is_null()
             && ((*obj).class_id == crate::collection_iter_object::MAP_ITERATOR_CLASS_ID
                 || (*obj).class_id == crate::collection_iter_object::SET_ITERATOR_CLASS_ID)
         {
-            let key_ptr = (key as *const u8).add(std::mem::size_of::<crate::StringHeader>());
-            let key_len = (*key).byte_len as usize;
-            let key_bytes = std::slice::from_raw_parts(key_ptr, key_len);
-            // `next` is an ordinary prototype method.  Do not bind it to the
-            // iterator at property-read time: `iterator.next.call(other)` must
-            // receive `other` and perform the spec brand check.  The remaining
-            // legacy synthetic methods still use the bound-method path.
-            let bind_name: Option<&'static [u8]> = match key_bytes {
-                b"return" => Some(b"return"),
-                b"throw" => Some(b"throw"),
-                b"@@iterator" => Some(b"@@iterator"),
-                _ => None,
-            };
-            if let Some(name) = bind_name {
-                let this_f64 =
-                    f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits());
-                let result = js_class_method_bind(this_f64, name.as_ptr(), name.len());
-                return JSValue::from_bits(result.to_bits());
-            }
-            if key_bytes != b"next" {
-                return JSValue::undefined();
+            if let Some(v) = super::accessors::map_set_iterator_property(obj, key) {
+                return v;
             }
         }
 

crates/perry-runtime/src/object/field_set_by_name/tail.rs

@@ -466,12 +466,16 @@ pub(crate) fn set_field_by_name_object_tail(
         // transition-cache fast path, whose `prev_shape_id` is read AFTER
         // this — then appends at the floor. Seeding allocates, so every raw
         // local is re-read through its handle.
-        if keys.is_null()
-            && crate::object::reserved_slot_floor_for_class_id((*obj).class_id) != 0
-            && crate::object::ensure_reserved_floor_keys(obj)
-        {
+        if keys.is_null() && crate::object::reserved_slot_floor_for_class_id((*obj).class_id) != 0 {
+            let seeded = crate::object::ensure_reserved_floor_keys(obj);
             refresh_roots_after_alloc!();
             keys = crate::object::object_keys_array(obj);
+            if !seeded && keys.is_null() {
+                // Seed failed (allocation refused): DROP the write rather
+                // than run the append below, whose index-0 slot is the
+                // backing-collection pointer.
+                return;
+            }
         }
 
         let mut prev_keys_usize = keys as usize;

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

@@ -371,7 +371,23 @@ pub(crate) unsafe fn call_overridden_iterator_next(
     // matching IteratorNext's GetV+Call — it must never fall through to the
     // builtin advance, which would ignore the patch the user installed.
     let own = super::js_object_get_own_field_or_undef(iter.get_nanbox_f64(), b"next".as_ptr(), 4);
-    if own.to_bits() != crate::value::TAG_UNDEFINED {
+    // `it.next = undefined` is PRESENT but non-callable (GetV yields the
+    // stored undefined, Call throws), which the value read alone cannot
+    // distinguish from absence. The bytes-based keys scan allocates nothing,
+    // and an unpatched iterator's keys edge is null, so the hot path pays
+    // one null check.
+    let own_present = own.to_bits() != crate::value::TAG_UNDEFINED || {
+        let obj = crate::value::js_nanbox_get_pointer(iter.get_nanbox_f64()) as *const ObjectHeader;
+        let keys = super::object_keys_array(obj);
+        !keys.is_null()
+            && super::keys_find_slot_by_bytes(
+                keys,
+                crate::array::js_array_length(keys) as u32,
+                b"next",
+            )
+            .is_some()
+    };
+    if own_present {
         if !JSValue::from_bits(own.to_bits()).is_pointer() {
             crate::closure::throw_not_callable();
         }

crates/perry-runtime/src/object/object_ops/keys_array.rs

@@ -32,11 +32,15 @@ pub(crate) unsafe fn ensure_key_in_keys_array(
         // accessor installs, which claim a keys slot with no data write)
         // can never take a raw internal field's index. The seeded receiver
         // then falls through to the ordinary existing-keys append below.
-        if crate::object::reserved_slot_floor_for_class_id((*obj).class_id) != 0
-            && crate::object::ensure_reserved_floor_keys(obj)
-        {
+        if crate::object::reserved_slot_floor_for_class_id((*obj).class_id) != 0 {
+            let seeded = crate::object::ensure_reserved_floor_keys(obj);
             refresh_define_property_roots!();
             keys = crate::object::object_keys_array(obj);
+            if !seeded && keys.is_null() {
+                // Seed failed (allocation refused): drop the key claim
+                // rather than let it take a raw internal field's index.
+                return;
+            }
         } else {
             let new_keys = crate::array::js_array_alloc(4);
             refresh_define_property_roots!();

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

@@ -272,6 +272,64 @@ mod tests {
         assert_eq!(reserved_slot_floor_for_class_id(42), 0);
     }
 
+    /// #9066 review: user properties must be READABLE, not merely stored.
+    /// The Map/Set-iterator GET arm used to answer `undefined` for every
+    /// non-`next` key without consulting own fields, which made the seeded
+    /// storage write-only — 12 properties written, all reading back
+    /// undefined, and squeeze survivors appearing to "lose" values that
+    /// were in the overflow spill all along.
+    #[test]
+    fn user_properties_read_back_through_the_get_path_at_scale() {
+        unsafe {
+            let iter = set_iter_with_10_20_30();
+            let backing_before = js_object_get_field(iter, 0).bits();
+            for i in 0..12 {
+                js_object_set_field_by_name(iter, key(&format!("p{i}")), (i as f64) * 100.0);
+            }
+            for i in 0..12 {
+                let got = f64::from_bits(
+                    crate::object::js_object_get_field_by_name(iter, key(&format!("p{i}"))).bits(),
+                );
+                assert_eq!(got, (i as f64) * 100.0, "p{i} must read back by name");
+            }
+            // Delete ten (crossing the hole-squeeze threshold) — the two
+            // survivors keep their VALUES and the raw fields stay intact.
+            for i in 0..10 {
+                assert_eq!(
+                    crate::object::js_object_delete_field(iter, key(&format!("p{i}"))),
+                    1
+                );
+            }
+            for i in 10..12 {
+                let got = f64::from_bits(
+                    crate::object::js_object_get_field_by_name(iter, key(&format!("p{i}"))).bits(),
+                );
+                assert_eq!(got, (i as f64) * 100.0, "survivor p{i} must keep its value");
+            }
+            assert_eq!(
+                js_object_get_field(iter, 0).bits(),
+                backing_before,
+                "raw fields survive the churn"
+            );
+            let r = crate::collection_iter_object::dispatch_set_iterator_method(iter, "next");
+            assert_eq!(result_value(r), 10.0);
+        }
+    }
+
+    /// An own `return` patch shadows the synthetic bound method on the GET
+    /// path (ordinary [[Get]] order).
+    #[test]
+    fn own_return_patch_shadows_the_synthetic_binding() {
+        unsafe {
+            let iter = set_iter_with_10_20_30();
+            js_object_set_field_by_name(iter, key("return"), 1234.0);
+            let got = f64::from_bits(
+                crate::object::js_object_get_field_by_name(iter, key("return")).bits(),
+            );
+            assert_eq!(got, 1234.0, "own value must shadow the bound method");
+        }
+    }
+
     /// Deleting the patch tombstones it without disturbing the reserved
     /// prefix, and the builtin advance resumes.
     #[test]

test-files/test_gap_iterator_patched_next.ts

@@ -115,3 +115,39 @@
   for (const v of it) got.push(v as number);
   console.log("L", got.join(","));
 }
+
+// N: user data properties are readable at scale, survive a hole-squeeze
+// (12 adds, 10 deletes), and never disturb iteration state.
+{
+  const s = new Set<number>([5, 6]);
+  const it: any = s.values();
+  for (let i = 0; i < 12; i++) it["p" + i] = i * 100;
+  console.log("N1", it.p0, it.p11);
+  for (let i = 0; i < 10; i++) delete it["p" + i];
+  console.log("N2", JSON.stringify(Object.keys(it)), it.p10, it.p11);
+  const r = it.next();
+  console.log("N3", r.value, r.done);
+}
+
+// O: an own `return` assignment shadows the builtin on the read path.
+{
+  const s = new Set<number>([1]);
+  const it: any = s.values();
+  it.ret0 = 7;
+  (it as any).return = 1234;
+  console.log("O", it.return, it.ret0);
+}
+
+// P: an own next EXPLICITLY set to undefined is present-but-non-callable —
+// for-of must throw, not fall back to the builtin advance.
+{
+  const s = new Set<number>([1]);
+  const it: any = s.values();
+  it.next = undefined;
+  try {
+    for (const v of it) console.log("P-unexpected", v);
+    console.log("P", "no-throw");
+  } catch (e: any) {
+    console.log("P", e instanceof TypeError);
+  }
+}