Skip to content

Commit

Permalink
Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC (#51983)
Browse files Browse the repository at this point in the history
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate`FlutterCallbackCache` and `FlutterKeyboardManager` from MRC to ARC.  These files do not themselves import any MRC files, making them leaf nodes in the dependency graph of MRC files.  

Doing a few at a time to make the dependency graph manageable, and to easily revert if this causes retain cycles or other memory management issues.

Part of flutter/flutter#137801.
  • Loading branch information
jmagman authored Apr 15, 2024
1 parent fed19a6 commit a504499
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 138 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ - (void)testResize {
[binaryMessenger stopMocking];
}

- (bool)testSetWarnsOnOverflow {
- (void)testSetWarnsOnOverflow {
NSString* channelName = @"flutter/test";
id binaryMessenger = OCMStrictProtocolMock(@protocol(FlutterBinaryMessenger));
id codec = OCMProtocolMock(@protocol(FlutterMethodCodec));
Expand Down
13 changes: 8 additions & 5 deletions shell/platform/darwin/ios/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,14 @@ source_set("flutter_framework_source_arc") {
public_configs = [ "//flutter:config" ]

sources = [
"framework/Source/FlutterCallbackCache.mm",
"framework/Source/FlutterCallbackCache_Internal.h",
"framework/Source/FlutterEmbedderKeyResponder.h",
"framework/Source/FlutterEmbedderKeyResponder.mm",
"framework/Source/FlutterKeyPrimaryResponder.h",
"framework/Source/FlutterKeySecondaryResponder.h",
"framework/Source/FlutterKeyboardManager.h",
"framework/Source/FlutterKeyboardManager.mm",
"framework/Source/FlutterMetalLayer.h",
"framework/Source/FlutterMetalLayer.mm",
"framework/Source/FlutterRestorationPlugin.h",
Expand All @@ -83,7 +87,10 @@ source_set("flutter_framework_source_arc") {
"IOSurface.framework",
]

deps += [ "//flutter/shell/platform/embedder:embedder_as_internal_library" ]
deps += [
"//flutter/lib/ui",
"//flutter/shell/platform/embedder:embedder_as_internal_library",
]
}

source_set("flutter_framework_source") {
Expand All @@ -98,8 +105,6 @@ source_set("flutter_framework_source") {
# New files are highly encouraged to be in ARC.
# To add new files in ARC, add them to the `flutter_framework_source_arc` target.
"framework/Source/FlutterAppDelegate.mm",
"framework/Source/FlutterCallbackCache.mm",
"framework/Source/FlutterCallbackCache_Internal.h",
"framework/Source/FlutterChannelKeyResponder.h",
"framework/Source/FlutterChannelKeyResponder.mm",
"framework/Source/FlutterDartProject.mm",
Expand All @@ -110,8 +115,6 @@ source_set("flutter_framework_source") {
"framework/Source/FlutterEngineGroup.mm",
"framework/Source/FlutterEngine_Internal.h",
"framework/Source/FlutterHeadlessDartRunner.mm",
"framework/Source/FlutterKeyboardManager.h",
"framework/Source/FlutterKeyboardManager.mm",
"framework/Source/FlutterOverlayView.h",
"framework/Source/FlutterOverlayView.mm",
"framework/Source/FlutterPlatformPlugin.h",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,9 @@
#include "flutter/fml/logging.h"
#include "flutter/lib/ui/plugins/callback_cache.h"

@implementation FlutterCallbackInformation

- (void)dealloc {
[_callbackName release];
[_callbackClassName release];
[_callbackLibraryPath release];
[super dealloc];
}
FLUTTER_ASSERT_ARC

@implementation FlutterCallbackInformation
@end

@implementation FlutterCallbackCache
Expand All @@ -25,7 +19,7 @@ + (FlutterCallbackInformation*)lookupCallbackInformation:(int64_t)handle {
if (info == nullptr) {
return nil;
}
FlutterCallbackInformation* new_info = [[[FlutterCallbackInformation alloc] init] autorelease];
FlutterCallbackInformation* new_info = [[FlutterCallbackInformation alloc] init];
new_info.callbackName = [NSString stringWithUTF8String:info->name.c_str()];
new_info.callbackClassName = [NSString stringWithUTF8String:info->class_name.c_str()];
new_info.callbackLibraryPath = [NSString stringWithUTF8String:info->library_path.c_str()];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
// found in the LICENSE file.

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.h"

#include "flutter/fml/platform/darwin/message_loop_darwin.h"
#include "flutter/fml/platform/darwin/weak_nsobject.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h"

FLUTTER_ASSERT_ARC

static constexpr CFTimeInterval kDistantFuture = 1.0e10;

Expand All @@ -13,13 +16,13 @@ @interface FlutterKeyboardManager ()
/**
* The primary responders added by addPrimaryResponder.
*/
@property(nonatomic, retain, readonly)
@property(nonatomic, copy, readonly)
NSMutableArray<id<FlutterKeyPrimaryResponder>>* primaryResponders;

/**
* The secondary responders added by addSecondaryResponder.
*/
@property(nonatomic, retain, readonly)
@property(nonatomic, copy, readonly)
NSMutableArray<id<FlutterKeySecondaryResponder>>* secondaryResponders;

- (void)dispatchToSecondaryResponders:(nonnull FlutterUIPressProxy*)press
Expand All @@ -28,16 +31,13 @@ - (void)dispatchToSecondaryResponders:(nonnull FlutterUIPressProxy*)press

@end

@implementation FlutterKeyboardManager {
std::unique_ptr<fml::WeakNSObjectFactory<FlutterKeyboardManager>> _weakFactory;
}
@implementation FlutterKeyboardManager

- (nonnull instancetype)init {
self = [super init];
if (self != nil) {
_primaryResponders = [[NSMutableArray alloc] init];
_secondaryResponders = [[NSMutableArray alloc] init];
_weakFactory = std::make_unique<fml::WeakNSObjectFactory<FlutterKeyboardManager>>(self);
}
return self;
}
Expand All @@ -50,24 +50,6 @@ - (void)addSecondaryResponder:(nonnull id<FlutterKeySecondaryResponder>)responde
[_secondaryResponders addObject:responder];
}

- (void)dealloc {
// It will be destroyed and invalidate its weak pointers
// before any other members are destroyed.
_weakFactory.reset();

[_primaryResponders removeAllObjects];
[_secondaryResponders removeAllObjects];
[_primaryResponders release];
[_secondaryResponders release];
_primaryResponders = nil;
_secondaryResponders = nil;
[super dealloc];
}

- (fml::WeakNSObject<FlutterKeyboardManager>)getWeakNSObject {
return _weakFactory->GetWeakNSObject();
}

- (void)handlePress:(nonnull FlutterUIPressProxy*)press
nextAction:(nonnull void (^)())next API_AVAILABLE(ios(13.4)) {
if (@available(iOS 13.4, *)) {
Expand All @@ -89,7 +71,7 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press
// encounter.
NSAssert([_primaryResponders count] >= 0, @"At least one primary responder must be added.");

__block auto weakSelf = [self getWeakNSObject];
__block __weak __typeof(self) weakSelf = self;
__block NSUInteger unreplied = [self.primaryResponders count];
__block BOOL anyHandled = false;
FlutterAsyncKeyCallback replyCallback = ^(BOOL handled) {
Expand All @@ -98,7 +80,7 @@ - (void)handlePress:(nonnull FlutterUIPressProxy*)press
anyHandled = anyHandled || handled;
if (unreplied == 0) {
if (!anyHandled && weakSelf) {
[weakSelf.get() dispatchToSecondaryResponders:press complete:completeCallback];
[weakSelf dispatchToSecondaryResponders:press complete:completeCallback];
} else {
completeCallback(true, press);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,43 +23,6 @@

using namespace flutter::testing;

/// Sometimes we have to use a custom mock to avoid retain cycles in ocmock.
@interface FlutterEnginePartialMock : FlutterEngine
@property(nonatomic, strong) FlutterBasicMessageChannel* lifecycleChannel;
@property(nonatomic, weak) FlutterViewController* viewController;
@property(nonatomic, assign) BOOL didCallNotifyLowMemory;
@end

@interface FlutterEngine ()
- (BOOL)createShell:(NSString*)entrypoint
libraryURI:(NSString*)libraryURI
initialRoute:(NSString*)initialRoute;
- (void)dispatchPointerDataPacket:(std::unique_ptr<flutter::PointerDataPacket>)packet;
@end

@interface FlutterEngine (TestLowMemory)
- (void)notifyLowMemory;
@end

extern NSNotificationName const FlutterViewControllerWillDealloc;

/// 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

@interface FlutterKeyboardManagerUnittestsObjC : NSObject
- (bool)nextResponderShouldThrowOnPressesEnded;
- (bool)singlePrimaryResponder;
- (bool)doublePrimaryResponder;
- (bool)singleSecondaryResponder;
- (bool)emptyNextResponder;
@end

namespace {

typedef void (^KeyCallbackSetter)(FlutterUIPressProxy* press, FlutterAsyncKeyCallback callback)
Expand All @@ -68,52 +31,28 @@ typedef void (^KeyCallbackSetter)(FlutterUIPressProxy* press, FlutterAsyncKeyCal

} // namespace

// These tests were designed to run on iOS 13.4 or later.
API_AVAILABLE(ios(13.4))
@interface FlutterKeyboardManagerTest : XCTestCase
@property(nonatomic, strong) id mockEngine;
- (FlutterViewController*)mockOwnerWithPressesBeginOnlyNext API_AVAILABLE(ios(13.4));
@end

@implementation FlutterKeyboardManagerTest

- (void)setUp {
// All of these tests were designed to run on iOS 13.4 or later.
if (@available(iOS 13.4, *)) {
} else {
XCTSkip(@"Required API not present for test.");
}

[super setUp];
self.mockEngine = OCMClassMock([FlutterEngine class]);
}

- (void)tearDown {
// We stop mocking here to avoid retain cycles that stop
// FlutterViewControllers from deallocing.
[self.mockEngine stopMocking];
self.mockEngine = nil;
[super tearDown];
}

- (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {
return [OCMArg checkWithBlock:^BOOL(id value) {
if (![value isKindOfClass:[FlutterUIPressProxy class]]) {
return NO;
}
FlutterUIPressProxy* press = value;
return press.key.keyCode == keyCode;
}];
}

- (id<FlutterKeyPrimaryResponder>)mockPrimaryResponder:(KeyCallbackSetter)callbackSetter
API_AVAILABLE(ios(13.4)) {
- (id<FlutterKeyPrimaryResponder>)mockPrimaryResponder:(KeyCallbackSetter)callbackSetter {
id<FlutterKeyPrimaryResponder> mock =
OCMStrictProtocolMock(@protocol(FlutterKeyPrimaryResponder));
OCMStub([mock handlePress:[OCMArg any] callback:[OCMArg any]])
.andDo((^(NSInvocation* invocation) {
FlutterUIPressProxy* press;
FlutterAsyncKeyCallback callback;
[invocation getArgument:&press atIndex:2];
[invocation getArgument:&callback atIndex:3];
__unsafe_unretained FlutterUIPressProxy* pressUnsafe;
__unsafe_unretained FlutterAsyncKeyCallback callbackUnsafe;

[invocation getArgument:&pressUnsafe atIndex:2];
[invocation getArgument:&callbackUnsafe atIndex:3];

// Retain the unretained parameters so they can
// be run in the perform block when this invocation goes out of scope.
FlutterUIPressProxy* press = pressUnsafe;
FlutterAsyncKeyCallback callback = callbackUnsafe;
CFRunLoopPerformBlock(CFRunLoopGetCurrent(),
fml::MessageLoopDarwin::kMessageLoopCFRunLoopMode, ^() {
callbackSetter(press, callback);
Expand All @@ -122,8 +61,7 @@ - (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {
return mock;
}

- (id<FlutterKeySecondaryResponder>)mockSecondaryResponder:(BoolGetter)resultGetter
API_AVAILABLE(ios(13.4)) {
- (id<FlutterKeySecondaryResponder>)mockSecondaryResponder:(BoolGetter)resultGetter {
id<FlutterKeySecondaryResponder> mock =
OCMStrictProtocolMock(@protocol(FlutterKeySecondaryResponder));
OCMStub([mock handlePress:[OCMArg any]]).andDo((^(NSInvocation* invocation) {
Expand All @@ -133,32 +71,27 @@ - (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {
return mock;
}

- (FlutterViewController*)mockOwnerWithPressesBeginOnlyNext API_AVAILABLE(ios(13.4)) {
- (void)testNextResponderShouldThrowOnPressesEnded {
// The nextResponder is a strict mock and hasn't stubbed pressesEnded.
// An error will be thrown on pressesEnded.
UIResponder* nextResponder = OCMStrictClassMock([UIResponder class]);
OCMStub([nextResponder pressesBegan:[OCMArg any] withEvent:[OCMArg any]]).andDo(nil);
OCMStub([nextResponder pressesBegan:OCMOCK_ANY withEvent:OCMOCK_ANY]);

FlutterViewController* viewController =
[[FlutterViewController alloc] initWithEngine:self.mockEngine nibName:nil bundle:nil];
id mockEngine = OCMClassMock([FlutterEngine class]);
FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:mockEngine
nibName:nil
bundle:nil];
FlutterViewController* owner = OCMPartialMock(viewController);
OCMStub([owner nextResponder]).andReturn(nextResponder);
return owner;
}

// Verify that the nextResponder returned from mockOwnerWithPressesBeginOnlyNext()
// throws exception when pressesEnded is called.
- (bool)testNextResponderShouldThrowOnPressesEnded API_AVAILABLE(ios(13.4)) {
FlutterViewController* owner = [self mockOwnerWithPressesBeginOnlyNext];
@try {
[owner.nextResponder pressesEnded:[NSSet init] withEvent:[[UIPressesEvent alloc] init]];
return false;
} @catch (...) {
return true;
}
XCTAssertThrowsSpecificNamed([owner.nextResponder pressesEnded:[[NSSet alloc] init]
withEvent:[[UIPressesEvent alloc] init]],
NSException, NSInternalInconsistencyException);

[mockEngine stopMocking];
}

- (void)testSinglePrimaryResponder API_AVAILABLE(ios(13.4)) {
- (void)testSinglePrimaryResponder {
FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];
__block BOOL primaryResponse = FALSE;
__block int callbackCount = 0;
Expand Down Expand Up @@ -190,7 +123,7 @@ - (void)testSinglePrimaryResponder API_AVAILABLE(ios(13.4)) {
XCTAssertFalse(completeHandled);
}

- (void)testDoublePrimaryResponder API_AVAILABLE(ios(13.4)) {
- (void)testDoublePrimaryResponder {
FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];

__block BOOL callback1Response = FALSE;
Expand Down Expand Up @@ -253,7 +186,7 @@ - (void)testDoublePrimaryResponder API_AVAILABLE(ios(13.4)) {
XCTAssertFalse(somethingWasHandled);
}

- (void)testSingleSecondaryResponder API_AVAILABLE(ios(13.4)) {
- (void)testSingleSecondaryResponder {
FlutterKeyboardManager* manager = [[FlutterKeyboardManager alloc] init];

__block BOOL primaryResponse = FALSE;
Expand Down Expand Up @@ -308,7 +241,7 @@ - (void)testSingleSecondaryResponder API_AVAILABLE(ios(13.4)) {
XCTAssertFalse(completeHandled);
}

- (void)testEventsProcessedSequentially API_AVAILABLE(ios(13.4)) {
- (void)testEventsProcessedSequentially {
constexpr UIKeyboardHIDUsage keyId1 = (UIKeyboardHIDUsage)0x50;
constexpr UIKeyboardHIDUsage keyId2 = (UIKeyboardHIDUsage)0x51;
FlutterUIPressProxy* event1 = keyDownEvent(keyId1);
Expand Down

0 comments on commit a504499

Please sign in to comment.