Scripts: a String object counts its rope's copy when a character is read

- wrapping a string no longer flattens it, so Object(s).length and new String(s) on a large concatenated string make no copy and cannot collect while a call's operands are off the stack
- reading a String object's character by index, `in` and hasOwnProperty count the flattened copy at the next limit check, and the run ends before the copy when the fuel for it is not left
- unary plus on an object whose valueOf returns a concatenated string, and a property key from toString, keep that string rooted while its copy is reserved
- the unshift comment cites step 6 of section 15.4.4.13

crates/js/src/builtins/array.rs

@@ -479,7 +479,7 @@ fn unshift(interp: &mut Interp, this: Value, args: &[Value], _: bool) -> JsResul
     with_this(interp, &this, |interp, o| {
         let len = u64::from(length_of(interp, o)?);
         let count = args.len() as u64;
-        // With no arguments each element is still read and written back in place (steps 6 and 7).
+        // With no arguments each element is still read and written back in place (step 6).
         let maps = vec![Map::Shift(-1), Map::Shift(count as i64 - 1)];
         let mut it = Indices::new(interp, o, 1, len + 1, false, maps)?;
         while let Some(k) = it.next(interp)? {

crates/js/src/interp.rs

@@ -205,6 +205,11 @@ pub(crate) struct Interp {
     pub(crate) date_host: crate::date_builtins::DateHost,
     /// The bytes native code holds outside the heap's cells through [`HeldBytes`].
     pub(crate) held: Rc<HeldCount>,
+    /// Bytes of the copies [`Self::get_own_property`] made flattening the rope of a String
+    /// object, which the heap's total adds before the next limit check.
+    flattened: std::cell::Cell<usize>,
+    /// Fuel for the units those copies took, which the next step takes.
+    pub(crate) flattened_fuel: std::cell::Cell<u64>,
     /// The global code's default XML namespace URI (ECMA-357 section 12.1).
     pub(crate) xml_default_uri: Key,
     /// Set by the host to end the current slice of a resumable run at the next safe point.
@@ -450,6 +455,8 @@ impl Interp {
             files: None,
             date_host: crate::date_builtins::DateHost::default(),
             held: Rc::default(),
+            flattened: std::cell::Cell::new(0),
+            flattened_fuel: std::cell::Cell::new(0),
             xml_default_uri: Rc::from(Vec::new()),
             pause: Rc::default(),
             in_slice: false,
@@ -569,6 +576,7 @@ impl Interp {
 
     fn tick(&mut self) -> JsResult<()> {
         self.use_fuel()?;
+        crate::regexp_vm::use_fuel_n(self, self.flattened_fuel.replace(0))?;
         self.settle_held();
         if self.heap.bytes > self.gc_threshold {
             self.collect_garbage();
@@ -595,10 +603,12 @@ impl Interp {
         Ok(())
     }
 
-    /// Takes the bytes that dropped [`HeldBytes`] released off the heap's total.
+    /// Takes the bytes that dropped [`HeldBytes`] released off the heap's total, and adds the
+    /// bytes of the ropes [`Self::get_own_property`] flattened.
     fn settle_held(&mut self) {
         let released = self.held.released.replace(0);
-        self.heap.bytes = self.heap.bytes.saturating_sub(released);
+        let flattened = self.flattened.replace(0);
+        self.heap.bytes = self.heap.bytes.saturating_sub(released).saturating_add(flattened);
     }
 
     /// Collects garbage now and sets when the next collection runs.
@@ -664,6 +674,7 @@ impl Interp {
         self.heap.collect(&roots);
         // The collection counts the live total again, so bytes already released are not in it.
         self.held.released.set(0);
+        self.flattened.set(0);
         self.heap.bytes = self.heap.bytes.saturating_add(self.held_bytes).saturating_add(self.held.now.get());
     }
 
@@ -1214,6 +1225,17 @@ impl Interp {
                 if let Some(i) = array_index(k)
                     && (i as usize) < s.len()
                 {
+                    if s.is_rope() {
+                        // Flattening copies the units. The run ends here when the fuel for the
+                        // copy is not left; otherwise the next step takes it, and the next limit
+                        // check adds the copy's bytes.
+                        let fuel = self.flattened_fuel.get().saturating_add((s.len() / crate::string_builtins::UNITS_PER_FUEL) as u64 + 1);
+                        if fuel > self.fuel {
+                            return Err(Abrupt::Limit(LimitKind::Fuel));
+                        }
+                        self.flattened_fuel.set(fuel);
+                        self.flattened.set(self.flattened.get().saturating_add(string_bytes(s.len())));
+                    }
                     let unit = s.flat().get(i as usize).copied().unwrap_or(0);
                     return Ok(Some(Prop::Data { value: Value::String(Str::from_units(vec![unit])), flags: ENUMERABLE }));
                 }
@@ -2216,19 +2238,26 @@ impl Interp {
         let arguments =
             if code.uses_arguments { OBJECT_BYTES.saturating_add(args.len().saturating_add(4).saturating_mul(binding_bytes(10))) } else { 0 };
         let need = declaration_bytes(&code).saturating_add(ENV_BYTES).saturating_add(params).saturating_add(arguments);
-        // The operands are off the stack. Reserving the heap collects, and so can boxing a string
-        // receiver, which flattens it; the operands stay rooted until the frame holds them.
-        let collects = self.heap.bytes.saturating_add(need) > self.limits.heap_bytes || (!code.strict && matches!(this, Value::String(_)));
-        let mark = self.temp_roots.len();
-        if collects {
+        if self.heap.bytes.saturating_add(need) > self.limits.heap_bytes {
+            let mark = self.temp_roots.len();
             self.temp_roots.push(Value::Object(callee));
             self.temp_roots.push(this.clone());
             self.temp_roots.extend(construct.map(Value::Object));
             self.temp_roots.extend(args.iter().cloned());
+            let reserved = self.reserve_heap(need);
+            self.temp_roots.truncate(mark);
+            reserved?;
         }
-        let received = self.receive_call(&code, env, this, &args, callee, need);
-        self.temp_roots.truncate(mark);
-        let (this, fenv) = received?;
+        let this = if code.strict {
+            this
+        } else {
+            match this {
+                Value::Undefined | Value::Null => Value::Object(self.global),
+                Value::Object(_) => this,
+                prim => Value::Object(self.to_object(&prim)?),
+            }
+        };
+        let fenv = self.instantiate_function(&code, env, &args, callee)?;
         let base = self.stack.len();
         self.frames.push(Frame {
             code,
@@ -2246,25 +2275,6 @@ impl Interp {
         Ok(())
     }
 
-    /// Reserves the `need` bytes of a call, boxes a primitive receiver for non-strict code and
-    /// binds the parameters, returning the receiver and the function's environment.
-    fn receive_call(&mut self, code: &Rc<Code>, env: EnvId, this: Value, args: &[Value], callee: ObjId, need: usize) -> JsResult<(Value, EnvId)> {
-        if self.heap.bytes.saturating_add(need) > self.limits.heap_bytes {
-            self.reserve_heap(need)?;
-        }
-        let this = if code.strict {
-            this
-        } else {
-            match this {
-                Value::Undefined | Value::Null => Value::Object(self.global),
-                Value::Object(_) => this,
-                prim => Value::Object(self.to_object(&prim)?),
-            }
-        };
-        let fenv = self.instantiate_function(code, env, args, callee)?;
-        Ok((this, fenv))
-    }
-
     /// Declaration binding instantiation for function code (section 10.5). Each parameter and
     /// declaration takes a unit of fuel.
     fn instantiate_function(&mut self, code: &Rc<Code>, outer: EnvId, args: &[Value], callee: ObjId) -> JsResult<EnvId> {
@@ -2578,7 +2588,8 @@ impl Interp {
             Value::String(s) => string_to_number(&flat_charged(self, s)?),
             Value::Object(_) => {
                 let p = self.to_primitive(v, Hint::Number)?;
-                return self.to_number(&p);
+                // Rooted, so what a string holds is still counted while its copy is reserved.
+                return self.rooted(std::slice::from_ref(&p), |i| i.to_number(&p));
             }
         })
     }
@@ -2613,7 +2624,8 @@ impl Interp {
             Value::Number(n) if *n >= 0.0 && *n < 4_294_967_295.0 && n.fract() == 0.0 => Ok(index_key(*n as u32)),
             _ => {
                 let s = self.to_str(v)?;
-                self.flat_reserved(&s)
+                // Rooted, so what the string holds is still counted while its copy is reserved.
+                self.rooted(&[Value::String(s.clone())], |i| i.flat_reserved(&s))
             }
         }
     }
@@ -2638,11 +2650,7 @@ impl Interp {
             Value::Undefined | Value::Null => return Err(self.type_error("cannot convert undefined or null to an object")),
             Value::Bool(b) => (Class::Boolean(*b), self.intrinsics.boolean_proto),
             Value::Number(n) => (Class::Number(*n), self.intrinsics.number_proto),
-            // Flattened now, with its copy counted, so reading a character copies nothing.
-            Value::String(s) => {
-                flat_charged(self, s)?;
-                (Class::String(s.clone()), self.intrinsics.string_proto)
-            }
+            Value::String(s) => (Class::String(s.clone()), self.intrinsics.string_proto),
         };
         Ok(self.heap.alloc_object(Object::new(class, Some(proto))))
     }

crates/js/src/realm.rs

@@ -230,6 +230,7 @@ impl Realm {
             return Err(RunError { kind: RunErrorKind::Internal, message: message.clone(), line: 0, column: 0 });
         }
         interp.fuel = interp.limits.fuel;
+        interp.flattened_fuel.set(0);
         interp.source_units = 0;
         interp.throw_site = None;
         interp.last_thrown = Value::Undefined;

crates/js/src/string_builtins.rs

@@ -214,9 +214,6 @@ fn string_ctor(interp: &mut Interp, _: Value, args: &[Value], construct: bool) -
     if !construct {
         return Ok(Value::String(s));
     }
-    // Flattened now, with its copy counted, so reading a character copies nothing. The string
-    // stays rooted, so what it holds is still counted if reserving the copy collects.
-    interp.rooted(&[Value::String(s.clone())], |interp| flat_charged(interp, &s))?;
     let proto = interp.intrinsics.string_proto;
     Ok(Value::Object(interp.heap.alloc_object(Object::new(Class::String(s), Some(proto)))))
 }

crates/js/src/text_tests.rs

@@ -355,22 +355,63 @@ fn a_flattened_temporary_stays_counted_while_the_next_copy_is_made() {
 }
 
 #[test]
-fn a_string_wrapper_counts_what_its_rope_holds_while_the_copy_is_made() {
+fn a_converted_rope_counts_what_it_holds_while_its_copy_is_made() {
     // `mk` returns a rope whose left half is a flat string of about half the heap, held by
     // nothing else. Its flattened copy is the other half, so the two don't fit together.
     let build = "var s = 'x'; for (var i = 0; i < 18; i++) s = s + s; function f(c) { return s + c; } \
                  function mk() { var a = f('a'); a[0]; return a + 'b'; }";
     let limits = Limits { heap_bytes: 4 * 262_146, ..Limits::default() };
-    let src = format!("{build} new String({{ toString: mk }}); 0");
-    let result = Realm::with_options(Options::default(), limits).run(&src);
-    assert!(matches!(result, Err(ref e) if e.kind == RunErrorKind::Limit(LimitKind::Heap)), "{result:?}");
+    let not_stopped: Vec<_> = ["+({ valueOf: mk })", "({})[{ toString: mk }]"]
+        .into_iter()
+        .filter(|read| {
+            let result = Realm::with_options(Options::default(), limits.clone()).run(&format!("{build} {read}; 0"));
+            !matches!(result, Err(ref e) if e.kind == RunErrorKind::Limit(LimitKind::Heap))
+        })
+        .collect();
+    assert!(not_stopped.is_empty(), "not stopped by the heap limit: {not_stopped:?}");
+}
+
+#[test]
+fn a_string_object_counts_its_flattened_rope_against_the_heap() {
+    // The same three ropes, each wrapped in a String object and read by index.
+    let build = "var s = 'x'; for (var i = 0; i < 19; i++) s = s + s; var t = s + 'y', u = s + 'z'; \
+                 String.prototype.first = function () { return this[0]; };";
+    let limits = Limits { heap_bytes: 3 << 20, ..Limits::default() };
+    // Wrapping alone reads no units, so it makes no copy.
+    let lengths =
+        Realm::with_options(Options::default(), limits.clone()).run(&format!("{build} Object(s).length + new String(t).length + Object(u).length"));
+    assert_eq!(lengths, Ok(JsValue::Number(1_572_866.0)));
+    for read in ["Object(X)[0]", "new String(X)[0]", "0 in Object(X)", "Object(X).hasOwnProperty(0)", "X.first()"] {
+        let src = format!("{build} {}; {}; {}; 0", read.replace('X', "s"), read.replace('X', "t"), read.replace('X', "u"));
+        let e = fail_with(Realm::with_options(Options::default(), limits.clone()), &src);
+        assert_eq!(e.kind, RunErrorKind::Limit(LimitKind::Heap), "{read}: {e:?}");
+    }
+    let wrapped =
+        Realm::with_options(Options::default(), Limits::default()).run(&format!("{build} Object(t)[524288] + new String(u)[524288] + t.first()"));
+    assert_eq!(wrapped, Ok(JsValue::String("yzx".into())));
+}
+
+#[test]
+fn a_string_object_takes_fuel_for_the_rope_it_flattens() {
+    let limits = Limits { fuel: 40_000, ..Limits::default() };
+    let build = "var s = ' '; for (var i = 0; i < 20; i++) s = s + s; Object(s).length";
+    assert!(Realm::with_options(Options::default(), limits.clone()).run(build).is_ok());
+    for read in ["Object(s)[0]", "0 in new String(s)"] {
+        let e = fail_with(Realm::with_options(Options::default(), limits.clone()), &format!("{build}; {read}"));
+        assert_eq!(e.kind, RunErrorKind::Limit(LimitKind::Fuel), "{read}: {e:?}");
+    }
+    // A run that ends with an error right after the read leaves the next run its whole fuel.
+    let mut realm = Realm::with_options(Options::default(), Limits { fuel: 100_000, ..Limits::default() });
+    let defined = realm.run("var s = ' '; for (var i = 0; i < 20; i++) s = s + s; Object.defineProperty(new String(s), 0, { value: 'a' })");
+    assert!(matches!(defined, Err(ref e) if e.kind == RunErrorKind::Thrown), "{defined:?}");
+    assert!(realm.run("for (var j = 0; j < 5000; j++) {} j").is_ok());
 }
 
 #[test]
 fn a_call_keeps_its_operands_while_a_string_receiver_is_boxed() {
-    // Boxing the rope receiver of a non-strict function flattens it, and reserving the copy
-    // collects the garbage string `g` left. The callee and the argument object are off the
-    // stack by then, and must survive the collection.
+    // `g` leaves a garbage string, so the heap is close to its limit when the rope receiver of a
+    // non-strict function is boxed. The callee and the argument object are off the stack by
+    // then, and must survive anything boxing the receiver does.
     let build = "var s = 'x'; for (var i = 0; i < 17; i++) s = s + s; function f(c) { return s + c; } \
                  function mk() { var a = f('a'); a[0]; return a + 'b'; } 0";
     let mut realm = Realm::with_options(Options::default(), Limits::default());