avm2: Properly handle Dictionary enumerants

Previously, there was an off-by-one bug in `get_enumerant_name`,
which caused us to produce a spurious 'null' as a key.
However, the 'dictionary_foreach' test only checked that certain
keys were present, so the presence of an additional key didn't break
the test.

This commit makes Dictionary enumerants behave in the same way as
Array enumerants - all of the object-specific enumerants
(in this case, the non-primitive dicitonar keys) come first,
followed by 'base' enumerants from ScriptObject (in this case,
primtive/String keys). Additionally, `setPropertyIsEnumerable` is now
ignored for `Dictionary`, consistent with Flash's behavior.

The `dictionary_foreach` test is updated to print out all of
the keys when inspecting the dictionary. Since the enumeration
order is unstable (under both Flash and Ruffle) due to the dependency
on pointer hashing, the test sorts the keys before printing them.
This ensures that we get stable output which is consistent between
Ruffle and Flash.

core/src/avm2/object/array_object.rs

@@ -189,10 +189,10 @@ impl<'gc> TObject<'gc> for ArrayObject<'gc> {
         _activation: &mut Activation<'_, 'gc, '_>,
     ) -> Result<Option<u32>, Error> {
         let read = self.0.read();
-        let last_enumerant = read.base.get_last_enumerant();
+        let num_enumerants = read.base.num_enumerants();
         let array_length = read.array.length() as u32;
 
-        if last_index < last_enumerant + array_length {
+        if last_index < num_enumerants + array_length {
             Ok(Some(last_index.saturating_add(1)))
         } else {
             Ok(None)

core/src/avm2/object/dictionary_object.rs

@@ -5,6 +5,7 @@ use crate::avm2::object::script_object::ScriptObjectData;
 use crate::avm2::object::{ClassObject, Object, ObjectPtr, TObject};
 use crate::avm2::value::Value;
 use crate::avm2::Error;
+use crate::string::AvmString;
 use fnv::FnvHashMap;
 use gc_arena::{Collect, GcCell, MutationContext};
 use std::cell::{Ref, RefMut};
@@ -104,10 +105,10 @@ impl<'gc> TObject<'gc> for DictionaryObject<'gc> {
         _activation: &mut Activation<'_, 'gc, '_>,
     ) -> Result<Option<u32>, Error> {
         let read = self.0.read();
-        let last_enumerant = read.base.get_last_enumerant();
+        let num_enumerants = read.base.num_enumerants();
         let object_space_length = read.object_space.keys().len() as u32;
 
-        if last_index < last_enumerant + object_space_length {
+        if last_index < num_enumerants + object_space_length {
             Ok(Some(last_index.saturating_add(1)))
         } else {
             Ok(None)
@@ -120,23 +121,29 @@ impl<'gc> TObject<'gc> for DictionaryObject<'gc> {
         _activation: &mut Activation<'_, 'gc, '_>,
     ) -> Result<Value<'gc>, Error> {
         let read = self.0.read();
-        let last_enumerant = read.base.get_last_enumerant();
-
-        if index < last_enumerant {
-            Ok(read
-                .base
-                .get_enumerant_name(index)
-                .unwrap_or(Value::Undefined))
-        } else {
-            let object_space_index = index.saturating_sub(last_enumerant);
-
-            Ok(read
-                .object_space
-                .keys()
-                .nth(object_space_index as usize)
-                .cloned()
+        let object_space_len = read.object_space.keys().len() as u32;
+        if object_space_len >= index {
+            Ok(index
+                .checked_sub(1)
+                .and_then(|index| read.object_space.keys().nth(index as usize).cloned())
                 .map(|v| v.into())
                 .unwrap_or(Value::Undefined))
+        } else {
+            Ok(read
+                .base
+                .get_enumerant_name(index - object_space_len)
+                .unwrap_or(Value::Undefined))
         }
     }
+
+    // Calling `setPropertyIsEnumerable` on a `Dictionary` has no effect -
+    // stringified properties are always enumerable.
+    fn set_local_property_is_enumerable(
+        &self,
+        _mc: MutationContext<'gc, '_>,
+        _name: AvmString<'gc>,
+        _is_enumerable: bool,
+    ) -> Result<(), Error> {
+        Ok(())
+    }
 }

core/src/avm2/object/script_object.rs

@@ -396,11 +396,8 @@ impl<'gc> ScriptObjectData<'gc> {
         Ok(())
     }
 
-    /// Get the end of (standard) enumerant space.
-    ///
-    /// Intended for objects that need to extend enumerant space. The index
-    /// returned is guaranteed to be unused by the base enumerant list.
-    pub fn get_last_enumerant(&self) -> u32 {
+    /// Gets the number of (standard) enumerants.
+    pub fn num_enumerants(&self) -> u32 {
         self.enumerants.len() as u32
     }
 

tests/tests/swfs/avm2/dictionary_foreach/Test.as

@@ -5,9 +5,32 @@
 
 import flash.utils.Dictionary;
 
+function printKeys(dict:Dictionary):void {
+	var keys:Array = new Array();
+	for (var k in dict) {
+		keys.push(k);
+	}
+	keys.sort();
+	trace("Keys: " + keys);
+}
+
 trace("///var a = new Dictionary()");
 var a = new Dictionary();
 
+a[new String("foo")] = "The value";
+trace("Different string: " + a[new String("foo")]);
+
+var firstKey = new Object();
+a[firstKey] = "Testing";
+a[1234567] = true;
+a.setPropertyIsEnumerable(1234567, false);
+trace("Is existent property enumerable: " + a.propertyIsEnumerable(1234567));
+trace("Is nonexistent property enumerable " + a.propertyIsEnumerable(new Object()));
+
+a.setPropertyIsEnumerable(firstKey, false);
+trace("Showing first key");
+printKeys(a);
+
 trace("///a[\"key\"] = 5");
 a["key"] = 5;
 
@@ -76,32 +99,8 @@ a["false"] = "stringy false";
 trace('///a[a] = a');
 a[a] = a;
 
-var has_key2 = false;
-var has_key3 = false;
-var has_key4 = false;
-
 trace("/// (enumerating object keys...)");
-for (var k in a) {
-	if (k === key2) {
-		has_key2 = true;
-	} else if (k === key3) {
-		has_key3 = true;
-	} else if (k === key4) {
-		has_key4 = true;
-	}
-}
-
-if (has_key2) {
-	trace("/// (Found key2!)");
-}
-
-if (has_key3) {
-	trace("/// (Found key3!)");
-}
-
-if (has_key4) {
-	trace("/// (Found key4!)");
-}
+printKeys(a);
 
 trace("///a.setPropertyIsEnumerable(key2, false);");
 a.setPropertyIsEnumerable(key2, false);
@@ -112,29 +111,5 @@ a.setPropertyIsEnumerable(key3, false);
 trace("///a.setPropertyIsEnumerable(key4, false);");
 a.setPropertyIsEnumerable(key4, false);
 
-has_key2 = false;
-has_key3 = false;
-has_key4 = false;
-
 trace("/// (enumerating object keys...)");
-for (var k in a) {
-	if (k === key2) {
-		has_key2 = true;
-	} else if (k === key3) {
-		has_key3 = true;
-	} else if (k === key4) {
-		has_key4 = true;
-	}
-}
-
-if (has_key2) {
-	trace("/// (Found key2!)");
-}
-
-if (has_key3) {
-	trace("/// (Found key3!)");
-}
-
-if (has_key4) {
-	trace("/// (Found key4!)");
-}
+printKeys(a);

tests/tests/swfs/avm2/dictionary_foreach/output.txt

@@ -1,4 +1,9 @@
 ///var a = new Dictionary()
+Different string: The value
+Is existent property enumerable: true
+Is nonexistent property enumerable false
+Showing first key
+Keys: 1234567,[object Object],foo
 ///a["key"] = 5
 ///a["key"]
 5
@@ -25,13 +30,9 @@
 ///a["false"] = "stringy false"
 ///a[a] = a
 /// (enumerating object keys...)
-/// (Found key2!)
-/// (Found key3!)
-/// (Found key4!)
+Keys: 1.123,1234567,13,[object Dictionary],[object Object],[object Test],[object Test],false,foo,key,key3,key4,key4,null,true,undefined
 ///a.setPropertyIsEnumerable(key2, false);
 ///a.setPropertyIsEnumerable(key3, false);
 ///a.setPropertyIsEnumerable(key4, false);
 /// (enumerating object keys...)
-/// (Found key2!)
-/// (Found key3!)
-/// (Found key4!)
+Keys: 1.123,1234567,13,[object Dictionary],[object Object],[object Test],[object Test],false,foo,key,key3,key4,key4,null,true,undefined