fix(android): fix bug w/ LA where animation never executed when scheduled from JS

[hannojg]

Summary

To my shame, i believe there is no issue for this yet and i also didn't create a reproduction (yet) 🥶

The issue:

There is a bug with layout animations where they don't seem to fire. Effects of this are:

• Views staying at opacity: 0
• Views not running their layout animation, and staying "stuck" on the screen

I have the hunch that it could for example fix this issue (will test tmrw):

• [Android] Reanimated entering/exiting animations not working properly #8445

The issue started happening after upgrading from 3.17.4 to 3.19.3, which includes this change:

• [LA] Add check if view is mounted on Android  #8083

---

The cause:

The mentioned change was introduced to circumvent this bug:

• Occasional crash on Android when entering animation #7493 (comment)

I believe the change introduced a regression where a layout animation might not run at all:

• NativeProxy.preserveMountedTags would return false when we are not on the UI thread, which is the case if pullTransaction is called from the JS thread
• In that case NativeProxy::preserveMountedTags will return an empty optional
• LayoutAnimationsProxy::addOngoingAnimations will return as the vector is empty:

react-native-reanimated/packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy.cpp

Lines 404 to 406 in 794a669

	auto maybeCorrectedTags = preserveMountedTags_(tagsToUpdate);
	if (!maybeCorrectedTags.has_value()) {
	return;

---

The fix:

When you look into FabricUIManager.resolveView(tag) it has UIThreadUtils.assertOnUIThread(), which looks problematic.
However, if you check all the data structures that are being used, all of them are ConcurrentHashMaps, so I think resolveView is actually thread safe:

• https://github.com/facebook/react-native/blob/d93d32547bfadba731999ea251ca5e1d6c6c8912/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java#L1038
• https://github.com/facebook/react-native/blob/d93d32547bfadba731999ea251ca5e1d6c6c8912/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java#L81
• https://github.com/facebook/react-native/blob/d93d32547bfadba731999ea251ca5e1d6c6c8912/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.kt#L43

Thus i removed the check if we are on the UI thread, and hence addOngoingAnimation() will execute, even if invoked from the JS thread.

This will cause soft exceptions to be logged in debug

I will make a RFC in react-native to remove the UIThread check there since i think its not needed.
Alternatively I was thinking of two other ways:

• In FabricUIManager add a new thread safe getViewExists method (since in SurfaceMountingManager this method isn't marked as UI thread specific)
• Add a mechanism to ShadowViews to track in which revision they've been created. Then we can listen to the shadowTreeDidMount hook to capture the latest mounted revision and check if the tag/ shadowView we want to animate is equal or lower to that latest mounted revision. This way we would also avoid the JNI call.

Test plan

I tested all the LA tests from the fabric-example app.

Note

I might completely have missed a good reason why we only ever want to run that code on the UI thread. But as i checked, before we also were okay with executing it from the JS thread, we just wanted to avoid running it on not yet mounted views.

[hannojg]

I'll dare to ping @bartlomiejbloniarz as you introduced the change :p

[hannojg]

Oh, also a second consideration. In case we continue over a mutation:

react-native-reanimated/packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy.cpp

Lines 418 to 424 in 794a669

	if (correctedTags[i] == -1) {
	// skip views that have not been mounted yet
	// on Android we start entering animations from the JS thread
	// so it might happen, that the first frame of the animation goes through
	// before the view is first mounted
	// https://github.com/software-mansion/react-native-reanimated/issues/7493
	continue;

At the end of LayoutAnimationsProxy::addOngoingAnimations we will clear the update map:

react-native-reanimated/packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy.cpp

Line 444 in 794a669

updateMap.clear();

I was wondering: don't we need to keep the update for the view we were skipping?

[hannojg]

I just tested, and applying this change on top of 3.19.3 in the reproduction of this issue:

• [Android] Reanimated entering/exiting animations not working properly #8445

does not fix the issue.

However, it certainly fixed the issue in our clients app.

[hannojg]

RFC to remove the ui thread check in rn core:

• change(android): make FabricUIManager#resolveView not thread specific facebook/react-native#54259

[bartlomiejbloniarz]

Hi @hannojg 👋 Thanks for the PR, I think we could merge it as it is, but I don't fully understand how the issue happens. When the addOngoing method is called on the js thread, we don't clear the updateMap, so when it's called on the main thread it should apply the updates (and we call trigger pullTransaction from performOperations on each frame that had a layout animations update). We only clear it if the view is not mounted, but it will be repopulated on the next frame and if the animation ends before the view is ever mounted, then we have the restoreOpacity... method.

Did you see why the updates are not applied/triggered? Because maybe there is a flow that's worth fixing before we allow for this check on the JS thread. The reason I'm not sure about it, is that calling this check from the JS thread, doesn't guarantee us that the view is currently mounted, when the transaction is actually applied on the main thread. So I want to avoid changes here if I can.