[iOS] Destroy the engine prior to application termination. #29295

zhongwuzw · 2021-10-22T12:41:32Z

Try fixes flutter/flutter#90783.
Based on the full crash log, Flutter tries to use std::mutex when App is terminating. we can fix it by leaking global static mutex. React Native and ComponentKit also faced this issue, we can refer to them.

cc. @chinmaygarde

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…ctor fiasco

flutter-dashboard · 2021-10-22T12:41:34Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chinmaygarde · 2021-10-23T19:15:50Z

I don't think this will fix crashing. It will just move the crash to some other spot. In fact, in the absence of graceful termination, I don't think there is a way to avoid crashes during termination.

Apple's Technical Q&A QA1561 explicitly states "Do not call the exit function. Applications calling exit will appear to the user to have crashed, rather than performing a graceful termination and animating back to the Home screen."

So, is the crash happening because someone called exit or because the engine is not reacting to graceful termination? If it's the former, the fix is to stop whoever is calling exit from doing so. If it's the latter, we need to figure out how that happens and gracefully terminate the engine.

zhongwuzw · 2021-10-25T15:05:42Z

I don't think this will fix crashing. It will just move the crash to some other spot. In fact, in the absence of graceful termination, I don't think there is a way to avoid crashes during termination.

Apple's Technical Q&A QA1561 explicitly states "Do not call the exit function. Applications calling exit will appear to the user to have crashed, rather than performing a graceful termination and animating back to the Home screen."

So, is the crash happening because someone called exit or because the engine is not reacting to graceful termination? If it's the former, the fix is to stop whoever is calling exit from doing so. If it's the latter, we need to figure out how that happens and gracefully terminate the engine.

@chinmaygarde You're right, we may need to change all global mutex to heap allocated or handle the termination gracefully. From the backtrace of crash log, I think the crash happening because engine is not reacting to graceful termination, case like user swipe up from the bottom into app switcher, then kill the App directly, seems we didn't handle this case, I think we can destroy the engine when we check app will terminate, the code like below .

diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
index cc5ac62d3..0a349b25a 100644
--- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
+++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
@@ -275,6 +275,11 @@ typedef enum UIAccessibilityContrast : NSInteger {
              selector:@selector(applicationWillResignActive:)
                  name:UIApplicationWillResignActiveNotification
                object:nil];
