avm2: Improve XML set_property_local implementation (#13041)

* avm2: Add error 1087

* avm2: Add deep_copy method to XmlObject

Changes existing AS3 copy method to use this instead

* avm2: Add a few utility methods to E4XNode

* avm2: Change XML.replace impl to use new utility methods

* avm2: allow setting XML/XMLList as a element

* avm2: Fix `xml_namespaced_property` test failure

* chore: fmt

* chore: Appease clippy

* avm2: Add test for XML/XMLList as element

core/src/avm2/e4x.rs

@@ -244,6 +244,57 @@ impl<'gc> E4XNode<'gc> {
         node
     }
 
+    /// Returns the amount of children in this node if this node is of Element kind, otherwise returns [None].
+    pub fn length(&self) -> Option<usize> {
+        let E4XNodeKind::Element { children, .. } = &*self.kind() else {
+            return None;
+        };
+
+        Some(children.len())
+    }
+
+    /// Removes all matching children matching provided name, returns the first child removed along with its index (if any).
+    pub fn remove_matching_children(
+        &self,
+        gc_context: &Mutation<'gc>,
+        name: &Multiname<'gc>,
+    ) -> Option<(usize, E4XNode<'gc>)> {
+        let E4XNodeKind::Element { children, .. } = &mut *self.kind_mut(gc_context) else {
+            return None;
+        };
+
+        let index = children
+            .iter()
+            .position(|x| name.is_any_name() || x.matches_name(name));
+
+        let val = if let Some(index) = index {
+            Some((index, children[index]))
+        } else {
+            None
+        };
+
+        children.retain(|x| {
+            if name.is_any_name() || x.matches_name(name) {
+                // Remove parent.
+                x.set_parent(None, gc_context);
+                false
+            } else {
+                true
+            }
+        });
+
+        val
+    }
+
+    pub fn insert_at(&self, gc_context: &Mutation<'gc>, index: usize, node: E4XNode<'gc>) {
+        let E4XNodeKind::Element { children, .. } = &mut *self.kind_mut(gc_context) else {
+            return;
+        };
+
+        node.set_parent(Some(*self), gc_context);
+        children.insert(index, node);
+    }
+
     pub fn remove_all_children(&self, gc_context: &Mutation<'gc>) {
         let mut this = self.0.write(gc_context);
         if let E4XNodeKind::Element { children, .. } = &mut this.kind {

core/src/avm2/error.rs

@@ -186,6 +186,20 @@ pub fn make_error_1004<'gc>(activation: &mut Activation<'_, 'gc>, method_name: &
     }
 }
 
+#[inline(never)]
+#[cold]
+pub fn make_error_1087<'gc>(activation: &mut Activation<'_, 'gc>) -> Error<'gc> {
+    let err = type_error(
+        activation,
+        "Error #1087: Assignment to indexed XML is not allowed.",
+        1087,
+    );
+    match err {
+        Ok(err) => Error::AvmError(err),
+        Err(err) => err,
+    }
+}
+
 #[inline(never)]
 #[cold]
 pub fn make_error_1118<'gc>(activation: &mut Activation<'_, 'gc>) -> Error<'gc> {

core/src/avm2/globals/xml.rs

@@ -239,8 +239,7 @@ pub fn copy<'gc>(
     _args: &[Value<'gc>],
 ) -> Result<Value<'gc>, Error<'gc>> {
     let xml = this.as_xml_object().unwrap();
-    let node = xml.node();
+    Ok(xml.deep_copy(activation).into())
-    Ok(XmlObject::new(node.deep_copy(activation.context.gc_context), activation).into())
 }
 
 pub fn parent<'gc>(
@@ -690,29 +689,12 @@ pub fn replace<'gc>(
     //       2. And then we insert a dummy E4XNode at the previously stored index, and use the replace method to correct it.
 
     let index =
-        if let E4XNodeKind::Element { children, .. } = &mut *self_node.kind_mut(activation.gc()) {
+        if let Some((index, _)) = self_node.remove_matching_children(activation.gc(), &multiname) {
-            let index = children
+            self_node.insert_at(activation.gc(), index, E4XNode::dummy(activation.gc()));
-                .iter()
+            index
-                .position(|x| multiname.is_any_name() || x.matches_name(&multiname));
+        // 8. If i == undefined, return x
-            children.retain(|x| {
-                if multiname.is_any_name() || x.matches_name(&multiname) {
-                    // Remove parent.
-                    x.set_parent(None, activation.gc());
-                    false
-                } else {
-                    true
-                }
-            });
-
-            if let Some(index) = index {
-                children.insert(index, E4XNode::dummy(activation.gc()));
-                index
-            // 8. If i == undefined, return x
-            } else {
-                return Ok(xml.into());
-            }
         } else {
-            unreachable!("E4XNode kind should be of element kind");
+            return Ok(xml.into());
         };
 
     // 9. Call the [[Replace]] method of x with arguments ToString(i) and c

core/src/avm2/object/xml_object.rs

@@ -2,6 +2,7 @@
 
 use crate::avm2::activation::Activation;
 use crate::avm2::e4x::{name_to_multiname, E4XNode, E4XNodeKind};
+use crate::avm2::error::make_error_1087;
 use crate::avm2::object::script_object::ScriptObjectData;
 use crate::avm2::object::{ClassObject, Object, ObjectPtr, TObject, XmlListObject};
 use crate::avm2::string::AvmString;
@@ -82,6 +83,11 @@ impl<'gc> XmlObject<'gc> {
         Ref::map(self.0.read(), |data| &data.node)
     }
 
+    pub fn deep_copy(&self, activation: &mut Activation<'_, 'gc>) -> XmlObject<'gc> {
+        let node = self.node();
+        XmlObject::new(node.deep_copy(activation.gc()), activation)
+    }
+
     pub fn equals(
         &self,
         other: &Value<'gc>,
@@ -302,15 +308,41 @@ impl<'gc> TObject<'gc> for XmlObject<'gc> {
         Ok(self.has_own_property(&name))
     }
 
+    // ECMA-357 9.1.1.2 [[Put]] (P, V)
     fn set_property_local(
         self,
         name: &Multiname<'gc>,
         value: Value<'gc>,
         activation: &mut Activation<'_, 'gc>,
     ) -> Result<(), Error<'gc>> {
-        let mc = activation.context.gc_context;
+        // 1. If ToString(ToUint32(P)) == P, throw a TypeError exception
+        if let Some(local_name) = name.local_name() {
+            if local_name.parse::<usize>().is_ok() {
+                return Err(make_error_1087(activation));
+            }
+        }
 
+        // 2. If x.[[Class]] ∈ {"text", "comment", "processing-instruction", "attribute"}, return
+        if !matches!(*self.node().kind(), E4XNodeKind::Element { .. }) {
+            return Ok(());
+        }
+
+        // 4. Else
+        // 4.a. Let c be the result of calling the [[DeepCopy]] method of V
+        let value = if let Some(xml) = value.as_object().and_then(|x| x.as_xml_object()) {
+            xml.deep_copy(activation).into()
+        } else if let Some(list) = value.as_object().and_then(|x| x.as_xml_list_object()) {
+            list.deep_copy(activation).into()
+        // 3. If (Type(V) ∉ {XML, XMLList}) or (V.[[Class]] ∈ {"text", "attribute"})
+        // 3.a. Let c = ToString(V)
+        } else {
+            value
+        };
+
+        // 5. Let n = ToXMLName(P)
+        // 6. If Type(n) is AttributeName
         if name.is_attribute() {
+            let mc = activation.context.gc_context;
             self.delete_property_local(activation, name)?;
             let Some(local_name) = name.local_name() else {
                 return Err(format!("Cannot set attribute {:?} without a local name", name).into());
@@ -328,50 +360,84 @@ impl<'gc> TObject<'gc> for XmlObject<'gc> {
             return Ok(());
         }
 
-        let self_node = *self.node();
+        // 7. Let isValidName be the result of calling the function isXMLName (section 13.1.2.1) with argument n
-        let write = self.0.write(mc);
+        let is_valid_name = name.local_name().map_or(Ok(false), |x| {
-        let mut kind = write.node.kind_mut(mc);
+            crate::avm2::e4x::is_xml_name(activation, x.into())
-        let E4XNodeKind::Element { children, .. } = &mut *kind else {
+        })?;
+        // 8. If isValidName is false and n.localName is not equal to the string "*", return
+        if !is_valid_name && !name.is_any_name() {
             return Ok(());
-        };
-
-        if value.as_object().map_or(false, |obj| {
-            obj.as_xml_object().is_some() || obj.as_xml_list_object().is_some()
-        }) {
-            return Err(
-                format!("Cannot set an XML/XMLList object {value:?} as an element yet").into(),
-            );
         }
 
-        if name.is_any_name() {
+        // 10. Let primitiveAssign = (Type(c) ∉ {XML, XMLList}) and (n.localName is not equal to the string "*")
-            return Err("Any name (*) not yet implemented for set".into());
+        let primitive_assign = !value.as_object().map_or(false, |x| {
-        }
+            x.as_xml_list_object().is_some() || x.as_xml_object().is_some()
+        }) && !name.is_any_name();
 
-        let text = value.coerce_to_string(activation)?;
+        let self_node = self.node();
 
-        let matches: Vec<_> = children
+        // 9. Let i = undefined
-            .iter()
+        // 11.
-            .filter(|child| child.matches_name(name))
+        let index = self_node.remove_matching_children(activation.gc(), name);
-            .collect();
+
-        match matches.as_slice() {
+        let index = if let Some((index, node)) = index {
-            [] => {
+            self_node.insert_at(activation.gc(), index, node);
-                let element_with_text = E4XNode::element(
+            index
-                    mc,
+        // 12. If i == undefined
+        } else {
+            // 12.a. Let i = x.[[Length]]
+            let index = self_node.length().expect("Node should be of element kind");
+            self_node.insert_at(activation.gc(), index, E4XNode::dummy(activation.gc()));
+
+            // 12.b. If (primitiveAssign == true)
+            if primitive_assign {
+                // 12.b.i. If (n.uri == null)
+                // 12.b.i.1. Let name be a new QName created as if by calling the constructor new
+                //           QName(GetDefaultNamespace(), n)
+                // 12.b.ii. Else
+                // 12.b.ii.1. Let name be a new QName created as if by calling the constructor new QName(n)
+
+                // 12.b.iii. Create a new XML object y with y.[[Name]] = name, y.[[Class]] = "element" and y.[[Parent]] = x
+                let node = E4XNode::element(
+                    activation.gc(),
                     name.explict_namespace(),
                     name.local_name().unwrap(),
-                    write.node,
+                    *self_node,
                 );
-                element_with_text.append_child(mc, E4XNode::text(mc, text, Some(self_node)))?;
+                // 12.b.v. Call the [[Replace]] method of x with arguments ToString(i) and y
-                children.push(element_with_text);
+                self_node.replace(index, XmlObject::new(node, activation).into(), activation)?;
-                Ok(())
+                // FIXME: 12.b.iv. Let ns be the result of calling [[GetNamespace]] on name with no arguments
+                // 12.b.vi. Call [[AddInScopeNamespace]] on y with argument ns
             }
-            [child] => {
+
-                child.remove_all_children(mc);
+            index
-                child.append_child(mc, E4XNode::text(mc, text, Some(self_node)))?;
+        };
-                Ok(())
+
+        // 13. If (primitiveAssign == true)
+        if primitive_assign {
+            let E4XNodeKind::Element { children, .. } = &mut *self_node.kind_mut(activation.gc())
+            else {
+                unreachable!("Node should be of Element kind");
+            };
+
+            // 13.a. Delete all the properties of the XML object x[i]
+            children[index].remove_all_children(activation.gc());
+
+            // 13.b. Let s = ToString(c)
+            let val = value.coerce_to_string(activation)?;
+
+            // 13.c. If s is not the empty string, call the [[Replace]] method of x[i] with arguments "0" and s
+            if !val.is_empty() {
+                children[index].replace(0, value, activation)?;
             }
-            _ => Err(format!("Can not replace multiple elements yet: {name:?} = {value:?}").into()),
+        // 14. Else
+        } else {
+            // 14.a. Call the [[Replace]] method of x with arguments ToString(i) and c
+            self_node.replace(index, value, activation)?;
         }
+
+        // 15. Return
+        Ok(())
     }
 
     fn delete_property_local(

tests/tests/swfs/avm2/xml_advanced/Test.as

@@ -0,0 +1,18 @@
+package {
+import flash.display.Sprite;
+
+public class Test extends Sprite {}
+}
+
+var main:XML = <root><hello/></root>;
+trace(main);
+
+trace("Set XML as child");
+var xmlChild:XML = <item><stuff>Hello</stuff><more><stuff/></more></item>;
+main.p = xmlChild;
+trace(main);
+
+trace("Set XMLList as child");
+var listChild:XMLList = new XMLList("<list><item>A</item><item>B</item><item>C</item></list>");
+main.x = listChild;
+trace(main);

tests/tests/swfs/avm2/xml_advanced/output.txt

@@ -0,0 +1,28 @@
+<root>
+  <hello/>
+</root>
+Set XML as child
+<root>
+  <hello/>
+  <item>
+    <stuff>Hello</stuff>
+    <more>
+      <stuff/>
+    </more>
+  </item>
+</root>
+Set XMLList as child
+<root>
+  <hello/>
+  <item>
+    <stuff>Hello</stuff>
+    <more>
+      <stuff/>
+    </more>
+  </item>
+  <list>
+    <item>A</item>
+    <item>B</item>
+    <item>C</item>
+  </list>
+</root>

tests/tests/swfs/avm2/xml_advanced/test.toml

@@ -0,0 +1 @@
+num_ticks = 1

tests/tests/swfs/from_avmplus/as3/RuntimeErrors/Error1087XmlAssignToIndexedXml/test.toml

@@ -1,2 +1 @@
 num_ticks = 1
-known_failure = true

tests/tests/swfs/from_avmplus/e4x/Regress/regress-263936/test.toml

@@ -1,2 +1 @@
 num_ticks = 1
-known_failure = true

tests/tests/swfs/from_avmplus/e4x/XML/kXMLAssignmentToIndexedXMLNotAllowedErr/test.toml

@@ -1,2 +1 @@
 num_ticks = 1
-known_failure = true