Skip to content
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

core: Fix "Failed to get Object" panics #18046

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Conversation

CUB3D
Copy link
Contributor

@CUB3D CUB3D commented Sep 23, 2024

This constitues the ruffle-side changes of ruffle-rs/rust-flash-lso#67 and ruffle-rs/rust-flash-lso#68.

This fixes the following (previously would panic on load):
#17902 - loads
#17709 - loads
#13748 - loads
#10520 - loads

This prevents panics on the following:
#17782 - (and possible dupe #17336 )
Now errors with "Property uploadFromBitmapData not found on flash.display3D.textures.RectangleTexture and there is no default value."

#14329
Fails to load past 54%, some URL load failures might be related

#10174
Same as 17782 above

These are not fixed, but one possible underlying issue is that a new AMF3Decoder is created for each call to ByteArray.readObject which could result in missing references that are replaced with Undefined when an element references a prior value in the stream. In the simple case, we could just cache the AMF3Decoder in the ByteArray object, but this won't handle more complex cases where the ByteArrays position is changed, so I'm leaving this for future work.
Update: I'm actually unsure if the above guess is true anymore, a missing change progressed the above and removed the invalid reference warnings but it still seems odd.

@evilpie evilpie added A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) labels Sep 23, 2024
@danielhjacobs danielhjacobs added the amf Issues relating to AMF serialization/deserialization label Sep 23, 2024
core/Cargo.toml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@torokati44
Copy link
Member

Since the act of merging ruffle-rs/rust-flash-lso#68 changed its HEAD hash (and there were some changes added to it even before the merge), I amended this branch accordingly. While at it, I also squashed a few "fixup" type commits into the original commits they fix, for cleaner commit history. I hope you don't mind, @CUB3D.

core/src/avm2/amf.rs Outdated Show resolved Hide resolved
Copy link
Member

@torokati44 torokati44 left a comment

Choose a reason for hiding this comment

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

Other than that one note, I vote on merging this soon. It looks fairly non-problematic.

Copy link
Contributor

@Dinnerbone Dinnerbone left a comment

Choose a reason for hiding this comment

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

LGTM (but mostly trusting you as the expert here :D)

Copy link
Collaborator

@Lord-McSweeney Lord-McSweeney left a comment

Choose a reason for hiding this comment

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

Approved except for above comment

@@ -114,6 +114,25 @@ impl<'gc> ArrayStorage<'gc> {
}
}

/// Replace the existing dense storage with a new dense storage
/// Does nothing if this `ArrayStorage` is sparse
Copy link
Member

Choose a reason for hiding this comment

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

This comment is now outdated due to the added panic!.
I'm just gonna fix this and merge.
Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

... also squashed this last "chore" commit into the first one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core player, where no other category fits amf Issues relating to AMF serialization/deserialization newsworthy T-fix Type: Bug fix (in something that's supposed to work already)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants