fix: race condition with frame callbacks

[andreialecu]

Summary

Fixes #6299 #6158

Test plan

I haven't yet been able to reproduce the issue - we just have it in Sentry -, but I assume that this is a race condition where setActive(false) is called after the callback was removed, where perhaps the component was unmounted.

[andreialecu]

/cc @szydlovsky - not sure if #6143 is the cause here

[andreialecu]

Crash reports in Sentry. We bumped from reanimated 3.12.1 to 3.14 recently. Notice that one is for "read property startTime" and the other is for write, and this PR specifically targets those lines.

[AndreiCalazans]

And this is our usage of useFrameCallback - https://github.com/PedroBern/react-native-collapsible-tab-view/blob/main/src/Container.tsx#L224-L236 and https://github.com/PedroBern/react-native-collapsible-tab-view/blob/main/src/Container.tsx#L261

[szydlovsky]

@andreialecu @AndreiCalazans It is hard to tell what caused such issue. The cleanup was definitely faulty, as it might have tried to read a callbackId from already deallocated object - which in turn would not unregister callbacks. Thus, I memoized it in useEffect.
I suspect that removing ref.current.callbackId = -1 might have made some problems, it might be that the internal logic needed that Id to go -1 for some reason - hard to tell honestly.
Would you be able to run the code that sometimes logged these sentry errors, but changing the useEffect block in the hook to this:

useEffect(() => {
    ref.current.callbackId =
      frameCallbackRegistry.registerFrameCallback(callback);
    const memoizedFrameCallback = ref.current;
    ref.current.setActive(ref.current.isActive);

    return () => {
      frameCallbackRegistry.unregisterFrameCallback(memoizedFrameCallback.callbackId);
      memoizedFrameCallback.callbackId = -1;
    };
  }, [callback, autostart]);

I believe that the higher-level fix we apply, the better it is for overall reliability and potential regressions.

[andreialecu]

I also suspected that the -1 was necessary, because it is checked here:

https://github.com/andreialecu/react-native-reanimated/blob/2025c16b3a79ae61a5e77ece507d098df9f0cdd4/packages/react-native-reanimated/src/frameCallback/FrameCallbackRegistryUI.ts#L108

I haven't been able to reproduce this, but within just a few hours of releasing the update to Google Play only, there were a lot of users that received this crash, as per Sentry.

The only way to test that would be to publish another live build that will be potentially be crashing still.

I think this PR should be merged as is regardless, even if fixing a the problem somewhere else. The get(...)! assertions don't seem like a good idea.

[andreialecu]

An update here of affected user numbers. It's quite high, considering only a fraction have updated so far.

It appears this was also caught by the pre-launch report when releasing to Google Play:

Also, it likely affects every user of react-native-collapsible-tab-view v8 and reanimated 3.13+

[andreialecu]

Also relevant:

[szydlovsky]

Hmmmph, this is tough now. I can offer a following solution: a 3.14.1 patch release with only the callbackId = -1 fix, so that You can bump the Reanimated and see if pre-launch report indicates anything. If it still does, then I will make a 3.14.2 but with your asserts. What do you think about it?

[szydlovsky]

Cause we probably won't be able to determine whether it is fixed or not without having it either tested by pre-launch report or live users 😄

[andreialecu]

I'm running this PR with patch-package at the moment, which is guaranteed to get rid of the crash. I'm not sure if other side effects exist however, but they're better than crashing.

[szydlovsky]

Oh, this means we can check which potential fix actually fixes it. I'd be super grateful if You could check both options (I believe you can run pre-launch report without publishing the app, can't you?).
Here's the patch anyway: reanimated_frameCallback.patch 🙏

[szydlovsky]

Okay @andreialecu I found a consistent but not really useful way to get the error - you can get it by running a new RN app with this in App.tsx and change some code in Reanimated's node_modules code of useFrameCallback (even add a console.log and tap save to trigger a hot reload). However, I don't think it is any useful since it is a hot reload...

[szydlovsky]

Adding this PR's code to the node_modules the same way doesn't generate the error (probably because of asserts), but makes the app do some strange things and hard reload is needed. Still unsure what should we do

[andreialecu]

Does reverting #6143 improve it?

[szydlovsky]

Doesn't seem to...

[szydlovsky]

I still think that this is currently the only way we can somehow be sure the fix works

[szydlovsky]

@andreialecu sorry to ask you again, but isn't there really any way to run this pre-launch report with some applied changes? This way we could definitely tell if potential fixes do the job or not

[szydlovsky]

Other PR that aims to fix the issue in a better and cleaner manner got merged already, hence closing