Resolve sync UI updates concurency issue on iOS

[kmagiera]

Summary

This PR tries to address the problem with assert that we make in REANodesManager.mm when receiving mounting block from the React's UI Manager.

The issue was due to a fact that we only hold a single reference for mounting block as well as the timed-out flag, while under certain conditions, the performOperations may re-enter before these values are cleaned up correctly. This didn't happen before #4239 because the lock would guarantee that performIOperation is never called again before the block scheduled on UIManagerQueue is finished. However in #4239 we changed this behavior to stop potential ANR issues due to locking and this caused this new issue to surface.

The change we are making here is that we create a new instance of observer that is specific to a given performOperation call. This way every call to performOperation shares an instance of observer with UIManagerQueue bloc, which keeps ref to mounting block and timeout flag, hence it is never possible for read/writes of these refs to interfere between subsequent performOperation runs.

We are now also moving the assert to the new place – to the observer dealloc. We always expect the mounting block to be executed (and cleaned up) and hence if the observer is deallocated with the mounting block set, we treat this as an error.

Test plan

We couldn't manage to reproduce this issue but it was tested courtesy of @gavrix who could reliably reproduce the assert failure on one of the apps he works on.

[tomekzaw]

The name "unblockUIThread" is quite ambiguous in this context

Suggested change

	- (void)unblockUIThread
	- (void)unblockUIManagerThread

[kmagiera]

But we unblock UI thread here because it is waiting on the semaphore that we signal. The UIManager thread is not blocked at this point and it is the thread where this method can be run from

[tomekzaw]

ah yes, my bad, I was confused by RCTAssertUIManagerQueue() but it makes sense now

[tomekzaw]

Looks good to me, just wondering if we should to move the code to REASyncUpdateObserver.h and REASyncUpdateObserver.mm but this class is only used from REANodesManager.mm so that's okay.

[kmagiera]

This should perhaps be backported to 2.x branch, similarly to how we did with #4239 which introduced this problem

[LeviWilliams]

Sorry if I misunderstand this pr but shouldn't this solve #2285 ?

[raphaelinmanage]

Hey, to what version have you merged this pr?
3.1.0? or 2.17.0?

[LeviWilliams]

@raphaelinmanage this pr is for v3

[kaancembertas]

Hey, the issue is exists on v2.17.0 as well. Do you plan to fix it on v2? @LeviWilliams @kmagiera

[LeviWilliams]

@kaancembertas there is a comment right above stating that they intend to do this though I wouldn't hold your breath on backwards compat maintenance. I would recommend migrating to v3 anyways as quickly as possible, there are large gains in stability and performance in v3.

[pizzacoffeecode]

Still experiencing this in 3.1.0 when switching between bottom sheets using react-native-bottom-sheet

[happyfloat]

It's not released yet I think. I just applied the patch manually and it also fixes one of my issues I had. 👍

[fsegouin]

Hey, the issue is exists on v2.17.0 as well. Do you plan to fix it on v2? @LeviWilliams @kmagiera

I have backported the fix to v2 while also fixing a typescript issue, using a patch:

diff --git a/node_modules/react-native-reanimated/ios/REANodesManager.m b/node_modules/react-native-reanimated/ios/REANodesManager.m
index fda5bb2..c9648df 100644
--- a/node_modules/react-native-reanimated/ios/REANodesManager.m
+++ b/node_modules/react-native-reanimated/ios/REANodesManager.m
@@ -92,10 +92,71 @@ - (void)runSyncUIUpdatesWithObserver:(id<RCTUIManagerObserver>)observer
 
 @end
 
-@interface REANodesManager () <RCTUIManagerObserver>
+#ifndef RCT_NEW_ARCH_ENABLED
 
+@interface REASyncUpdateObserver : NSObject <RCTUIManagerObserver>
 @end
 
