From 54b792ef3acf1d39e6b9ba08e92abbb646717aa1 Mon Sep 17 00:00:00 2001 From: David Wendt Date: Fri, 28 Feb 2020 20:55:09 -0500 Subject: [PATCH] Ensure that called setters are properly resolved so that errors in setters propagate up the Rust stack correctly. The previous system for handling setters would execute the setter and then return a value to indicate whether or not the caller needed to resolve a stack frame. However, no caller of `Property.set` actually did this. Ergo, errors in setters and getters would not resolve up the stack at the correct time. This problem also exists in AVM1 but is far less noticable as AVM1 only has two very uncommon runtime errors and very few movies use `throw`. --- core/src/avm2/function.rs | 4 ++-- core/src/avm2/object.rs | 32 ++++++++++++++++++++++------ core/src/avm2/property.rs | 38 ++++++++++++++-------------------- core/src/avm2/script_object.rs | 22 +++++++++++--------- 4 files changed, 56 insertions(+), 40 deletions(-) diff --git a/core/src/avm2/function.rs b/core/src/avm2/function.rs index e21184efa..2e3246434 100644 --- a/core/src/avm2/function.rs +++ b/core/src/avm2/function.rs @@ -451,7 +451,7 @@ impl<'gc> TObject<'gc> for FunctionObject<'gc> { value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error> { + ) -> Result, Error> { self.0 .write(context.gc_context) .base @@ -465,7 +465,7 @@ impl<'gc> TObject<'gc> for FunctionObject<'gc> { value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error> { + ) -> Result, Error> { self.0 .write(context.gc_context) .base diff --git a/core/src/avm2/object.rs b/core/src/avm2/object.rs index 7082c7a59..2af100d12 100644 --- a/core/src/avm2/object.rs +++ b/core/src/avm2/object.rs @@ -68,6 +68,9 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy } /// Set a property on this specific object. + /// + /// This function returns a `ReturnValue` which should be resolved. The + /// resulting `Value` is unimportant and should be discarded. fn set_property_local( self, reciever: Object<'gc>, @@ -75,7 +78,7 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error>; + ) -> Result, Error>; /// Set a property by it's QName. fn set_property( @@ -87,7 +90,10 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy context: &mut UpdateContext<'_, 'gc, '_>, ) -> Result<(), Error> { if self.has_own_virtual_setter(name) { - return self.set_property_local(reciever, name, value, avm, context); + self.set_property_local(reciever, name, value, avm, context)? + .resolve(avm, context)?; + + return Ok(()); } let mut proto = self.proto(); @@ -102,10 +108,17 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy proto = my_proto.proto(); } - reciever.set_property_local(reciever, name, value, avm, context) + reciever + .set_property_local(reciever, name, value, avm, context)? + .resolve(avm, context)?; + + Ok(()) } /// Init a property on this specific object. + /// + /// This function returns a `ReturnValue` which should be resolved. The + /// resulting `Value` is unimportant and should be discarded. fn init_property_local( self, reciever: Object<'gc>, @@ -113,7 +126,7 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error>; + ) -> Result, Error>; /// Init a property by it's QName. fn init_property( @@ -125,7 +138,10 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy context: &mut UpdateContext<'_, 'gc, '_>, ) -> Result<(), Error> { if self.has_own_virtual_setter(name) { - return self.init_property_local(reciever, name, value, avm, context); + self.init_property_local(reciever, name, value, avm, context)? + .resolve(avm, context)?; + + return Ok(()); } let mut proto = self.proto(); @@ -140,7 +156,11 @@ pub trait TObject<'gc>: 'gc + Collect + Debug + Into> + Clone + Copy proto = my_proto.proto(); } - reciever.init_property_local(reciever, name, value, avm, context) + reciever + .init_property_local(reciever, name, value, avm, context)? + .resolve(avm, context)?; + + Ok(()) } /// Retrieve a slot by it's index. diff --git a/core/src/avm2/property.rs b/core/src/avm2/property.rs index 361ebd8c8..16186fa90 100644 --- a/core/src/avm2/property.rs +++ b/core/src/avm2/property.rs @@ -153,10 +153,8 @@ impl<'gc> Property<'gc> { /// Set a property slot. /// - /// This function returns `true` if the set has completed, or `false` if - /// it has not yet occured. If `false`, and you need to run code after the - /// set has occured, you must recursively execute the top-most frame via - /// `run_current_frame`. + /// This function returns a `ReturnValue` which should be resolved. The + /// resulting `Value` is unimportant and should be discarded. /// /// This function cannot set slot properties and will panic if one /// is encountered. @@ -166,21 +164,20 @@ impl<'gc> Property<'gc> { context: &mut UpdateContext<'_, 'gc, '_>, this: Object<'gc>, new_value: impl Into>, - ) -> Result { + ) -> Result, Error> { match self { Property::Virtual { set, .. } => { if let Some(function) = set { - let return_value = function.exec( + return function.exec( Some(this), &[new_value.into()], avm, context, this.proto(), - )?; - Ok(return_value.is_immediate()) - } else { - Ok(true) + ); } + + Ok(Value::Undefined.into()) } Property::Stored { value, attributes, .. @@ -189,7 +186,7 @@ impl<'gc> Property<'gc> { *value = new_value.into(); } - Ok(true) + Ok(Value::Undefined.into()) } Property::Slot { .. } => panic!("Cannot recursively set slots"), } @@ -202,10 +199,8 @@ impl<'gc> Property<'gc> { /// properties, at least once. Virtual properties with no setter cannot be /// initialized. /// - /// This function returns `true` if the set has completed, or `false` if - /// it has not yet occured. If `false`, and you need to run code after the - /// set has occured, you must recursively execute the top-most frame via - /// `run_current_frame`. + /// This function returns a `ReturnValue` which should be resolved. The + /// resulting `Value` is unimportant and should be discarded. /// /// This function cannot initialize slot properties and will panic if one /// is encountered. @@ -215,26 +210,25 @@ impl<'gc> Property<'gc> { context: &mut UpdateContext<'_, 'gc, '_>, this: Object<'gc>, new_value: impl Into>, - ) -> Result { + ) -> Result, Error> { match self { Property::Virtual { set, .. } => { if let Some(function) = set { - let return_value = function.exec( + return function.exec( Some(this), &[new_value.into()], avm, context, this.proto(), - )?; - Ok(return_value.is_immediate()) - } else { - Ok(true) + ); } + + Ok(Value::Undefined.into()) } Property::Stored { value, .. } => { *value = new_value.into(); - Ok(true) + Ok(Value::Undefined.into()) } Property::Slot { .. } => panic!("Cannot recursively init slots"), } diff --git a/core/src/avm2/script_object.rs b/core/src/avm2/script_object.rs index d6ed1208e..e2ab31b78 100644 --- a/core/src/avm2/script_object.rs +++ b/core/src/avm2/script_object.rs @@ -54,7 +54,7 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> { value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error> { + ) -> Result, Error> { self.0 .write(context.gc_context) .set_property_local(reciever, name, value, avm, context) @@ -67,7 +67,7 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> { value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error> { + ) -> Result, Error> { self.0 .write(context.gc_context) .init_property_local(reciever, name, value, avm, context) @@ -241,20 +241,21 @@ impl<'gc> ScriptObjectData<'gc> { value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error> { + ) -> Result, Error> { if let Some(prop) = self.values.get_mut(name) { if let Some(slot_id) = prop.slot_id() { self.set_slot(slot_id, value, context.gc_context)?; + Ok(Value::Undefined.into()) } else { - prop.set(avm, context, reciever, value)?; + prop.set(avm, context, reciever, value) } } else { //TODO: Not all classes are dynamic like this self.values .insert(name.clone(), Property::new_dynamic_property(value)); - } - Ok(()) + Ok(Value::Undefined.into()) + } } pub fn init_property_local( @@ -264,20 +265,21 @@ impl<'gc> ScriptObjectData<'gc> { value: Value<'gc>, avm: &mut Avm2<'gc>, context: &mut UpdateContext<'_, 'gc, '_>, - ) -> Result<(), Error> { + ) -> Result, Error> { if let Some(prop) = self.values.get_mut(name) { if let Some(slot_id) = prop.slot_id() { self.init_slot(slot_id, value, context.gc_context)?; + Ok(Value::Undefined.into()) } else { - prop.init(avm, context, reciever, value)?; + prop.init(avm, context, reciever, value) } } else { //TODO: Not all classes are dynamic like this self.values .insert(name.clone(), Property::new_dynamic_property(value)); - } - Ok(()) + Ok(Value::Undefined.into()) + } } pub fn delete_property(&mut self, name: &QName) -> bool {