From 2ce6679643d1e97c83becc4d8c4a8447f91c6471 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 30 Jul 2022 19:58:50 -0500 Subject: [PATCH] avm2: Store PropertyClass in `VtableData` Calling `get_trait` copies the returned `Property`, so the caching we performed in `PropertyClass` was never actually getting used. Instead, we now store our `PropertyClass` values in a `Vec` indexed by slot id. `set_property` and `init_property` now perform coercions by going through the `VTable,` which writes the updated `PropertyClass` back into the array. --- core/src/avm2/object.rs | 23 ++++++++------ core/src/avm2/property.rs | 57 +++++++++++++++------------------ core/src/avm2/vtable.rs | 66 +++++++++++++++++++++++++++++---------- 3 files changed, 90 insertions(+), 56 deletions(-) diff --git a/core/src/avm2/object.rs b/core/src/avm2/object.rs index bff864a8b..c805a1300 100644 --- a/core/src/avm2/object.rs +++ b/core/src/avm2/object.rs @@ -142,8 +142,9 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy activation: &mut Activation<'_, 'gc, '_>, ) -> Result, Error> { match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) { - Some(Property::Slot { slot_id, class: _ }) - | Some(Property::ConstSlot { slot_id, class: _ }) => self.base().get_slot(slot_id), + Some(Property::Slot { slot_id }) | Some(Property::ConstSlot { slot_id }) => { + self.base().get_slot(slot_id) + } Some(Property::Method { disp_id }) => { if let Some(bound_method) = self.get_bound_method(disp_id) { return Ok(bound_method.into()); @@ -197,8 +198,11 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy activation: &mut Activation<'_, 'gc, '_>, ) -> Result<(), Error> { match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) { - Some(Property::Slot { slot_id, mut class }) => { - let value = class.coerce(activation, value)?; + Some(Property::Slot { slot_id }) => { + let value = self + .vtable() + .unwrap() + .coerce_trait_value(slot_id, value, activation)?; self.base_mut(activation.context.gc_context).set_slot( slot_id, value, @@ -247,9 +251,11 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy activation: &mut Activation<'_, 'gc, '_>, ) -> Result<(), Error> { match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) { - Some(Property::Slot { slot_id, mut class }) - | Some(Property::ConstSlot { slot_id, mut class }) => { - let value = class.coerce(activation, value)?; + Some(Property::Slot { slot_id }) | Some(Property::ConstSlot { slot_id }) => { + let value = self + .vtable() + .unwrap() + .coerce_trait_value(slot_id, value, activation)?; self.base_mut(activation.context.gc_context).set_slot( slot_id, value, @@ -303,8 +309,7 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy activation: &mut Activation<'_, 'gc, '_>, ) -> Result, Error> { match self.vtable().and_then(|vtable| vtable.get_trait(multiname)) { - Some(Property::Slot { slot_id, class: _ }) - | Some(Property::ConstSlot { slot_id, class: _ }) => { + Some(Property::Slot { slot_id }) | Some(Property::ConstSlot { slot_id }) => { let obj = self.base().get_slot(slot_id)?.as_callable( activation, Some(multiname), diff --git a/core/src/avm2/property.rs b/core/src/avm2/property.rs index 67c5149ad..34f17b7ce 100644 --- a/core/src/avm2/property.rs +++ b/core/src/avm2/property.rs @@ -11,22 +11,11 @@ use gc_arena::{Collect, Gc}; #[derive(Debug, Collect, Clone, Copy)] #[collect(no_drop)] -pub enum Property<'gc> { - Virtual { - get: Option, - set: Option, - }, - Method { - disp_id: u32, - }, - Slot { - slot_id: u32, - class: PropertyClass<'gc>, - }, - ConstSlot { - slot_id: u32, - class: PropertyClass<'gc>, - }, +pub enum Property { + Virtual { get: Option, set: Option }, + Method { disp_id: u32 }, + Slot { slot_id: u32 }, + ConstSlot { slot_id: u32 }, } /// The type of a `Slot`/`ConstSlot` property, represented @@ -42,7 +31,7 @@ pub enum Property<'gc> { /// Additionally, property class resolution uses special /// logic, different from normal "runtime" class resolution, /// that allows private types to be referenced. -#[derive(Debug, Collect, Clone, Copy)] +#[derive(Debug, Collect, Clone)] #[collect(no_drop)] pub enum PropertyClass<'gc> { /// The type `*` (Multiname::is_any()). This allows @@ -61,24 +50,28 @@ impl<'gc> PropertyClass<'gc> { ) -> Self { PropertyClass::Name(Gc::allocate(activation.context.gc_context, (name, unit))) } + + /// Returns `value` coerced to the type of this `PropertyClass`. + /// The bool is `true` if this `PropertyClass` was just modified + /// to cache a class resolution, and `false` if it was not modified. pub fn coerce( &mut self, activation: &mut Activation<'_, 'gc, '_>, value: Value<'gc>, - ) -> Result, Error> { - let class = match self { - PropertyClass::Class(class) => Some(*class), + ) -> Result<(Value<'gc>, bool), Error> { + let (class, changed) = match self { + PropertyClass::Class(class) => (Some(*class), false), PropertyClass::Name(gc) => { let (name, unit) = &**gc; let outcome = resolve_class_private(name, *unit, activation)?; let class = match outcome { ResolveOutcome::Class(class) => { *self = PropertyClass::Class(class); - Some(class) + (Some(class), true) } ResolveOutcome::Any => { *self = PropertyClass::Any; - None + (None, true) } ResolveOutcome::NotFound => { // FP allows a class to reference its own type in a static initializer: @@ -103,7 +96,9 @@ impl<'gc> PropertyClass<'gc> { && unit.map(|u| u.domain()) == Some(class.class_scope().domain()) { - return Ok(value); + // Even though resolution succeeded, we haven't modified + // this `PropertyClass`, so return `false` + return Ok((value, false)); } } } @@ -115,15 +110,15 @@ impl<'gc> PropertyClass<'gc> { }; class } - PropertyClass::Any => None, + PropertyClass::Any => (None, false), }; if let Some(class) = class { - value.coerce_to_type(activation, class) + Ok((value.coerce_to_type(activation, class)?, changed)) } else { // We have a type of '*' ("any"), so don't // perform any coercions - Ok(value) + Ok((value, changed)) } } } @@ -191,7 +186,7 @@ fn resolve_class_private<'gc>( } } -impl<'gc> Property<'gc> { +impl Property { pub fn new_method(disp_id: u32) -> Self { Property::Method { disp_id } } @@ -210,11 +205,11 @@ impl<'gc> Property<'gc> { } } - pub fn new_slot(slot_id: u32, class: PropertyClass<'gc>) -> Self { - Property::Slot { slot_id, class } + pub fn new_slot(slot_id: u32) -> Self { + Property::Slot { slot_id } } - pub fn new_const_slot(slot_id: u32, class: PropertyClass<'gc>) -> Self { - Property::ConstSlot { slot_id, class } + pub fn new_const_slot(slot_id: u32) -> Self { + Property::ConstSlot { slot_id } } } diff --git a/core/src/avm2/vtable.rs b/core/src/avm2/vtable.rs index 45f3fea2d..b69388de1 100644 --- a/core/src/avm2/vtable.rs +++ b/core/src/avm2/vtable.rs @@ -27,7 +27,11 @@ pub struct VTableData<'gc> { protected_namespace: Option>, - resolved_traits: PropertyMap<'gc, Property<'gc>>, + resolved_traits: PropertyMap<'gc, Property>, + + /// Stores the `PropertyClass` for each slot, + /// indexed by `slot_id` + slot_classes: Vec>, method_table: Vec>, @@ -54,6 +58,7 @@ impl<'gc> VTable<'gc> { scope: None, protected_namespace: None, resolved_traits: PropertyMap::new(), + slot_classes: vec![], method_table: vec![], default_slots: vec![], }, @@ -64,7 +69,7 @@ impl<'gc> VTable<'gc> { VTable(GcCell::allocate(mc, self.0.read().clone())) } - pub fn get_trait(self, name: &Multiname<'gc>) -> Option> { + pub fn get_trait(self, name: &Multiname<'gc>) -> Option { self.0 .read() .resolved_traits @@ -72,6 +77,26 @@ impl<'gc> VTable<'gc> { .cloned() } + /// Coerces `value` to the type of the slot with id `slot_id` + pub fn coerce_trait_value( + &self, + slot_id: u32, + value: Value<'gc>, + activation: &mut Activation<'_, 'gc, '_>, + ) -> Result, Error> { + // Drop the `write()` guard, as 'slot_class.coerce' may need to access this vtable. + let mut slot_class = { self.0.read().slot_classes[slot_id as usize].clone() }; + + let (value, changed) = slot_class.coerce(activation, value)?; + + // Calling coerce modified `PropertyClass` to cache the class lookup, + // so store the new value back in the vtable. + if changed { + self.0.write(activation.context.gc_context).slot_classes[slot_id as usize] = slot_class; + } + Ok(value) + } + pub fn has_trait(self, name: &Multiname<'gc>) -> bool { self.0 .read() @@ -175,6 +200,7 @@ impl<'gc> VTable<'gc> { if let Some(superclass_vtable) = superclass_vtable { write.resolved_traits = superclass_vtable.0.read().resolved_traits.clone(); + write.slot_classes = superclass_vtable.0.read().slot_classes.clone(); write.method_table = superclass_vtable.0.read().method_table.clone(); write.default_slots = superclass_vtable.0.read().default_slots.clone(); @@ -195,10 +221,11 @@ impl<'gc> VTable<'gc> { } } - let (resolved_traits, method_table, default_slots) = ( + let (resolved_traits, method_table, default_slots, slot_classes) = ( &mut write.resolved_traits, &mut write.method_table, &mut write.default_slots, + &mut write.slot_classes, ); for trait_data in traits { @@ -299,31 +326,38 @@ impl<'gc> VTable<'gc> { slot_id }; - let new_prop = match trait_data.kind() { + if new_slot_id as usize >= slot_classes.len() { + // We will overwrite `PropertyClass::Any` when we process the slots + // with the ids that we just skipped over. + slot_classes.resize(new_slot_id as usize + 1, PropertyClass::Any); + } + + let (new_prop, new_class) = match trait_data.kind() { TraitKind::Slot { type_name, unit, .. - } => Property::new_slot( - new_slot_id, + } => ( + Property::new_slot(new_slot_id), PropertyClass::name(activation, type_name.clone(), *unit), ), - TraitKind::Function { .. } => Property::new_slot( - new_slot_id, + TraitKind::Function { .. } => ( + Property::new_slot(new_slot_id), PropertyClass::Class(activation.avm2().classes().function), ), TraitKind::Const { type_name, unit, .. - } => Property::new_const_slot( - new_slot_id, + } => ( + Property::new_const_slot(new_slot_id), PropertyClass::name(activation, type_name.clone(), *unit), ), - TraitKind::Class { .. } => Property::new_const_slot( - new_slot_id, + TraitKind::Class { .. } => ( + Property::new_const_slot(new_slot_id), PropertyClass::Class(activation.avm2().classes().class), ), _ => unreachable!(), }; resolved_traits.insert(trait_data.name(), new_prop); + slot_classes[new_slot_id as usize] = new_class; } } } @@ -379,10 +413,10 @@ impl<'gc> VTable<'gc> { write.default_slots.push(Some(value)); let new_slot_id = write.default_slots.len() as u32 - 1; - write.resolved_traits.insert( - name, - Property::new_slot(new_slot_id, PropertyClass::Class(class)), - ); + write + .resolved_traits + .insert(name, Property::new_slot(new_slot_id)); + write.slot_classes.push(PropertyClass::Class(class)); new_slot_id }