+@implementation REASyncUpdateObserver {
+  volatile void (^_mounting)(void);
+  volatile BOOL _waitTimedOut;
+  dispatch_semaphore_t _semaphore;
+}
+
+- (instancetype)init
+{
+  self = [super init];
+  if (self) {
+    _mounting = nil;
+    _waitTimedOut = NO;
+    _semaphore = dispatch_semaphore_create(0);
+  }
+  return self;
+}
+
+- (void)dealloc
+{
+  RCTAssert(_mounting == nil, @"Mouting block was set but never executed. This may lead to UI inconsistencies");
+}
+
+- (void)unblockUIThread
+{
+  RCTAssertUIManagerQueue();
+  dispatch_semaphore_signal(_semaphore);
+}
+
+- (void)waitAndMountWithTimeout:(NSTimeInterval)timeout
+{
+  RCTAssertMainQueue();
+  long result = dispatch_semaphore_wait(_semaphore, dispatch_time(DISPATCH_TIME_NOW, timeout * NSEC_PER_SEC));
+  if (result != 0) {
+    @synchronized(self) {
+      _waitTimedOut = YES;
+    }
+  }
+  if (_mounting) {
+    _mounting();
+    _mounting = nil;
+  }
+}
+
+- (BOOL)uiManager:(RCTUIManager *)manager performMountingWithBlock:(RCTUIManagerMountingBlock)block
+{
+  RCTAssertUIManagerQueue();
+  @synchronized(self) {
+    if (_waitTimedOut) {
+      return NO;
+    } else {
+      _mounting = block;
+      return YES;
+    }
+  }
+}
+
+@end
+
+#endif
+
 @implementation REANodesManager {
   NSMutableDictionary<REANodeID, REANode *> *_nodes;
   NSMapTable<NSString *, REANode *> *_eventMapping;
@@ -108,9 +169,6 @@ @implementation REANodesManager {
   NSMutableArray<REANativeAnimationOp> *_operationsInBatch;
   BOOL _tryRunBatchUpdatesSynchronously;
   REAEventHandler _eventHandler;
-  volatile void (^_mounting)(void);
-  NSObject *_syncLayoutUpdatesWaitLock;
-  volatile BOOL _syncLayoutUpdatesWaitTimedOut;
   NSMutableDictionary<NSNumber *, ComponentUpdate *> *_componentUpdateBuffer;
   volatile atomic_bool _shouldFlushUpdateBuffer;
   NSMutableDictionary<NSNumber *, UIView *> *_viewRegistry;
@@ -130,7 +188,6 @@ - (instancetype)initWithModule:(REAModule *)reanimatedModule uiManager:(RCTUIMan
     _operationsInBatch = [NSMutableArray new];
     _componentUpdateBuffer = [NSMutableDictionary new];
     _viewRegistry = [_uiManager valueForKey:@"_viewRegistry"];
-    _syncLayoutUpdatesWaitLock = [NSObject new];
     _shouldFlushUpdateBuffer = false;
   }
 
@@ -232,19 +289,6 @@ - (void)onAnimationFrame:(CADisplayLink *)displayLink
   }
 }
 
