Initialize CADisplayLink on UI thread #4694

Latropos · 2023-07-10T12:11:13Z

Summary

Some users have observed following thread lock:

Error log

Thread 0 - com.apple.main-thread - (TH_STATE_WAITING)
0  libsystem_kernel.dylib +0x1c88  ___psynch_mutexwait
1  libsystem_pthread.dylib +0x2110 __pthread_mutex_firstfit_lock_wait
2  libsystem_pthread.dylib +0x9314 __pthread_mutex_firstfit_lock_slow
3  QuartzCore +0xa07cc             CA::Display::DisplayLinkItem::set_preferred_fps_range(CAFrameRateRange, bool, bool)
4  QuartzCore +0x3a918             CA::Display::DisplayLink::update_timer_locked(bool)
5  QuartzCore +0x3a504             CA::Display::DisplayLink::update_timer(bool)
6  QuartzCore +0xbf40c             CA::Display::DisplayLink::callback(_CADisplayTimer*, unsigned long long, unsigned long long, unsigned long long, bool, void*)
7  QuartzCore +0x39410             display_timer_callback(__CFMachPort*, void*, long, void*)
8  CoreFoundation +0x79d50         ___CFMachPortPerform
9  CoreFoundation +0x97144         ___CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__
10 CoreFoundation +0x98e34         ___CFRunLoopDoSource1
11 CoreFoundation +0x7a654         ___CFRunLoopRun
12 CoreFoundation +0x7f3e8         _CFRunLoopRunSpecific
13 GraphicsServices +0x1358        _GSEventRunModal
14 UIKitCore +0x39d6e4             -[UIApplication _run]
15 UIKitCore +0x39d348             _UIApplicationMain
16 MyApp +0x720c                 main (AppDelegate.swift:24:7)
17 dyld +0x15de8                   start

Thread 13 - com.facebook.react.JavaScript - (TH_STATE_WAITING)
0  libsystem_kernel.dylib +0x17bc  ___ulock_wait
1  libsystem_platform.dylib +0xfac __os_unfair_lock_lock_slow
2  QuartzCore +0xd09ec             CA::Display::DisplayLinkItem::update_link(__CFRunLoop*)
3  MyAppReactNative +0x205698    -[REANodesManager initWithModule:uiManager:] (REANodesManager.mm:214:3)
4  MyAppReactNative +0x200e30    -[REAModule setBridge:] (REAModule.mm:202:19)
5  Foundation +0x69788             -[NSObject(NSKeyValueCoding) setValue:forKey:]
6  MyAppReactNative +0x2c6aec    -[RCTModuleData setBridgeForInstance] (RCTModuleData.mm:267:7)
7  MyAppReactNative +0x2c6964    -[RCTModuleData setUpInstanceAndBridge:] (RCTModuleData.mm:221:7)
8  MyAppReactNative +0x2c73dc    -[RCTModuleData instance] (RCTModuleData.mm:406:7)
9  MyAppReactNative +0x2cc9f0    facebook::react::invokeInner(RCTBridge*, RCTModuleData*, unsigned int, folly::dynamic const&, int, (anonymous namespace)::SchedulingContext) (RCTNativeModule.mm:196:67)
10 MyAppReactNative +0x2cc820    facebook::react::RCTNativeModule::callSerializableNativeHook(unsigned int, folly::dynamic&&) (RCTNativeModule.mm:143:10)
11 MyAppReactNative +0x3cb198    facebook::react::JSIExecutor::nativeCallSyncHook(facebook::jsi::Value const*, unsigned long) (JSIExecutor.cpp:493:40)
12 MyAppReactNative +0x5e37f0    std::__1::function<facebook::jsi::Value (facebook::jsi::Runtime&, facebook::jsi::Value const&, facebook::jsi::Value const*, unsigned long)>::operator()(facebook::jsi::Runtime&, facebook::jsi::Value const&, facebook::jsi::Value const*, unsigned long) const (function.h:512:16)
13 hermes +0x131d8                 0x131d8 (0x130b0 + 296)
14 hermes +0x2702c                 0x2702c (0x26f94 + 152)
15 hermes +0x4625c                 0x4625c (0x45764 + 2808)
16 hermes +0x45718                 0x45718 (0x45698 + 128)
17 hermes +0x8a640                 0x8a640 (0x89f90 + 1712)
18 hermes +0x658c                  0x658c (0x64ac + 224)
19 hermes +0x63d4                  facebook::hermes::HermesRuntime::evaluateJavaScriptWithSourceMap(std::__1::shared_ptr<facebook::jsi::Buffer const> const&, std::__1::shared_ptr<facebook::jsi::Buffer const> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)
20 hermes +0x76f4                  0x76f4 (0x76dc + 24)
21 MyAppReactNative +0x38e19c    facebook::jsi::WithRuntimeDecorator<facebook::react::(anonymous namespace)::ReentrancyCheck, facebook::jsi::Runtime, facebook::jsi::Runtime>::evaluateJavaScript(std::__1::shared_ptr<facebook::jsi::Buffer const> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) (decorator.h:118:20)
22 MyAppReactNative +0x3c9704    facebook::react::JSIExecutor::loadBundle(std::__1::unique_ptr<facebook::react::JSBigString const, std::__1::default_delete<facebook::react::JSBigString const> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >) (JSIExecutor.cpp:157:13)
23 MyAppReactNative +0x35f59c    facebook::react::NativeToJsBridge::loadBundle(std::__1::unique_ptr<facebook::react::RAMBundleRegistry, std::__1::default_delete<facebook::react::RAMBundleRegistry> >, std::__1::unique_ptr<facebook::react::JSBigString const, std::__1::default_delete<facebook::react::JSBigString const> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >)::$_1::operator()(facebook::react::JSExecutor*) (NativeToJsBridge.cpp:146:21)
24 MyAppReactNative +0x3606ac    std::__1::__function::__func<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>)::$_8, std::__1::allocator<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>)::$_8>, void ()>::operator()() (function.h:512:16)
25 MyAppReactNative +0x2b6950    facebook::react::tryAndReturnError(std::__1::function<void ()> const&) (function.h:512:16)
26 MyAppReactNative +0x2c33e0    facebook::react::RCTMessageThread::tryFunc(std::__1::function<void ()> const&) (RCTMessageThread.mm:69:20)
27 MyAppReactNative +0x2c31f4    ___ZN8facebook5react16RCTMessageThread8runAsyncENSt3__18functionIFvvEEE_block_invoke (function.h:512:16)
28 CoreFoundation +0x436dc         ___CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__
29 CoreFoundation +0xaa20c         ___CFRunLoopDoBlocks
30 CoreFoundation +0x7a714         ___CFRunLoopRun
31 CoreFoundation +0x7f3e8         _CFRunLoopRunSpecific
32 MyAppReactNative +0x2ac2cc    +[RCTCxxBridge runRunLoop] (RCTCxxBridge.mm:336:12)
33 Foundation +0x5b540             ___NSThread__start__
34 libsystem_pthread.dylib +0x16b4 __pthread_start

