Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] Fixes main thread stuck when reload in bridgeless mode #45486

Closed
wants to merge 3 commits into from

Conversation

zhongwuzw
Copy link
Contributor

Summary:

In fabric bridgeless mode, when we reload, main thread may block because of dead lock. the backtrace example as below:

(lldb) bt
* thread #1, stop reason = signal SIGSTOP
  * frame #0: 0x000000010a5c76f2 libsystem_kernel.dylib`__psynch_mutexwait + 10
    frame #1: 0x00000001099e0a70 libsystem_pthread.dylib`_pthread_mutex_firstfit_lock_wait + 78
    frame #2: 0x00000001099de82b libsystem_pthread.dylib`_pthread_mutex_firstfit_lock_slow + 217
    frame #3: 0x00007ff80030c6b9 libc++.1.dylib`std::__1::mutex::lock() + 9
    frame #4: 0x0000000106968b13 RNTester`std::__1::lock_guard<std::__1::mutex>::lock_guard[abi:ue170006](this=0x00007ff7b95e2478, __m=0x000060000377c958) at lock_guard.h:35:10
    frame #5: 0x00000001069689ed RNTester`std::__1::lock_guard<std::__1::mutex>::lock_guard[abi:ue170006](this=0x00007ff7b95e2478, __m=0x000060000377c958) at lock_guard.h:34:19
    frame #6: 0x00000001070691c1 RNTester`-[RCTInstance invalidate](self=0x000060000377c900, _cmd="invalidate") at RCTInstance.mm:146:31
    frame #7: 0x0000000107060fd2 RNTester`-[RCTHost didReceiveReloadCommand](self=0x0000600003d100f0, _cmd="didReceiveReloadCommand") at RCTHost.mm:317:3
    frame #8: 0x0000000106b005a5 RNTester`RCTTriggerReloadCommandListeners(reason=@"Global hotkey") at RCTReloadCommand.m:57:5
    frame #9: 0x0000000106b86da5 RNTester`__28-[RCTDevSettings initialize]_block_invoke.157(.block_descriptor=0x0000000107496170, params=0x00007ff84002f610) at RCTDevSettings.mm:201:11
    frame #10: 0x0000000106ae658e RNTester`__65-[RCTPackagerConnection reconnectingWebSocket:didReceiveMessage:]_block_invoke.68(.block_descriptor=0x0000600000c82df0) at RCTPackagerConnection.mm:293:9
    frame #11: 0x0000000109a4529d libdispatch.dylib`_dispatch_call_block_and_release + 12
    frame #12: 0x0000000109a4658f libdispatch.dylib`_dispatch_client_callout + 8
    frame #13: 0x0000000109a563ee libdispatch.dylib`_dispatch_main_queue_drain + 1362
    frame #14: 0x0000000109a55e8e libdispatch.dylib`_dispatch_main_queue_callback_4CF + 31
    frame #15: 0x00007ff800429af4 CoreFoundation`__CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
    frame #16: 0x00007ff80042442f CoreFoundation`__CFRunLoopRun + 2463
    frame #17: 0x00007ff8004236ad CoreFoundation`CFRunLoopRunSpecific + 557
    frame #18: 0x00007ff8103da08f GraphicsServices`GSEventRunModal + 137
    frame #19: 0x00007ff805cc0ad1 UIKitCore`-[UIApplication _run] + 972
    frame #20: 0x00007ff805cc5551 UIKitCore`UIApplicationMain + 123
    frame #21: 0x00000001069205a0 RNTester`main(argc=1, argv=0x00007ff7b95e3b60) at main.m:15:12
    frame #22: 0x00000001099023e0 dyld_sim`start_sim + 10
    frame #23: 0x0000000116b92366 dyld`start + 1942