-- (BOOL)uiManager:(RCTUIManager *)manager performMountingWithBlock:(RCTUIManagerMountingBlock)block
-{
-  RCTAssert(_mounting == nil, @"Mouting block is expected to not be set");
-  @synchronized(_syncLayoutUpdatesWaitLock) {
-    if (_syncLayoutUpdatesWaitTimedOut) {
-      return NO;
-    } else {
-      _mounting = block;
-      return YES;
-    }
-  }
-}
-
 - (void)performOperations
 {
   if (_wantRunUpdates) {
@@ -258,8 +302,7 @@ - (void)performOperations
     _tryRunBatchUpdatesSynchronously = NO;
 
     __weak typeof(self) weakSelf = self;
-    dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
-    _syncLayoutUpdatesWaitTimedOut = NO;
+    REASyncUpdateObserver *syncUpdateObserver = [REASyncUpdateObserver new];
     RCTExecuteOnUIManagerQueue(^{
       __typeof__(self) strongSelf = weakSelf;
       if (strongSelf == nil) {
@@ -268,7 +311,7 @@ - (void)performOperations
       BOOL canUpdateSynchronously = trySynchronously && ![strongSelf.uiManager hasEnqueuedUICommands];
 
       if (!canUpdateSynchronously) {
-        dispatch_semaphore_signal(semaphore);
+        [syncUpdateObserver unblockUIThread];
       }
 
       for (int i = 0; i < copiedOperationsQueue.count; i++) {
@@ -276,8 +319,8 @@ - (void)performOperations
       }
 
       if (canUpdateSynchronously) {
-        [strongSelf.uiManager runSyncUIUpdatesWithObserver:strongSelf];
-        dispatch_semaphore_signal(semaphore);
+        [strongSelf.uiManager runSyncUIUpdatesWithObserver:syncUpdateObserver];
+        [syncUpdateObserver unblockUIThread];
       }
       // In case canUpdateSynchronously=true we still have to send uiManagerWillPerformMounting event
       // to observers because some components (e.g. TextInput) update their UIViews only on that event.
@@ -288,17 +331,7 @@ - (void)performOperations
       // from CADisplayLink but it is easier to hardcode it for the time being.
       // The reason why we use frame duration here is that if takes longer than one frame to complete layout tasks
       // there is no point of synchronizing layout with the UI interaction as we get that one frame delay anyways.
-      long result = dispatch_semaphore_wait(semaphore, dispatch_time(DISPATCH_TIME_NOW, 16 * NSEC_PER_MSEC));
-      if (result != 0) {
-        @synchronized(_syncLayoutUpdatesWaitLock) {
-          _syncLayoutUpdatesWaitTimedOut = YES;
-        }
-      }
-    }
-
-    if (_mounting) {
-      _mounting();
-      _mounting = nil;
+      [syncUpdateObserver waitAndMountWithTimeout:0.016];
     }
   }
   _wantRunUpdates = NO;
diff --git a/node_modules/react-native-reanimated/react-native-reanimated.d.ts b/node_modules/react-native-reanimated/react-native-reanimated.d.ts
index c33d830..9bbf091 100644
--- a/node_modules/react-native-reanimated/react-native-reanimated.d.ts
+++ b/node_modules/react-native-reanimated/react-native-reanimated.d.ts
@@ -128,7 +128,7 @@ declare module 'react-native-reanimated' {
 
     export type TransformStyleTypes = TransformsStyle['transform'] extends
       | readonly (infer T)[]
-      | undefined
+      | undefined | string
       ? T
       : never;
     export type AdaptTransforms<T> = {
@@ -138,7 +138,7 @@ declare module 'react-native-reanimated' {
 
     export type AnimateStyle<S> = {
       [K in keyof S]: K extends 'transform'
-        ? AnimatedTransform
+        ? AnimatedTransform | string
         : S[K] extends ReadonlyArray<any>
         ? ReadonlyArray<AnimateStyle<S[K][0]>>
         : S[K] extends object
diff --git a/node_modules/react-native-reanimated/src/reanimated2/platform-specific/checkVersion.ts b/node_modules/react-native-reanimated/src/reanimated2/platform-specific/checkVersion.ts
index 1b64bb6..c3db771 100644
--- a/node_modules/react-native-reanimated/src/reanimated2/platform-specific/checkVersion.ts
+++ b/node_modules/react-native-reanimated/src/reanimated2/platform-specific/checkVersion.ts
@@ -4,6 +4,7 @@ import { version as jsVersion } from '../../../package.json';
  * Checks that native and js versions of reanimated match.
  */
 export function checkVersion(): void {
+  // @ts-ignore
   const cppVersion = global._REANIMATED_VERSION_CPP;
   if (cppVersion === undefined) {
     console.error(

I was hoping they would bring it to v2 but it looks like they've already abandoned it. Migrating to v3 isn't an option on the project I'm working on currently unfortunately, hence the patch.