avm1: Allow stored and virtual properties to co-exist

Currently properties added using `addProperty` overwrite existing
stored properties. However, there are some cases where the original
stored value can still be retrieved, which indicates that Flash
Player doesn't overwrite these properties internally.

As a solution, unify `Property::Stored` and `Property::Virtual` to
a single struct. This allows to simultaneously store regular data
and getter/setter on the same property. It also simplifies some
logic in `ScriptObject`.

core/src/avm1/object/script_object.rs

@@ -129,16 +129,11 @@ impl<'gc> ScriptObject<'gc> {
     /// Doesn't look up the prototype chain and ignores virtual properties, thus cannot cause
     /// any side-effects.
     pub fn get_data(&self, name: &str, activation: &mut Activation<'_, 'gc, '_>) -> Value<'gc> {
-        if let Some(Property::Stored { value, .. }) = self
+        self.0
-            .0
             .read()
             .values
             .get(name, activation.is_case_sensitive())
-        {
+            .map_or(Value::Undefined, |property| property.data())
-            value.to_owned()
-        } else {
-            Value::Undefined
-        }
     }
 
     /// Sets a data property on this object.
@@ -158,15 +153,8 @@ impl<'gc> ScriptObject<'gc> {
             .values
             .entry(name, activation.is_case_sensitive())
         {
-            Entry::Occupied(mut entry) => {
+            Entry::Occupied(mut entry) => entry.get_mut().set_data(value),
-                entry.get_mut().set(value);
+            Entry::Vacant(entry) => entry.insert(Property::new_stored(value, Attribute::empty())),
-            }
-            Entry::Vacant(entry) => {
-                entry.insert(Property::Stored {
-                    value,
-                    attributes: Attribute::empty(),
-                });
-            }
         }
         Ok(())
     }
@@ -186,8 +174,13 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
             .values
             .get(name, activation.is_case_sensitive())
         {
-            Some(Property::Virtual { get, .. }) => get.to_owned(),
+            Some(property) => {
-            Some(Property::Stored { value, .. }) => return Some(Ok(value.to_owned())),
+                if let Some(getter) = property.getter() {
+                    getter
+                } else {
+                    return Some(Ok(property.data()));
+                }
+            }
             None => return None,
         };
 
@@ -249,12 +242,13 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
             .values
             .entry(name, activation.is_case_sensitive())
         {
-            Entry::Occupied(mut entry) => entry.get_mut().set(value),
+            Entry::Occupied(mut entry) => {
+                let entry = entry.get_mut();
+                entry.set_data(value);
+                entry.setter()
+            }
             Entry::Vacant(entry) => {
-                entry.insert(Property::Stored {
+                entry.insert(Property::new_stored(value, Attribute::empty()));
-                    value,
-                    attributes: Attribute::empty(),
-                });
                 None
             }
         };
@@ -297,18 +291,14 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
     fn call_setter(
         &self,
         name: &str,
-        value: Value<'gc>,
+        _value: Value<'gc>,
         activation: &mut Activation<'_, 'gc, '_>,
     ) -> Option<Object<'gc>> {
-        match self
+        self.0
-            .0
+            .read()
-            .write(activation.context.gc_context)
             .values
-            .get_mut(name, activation.is_case_sensitive())
+            .get(name, activation.is_case_sensitive())
-        {
+            .and_then(|property| property.setter())
-            Some(propref) if propref.is_virtual() => propref.set(value),
-            _ => None,
-        }
     }
 
     fn create_bare_object(
@@ -341,38 +331,33 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
         &self,
         gc_context: MutationContext<'gc, '_>,
         name: &str,
-        get: Object<'gc>,
+        getter: Object<'gc>,
-        set: Option<Object<'gc>>,
+        setter: Option<Object<'gc>>,
         attributes: Attribute,
     ) {
-        self.0.write(gc_context).values.insert(
+        match self.0.write(gc_context).values.entry(name, false) {
-            name,
+            Entry::Occupied(mut entry) => entry.get_mut().set_virtual(getter, setter),
-            Property::Virtual {
+            Entry::Vacant(entry) => entry.insert(Property::new_virtual(getter, setter, attributes)),
-                get,
+        }
-                set,
-                attributes,
-            },
-            false,
-        );
     }
 
     fn add_property_with_case(
         &self,
         activation: &mut Activation<'_, 'gc, '_>,
         name: &str,
-        get: Object<'gc>,
+        getter: Object<'gc>,
-        set: Option<Object<'gc>>,
+        setter: Option<Object<'gc>>,
         attributes: Attribute,
     ) {
-        self.0.write(activation.context.gc_context).values.insert(
+        match self
-            name,
+            .0
-            Property::Virtual {
+            .write(activation.context.gc_context)
-                get,
+            .values
-                set,
+            .entry(name, activation.is_case_sensitive())
-                attributes,
+        {
-            },
+            Entry::Occupied(mut entry) => entry.get_mut().set_virtual(getter, setter),
-            activation.is_case_sensitive(),
+            Entry::Vacant(entry) => entry.insert(Property::new_virtual(getter, setter, attributes)),
-        );
+        }
     }
 
     fn watch(
@@ -390,12 +375,11 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
     }
 
     fn unwatch(&self, activation: &mut Activation<'_, 'gc, '_>, name: Cow<str>) -> bool {
-        let old = self
+        self.0
-            .0
             .write(activation.context.gc_context)
             .watchers
-            .remove(name.as_ref(), activation.is_case_sensitive());
+            .remove(name.as_ref(), activation.is_case_sensitive())
-        old.is_some()
+            .is_some()
     }
 
     fn define_value(
@@ -408,7 +392,7 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
         self.0
             .write(gc_context)
             .values
-            .insert(name, Property::Stored { value, attributes }, true);
+            .insert(name, Property::new_stored(value, attributes), true);
     }
 
     fn set_attributes(
@@ -459,30 +443,20 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
     }
 
     fn has_own_virtual(&self, activation: &mut Activation<'_, 'gc, '_>, name: &str) -> bool {
-        if let Some(slot) = self
+        self.0
-            .0
             .read()
             .values
             .get(name, activation.is_case_sensitive())
-        {
+            .map_or(false, |property| property.is_virtual())
-            slot.is_virtual()
-        } else {
-            false
-        }
     }
 
     /// Checks if a named property appears when enumerating the object.
     fn is_property_enumerable(&self, activation: &mut Activation<'_, 'gc, '_>, name: &str) -> bool {
-        if let Some(prop) = self
+        self.0
-            .0
             .read()
             .values
             .get(name, activation.is_case_sensitive())
-        {
+            .map_or(false, |property| property.is_enumerable())
-            prop.is_enumerable()
-        } else {
-            false
-        }
     }
 
     /// Enumerate the object.
@@ -493,14 +467,13 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> {
             Vec::new()
         };
         let mut out_keys = vec![];
-        let object = self.0.read();
 
         // Prototype keys come first.
-        out_keys.extend(proto_keys.into_iter().filter(|k| {
+        out_keys.extend(
-            !object
+            proto_keys
-                .values
+                .into_iter()
-                .contains_key(k, activation.is_case_sensitive())
+                .filter(|k| !self.has_own_property(activation, k)),
-        }));
+        );
 
         // Then our own keys.
         out_keys.extend(self.0.read().values.iter().filter_map(move |(k, p)| {

core/src/avm1/property.rs

@@ -17,113 +17,99 @@ bitflags! {
     }
 }
 
-#[allow(clippy::large_enum_variant)]
 #[derive(Clone, Collect)]
 #[collect(no_drop)]
-pub enum Property<'gc> {
+pub struct Property<'gc> {
-    Virtual {
+    data: Value<'gc>,
-        get: Object<'gc>,
+    getter: Option<Object<'gc>>,
-        set: Option<Object<'gc>>,
+    setter: Option<Object<'gc>>,
-        attributes: Attribute,
+    attributes: Attribute,
-    },
-    Stored {
-        value: Value<'gc>,
-        attributes: Attribute,
-    },
 }
 
 impl<'gc> Property<'gc> {
-    /// Set a property slot.
+    pub fn new_stored(data: Value<'gc>, attributes: Attribute) -> Self {
-    ///
+        Self {
-    /// This function may return an `Executable` of the property's virtual
+            data,
-    /// function, if any happen to exist. It should be resolved, and its value
+            getter: None,
-    /// discarded.
+            setter: None,
-    pub fn set(&mut self, new_value: impl Into<Value<'gc>>) -> Option<Object<'gc>> {
+            attributes,
-        match self {
-            Property::Virtual {
-                set: Some(function),
-                ..
-            } => Some(function.to_owned()),
-            Property::Stored {
-                value, attributes, ..
-            } => {
-                if !attributes.contains(Attribute::READ_ONLY) {
-                    *value = new_value.into();
-                }
-
-                None
-            }
-            _ => None,
         }
     }
 
+    pub fn new_virtual(
+        getter: Object<'gc>,
+        setter: Option<Object<'gc>>,
+        attributes: Attribute,
+    ) -> Self {
+        Self {
+            data: Value::Undefined,
+            getter: Some(getter),
+            setter,
+            attributes,
+        }
+    }
+
+    pub fn data(&self) -> Value<'gc> {
+        self.data
+    }
+
+    pub fn getter(&self) -> Option<Object<'gc>> {
+        self.getter
+    }
+
+    pub fn setter(&self) -> Option<Object<'gc>> {
+        self.setter
+    }
+
+    /// Store data on this property, ignoring virtual setters.
+    ///
+    /// Read-only properties are not affected.
+    pub fn set_data(&mut self, data: Value<'gc>) {
+        if self.is_overwritable() {
+            self.data = data;
+        }
+    }
+
+    /// Make this property virtual by attaching a getter/setter to it.
+    pub fn set_virtual(&mut self, getter: Object<'gc>, setter: Option<Object<'gc>>) {
+        self.getter = Some(getter);
+        self.setter = setter;
+    }
+
     /// List this property's attributes.
     pub fn attributes(&self) -> Attribute {
-        match self {
+        self.attributes
-            Property::Virtual { attributes, .. } => *attributes,
-            Property::Stored { attributes, .. } => *attributes,
-        }
     }
 
     /// Re-define this property's attributes.
-    pub fn set_attributes(&mut self, new_attributes: Attribute) {
+    pub fn set_attributes(&mut self, attributes: Attribute) {
-        match self {
+        self.attributes = attributes;
-            Property::Virtual {
-                ref mut attributes, ..
-            } => *attributes = new_attributes,
-            Property::Stored {
-                ref mut attributes, ..
-            } => *attributes = new_attributes,
-        }
-    }
-
-    pub fn can_delete(&self) -> bool {
-        match self {
-            Property::Virtual { attributes, .. } => !attributes.contains(Attribute::DONT_DELETE),
-            Property::Stored { attributes, .. } => !attributes.contains(Attribute::DONT_DELETE),
-        }
     }
 
     pub fn is_enumerable(&self) -> bool {
-        match self {
+        !self.attributes.contains(Attribute::DONT_ENUM)
-            Property::Virtual { attributes, .. } => !attributes.contains(Attribute::DONT_ENUM),
+    }
-            Property::Stored { attributes, .. } => !attributes.contains(Attribute::DONT_ENUM),
+
-        }
+    pub fn can_delete(&self) -> bool {
+        !self.attributes.contains(Attribute::DONT_DELETE)
     }
 
-    #[allow(dead_code)]
     pub fn is_overwritable(&self) -> bool {
-        match self {
+        !self.attributes.contains(Attribute::READ_ONLY)
-            Property::Virtual {
-                attributes, set, ..
-            } => !attributes.contains(Attribute::READ_ONLY) && !set.is_none(),
-            Property::Stored { attributes, .. } => !attributes.contains(Attribute::READ_ONLY),
-        }
     }
 
     pub fn is_virtual(&self) -> bool {
-        matches!(self, Property::Virtual { .. })
+        self.getter.is_some()
     }
 }
 
 impl fmt::Debug for Property<'_> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
+        f.debug_struct("Property")
-            Property::Virtual {
+            .field("data", &self.data)
-                get: _,
+            .field("getter", &self.getter)
-                set,
+            .field("setter", &self.setter)
-                attributes,
+            .field("attributes", &self.attributes)
-            } => f
+            .finish()
-                .debug_struct("Property::Virtual")
-                .field("get", &true)
-                .field("set", &set.is_some())
-                .field("attributes", &attributes)
-                .finish(),
-            Property::Stored { value, attributes } => f
-                .debug_struct("Property::Stored")
-                .field("value", &value)
-                .field("attributes", &attributes)
-                .finish(),
-        }
     }
 }