From 9a2c95dc83810e68e3ef8ac11de001034b7d3762 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Sat, 16 Nov 2024 19:50:19 -0800 Subject: [PATCH] iOS: Eliminate FlutterEngine dealloc notification (flutter/engine#56650) FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon reception of the notification. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must not contain a nil pointer and can thus skip compaction. Under ARC, weak pointers are automatically nilled out when the last strong reference is released, so the above heuristic is no longer valid. We work around the issue by storing a nil pointer before calling compact. See http://www.openradar.me/15396578 for the radar tracking this bug. I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: https://github.com/flutter/flutter/issues/155943 Issue: https://github.com/flutter/flutter/issues/137801 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../ios/framework/Source/FlutterEngine.mm | 5 ---- .../framework/Source/FlutterEngineGroup.mm | 26 +++++++------------ .../Source/FlutterEngineGroupTest.mm | 5 ++++ .../ios/framework/Source/FlutterEngineTest.mm | 17 ------------ .../framework/Source/FlutterEngine_Internal.h | 2 -- .../ios/framework/Source/FlutterEngine_Test.h | 2 -- 6 files changed, 15 insertions(+), 42 deletions(-) diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index 8ab4cf61c0..85f1ba5519 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -84,7 +84,6 @@ NSString* const FlutterDefaultInitialRoute = nil; #pragma mark - Internal constants -NSString* const kFlutterEngineWillDealloc = @"FlutterEngineWillDealloc"; NSString* const kFlutterKeyDataChannel = @"flutter/keydata"; static constexpr int kNumProfilerSamplesPerSec = 5; @@ -282,10 +281,6 @@ static constexpr int kNumProfilerSamplesPerSec = 5; } }]; - [[NSNotificationCenter defaultCenter] postNotificationName:kFlutterEngineWillDealloc - object:self - userInfo:nil]; - // nil out weak references. // TODO(cbracken): https://github.com/flutter/flutter/issues/156222 // Ensure that FlutterEngineRegistrar is using weak pointers, then eliminate this code. diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroup.mm b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroup.mm index b274475f7e..3202c195d9 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroup.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroup.mm @@ -12,7 +12,7 @@ FLUTTER_ASSERT_ARC @interface FlutterEngineGroup () @property(nonatomic, copy) NSString* name; -@property(nonatomic, strong) NSMutableArray* engines; +@property(nonatomic, strong) NSPointerArray* engines; @property(nonatomic, copy) FlutterDartProject* project; @property(nonatomic, assign) NSUInteger enginesCreatedCount; @end @@ -23,7 +23,7 @@ FLUTTER_ASSERT_ARC self = [super init]; if (self) { _name = [name copy]; - _engines = [[NSMutableArray alloc] init]; + _engines = [NSPointerArray weakObjectsPointerArray]; _project = project; } return self; @@ -51,6 +51,12 @@ FLUTTER_ASSERT_ARC NSArray* entrypointArgs = options.entrypointArgs; FlutterEngine* engine; + // NSPointerArray is clever and assumes that unless a mutation operation has occurred on it that + // has set one of its values to nil, nothing could have changed and it can skip compaction. + // That's reasonable behaviour on a regular NSPointerArray but not for a weakObjectPointerArray. + // As a workaround, we mutate it first. See: http://www.openradar.me/15396578 + [self.engines addPointer:nil]; + [self.engines compact]; if (self.engines.count <= 0) { engine = [self makeEngine]; [engine runWithEntrypoint:entrypoint @@ -58,20 +64,13 @@ FLUTTER_ASSERT_ARC initialRoute:initialRoute entrypointArgs:entrypointArgs]; } else { - FlutterEngine* spawner = (FlutterEngine*)[self.engines[0] pointerValue]; + FlutterEngine* spawner = (__bridge FlutterEngine*)[self.engines pointerAtIndex:0]; engine = [spawner spawnWithEntrypoint:entrypoint libraryURI:libraryURI initialRoute:initialRoute entrypointArgs:entrypointArgs]; } - // TODO(cbracken): https://github.com/flutter/flutter/issues/155943 - [self.engines addObject:[NSValue valueWithPointer:(__bridge void*)engine]]; - - NSNotificationCenter* center = [NSNotificationCenter defaultCenter]; - [center addObserver:self - selector:@selector(onEngineWillBeDealloced:) - name:kFlutterEngineWillDealloc - object:engine]; + [self.engines addPointer:(__bridge void*)engine]; return engine; } @@ -82,9 +81,4 @@ FLUTTER_ASSERT_ARC return [[FlutterEngine alloc] initWithName:engineName project:self.project]; } -- (void)onEngineWillBeDealloced:(NSNotification*)notification { - // TODO(cbracken): https://github.com/flutter/flutter/issues/155943 - [self.engines removeObject:[NSValue valueWithPointer:(__bridge void*)notification.object]]; -} - @end diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroupTest.mm b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroupTest.mm index 77929ea6b4..2d25d457c0 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroupTest.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineGroupTest.mm @@ -37,11 +37,16 @@ FLUTTER_ASSERT_ARC } - (void)testDeleteLastEngine { + __weak FlutterEngine* weakSpawner; FlutterEngineGroup* group = [[FlutterEngineGroup alloc] initWithName:@"foo" project:nil]; @autoreleasepool { FlutterEngine* spawner = [group makeEngineWithEntrypoint:nil libraryURI:nil]; + weakSpawner = spawner; XCTAssertNotNil(spawner); + XCTAssertNotNil(weakSpawner); } + XCTAssertNil(weakSpawner); + FlutterEngine* spawnee = [group makeEngineWithEntrypoint:nil libraryURI:nil]; XCTAssertNotNil(spawnee); } diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm index ddc18c5429..58bfbdf160 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm @@ -262,23 +262,6 @@ FLUTTER_ASSERT_ARC XCTAssertNotNil(spawn); } -- (void)testDeallocNotification { - XCTestExpectation* deallocNotification = [self expectationWithDescription:@"deallocNotification"]; - NSNotificationCenter* center = [NSNotificationCenter defaultCenter]; - id observer; - @autoreleasepool { - FlutterEngine* engine = [[FlutterEngine alloc] initWithName:@"foobar"]; - observer = [center addObserverForName:kFlutterEngineWillDealloc - object:engine - queue:[NSOperationQueue mainQueue] - usingBlock:^(NSNotification* note) { - [deallocNotification fulfill]; - }]; - } - [self waitForExpectations:@[ deallocNotification ]]; - [center removeObserver:observer]; -} - - (void)testSetHandlerAfterRun { FlutterEngine* engine = [[FlutterEngine alloc] initWithName:@"foobar"]; XCTestExpectation* gotMessage = [self expectationWithDescription:@"gotMessage"]; diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h index 489dfd38c4..6f954adbcb 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h @@ -29,8 +29,6 @@ NS_ASSUME_NONNULL_BEGIN -extern NSString* const kFlutterEngineWillDealloc; - @interface FlutterEngine () - (flutter::Shell&)shell; diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Test.h b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Test.h index 238f6528aa..104ab9aef5 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Test.h +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Test.h @@ -11,8 +11,6 @@ #import "flutter/shell/platform/darwin/ios/rendering_api_selection.h" #include "flutter/shell/platform/embedder/embedder.h" -extern NSString* const kFlutterEngineWillDealloc; - @class FlutterBinaryMessengerRelay; namespace flutter {