From 2c2e5c50676f4b4720a0998aa4a279b881362e42 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 18 Oct 2024 15:18:40 -0700 Subject: [PATCH 1/3] iOS: Improve thread safety of first frame callback Previously, we dispatched a task to a background thread to wait for the first frame to be ready. In the background task, we then throw a task on the main thread that invokes that user-provided callback to notify them that the first frame is ready. During ARC migration, we ensured that self was strongly-captured in the task that runs on the main thread, in order to prevent the possibility that the last remaining reference to the FlutterEngine instance be the one on the background thread. However, with the previous code, it's possible that the callback is dispatched to the main thread, executes, and completes before the block on the background thread finishes. In that case, the last remaining FlutterEngine reference may *still* be the one on the background thread. In order to prevent this scenario, we use `dispatch_groupd_notify` to ensure that the tasks are executed sequentially, and that the callback task is not dispatched to the main thread until the background task has completed. Further, we capture FlutterEngine strongly in the second task to ensure that *it* is the last remaining task. No changes to tests since this isn't testable as written. Issue: https://github.com/flutter/flutter/issues/156177 --- .../ios/framework/Source/FlutterEngine.mm | 46 +++++++++++-------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index 9fc52d4d551cf..cbeacd11eecf6 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -1359,34 +1359,40 @@ - (void)onLocaleUpdated:(NSNotification*)notification { - (void)waitForFirstFrame:(NSTimeInterval)timeout callback:(void (^_Nonnull)(BOOL didTimeout))callback { dispatch_queue_t queue = dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0); + dispatch_group_t group = dispatch_group_create(); + __weak FlutterEngine* weakSelf = self; - dispatch_async(queue, ^{ + __block BOOL didTimeout = NO; + dispatch_group_async(group, queue, ^{ FlutterEngine* strongSelf = weakSelf; if (!strongSelf) { return; } fml::TimeDelta waitTime = fml::TimeDelta::FromMilliseconds(timeout * 1000); - BOOL didTimeout = + didTimeout = strongSelf.shell.WaitForFirstFrame(waitTime).code() == fml::StatusCode::kDeadlineExceeded; - dispatch_async(dispatch_get_main_queue(), ^{ - // Capture strongSelf to ensure that destruction does not occur on a background thread. - // - // The containing block, executed on a background thread, strongly captures self, then makes a - // blocking call to self.shell.WaitForFirstFrame(). If, during this time, all other instances - // of self are released, the containing block's reference would be the last one, resulting in - // `[FlutterEngine dealloc]` being called when it goes out of scope at the end of that block, - // on a background thread. FlutterEngine owns a reference to a PlatformViewsController, which - // owns a WeakPtrFactory whose destructor asserts that it be freed on the platform thread. To - // avoid this, we capture strongSelf in the current block, which is executed on the platform - // thread. - // - // strongSelf is never nil here since it's a strong reference that's verified non-nil above, - // but we use a conditional check to avoid and unused expression compiler warning. - if (strongSelf) { - callback(didTimeout); - } - }); + }); + + // Only execute the main queue task once the background task has completely finished executing. + dispatch_group_notify(group, dispatch_get_main_queue(), ^{ + // Strongly capture self on the task dispatched to the main thread. + // + // When we capture weakSelf strongly in the above block on a background thread, we risk the + // possibility that all other strong references to FlutterEngine go out of scope while the block + // executes and that the engine is dealloc'ed at the end of the above block on a background + // thread. FlutterEngine owns a reference to a PlatformViewsController, which + // owns a WeakPtrFactory whose destructor asserts that it be freed on the platform thread. To + // avoid this, we strongly capture self in the current block, which is executed on the platform + // thread. + // + // self is never nil here since it's a strong reference that's verified non-nil above, but we + // use a conditional check to avoid and unused expression compiler warning. + FlutterEngine* strongSelf = self; + if (!strongSelf) { + return; + } + callback(didTimeout); }); } From 6e7588efff604f48f65aaa07d4bbda633e8d2233 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 18 Oct 2024 15:42:37 -0700 Subject: [PATCH 2/3] Typo --- shell/platform/darwin/ios/framework/Source/FlutterEngine.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index cbeacd11eecf6..8eaa3f09c1f21 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -1387,7 +1387,7 @@ - (void)waitForFirstFrame:(NSTimeInterval)timeout // thread. // // self is never nil here since it's a strong reference that's verified non-nil above, but we - // use a conditional check to avoid and unused expression compiler warning. + // use a conditional check to avoid an unused expression compiler warning. FlutterEngine* strongSelf = self; if (!strongSelf) { return; From 4b74fe761d8d903a4c5da0fb1911ecf67b08815b Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 18 Oct 2024 15:44:47 -0700 Subject: [PATCH 3/3] Update comment --- shell/platform/darwin/ios/framework/Source/FlutterEngine.mm | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index 8eaa3f09c1f21..b5398514d2e9c 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -1381,10 +1381,7 @@ - (void)waitForFirstFrame:(NSTimeInterval)timeout // When we capture weakSelf strongly in the above block on a background thread, we risk the // possibility that all other strong references to FlutterEngine go out of scope while the block // executes and that the engine is dealloc'ed at the end of the above block on a background - // thread. FlutterEngine owns a reference to a PlatformViewsController, which - // owns a WeakPtrFactory whose destructor asserts that it be freed on the platform thread. To - // avoid this, we strongly capture self in the current block, which is executed on the platform - // thread. + // thread. FlutterEngine is not safe to release on any thread other than the main thread. // // self is never nil here since it's a strong reference that's verified non-nil above, but we // use a conditional check to avoid an unused expression compiler warning.