Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

jmagman · 2024-04-09T05:39:23Z

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

MigrateFlutterCallbackCache 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.

flutter-dashboard · 2024-04-09T05:39:25Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

jmagman · 2024-04-09T05:41:02Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.mm

@@ -89,7 +76,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;


Removed the WeakNSObjectFactory in favor of __weak where it's used before the block. I can swap __typeof(self) to FlutterKeyboardManager* I don't really have a preference.

jmagman · 2024-04-11T00:24:43Z

shell/platform/darwin/common/framework/Source/FlutterChannelsTest.m

@@ -168,7 +168,7 @@ - (void)testResize {
  [binaryMessenger stopMocking];
 }

- (bool)testSetWarnsOnOverflow {
+- (void)testSetWarnsOnOverflow {


Unrelated, but noticed this wasn't running because the signature wasn't void.

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

jmagman · 2024-04-11T00:28:18Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

-}  // namespace
-
+// These tests were designed to run on iOS 13.4 or later.
+API_AVAILABLE(ios(13.4))


Putting the availability on the class FlutterKeyboardManagerTest means it won't need to be checked in setUp or decorated on the methods.

jmagman · 2024-04-11T00:28:54Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

-@property(nonatomic, assign) BOOL didCallNotifyLowMemory;
-@end
-
-@interface FlutterEngine ()


jmagman · 2024-04-11T00:28:59Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

 using namespace flutter::testing;

-/// Sometimes we have to use a custom mock to avoid retain cycles in ocmock.
-@interface FlutterEnginePartialMock : FlutterEngine


jmagman · 2024-04-11T00:29:46Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

-  [super tearDown];
-}
-
- (id)checkKeyDownEvent:(UIKeyboardHIDUsage)keyCode API_AVAILABLE(ios(13.4)) {


jmagman · 2024-04-11T00:30:04Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

 @interface FlutterKeyboardManagerTest : XCTestCase
-@property(nonatomic, strong) id mockEngine;


Moved this into a local in the one remaining spot the engine is mocked.

jmagman · 2024-04-11T00:30:37Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm


-// Verify that the nextResponder returned from mockOwnerWithPressesBeginOnlyNext()
-// throws exception when pressesEnded is called.
- (bool)testNextResponderShouldThrowOnPressesEnded API_AVAILABLE(ios(13.4)) {


This wasn't actually running because the return type wasn't void.

Maybe we need a custom CI check on this? It seems like anything in Test.{m,mm} that starts - (anything but void)test is probably wrong.

Filed flutter/flutter#146671

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

stuartmorgan · 2024-04-11T14:27:49Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManager.mm

-  // It will be destroyed and invalidate its weak pointers
-  // before any other members are destroyed.
-  _weakFactory.reset();
-
  [_primaryResponders removeAllObjects];
  [_secondaryResponders removeAllObjects];


Why would we need to remove everything from private properties in dealloc? Is there some way we are vending outside references to these arrays?

Let me check.

I don't think it would be necessary, this is the only place -addPrimaryResponder and -addSecondaryResponder are called:

engine/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

Lines 823 to 831 in 76a270f

[self.keyboardManager addPrimaryResponder:[[[FlutterEmbedderKeyResponder alloc]

initWithSendEvent:sendEvent] autorelease]];

FlutterChannelKeyResponder* responder = [[[FlutterChannelKeyResponder alloc]

initWithChannel:self.engine.keyEventChannel] autorelease];

[self.keyboardManager addPrimaryResponder:responder];

FlutterTextInputPlugin* textInputPlugin = self.engine.textInputPlugin;

if (textInputPlugin != nil) {

[self.keyboardManager addSecondaryResponder:textInputPlugin];

}

I'll remove.

stuartmorgan · 2024-04-11T14:30:05Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm

-        FlutterUIPressProxy* press;
-        FlutterAsyncKeyCallback callback;
+        __unsafe_unretained FlutterUIPressProxy* press;
+        __unsafe_unretained FlutterAsyncKeyCallback callback;


This seems scary. Why are we passing unsafe unretained pointers to an async block? It seems like this code was probably just wrong before?

I think it's needed for ARC NSInvocation calls, that's the only place I've regularly seen __unsafe_unretained used

https://developer.apple.com/documentation/foundation/nsinvocation/1437830-getargument#discussion

__unsafe_unretained id tempObject = nil; [myInvocation getArgument: &tempObject atIndex: 2]; self.myObject = tempObject; NSLog(@"Argument 0 was: %@", self.myObject); // prints "A string"

Given that the next thing we do is pass the arguments into an async block, I think we are missing a critical intermediate step:

You can then copy this into another form of storage, like a strongly-held property.

I know this is pre-existing, but it seems like a very likely source of flaky test crashing, so we should fix it while we are here. We can rename these something like pressUnsafe and callbackUnsafe and then do

FlutterUIPressProxy* press = pressUnsafe; FlutterAsyncKeyCallback callback = callbackUnsafe;

That way we won't be trying to run an async block with pointers into memory owned by an invocation that's almost certainly out of scope. I'm pretty surprised that the current code is ever working.

stuartmorgan · 2024-04-11T14:31:17Z

shell/platform/darwin/ios/framework/Source/FlutterKeyboardManagerTest.mm


-// Verify that the nextResponder returned from mockOwnerWithPressesBeginOnlyNext()
-// throws exception when pressesEnded is called.
- (bool)testNextResponderShouldThrowOnPressesEnded API_AVAILABLE(ios(13.4)) {


Maybe we need a custom CI check on this? It seems like anything in Test.{m,mm} that starts - (anything but void)test is probably wrong.

stuartmorgan

LGTM. Thanks for the fixes!

auto-submit · 2024-04-12T20:10:48Z

auto label is removed for flutter/engine/51983, due to Pull request flutter/engine/51983 is not in a mergeable state.

cbracken

Nice! Also nice catch on the dead code. Wonder how much else we have floating around :/

…ARC (flutter/engine#51983)

…146776) flutter/engine@84238c4...07ae93c 2024-04-15 [email protected] Roll Dart SDK from ac31be3c8546 to 3c2376cb9850 (1 revision) (flutter/engine#52095) 2024-04-15 [email protected] Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC (flutter/engine#51983) 2024-04-15 [email protected] Roll buildroot and set ios_use_simulator variable used by Skia GN scripts (flutter/engine#52101) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#146776) flutter/engine@84238c4...07ae93c 2024-04-15 [email protected] Roll Dart SDK from ac31be3c8546 to 3c2376cb9850 (1 revision) (flutter/engine#52095) 2024-04-15 [email protected] Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC (flutter/engine#51983) 2024-04-15 [email protected] Roll buildroot and set ios_use_simulator variable used by Skia GN scripts (flutter/engine#52101) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jmagman self-assigned this Apr 9, 2024

jmagman marked this pull request as draft April 9, 2024 05:39

github-actions bot added the platform-ios label Apr 9, 2024

jmagman commented Apr 9, 2024

View reviewed changes

jmagman force-pushed the cache-keyboard-arc branch from ed4e0e3 to a1a9cde Compare April 9, 2024 06:23

github-actions bot added affects: desktop platform-macos labels Apr 11, 2024

jmagman force-pushed the cache-keyboard-arc branch from de33c74 to a0a0ebf Compare April 11, 2024 00:32

jmagman commented Apr 11, 2024

View reviewed changes

jmagman force-pushed the cache-keyboard-arc branch 2 times, most recently from 46815b3 to 0c8d245 Compare April 11, 2024 03:17

jmagman marked this pull request as ready for review April 11, 2024 03:53

jmagman requested review from stuartmorgan and cbracken April 11, 2024 03:54

stuartmorgan reviewed Apr 11, 2024

View reviewed changes

jmagman force-pushed the cache-keyboard-arc branch from f79c459 to 92daaec Compare April 12, 2024 04:41

jmagman mentioned this pull request Apr 12, 2024

Add tooling to prevent iOS XCTests tests that do not match the pattern -(void)test* flutter/flutter#146671

Open

jmagman requested a review from hellohuanlin April 12, 2024 04:47

stuartmorgan approved these changes Apr 12, 2024

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 12, 2024

hellohuanlin approved these changes Apr 12, 2024

View reviewed changes

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 12, 2024

jmagman added 6 commits April 12, 2024 13:22

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC

cfae8b6

Rewrite keyboard manager tests to use expectations

bbb9620

verify

41c6cfe

Revert the runloop parts of the keyboard manager tests

e431d11

strong

2f6a591

__unsafe_unretained

25bfa11

jmagman force-pushed the cache-keyboard-arc branch from 92daaec to 25bfa11 Compare April 12, 2024 20:24

cbracken approved these changes Apr 12, 2024

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 13, 2024

auto-submit bot merged commit a504499 into flutter:main Apr 15, 2024
28 checks passed

engine-flutter-autoroll mentioned this pull request Apr 15, 2024

Roll Flutter Engine from 84238c411001 to 07ae93c9b755 (3 revisions) flutter/flutter#146776

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 15, 2024

a5044997b Migrate FlutterCallbackCache and FlutterKeyboardManager to …

a5e083c

…ARC (flutter/engine#51983)

jmagman deleted the cache-keyboard-arc branch April 15, 2024 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

jmagman commented Apr 9, 2024

flutter-dashboard bot commented Apr 9, 2024

jmagman Apr 9, 2024

jmagman Apr 11, 2024

jmagman Apr 11, 2024 •

edited

Loading

jmagman Apr 11, 2024

jmagman Apr 11, 2024

jmagman Apr 11, 2024

jmagman Apr 11, 2024

jmagman Apr 11, 2024

stuartmorgan Apr 11, 2024

jmagman Apr 12, 2024

stuartmorgan Apr 11, 2024

jmagman Apr 11, 2024

jmagman Apr 12, 2024

stuartmorgan Apr 11, 2024

jmagman Apr 11, 2024

stuartmorgan Apr 11, 2024

stuartmorgan Apr 11, 2024

stuartmorgan left a comment

auto-submit bot commented Apr 12, 2024

cbracken left a comment

		@interface FlutterKeyboardManagerTest : XCTestCase
		@property(nonatomic, strong) id mockEngine;

	[self.keyboardManager addPrimaryResponder:[[[FlutterEmbedderKeyResponder alloc]
	initWithSendEvent:sendEvent] autorelease]];
	FlutterChannelKeyResponder* responder = [[[FlutterChannelKeyResponder alloc]
	initWithChannel:self.engine.keyEventChannel] autorelease];
	[self.keyboardManager addPrimaryResponder:responder];
	FlutterTextInputPlugin* textInputPlugin = self.engine.textInputPlugin;
	if (textInputPlugin != nil) {
	[self.keyboardManager addSecondaryResponder:textInputPlugin];
	}

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC #51983

Conversation

jmagman commented Apr 9, 2024

flutter-dashboard bot commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

auto-submit bot commented Apr 12, 2024

cbracken left a comment

Choose a reason for hiding this comment

jmagman Apr 11, 2024 •

edited

Loading