Migrate darwin common "framework_shared" target to ARC #37049

cyanglaz · 2022-10-26T19:02:14Z

The "framework_shared" target was built with arc, there is no blocker to move it to arc besides some standard mrc to arc code changes.

Code changes includes:

Make retain properties to be either "Strong" or "Copy" (I made properties to be copy when I think they should have been copy)
Make non-retain properties "Assign"
Remove all dealloc method, remove all retains call, remove all autorelease call
Make strong instance variables weak before being accessed in the block

This helps the iOS embedder migration work, after this lands, we can now freely move shared code from macOS to "framework_shared"

part of flutter/flutter#112232

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cyanglaz · 2022-10-26T20:28:37Z

shell/platform/darwin/common/framework/Headers/FlutterCodecs.h

@@ -230,7 +230,7 @@ FLUTTER_DARWIN_EXPORT
 /**
 * The method name.
 */
-@property(readonly, nonatomic) NSString* method;
+@property(readonly, nonatomic, copy) NSString* method;


Technically it is a breaking change.

Though I don't think making it a "copy" is going to change the way the public API is actually used unless it was abused where someone intentionally make it mutable?

Thoughts?

Since it's readonly, the setter is not synthesized, so it's not a breaking change here.

The breaking part I'm talking about is when someone assigns it to a MutableString, then changes the mutableString and this property will be changed.

So it was originally a bug. By "breaking". I meant if someone is abusing this bug to update the value of these properties, their code will stop working.

After another look, I realized I was mixing stuff in my head. I think @hellohuanlin was right. And in addition, since the setters are not synthesized, copy doesn't even make sense here. I will revert these changes.

hellohuanlin

overall LGTM

hellohuanlin · 2022-10-26T20:56:00Z

shell/platform/darwin/BUILD.gn

+    "//flutter/fml",
+    # "//flutter/runtime",
+    # "//flutter/shell/common",
+    # "//third_party/skia",


delete these?

oops, forgot to push.

hellohuanlin · 2022-10-26T20:58:44Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

@@ -108,7 +100,7 @@ - (void)setMessageHandler:(FlutterMessageHandler)handler {
    return;
  }
  // Grab reference to avoid retain on self.
-  NSObject<FlutterMessageCodec>* codec = _codec;
+  __weak NSObject<FlutterMessageCodec>* codec = _codec;
  FlutterBinaryMessageHandler messageHandler = ^(NSData* message, FlutterBinaryReply callback) {
    handler([codec decode:message], ^(id reply) {


maybe just early return if codec is nil? if codec is nil, should this handler be called?

I haven't looked into the implementation details. But just looking at this method without context, the behavior is different if we early return when codec is nil. The current behavior is that the handler is always called regardless of the codec's value. So I'd say we should keep it the same behavior and maybe update the code later if we think that's a bug.

Edit: we probably shouldn't even support a nil codec, maybe we can add an FML_DCHECK. But it is probably a good idea doing it in a separate PR.

Agreed there's not real-world case where a nil codec makes sense. Asserting it's not nil sgtm in a followup patch.

hellohuanlin · 2022-10-26T20:59:40Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

@@ -285,7 +255,7 @@ - (void)setMethodCallHandler:(FlutterMethodCallHandler)handler {
    return;
  }
  // Make sure the block captures the codec, not self.
-  NSObject<FlutterMethodCodec>* codec = _codec;
+  __weak NSObject<FlutterMethodCodec>* codec = _codec;


same q here

This is wrong, codec needs to be captured by the block to live longer than the channel object. _codec was actually nil in some scenarios if not captured strongly.

jmagman · 2022-10-26T21:19:47Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

-  _messenger = [messenger retain];
-  _codec = [codec retain];
-  _taskQueue = [taskQueue retain];
+  _name = name;


Better to copy mutable objects like NSString so if they are really NSMutableString they don't get changed from under you.

Suggested change

_name = name;

_name = [name copy];

Also all the FlutterBasicMessageChannel ivars should be properties... but that doesn't need to be done here.

jmagman · 2022-10-26T21:23:26Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

+  _code = code;
+  _message = message;
+  _details = details;


And these properties are copy but you only get that on the property setter, not when you set the synthesized ivar.

Suggested change

_code = code;

_message = message;

_details = details;

_code = [code copy];

_message = [message copy];

_details = [details copy];

jmagman · 2022-10-26T21:24:42Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

 }

 - (instancetype)initWithMethodName:(NSString*)method arguments:(id)arguments {
  NSAssert(method, @"Method name cannot be nil");
  self = [super init];
  NSAssert(self, @"Super init cannot be nil");
-  _method = [method retain];
-  _arguments = [arguments retain];
+  _method = method;


Suggested change

_method = method;

_method = [method copy];

jmagman · 2022-10-26T21:25:43Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

-  _messenger = [messenger retain];
-  _codec = [codec retain];
-  _taskQueue = [taskQueue retain];
+  _name = name;


jmagman · 2022-10-26T21:26:14Z

shell/platform/darwin/common/framework/Source/FlutterStandardCodec.mm

@@ -182,18 +170,13 @@ - (instancetype)initWithData:(NSData*)data type:(FlutterStandardDataType)type {
  NSAssert(data.length % elementSize == 0, @"Data must contain integral number of elements");
  self = [super init];
  NSAssert(self, @"Super init cannot be nil");
-  _data = [data retain];
+  _data = data;


Same idea with NSMutableData

Suggested change

_data = data;

_data = [data copy];

jmagman · 2022-10-26T21:26:43Z

shell/platform/darwin/common/BUILD.gn

-    "//third_party/dart/runtime:dart_api",
-    "//third_party/skia",
-  ]
+  deps = [ "//flutter/fml" ]


The other deps aren't necessary?

Correct. They were probably initially necessary but was forgotten to be removed at some point when refactoring the code.

I wonder if some of these were copy-pasted from the macOS embedder initially? I can't imagine why we'd depend on flow, for example, given the current code. Either way, nice catch!

They were added 6 years ago so I guess the code structure was a lot different.

flutter-dashboard · 2022-10-26T22:07:59Z

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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

Hixie · 2022-10-26T22:15:04Z

test-exemption: code refactor with no semantic change

jmagman

I see FLUTTER_ASSERT_ARC in a few places, but not many. Should that be added to the .mm files so it's not accidentally built in the future with MRC?

jmagman

LGTM, nit about FLUTTER_ASSERT_ARC.

cyanglaz · 2022-10-27T16:55:28Z

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

@@ -80,7 +80,7 @@ - (void)testRestorationEnabledWaitsForData {
  [restorationPlugin setRestorationData:data];
  XCTAssertEqual([capturedResult count], 2u);
  XCTAssertEqual([capturedResult objectForKey:@"enabled"], @YES);
-  XCTAssertEqual([[capturedResult objectForKey:@"data"] data], data);
+  XCTAssertEqualObjects([[capturedResult objectForKey:@"data"] data], data);


@goderbauer Could you take a look at the changes I made in this test file to see if they are ok? Did you remember if XCTAssertEqual was intentionally used to compare the address of the 2 data objects, or we just need the content of the data objects to be the same.

Original PR: #23495

We just need the content of the data objects to be the same.

…tter/engine#37049)

…er#37049)" This reverts commit 866b670.

…tter/engine#37049)

…" (#37121)

…tter/engine#37049)

…er#37049)" This reverts commit 866b670.

…ARC (#37049)" (#37195) * Revert "Use iOS 16 APIs to rotate orientation (#36874)" (#37165) * Revert "Migrate darwin common "framework_shared" target to ARC (#37049)" This reverts commit 866b670.

flutter#37049)" (flutter#37121)" This reverts commit 774cb34.

cyanglaz · 2022-11-01T19:29:07Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

@@ -108,7 +102,7 @@ - (void)setMessageHandler:(FlutterMessageHandler)handler {
    return;
  }
  // Grab reference to avoid retain on self.
-  NSObject<FlutterMessageCodec>* codec = _codec;
+  __weak NSObject<FlutterMessageCodec>* codec = _codec;


This is wrong, codec needs to be captured by the block to live longer than the channel object.

#37219)

…#37049" (#37219)" This reverts commit edb0492.

…#37049" (#37219)" (#37320) This reverts commit edb0492.

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2.

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2. remove copy format

…r#37049" (flutter#37219)

…flutter#37049" (flutter#37219)" (flutter#37320) This reverts commit edb0492.

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2.

…et to ARC #37049" (#37219)" (#37320)" (#37883) This reverts commit cf22fc2.

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" (flutter#37883) This reverts commit cf22fc2.

flutter-dashboard bot added platform-ios platform-macos labels Oct 26, 2022

fix

425aea4

cyanglaz force-pushed the darwin_common_arc branch from 0848450 to 425aea4 Compare October 26, 2022 19:04

update darwin/BUILD.gn

3f5368c

cyanglaz marked this pull request as ready for review October 26, 2022 20:14

cyanglaz requested a review from jmagman October 26, 2022 20:14

cyanglaz commented Oct 26, 2022

View reviewed changes

remove unused deps

a92065a

cyanglaz requested a review from iskakaushik October 26, 2022 20:58

hellohuanlin approved these changes Oct 26, 2022

View reviewed changes

Chris Yang added 2 commits October 26, 2022 14:05

more unnecessary deps

1265796

gn format

3f8c2fb

jmagman reviewed Oct 26, 2022

View reviewed changes

Chris Yang added 2 commits October 26, 2022 14:45

copy

7b6a5a4

copy

6157ddf

cyanglaz requested a review from jmagman October 26, 2022 21:47

flutter-dashboard bot added the needs tests label Oct 26, 2022

NSData also made to copy

35559e1

jmagman reviewed Oct 26, 2022

View reviewed changes

cbracken approved these changes Oct 26, 2022

View reviewed changes

Chris Yang added 2 commits October 26, 2022 16:33

revert copy

ea06e4e

revert details copy

1fde705

jmagman approved these changes Oct 26, 2022

View reviewed changes

use equal objects to compare data

700da71

cyanglaz commented Oct 27, 2022

View reviewed changes

cyanglaz requested a review from goderbauer October 27, 2022 16:55

assert arc

0dd00d5

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 27, 2022

auto-submit bot merged commit 866b670 into flutter:main Oct 27, 2022

cyanglaz deleted the darwin_common_arc branch October 27, 2022 20:34

engine-flutter-autoroll mentioned this pull request Oct 27, 2022

Roll Flutter Engine from d90e5b7372a3 to b5c52563bbb6 (7 revisions) flutter/flutter#114198

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 27, 2022

866b670c1 Migrate darwin common "framework_shared" target to ARC (flu…

c2c471d

…tter/engine#37049)

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Oct 28, 2022

Revert "Migrate darwin common "framework_shared" target to ARC (flutt…

2aa5b71

…er#37049)" This reverts commit 866b670.

cyanglaz mentioned this pull request Oct 28, 2022

Revert "Migrate darwin common "framework_shared" target to ARC (#37049)" #37121

Merged

8 tasks

engine-flutter-autoroll mentioned this pull request Oct 28, 2022

Roll Flutter Engine from d90e5b7372a3 to 0e52894f30f7 (21 revisions) flutter/flutter#114248

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2022

866b670c1 Migrate darwin common "framework_shared" target to ARC (flu…

2adb745

…tter/engine#37049)

auto-submit bot pushed a commit that referenced this pull request Oct 28, 2022

Revert "Migrate darwin common "framework_shared" target to ARC (#37049)…

774cb34

…" (#37121)

engine-flutter-autoroll mentioned this pull request Oct 28, 2022

Roll Flutter Engine from d90e5b7372a3 to 774cb34d3ff5 (27 revisions) flutter/flutter#114258

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2022

866b670c1 Migrate darwin common "framework_shared" target to ARC (flu…

feb9531

…tter/engine#37049)

CaseyHillers pushed a commit to CaseyHillers/engine that referenced this pull request Nov 1, 2022

Revert "Migrate darwin common "framework_shared" target to ARC (flutt…

6e6358b

…er#37049)" This reverts commit 866b670.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Nov 1, 2022

Revert "Revert "Migrate darwin common "framework_shared" target to ARC (

6280c40

flutter#37049)" (flutter#37121)" This reverts commit 774cb34.

cyanglaz mentioned this pull request Nov 1, 2022

reland "Migrate darwin common "framework_shared" target to ARC #37049" #37219

Merged

8 tasks

cyanglaz commented Nov 1, 2022

View reviewed changes

auto-submit bot pushed a commit that referenced this pull request Nov 2, 2022

reland "Migrate darwin common "framework_shared" target to ARC #37049" (

edb0492

#37219)

cyanglaz pushed a commit that referenced this pull request Nov 4, 2022

Revert "reland "Migrate darwin common "framework_shared" target to ARC …

56588e9

…#37049" (#37219)" This reverts commit edb0492.

auto-submit bot pushed a commit that referenced this pull request Nov 4, 2022

Revert "reland "Migrate darwin common "framework_shared" target to ARC …

cf22fc2

…#37049" (#37219)" (#37320) This reverts commit edb0492.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Nov 9, 2022

Revert "Revert "reland "Migrate darwin common "framework_shared" targ…

d0008a2

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Nov 14, 2022

Revert "Revert "reland "Migrate darwin common "framework_shared" targ…

3784633

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2. remove copy format

schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022

reland "Migrate darwin common "framework_shared" target to ARC flutte…

b1cafc2

…r#37049" (flutter#37219)

schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022

Revert "reland "Migrate darwin common "framework_shared" target to ARC …

f4e7446

…flutter#37049" (flutter#37219)" (flutter#37320) This reverts commit edb0492.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Nov 23, 2022

Revert "Revert "reland "Migrate darwin common "framework_shared" targ…

f66ba5e

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Nov 29, 2022

Revert "Revert "reland "Migrate darwin common "framework_shared" targ…

fff6d04

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Dec 13, 2022

Revert "Revert "reland "Migrate darwin common "framework_shared" targ…

5200542

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" This reverts commit cf22fc2.

auto-submit bot pushed a commit that referenced this pull request Dec 15, 2022

Revert "Revert "reland "Migrate darwin common "framework_shared" targ…

4881fe2

…et to ARC #37049" (#37219)" (#37320)" (#37883) This reverts commit cf22fc2.

loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Jan 3, 2023

Revert "Revert "reland "Migrate darwin common "framework_shared" targ…

06221b2

…et to ARC flutter#37049" (flutter#37219)" (flutter#37320)" (flutter#37883) This reverts commit cf22fc2.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate darwin common "framework_shared" target to ARC #37049

Migrate darwin common "framework_shared" target to ARC #37049

cyanglaz commented Oct 26, 2022 •

edited

Loading

cyanglaz Oct 26, 2022 •

edited

Loading

hellohuanlin Oct 26, 2022

cyanglaz Oct 26, 2022 •

edited

Loading

cyanglaz Oct 26, 2022 •

edited

Loading

hellohuanlin left a comment

hellohuanlin Oct 26, 2022

cyanglaz Oct 26, 2022

hellohuanlin Oct 26, 2022

cyanglaz Oct 26, 2022 •

edited

Loading

cbracken Oct 26, 2022

hellohuanlin Oct 26, 2022

cyanglaz Nov 1, 2022

jmagman Oct 26, 2022

jmagman Oct 26, 2022

jmagman Oct 26, 2022

jmagman Oct 26, 2022

jmagman Oct 26, 2022

jmagman Oct 26, 2022

cyanglaz Oct 26, 2022

cbracken Oct 26, 2022

cyanglaz Oct 26, 2022

flutter-dashboard bot commented Oct 26, 2022

Hixie commented Oct 26, 2022

jmagman left a comment

jmagman left a comment

cyanglaz Oct 27, 2022

goderbauer Oct 27, 2022

cyanglaz Nov 1, 2022

Migrate darwin common "framework_shared" target to ARC #37049

Migrate darwin common "framework_shared" target to ARC #37049

Conversation

cyanglaz commented Oct 26, 2022 • edited Loading

Pre-launch Checklist

cyanglaz Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

cyanglaz Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

hellohuanlin left a 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

cyanglaz Oct 26, 2022 • 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

flutter-dashboard bot commented Oct 26, 2022

Hixie commented Oct 26, 2022

jmagman left a comment

Choose a reason for hiding this comment

jmagman left a 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

cyanglaz commented Oct 26, 2022 •

edited

Loading

cyanglaz Oct 26, 2022 •

edited

Loading

cyanglaz Oct 26, 2022 •

edited

Loading

cyanglaz Oct 26, 2022 •

edited

Loading

cyanglaz Oct 26, 2022 •

edited

Loading