Skip to content

Commit 182dc37

Browse files
authored
Resolve sync UI updates concurency issue on iOS (software-mansion#4403)
## 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.
1 parent 4a2838b commit 182dc37

1 file changed

Lines changed: 67 additions & 34 deletions

File tree

ios/REANodesManager.mm

Lines changed: 67 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -89,19 +89,77 @@ - (void)runSyncUIUpdatesWithObserver:(id<RCTUIManagerObserver>)observer
8989

9090
@end
9191

92-
@interface REANodesManager () <RCTUIManagerObserver>
92+
#ifndef RCT_NEW_ARCH_ENABLED
9393

94+
@interface REASyncUpdateObserver : NSObject <RCTUIManagerObserver>
9495
@end
9596

97+
@implementation REASyncUpdateObserver {
98+
volatile void (^_mounting)(void);
99+
volatile BOOL _waitTimedOut;
100+
dispatch_semaphore_t _semaphore;
101+
}
102+
103+
- (instancetype)init
104+
{
105+
self = [super init];
106+
if (self) {
107+
_mounting = nil;
108+
_waitTimedOut = NO;
109+
_semaphore = dispatch_semaphore_create(0);
110+
}
111+
return self;
112+
}
113+
114+
- (void)dealloc
115+
{
116+
RCTAssert(_mounting == nil, @"Mouting block was set but never executed. This may lead to UI inconsistencies");
117+
}
118+
119+
- (void)unblockUIThread
120+
{
121+
RCTAssertUIManagerQueue();
122+
dispatch_semaphore_signal(_semaphore);
123+
}
124+
125+
- (void)waitAndMountWithTimeout:(NSTimeInterval)timeout
126+
{
127+
RCTAssertMainQueue();
128+
long result = dispatch_semaphore_wait(_semaphore, dispatch_time(DISPATCH_TIME_NOW, timeout * NSEC_PER_SEC));
129+
if (result != 0) {
130+
@synchronized(self) {
131+
_waitTimedOut = YES;
132+
}
133+
}
134+
if (_mounting) {
135+
_mounting();
136+
_mounting = nil;
137+
}
138+
}
139+
140+
- (BOOL)uiManager:(RCTUIManager *)manager performMountingWithBlock:(RCTUIManagerMountingBlock)block
141+
{
142+
RCTAssertUIManagerQueue();
143+
@synchronized(self) {
144+
if (_waitTimedOut) {
145+
return NO;
146+
} else {
147+
_mounting = block;
148+
return YES;
149+
}
150+
}
151+
}
152+
153+
@end
154+
155+
#endif
156+
96157
@implementation REANodesManager {
97158
CADisplayLink *_displayLink;
98159
BOOL _wantRunUpdates;
99160
NSMutableArray<REAOnAnimationCallback> *_onAnimationCallbacks;
100161
BOOL _tryRunBatchUpdatesSynchronously;
101162
REAEventHandler _eventHandler;
102-
volatile void (^_mounting)(void);
103-
NSObject *_syncLayoutUpdatesWaitLock;
104-
volatile BOOL _syncLayoutUpdatesWaitTimedOut;
105163
NSMutableDictionary<NSNumber *, ComponentUpdate *> *_componentUpdateBuffer;
106164
NSMutableDictionary<NSNumber *, UIView *> *_viewRegistry;
107165
#ifdef RCT_NEW_ARCH_ENABLED
@@ -129,7 +187,6 @@ - (nonnull instancetype)initWithModule:(REAModule *)reanimatedModule
129187
_operationsInBatch = [NSMutableDictionary new];
130188
_componentUpdateBuffer = [NSMutableDictionary new];
131189
_viewRegistry = [_uiManager valueForKey:@"_viewRegistry"];
132-
_syncLayoutUpdatesWaitLock = [NSObject new];
133190
}
134191

135192
_displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(onAnimationFrame:)];
@@ -245,19 +302,6 @@ - (void)onAnimationFrame:(CADisplayLink *)displayLink
245302
}
246303
}
247304

248-
- (BOOL)uiManager:(RCTUIManager *)manager performMountingWithBlock:(RCTUIManagerMountingBlock)block
249-
{
250-
RCTAssert(_mounting == nil, @"Mouting block is expected to not be set");
251-
@synchronized(_syncLayoutUpdatesWaitLock) {
252-
if (_syncLayoutUpdatesWaitTimedOut) {
253-
return NO;
254-
} else {
255-
_mounting = block;
256-
return YES;
257-
}
258-
}
259-
}
260-
261305
- (void)performOperations
262306
{
263307
#ifdef RCT_NEW_ARCH_ENABLED
@@ -272,8 +316,7 @@ - (void)performOperations
272316
_tryRunBatchUpdatesSynchronously = NO;
273317

274318
__weak __typeof__(self) weakSelf = self;
275-
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
276-
_syncLayoutUpdatesWaitTimedOut = NO;
319+
REASyncUpdateObserver *syncUpdateObserver = [REASyncUpdateObserver new];
277320
RCTExecuteOnUIManagerQueue(^{
278321
__typeof__(self) strongSelf = weakSelf;
279322
if (strongSelf == nil) {
@@ -282,16 +325,16 @@ - (void)performOperations
282325
BOOL canUpdateSynchronously = trySynchronously && ![strongSelf.uiManager hasEnqueuedUICommands];
283326

284327
if (!canUpdateSynchronously) {
285-
dispatch_semaphore_signal(semaphore);
328+
[syncUpdateObserver unblockUIThread];
286329
}
287330

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

292335
if (canUpdateSynchronously) {
293-
[strongSelf.uiManager runSyncUIUpdatesWithObserver:strongSelf];
294-
dispatch_semaphore_signal(semaphore);
336+
[strongSelf.uiManager runSyncUIUpdatesWithObserver:syncUpdateObserver];
337+
[syncUpdateObserver unblockUIThread];
295338
}
296339
// In case canUpdateSynchronously=true we still have to send uiManagerWillPerformMounting event
297340
// to observers because some components (e.g. TextInput) update their UIViews only on that event.
@@ -302,17 +345,7 @@ - (void)performOperations
302345
// from CADisplayLink but it is easier to hardcode it for the time being.
303346
// The reason why we use frame duration here is that if takes longer than one frame to complete layout tasks
304347
// there is no point of synchronizing layout with the UI interaction as we get that one frame delay anyways.
305-
long result = dispatch_semaphore_wait(semaphore, dispatch_time(DISPATCH_TIME_NOW, 16 * NSEC_PER_MSEC));
306-
if (result != 0) {
307-
@synchronized(_syncLayoutUpdatesWaitLock) {
308-
_syncLayoutUpdatesWaitTimedOut = YES;
309-
}
310-
}
311-
}
312-
313-
if (_mounting) {
314-
_mounting();
315-
_mounting = nil;
348+
[syncUpdateObserver waitAndMountWithTimeout:0.016];
316349
}
317350
}
318351
_wantRunUpdates = NO;

0 commit comments

Comments
 (0)