Skip to content

Commit

Permalink
Resolve sync UI updates concurency issue on iOS (software-mansion#4403)
Browse files Browse the repository at this point in the history
## 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 software-mansion#4239 because the lock
would guarantee that `performIOperation` is never called again before
the block scheduled on UIManagerQueue is finished. However in software-mansion#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.
  • Loading branch information
kmagiera authored and fluiddot committed Jun 5, 2023
1 parent 889e5cd commit 9d3240a
Showing 1 changed file with 67 additions and 34 deletions.
101 changes: 67 additions & 34 deletions ios/REANodesManager.mm
Original file line number Diff line number Diff line change
Expand Up @@ -89,19 +89,77 @@ - (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 {
CADisplayLink *_displayLink;
BOOL _wantRunUpdates;
NSMutableArray<REAOnAnimationCallback> *_onAnimationCallbacks;
BOOL _tryRunBatchUpdatesSynchronously;
REAEventHandler _eventHandler;
volatile void (^_mounting)(void);
NSObject *_syncLayoutUpdatesWaitLock;
volatile BOOL _syncLayoutUpdatesWaitTimedOut;
NSMutableDictionary<NSNumber *, ComponentUpdate *> *_componentUpdateBuffer;
NSMutableDictionary<NSNumber *, UIView *> *_viewRegistry;
#ifdef RCT_NEW_ARCH_ENABLED
Expand Down Expand Up @@ -129,7 +187,6 @@ - (nonnull instancetype)initWithModule:(REAModule *)reanimatedModule
_operationsInBatch = [NSMutableDictionary new];
_componentUpdateBuffer = [NSMutableDictionary new];
_viewRegistry = [_uiManager valueForKey:@"_viewRegistry"];
_syncLayoutUpdatesWaitLock = [NSObject new];
}

_displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(onAnimationFrame:)];
Expand Down Expand Up @@ -245,19 +302,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
{
#ifdef RCT_NEW_ARCH_ENABLED
Expand All @@ -272,8 +316,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) {
Expand All @@ -282,16 +325,16 @@ - (void)performOperations
BOOL canUpdateSynchronously = trySynchronously && ![strongSelf.uiManager hasEnqueuedUICommands];

if (!canUpdateSynchronously) {
dispatch_semaphore_signal(semaphore);
[syncUpdateObserver unblockUIThread];
}

for (int i = 0; i < copiedOperationsQueue.count; i++) {
copiedOperationsQueue[i](strongSelf.uiManager);
}

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.
Expand All @@ -302,17 +345,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;
Expand Down

0 comments on commit 9d3240a

Please sign in to comment.