Skip to content

avm1: Reference display objects indirectly by string path #9447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 32 commits into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
e9691e0
avm1: MovieClips are indirectly referenced by string paths
CUB3D Feb 7, 2023
ae10974
avm1: Treat MovieClips as strings in add2
CUB3D Feb 7, 2023
c2d804e
avm1: AS-instantiated clips with unload handlers need to have delayed…
CUB3D Feb 8, 2023
19df898
avm1: Don't convert SuperObjects to Value::MovieClip, or you break cl…
CUB3D Feb 8, 2023
e1cb64a
avm1: Cache MovieClips in Value::MovieClip, this is required to corre…
CUB3D Feb 9, 2023
4d0e4d7
avm1: Correctly invalidate cache on clip removal
CUB3D Feb 9, 2023
47c2a0a
avm1: Timers attached to clips don't fire when the clip is removed
CUB3D Feb 9, 2023
08d31f5
avm1: Fix add2 coerce
CUB3D Feb 10, 2023
ba0fed9
avm1: Locals on removed DisplayObjects can't be accessed, should be u…
CUB3D Feb 10, 2023
e878ef2
avm1: Temporary fix, block events when clips are removed
CUB3D Feb 10, 2023
880c69b
avm1: In swfv5, resolving a non MovieClip path, should resolve to the…
CUB3D Feb 10, 2023
19736c3
test: Add button v5/v6 tests
CUB3D Feb 10, 2023
241ec71
test: Add test for basic string paths
CUB3D Feb 10, 2023
8ab374f
test: Add test for string paths aliasing
CUB3D Feb 10, 2023
3ae45cd
test: Add test for unload and variable scopes
CUB3D Feb 11, 2023
3fc9a2b
test: Add test for clip constructors and timers
CUB3D Feb 11, 2023
1ceb72c
avm1: Fallback to resolving properties on _root when the are not in t…
CUB3D Feb 12, 2023
6d39d3b
avm1: Fixup clippy lints and formatting
CUB3D Feb 12, 2023
84aae54
avm1: Fix test failures
CUB3D Feb 12, 2023
e17b5d0
tests: Add string paths keypress event test, add support for keydown …
CUB3D Feb 12, 2023
2699eab
tests: Add more string paths tests
CUB3D Feb 12, 2023
1e37e8e
tests: Uniform test naming
CUB3D Feb 12, 2023
72048e5
avm1: Refactor movieclip path logic
CUB3D Feb 13, 2023
5dee16d
avm1: Remove some mc todos
CUB3D Feb 13, 2023
0c7724b
avm1: Cleanup todos, add even more tests
CUB3D Feb 13, 2023
aa622bc
avm1: Fix beta clippy
CUB3D Feb 13, 2023
56946c3
avm1: Dont double borrow the cache, update test output
CUB3D Feb 13, 2023
9fd26e7
avm1: Fix getVariable when path is an object+var and object is a clip
CUB3D Feb 14, 2023
fbfe454
avm1: More scope fixes
CUB3D Feb 14, 2023
eae80a0
test: Even more tests
CUB3D Feb 14, 2023
da21b5d
avm1: Remove resolved todos, add comment and test about known referen…
CUB3D Feb 21, 2023
2c0ba51
avm1: Don't use stack_push when the pushed value can't be an Object
CUB3D Feb 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/avm1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ mod error;
mod fscommand;
pub(crate) mod globals;
mod object;
mod object_reference;
mod property;
mod property_map;
mod runtime;
Expand Down
119 changes: 102 additions & 17 deletions core/src/avm1/activation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ use swf::avm1::types::*;
use swf::Twips;
use url::form_urlencoded;

use super::object_reference::MovieClipReference;

