Skip to content

avm2: Return undefined for construct_prop on primitives and namespaces #21417

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

avm2: Return undefined for construct_prop on primitives and namespaces #21417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions core/src/avm2/activation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1696,8 +1696,7 @@ impl<'a, 'gc> Activation<'a, 'gc> {
let multiname = multiname.fill_with_runtime_params(self)?;
let source = self.pop_stack().null_check(self, Some(&multiname))?;

let ctor = source.get_property(&multiname, self)?;
let constructed_object = ctor.construct(self, args)?;
let constructed_object = source.construct_property(&multiname, args, self)?;

self.push_stack(constructed_object);

Expand Down
26 changes: 26 additions & 0 deletions core/src/avm2/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,32 @@ impl<'gc> Value<'gc> {
)
}

/// Get a property and use it as a constructor
///
/// This method will panic if called on null or undefined.
pub fn construct_property(
&self,
multiname: &Multiname<'gc>,
arguments: FunctionArgs<'_, 'gc>,
activation: &mut Activation<'_, 'gc>,
) -> Result<Value<'gc>, Error<'gc>> {
let ctor = match self {
Value::Object(obj) if obj.as_namespace_object().is_none() => {
self.get_property(multiname, activation)?
}
_ => self
.get_property(multiname, activation)
.unwrap_or_else(|_| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still feels like a hacky solution to me, since

  1. The property not being found is not the only reason get_property can return an Err
  2. avmplus's version of construct_property throws different errors than its get_property (e.g. when run on a slot that isn't callable), so we'll have to change it to work that way when we implement it correctly anyways

Also, this shouldn't be trying to construct the proto; it should be trying to construct the result of a dynamic lookup of the multiname on the proto.

Could you make this match the way avmplus does it, or even just inline the vtable lookup logic from get_property here (with the special case for primitives)? That would solve the issues I mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

// This can only ever run for primitives and namespaces objects, so it should never panic
let proto = self.proto(activation).unwrap();

Value::from(proto)
}),
};

ctor.construct(activation, arguments)
}

/// Call a method by its index.
///
/// This directly corresponds with the AVM2 operation `callmethod`.
Expand Down
1 change: 0 additions & 1 deletion tests/tests/swfs/from_avmplus/regress/bug_638233/test.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
num_ticks = 1
known_failure = true
Loading