Use lock for map iteration (#1366)

boa/src/builtins/map/map_iterator.rs

@@ -7,6 +7,8 @@ use crate::{
 };
 use gc::{Finalize, Trace};
 
+use super::{ordered_map::MapLock, Map};
+
 #[derive(Debug, Clone, Finalize, Trace)]
 pub enum MapIterationKind {
     Key,
@@ -25,18 +27,21 @@ pub struct MapIterator {
     iterated_map: Value,
     map_next_index: usize,
     map_iteration_kind: MapIterationKind,
+    lock: MapLock,
 }
 
 impl MapIterator {
     pub(crate) const NAME: &'static str = "MapIterator";
 
     /// Constructs a new `MapIterator`, that will iterate over `map`, starting at index 0
-    fn new(map: Value, kind: MapIterationKind) -> Self {
+    fn new(map: Value, kind: MapIterationKind, context: &mut Context) -> Result<Self> {
-        MapIterator {
+        let lock = Map::lock(&map, context)?;
+        Ok(MapIterator {
             iterated_map: map,
             map_next_index: 0,
             map_iteration_kind: kind,
-        }
+            lock,
+        })
     }
 
     /// Abstract operation CreateMapIterator( map, kind )
@@ -48,17 +53,17 @@ impl MapIterator {
     ///
     /// [spec]: https://www.ecma-international.org/ecma-262/11.0/index.html#sec-createmapiterator
     pub(crate) fn create_map_iterator(
-        context: &Context,
+        context: &mut Context,
         map: Value,
         kind: MapIterationKind,
-    ) -> Value {
+    ) -> Result<Value> {
         let map_iterator = Value::new_object(context);
-        map_iterator.set_data(ObjectData::MapIterator(Self::new(map, kind)));
+        map_iterator.set_data(ObjectData::MapIterator(Self::new(map, kind, context)?));
         map_iterator
             .as_object()
             .expect("map iterator object")
             .set_prototype_instance(context.iterator_prototypes().map_iterator().into());
-        map_iterator
+        Ok(map_iterator)
     }
 
     /// %MapIteratorPrototype%.next( )
@@ -83,7 +88,7 @@ impl MapIterator {
 
                 if let Value::Object(ref object) = m {
                     if let Some(entries) = object.borrow().as_map_ref() {
-                        let num_entries = entries.len();
+                        let num_entries = entries.full_len();
                         while index < num_entries {
                             let e = entries.get_index(index);
                             index += 1;

boa/src/builtins/map/mod.rs

@@ -24,12 +24,14 @@ use ordered_map::OrderedMap;
 pub mod map_iterator;
 use map_iterator::{MapIterationKind, MapIterator};
 
+use self::ordered_map::MapLock;
+
 pub mod ordered_map;
 #[cfg(test)]
 mod tests;
 
 #[derive(Debug, Clone)]
-pub(crate) struct Map(OrderedMap<Value, Value>);
+pub(crate) struct Map(OrderedMap<Value>);
 
 impl BuiltIn for Map {
     const NAME: &'static str = "Map";
@@ -198,11 +200,7 @@ impl Map {
     /// [spec]: https://www.ecma-international.org/ecma-262/11.0/index.html#sec-map.prototype.entries
     /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/entries
     pub(crate) fn entries(this: &Value, _: &[Value], context: &mut Context) -> Result<Value> {
-        Ok(MapIterator::create_map_iterator(
+        MapIterator::create_map_iterator(context, this.clone(), MapIterationKind::KeyAndValue)
-            context,
-            this.clone(),
-            MapIterationKind::KeyAndValue,
-        ))
     }
 
     /// `Map.prototype.keys()`
@@ -216,11 +214,7 @@ impl Map {
     /// [spec]: https://tc39.es/ecma262/#sec-map.prototype.keys
     /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/keys
     pub(crate) fn keys(this: &Value, _: &[Value], context: &mut Context) -> Result<Value> {
-        Ok(MapIterator::create_map_iterator(
+        MapIterator::create_map_iterator(context, this.clone(), MapIterationKind::Key)
-            context,
-            this.clone(),
-            MapIterationKind::Key,
-        ))
     }
 
     /// Helper function to set the size property.
@@ -392,7 +386,9 @@ impl Map {
 
         let mut index = 0;
 
-        while index < Map::get_size(this, context)? {
+        let lock = Map::lock(this, context)?;
+
+        while index < Map::get_full_len(this, context)? {
             let arguments = if let Value::Object(ref object) = this {
                 let object = object.borrow();
                 if let Some(map) = object.as_map_ref() {
@@ -415,15 +411,31 @@ impl Map {
             index += 1;
         }
 
+        drop(lock);
+
         Ok(Value::Undefined)
     }
 
-    /// Helper function to get the size of the map.
+    /// Helper function to get the full size of the map.
-    fn get_size(map: &Value, context: &mut Context) -> Result<usize> {
+    fn get_full_len(map: &Value, context: &mut Context) -> Result<usize> {
         if let Value::Object(ref object) = map {
             let object = object.borrow();
             if let Some(map) = object.as_map_ref() {
-                Ok(map.len())
+                Ok(map.full_len())
+            } else {
+                Err(context.construct_type_error("'this' is not a Map"))
+            }
+        } else {
+            Err(context.construct_type_error("'this' is not a Map"))
+        }
+    }
+
+    /// Helper function to lock the map.
+    fn lock(map: &Value, context: &mut Context) -> Result<MapLock> {
+        if let Value::Object(ref object) = map {
+            let mut map = object.borrow_mut();
+            if let Some(map) = map.as_map_mut() {
+                Ok(map.lock(object.clone()))
             } else {
                 Err(context.construct_type_error("'this' is not a Map"))
             }
@@ -443,11 +455,7 @@ impl Map {
     /// [spec]: https://tc39.es/ecma262/#sec-map.prototype.values
     /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/values
     pub(crate) fn values(this: &Value, _: &[Value], context: &mut Context) -> Result<Value> {
-        Ok(MapIterator::create_map_iterator(
+        MapIterator::create_map_iterator(context, this.clone(), MapIterationKind::Value)
-            context,
-            this.clone(),
-            MapIterationKind::Value,
-        ))
     }
 
     /// Helper function to get a key-value pair from an array.

boa/src/builtins/map/ordered_map.rs

@@ -1,63 +1,109 @@
-use crate::gc::{custom_trace, Finalize, Trace};
+use crate::{
-use indexmap::{map::IntoIter, map::Iter, map::IterMut, IndexMap};
+    gc::{custom_trace, Finalize, Trace},
+    object::GcObject,
+    Value,
+};
+use indexmap::{Equivalent, IndexMap};
 use std::{
     collections::hash_map::RandomState,
     fmt::Debug,
-    hash::{BuildHasher, Hash},
+    hash::{BuildHasher, Hash, Hasher},
 };
 
+#[derive(PartialEq, Eq, Clone, Debug)]
+enum MapKey {
+    Key(Value),
+    Empty(usize), // Necessary to ensure empty keys are still unique.
+}
+
+// This ensures that a MapKey::Key(value) hashes to the same as value. The derived PartialEq implementation still holds.
+#[allow(clippy::derive_hash_xor_eq)]
+impl Hash for MapKey {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        match self {
+            MapKey::Key(v) => v.hash(state),
+            MapKey::Empty(e) => e.hash(state),
+        }
+    }
+}
+
+impl Equivalent<MapKey> for Value {
+    fn equivalent(&self, key: &MapKey) -> bool {
+        match key {
+            MapKey::Key(v) => v == self,
+            _ => false,
+        }
+    }
+}
+
 /// A newtype wrapping indexmap::IndexMap
 #[derive(Clone)]
-pub struct OrderedMap<K, V, S = RandomState>(IndexMap<K, V, S>)
+pub struct OrderedMap<V, S = RandomState> {
-where
+    map: IndexMap<MapKey, Option<V>, S>,
-    K: Hash + Eq;
+    lock: u32,
+    empty_count: usize,
+}
 
-impl<K: Eq + Hash + Trace, V: Trace, S: BuildHasher> Finalize for OrderedMap<K, V, S> {}
+impl<V: Trace, S: BuildHasher> Finalize for OrderedMap<V, S> {}
-unsafe impl<K: Eq + Hash + Trace, V: Trace, S: BuildHasher> Trace for OrderedMap<K, V, S> {
+unsafe impl<V: Trace, S: BuildHasher> Trace for OrderedMap<V, S> {
     custom_trace!(this, {
-        for (k, v) in this.0.iter() {
+        for (k, v) in this.map.iter() {
-            mark(k);
+            if let MapKey::Key(key) = k {
+                mark(key);
+            }
             mark(v);
         }
     });
 }
 
-impl<K: Hash + Eq + Debug, V: Debug> Debug for OrderedMap<K, V> {
+impl<V: Debug> Debug for OrderedMap<V> {
     fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        self.0.fmt(formatter)
+        self.map.fmt(formatter)
     }
 }
 
-impl<K: Hash + Eq, V> Default for OrderedMap<K, V> {
+impl<V> Default for OrderedMap<V> {
     fn default() -> Self {
         Self::new()
     }
 }
 
-impl<K, V> OrderedMap<K, V>
+impl<V> OrderedMap<V> {
-where
-    K: Hash + Eq,
-{
     pub fn new() -> Self {
-        OrderedMap(IndexMap::new())
+        OrderedMap {
+            map: IndexMap::new(),
+            lock: 0,
+            empty_count: 0,
+        }
     }
 
     pub fn with_capacity(capacity: usize) -> Self {
-        OrderedMap(IndexMap::with_capacity(capacity))
+        OrderedMap {
+            map: IndexMap::with_capacity(capacity),
+            lock: 0,
+            empty_count: 0,
+        }
     }
 
-    /// Return the number of key-value pairs in the map.
+    /// Return the number of key-value pairs in the map, including empty values.
+    ///
+    /// Computes in **O(1)** time.
+    pub fn full_len(&self) -> usize {
+        self.map.len()
+    }
+
+    /// Gets the number of key-value pairs in the map, not including empty values.
     ///
     /// Computes in **O(1)** time.
     pub fn len(&self) -> usize {
-        self.0.len()
+        self.map.len() - self.empty_count
     }
 
     /// Returns true if the map contains no elements.
     ///
     /// Computes in **O(1)** time.
     pub fn is_empty(&self) -> bool {
-        self.0.len() == 0
+        self.len() == 0
     }
 
     /// Insert a key-value pair in the map.
@@ -70,8 +116,8 @@ where
     /// inserted, last in order, and `None` is returned.
     ///
     /// Computes in **O(1)** time (amortized average).
-    pub fn insert(&mut self, key: K, value: V) -> Option<V> {
+    pub fn insert(&mut self, key: Value, value: V) -> Option<V> {
-        self.0.insert(key, value)
+        self.map.insert(MapKey::Key(key), Some(value)).flatten()
     }
 
     /// Remove the key-value pair equivalent to `key` and return
@@ -84,70 +130,89 @@ where
     /// Return `None` if `key` is not in map.
     ///
     /// Computes in **O(n)** time (average).
-    pub fn remove(&mut self, key: &K) -> Option<V> {
+    pub fn remove(&mut self, key: &Value) -> Option<V> {
-        self.0.shift_remove(key)
+        if self.lock == 0 {
+            self.map.shift_remove(key).flatten()
+        } else if self.map.contains_key(key) {
+            self.map.insert(MapKey::Empty(self.empty_count), None);
+            self.empty_count += 1;
+            self.map.swap_remove(key).flatten()
+        } else {
+            None
+        }
     }
 
     /// Return a reference to the value stored for `key`, if it is present,
     /// else `None`.
     ///
     /// Computes in **O(1)** time (average).
-    pub fn get(&self, key: &K) -> Option<&V> {
+    pub fn get(&self, key: &Value) -> Option<&V> {
-        self.0.get(key)
+        self.map.get(key).map(Option::as_ref).flatten()
     }
 
     /// Get a key-value pair by index
-    /// Valid indices are 0 <= index < self.len()
+    /// Valid indices are 0 <= index < self.full_len()
     /// Computes in O(1) time.
-    pub fn get_index(&self, index: usize) -> Option<(&K, &V)> {
+    pub fn get_index(&self, index: usize) -> Option<(&Value, &V)> {
-        self.0.get_index(index)
+        if let (MapKey::Key(key), Some(value)) = self.map.get_index(index)? {
+            Some((key, value))
+        } else {
+            None
+        }
     }
 
     /// Return an iterator over the key-value pairs of the map, in their order
-    pub fn iter(&self) -> Iter<'_, K, V> {
+    pub fn iter(&self) -> impl Iterator<Item = (&Value, &V)> {
-        self.0.iter()
+        self.map.iter().filter_map(|o| {
+            if let (MapKey::Key(key), Some(value)) = o {
+                Some((key, value))
+            } else {
+                None
+            }
+        })
     }
 
     /// Return `true` if an equivalent to `key` exists in the map.
     ///
     /// Computes in **O(1)** time (average).
-    pub fn contains_key(&self, key: &K) -> bool {
+    pub fn contains_key(&self, key: &Value) -> bool {
-        self.0.contains_key(key)
+        self.map.contains_key(key)
+    }
+
+    /// Increases the lock counter and returns a lock object that will decrement the counter when dropped.
+    ///
+    /// This allows objects to be removed from the map during iteration without affecting the indexes until the iteration has completed.
+    pub(crate) fn lock(&mut self, map: GcObject) -> MapLock {
+        self.lock += 1;
+        MapLock(map)
     }
-}
 
-impl<'a, K, V, S> IntoIterator for &'a OrderedMap<K, V, S>
+    /// Decreases the lock counter and, if 0, removes all empty entries.
-where
+    fn unlock(&mut self) {
-    K: Hash + Eq,
+        self.lock -= 1;
-    S: BuildHasher,
+        if self.lock == 0 {
-{
+            self.map.retain(|k, _| matches!(k, MapKey::Key(_)));
-    type Item = (&'a K, &'a V);
+            self.empty_count = 0;
-    type IntoIter = Iter<'a, K, V>;
+        }
-    fn into_iter(self) -> Self::IntoIter {
-        self.0.iter()
     }
 }
 
-impl<'a, K, V, S> IntoIterator for &'a mut OrderedMap<K, V, S>
+/// Increases the lock count of the map for the lifetime of the guard. This should not be dropped until iteration has completed.
-where
+#[derive(Debug, Trace)]
-    K: Hash + Eq,
+pub(crate) struct MapLock(GcObject);
-    S: BuildHasher,
+
-{
+impl Clone for MapLock {
-    type Item = (&'a K, &'a mut V);
+    fn clone(&self) -> Self {
-    type IntoIter = IterMut<'a, K, V>;
+        let mut map = self.0.borrow_mut();
-    fn into_iter(self) -> Self::IntoIter {
+        let map = map.as_map_mut().expect("MapLock does not point to a map");
-        self.0.iter_mut()
+        map.lock(self.0.clone())
     }
 }
 
-impl<K, V, S> IntoIterator for OrderedMap<K, V, S>
+impl Finalize for MapLock {
-where
+    fn finalize(&self) {
-    K: Hash + Eq,
+        let mut map = self.0.borrow_mut();
-    S: BuildHasher,
+        let map = map.as_map_mut().expect("MapLock does not point to a map");
-{
+        map.unlock();
-    type Item = (K, V);
-    type IntoIter = IntoIter<K, V>;
-    fn into_iter(self) -> IntoIter<K, V> {
-        self.0.into_iter()
     }
 }

boa/src/builtins/map/tests.rs

@@ -354,3 +354,53 @@ fn not_a_function() {
         "\"TypeError: calling a builtin Map constructor without new is forbidden\""
     );
 }
+
+#[test]
+fn for_each_delete() {
+    let mut context = Context::new();
+    let init = r#"
+        let map = new Map([[0, "a"], [1, "b"], [2, "c"]]);
+        let result = [];
+        map.forEach(function(value, key) {
+            if (key === 0) {
+                map.delete(0);
+                map.set(3, "d");
+            }
+            result.push([key, value]);
+        })
+    "#;
+    forward(&mut context, init);
+    assert_eq!(forward(&mut context, "result[0][0]"), "0");
+    assert_eq!(forward(&mut context, "result[0][1]"), "\"a\"");
+    assert_eq!(forward(&mut context, "result[1][0]"), "1");
+    assert_eq!(forward(&mut context, "result[1][1]"), "\"b\"");
+    assert_eq!(forward(&mut context, "result[2][0]"), "2");
+    assert_eq!(forward(&mut context, "result[2][1]"), "\"c\"");
+    assert_eq!(forward(&mut context, "result[3][0]"), "3");
+    assert_eq!(forward(&mut context, "result[3][1]"), "\"d\"");
+}
+
+#[test]
+fn for_of_delete() {
+    let mut context = Context::new();
+    let init = r#"
+        let map = new Map([[0, "a"], [1, "b"], [2, "c"]]);
+        let result = [];
+        for (a of map) {
+            if (a[0] === 0) {
+                map.delete(0);
+                map.set(3, "d");
+            }
+            result.push([a[0], a[1]]);
+        }
+    "#;
+    forward(&mut context, init);
+    assert_eq!(forward(&mut context, "result[0][0]"), "0");
+    assert_eq!(forward(&mut context, "result[0][1]"), "\"a\"");
+    assert_eq!(forward(&mut context, "result[1][0]"), "1");
+    assert_eq!(forward(&mut context, "result[1][1]"), "\"b\"");
+    assert_eq!(forward(&mut context, "result[2][0]"), "2");
+    assert_eq!(forward(&mut context, "result[2][1]"), "\"c\"");
+    assert_eq!(forward(&mut context, "result[3][0]"), "3");
+    assert_eq!(forward(&mut context, "result[3][1]"), "\"d\"");
+}

boa/src/object/mod.rs

@@ -83,7 +83,7 @@ pub struct Object {
 pub enum ObjectData {
     Array,
     ArrayIterator(ArrayIterator),
-    Map(OrderedMap<Value, Value>),
+    Map(OrderedMap<Value>),
     MapIterator(MapIterator),
     RegExp(Box<RegExp>),
     RegExpStringIterator(RegExpStringIterator),
@@ -354,7 +354,7 @@ impl Object {
     }
 
     #[inline]
-    pub fn as_map_ref(&self) -> Option<&OrderedMap<Value, Value>> {
+    pub fn as_map_ref(&self) -> Option<&OrderedMap<Value>> {
         match self.data {
             ObjectData::Map(ref map) => Some(map),
             _ => None,
@@ -362,7 +362,7 @@ impl Object {
     }
 
     #[inline]
-    pub fn as_map_mut(&mut self) -> Option<&mut OrderedMap<Value, Value>> {
+    pub fn as_map_mut(&mut self) -> Option<&mut OrderedMap<Value>> {
         match &mut self.data {
             ObjectData::Map(map) => Some(map),
             _ => None,