From dadb12540df3eca4c0c010bc4e985d8240b8a1ac Mon Sep 17 00:00:00 2001 From: Lord-McSweeney Date: Sun, 29 Jun 2025 10:03:53 -0700 Subject: [PATCH 1/5] avm2: Add `ScriptObject::set_dynamic_property` and use it `set_dynamic_property` works just like `set_string_property_local`, except it takes a Mutation instead of an Activation and an AvmString instead of a Multiname --- core/src/avm2/globals/avmplus.rs | 160 ++++++++++++-------------- core/src/avm2/globals/json.rs | 2 +- core/src/avm2/object.rs | 18 +++ core/src/avm2/object/script_object.rs | 2 +- 4 files changed, 94 insertions(+), 88 deletions(-) diff --git a/core/src/avm2/globals/avmplus.rs b/core/src/avm2/globals/avmplus.rs index 6640a9502239..82df3ea59ab6 100644 --- a/core/src/avm2/globals/avmplus.rs +++ b/core/src/avm2/globals/avmplus.rs @@ -38,33 +38,33 @@ pub fn describe_type_json<'gc>( .dollar_removed_name(activation.gc()) .to_qualified_name(activation.gc()); - object.set_string_property_local(istr!("name"), qualified_name.into(), activation)?; + object.set_dynamic_property(istr!("name"), qualified_name.into(), activation.gc()); - object.set_string_property_local( + object.set_dynamic_property( istr!("isDynamic"), (!used_class_def.is_sealed()).into(), - activation, - )?; - object.set_string_property_local( + activation.gc(), + ); + object.set_dynamic_property( istr!("isFinal"), used_class_def.is_final().into(), - activation, - )?; - object.set_string_property_local( + activation.gc(), + ); + object.set_dynamic_property( istr!("isStatic"), value .as_object() .and_then(|o| o.as_class_object()) .is_some() .into(), - activation, - )?; + activation.gc(), + ); let traits = describe_internal_body(activation, used_class_def, flags)?; if flags.contains(DescribeTypeFlags::INCLUDE_TRAITS) { - object.set_string_property_local(istr!("traits"), traits.into(), activation)?; + object.set_dynamic_property(istr!("traits"), traits.into(), activation.gc()); } else { - object.set_string_property_local(istr!("traits"), Value::Null, activation)?; + object.set_dynamic_property(istr!("traits"), Value::Null, activation.gc()); } Ok(object.into()) @@ -103,33 +103,33 @@ fn describe_internal_body<'gc>( let methods = ArrayObject::empty(activation); if flags.contains(DescribeTypeFlags::INCLUDE_BASES) { - traits.set_string_property_local(istr!("bases"), bases.into(), activation)?; + traits.set_dynamic_property(istr!("bases"), bases.into(), activation.gc()); } else { - traits.set_string_property_local(istr!("bases"), Value::Null, activation)?; + traits.set_dynamic_property(istr!("bases"), Value::Null, activation.gc()); } if flags.contains(DescribeTypeFlags::INCLUDE_INTERFACES) { - traits.set_string_property_local(istr!("interfaces"), interfaces.into(), activation)?; + traits.set_dynamic_property(istr!("interfaces"), interfaces.into(), activation.gc()); } else { - traits.set_string_property_local(istr!("interfaces"), Value::Null, activation)?; + traits.set_dynamic_property(istr!("interfaces"), Value::Null, activation.gc()); } if flags.contains(DescribeTypeFlags::INCLUDE_VARIABLES) { - traits.set_string_property_local(istr!("variables"), variables.into(), activation)?; + traits.set_dynamic_property(istr!("variables"), variables.into(), activation.gc()); } else { - traits.set_string_property_local(istr!("variables"), Value::Null, activation)?; + traits.set_dynamic_property(istr!("variables"), Value::Null, activation.gc()); } if flags.contains(DescribeTypeFlags::INCLUDE_ACCESSORS) { - traits.set_string_property_local(istr!("accessors"), accessors.into(), activation)?; + traits.set_dynamic_property(istr!("accessors"), accessors.into(), activation.gc()); } else { - traits.set_string_property_local(istr!("accessors"), Value::Null, activation)?; + traits.set_dynamic_property(istr!("accessors"), Value::Null, activation.gc()); } if flags.contains(DescribeTypeFlags::INCLUDE_METHODS) { - traits.set_string_property_local(istr!("methods"), methods.into(), activation)?; + traits.set_dynamic_property(istr!("methods"), methods.into(), activation.gc()); } else { - traits.set_string_property_local(istr!("methods"), Value::Null, activation)?; + traits.set_dynamic_property(istr!("methods"), Value::Null, activation.gc()); } let mut bases_array = bases.array_storage_mut(mc); @@ -214,31 +214,31 @@ fn describe_internal_body<'gc>( let trait_metadata = vtable.get_metadata_for_slot(*slot_id); let variable = ScriptObject::new_object(activation); - variable.set_string_property_local(istr!("name"), prop_name.into(), activation)?; - variable.set_string_property_local( + variable.set_dynamic_property(istr!("name"), prop_name.into(), activation.gc()); + variable.set_dynamic_property( istr!("type"), prop_class_name.into(), - activation, - )?; - variable.set_string_property_local(istr!("access"), access.into(), activation)?; - variable.set_string_property_local( + activation.gc(), + ); + variable.set_dynamic_property(istr!("access"), access.into(), activation.gc()); + variable.set_dynamic_property( istr!("uri"), uri.map_or(Value::Null, |u| u.into()), - activation, - )?; + activation.gc(), + ); - variable.set_string_property_local(istr!("metadata"), Value::Null, activation)?; + variable.set_dynamic_property(istr!("metadata"), Value::Null, activation.gc()); if flags.contains(DescribeTypeFlags::INCLUDE_METADATA) { let metadata_object = ArrayObject::empty(activation); if let Some(metadata) = trait_metadata { write_metadata(metadata_object, metadata, activation)?; } - variable.set_string_property_local( + variable.set_dynamic_property( istr!("metadata"), metadata_object.into(), - activation, - )?; + activation.gc(), + ); } variables_array.push(variable.into()); @@ -277,47 +277,43 @@ fn describe_internal_body<'gc>( let method_obj = ScriptObject::new_object(activation); - method_obj.set_string_property_local( - istr!("name"), - prop_name.into(), - activation, - )?; - method_obj.set_string_property_local( + method_obj.set_dynamic_property(istr!("name"), prop_name.into(), activation.gc()); + method_obj.set_dynamic_property( istr!("returnType"), return_type_name.into(), - activation, - )?; - method_obj.set_string_property_local( + activation.gc(), + ); + method_obj.set_dynamic_property( istr!("declaredBy"), declared_by_name.into(), - activation, - )?; + activation.gc(), + ); - method_obj.set_string_property_local( + method_obj.set_dynamic_property( istr!("uri"), uri.map_or(Value::Null, |u| u.into()), - activation, - )?; + activation.gc(), + ); let params = write_params(&method.method, activation)?; - method_obj.set_string_property_local( + method_obj.set_dynamic_property( istr!("parameters"), params.into(), - activation, - )?; + activation.gc(), + ); - method_obj.set_string_property_local(istr!("metadata"), Value::Null, activation)?; + method_obj.set_dynamic_property(istr!("metadata"), Value::Null, activation.gc()); if flags.contains(DescribeTypeFlags::INCLUDE_METADATA) { let metadata_object = ArrayObject::empty(activation); if let Some(metadata) = trait_metadata { write_metadata(metadata_object, metadata, activation)?; } - method_obj.set_string_property_local( + method_obj.set_dynamic_property( istr!("metadata"), metadata_object.into(), - activation, - )?; + activation.gc(), + ); } methods_array.push(method_obj.into()); } @@ -362,31 +358,23 @@ fn describe_internal_body<'gc>( let declared_by = defining_class.dollar_removed_name(mc).to_qualified_name(mc); let accessor_obj = ScriptObject::new_object(activation); - accessor_obj.set_string_property_local( - istr!("name"), - prop_name.into(), - activation, - )?; - accessor_obj.set_string_property_local( - istr!("access"), - access.into(), - activation, - )?; - accessor_obj.set_string_property_local( + accessor_obj.set_dynamic_property(istr!("name"), prop_name.into(), activation.gc()); + accessor_obj.set_dynamic_property(istr!("access"), access.into(), activation.gc()); + accessor_obj.set_dynamic_property( istr!("type"), accessor_type.into(), - activation, - )?; - accessor_obj.set_string_property_local( + activation.gc(), + ); + accessor_obj.set_dynamic_property( istr!("declaredBy"), declared_by.into(), - activation, - )?; - accessor_obj.set_string_property_local( + activation.gc(), + ); + accessor_obj.set_dynamic_property( istr!("uri"), uri.map_or(Value::Null, |u| u.into()), - activation, - )?; + activation.gc(), + ); let metadata_object = ArrayObject::empty(activation); @@ -405,17 +393,17 @@ fn describe_internal_body<'gc>( if flags.contains(DescribeTypeFlags::INCLUDE_METADATA) && metadata_object.array_storage().length() > 0 { - accessor_obj.set_string_property_local( + accessor_obj.set_dynamic_property( istr!("metadata"), metadata_object.into(), - activation, - )?; + activation.gc(), + ); } else { - accessor_obj.set_string_property_local( + accessor_obj.set_dynamic_property( istr!("metadata"), Value::Null, - activation, - )?; + activation.gc(), + ); } accessors_array.push(accessor_obj.into()); @@ -429,10 +417,10 @@ fn describe_internal_body<'gc>( !c.signature().is_empty() && flags.contains(DescribeTypeFlags::INCLUDE_CONSTRUCTOR) }) { let params = write_params(&constructor, activation)?; - traits.set_string_property_local(istr!("constructor"), params.into(), activation)?; + traits.set_dynamic_property(istr!("constructor"), params.into(), activation.gc()); } else { // This is needed to override the normal 'constructor' property - traits.set_string_property_local(istr!("constructor"), Value::Null, activation)?; + traits.set_dynamic_property(istr!("constructor"), Value::Null, activation.gc()); } if flags.contains(DescribeTypeFlags::INCLUDE_METADATA) { @@ -444,9 +432,9 @@ fn describe_internal_body<'gc>( ); let metadata_object = ArrayObject::empty(activation); - traits.set_string_property_local(istr!("metadata"), metadata_object.into(), activation)?; + traits.set_dynamic_property(istr!("metadata"), metadata_object.into(), activation.gc()); } else { - traits.set_string_property_local(istr!("metadata"), Value::Null, activation)?; + traits.set_dynamic_property(istr!("metadata"), Value::Null, activation.gc()); } Ok(traits) @@ -473,8 +461,8 @@ fn write_params<'gc>( let param_type_name = display_name(activation.strings(), param.param_type_name); let optional = param.default_value.is_some(); let param_obj = ScriptObject::new_object(activation); - param_obj.set_string_property_local(istr!("type"), param_type_name.into(), activation)?; - param_obj.set_string_property_local(istr!("optional"), optional.into(), activation)?; + param_obj.set_dynamic_property(istr!("type"), param_type_name.into(), activation.gc()); + param_obj.set_dynamic_property(istr!("optional"), optional.into(), activation.gc()); params_array.push(param_obj.into()); } Ok(params) diff --git a/core/src/avm2/globals/json.rs b/core/src/avm2/globals/json.rs index eaadb0c77bc8..c1fa63a7b51c 100644 --- a/core/src/avm2/globals/json.rs +++ b/core/src/avm2/globals/json.rs @@ -45,7 +45,7 @@ fn deserialize_json_inner<'gc>( if matches!(mapped_val, Value::Undefined) { obj.delete_string_property_local(key, activation)?; } else { - obj.set_string_property_local(key, mapped_val, activation)?; + obj.set_dynamic_property(key, mapped_val, activation.gc()); } } obj.into() diff --git a/core/src/avm2/object.rs b/core/src/avm2/object.rs index 38a8f43e3c66..e6e1bb77f2c3 100644 --- a/core/src/avm2/object.rs +++ b/core/src/avm2/object.rs @@ -341,6 +341,24 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + result.call(activation, self_val, arguments) } + /// Set a dynamic property on this Object by name. This is like + /// `set_property_local`, but it skips dynamic-dispatch TObject logic + /// and always sets a dynamic property on the base ScriptObject. + #[no_dynamic] + fn set_dynamic_property( + self, + local_name: AvmString<'gc>, + value: Value<'gc>, + mc: &Mutation<'gc>, + ) { + use crate::avm2::object::script_object::maybe_int_property; + + // See the comment in ScriptObjectWrapper::set_property_local + let key = maybe_int_property(local_name); + + self.base().values_mut(mc).insert(key, value); + } + /// Retrieve a slot by its index. #[no_dynamic] #[inline(always)] diff --git a/core/src/avm2/object/script_object.rs b/core/src/avm2/object/script_object.rs index 0173c8cd9597..636f4e1e951b 100644 --- a/core/src/avm2/object/script_object.rs +++ b/core/src/avm2/object/script_object.rs @@ -74,7 +74,7 @@ impl<'gc> TObject<'gc> for ScriptObject<'gc> { } } -fn maybe_int_property(name: AvmString<'_>) -> DynamicKey<'_> { +pub fn maybe_int_property(name: AvmString<'_>) -> DynamicKey<'_> { // TODO: this should use a custom implementation, not parse() // FP is much stricter here, only allowing pure natural numbers without sign or leading zeros if let Ok(val) = name.parse::() { From 12ca596f049f5a7bd00cd090ebf45fd71f404fc8 Mon Sep 17 00:00:00 2001 From: Lord-McSweeney Date: Sun, 29 Jun 2025 10:10:47 -0700 Subject: [PATCH 2/5] avm2: Use `set_dynamic_property` in more places This allows us to make `avmplus::describe_internal_body` infallible --- core/src/avm2/activation.rs | 6 ++--- core/src/avm2/amf.rs | 27 ++++++++++++------- core/src/avm2/flv.rs | 12 ++++----- core/src/avm2/globals/avmplus.rs | 27 +++++++++---------- .../avm2/globals/flash/display/loader_info.rs | 2 +- .../avm2/globals/flash/display/shader_data.rs | 10 +++---- .../globals/flash/display/shader_parameter.rs | 12 ++++----- .../avm2/globals/flash/text/style_sheet.rs | 12 ++++----- core/src/avm2/globals/reg_exp.rs | 14 +++++----- core/src/avm2/metadata.rs | 27 +++++++------------ core/src/avm2/object/array_object.rs | 2 +- core/src/avm2/object/class_object.rs | 4 +-- core/src/avm2/object/event_object.rs | 4 +-- core/src/external.rs | 3 +-- 14 files changed, 77 insertions(+), 85 deletions(-) diff --git a/core/src/avm2/activation.rs b/core/src/avm2/activation.rs index 69f19cf58e67..b13b5b3439b4 100644 --- a/core/src/avm2/activation.rs +++ b/core/src/avm2/activation.rs @@ -334,9 +334,7 @@ impl<'a, 'gc> Activation<'a, 'gc> { { let string_callee = istr!(self, "callee"); - args_object - .set_string_property_local(string_callee, callee, self) - .unwrap(); + args_object.set_dynamic_property(string_callee, callee, self.gc()); args_object.set_local_property_is_enumerable(self.gc(), string_callee, false); } @@ -1811,7 +1809,7 @@ impl<'a, 'gc> Activation<'a, 'gc> { let value = self.pop_stack(); let name = self.pop_stack(); - object.set_string_property_local(name.coerce_to_string(self)?, value, self)?; + object.set_dynamic_property(name.coerce_to_string(self)?, value, self.gc()); } self.push_stack(object); diff --git a/core/src/avm2/amf.rs b/core/src/avm2/amf.rs index 174ab508b993..ff4fa5fb8ade 100644 --- a/core/src/avm2/amf.rs +++ b/core/src/avm2/amf.rs @@ -313,11 +313,20 @@ pub fn deserialize_value_impl<'gc>( // Now let's add each element as a property for element in elements { - array.set_string_property_local( - AvmString::new_utf8(activation.gc(), element.name()), - deserialize_value_impl(activation, element.value(), object_map)?, - activation, - )?; + let name = element.name(); + let value = deserialize_value_impl(activation, element.value(), object_map)?; + + // If the name of the element was numerical, we set an element on + // the array instead of setting a dynamic property. + if let Ok(index) = name.parse::() { + array.set_element(activation.gc(), index, value); + } else { + array.set_dynamic_property( + AvmString::new_utf8(activation.gc(), name), + value, + activation.gc(), + ); + } } array.into() } @@ -462,7 +471,7 @@ pub fn deserialize_value_impl<'gc>( dict_obj.set_property_by_object(key, value, activation.gc()); } else { let key_string = key.coerce_to_string(activation)?; - dict_obj.set_string_property_local(key_string, value, activation)?; + dict_obj.set_dynamic_property(key_string, value, activation.gc()); } } dict_obj.into() @@ -499,11 +508,11 @@ pub fn deserialize_lso<'gc>( let obj = ScriptObject::new_object(activation); for child in &lso.body { - obj.set_string_property_local( + obj.set_dynamic_property( AvmString::new_utf8(activation.gc(), &child.name), deserialize_value(activation, child.value())?, - activation, - )?; + activation.gc(), + ); } Ok(obj) diff --git a/core/src/avm2/flv.rs b/core/src/avm2/flv.rs index a220a6314bab..fd661e361048 100644 --- a/core/src/avm2/flv.rs +++ b/core/src/avm2/flv.rs @@ -15,13 +15,11 @@ fn avm2_object_from_flv_variables<'gc>( for value in variables { let property_name = value.name; - info_object - .set_string_property_local( - AvmString::new_utf8_bytes(activation.gc(), property_name), - value.data.to_avm2_value(activation), - activation, - ) - .expect("valid set"); + info_object.set_dynamic_property( + AvmString::new_utf8_bytes(activation.gc(), property_name), + value.data.to_avm2_value(activation), + activation.gc(), + ); } info_object.into() diff --git a/core/src/avm2/globals/avmplus.rs b/core/src/avm2/globals/avmplus.rs index 82df3ea59ab6..80cb00ecec76 100644 --- a/core/src/avm2/globals/avmplus.rs +++ b/core/src/avm2/globals/avmplus.rs @@ -60,7 +60,7 @@ pub fn describe_type_json<'gc>( activation.gc(), ); - let traits = describe_internal_body(activation, used_class_def, flags)?; + let traits = describe_internal_body(activation, used_class_def, flags); if flags.contains(DescribeTypeFlags::INCLUDE_TRAITS) { object.set_dynamic_property(istr!("traits"), traits.into(), activation.gc()); } else { @@ -91,7 +91,7 @@ fn describe_internal_body<'gc>( activation: &mut Activation<'_, 'gc>, class_def: Class<'gc>, flags: DescribeTypeFlags, -) -> Result, Error<'gc>> { +) -> Object<'gc> { let mc = activation.gc(); let traits = ScriptObject::new_object(activation); @@ -232,7 +232,7 @@ fn describe_internal_body<'gc>( if flags.contains(DescribeTypeFlags::INCLUDE_METADATA) { let metadata_object = ArrayObject::empty(activation); if let Some(metadata) = trait_metadata { - write_metadata(metadata_object, metadata, activation)?; + write_metadata(metadata_object, metadata, activation); } variable.set_dynamic_property( istr!("metadata"), @@ -295,7 +295,7 @@ fn describe_internal_body<'gc>( activation.gc(), ); - let params = write_params(&method.method, activation)?; + let params = write_params(&method.method, activation); method_obj.set_dynamic_property( istr!("parameters"), params.into(), @@ -307,7 +307,7 @@ fn describe_internal_body<'gc>( if flags.contains(DescribeTypeFlags::INCLUDE_METADATA) { let metadata_object = ArrayObject::empty(activation); if let Some(metadata) = trait_metadata { - write_metadata(metadata_object, metadata, activation)?; + write_metadata(metadata_object, metadata, activation); } method_obj.set_dynamic_property( istr!("metadata"), @@ -380,13 +380,13 @@ fn describe_internal_body<'gc>( if let Some(get_disp_id) = get { if let Some(metadata) = vtable.get_metadata_for_disp(*get_disp_id) { - write_metadata(metadata_object, metadata, activation)?; + write_metadata(metadata_object, metadata, activation); } } if let Some(set_disp_id) = set { if let Some(metadata) = vtable.get_metadata_for_disp(*set_disp_id) { - write_metadata(metadata_object, metadata, activation)?; + write_metadata(metadata_object, metadata, activation); } } @@ -416,7 +416,7 @@ fn describe_internal_body<'gc>( if let Some(constructor) = constructor.filter(|c| { !c.signature().is_empty() && flags.contains(DescribeTypeFlags::INCLUDE_CONSTRUCTOR) }) { - let params = write_params(&constructor, activation)?; + let params = write_params(&constructor, activation); traits.set_dynamic_property(istr!("constructor"), params.into(), activation.gc()); } else { // This is needed to override the normal 'constructor' property @@ -437,7 +437,7 @@ fn describe_internal_body<'gc>( traits.set_dynamic_property(istr!("metadata"), Value::Null, activation.gc()); } - Ok(traits) + traits } fn display_name<'gc>( @@ -454,7 +454,7 @@ fn display_name<'gc>( fn write_params<'gc>( method: &Method<'gc>, activation: &mut Activation<'_, 'gc>, -) -> Result, Error<'gc>> { +) -> ArrayObject<'gc> { let params = ArrayObject::empty(activation); let mut params_array = params.array_storage_mut(activation.gc()); for param in method.signature() { @@ -465,20 +465,19 @@ fn write_params<'gc>( param_obj.set_dynamic_property(istr!("optional"), optional.into(), activation.gc()); params_array.push(param_obj.into()); } - Ok(params) + params } fn write_metadata<'gc>( metadata_object: ArrayObject<'gc>, trait_metadata: &[Metadata<'gc>], activation: &mut Activation<'_, 'gc>, -) -> Result<(), Error<'gc>> { +) { let mut metadata_array = metadata_object.array_storage_mut(activation.gc()); for single_trait in trait_metadata.iter() { - metadata_array.push(single_trait.as_json_object(activation)?.into()); + metadata_array.push(single_trait.as_json_object(activation).into()); } - Ok(()) } /// Like `Value::instance_class`, but supports Value::Null and Value::Undefined, diff --git a/core/src/avm2/globals/flash/display/loader_info.rs b/core/src/avm2/globals/flash/display/loader_info.rs index a6d088f648a2..0722ea1b6226 100644 --- a/core/src/avm2/globals/flash/display/loader_info.rs +++ b/core/src/avm2/globals/flash/display/loader_info.rs @@ -554,7 +554,7 @@ pub fn get_parameters<'gc>( for (k, v) in parameters.iter() { let avm_k = AvmString::new_utf8(activation.gc(), k); let avm_v = AvmString::new_utf8(activation.gc(), v); - params_obj.set_string_property_local(avm_k, avm_v.into(), activation)?; + params_obj.set_dynamic_property(avm_k, avm_v.into(), activation.gc()); } return Ok(params_obj.into()); diff --git a/core/src/avm2/globals/flash/display/shader_data.rs b/core/src/avm2/globals/flash/display/shader_data.rs index 011eedd3d468..bec0d516ba45 100644 --- a/core/src/avm2/globals/flash/display/shader_data.rs +++ b/core/src/avm2/globals/flash/display/shader_data.rs @@ -32,13 +32,13 @@ pub fn init<'gc>( // Top-level metadata appears to turn `TInt` into a plain integer value, // rather than a single-element array. let value = meta.value.as_avm2_value(activation, true)?; - this.set_string_property_local(name, value, activation)?; + this.set_dynamic_property(name, value, activation.gc()); } - this.set_string_property_local( + this.set_dynamic_property( istr!("name"), AvmString::new_utf8(activation.gc(), &shader.name).into(), - activation, - )?; + activation.gc(), + ); let mut normal_index = 0; let mut texture_index = 0; @@ -66,7 +66,7 @@ pub fn init<'gc>( let name = AvmString::new_utf8(activation.gc(), name); let param_obj = make_shader_parameter(activation, param, index)?; - this.set_string_property_local(name, param_obj, activation)?; + this.set_dynamic_property(name, param_obj, activation.gc()); } let shader_handle = activation diff --git a/core/src/avm2/globals/flash/display/shader_parameter.rs b/core/src/avm2/globals/flash/display/shader_parameter.rs index 3f2deb3b0761..50de6509fc8b 100644 --- a/core/src/avm2/globals/flash/display/shader_parameter.rs +++ b/core/src/avm2/globals/flash/display/shader_parameter.rs @@ -43,11 +43,11 @@ pub fn make_shader_parameter<'gc>( param_object.set_slot(parameter_slots::_VALUE, value, activation)?; } } - param_object.set_string_property_local( + param_object.set_dynamic_property( istr!("name"), AvmString::new_utf8(activation.gc(), name).into(), - activation, - )?; + activation.gc(), + ); Ok(param_value) } PixelBenderParam::Texture { name, channels, .. } => { @@ -61,11 +61,11 @@ pub fn make_shader_parameter<'gc>( obj.set_slot(input_slots::_CHANNELS, (*channels).into(), activation)?; obj.set_slot(input_slots::_INDEX, index.into(), activation)?; - obj.set_string_property_local( + obj.set_dynamic_property( istr!("name"), AvmString::new_utf8(activation.gc(), name).into(), - activation, - )?; + activation.gc(), + ); Ok(obj.into()) } } diff --git a/core/src/avm2/globals/flash/text/style_sheet.rs b/core/src/avm2/globals/flash/text/style_sheet.rs index 85be408ea17d..49600800280e 100644 --- a/core/src/avm2/globals/flash/text/style_sheet.rs +++ b/core/src/avm2/globals/flash/text/style_sheet.rs @@ -20,18 +20,18 @@ pub fn inner_parse_css<'gc>( let object = ScriptObject::new_object(activation); for (key, value) in properties.into_iter() { - object.set_string_property_local( + object.set_dynamic_property( AvmString::new(activation.gc(), transform_dashes_to_camel_case(key)), Value::String(AvmString::new(activation.gc(), value)), - activation, - )?; + activation.gc(), + ); } - result.set_string_property_local( + result.set_dynamic_property( AvmString::new(activation.gc(), selector), Value::Object(object), - activation, - )?; + activation.gc(), + ); } } diff --git a/core/src/avm2/globals/reg_exp.rs b/core/src/avm2/globals/reg_exp.rs index 923c9e220a2d..1be56bbf86e9 100644 --- a/core/src/avm2/globals/reg_exp.rs +++ b/core/src/avm2/globals/reg_exp.rs @@ -241,20 +241,20 @@ pub fn exec<'gc>( |range| AvmString::new(activation.gc(), &text[range]), ); - object.set_string_property_local( + object.set_dynamic_property( AvmString::new_utf8(activation.gc(), name), string.into(), - activation, - )?; + activation.gc(), + ); } - object.set_string_property_local( + object.set_dynamic_property( istr!("index"), Value::Number(matched.start() as f64), - activation, - )?; + activation.gc(), + ); - object.set_string_property_local(istr!("input"), text.into(), activation)?; + object.set_dynamic_property(istr!("input"), text.into(), activation.gc()); return Ok(object.into()); } diff --git a/core/src/avm2/metadata.rs b/core/src/avm2/metadata.rs index 19137b0d4996..dce4aefbb79e 100644 --- a/core/src/avm2/metadata.rs +++ b/core/src/avm2/metadata.rs @@ -74,35 +74,28 @@ impl<'gc> Metadata<'gc> { } // Converts the Metadata to an Object of the form used in avmplus:describeTypeJSON(). - pub fn as_json_object( - &self, - activation: &mut Activation<'_, 'gc>, - ) -> Result, Error<'gc>> { + pub fn as_json_object(&self, activation: &mut Activation<'_, 'gc>) -> Object<'gc> { let object = ScriptObject::new_object(activation); - object.set_string_property_local(istr!("name"), self.name.into(), activation)?; + object.set_dynamic_property(istr!("name"), self.name.into(), activation.gc()); let values = self .items .iter() .map(|item| { let value_object = ScriptObject::new_object(activation); - value_object.set_string_property_local( - istr!("key"), - item.key.into(), - activation, - )?; - value_object.set_string_property_local( + value_object.set_dynamic_property(istr!("key"), item.key.into(), activation.gc()); + value_object.set_dynamic_property( istr!("value"), item.value.into(), - activation, - )?; - Ok(Some(value_object.into())) + activation.gc(), + ); + Some(value_object.into()) }) - .collect::>>, Error<'gc>>>()?; + .collect::>>>(); let values_array = ArrayObject::from_storage(activation, ArrayStorage::from_storage(values)); - object.set_string_property_local(istr!("value"), values_array.into(), activation)?; - Ok(object) + object.set_dynamic_property(istr!("value"), values_array.into(), activation.gc()); + object } } diff --git a/core/src/avm2/object/array_object.rs b/core/src/avm2/object/array_object.rs index ba9dcac2b2b8..35b6e51ab108 100644 --- a/core/src/avm2/object/array_object.rs +++ b/core/src/avm2/object/array_object.rs @@ -84,7 +84,7 @@ impl<'gc> ArrayObject<'gc> { )) } - fn set_element(self, mc: &Mutation<'gc>, index: usize, value: Value<'gc>) { + pub fn set_element(self, mc: &Mutation<'gc>, index: usize, value: Value<'gc>) { unlock!(Gc::write(mc, self.0), ArrayObjectData, array) .borrow_mut() .set(index, value); diff --git a/core/src/avm2/object/class_object.rs b/core/src/avm2/object/class_object.rs index 9992464791ba..426ef034e1ef 100644 --- a/core/src/avm2/object/class_object.rs +++ b/core/src/avm2/object/class_object.rs @@ -260,9 +260,7 @@ impl<'gc> ClassObject<'gc> { let mc = activation.gc(); unlock!(Gc::write(mc, self.0), ClassObjectData, prototype).set(Some(class_proto)); - class_proto - .set_string_property_local(istr!("constructor"), self.into(), activation) - .expect("Prototype is a dynamic object"); + class_proto.set_dynamic_property(istr!("constructor"), self.into(), mc); class_proto.set_local_property_is_enumerable(mc, istr!("constructor"), false); } diff --git a/core/src/avm2/object/event_object.rs b/core/src/avm2/object/event_object.rs index 2eacc3bc06df..e94621474260 100644 --- a/core/src/avm2/object/event_object.rs +++ b/core/src/avm2/object/event_object.rs @@ -236,9 +236,7 @@ impl<'gc> EventObject<'gc> { let key = AvmString::new_utf8(activation.gc(), key); let value = AvmString::new_utf8(activation.gc(), value); - info_object - .set_string_property_local(key, Value::String(value), activation) - .unwrap(); + info_object.set_dynamic_property(key, Value::String(value), activation.gc()); } let event_name = istr!("netStatus"); diff --git a/core/src/external.rs b/core/src/external.rs index 55a94ade360c..0fbbe3be757f 100644 --- a/core/src/external.rs +++ b/core/src/external.rs @@ -248,8 +248,7 @@ impl Value { for (key, value) in values.into_iter() { let key = AvmString::new_utf8(activation.gc(), key); let value = value.into_avm2(activation); - obj.set_string_property_local(key, value, activation) - .unwrap(); + obj.set_dynamic_property(key, value, activation.gc()); } Avm2Value::Object(obj) } From f22cfb046e9d0c3ae7e856003889f2111f91b945 Mon Sep 17 00:00:00 2001 From: Lord-McSweeney Date: Mon, 30 Jun 2025 00:51:13 -0700 Subject: [PATCH 3/5] avm2: Add and use `get_dynamic_property` and `delete_dynamic_property` These functions replace `get_string_property_local` and `delete_string_property_local` --- .../avm2/globals/flash/display/shader_job.rs | 10 +- core/src/avm2/globals/json.rs | 2 +- core/src/avm2/object.rs | 127 ++++++++---------- 3 files changed, 62 insertions(+), 77 deletions(-) diff --git a/core/src/avm2/globals/flash/display/shader_job.rs b/core/src/avm2/globals/flash/display/shader_job.rs index aeb244e9b730..5e8f2ecf6af3 100644 --- a/core/src/avm2/globals/flash/display/shader_job.rs +++ b/core/src/avm2/globals/flash/display/shader_job.rs @@ -70,10 +70,7 @@ pub fn get_shader_args<'gc>( }); } let shader_param = shader_data - .get_string_property_local( - AvmString::new_utf8(activation.gc(), name), - activation, - ) + .get_dynamic_property(AvmString::new_utf8(activation.gc(), name)) .expect("Missing normal property"); let shader_param = shader_param @@ -104,10 +101,7 @@ pub fn get_shader_args<'gc>( name, } => { let shader_input = shader_data - .get_string_property_local( - AvmString::new_utf8(activation.gc(), name), - activation, - ) + .get_dynamic_property(AvmString::new_utf8(activation.gc(), name)) .expect("Missing property") .as_object() .expect("Shader input is not an object"); diff --git a/core/src/avm2/globals/json.rs b/core/src/avm2/globals/json.rs index c1fa63a7b51c..e292f075e934 100644 --- a/core/src/avm2/globals/json.rs +++ b/core/src/avm2/globals/json.rs @@ -43,7 +43,7 @@ fn deserialize_json_inner<'gc>( Some(reviver) => reviver.call(activation, Value::Null, &[key.into(), val])?, }; if matches!(mapped_val, Value::Undefined) { - obj.delete_string_property_local(key, activation)?; + obj.delete_dynamic_property(key, activation.gc()); } else { obj.set_dynamic_property(key, mapped_val, activation.gc()); } diff --git a/core/src/avm2/object.rs b/core/src/avm2/object.rs index e6e1bb77f2c3..0b1bdc462a3c 100644 --- a/core/src/avm2/object.rs +++ b/core/src/avm2/object.rs @@ -244,15 +244,21 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + self.base().get_property_local(name, activation) } - /// Same as get_property_local, but constructs a public Multiname for you. + /// Get a dynamic property on this Object by name. This is like + /// `get_property_local`, but it skips dynamic-dispatch TObject logic + /// and always gets a dynamic property on the base ScriptObject. If the + /// property does not exist on the ScriptObject, this returns `None`. #[no_dynamic] - fn get_string_property_local( - self, - name: AvmString<'gc>, - activation: &mut Activation<'_, 'gc>, - ) -> Result, Error<'gc>> { - let name = Multiname::new(activation.avm2().namespaces.public_vm_internal(), name); - self.get_property_local(&name, activation) + fn get_dynamic_property(self, local_name: AvmString<'gc>) -> Option> { + use crate::avm2::object::script_object::maybe_int_property; + + // See the comment in script_object::get_dynamic_property + let key = maybe_int_property(local_name); + + let base = self.base(); + let values = base.values(); + let value = values.as_hashmap().get(&key); + value.map(|v| v.value) } /// Purely an optimization for "array-like" access. This should return @@ -261,17 +267,6 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + None } - /// Purely an optimization for "array-like" access. This should return - /// `None` when the lookup needs to be forwarded to the base. - fn set_index_property( - self, - _activation: &mut Activation<'_, 'gc>, - _index: usize, - _value: Value<'gc>, - ) -> Option>> { - None - } - /// Set a local property of the object. The Multiname should always be public. /// /// This skips class field lookups and looks at: @@ -287,19 +282,33 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + base.set_property_local(name, value, activation) } - /// Same as set_property_local, but constructs a public Multiname for you. - /// TODO: this feels upside down, as in: we shouldn't need multinames/namespaces - /// by the time we reach dynamic properties. - /// But for now, this function is a smaller change to the core than a full refactor. + /// Set a dynamic property on this Object by name. This is like + /// `set_property_local`, but it skips dynamic-dispatch TObject logic + /// and always sets a dynamic property on the base ScriptObject. #[no_dynamic] - fn set_string_property_local( + fn set_dynamic_property( self, - name: impl Into>, + local_name: AvmString<'gc>, value: Value<'gc>, - activation: &mut Activation<'_, 'gc>, - ) -> Result<(), Error<'gc>> { - let name = Multiname::new(activation.avm2().namespaces.public_vm_internal(), name); - self.set_property_local(&name, value, activation) + mc: &Mutation<'gc>, + ) { + use crate::avm2::object::script_object::maybe_int_property; + + // See the comment in ScriptObjectWrapper::set_property_local + let key = maybe_int_property(local_name); + + self.base().values_mut(mc).insert(key, value); + } + + /// Purely an optimization for "array-like" access. This should return + /// `None` when the lookup needs to be forwarded to the base. + fn set_index_property( + self, + _activation: &mut Activation<'_, 'gc>, + _index: usize, + _value: Value<'gc>, + ) -> Option>> { + None } /// Init a local property of the object. The Multiname should always be public. @@ -341,22 +350,30 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + result.call(activation, self_val, arguments) } - /// Set a dynamic property on this Object by name. This is like - /// `set_property_local`, but it skips dynamic-dispatch TObject logic - /// and always sets a dynamic property on the base ScriptObject. - #[no_dynamic] - fn set_dynamic_property( + /// Delete a property by QName, after multiname resolution and all other + /// considerations have been taken. + /// + /// This required method is only intended to be called by other TObject + /// methods. + fn delete_property_local( self, - local_name: AvmString<'gc>, - value: Value<'gc>, - mc: &Mutation<'gc>, - ) { - use crate::avm2::object::script_object::maybe_int_property; + activation: &mut Activation<'_, 'gc>, + name: &Multiname<'gc>, + ) -> Result> { + let base = self.base(); - // See the comment in ScriptObjectWrapper::set_property_local - let key = maybe_int_property(local_name); + Ok(base.delete_property_local(activation.gc(), name)) + } - self.base().values_mut(mc).insert(key, value); + /// Delete a dynamic property on this Object by name. This is like + /// `delete_property_local`, but it skips dynamic-dispatch TObject logic + /// and always tries to delete a dynamic property on the base ScriptObject. + #[no_dynamic] + fn delete_dynamic_property(self, name: AvmString<'gc>, mc: &Mutation<'gc>) { + use crate::avm2::object::script_object::maybe_int_property; + + let key = maybe_int_property(name); + self.base().values_mut(mc).remove(&key); } /// Retrieve a slot by its index. @@ -439,32 +456,6 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + self.vtable().has_trait(name) } - /// Delete a property by QName, after multiname resolution and all other - /// considerations have been taken. - /// - /// This required method is only intended to be called by other TObject - /// methods. - fn delete_property_local( - self, - activation: &mut Activation<'_, 'gc>, - name: &Multiname<'gc>, - ) -> Result> { - let base = self.base(); - - Ok(base.delete_property_local(activation.gc(), name)) - } - - /// Same as delete_property_local, but constructs a public Multiname for you. - #[no_dynamic] - fn delete_string_property_local( - self, - name: impl Into>, - activation: &mut Activation<'_, 'gc>, - ) -> Result> { - let name = Multiname::new(activation.avm2().namespaces.public_vm_internal(), name); - self.delete_property_local(activation, &name) - } - /// Retrieve the `__proto__` of a given object. /// /// The proto is another object used to resolve methods across a class of From 1a04a2943da0fb678a17910925ab713346a882e2 Mon Sep 17 00:00:00 2001 From: Lord-McSweeney Date: Mon, 14 Jul 2025 04:15:57 -0700 Subject: [PATCH 4/5] avm2: Panic when calling `set_dynamic_property` methods on a sealed object --- core/src/avm2/object.rs | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/core/src/avm2/object.rs b/core/src/avm2/object.rs index 0b1bdc462a3c..f7cc41b46876 100644 --- a/core/src/avm2/object.rs +++ b/core/src/avm2/object.rs @@ -247,7 +247,8 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + /// Get a dynamic property on this Object by name. This is like /// `get_property_local`, but it skips dynamic-dispatch TObject logic /// and always gets a dynamic property on the base ScriptObject. If the - /// property does not exist on the ScriptObject, this returns `None`. + /// object is sealed, or the dynamic property does not exist on the + /// ScriptObject, this returns `None`. #[no_dynamic] fn get_dynamic_property(self, local_name: AvmString<'gc>) -> Option> { use crate::avm2::object::script_object::maybe_int_property; @@ -285,6 +286,14 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + /// Set a dynamic property on this Object by name. This is like /// `set_property_local`, but it skips dynamic-dispatch TObject logic /// and always sets a dynamic property on the base ScriptObject. + /// + /// Note that calling this method on a non-dynamic (sealed) object will + /// panic, as sealed objects cannot have dynamic properties set on them. + /// + /// Additionally, if the vtable of the object has the property `local_name` + /// in it, this method will still declare a dynamic property on the object + /// with the same name, so take care to only call this method on objects + /// that are known to not have the property `local_name` in their vtable. #[no_dynamic] fn set_dynamic_property( self, @@ -294,10 +303,13 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + ) { use crate::avm2::object::script_object::maybe_int_property; + let base = self.base(); + assert!(!base.is_sealed()); + // See the comment in ScriptObjectWrapper::set_property_local let key = maybe_int_property(local_name); - self.base().values_mut(mc).insert(key, value); + base.values_mut(mc).insert(key, value); } /// Purely an optimization for "array-like" access. This should return @@ -368,12 +380,18 @@ pub trait TObject<'gc>: 'gc + Collect<'gc> + Debug + Into> + Clone + /// Delete a dynamic property on this Object by name. This is like /// `delete_property_local`, but it skips dynamic-dispatch TObject logic /// and always tries to delete a dynamic property on the base ScriptObject. + /// + /// This method will panic when called on a non-dynamic (sealed) object, as + /// sealed objects don't have dynamic properties to delete anyway. #[no_dynamic] fn delete_dynamic_property(self, name: AvmString<'gc>, mc: &Mutation<'gc>) { use crate::avm2::object::script_object::maybe_int_property; + let base = self.base(); + assert!(!base.is_sealed()); + let key = maybe_int_property(name); - self.base().values_mut(mc).remove(&key); + base.values_mut(mc).remove(&key); } /// Retrieve a slot by its index. From b40044924c4b5a039e74c988fd69614f18c26143 Mon Sep 17 00:00:00 2001 From: Lord-McSweeney Date: Wed, 16 Jul 2025 01:17:18 -0700 Subject: [PATCH 5/5] avm2: Factor out array index parsing logic into a function --- core/src/avm2/amf.rs | 10 +++++----- core/src/avm2/object/array_object.rs | 17 +++++++++++------ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/core/src/avm2/amf.rs b/core/src/avm2/amf.rs index ff4fa5fb8ade..3223aac9f859 100644 --- a/core/src/avm2/amf.rs +++ b/core/src/avm2/amf.rs @@ -313,16 +313,16 @@ pub fn deserialize_value_impl<'gc>( // Now let's add each element as a property for element in elements { - let name = element.name(); + let name = ruffle_wstr::from_utf8(element.name()); let value = deserialize_value_impl(activation, element.value(), object_map)?; - // If the name of the element was numerical, we set an element on - // the array instead of setting a dynamic property. - if let Ok(index) = name.parse::() { + // If the name of the element was a valid array index, we set an + // element on the array instead of setting a dynamic property. + if let Some(index) = ArrayObject::as_array_index(&name) { array.set_element(activation.gc(), index, value); } else { array.set_dynamic_property( - AvmString::new_utf8(activation.gc(), name), + AvmString::new(activation.gc(), name), value, activation.gc(), ); diff --git a/core/src/avm2/object/array_object.rs b/core/src/avm2/object/array_object.rs index 35b6e51ab108..18bfb8376c32 100644 --- a/core/src/avm2/object/array_object.rs +++ b/core/src/avm2/object/array_object.rs @@ -7,7 +7,7 @@ use crate::avm2::object::{ClassObject, Object, ObjectPtr, TObject}; use crate::avm2::value::Value; use crate::avm2::Error; use crate::avm2::Multiname; -use crate::string::AvmString; +use crate::string::{AvmString, WStr}; use crate::utils::HasPrefixField; use core::fmt; use gc_arena::barrier::unlock; @@ -84,6 +84,11 @@ impl<'gc> ArrayObject<'gc> { )) } + pub fn as_array_index(local_name: &WStr) -> Option { + // TODO match avmplus + local_name.parse::().ok() + } + pub fn set_element(self, mc: &Mutation<'gc>, index: usize, value: Value<'gc>) { unlock!(Gc::write(mc, self.0), ArrayObjectData, array) .borrow_mut() @@ -115,7 +120,7 @@ impl<'gc> TObject<'gc> for ArrayObject<'gc> { ) -> Result, Error<'gc>> { if name.valid_dynamic_name() { if let Some(name) = name.local_name() { - if let Ok(index) = name.parse::() { + if let Some(index) = ArrayObject::as_array_index(&name) { if let Some(result) = self.get_index_property(index) { return Ok(result); } @@ -151,7 +156,7 @@ impl<'gc> TObject<'gc> for ArrayObject<'gc> { if name.valid_dynamic_name() { if let Some(name) = name.local_name() { - if let Ok(index) = name.parse::() { + if let Some(index) = ArrayObject::as_array_index(&name) { self.set_element(mc, index, value); return Ok(()); @@ -172,7 +177,7 @@ impl<'gc> TObject<'gc> for ArrayObject<'gc> { if name.valid_dynamic_name() { if let Some(name) = name.local_name() { - if let Ok(index) = name.parse::() { + if let Some(index) = ArrayObject::as_array_index(&name) { self.set_element(mc, index, value); return Ok(()); @@ -192,7 +197,7 @@ impl<'gc> TObject<'gc> for ArrayObject<'gc> { if name.valid_dynamic_name() { if let Some(name) = name.local_name() { - if let Ok(index) = name.parse::() { + if let Some(index) = ArrayObject::as_array_index(&name) { unlock!(Gc::write(mc, self.0), ArrayObjectData, array) .borrow_mut() .delete(index); @@ -208,7 +213,7 @@ impl<'gc> TObject<'gc> for ArrayObject<'gc> { fn has_own_property(self, name: &Multiname<'gc>) -> bool { if name.valid_dynamic_name() { if let Some(name) = name.local_name() { - if let Ok(index) = name.parse::() { + if let Some(index) = ArrayObject::as_array_index(&name) { return self.0.array.borrow().get(index).is_some(); } }