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

avm2: Don't hold GcCell read ref open for Loader.loadBytes #14790

Merged

Conversation

Aaron1011
Copy link
Member

We were holding this open while dispatching an event, leading to a panic if anything attempted to modify the ByteArray.

@Lord-McSweeney
Copy link
Collaborator

Can you add a test?

@Aaron1011 Aaron1011 force-pushed the aaron1011/loader-short-bytearray-read branch from 6ac086d to 07da619 Compare January 17, 2024 21:02
@Aaron1011
Copy link
Member Author

I updated an existing test to print out bytearray.position from event handlers.

@Lord-McSweeney
Copy link
Collaborator

Lord-McSweeney commented Jan 17, 2024

Are you sure that the test panics on current master? #14789 was a result of a mutable borrow (readUnsignedInt or whatever it was needs to update position) after the immutable borrow in the load_bytes, but the added test only calls bytearray::get_position, which doesn't mutably borrow the bytearray.

@torokati44
Copy link
Member

I had the same concerns, so I downloaded the updated test SWF, and started it in the desktop player and the web demo, got an error in both.

@Aaron1011
Copy link
Member Author

Yes, I confirmed that this errors on master. bytearray.position ends up mutating bytearray in order to attach the bound method.

We were holding this open while dispatching an event,
leading to a panic if anything attempted to modify the
ByteArray.
@Aaron1011 Aaron1011 force-pushed the aaron1011/loader-short-bytearray-read branch from 07da619 to ce73cb3 Compare January 17, 2024 21:34
@Aaron1011 Aaron1011 enabled auto-merge (rebase) January 17, 2024 21:34
@Aaron1011 Aaron1011 merged commit 97f868b into ruffle-rs:master Jan 17, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants