JavaScript: built-ins keep a new value rooted while they reserve room to store it

Array methods that store an element (map, filter, slice, splice, concat,
shift, unshift and sort), the JSON.parse reviver walk and JSON.stringify
reserved heap for a value that only native code held. When the heap was
near its limit, that reservation ran a collection that freed the value
before it was stored, and the script later failed with "object out of
range". The value now stays on the temporary roots until it is stored,
and stringify roots an object from toJSON or a replacer before it writes
the separator and key.

crates/js/src/builtins.rs

@@ -154,10 +154,13 @@ fn has_at(interp: &mut Interp, o: ObjId, k: f64) -> JsResult<bool> {
 /// The estimated bytes a new element adds.
 const ELEMENT_BYTES: usize = PROP_BYTES + 32 + 2 * 10;
 
-/// `o[k] = v` as the array methods assign: a failure throws.
+/// `o[k] = v` as the array methods assign: a failure throws. `v` stays rooted while heap is
+/// reserved for it, since the caller may hold it nowhere else.
 fn put_at(interp: &mut Interp, o: ObjId, k: f64, v: Value) -> JsResult<()> {
-    interp.reserve_heap(ELEMENT_BYTES)?;
-    interp.put(o, key_of(k), v, true)
+    interp.rooted(std::slice::from_ref(&v), |i| {
+        i.reserve_heap(ELEMENT_BYTES)?;
+        i.put(o, key_of(k), v.clone(), true)
+    })
 }
 
 fn delete_at(interp: &mut Interp, o: ObjId, k: f64) -> JsResult<()> {
@@ -165,10 +168,13 @@ fn delete_at(interp: &mut Interp, o: ObjId, k: f64) -> JsResult<()> {
 }
 
 /// Defines `a[k]` as a writable, enumerable and configurable data property, as the methods that
-/// build a new array do (DefineOwnProperty with `throw` false).
+/// build a new array do (DefineOwnProperty with `throw` false). `v` stays rooted while heap is
+/// reserved for it.
 fn create_at(interp: &mut Interp, a: ObjId, k: f64, v: Value) -> JsResult<()> {
-    interp.reserve_heap(ELEMENT_BYTES)?;
-    interp.define_own_property(a, key_of(k), &Desc::data(v, OPEN), false).map(drop)
+    interp.rooted(std::slice::from_ref(&v), |i| {
+        i.reserve_heap(ELEMENT_BYTES)?;
+        i.define_own_property(a, key_of(k), &Desc::data(v.clone(), OPEN), false).map(drop)
+    })
 }
 
 /// A new array with the given length and no elements.

crates/js/src/builtins/array.rs

@@ -630,8 +630,8 @@ fn sort_elements(interp: &mut Interp, o: ObjId, len: u64, compare: &Value) -> Js
         match element(interp, o, k)? {
             Some(Value::Undefined) => undefined += 1,
             Some(v) => {
-                interp.reserve_heap(ELEMENT_BYTES)?;
                 interp.temp_roots.push(v.clone());
+                interp.reserve_heap(ELEMENT_BYTES)?;
                 values.push(v);
             }
             None => {}

crates/js/src/builtins/json.rs

@@ -362,8 +362,10 @@ fn revive(interp: &mut Interp, v: Value, reviver: &Value) -> JsResult<Value> {
             if matches!(r, Value::Undefined) {
                 interp.delete(o, &k, false)?;
             } else {
-                interp.reserve_heap(ELEMENT_BYTES)?;
-                interp.define_own_property(o, k, &Desc::data(r, OPEN), false)?;
+                interp.rooted(std::slice::from_ref(&r), |i| {
+                    i.reserve_heap(ELEMENT_BYTES)?;
+                    i.define_own_property(o, k, &Desc::data(r.clone(), OPEN), false)
+                })?;
             }
         }
         if let (Some(k), Value::Object(o)) = (top.keys.get(top.next).cloned(), &top.val) {
@@ -436,7 +438,10 @@ fn stringify_rooted(interp: &mut Interp, args: &[Value]) -> JsResult<Value> {
     match resolve(interp, &settings, wrapper, &key(""))? {
         Item::Skip => return Ok(Value::Undefined),
         Item::Primitive(v) => write_primitive(interp, &mut w.out, &v)?,
-        Item::Container(o) => open(interp, &settings, &mut w, o)?,
+        Item::Container(o) => {
+            interp.temp_roots.push(Value::Object(o));
+            open(interp, &settings, &mut w, o)?;
+        }
     }
     while !w.stack.is_empty() {
         step(interp, &settings, &mut w)?;
@@ -585,12 +590,12 @@ fn write_primitive(interp: &mut Interp, out: &mut Builder, v: &Value) -> JsResul
 }
 
 /// Starts writing an object or array: a cycle back to one already being written throws a
-/// `TypeError`.
+/// `TypeError`. The caller pushes `o` onto the temporary roots first, and `step` pops it when the
+/// object or array is closed.
 fn open(interp: &mut Interp, settings: &Settings, w: &mut Writer, o: ObjId) -> JsResult<()> {
     if w.open.contains(&o) {
         return Err(interp.type_error("JSON.stringify cannot write a structure that contains itself"));
     }
-    interp.temp_roots.push(Value::Object(o));
     let array = interp.heap.object(o).is_some_and(|x| matches!(x.class, Class::Array { .. }));
     let keys = if array {
         let len = length_of(interp, o)?;
@@ -664,6 +669,10 @@ fn step(interp: &mut Interp, settings: &Settings, w: &mut Writer) -> JsResult<()
     if let Some(top) = w.stack.last_mut() {
         top.count += 1;
     }
+    // The separator and the key reserve heap, so a container is rooted before they are written.
+    if let Item::Container(o) = item {
+        interp.temp_roots.push(Value::Object(o));
+    }
     separator(interp, w, count == 0, gap)?;
     if !array {
         write_quoted(interp, &mut w.out, &k)?;

crates/js/src/builtins_tests.rs

@@ -300,6 +300,63 @@ fn values_built_ins_hold_survive_collections() {
     assert_eq!(realm.run(src), Ok(JsValue::Number(598.0)));
 }
 
+#[test]
+fn fresh_values_survive_a_collection_while_they_are_stored() {
+    // Below 1 MiB the only collections run at the heap limit, many of them while a built-in
+    // reserves room for a value that a callback has just made and nothing else holds.
+    let limits = Limits { heap_bytes: 768 << 10, ..Limits::default() };
+    let src = "var bad = 0; \
+               for (var n = 0; n < 2000; n++) { \
+                 var m = [1, 2, 3, 4, 5, 6, 7, 8].map(function (x) { var g = {x: x}; return {v: x}; }); \
+                 var f = [1, 2, 3, 4].filter(function () { return true; }).map(function (x) { return [x]; }); \
+                 for (var i = 0; i < 8; i++) if (!m[i] || m[i].v !== i + 1) bad++; \
+                 for (var i = 0; i < 4; i++) if (!f[i] || f[i][0] !== i + 1) bad++; \
+               } bad";
+    let mut realm = small(limits.clone());
+    assert_eq!(realm.run(src), Ok(JsValue::Number(0.0)));
+    assert!(realm.interp().heap.collections > 0);
+    // Getters make a new object for each read, which `shift` and `sort` then store.
+    let src = "function fresh(v) { var g = {x: v}; return {v: v}; } \
+               function set(i) { return function (x) { Object.defineProperty(this, i, {value: x, writable: true, enumerable: true, configurable: true}); }; } \
+               var p = {}; \
+               for (var i = 0; i < 4; i++) Object.defineProperty(p, i, {get: (function (v) { return function () { return fresh(v); }; })(4 - i), set: set(i)}); \
+               var bad = 0; \
+               for (var n = 0; n < 1000; n++) { \
+                 var o = Object.create(p); o.length = 4; Array.prototype.shift.call(o); \
+                 if (!o[0] || o[0].v !== 3 || o[2].v !== 1) bad++; \
+                 var s = Object.create(p); s.length = 4; Array.prototype.sort.call(s, function (a, b) { return a.v - b.v; }); \
+                 for (var i = 0; i < 4; i++) if (!s[i] || s[i].v !== i + 1) bad++; \
+               } \
+               var q = {}; \
+               for (var i = 0; i < 4; i++) Object.defineProperty(q, i, {get: (function (v) { return function () { return {v: v}; }; })(4 - i), set: function () {}}); \
+               for (var n = 0; n < 20000; n++) { \
+                 var u = Object.create(q); u.length = 4; Array.prototype.unshift.call(u, 0, 0, 0, 0); \
+                 for (var i = 4; i < 8; i++) if (!u.hasOwnProperty(i) || u[i].v !== 8 - i) bad++; \
+               } \
+               for (var n = 0; n < 5000; n++) { var t = Object.create(q); t.length = 4; Array.prototype.sort.call(t); } \
+               bad";
+    let mut realm = small(limits.clone());
+    assert_eq!(realm.run(src), Ok(JsValue::Number(0.0)));
+    assert!(realm.interp().heap.collections > 0);
+    // The `z` objects change each round's garbage, so the collections fall at different points.
+    let src = "var bad = 0; \
+               for (var n = 0; n < 10000; n++) { \
+                 for (var z = 0; z < n % 7; z++) var g = {z: z}; \
+                 var j = JSON.parse('[1, 2, 3, 4, 5, 6, 7, 8]', function (k, v) { return k === '' ? v : {v: v}; }); \
+                 for (var i = 0; i < 8; i++) if (!j[i] || j[i].v !== i + 1) bad++; \
+               } bad";
+    let mut realm = small(limits.clone());
+    assert_eq!(realm.run(src), Ok(JsValue::Number(0.0)));
+    assert!(realm.interp().heap.collections > 0);
+    let src = "var k = ''; for (var i = 0; i < 300; i++) k += 'k'; \
+               var o = {}; o[k] = {toJSON: function () { var g = {x: 1}; return {a: 1}; }}; \
+               var want = '{\"' + k + '\":{\"a\":1}}'; \
+               var bad = 0; for (var n = 0; n < 4000; n++) if (JSON.stringify(o) !== want) bad++; bad";
+    let mut realm = small(limits);
+    assert_eq!(realm.run(src), Ok(JsValue::Number(0.0)));
+    assert!(realm.interp().heap.collections > 0);
+}
+
 // ---- errors ----
 
 #[test]