macro_rules! avm_debug {
($avm: expr, $($arg:tt)*) => (
if $avm.show_debug_output() {
Expand Down Expand Up @@ -537,6 +539,24 @@ impl<'a, 'gc> Activation<'a, 'gc> {
}
}

fn stack_push(&mut self, mut value: Value<'gc>) {
if let Value::Object(Object::StageObject(s)) = value {
Copy link
Member

@Aaron1011 Aaron1011 Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the ability to push to the AVM1 stack without this logic running? If not, could we move this logic into the push method, and have it take an Activation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really need to be able to push without this running, so moving it would be possible, but because we need a mutable borrow of activation inside of push to create a MovieClipReference as well as a mutable borrow of self.context.avm1 to call push it would likely be a much more invasive change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a pretty subtle issue, I'd like to make it harder to accidentally skip running this logic (by using avm1.push instead of stack_push). However, I don't want to block merging this PR, so I think this could be done as a follow-up refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding that the only solution I can think of, would be either removing avm1.push and switching everything to use activation.push or using interior mutability so that avm1.push can take &self over &mut self. Did you have another solution in mind?

Copy link
Collaborator

@adrian17 adrian17 Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make it harder to accidentally skip running this logic (by using avm1.push instead of stack_push).

On the contary, I'd consider minimizing complexity of push whenever it's not needed (say action_add). In avm2, stack push keeps popping up on profiles with non-trivial %, while on paper it should be one of the cheapest things ever.

// 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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't the only way, right? We could do it without changing storage, but refactoring the places where the to-from-object conversions are done.
Anyway, let's just make this comment more general I guess ;D

// 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, s) {
value = Value::MovieClip(mcr);
}
}

self.context.avm1.push(value);
}

fn action_add(&mut self) -> Result<FrameControl<'gc>, Error<'gc>> {
let a = self.context.avm1.pop();
let b = self.context.avm1.pop();
Expand All @@ -549,6 +569,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
// ECMA-262 s. 11.6.1
let a = self.context.avm1.pop().to_primitive(self)?;
let b = self.context.avm1.pop().to_primitive(self)?;

let result: Value<'_> = match (a, b) {
(Value::String(a), Value::String(b)) => {
AvmString::concat(self.context.gc_context, b, a).into()
Expand Down Expand Up @@ -729,7 +750,12 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let num_args = num_args.min(self.context.avm1.stack_len());
let mut args = Vec::with_capacity(num_args);
for _ in 0..num_args {
args.push(self.context.avm1.pop());
let arg = self.context.avm1.pop();
if let Value::MovieClip(_) = arg {
args.push(Value::Object(arg.coerce_to_object(self)));
} else {
args.push(arg);
}
}

let variable = self.get_variable(fn_name)?;
Expand All @@ -740,7 +766,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
self,
&args,
)?;
self.context.avm1.push(result);
self.stack_push(result);

// After any function call, execution of this frame stops if the base clip doesn't exist.
// For example, a _root.gotoAndStop moves the timeline to a frame where the clip was removed.
Expand All @@ -754,12 +780,17 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let num_args = num_args.min(self.context.avm1.stack_len());
let mut args = Vec::with_capacity(num_args);
for _ in 0..num_args {
args.push(self.context.avm1.pop());
let arg = self.context.avm1.pop();
if let Value::MovieClip(_) = arg {
args.push(Value::Object(arg.coerce_to_object(self)));
} else {
args.push(arg);
}
}

// Can not call method on undefined/null.
if matches!(object_val, Value::Undefined | Value::Null) {
self.context.avm1.push(Value::Undefined);
self.stack_push(Value::Undefined);
return Ok(FrameControl::Continue);
}

Expand All @@ -778,7 +809,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
// Call `this[method_name]`.
object.call_method(method_name, &args, self, ExecutionReason::FunctionCall)?
};
self.context.avm1.push(result);
self.stack_push(result);

self.continue_if_base_clip_exists()
}
Expand All @@ -790,6 +821,10 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let is_instance_of = if let Value::Object(obj) = obj {
let prototype = constr.get("prototype", self)?.coerce_to_object(self);
obj.is_instance_of(self, constr, prototype)?
} else if let Value::MovieClip(_) = obj {
let obj = obj.coerce_to_object(self);
let prototype = constr.get("prototype", self)?.coerce_to_object(self);
obj.is_instance_of(self, constr, prototype)?
} else {
false
};
Expand Down Expand Up @@ -904,6 +939,9 @@ impl<'a, 'gc> Activation<'a, 'gc> {

let success = if let Value::Object(object) = object {
object.delete(self, name)
} else if let Value::MovieClip(_) = object {
let object = object.coerce_to_object(self);
object.delete(self, name)
} else {
avm_warn!(self, "Cannot delete property {} from {:?}", name, object);
false
Expand Down Expand Up @@ -962,9 +1000,15 @@ impl<'a, 'gc> Activation<'a, 'gc> {
self.context.avm1.push(Value::Undefined); // Sentinel that indicates end of enumeration

match object {
Value::MovieClip(_) => {
let ob = object.coerce_to_object(self);
for k in ob.get_keys(self).into_iter().rev() {
self.stack_push(k.into());
}
}
Value::Object(ob) => {
for k in ob.get_keys(self).into_iter().rev() {
self.context.avm1.push(k.into());
self.stack_push(k.into());
}
}
_ => avm_error!(self, "Cannot enumerate properties of {}", name),
Expand All @@ -978,9 +1022,14 @@ impl<'a, 'gc> Activation<'a, 'gc> {

self.context.avm1.push(Value::Undefined); // Sentinel that indicates end of enumeration

if let Value::Object(object) = value {
if let Value::MovieClip(_) = value {
let object = value.coerce_to_object(self);
for k in object.get_keys(self).into_iter().rev() {
self.stack_push(k.into());
}
} else if let Value::Object(object) = value {
for k in object.get_keys(self).into_iter().rev() {
self.context.avm1.push(k.into());
self.stack_push(k.into());
}
} else {
avm_warn!(self, "Cannot enumerate {:?}", value);
Expand Down Expand Up @@ -1051,7 +1100,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let object = object_val.coerce_to_object(self);

let result = object.get(name, self)?;
self.context.avm1.push(result);
self.stack_push(result);

Ok(FrameControl::Continue)
}
Expand Down Expand Up @@ -1082,7 +1131,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
avm_warn!(self, "GetProperty: Invalid base clip");
Value::Undefined
};
self.context.avm1.push(result);
self.stack_push(result);
Ok(FrameControl::Continue)
}

Expand All @@ -1106,7 +1155,8 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let path = var_path.coerce_to_string(self)?;

let value: Value<'gc> = self.get_variable(path)?.into();
self.context.avm1.push(value);

self.stack_push(value);

Ok(FrameControl::Continue)
}
Expand Down Expand Up @@ -1194,6 +1244,9 @@ impl<'a, 'gc> Activation<'a, 'gc> {
} else if action.is_load_vars() || action.is_target_sprite() {
if let Value::Object(target) = target_val {
target.as_display_object()
} else if let Value::MovieClip(_) = target_val {
let tgt = target_val.coerce_to_object(self);
tgt.as_display_object()
} else {
let start = self.target_clip_or_root();
self.resolve_target_display_object(start, target_val, true)?
Expand All @@ -1213,6 +1266,11 @@ impl<'a, 'gc> Activation<'a, 'gc> {
is_load_vars = DisplayObject::ptr_eq(clip, self.base_clip().avm1_root());
}
}
if matches!(target_val, Value::MovieClip(_)) {
if let Some(clip) = clip_target {
is_load_vars = DisplayObject::ptr_eq(clip, self.base_clip().avm1_root());
}
}
}
if is_load_vars {
if let Some(clip_target) = clip_target {
Expand Down Expand Up @@ -1402,7 +1460,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
Value::Object(object.into())
};

self.context.avm1.push(result);
self.stack_push(result);
Ok(FrameControl::Continue)
}

Expand Down Expand Up @@ -1441,6 +1499,10 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let result = if let Value::Object(obj) = obj {
let prototype = constr.get("prototype", self)?.coerce_to_object(self);
obj.is_instance_of(self, constr, prototype)?
} else if let Value::MovieClip(_) = obj {
let obj = obj.coerce_to_object(self);
let prototype = constr.get("prototype", self)?.coerce_to_object(self);
obj.is_instance_of(self, constr, prototype)?
} else {
false
};
Expand Down Expand Up @@ -1601,7 +1663,12 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let num_args = num_args.min(self.context.avm1.stack_len());
let mut args = Vec::with_capacity(num_args);
for _ in 0..num_args {
args.push(self.context.avm1.pop());
let arg = self.context.avm1.pop();
if let Value::MovieClip(_) = arg {
args.push(Value::Object(arg.coerce_to_object(self)));
} else {
args.push(arg);
}
}

// Can not call method on undefined/null.
Expand Down Expand Up @@ -1636,7 +1703,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
}
};

self.context.avm1.push(result);
self.stack_push(result);

self.continue_if_base_clip_exists()
}
Expand All @@ -1648,13 +1715,18 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let num_args = num_args.min(self.context.avm1.stack_len());
let mut args = Vec::with_capacity(num_args);
for _ in 0..num_args {
args.push(self.context.avm1.pop());
let arg = self.context.avm1.pop();
if let Value::MovieClip(_) = arg {
args.push(Value::Object(arg.coerce_to_object(self)));
} else {
args.push(arg);
}
}

