avm1: More accurate handling of preload/suppress flags in functions

moulins · Herschel · commit c7bf11ece542 · 2022-06-29T16:02:13.000-07:00
- Handle the case where both preload aud suppress flags are
set for the same variable;
- Remove `arguments` field in `Activation`; instead use a normal
local definition;
- When `suppress_this` is set, inherit the `this` value from parent
activation. (This isn't entirely correct, as FP's `this` is mutable
and seems to be part of the scope chain, but this would require a
larger refactoring)
diff --git a/core/src/avm1.rs b/core/src/avm1.rs
@@ -211,7 +211,6 @@ impl<'gc> Avm1<'gc> {
             active_clip,
             clip_obj.into(),
             None,
-            None,
         );
         if let Err(e) = child_activation.run_actions(code) {
             root_error_handler(&mut child_activation, e);
@@ -251,7 +250,6 @@ impl<'gc> Avm1<'gc> {
             active_clip,
             clip_obj.into(),
             None,
-            None,
         );
         function(&mut activation)
     }
@@ -300,7 +298,6 @@ impl<'gc> Avm1<'gc> {
             active_clip,
             clip_obj.into(),
             None,
-            None,
         );
         if let Err(e) = child_activation.run_actions(code) {
             root_error_handler(&mut child_activation, e);
diff --git a/core/src/avm1/activation.rs b/core/src/avm1/activation.rs
@@ -194,14 +194,20 @@ pub struct Activation<'a, 'gc: 'a, 'gc_context: 'a> {
     constant_pool: GcCell<'gc, Vec<Value<'gc>>>,
 
     /// The immutable value of `this`.
+    ///
+    /// This differs from Flash Player, where `this` is mutable and seems
+    /// to be part of the scope chain (e.g. a function with the `suppress_this` flag
+    /// set can modify the `this` value of its closure).
+    ///
+    /// Fortunately, ActionScript syntax prevents mutating `this` altogether, so
+    /// observing this behavior requires manually-crafted bytecode.
+    ///
+    /// TODO: implement correct semantics for mutable `this`.
     this: Value<'gc>,
 
     /// The function object being called.
     pub callee: Option<Object<'gc>>,
 
-    /// The arguments this function was called by.
-    pub arguments: Option<Object<'gc>>,
-
     /// Local registers, if any.
     ///
     /// None indicates a function executing out of the global register set.
@@ -254,7 +260,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
         base_clip: DisplayObject<'gc>,
         this: Value<'gc>,
         callee: Option<Object<'gc>>,
-        arguments: Option<Object<'gc>>,
     ) -> Self {
         avm_debug!(context.avm1, "START {}", id);
         Self {
@@ -268,7 +273,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
             base_clip_unloaded: base_clip.removed(),
             this,
             callee,
-            arguments,
             local_registers: None,
             actions_since_timeout_check: 0,
         }
@@ -293,7 +297,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
             base_clip_unloaded: self.base_clip_unloaded,
             this: self.this,
             callee: self.callee,
-            arguments: self.arguments,
             local_registers: self.local_registers,
             actions_since_timeout_check: 0,
         }
@@ -329,7 +332,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
             base_clip_unloaded: base_clip.removed(),
             this: globals.into(),
             callee: None,
-            arguments: None,
             local_registers: None,
             actions_since_timeout_check: 0,
         }
@@ -383,7 +385,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
             active_clip,
             clip_obj.into(),
             None,
-            None,
         );
         child_activation.run_actions(code)
     }
@@ -421,7 +422,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
             active_clip,
             clip_obj.into(),
             None,
-            None,
         );
         function(&mut activation)
     }
@@ -2131,7 +2131,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
                     self.base_clip,
                     self.this,
                     self.callee,
-                    self.arguments,
                 );
 
                 match catch_vars {
@@ -2737,12 +2736,6 @@ impl<'a, 'gc, 'gc_context> Activation<'a, 'gc, 'gc_context> {
             return Ok(CallableValue::UnCallable(self.this_cell()));
         }
 
-        if &name == b"arguments" && self.arguments.is_some() {
-            return Ok(CallableValue::UnCallable(Value::Object(
-                self.arguments.unwrap(),
-            )));
-        }
-
         self.scope_cell().read().resolve(name, self)
     }
 