+  
+  [center addObserver:self
+             selector:@selector(applicationWillTerminate:)
+                 name:UIApplicationWillTerminateNotification
+               object:nil];
 
   [center addObserver:self
              selector:@selector(applicationDidEnterBackground:)
@@ -790,6 +795,10 @@ static void sendFakeTouchEvent(FlutterEngine* engine,
   [self goToApplicationLifecycle:@"AppLifecycleState.inactive"];
 }
 
+- (void)applicationWillTerminate:(NSNotification*)notification {
+  [self goToApplicationLifecycle:@"AppLifecycleState.detached"];
+  [self.engine destroyContext];
+}
+
 - (void)applicationDidEnterBackground:(NSNotification*)notification {
   TRACE_EVENT0("flutter", "applicationDidEnterBackground");
   [self surfaceUpdated:NO];

chinmaygarde · 2021-10-25T17:06:11Z

I think the crash happening because engine is not reacting to graceful termination, case like user swipe up from the bottom into app switcher, then kill the App directly, seems we didn't handle this case,

Right, this seems to the most likely cause of the error. This would also explain why it's hard to observe or reproduce. Changing all global mutexes to be heap allocated and leaked will also not help because those mutexes guard process global information in the VM. So I think graceful termination is the only way to go. You patch to do so makes a lot of sense to me. Have you been able to reproduce the crash with your error reporting solution and swiping the app up to terminate it? If so, we can pretty confidently say we have the reproducible case.

…d destructor fiasco" This reverts commit a2c913f.

zhongwuzw · 2021-10-26T16:07:49Z

Have you been able to reproduce the crash with your error reporting solution and swiping the app up to terminate it? If so, we can pretty confidently say we have the reproducible case.

@chinmaygarde Sorry, I didn't reproduce natively, but I think as long as we ensure terminate the engine before App called static global variables destructor, we can avoid the crash. UIApplicationWillTerminateNotification is called before __cxa_finalize_ranges,so we can do engine's termination when observed UIApplicationWillTerminateNotification.

Thread 0 name:
Thread 0:
0   BBRtc                         	0x0000000104bad558 0x104afc000 + 726360
1   libsystem_c.dylib             	0x00000001a831c0a8 __cxa_finalize_ranges + 408 (atexit.c:284)
2   libsystem_c.dylib             	0x00000001a831c400 exit + 28 (exit.c:81)
3   UIKitCore                     	0x00000001a18ceae4 -[UIApplication _terminateWithStatus:] + 504 (UIApplication.m:6510)
4   UIKitCore                     	0x00000001a0f1d39c -[_UISceneLifecycleMultiplexer _evalTransitionToSettings:fromSettings:forceExit:withTransitionStore:] + 128 (_UISceneLifecycleMultiplexer.m:831)
5   UIKitCore                     	0x00000001a0f1cfcc -[_UISceneLifecycleMultiplexer forceExitWithTransitionContext:scene:] + 220 (_UISceneLifecycleMultiplexer.m:466)
6   UIKitCore                     	0x00000001a18c454c -[UIApplication workspaceShouldExit:withTransitionContext:] + 212 (UIApplication.m:3583)
7   FrontBoardServices            	0x00000001aec75780 -[FBSUIApplicationWorkspaceShim workspaceShouldExit:withTransitionContext:] + 88 (FBSUIApplicationWorkspace.m:144)
8   FrontBoardServices            	0x00000001aeca4390 __63-[FBSWorkspaceScenesClient willTerminateWithTransitionContext:]_block_invoke_2 + 80 (FBSWorkspaceScenesClient.m:312)
9   FrontBoardServices            	0x00000001aec884a0 -[FBSWorkspace _calloutQueue_executeCalloutFromSource:withBlock:] + 240 (FBSWorkspace.m:355)
10  FrontBoardServices            	0x00000001aeca4328 __63-[FBSWorkspaceScenesClient willTerminateWithTransitionContext:]_block_invoke + 132 (FBSWorkspaceScenesClient.m:309)
11  libdispatch.dylib             	0x000000019eb01db0 _dispatch_client_callout + 20 (object.m:559)
12  libdispatch.dylib             	0x000000019eb05738 _dispatch_block_invoke_direct + 268 (queue.c:468)
13  FrontBoardServices            	0x00000001aeccd250 __FBSSERIALQUEUE_IS_CALLING_OUT_TO_A_BLOCK__ + 48 (FBSSerialQueue.m:184)
14  FrontBoardServices            	0x00000001aecccee0 -[FBSSerialQueue _targetQueue_performNextIfPossible] + 448 (FBSSerialQueue.m:227)
15  FrontBoardServices            	0x00000001aeccd434 -[FBSSerialQueue _performNextFromRunLoopSource] + 32 (FBSSerialQueue.m:258)
16  CoreFoundation                	0x000000019ee8976c __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 28 (CFRunLoop.c:1967)
17  CoreFoundation                	0x000000019ee89668 __CFRunLoopDoSource0 + 208 (CFRunLoop.c:2011)
18  CoreFoundation                	0x000000019ee88960 __CFRunLoopDoSources0 + 268 (CFRunLoop.c:2048)
19  CoreFoundation                	0x000000019ee82a8c __CFRunLoopRun + 824 (CFRunLoop.c:2925)
20  CoreFoundation                	0x000000019ee8221c CFRunLoopRunSpecific + 600 (CFRunLoop.c:3242)
21  GraphicsServices              	0x00000001b6a4f784 GSEventRunModal + 164 (GSEvent.c:2259)
22  UIKitCore                     	0x00000001a18c2ee8 -[UIApplication _run] + 1072 (UIApplication.m:3253)
23  UIKitCore                     	0x00000001a18c875c UIApplicationMain + 168 (UIApplication.m:4707)
24  Runner                        	0x0000000102140534 0x102138000 + 34100
25  libdyld.dylib                 	0x000000019eb426b0 start + 4

zhongwuzw · 2021-10-28T14:36:17Z

macOS desktop also has a similar crash, which also happened in App closing, C++ destruction stage.

The-Redhat · 2021-11-11T09:25:06Z

Hey @zhongwuzw @chinmaygarde do you currently have a rough eta when this fix will land?

zhongwuzw · 2021-11-13T14:48:01Z

Hey @zhongwuzw @chinmaygarde do you currently have a rough eta when this fix will land?

Sorry, no eta currently, still under review.

chinmaygarde

I edited commit message to indicate what is actually going on in this patch. Othewise, LGTM.

…x_crash

fluttergithubbot · 2021-11-20T15:18:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…x_crash

…lutter/engine#29295)

* 'Update Dart SDK to 1278bd5' * Update cirrus secret. * [iOS] Destroy the engine prior to application termination. (#29295) * Update licenses hash. * Update shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm Co-authored-by: Jenn Magder <[email protected]> Co-authored-by: Wu Zhong <[email protected]> Co-authored-by: Jenn Magder <[email protected]>

…9295)

Leak the global static mutex in MessageLoopTaskQueues to avoid destru…

a2c913f

…ctor fiasco

flutter-dashboard bot added the needs tests label Oct 22, 2021

google-cla bot added the cla: yes label Oct 22, 2021

zhongwuzw changed the title ~~Leak the global static mutex in MessageLoopTaskQueues to avoid destructor fiasco~~ [iOS] Leak the global static mutex in MessageLoopTaskQueues to avoid destructor fiasco Oct 22, 2021

zhongwuzw added 2 commits October 26, 2021 20:54

Revert "Leak the global static mutex in MessageLoopTaskQueues to avoi…

f2aa315

…d destructor fiasco" This reverts commit a2c913f.

Terminate engine gracefully when App will terminate

82aa5f8

flutter-dashboard bot added the platform-ios label Oct 26, 2021

This was referenced Oct 27, 2021

iOS crash libc++abi.dylib std::__terminate(void (*)()) Fatal Exception: std::__1::system_error recursive_mutex lock failed: Invalid argument flutter/flutter#90783

Closed

iOS crashed log from AppStore flutter/flutter#91655

Closed

zhongwuzw requested a review from chinmaygarde November 4, 2021 00:07

CaseyHillers changed the base branch from master to main November 15, 2021 18:13

fluttergithubbot force-pushed the master branch from b1a8771 to 2962099 Compare November 15, 2021 18:30

chinmaygarde changed the title ~~[iOS] Leak the global static mutex in MessageLoopTaskQueues to avoid destructor fiasco~~ [iOS] Destroy the engine prior to application termination. Nov 18, 2021

chinmaygarde approved these changes Nov 18, 2021

View reviewed changes

Merge branch 'main' into fix_mutex_crash

c1d16c9

zhongwuzw force-pushed the fix_mutex_crash branch from 39e9925 to c1d16c9 Compare November 19, 2021 02:50

zhongwuzw changed the base branch from master to main November 19, 2021 02:51

Fixes CI tests failure

34c5c36

zhongwuzw added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 19, 2021

Merge commit '8b278873af91256a4e0c8c5b37d25f145c4755a3' into fix_mute…

766430f

…x_crash

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 20, 2021

Merge commit 'ddb8b94eebee749d415ee55bcbfc49c631ff9f28' into fix_mute…

f0ca277

…x_crash

zhongwuzw added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 21, 2021

fluttergithubbot merged commit 09ce0bd into flutter:main Nov 23, 2021

engine-flutter-autoroll mentioned this pull request Nov 23, 2021

Roll Engine from fd1151a43bb6 to 3845cee5b721 (4 revisions) flutter/flutter#94081

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2021

09ce0bd [iOS] Destroy the engine prior to application termination. (f…

b6fa25e

…lutter/engine#29295)

engine-flutter-autoroll mentioned this pull request Nov 23, 2021

Roll Engine from fd1151a43bb6 to 147824762f95 (8 revisions) flutter/flutter#94082

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 23, 2021

09ce0bd [iOS] Destroy the engine prior to application termination. (f…

a30e4df

…lutter/engine#29295)

Maru-zhang mentioned this pull request Dec 27, 2021

Report EXC_BAD_ACCESS KERN_INVALID_ADDRESS when App Terminating flutter/flutter#95844

Closed

yx-mike added a commit to yx-mike/engine that referenced this pull request Jan 12, 2022

[iOS] Destroy the engine prior to application termination. (flutter#2…

a1a678f

…9295)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[iOS] Destroy the engine prior to application termination. #29295

[iOS] Destroy the engine prior to application termination. #29295

Uh oh!

zhongwuzw commented Oct 22, 2021 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Oct 22, 2021

Uh oh!

chinmaygarde commented Oct 23, 2021

Uh oh!

zhongwuzw commented Oct 25, 2021 •

edited

Loading

Uh oh!

chinmaygarde commented Oct 25, 2021

Uh oh!

zhongwuzw commented Oct 26, 2021

Uh oh!

zhongwuzw commented Oct 28, 2021 •

edited

Loading

Uh oh!

The-Redhat commented Nov 11, 2021

Uh oh!

zhongwuzw commented Nov 13, 2021

Uh oh!

chinmaygarde left a comment

Uh oh!

fluttergithubbot commented Nov 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[iOS] Destroy the engine prior to application termination. #29295

[iOS] Destroy the engine prior to application termination. #29295

Uh oh!

Conversation

zhongwuzw commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 22, 2021

Uh oh!

chinmaygarde commented Oct 23, 2021

Uh oh!

zhongwuzw commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Oct 25, 2021

Uh oh!

zhongwuzw commented Oct 26, 2021

Uh oh!

zhongwuzw commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

The-Redhat commented Nov 11, 2021

Uh oh!

zhongwuzw commented Nov 13, 2021

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Nov 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhongwuzw commented Oct 22, 2021 •

edited

Loading

zhongwuzw commented Oct 25, 2021 •

edited

Loading

zhongwuzw commented Oct 28, 2021 •

edited

Loading