let name_value: Value<'gc> = self.resolve(fn_name)?.into();
let constructor = name_value.coerce_to_object(self);
let result = constructor.construct(self, &args)?;
self.context.avm1.push(result);
self.stack_push(result);

self.continue_if_base_clip_exists()
}
Expand Down Expand Up @@ -1728,7 +1800,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
}
}
};
self.context.avm1.push(value);
self.stack_push(value); // Needs to handle MovieClipReferences, in case pushing a register
}
Ok(FrameControl::Continue)
}
Expand Down Expand Up @@ -1854,6 +1926,17 @@ impl<'a, 'gc> Activation<'a, 'gc> {
return self.set_target(&target);
}
}
Value::MovieClip(_) => {
let o = target.coerce_to_object(self);
if let Some(clip) = o.as_display_object() {
// MovieClips can be targeted directly.
self.set_target_clip(Some(clip));
} else {
// Other objects get coerced to string.
let target = target.coerce_to_string(self)?;
return self.set_target(&target);
}
}
_ => {
let target = target.coerce_to_string(self)?;
return self.set_target(&target);
Expand Down Expand Up @@ -2480,6 +2563,8 @@ impl<'a, 'gc> Activation<'a, 'gc> {
// Resolve the value to an object while traversing the path.
object = if let Value::Object(o) = val {
o
} else if let Value::MovieClip(_) = val {
val.coerce_to_object(self)
} else {
return Ok(None);
};
Expand Down
2 changes: 1 addition & 1 deletion core/src/avm1/callable_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::avm1::{Object, TObject, Value};
use crate::string::AvmString;
use gc_arena::Collect;

#[derive(Clone, Collect)]
#[derive(Clone, Collect, Debug)]
#[collect(no_drop)]
pub enum CallableValue<'gc> {
UnCallable(Value<'gc>),
Expand Down
4 changes: 4 additions & 0 deletions core/src/avm1/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ impl<'a> VariableDumper<'a> {
Value::Object(object) => {
self.print_object(object, activation);
}
Value::MovieClip(_) => {
let obj = value.coerce_to_object(activation);
self.print_object(&obj, activation);
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions core/src/avm1/globals/as_broadcaster.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ pub fn broadcast_internal<'gc>(
activation,
ExecutionReason::Special,
)?;
} else if let Value::MovieClip(_) = listener {
let object = listener.coerce_to_object(activation);
object.call_method(method_name, call_args, activation, ExecutionReason::Special)?;
}
}

Expand Down
3 changes: 3 additions & 0 deletions core/src/avm1/globals/movie_clip_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ fn load_clip<'gc>(
Some(activation.resolve_level(*level_id as i32))
}
Value::Object(object) => object.as_display_object(),
Value::MovieClip(_) => target.coerce_to_object(activation).as_display_object(),
_ => None,
};
if let Some(target) = target {
Expand Down Expand Up @@ -96,6 +97,7 @@ fn unload_clip<'gc>(
activation.context.stage.child_by_depth(*level_id as i32)
}
Value::Object(object) => object.as_display_object(),
Value::MovieClip(_) => target.coerce_to_object(activation).as_display_object(),
_ => None,
};
if let Some(target) = target {
Expand Down Expand Up @@ -131,6 +133,7 @@ fn get_progress<'gc>(
Value::Object(object) if object.as_display_object().is_some() => {
object.as_display_object()
}
Value::MovieClip(_) => target.coerce_to_object(activation).as_display_object(),
_ => return Ok(Value::Undefined),
};
let result = ScriptObject::new(activation.context.gc_context, None);
Expand Down
14 changes: 14 additions & 0 deletions core/src/avm1/globals/selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,20 @@ pub fn set_focus<'gc>(
Ok(true.into())
}
}
Some(Value::MovieClip(_)) => {
let obj = args.get(0).unwrap().coerce_to_object(activation);

if let Some(display_object) = obj.as_display_object() {
if display_object.is_focusable() {
tracker.set(Some(display_object), &mut activation.context);
}
// [NA] Note: The documentation says true is success and false is failure,
// but from testing this seems to be opposite.
Ok(false.into())
} else {
Ok(true.into())
}
}
_ => Ok(false.into()),
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/avm1/globals/shared_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ fn serialize_value<'gc>(
elem: Value<'gc>,
) -> Option<AmfValue> {
match elem {
Value::Undefined => Some(AmfValue::Undefined),
Value::Undefined | Value::MovieClip(_) => Some(AmfValue::Undefined),
Value::Null => Some(AmfValue::Null),
Value::Bool(b) => Some(AmfValue::Bool(b)),
Value::Number(f) => Some(AmfValue::Number(f)),
Expand Down
Loading