React-native-reanimated: Reanimated 2 causes uimanager commands to run out of order

[kmagiera]

Description

The issue has been identified on one of our partner's app and results in the crash that's been reported independently in #1318 (redbox with message "parentNode or node is a required parameter"). Despite that old issue being resolved I believe we haven't got to the bottom of the problem at that time, so I believe it has not been fixed and, as observed in other apps, it can still happen.

The crash can happen in various places and in particular in this line in Reanimated 1 code: https://github.com/software-mansion/react-native-reanimated/blob/master/ios/REANodesManager.m#L357

After investigating I found out that the crash is due to the native commands being run in a different order than the order at which they are dispatched in JS. I.e. we dispatch 1) createNode 2) connectToView 3) disconnectFromView 4) dropNode while sometimes we may run command no (3) first which results in the crash.

Further investigation shown that the culprit is the synchronous layout mechanism that, when reanimated 1 and 2 code is mixed, can turn on for random UI-manager batches. The relevant fragment that is problematic in particular is here: https://github.com/software-mansion/react-native-reanimated/blob/master/ios/REAModule.m#L174

- (void)uiManagerWillPerformMounting:(RCTUIManager *)uiManager {
  ...
  NSArray<AnimatedOperation> *operations = _operations;
  _operations = [NSMutableArray new];

  REANodesManager *nodesManager = _nodesManager;

  [uiManager addUIBlock:^(__unused RCTUIManager *manager, __unused NSDictionary<NSNumber *, UIView *> *viewRegistry) {
    for (AnimatedOperation operation in operations) {
      operation(nodesManager);
    }
    [nodesManager operationsBatchDidComplete];
  }];
}

It turns out that under certain circumstances the blocks that we enqueue with addUIBlock can run out of order.

The reason for that is that when operationsBatchDidComplete from the block is run, it may trigger uiManagerWillPerformMounting. The re-entry is likely undesirable and can happen because in operationsBatchDidComplete we call performOperations which in turn may call to [UIManager batchDidComplete] that the call layoutAndMount which triggers new willPerformMounting event.

However, that's not enough for the scheduled order mixup. On top of that, in NodesManager we intercept "mounting block" – this is the block that runs all the code enqueued with uimanager addUIBlock. As a result of intercepting it, we store in nodes manager while we wait for layout to finish. What can happen in the meantime is that another willPerformMounting event arrives and that in this case there are no reanimated 2 operations (that is, this if condition check fails) which makes the newly enqueued ui block to be processed sooner than the "mounting" block that we saved. This violates the order of commands that should be executed and in particular can result in "disconnect" command being run before "create" command.

As a consequence of the way we store mounting block and the issue we re-entry, what can happen is that we may override the mounting block which could result in some of the commands being completely skipped. What we observed in the logs kind of matches that other scenario confirming that the issue is both in the possibility of commands being run in a different order and also the possibility of some commands being completely discarded.

Steps To Reproduce

I don't have a repro case just yet but hope the detailed description can help in working on the fix. I haven't focused on working on a repro case just yet but would imagine it working as follows:
1) we should have an app that runs two sets of commands in separate batches
2) first set of command should consist of instantiating animated view with reanimated 1 and another with reanimated 2 (such that we get some reanimated 2 commands and we also expect some reanimated 1 propsnode being created and attached to view)
3) immediately after we unmount view using reanimated 1 causing "disconnect" command dispatch. At this point we should have no reanimated 1 commands to avoid blocking on layout

Expected behavior

All commands dispatched from JS should run in exact order in native. No commands should be skipped

Actual behavior

As described above some commands run out of order while others are discarded and never executed

Package versions

• React: 16.13.1
• React Native: 0.63.0
• React Native Reanimated: 2.0.0-rc.0
• NodeJS: 14.12.0

[github-actions[bot]]

Issue validator - update # 5

Hello!
Congratulations! Your issue passed the validator! Thank you!