Scripts: a property name converted from an object stays counted while its getter or setter runs

A computed name such as o[key], where key's toString returns a rope,
is flattened into a copy that only native code held. A getter or
setter that started a collection dropped that copy from the heap
estimate, so the live heap could pass the limit by one name per
nested access. Indexed reads, ++ and -- on an indexed property, and
Object.defineProperty now keep the converted name reachable while the
accessor runs.

crates/js/src/builtins/object.rs

@@ -166,10 +166,11 @@ fn create(interp: &mut Interp, _: Value, args: &[Value], _: bool) -> JsResult<Va
 /// `Object.defineProperty` (section 15.2.3.6).
 fn define_property(interp: &mut Interp, _: Value, args: &[Value], _: bool) -> JsResult<Value> {
     let o = require_object(interp, &arg(args, 0), "defineProperty")?;
-    let k = interp.to_key(&arg(args, 1))?;
-    let desc = to_desc(interp, &arg(args, 2))?;
-    let roots = desc_values(&desc);
-    interp.rooted(&roots, |interp| interp.define_own_property(o, k, &desc, true))?;
+    interp.with_key(&arg(args, 1), |interp, k| {
+        let desc = to_desc(interp, &arg(args, 2))?;
+        let roots = desc_values(&desc);
+        interp.rooted(&roots, |interp| interp.define_own_property(o, k, &desc, true))
+    })?;
     Ok(Value::Object(o))
 }
 

crates/js/src/interp.rs

@@ -1779,8 +1779,7 @@ impl Interp {
             return self.replace(2, v);
         }
         self.check_coercible(&base, &[])?;
-        let name = self.to_key(&k)?;
-        let v = self.get_value(&base, &name)?;
+        let v = self.with_key(&k, |i, name| i.get_value(&base, &name))?;
         self.replace(2, v)?;
         if with_this { self.push(base) } else { Ok(()) }
     }
@@ -1852,8 +1851,7 @@ impl Interp {
             return self.replace(2, Value::Number(if prefix { new } else { old }));
         }
         self.check_coercible(&base, &[])?;
-        let name = self.to_key(&k)?;
-        let r = self.inc_property(&base, name, prefix, dec)?;
+        let r = self.with_key(&k, |i, name| i.inc_property(&base, name, prefix, dec))?;
         self.replace(2, r)
     }
 
@@ -2670,6 +2668,18 @@ impl Interp {
         }
     }
 
+    /// Runs `f` with the property name [`Self::to_key`] makes from `v`. A name converted from a
+    /// value that is not a string is kept reachable while `f` runs, so a collection that a getter
+    /// or setter starts still counts its units.
+    pub(crate) fn with_key<T>(&mut self, v: &Value, f: impl FnOnce(&mut Self, Key) -> JsResult<T>) -> JsResult<T> {
+        let name = self.to_key(v)?;
+        if matches!(v, Value::String(_)) {
+            return f(self, name);
+        }
+        let root = Value::String(Str::from_rc(name.clone()));
+        self.rooted(&[root], |i| f(i, name))
+    }
+
     /// The code units of `s`. Flattening a rope makes a copy of its units, so for a rope that
     /// copy's bytes are reserved and added to the heap first, without taking fuel. The rope stays
     /// rooted while the copy is reserved; everything else the caller still needs must be

crates/js/src/text_tests.rs

@@ -434,6 +434,32 @@ fn a_call_keeps_its_operands_while_a_string_receiver_is_boxed() {
     assert_eq!(realm.run(src), Ok(JsValue::Number(1.0)));
 }
 
+#[test]
+fn a_computed_key_stays_counted_while_its_getter_or_setter_runs() {
+    // `o` has an accessor named by a flat string of 131,073 units. `key`'s toString returns a new
+    // rope with the same units, so using `key` as a name makes a second flat copy, which must stay
+    // counted while an accessor makes a third. The heap has room for one more copy, not two.
+    let build = "var s = 'x'; for (var i = 0; i < 17; i++) s = s + s; function f(c) { return s + c; } \
+                 function copy() { var g = f('g'); g[0]; return 1; } var o = {}, name = f('k'); name[0]; \
+                 Object.defineProperty(o, name, { get: copy, set: copy }); \
+                 var key = { toString: function () { return f('k'); } }; 0";
+    let realm = || {
+        let mut realm = Realm::with_options(Options::default(), Limits::default());
+        assert_eq!(realm.run(build), Ok(JsValue::Number(0.0)));
+        let interp = realm.interp();
+        interp.collect_garbage();
+        let copy = crate::heap::string_bytes(131_073);
+        interp.limits.heap_bytes = interp.heap.bytes + copy + copy / 2;
+        interp.reset_gc_threshold();
+        realm
+    };
+    assert_eq!(realm().run("o[name]"), Ok(JsValue::Number(1.0)), "the accessor's own copy fits");
+    for read in ["o[key]", "o[key] = 1", "o[key]++", "Object.defineProperty({}, key, { get value() { return copy(); } })"] {
+        let result = realm().run(read);
+        assert!(matches!(result, Err(ref e) if e.kind == RunErrorKind::Limit(LimitKind::Heap)), "{read}: {result:?}");
+    }
+}
+
 #[test]
 fn compiling_a_pattern_counts_against_the_heap() {
     let src = "var s = '[\\\\s\\\\S]a*'; for (var i = 0; i < 14; i++) s = s + s; new RegExp(s).source.length";