This PR moves initialisation of CADisplayLink into UI thread, which may solve the problem

Test plan

piaskowyk

I think we also need to do the same think with displayLink from KeyboardEventObserver

ios/REANodesManager.mm

Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com>

ios/keyboardObserver/REAKeyboardEventObserver.m

Caiopia · 2023-07-10T22:15:53Z

ios/REANodesManager.mm

+    _displayLink.preferredFramesPerSecond = 120; // will fallback to 60 fps for devices without Pro Motion display
+    [_displayLink addToRunLoop:[NSRunLoop mainRunLoop] forMode:NSRunLoopCommonModes];
+    [_displayLink setPaused:true];
+  });


Similarly here, with an async init, couldn't it be possible to init this REANodesManager and immediately try to use _displayLink before it's initialized in something like - (void)invalidate or - (void)postOnAnimation:(REAOnAnimationCallback)clb?

ios/REANodesManager.mm

ios/keyboardObserver/REAKeyboardEventObserver.m

kmagiera · 2023-07-12T09:12:09Z

ios/REANodesManager.mm

-  [_displayLink setPaused:true];
+  __weak __typeof__(self) weakSelf = self;
+
+  RCTExecuteOnMainQueue(^void() {


I feel like a better approach here would be to update displayLink usages to access it using getter. then in the getter we'd optionally initialize. It is important that the display link is both created and accessed on the UI thread. So we could also add some assertions to verify that. Schedulinng initialization may have its own problems as we don't want to guarantee that the field wouldn't be used before the task gets executed

ios/keyboardObserver/REAKeyboardEventObserver.m

ios/REANodesManager.mm

tomekzaw · 2023-07-19T14:00:20Z

ios/REANodesManager.mm

-  if (_displayLink) {
-    [_displayLink setPaused:true];
-  }
+  [[self getDisplayLink] setPaused:true];


Suggested change

[[self getDisplayLink] setPaused:true];

[[self getDisplayLink] setPaused:YES];

tomekzaw · 2023-07-19T14:00:28Z

ios/REANodesManager.mm

@@ -211,19 +216,16 @@ - (instancetype)initWithModule:(REAModule *)reanimatedModule uiManager:(RCTUIMan
    _viewRegistry = [_uiManager valueForKey:@"_viewRegistry"];
    _shouldFlushUpdateBuffer = false;
  }
+#endif
+  [[self getDisplayLink] setPaused:true];


Suggested change

[[self getDisplayLink] setPaused:true];

[[self getDisplayLink] setPaused:YES];

pereira-carlos · 2023-07-20T02:54:44Z

ios/REANodesManager.mm

@@ -173,6 +173,18 @@ @implementation REANodesManager {
 #endif
 }

+- (CADisplayLink *)getDisplayLink
+{
+  RCTAssertMainQueue();


It seems like the assertion will raise an exception with this approach, since the code is not being dispatched to the main queue. Or am I missing something?

This assertion will fail when running from a non-main thread.

I can confirm that crash on startup

piaskowyk

Application with Paper architecture crash on startup. We need to fix it before merge.

Aleksandra Cynk added 2 commits July 10, 2023 14:02

Add "RCTExecuteOnMainQueue"

d31945c

Remove boiler-plate

42abfda

Latropos marked this pull request as ready for review July 10, 2023 12:38

Latropos requested a review from tomekzaw July 10, 2023 12:38

piaskowyk requested changes Jul 10, 2023

View reviewed changes

ios/REANodesManager.mm Outdated Show resolved Hide resolved

Update ios/REANodesManager.mm

5d57262

Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com>

Latropos changed the title ~~Add "RCTExecuteOnMainQueue"~~ Initialize CADisplayLink on UI thread Jul 10, 2023

code review

b3d76e1

Latropos requested a review from piaskowyk July 10, 2023 14:34

Caiopia reviewed Jul 10, 2023

View reviewed changes

ios/keyboardObserver/REAKeyboardEventObserver.m Show resolved Hide resolved

Caiopia reviewed Jul 10, 2023

View reviewed changes

Code review

c668647

Latropos requested a review from Caiopia July 11, 2023 10:12

Delete _currentAnimationTimestamp

3c21b0d

tomekzaw reviewed Jul 11, 2023

View reviewed changes

Code review

f93b698

kmagiera reviewed Jul 12, 2023

View reviewed changes

Aleksandra Cynk added 2 commits July 18, 2023 10:59

Add getter

0713f1b

Add second file

7697816

Latropos requested review from tomekzaw and kmagiera July 18, 2023 11:03

tomekzaw reviewed Jul 18, 2023

View reviewed changes

ios/keyboardObserver/REAKeyboardEventObserver.m Outdated Show resolved Hide resolved

tomekzaw reviewed Jul 18, 2023

View reviewed changes

ios/keyboardObserver/REAKeyboardEventObserver.m Outdated Show resolved Hide resolved

tomekzaw reviewed Jul 18, 2023

View reviewed changes

ios/keyboardObserver/REAKeyboardEventObserver.m Outdated Show resolved Hide resolved

tomekzaw reviewed Jul 18, 2023

View reviewed changes

ios/REANodesManager.mm Outdated Show resolved Hide resolved

Code review

a81b770

Latropos commented Jul 19, 2023

View reviewed changes

ios/REANodesManager.mm Show resolved Hide resolved

Aleksandra Cynk added 2 commits July 19, 2023 12:46

--amend

521ae94

--amend

baad88e

Latropos requested a review from tomekzaw July 19, 2023 10:50

tomekzaw reviewed Jul 19, 2023

View reviewed changes

--amend

24543ef

tomekzaw approved these changes Jul 19, 2023

View reviewed changes

piaskowyk previously approved these changes Jul 19, 2023

View reviewed changes

pereira-carlos reviewed Jul 20, 2023

View reviewed changes

piaskowyk requested changes Jul 20, 2023

View reviewed changes

Latropos requested a review from piaskowyk July 24, 2023 08:58

Latropos force-pushed the acynk/fix-lock branch from 7ceaf46 to 4d57e2d Compare July 24, 2023 09:30

Add RCTExecuteOnMainQueue

07e63fb

Latropos force-pushed the acynk/fix-lock branch from 4d57e2d to 07e63fb Compare July 24, 2023 09:32

Aleksandra Cynk added 3 commits July 24, 2023 11:50

Remove RCTAssertMainQueue()

2d9e055

-||-

3a4d954

Add useDisplayLinkOnMainQueue

24370f1

piaskowyk approved these changes Jul 24, 2023

View reviewed changes

tomekzaw approved these changes Jul 24, 2023

View reviewed changes

Latropos added this pull request to the merge queue Jul 24, 2023

Merged via the queue into main with commit b60dea2 Jul 24, 2023
4 checks passed

Latropos deleted the acynk/fix-lock branch July 24, 2023 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize CADisplayLink on UI thread #4694

Initialize CADisplayLink on UI thread #4694

Latropos commented Jul 10, 2023 •

edited

piaskowyk left a comment

Caiopia Jul 10, 2023

kmagiera Jul 12, 2023

tomekzaw Jul 19, 2023

tomekzaw Jul 19, 2023

pereira-carlos Jul 20, 2023 •

edited

piaskowyk left a comment

	[[self getDisplayLink] setPaused:true];
	[[self getDisplayLink] setPaused:YES];

Initialize CADisplayLink on UI thread #4694

Initialize CADisplayLink on UI thread #4694

Conversation

Latropos commented Jul 10, 2023 • edited

Summary

Test plan

piaskowyk left a comment

Choose a reason for hiding this comment

Caiopia Jul 10, 2023

Choose a reason for hiding this comment

kmagiera Jul 12, 2023

Choose a reason for hiding this comment

tomekzaw Jul 19, 2023

Choose a reason for hiding this comment

tomekzaw Jul 19, 2023

Choose a reason for hiding this comment

pereira-carlos Jul 20, 2023 • edited

Choose a reason for hiding this comment

piaskowyk left a comment

Choose a reason for hiding this comment

Latropos commented Jul 10, 2023 •

edited

pereira-carlos Jul 20, 2023 •

edited