From ecbf325d415747c1529c09db8d3d37209f29ec64 Mon Sep 17 00:00:00 2001 From: sleepycatcoding <131554884+sleepycatcoding@users.noreply.github.com> Date: Fri, 15 Sep 2023 00:33:32 +0300 Subject: [PATCH] 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 | 51 +++++++ core/src/avm2/error.rs | 14 ++ core/src/avm2/globals/xml.rs | 30 +--- core/src/avm2/object/xml_object.rs | 134 +++++++++++++----- tests/tests/swfs/avm2/xml_advanced/Test.as | 18 +++ tests/tests/swfs/avm2/xml_advanced/output.txt | 28 ++++ tests/tests/swfs/avm2/xml_advanced/test.swf | Bin 0 -> 969 bytes tests/tests/swfs/avm2/xml_advanced/test.toml | 1 + .../Error1087XmlAssignToIndexedXml/test.toml | 1 - .../e4x/Regress/regress-263936/test.toml | 1 - .../test.toml | 1 - 11 files changed, 218 insertions(+), 61 deletions(-) create mode 100644 tests/tests/swfs/avm2/xml_advanced/Test.as create mode 100644 tests/tests/swfs/avm2/xml_advanced/output.txt create mode 100644 tests/tests/swfs/avm2/xml_advanced/test.swf create mode 100644 tests/tests/swfs/avm2/xml_advanced/test.toml diff --git a/core/src/avm2/e4x.rs b/core/src/avm2/e4x.rs index ae75e86f2..95b1e5a1b 100644 --- a/core/src/avm2/e4x.rs +++ b/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 { + 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 { diff --git a/core/src/avm2/error.rs b/core/src/avm2/error.rs index c85c94995..07a75f6ff 100644 --- a/core/src/avm2/error.rs +++ b/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> { diff --git a/core/src/avm2/globals/xml.rs b/core/src/avm2/globals/xml.rs index 17156e41e..d067a3214 100644 --- a/core/src/avm2/globals/xml.rs +++ b/core/src/avm2/globals/xml.rs @@ -239,8 +239,7 @@ pub fn copy<'gc>( _args: &[Value<'gc>], ) -> Result, Error<'gc>> { let xml = this.as_xml_object().unwrap(); - let node = xml.node(); - Ok(XmlObject::new(node.deep_copy(activation.context.gc_context), activation).into()) + Ok(xml.deep_copy(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()) { - let index = children - .iter() - .position(|x| multiname.is_any_name() || x.matches_name(&multiname)); - 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()); - } + if let Some((index, _)) = self_node.remove_matching_children(activation.gc(), &multiname) { + self_node.insert_at(activation.gc(), index, E4XNode::dummy(activation.gc())); + index + // 8. If i == undefined, return x } 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 diff --git a/core/src/avm2/object/xml_object.rs b/core/src/avm2/object/xml_object.rs index 4ae28111f..e39ace370 100644 --- a/core/src/avm2/object/xml_object.rs +++ b/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::().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(); - let write = self.0.write(mc); - let mut kind = write.node.kind_mut(mc); - let E4XNodeKind::Element { children, .. } = &mut *kind else { + // 7. Let isValidName be the result of calling the function isXMLName (section 13.1.2.1) with argument n + let is_valid_name = name.local_name().map_or(Ok(false), |x| { + crate::avm2::e4x::is_xml_name(activation, x.into()) + })?; + // 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() { - return Err("Any name (*) not yet implemented for set".into()); - } + // 10. Let primitiveAssign = (Type(c) ∉ {XML, XMLList}) and (n.localName is not equal to the string "*") + 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 - .iter() - .filter(|child| child.matches_name(name)) - .collect(); - match matches.as_slice() { - [] => { - let element_with_text = E4XNode::element( - mc, + // 9. Let i = undefined + // 11. + let index = self_node.remove_matching_children(activation.gc(), name); + + let index = if let Some((index, node)) = index { + self_node.insert_at(activation.gc(), index, node); + index + // 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)))?; - children.push(element_with_text); - Ok(()) + // 12.b.v. Call the [[Replace]] method of x with arguments ToString(i) and y + self_node.replace(index, XmlObject::new(node, activation).into(), activation)?; + // 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); - child.append_child(mc, E4XNode::text(mc, text, Some(self_node)))?; - Ok(()) + + index + }; + + // 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( diff --git a/tests/tests/swfs/avm2/xml_advanced/Test.as b/tests/tests/swfs/avm2/xml_advanced/Test.as new file mode 100644 index 000000000..a8459e8a4 --- /dev/null +++ b/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 = ; +trace(main); + +trace("Set XML as child"); +var xmlChild:XML = Hello; +main.p = xmlChild; +trace(main); + +trace("Set XMLList as child"); +var listChild:XMLList = new XMLList("ABC"); +main.x = listChild; +trace(main); \ No newline at end of file diff --git a/tests/tests/swfs/avm2/xml_advanced/output.txt b/tests/tests/swfs/avm2/xml_advanced/output.txt new file mode 100644 index 000000000..c7fc5b8b1 --- /dev/null +++ b/tests/tests/swfs/avm2/xml_advanced/output.txt @@ -0,0 +1,28 @@ + + + +Set XML as child + + + + Hello + + + + + +Set XMLList as child + + + + Hello + + + + + + A + B + C + + diff --git a/tests/tests/swfs/avm2/xml_advanced/test.swf b/tests/tests/swfs/avm2/xml_advanced/test.swf new file mode 100644 index 0000000000000000000000000000000000000000..277d36a425cdef273d98ab7704fe9d60b042d0b3 GIT binary patch literal 969 zcmV;)12+6aS5qds1poke0fkddZ{tK5o*6s-NYgg`*t7+9Hw#D=qKTbE6x#Jh%{G)> zSSZ?nSRq-_)E+mcj&0eKCVN8Sz=0pa4fqM1IB+Wm{s1Sqg%8$jSU*|9SVQIvGZBz7Ocp_JH$r-?GNzaedsX zC)ugh*xTE)?55Rh)__sVU&fqHYH|M7c3T>BS?*4mzQ4`6-a3? z)biQyE$9=3e*Wd&_dmewx8fHp=jhk`Rvvohr(}=#*dTADNrX8Pr!#uwH;W@NN7gChN;bx^7mx*Y;DB>WQ zQCFVEoMqJG;W?W{D!1~733O~2vrHBb6};Wtdob?+>L(|x?j=L3(`#8x+upTKMq$Lm zSb6d3vpY_fB)shmSQI6e*iius92R5c9^+NuRB2xI24UpyI#B(v?c{tE1nnn6;#f1_ z43i9wNCIIqgO=pubbaphEX?_}n-10@@V*1h2PH^u$C+tn=|%B|1|0I zr0=sJj6-pu`tT#Ek|sHMucZ!QPsj%?11|res9$O9Hcg?Op_bh+3%Eimg-u*B3yT}X zSW*f^(R4#0nz5=AtOFprUNCf}s4EM)QZhBHlVx*-nCrx>5Ob60+8erdPuHqCsX)&V z=o1wNLFyKM?R6M;VcduD0ApgRvZ*jpaTmjM>42CBU!u|+a z5M#4gys$5xqJNd?8bk;o*!T%w*nm}rTqRI!R}EC#Ed&MI)dJ0S^+2~>5)j)p0^;71Bwm@eLsmw|jo rlFM#sXXE3Nuv4cilB?3i>FO2tyFdSUV!%Bz*RC(q0qo9yKX#n)O2y>% literal 0 HcmV?d00001 diff --git a/tests/tests/swfs/avm2/xml_advanced/test.toml b/tests/tests/swfs/avm2/xml_advanced/test.toml new file mode 100644 index 000000000..cf6123969 --- /dev/null +++ b/tests/tests/swfs/avm2/xml_advanced/test.toml @@ -0,0 +1 @@ +num_ticks = 1 diff --git a/tests/tests/swfs/from_avmplus/as3/RuntimeErrors/Error1087XmlAssignToIndexedXml/test.toml b/tests/tests/swfs/from_avmplus/as3/RuntimeErrors/Error1087XmlAssignToIndexedXml/test.toml index 29f3cef79..cf6123969 100644 --- a/tests/tests/swfs/from_avmplus/as3/RuntimeErrors/Error1087XmlAssignToIndexedXml/test.toml +++ b/tests/tests/swfs/from_avmplus/as3/RuntimeErrors/Error1087XmlAssignToIndexedXml/test.toml @@ -1,2 +1 @@ num_ticks = 1 -known_failure = true diff --git a/tests/tests/swfs/from_avmplus/e4x/Regress/regress-263936/test.toml b/tests/tests/swfs/from_avmplus/e4x/Regress/regress-263936/test.toml index 29f3cef79..cf6123969 100644 --- a/tests/tests/swfs/from_avmplus/e4x/Regress/regress-263936/test.toml +++ b/tests/tests/swfs/from_avmplus/e4x/Regress/regress-263936/test.toml @@ -1,2 +1 @@ num_ticks = 1 -known_failure = true diff --git a/tests/tests/swfs/from_avmplus/e4x/XML/kXMLAssignmentToIndexedXMLNotAllowedErr/test.toml b/tests/tests/swfs/from_avmplus/e4x/XML/kXMLAssignmentToIndexedXMLNotAllowedErr/test.toml index 29f3cef79..cf6123969 100644 --- a/tests/tests/swfs/from_avmplus/e4x/XML/kXMLAssignmentToIndexedXMLNotAllowedErr/test.toml +++ b/tests/tests/swfs/from_avmplus/e4x/XML/kXMLAssignmentToIndexedXMLNotAllowedErr/test.toml @@ -1,2 +1 @@ num_ticks = 1 -known_failure = true