dohooo / react-native-reanimated-carousel

🎠 React Native swiper/carousel component, fully implemented using reanimated v2, support to iOS/Android/Web. (Swiper/Carousel)
https://react-native-reanimated-carousel.vercel.app
MIT License
2.85k stars 329 forks source link

fix: typeof "function" onProgressChange - fixes app crash #648

Closed nmassey closed 2 months ago

nmassey commented 4 months ago

What: the bug

On iOS, when running in "Release" configuration (i.e. not "development" configuration), if onProgressChange is a function, the app crashes. 😖

On Android, similar crash. Thanks to @yannick-softwerft for providing a traceback in a comment below.

This bug was accidentally introduced in 0d2b930f394f65fd70a03593ea8c7b16fb552e62 (in code here)

Why

Since the callback functions in useAnimatedReaction are automatically workletized, when running in Xcode "Release" configuration (i.e. not "development" configuration), if the variable onProgressChange is set to a function (from within the JS thread), then typeof onProgressChange will be equal to "object" from within the workletized function (in the UI thread).

So, the code assumes that the value is a SharedValue and attempts to set onProgressChange.value = absoluteProgress. However, this seems to immediately cause the app to crash.

My configuration

  1. expo: 50.0.19
  2. react-native-reanimated: 3.6.2
  3. react-native-reanimated-carousel: 4.0.0-alpha.12

What: the fix

Remember the value for typeof onProgressChange in the JS thread (instead trying to check it from within the UI thread).

App no longer crashes! And onProgressChange as function works correctly again!! 🥳🙌

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 0abdb2d51fba1d309a065ed614713d792b6e70a5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | -------------------------------- | ----- | | react-native-reanimated-carousel | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-reanimated-carousel ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 22, 2024 2:10am
yannick-softwerft commented 3 months ago

The error Message I got caused by this issue:

07-29 17:12:47.816  8490  8490 E AndroidRuntime: com.facebook.jni.CppException: TypeError: Cannot assign to property 'value' on HostObject with default setter
07-29 17:12:47.816  8490  8490 E AndroidRuntime: 
07-29 17:12:47.816  8490  8490 E AndroidRuntime: Error: TypeError: Cannot assign to property 'value' on HostObject with default setter
07-29 17:12:47.816  8490  8490 E AndroidRuntime:     at anonymous (JavaScript:1:597)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:     at anonymous (JavaScript:1:95)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:     at mapperRun (JavaScript:1:1044)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:     at anonymous (JavaScript:1:350)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:     at callMicrotasksOnUIThread (JavaScript:1:61)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:     at anonymous (JavaScript:1:139)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at com.swmansion.reanimated.AndroidUIScheduler.triggerUI(Native Method)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at com.swmansion.reanimated.AndroidUIScheduler$1.run(AndroidUIScheduler.java:24)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at com.swmansion.reanimated.AndroidUIScheduler$2.runGuarded(AndroidUIScheduler.java:43)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at com.facebook.react.bridge.GuardedRunnable.run(GuardedRunnable.java:29)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at android.os.Handler.handleCallback(Handler.java:958)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at android.os.Handler.dispatchMessage(Handler.java:99)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at android.os.Looper.loopOnce(Looper.java:205)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at android.os.Looper.loop(Looper.java:294)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at android.app.ActivityThread.main(ActivityThread.java:8177)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at java.lang.reflect.Method.invoke(Native Method)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
07-29 17:12:47.816  8490  8490 E AndroidRuntime:    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971)

Based on this it took me some time to find this PR. Thx so much for providing a patch.

quememo commented 3 months ago

would love to see this merged soon

JanDoeTian commented 3 months ago

Thanks for fixing, hope this is merged soon!

jvgeee commented 3 months ago

@dohooo / @oliverloops Can you merge this in please?

qwertychouskie commented 3 months ago

@dohooo Is there anything holding up this PR from being merged?

sebak94 commented 2 months ago

We need this one 🙏 Thanks!

qwertychouskie commented 2 months ago

@dohooo Thanks for merging! Is there any approximate timeline on merging the other bugfix PRs that are waiting and publishing a new release?