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

🐞 Fix Bad Access exception #275

Merged
merged 12 commits into from
Mar 23, 2022
Merged

🐞 Fix Bad Access exception #275

merged 12 commits into from
Mar 23, 2022

Conversation

chrfalch
Copy link
Contributor

This fixes BAD_ACCESS and SIG_ABORTS on iOS/Android in Skia views being accessed by the render thread after they have been deleted.

The PR Fixes adds a few guards to ensure that we're not accessing objects after destruction. A test with 100 Skia Views being constantly created/recreated has been added and can be used to verify that this bug is fixed.

Fixes #243

@chrfalch chrfalch requested a review from wcandillon March 18, 2022 20:48
@@ -0,0 +1,75 @@
import { Canvas, useImage, Image } from "@shopify/react-native-skia";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the perfect test case :))

@wcandillon
Copy link
Contributor

wcandillon commented Mar 19, 2022

The use case is perfect. In fact it reminded me why I couldn't reproduce the error the last time (I forgot that I was displaying images gotten from makeImageSnapshot).

This clearly fixes the issue on both iOS and Android but on iOS it looks like I could reproduce the error once. The screenshot below seems to confirm that this was the right version of the code running. Regardless this is still a dramatic improvement.
Screenshot 2022-03-19 at 08 53 30

@chrfalch
Copy link
Contributor Author

Nice find - I've pushed an update that fixes this - still seeing some crashes on Android that I want to figure out,

@wcandillon
Copy link
Contributor

wcandillon commented Mar 21, 2022

If we have a drawing that takes 50ms, animating will result in all frames being displayed slowly. Maybe we should skip the frame instead? In the context of gestures, not dropping frames seems to be quite a big issue.

If we have a looping animation that runs at 32ms and we let it run 5 seconds, starting a gesture will wait for all frames to be drawn before starting the gesture which could take many seconds.

@wcandillon
Copy link
Contributor

The drawing time in debug seems to be also out of sync, in the same manner, I'm wondering if this is related.

@ghost ghost added the cla-needed label Mar 23, 2022
@chrfalch
Copy link
Contributor Author

Added a new implementation of the OpenGL renderer for Android that looks very promising. No errors occurs when running the heavy examples, reload and hot refresh works as expected!

@ghost ghost removed the cla-needed label Mar 23, 2022
@chrfalch chrfalch changed the title Fix Bad Access exception 🐞 Fix Bad Access exception Mar 23, 2022
@chrfalch chrfalch force-pushed the bugfix/243-BAD_ACCESS branch from c52df9b to 1d2965a Compare March 23, 2022 14:36
@wcandillon
Copy link
Contributor

This is huge, congrats 🎉

@wcandillon wcandillon self-requested a review March 23, 2022 16:59
@wcandillon wcandillon merged commit 7b249a6 into main Mar 23, 2022
@rudin
Copy link

rudin commented Mar 24, 2022

Great to see this has been fixed! Can't wait for the new release. Thanks!

@wcandillon wcandillon deleted the bugfix/243-BAD_ACCESS branch March 28, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EXC_BAD_ACCESS when mounting Canvas
3 participants