avm1: Fix MovieClipReference laundering issue

core/src/avm1.rs

@@ -16,7 +16,7 @@ mod flv;
 mod fscommand;
 pub(crate) mod globals;
 mod object;
-mod object_reference;
+pub(crate) mod object_reference;
 mod parameters;
 mod property;
 mod property_map;

core/src/avm1/activation.rs

@@ -551,14 +551,6 @@ impl<'a, 'gc> Activation<'a, 'gc> {
 
     fn stack_push(&mut self, mut value: Value<'gc>) {
         if let Value::Object(obj) = value {
-            // Note that there currently exists a subtle issue with this logic:
-            // If the cached `Object` in a `MovieClipReference` becomes invalidated, causing it to switch back to path-based object resolution,
-            // it should *never* switch back to cache-based resolution
-            // However, currently if a `MovieClipReference` in this invalidated-cache state is converted back to an `Object`, such as when passed as an argument to a function,
-            // if it pushed back onto the stack then it will be converted into a new `MovieClipReference`, causing it to switch back to cache-based resolution
-            // Fixing this will require a thorough refactor of AVM1 to store `Either<MovieClipReference, Object>
-            // can refer to a MovieClip
-            // There is a ignored test for this issue of "reference laundering" at "avm1/string_paths_reference_launder"
             if let Some(mcr) = MovieClipReference::try_from_stage_object(self, obj) {
                 value = Value::MovieClip(mcr);
             }

core/src/avm1/object_reference.rs

@@ -1,4 +1,4 @@
-use super::{Activation, Object, object::ObjectWeak};
+use super::{Activation, NativeObject, Object, object::ObjectWeak};
 use crate::{
     display_object::{DisplayObject, TDisplayObject, TDisplayObjectContainer},
     string::{AvmString, WStr, WString},
@@ -71,13 +71,20 @@ impl<'gc> MovieClipReference<'gc> {
         activation: &mut Activation<'_, 'gc>,
         object: Object<'gc>,
     ) -> Option<Self> {
+        // If we are creating a reference to a clip which itself was formed by resolving a reference,
+        // then return that original reference.
+        if let NativeObject::MovieClip(mc) = object.native()
+            && let Some(original_ref) = mc.original_reference()
+        {
+            return Some(original_ref);
+        }
+
         // We can't use as_display_object here as we explicitly don't want to convert `SuperObjects`
         let display_object = object.as_display_object_no_super()?;
         let (path, cached) = if let DisplayObject::MovieClip(mc) = display_object {
             (mc.path(), display_object.object1()?)
         } else if activation.swf_version() <= 5 {
             let display_object = Self::process_swf5_references(activation, display_object)?;
-
             (display_object.path(), display_object.object1()?)
         } else {
             return None;
@@ -177,7 +184,12 @@ impl<'gc> MovieClipReference<'gc> {
 
     /// Convert this reference to an `Object`
     pub fn coerce_to_object(self, activation: &mut Activation<'_, 'gc>) -> Option<Object<'gc>> {
-        let (_, object, _) = self.resolve_reference(activation)?;
+        let (cached, object, display_object) = self.resolve_reference(activation)?;
+        // Track the original reference used to produce this clip, so we can get it back later.
+        // Only do this if the cache was invalid, as that's the state we need to preserve.
+        if !cached && let Some(mc) = display_object.as_movie_clip() {
+            mc.set_original_reference(activation.gc(), self);
+        }
         Some(object)
     }
 

core/src/display_object/movie_clip.rs

@@ -1,6 +1,7 @@
 //! `MovieClip` display object and support code.
 use crate::avm1::Avm1;
 use crate::avm1::globals::AVM_DEPTH_BIAS;
+use crate::avm1::object_reference::MovieClipReference;
 use crate::avm1::{Activation as Avm1Activation, ActivationIdentifier};
 use crate::avm1::{NativeObject as Avm1NativeObject, Object as Avm1Object};
 use crate::avm2::Activation as Avm2Activation;
@@ -175,6 +176,13 @@ pub struct MovieClipData<'gc> {
     object1: Lock<Option<Avm1Object<'gc>>>,
     object2: Lock<Option<Avm2StageObject<'gc>>>,
 
+    /// When a MovieClip is passed around in AVM1, it is usually passed as a `MovieClipReference`,
+    /// however we need to resolve this reference into a concrete `Object` in various places.
+    /// Because references hold observable state, we need to ensure that when we convert this clip
+    /// back into a Reference that we generate the *original* reference, rather than a fresh one.
+    /// This stores the original reference that produced this clip.
+    original_reference: Lock<Option<MovieClipReference<'gc>>>,
+
     drop_target: Lock<Option<DisplayObject<'gc>>>,
 
     /// A DisplayObject (doesn't need to be visible) to use for hit tests instead of this clip.
@@ -244,6 +252,7 @@ impl<'gc> MovieClipData<'gc> {
             audio_stream: Cell::new(None),
             object1: Lock::new(None),
             object2: Lock::new(None),
+            original_reference: Lock::new(None),
             clip_event_handlers: OnceCell::new(),
             clip_event_flags: Cell::new(ClipEventFlag::empty()),
             flags: Cell::new(MovieClipFlags::empty()),
@@ -2417,6 +2426,17 @@ impl<'gc> MovieClip<'gc> {
         let has_pending_script = self.has_frame_script(self.0.current_frame.get());
         self.0.has_pending_script.set(has_pending_script);
     }
+
+    /// Mark this `MovieClip` as being originated from resolving the given `MovieClipReference`.
+    pub fn set_original_reference(self, mc: &Mutation<'gc>, reference: MovieClipReference<'gc>) {
+        let write = Gc::write(mc, self.0);
+        unlock!(write, MovieClipData, original_reference).set(Some(reference));
+    }
+
+    /// Get the originating `MovieClipReference` of this `MovieClip`, if it exists.
+    pub fn original_reference(self) -> Option<MovieClipReference<'gc>> {
+        self.0.original_reference.get()
+    }
 }
 
 impl<'gc> TDisplayObject<'gc> for MovieClip<'gc> {

tests/tests/swfs/avm1/string_paths_reference_launder/test.toml

@@ -1,2 +1 @@
 num_frames = 1
-known_failure = true # See the comment in `stack_push` in avm1/activiation.rs for details