(lldb) bt
* thread #3
    frame #0: 0x000000010a5c6b86 libsystem_kernel.dylib`__ulock_wait + 10
    frame #1: 0x0000000109a46eb1 libdispatch.dylib`_dlock_wait + 46
    frame #2: 0x0000000109a46d08 libdispatch.dylib`_dispatch_thread_event_wait_slow + 40
    frame #3: 0x0000000109a5774a libdispatch.dylib`__DISPATCH_WAIT_FOR_QUEUE__ + 371
    frame #4: 0x0000000109a57161 libdispatch.dylib`_dispatch_sync_f_slow + 240
    frame #5: 0x0000000106b3b33b RNTester`RCTUnsafeExecuteOnMainQueueSync(block=0x0000000106f116c0) at RCTUtils.m:291:5
  * frame #6: 0x0000000106f115ad RNTester`-[RCTFabricSurface start](self=0x000000010af0df40, _cmd="start") at RCTFabricSurface.mm:102:3
    frame #7: 0x00000001070601ce RNTester`__108-[RCTHost initWithBundleURLProvider:hostDelegate:turboModuleManagerDelegate:jsEngineProvider:launchOptions:]_block_invoke_2(.block_descriptor=0x0000600000c75590) at RCTHost.mm:211:9
    frame #8: 0x000000010706cdc8 RNTester`-[RCTInstance _loadScriptFromSource:](self=0x000060000377c900, _cmd="_loadScriptFromSource:", source=0x0000600000cd57d0) at RCTInstance.mm:472:5
    frame #9: 0x000000010706ca81 RNTester`__29-[RCTInstance _loadJSBundle:]_block_invoke.120(.block_descriptor=0x0000600000c96d00, error=0x0000000000000000, source=0x0000600000cd57d0) at RCTInstance.mm:452:9
    frame #10: 0x0000000106ab1919 RNTester`invocation function for block in attemptAsynchronousLoadOfBundleAtURL(.block_descriptor=0x00006000017b0fc0, statusCode=200, headers=6 key/value pairs, data=0x00006000002a4760, error=0x0000000000000000, done=YES) block_pointer, void (NSError*, RCTSource*) block_pointer) at RCTJavaScriptLoader.mm:318:9
    frame #11: 0x0000000106ad92a6 RNTester`__80-[RCTMultipartDataTask URLSession:streamTask:didBecomeInputStream:outputStream:]_block_invoke(.block_descriptor=0x000070000035c7a0, headers=6 key/value pairs, content=0x00006000002a4760, done=YES) at RCTMultipartDataTask.m:121:9
    frame #12: 0x0000000106ad9b4f RNTester`-[RCTMultipartStreamReader emitChunk:headers:callback:done:](self=0x00006000002b4220, _cmd="emitChunk:headers:callback:done:", data=0x00006000002a4020, headers=6 key/value pairs, callback=0x0000000106ad9230, done=YES) at RCTMultipartStreamReader.m:57:5
    frame #13: 0x0000000106ada800 RNTester`-[RCTMultipartStreamReader readAllPartsWithCompletionCallback:progressCallback:](self=0x00006000002b4220, _cmd="readAllPartsWithCompletionCallback:progressCallback:", callback=0x0000000106ad9230, progressCallback=0x0000000106ab2a60) at RCTMultipartStreamReader.m:154:7
    frame #14: 0x0000000106ad9130 RNTester`-[RCTMultipartDataTask URLSession:streamTask:didBecomeInputStream:outputStream:](self=0x00006000017b0d40, _cmd="URLSession:streamTask:didBecomeInputStream:outputStream:", session=0x000000010e02a0a0, streamTask=0x000000010c83ba00, inputStream=0x000060000300d4d0, outputStream=0x000060000300c990) at RCTMultipartDataTask.m:119:20
    frame #15: 0x00007ff80479fdf9 CFNetwork`___lldb_unnamed_symbol2876 + 42
    frame #16: 0x0000000109a4529d libdispatch.dylib`_dispatch_call_block_and_release + 12
    frame #17: 0x0000000109a4658f libdispatch.dylib`_dispatch_client_callout + 8
    frame #18: 0x0000000109a4e4ba libdispatch.dylib`_dispatch_lane_serial_drain + 1127
    frame #19: 0x0000000109a4f255 libdispatch.dylib`_dispatch_lane_invoke + 441
    frame #20: 0x0000000109a5c356 libdispatch.dylib`_dispatch_root_queue_drain_deferred_wlh + 318
    frame #21: 0x0000000109a5b751 libdispatch.dylib`_dispatch_workloop_worker_thread + 590
    frame #22: 0x00000001099dfb84 libsystem_pthread.dylib`_pthread_wqthread + 327
    frame #23: 0x00000001099deacf libsystem_pthread.dylib`start_wqthread + 15

Changelog:

[IOS] [FIXED] - Fixes main thread stuck when reload in bridgeless mode

Test Plan:

RNTester, enables fabric, which is very easy to repro by tapping r command multiple times quickly to trigger reload.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jul 17, 2024
@analysis-bot
Copy link

analysis-bot commented Jul 17, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 21,353,613 -1,243
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 24,549,459 -532
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: fcd526d
Branch: main

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zhongwuzw, thanks for the fix. I left a question/suggestion as the change is slightly different from the original code.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cipolleschi
Copy link
Contributor

Hi @zhongwuzw! A colleague suggested that we might just remove the callback.

Given this todo, we should be able to just start the surface after it is created.

Do you mind implementing the changes, please?

@zhongwuzw
Copy link
Contributor Author

Hi @zhongwuzw! A colleague suggested that we might just remove the callback.

Given this todo, we should be able to just start the surface after it is created.

Do you mind implementing the changes, please?

@cipolleschi Hi, do you mean we can call the start surface on BufferedRuntimeExecutor?

@cipolleschi
Copy link
Contributor

Yes, probably yes! :D

@zhongwuzw
Copy link
Contributor Author

@cipolleschi I changed to bufferedruntime.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Aug 2, 2024
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in a778979.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @zhongwuzw in a778979

When will my fix make it into a release? | How to file a pick request?

moduleRegistry:_moduleRegistry
parentInspectorTarget:_inspectorTarget.get()
launchOptions:_launchOptions];
[_hostDelegate hostDidStart:self];

for (RCTFabricSurface *surface in [self _getAttachedSurfaces]) {
[surface resetWithSurfacePresenter:self.surfacePresenter];
[_instance callFunctionOnBufferedRumtimeExecutor:[surface](facebook::jsi::Runtime &_) { [surface start]; }];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small typo "rumtime"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch, I would fix it in my next PR. :)

facebook-github-bot pushed a commit that referenced this pull request Aug 5, 2024
Summary:
I introduced a typo in #45486 . Thanks migueldaipre
 for the catch-up. cc cipolleschi

## Changelog:

[IOS] [FIXED] - Fixes typo of  function callFunctionOnBufferedRumtimeExecutor

Pull Request resolved: #45902

Test Plan: Just a typo.

Reviewed By: cipolleschi

Differential Revision: D60775511

Pulled By: arushikesarwani94

fbshipit-source-id: da781ea5ecf2e0a15e5419430240e10194043b1b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants