Migrate FlutterEmbedderKeyResponder to ARC #52048

jmagman · 2024-04-11T04:59:23Z

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

Migrate FlutterEmbedderKeyResponder from MRC to ARC. Clean up some header imports which made this seem like it depended on not-yet-migrated MRC files.

Part of flutter/flutter#137801.

jmagman · 2024-04-11T05:00:00Z

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

@@ -313,17 +316,12 @@ @implementation FlutterKeyCallbackGuard {
 - (nonnull instancetype)initWithCallback:(FlutterAsyncKeyCallback)callback {
  self = [super init];
  if (self != nil) {
-    _callback = [callback copy];
+    _callback = callback;


I can't think of why these blocks would need to be copied instead of retained.

Could be old enough that it was written when you had to explicitly copy a block to make it an object that could be stored.

Yes, i remember in the old days blocks need to be copied, since it's created on the stack for performance reason and needs to be copied to the heap. In ARC this is automatic

jmagman · 2024-04-11T05:00:17Z

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

@@ -492,7 +490,7 @@ @implementation FlutterEmbedderKeyResponder
 - (nonnull instancetype)initWithSendEvent:(FlutterSendKeyEvent)sendEvent {
  self = [super init];
  if (self != nil) {
-    _sendEvent = [sendEvent copy];
+    _sendEvent = sendEvent;


Same, a block that was copied.

jmagman · 2024-04-11T05:09:57Z

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

  [callback pendTo:_pendingResponses withId:responseId];
-  _sendEvent(event, HandleResponse, pending);
+  _sendEvent(event, HandleResponse, (__bridge_retained void* _Nullable)pending);


We had tests that crashed when this was just __bridge (reminder about what this does: https://medium.com/@hadhi631/when-to-use-bridge-bridge-transfer-cfbridgingrelease-and-bridge-retained-cfbridgingretain-4b3d2fc932df)

jmagman · 2024-04-11T05:10:11Z

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

@@ -883,7 +874,7 @@ - (UInt32)adjustModifiers:(nonnull FlutterUIPressProxy*)press API_AVAILABLE(ios(

 namespace {
 void HandleResponse(bool handled, void* user_data) {
-  FlutterKeyPendingResponse* pending = reinterpret_cast<FlutterKeyPendingResponse*>(user_data);
+  FlutterKeyPendingResponse* pending = (__bridge_transfer FlutterKeyPendingResponse*)user_data;


Same, tests crashed with just a __bridge.

What crash did you get? Does the owner of user_data release it too?

Oh i saw the caller uses autorelease to keep it alive for current runloop. So in ARC it makes sense to bridge retain, and bridge release afterwards.

The crash was an overrelease, it was zombied.

ya what you did here should be fine. In the new code, the caller site doesn't release it, so won't be over-released.

stuartmorgan-g

LGTM

stuartmorgan-g · 2024-04-11T18:15:48Z

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

@@ -313,17 +316,12 @@ @implementation FlutterKeyCallbackGuard {
 - (nonnull instancetype)initWithCallback:(FlutterAsyncKeyCallback)callback {
  self = [super init];
  if (self != nil) {
-    _callback = [callback copy];
+    _callback = callback;


Could be old enough that it was written when you had to explicitly copy a block to make it an object that could be stored.

stuartmorgan-g · 2024-04-11T18:16:56Z

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


 /**
 * A map of pressed keys.
 *
 * The keys of the dictionary are physical keys, while the values are the logical keys
 * of the key down event.
 */
-@property(nonatomic, retain, readonly) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords;
+@property(nonatomic, readonly) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords;


These make me a bit nervous, but I guess they are internal-only so hopefully not copying is okay.

nit: might as well just be explicit here. Can either keep this retain or use strong instead.

The style guide isn't explicit about this, but all the examples leave off strong since it's the default:

For example

/** The retained Bar. */ @property(nonatomic) Bar *bar; /** The current drawing attributes. */ @property(nonatomic, copy) NSDictionary<NSString *, NSNumber *> *attributes;

@stuartmorgan would you like me to make this a copy and see if anything Bad happens (probably nothing).

If you're willing to test or audit that it's safe, I would feel better with it as a copy. But it's not changing behavior and this isn't super dangerous like the unretained thing, so if you would prefer to make this behavior-neutral I'm fine with that as well.

/** The retained Bar. */
@Property(nonatomic) Bar *bar;

looks like it's just me being used to always writing strong 🤷

hellohuanlin · 2024-04-11T18:49:04Z

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

@@ -356,15 +354,15 @@ @interface FlutterEmbedderKeyResponder ()
 *
 * Set by the initializer.
 */
-@property(nonatomic, copy, readonly) FlutterSendKeyEvent sendEvent;
+@property(nonatomic, readonly) FlutterSendKeyEvent sendEvent;


nit: iirc Apple still recommend using copy for blocks in ARC, despite that retain/strong will behave the same for blocks. It's more of a hint to the reader.

From https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/WorkingwithBlocks/WorkingwithBlocks.html

Note: You should specify copy as the property attribute, because a block needs to be copied to keep track of its captured state outside of the original scope. This isn’t something you need to worry about when using Automatic Reference Counting, as it will happen automatically, but it’s best practice for the property attribute to show the resultant behavior. For more information, see Blocks Programming Topics.

Ah, didn't know about that "it’s best practice for the property attribute to show the resultant behavior" recommendation. I'll change it back I guess...

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#blocks

…52048)

…146703) flutter/engine@0e56e3d...1a13c7d 2024-04-12 [email protected] Roll Skia from 5101cbe5a6bb to 761f6f7f6250 (1 revision) (flutter/engine#52076) 2024-04-12 [email protected] Migrate FlutterEmbedderKeyResponder to ARC (flutter/engine#52048) 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

XilaiZhang · 2024-04-14T18:08:47Z

cc @Jasguerrero do you happen to know why webhook didn't trigger Google Testing to run on this PR?

Jasguerrero · 2024-04-15T20:35:11Z

cc @Jasguerrero do you happen to know why webhook didn't trigger Google Testing to run on this PR?

@XilaiZhang
Looking at the logs I see that Google Testing did run (since it was 5 days ago it has been deleted by now). Do you mean why the github check did not show for the PR? We do not surface that check for the engine repo

…lutter#146703) flutter/engine@0e56e3d...1a13c7d 2024-04-12 [email protected] Roll Skia from 5101cbe5a6bb to 761f6f7f6250 (1 revision) (flutter/engine#52076) 2024-04-12 [email protected] Migrate FlutterEmbedderKeyResponder to ARC (flutter/engine#52048) 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 11, 2024

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

jmagman commented Apr 11, 2024

View reviewed changes

jmagman force-pushed the FlutterEmbedderKeyResponder-arc branch from 7e74123 to 0bf160a Compare April 11, 2024 05:08

jmagman commented Apr 11, 2024

View reviewed changes

jmagman marked this pull request as ready for review April 11, 2024 15:51

jmagman requested review from stuartmorgan-g and hellohuanlin April 11, 2024 18:08

stuartmorgan-g approved these changes Apr 11, 2024

View reviewed changes

hellohuanlin reviewed Apr 11, 2024

View reviewed changes

jmagman added 2 commits April 11, 2024 21:13

Migrate FlutterEmbedderKeyResponder to ARC

cfbd14e

copies

47254ff

jmagman force-pushed the FlutterEmbedderKeyResponder-arc branch from 0bf160a to 47254ff Compare April 12, 2024 04:14

hellohuanlin 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

auto-submit bot merged commit 6bd43ce into flutter:main Apr 12, 2024
29 checks passed

jmagman deleted the FlutterEmbedderKeyResponder-arc branch April 12, 2024 20:18

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

Roll Flutter Engine from 0e56e3dffe43 to 1a13c7d1f40e (2 revisions) flutter/flutter#146703

Merged

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

6bd43ce03 Migrate FlutterEmbedderKeyResponder to ARC (flutter/engine#…

070ef1e

…52048)

Migrate FlutterEmbedderKeyResponder to ARC #52048

Migrate FlutterEmbedderKeyResponder to ARC #52048

Uh oh!

Conversation

jmagman commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

XilaiZhang commented Apr 14, 2024

Uh oh!

Jasguerrero commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jmagman commented Apr 11, 2024 •

edited

Loading

hellohuanlin Apr 11, 2024 •

edited

Loading

hellohuanlin Apr 11, 2024 •

edited

Loading

hellohuanlin Apr 11, 2024 •

edited

Loading

Jasguerrero commented Apr 15, 2024 •

edited

Loading