Race condition in NativeReanimatedModule::performOperations()

[yard]

Description

Background

We have been debugging a weird issue where sometimes the app would incorrectly account for keyboard state. To spare you from all the steps on that journey, we were able to confirm that the styles computed by useAnimatedStyle hook will not apply (although debug logs confirmed they did re-compute and did that correctly). After carefully studying the issue I think we have found the root cause for that behavior.

Issue description

performOperations() method of NativeReanimatedModule can submit updated props to the registry while React is committing a new shadow tree. Specifically, it can happen so that ReanimatedCommitHook has already gathered the props from the registry and returned the updated tree to React, enabled shouldReanimatedSkipCommit_ flag and React is now reconciling the tree; at this very moment updates applied to props registry will effectively be lost: performOperations() would bail out (as the flag is set). Until next re-render happens, the UI would be presented with the old props.

Surely, this is a hard thing to reproduce – sorry for not providing a snack/repo, I am trying to think of a way of how to demonstrate that without too many external dependencies, but I hope that explanation is clear enough to at least confirm the issue is present and valid.

Proposed fix

We have applied a patch that adds another flag to props registry, which gets set whenever any prop update happens during mount phase. This way, ReanimatedMountHook can check if anything was changed during mount phase and trigger a re-render. I am not exactly sure if that's the best way to fix the problem, but it does seem to work and does so reliably and without causing excessive renders.

If someone could confirm this is a legit way of resolving the problem, I am happy to prepare a PR with the fix.

Steps to reproduce

N/A

Snack or a link to a repository

N/A

Reanimated version

3.8.1

React Native version

0.73.1

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native

Architecture

Fabric (New Architecture)

Build type

Debug app & dev bundle

Device

Real device

Device model

Any iOS device

Acknowledgements

Yes

[github-actions]

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

[github-actions]

Hey! 👋

It looks like you've omitted a few important sections from the issue template.

Please complete Description section.

[joe-sam]

By definition all race conditions are nearly impossible to reproduce! So kindly please make a PR fix and include a before and after video capturing/evading the glitch respectively.

[bartlomiejbloniarz]

Hi @yard! I am currently working on resolving some issues related to performOperations, so I will take this problem into considerations. Your solution seems good, I think you can keep it as a patch, but since there will be some changes to this logic soon I don't think that a PR is necessary, I will handle this problem.

[yard]

Hi @bartlomiejbloniarz ! Noted, do you want me to send you the diff or something or you are good for now?

[bartlomiejbloniarz]

Thank you @yard. The diff could be useful so if you have time it would be cool if you could send it!

[yard]

Sure thing, here you go:

react-native-reanimated-npm-3.8.1-d993815628.patch

The idea is pretty simple here: we added a flag propsUpdatedDuringMount_ which gets set whenever PropsRegistry receive an update with shouldReanimatedSkipCommit_ set and in ReanimatedMountHook we check if updates happenned during mount phase to re-apply the props. Could be tidied up etc, but does the job for us.