From 4610d7e62ec8ec652678cd16815538b517cced28 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Sep 2019 17:09:48 -0700 Subject: [PATCH 1/6] Made the flutter view controllers dealloc notification more generic and started turning off semantics when the view controller is remotely deleted. --- .../darwin/ios/framework/Source/FlutterEngine.mm | 12 ++++++++++++ .../ios/framework/Source/FlutterEngine_Internal.h | 1 - .../ios/framework/Source/FlutterViewController.mm | 7 ++++++- .../Source/FlutterViewController_Internal.h | 3 +++ shell/platform/darwin/ios/platform_view_ios.h | 1 + shell/platform/darwin/ios/platform_view_ios.mm | 13 +++++++++++++ 6 files changed, 35 insertions(+), 2 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index 10fe77d6a217f..4493e61f0b9ef 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -33,6 +33,7 @@ @interface FlutterEngine () @property(nonatomic, readonly) NSMutableDictionary* pluginPublications; @property(nonatomic, readwrite, copy) NSString* isolateId; +@property(nonatomic, retain) id flutterViewControllerWillDeallocObserver; @end @interface FlutterEngineRegistrar : NSObject @@ -111,6 +112,7 @@ - (void)dealloc { NSNotificationCenter* center = [NSNotificationCenter defaultCenter]; [center removeObserver:self name:UIApplicationDidReceiveMemoryWarningNotification object:nil]; + [_flutterViewControllerWillDeallocObserver release]; [super dealloc]; } @@ -167,12 +169,22 @@ - (void)setViewController:(FlutterViewController*)viewController { _viewController = [viewController getWeakPtr]; self.iosPlatformView->SetOwnerViewController(_viewController); [self maybeSetupPlatformViewChannels]; + + self.flutterViewControllerWillDeallocObserver = + [[NSNotificationCenter defaultCenter] + addObserverForName:FlutterViewControllerWillDealloc + object:viewController + queue:[NSOperationQueue mainQueue] + usingBlock:^(NSNotification *note) { + [self notifyViewControllerDeallocated]; + }]; } - (void)notifyViewControllerDeallocated { if (!_allowHeadlessExecution) { [self destroyContext]; } + _viewController.reset(); } - (void)destroyContext { diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h b/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h index 868d2176a309b..573333ea732e6 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h @@ -44,7 +44,6 @@ - (FlutterTextInputPlugin*)textInputPlugin; - (void)launchEngine:(NSString*)entrypoint libraryURI:(NSString*)libraryOrNil; - (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryOrNil; -- (void)notifyViewControllerDeallocated; @end diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 3cea602f86bad..8e1e824fa2613 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -25,6 +25,9 @@ NSNotificationName const FlutterSemanticsUpdateNotification = @"FlutterSemanticsUpdate"; +NSNotificationName const FlutterViewControllerWillDealloc = @"FlutterViewControllerWillDealloc"; + + // This is left a FlutterBinaryMessenger privately for now to give people a chance to notice the // change. Unfortunately unless you have Werror turned on, incompatible pointers as arguments are // just a warning. @@ -521,7 +524,9 @@ - (void)flushOngoingTouches { } - (void)dealloc { - [_engine.get() notifyViewControllerDeallocated]; + [[NSNotificationCenter defaultCenter] postNotificationName:FlutterViewControllerWillDealloc + object:self + userInfo:nil]; [[NSNotificationCenter defaultCenter] removeObserver:self]; [_ongoingTouches release]; [super dealloc]; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h b/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h index 5947a9327ff3d..afe5b8166ce4a 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h @@ -12,6 +12,9 @@ #include "flutter/shell/platform/darwin/ios/framework/Headers/FlutterViewController.h" #include "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h" +FLUTTER_EXPORT +extern NSNotificationName const FlutterViewControllerWillDealloc; + @interface FlutterViewController () - (fml::WeakPtr)getWeakPtr; diff --git a/shell/platform/darwin/ios/platform_view_ios.h b/shell/platform/darwin/ios/platform_view_ios.h index 38fc2a9f30b28..a8574350d010c 100644 --- a/shell/platform/darwin/ios/platform_view_ios.h +++ b/shell/platform/darwin/ios/platform_view_ios.h @@ -52,6 +52,7 @@ class PlatformViewIOS final : public PlatformView { std::unique_ptr accessibility_bridge_; fml::scoped_nsprotocol text_input_plugin_; fml::closure firstFrameCallback_; + fml::scoped_nsprotocol dealloc_view_controller_observer_; // |PlatformView| void HandlePlatformMessage(fml::RefPtr message) override; diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index c80ae377c03a1..81b7db6cac6ff 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -49,6 +49,19 @@ accessibility_bridge_.reset(); } owner_controller_ = owner_controller; + + // Add an observer that will clear out the owner_controller_ ivar and + // the accessibility_bridge_ in case the view controller is deleted. + dealloc_view_controller_observer_.reset( + [[NSNotificationCenter defaultCenter] + addObserverForName:FlutterViewControllerWillDealloc + object:owner_controller_.get() + queue:[NSOperationQueue mainQueue] + usingBlock:^(NSNotification *note) { + accessibility_bridge_.reset(); + this->owner_controller_.reset(); + }]); + if (owner_controller_) { ios_surface_ = [static_cast(owner_controller.get().view) createSurface:gl_context_]; From 4dc44a6a6250b3437b8cfc9de82d33801f8f1161 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Sep 2019 17:31:27 -0700 Subject: [PATCH 2/6] Cleared up implicit copy. --- shell/platform/darwin/ios/platform_view_ios.mm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index 81b7db6cac6ff..84fa5f6f1674b 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -58,8 +58,9 @@ object:owner_controller_.get() queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification *note) { + // Implicit copy of 'this' is fine. accessibility_bridge_.reset(); - this->owner_controller_.reset(); + owner_controller_.reset(); }]); if (owner_controller_) { From f1dff6bf2c0232e340627f8cf89cf46a60de40cd Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Sep 2019 17:38:04 -0700 Subject: [PATCH 3/6] fixed formatting --- .../ios/framework/Source/FlutterEngine.mm | 13 ++++++------- .../framework/Source/FlutterViewController.mm | 1 - .../platform/darwin/ios/platform_view_ios.mm | 19 +++++++++---------- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm index 4493e61f0b9ef..b2374fa0d147c 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm @@ -171,13 +171,12 @@ - (void)setViewController:(FlutterViewController*)viewController { [self maybeSetupPlatformViewChannels]; self.flutterViewControllerWillDeallocObserver = - [[NSNotificationCenter defaultCenter] - addObserverForName:FlutterViewControllerWillDealloc - object:viewController - queue:[NSOperationQueue mainQueue] - usingBlock:^(NSNotification *note) { - [self notifyViewControllerDeallocated]; - }]; + [[NSNotificationCenter defaultCenter] addObserverForName:FlutterViewControllerWillDealloc + object:viewController + queue:[NSOperationQueue mainQueue] + usingBlock:^(NSNotification* note) { + [self notifyViewControllerDeallocated]; + }]; } - (void)notifyViewControllerDeallocated { diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 8e1e824fa2613..ab7564d2d7132 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -27,7 +27,6 @@ NSNotificationName const FlutterViewControllerWillDealloc = @"FlutterViewControllerWillDealloc"; - // This is left a FlutterBinaryMessenger privately for now to give people a chance to notice the // change. Unfortunately unless you have Werror turned on, incompatible pointers as arguments are // just a warning. diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index 84fa5f6f1674b..d42b3adf2d153 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -52,16 +52,15 @@ // Add an observer that will clear out the owner_controller_ ivar and // the accessibility_bridge_ in case the view controller is deleted. - dealloc_view_controller_observer_.reset( - [[NSNotificationCenter defaultCenter] - addObserverForName:FlutterViewControllerWillDealloc - object:owner_controller_.get() - queue:[NSOperationQueue mainQueue] - usingBlock:^(NSNotification *note) { - // Implicit copy of 'this' is fine. - accessibility_bridge_.reset(); - owner_controller_.reset(); - }]); + dealloc_view_controller_observer_.reset([[NSNotificationCenter defaultCenter] + addObserverForName:FlutterViewControllerWillDealloc + object:owner_controller_.get() + queue:[NSOperationQueue mainQueue] + usingBlock:^(NSNotification* note) { + // Implicit copy of 'this' is fine. + accessibility_bridge_.reset(); + owner_controller_.reset(); + }]); if (owner_controller_) { ios_surface_ = From 058d0e618d6a1fd526796f178967fa5621af6cbb Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 13 Sep 2019 13:59:37 -0700 Subject: [PATCH 4/6] Added test for the dealloc callback. --- .../Source/FlutterViewControllerTest.m | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m b/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m index ca431da779094..bd6850a18c623 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m @@ -12,6 +12,19 @@ #error ARC must be enabled! #endif +extern NSNotificationName const FlutterViewControllerWillDealloc; + +/// OCMock can't be used for FlutterEngine sometimes because it retains arguments to invocations +/// so it is impossible to test deleting of FluterViewControllers. +@interface MockEngine : NSObject +@end + +@implementation MockEngine +- (void)setViewController:(FlutterViewController*)viewController { + // noop +} +@end + @interface FlutterViewControllerTest : XCTestCase @end @@ -244,4 +257,20 @@ - (UITraitCollection*)fakeTraitCollectionWithContrast:(UIAccessibilityContrast)c return mockTraitCollection; } +- (void)testWillDeallocNotification { + XCTestExpectation *expectation = [[XCTestExpectation alloc] initWithDescription:@"notification called"]; + id engine = [[MockEngine alloc] init]; + FlutterViewController* realVC = [[FlutterViewController alloc] initWithEngine:engine + nibName:nil + bundle:nil]; + id observer = [[NSNotificationCenter defaultCenter] addObserverForName:FlutterViewControllerWillDealloc + object:nil + queue:[NSOperationQueue mainQueue] + usingBlock:^(NSNotification * _Nonnull note) { + [expectation fulfill]; + }]; + realVC = nil; + [self waitForExpectations:@[expectation] timeout:1.0]; +} + @end From b0ce5d67a7f6608874969eadda184ad528697ca7 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 13 Sep 2019 14:21:23 -0700 Subject: [PATCH 5/6] Updated formatting. --- .../Source/FlutterViewControllerTest.m | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m b/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m index bd6850a18c623..7130d34773f1c 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m @@ -258,19 +258,21 @@ - (UITraitCollection*)fakeTraitCollectionWithContrast:(UIAccessibilityContrast)c } - (void)testWillDeallocNotification { - XCTestExpectation *expectation = [[XCTestExpectation alloc] initWithDescription:@"notification called"]; + XCTestExpectation* expectation = + [[XCTestExpectation alloc] initWithDescription:@"notification called"]; id engine = [[MockEngine alloc] init]; FlutterViewController* realVC = [[FlutterViewController alloc] initWithEngine:engine nibName:nil bundle:nil]; - id observer = [[NSNotificationCenter defaultCenter] addObserverForName:FlutterViewControllerWillDealloc - object:nil - queue:[NSOperationQueue mainQueue] - usingBlock:^(NSNotification * _Nonnull note) { - [expectation fulfill]; - }]; + id observer = + [[NSNotificationCenter defaultCenter] addObserverForName:FlutterViewControllerWillDealloc + object:nil + queue:[NSOperationQueue mainQueue] + usingBlock:^(NSNotification* _Nonnull note) { + [expectation fulfill]; + }]; realVC = nil; - [self waitForExpectations:@[expectation] timeout:1.0]; + [self waitForExpectations:@[ expectation ] timeout:1.0]; } @end From ca3d5d509b01ef8631a1f7efb19d05bd4fe6b00f Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 13 Sep 2019 15:40:43 -0700 Subject: [PATCH 6/6] update docstring --- .../ios/framework/Source/FlutterViewControllerTest.m | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m b/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m index 7130d34773f1c..22cdeb3d676c0 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.m @@ -14,8 +14,12 @@ extern NSNotificationName const FlutterViewControllerWillDealloc; -/// OCMock can't be used for FlutterEngine sometimes because it retains arguments to invocations -/// so it is impossible to test deleting of FluterViewControllers. +/// A simple mock class for FlutterEngine. +/// +/// OCMockClass can't be used for FlutterEngine sometimes because OCMock retains arguments to +/// invocations and since the init for FlutterViewController calls a method on the +/// FlutterEngine it creates a retain cycle that stops us from testing behaviors related to +/// deleting FlutterViewControllers. @interface MockEngine : NSObject @end