diff --git a/core/src/avm1/function.rs b/core/src/avm1/function.rs
@@ -161,33 +161,83 @@ impl<'gc> Avm1Function<'gc> {
     }
 
     fn load_this(&self, frame: &mut Activation<'_, 'gc, '_>, this: Value<'gc>, preload_r: &mut u8) {
-        if self.flags.contains(FunctionFlags::PRELOAD_THIS) {
-            //TODO: What happens if you specify both suppress and
-            //preload for this?
+        let preload = self.flags.contains(FunctionFlags::PRELOAD_THIS);
+        let suppress = self.flags.contains(FunctionFlags::SUPPRESS_THIS);
+
+        if preload {
+            // The register is set to undefined if both flags are set.
+            let this = if suppress { Value::Undefined } else { this };
             frame.set_local_register(*preload_r, this);
             *preload_r += 1;
         }
     }
 
-    fn load_arguments(&self, frame: &mut Activation<'_, 'gc, '_>, arguments: Value<'gc>, preload_r: &mut u8) {
-        if self.flags.contains(FunctionFlags::PRELOAD_ARGUMENTS) {
-            //TODO: What happens if you specify both suppress and
-            //preload for arguments?
+    fn load_arguments(
+        &self,
+        frame: &mut Activation<'_, 'gc, '_>,
+        args: &[Value<'gc>],
+        caller: Option<Object<'gc>>,
+        preload_r: &mut u8,
+    ) {
+        let preload = self.flags.contains(FunctionFlags::PRELOAD_ARGUMENTS);
+        let suppress = self.flags.contains(FunctionFlags::SUPPRESS_ARGUMENTS);
+
+        if suppress && !preload {
+            return;
+        }
+
+        let arguments = ArrayObject::new(
+            frame.context.gc_context,
+            frame.context.avm1.prototypes().array,
+            args.iter().cloned(),
+        );
+
+        arguments.define_value(
+            frame.context.gc_context,
+            "callee",
+            frame.callee.unwrap().into(),
+            Attribute::DONT_ENUM,
+        );
+
+        arguments.define_value(
+            frame.context.gc_context,
+            "caller",
+            caller.map(Value::from).unwrap_or(Value::Null),
+            Attribute::DONT_ENUM,
+        );
+
+        let arguments = Value::from(arguments);
+
+        // Contrarily to `this` and `super`, setting both flags is equivalent to just setting `preload`.
+        if preload {
             frame.set_local_register(*preload_r, arguments);
             *preload_r += 1;
+        } else {
+            frame.force_define_local("arguments".into(), arguments);
         }
     }
 
-    fn load_super(&self, frame: &mut Activation<'_, 'gc, '_>, super_object: Option<Object<'gc>>, preload_r: &mut u8) {
-        if let Some(super_object) = super_object {
-            if self.flags.contains(FunctionFlags::PRELOAD_SUPER) {
-                frame.set_local_register(*preload_r, super_object.into());
-                //TODO: What happens if you specify both suppress and
-                //preload for super?
-                *preload_r += 1;
-            } else {
-                frame.force_define_local("super".into(), super_object.into());
-            }
+    fn load_super(
+        &self,
+        frame: &mut Activation<'_, 'gc, '_>,
+        this: Option<Object<'gc>>,
+        depth: u8,
+        preload_r: &mut u8,
+    ) {
+        let preload = self.flags.contains(FunctionFlags::PRELOAD_SUPER);
+        let suppress = self.flags.contains(FunctionFlags::SUPPRESS_SUPER);
+
+        // TODO: `super` should only be defined if this was a method call (depth > 0?)
+        // `f[""]()` emits a CallMethod op, causing `this` to be undefined, but `super` is a function; what is it?
+        let zuper = this
+            .filter(|_| !suppress)
+            .map(|this| SuperObject::new(frame, this, depth).into());
+
+        if preload {
+            // The register is set to undefined if both flags are set.
+            frame.set_local_register(*preload_r, zuper.unwrap_or(Value::Undefined));
+        } else if let Some(zuper) = zuper {
+            frame.force_define_local("super".into(), zuper);
         }
     }
 
@@ -309,15 +359,14 @@ impl<'gc> Executable<'gc> {
 
         let target = activation.target_clip_or_root();
         let is_closure = activation.swf_version() >= 6;
-        let base_clip = if (is_closure || reason == ExecutionReason::Special)
-            && !af.base_clip.removed()
-        {
-            af.base_clip
-        } else {
-            this_obj
-                .and_then(|this| this.as_display_object())
-                .unwrap_or(target)
-        };
+        let base_clip =
+            if (is_closure || reason == ExecutionReason::Special) && !af.base_clip.removed() {
+                af.base_clip
+            } else {
+                this_obj
+                    .and_then(|this| this.as_display_object())
+                    .unwrap_or(target)
+            };
         let (swf_version, parent_scope) = if is_closure {
             // Function calls in a v6+ SWF are proper closures, and "close" over the scope that defined the function:
             // * Use the SWF version from the SWF that defined the function.
@@ -355,45 +404,24 @@ impl<'gc> Executable<'gc> {
             Scope::new_local_scope(parent_scope, activation.context.gc_context),
         );
 
-        let arguments = if af.flags.contains(FunctionFlags::SUPPRESS_ARGUMENTS) {
-            ArrayObject::empty(activation)
-        } else {
-            ArrayObject::new(
-                activation.context.gc_context,
-                activation.context.avm1.prototypes().array,
-                args.iter().cloned(),
-            )
-        };
-        arguments.define_value(
-            activation.context.gc_context,
-            "callee",
-            callee.into(),
-            Attribute::DONT_ENUM,
-        );
         // The caller is the previous callee.
-        arguments.define_value(
-            activation.context.gc_context,
-            "caller",
-            activation.callee.map(Value::from).unwrap_or(Value::Null),
-            Attribute::DONT_ENUM,
-        );
-
-        // TODO: `super` should only be defined if this was a method call (depth > 0?)
-        // `f[""]()` emits a CallMethod op, causing `this` to be undefined, but `super` is a function; what is it?
-        let super_object: Option<Object<'gc>> = this_obj.and_then(|this| {
-            if !af.flags.contains(FunctionFlags::SUPPRESS_SUPER) {
-                Some(SuperObject::new(activation, this, depth).into())
-            } else {
-                None
-            }
-        });
+        let arguments_caller = activation.callee;
 
         let name = if cfg!(feature = "avm_debug") {
             Cow::Owned(af.debug_string_for_call(name, args))
         } else {
             Cow::Borrowed("[Anonymous]")
         };
 
+        let is_this_inherited = af
+            .flags
+            .intersects(FunctionFlags::PRELOAD_THIS | FunctionFlags::SUPPRESS_THIS);
+        let local_this = if is_this_inherited {
+            activation.this_cell()
+        } else {
+            this
+        };
+
         let max_recursion_depth = activation.context.avm1.max_recursion_depth();
         let mut frame = Activation::from_action(
             activation.context.reborrow(),
@@ -402,17 +430,16 @@ impl<'gc> Executable<'gc> {
             child_scope,
             af.constant_pool,
             base_clip,
-            this,
+            local_this,
             Some(callee),
-            Some(arguments.into()),
         );
 
         frame.allocate_local_registers(af.register_count(), frame.context.gc_context);
 
         let mut preload_r = 1;
         af.load_this(&mut frame, this, &mut preload_r);
-        af.load_arguments(&mut frame, arguments.into(), &mut preload_r);
-        af.load_super(&mut frame, super_object, &mut preload_r);
+        af.load_arguments(&mut frame, args, arguments_caller, &mut preload_r);
+        af.load_super(&mut frame, this_obj, depth, &mut preload_r);
         af.load_root(&mut frame, &mut preload_r);
         af.load_parent(&mut frame, &mut preload_r);
         af.load_global(&mut frame, &mut preload_r);
diff --git a/tests/tests/regression_tests.rs b/tests/tests/regression_tests.rs
@@ -577,7 +577,7 @@ swf_tests! {
     (function_as_function, "avm1/function_as_function", 1),
     (function_base_clip_removed, "avm1/function_base_clip_removed", 3),
     (function_base_clip, "avm1/function_base_clip", 2),
-    #[ignore] (function_suppress_and_preload, "avm1/function_suppress_and_preload", 1),
+    (function_suppress_and_preload, "avm1/function_suppress_and_preload", 1),
     (funky_function_calls, "avm1/funky_function_calls", 1),
     (get_bytes_total, "avm1/get_bytes_total", 1),
     (getproperty_swf4, "avm1/getproperty_swf4", 1),
