Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Migrate darwin common "framework_shared" target to ARC #37049

Merged
auto-submit merged 14 commits into from
Oct 27, 2022
Merged

Migrate darwin common "framework_shared" target to ARC #37049

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
425aea4
fix
Oct 26, 2022
3f5368c
update darwin/BUILD.gn
Oct 26, 2022
a92065a
remove unused deps
Oct 26, 2022
1265796
more unnecessary deps
Oct 26, 2022
3f8c2fb
gn format
Oct 26, 2022
7b6a5a4
copy
Oct 26, 2022
6157ddf
copy
Oct 26, 2022
35559e1
NSData also made to copy
Oct 26, 2022
ea06e4e
revert copy
Oct 26, 2022
1fde705
revert details copy
Oct 26, 2022
700da71
use equal objects to compare data
Oct 27, 2022
0dd00d5
assert arc
Oct 27, 2022
12a7fd7
fix wrong assert_arc
Oct 27, 2022
0a5aec4
fix one more equal objects
Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions shell/platform/darwin/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,20 @@ source_set("flutter_channels") {
sources = [
"common/buffer_conversions.h",
"common/buffer_conversions.mm",
]

deps = [ "//flutter/fml" ]

public_deps = [ ":flutter_channels_arc" ]

public_configs = [ "//flutter:config" ]
}

source_set("flutter_channels_arc") {
cflags_objc = flutter_cflags_objc_arc
cflags_objcc = flutter_cflags_objcc_arc

sources = [
"common/framework/Headers/FlutterBinaryMessenger.h",
"common/framework/Headers/FlutterChannels.h",
"common/framework/Headers/FlutterCodecs.h",
Expand All @@ -36,13 +50,12 @@ source_set("flutter_channels") {
"common/framework/Source/FlutterStandardCodec_Internal.h",
]

deps = [
"//flutter/common",
"//flutter/flow",
"//flutter/fml",
"//flutter/runtime",
"//flutter/shell/common",
"//third_party/skia",
public = [
"common/framework/Headers/FlutterBinaryMessenger.h",
"common/framework/Headers/FlutterChannels.h",
"common/framework/Headers/FlutterCodecs.h",
"common/framework/Headers/FlutterMacros.h",
"common/framework/Source/FlutterStandardCodec_Internal.h",
]

public_configs = [ "//flutter:config" ]
Expand Down
14 changes: 3 additions & 11 deletions shell/platform/darwin/common/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,7 @@ source_set("common") {
"command_line.mm",
]

deps = [
"//flutter/common",
"//flutter/flow",
"//flutter/fml",
"//flutter/runtime",
"//flutter/shell/common",
"//third_party/dart/runtime:dart_api",
"//third_party/skia",
]
deps = [ "//flutter/fml" ]
Copy link
Member

@jmagman jmagman Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other deps aren't necessary?

Copy link
Contributor Author

@cyanglaz cyanglaz Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@cbracken cbracken Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@cyanglaz cyanglaz Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public_configs = [ "//flutter:config" ]
}
Expand All @@ -38,8 +30,8 @@ config("framework_relative_headers") {

# Framework code shared between iOS and macOS.
source_set("framework_shared") {
cflags_objc = flutter_cflags_objc
cflags_objcc = flutter_cflags_objcc
cflags_objc = flutter_cflags_objc_arc
cflags_objcc = flutter_cflags_objcc_arc

sources = [
"framework/Source/FlutterChannels.mm",
Expand Down
12 changes: 6 additions & 6 deletions shell/platform/darwin/common/framework/Headers/FlutterCodecs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ FLUTTER_DARWIN_EXPORT
/**
* The method name.
*/
@property(readonly, nonatomic) NSString* method;
@property(readonly, nonatomic, copy) NSString* method;
Copy link
Contributor Author

@cyanglaz cyanglaz Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@hellohuanlin hellohuanlin Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cyanglaz cyanglaz Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@cyanglaz cyanglaz Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* The arguments.
Expand All @@ -257,12 +257,12 @@ FLUTTER_DARWIN_EXPORT
/**
The error code.
*/
@property(readonly, nonatomic) NSString* code;
@property(readonly, nonatomic, copy) NSString* code;

/**
The error message.
*/
@property(readonly, nonatomic, nullable) NSString* message;
@property(readonly, nonatomic, nullable, copy) NSString* message;

/**
The error details.
Expand Down Expand Up @@ -344,17 +344,17 @@ FLUTTER_DARWIN_EXPORT
/**
* The type of the encoded values.
*/
@property(readonly, nonatomic) FlutterStandardDataType type;
@property(readonly, nonatomic, assign) FlutterStandardDataType type;

/**
* The number of value items encoded.
*/
@property(readonly, nonatomic) UInt32 elementCount;
@property(readonly, nonatomic, assign) UInt32 elementCount;

/**
* The number of bytes used by the encoding of a single value item.
*/
@property(readonly, nonatomic) UInt8 elementSize;
@property(readonly, nonatomic, assign) UInt8 elementSize;
@end

/**
Expand Down
91 changes: 26 additions & 65 deletions shell/platform/darwin/common/framework/Source/FlutterChannels.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ + (instancetype)messageChannelWithName:(NSString*)name
+ (instancetype)messageChannelWithName:(NSString*)name
binaryMessenger:(NSObject<FlutterBinaryMessenger>*)messenger
codec:(NSObject<FlutterMessageCodec>*)codec {
return [[[FlutterBasicMessageChannel alloc] initWithName:name
binaryMessenger:messenger
codec:codec] autorelease];
return [[FlutterBasicMessageChannel alloc] initWithName:name
binaryMessenger:messenger
codec:codec];
}

- (instancetype)initWithName:(NSString*)name
Expand All @@ -69,21 +69,13 @@ - (instancetype)initWithName:(NSString*)name
taskQueue:(NSObject<FlutterTaskQueue>*)taskQueue {
self = [super init];
NSAssert(self, @"Super init cannot be nil");
_name = [name retain];
_messenger = [messenger retain];
_codec = [codec retain];
_taskQueue = [taskQueue retain];
_name = name;
Copy link
Member

@jmagman jmagman Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_messenger = messenger;
_codec = codec;
_taskQueue = taskQueue;
return self;
}

- (void)dealloc {
[_name release];
[_messenger release];
[_codec release];
[_taskQueue release];
[super dealloc];
}

- (void)sendMessage:(id)message {
[_messenger sendOnChannel:_name message:[_codec encode:message]];
}
Expand All @@ -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;
Copy link
Contributor Author

@cyanglaz cyanglaz Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FlutterBinaryMessageHandler messageHandler = ^(NSData* message, FlutterBinaryReply callback) {
handler([codec decode:message], ^(id reply) {
Copy link
Contributor

@hellohuanlin hellohuanlin Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cyanglaz cyanglaz Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cbracken cbracken Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

callback([codec encode:reply]);
Expand All @@ -128,26 +120,19 @@ - (void)resizeChannelBuffer:(NSInteger)newSize {
////////////////////////////////////////////////////////////////////////////////
@implementation FlutterError
+ (instancetype)errorWithCode:(NSString*)code message:(NSString*)message details:(id)details {
return [[[FlutterError alloc] initWithCode:code message:message details:details] autorelease];
return [[FlutterError alloc] initWithCode:code message:message details:details];
}

- (instancetype)initWithCode:(NSString*)code message:(NSString*)message details:(id)details {
NSAssert(code, @"Code cannot be nil");
self = [super init];
NSAssert(self, @"Super init cannot be nil");
_code = [code retain];
_message = [message retain];
_details = [details retain];
_code = code;
_message = message;
_details = details;
Copy link
Member

@jmagman jmagman Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

return self;
}

- (void)dealloc {
[_code release];
[_message release];
[_details release];
[super dealloc];
}

- (BOOL)isEqual:(id)object {
if (self == object) {
return YES;
Expand All @@ -169,24 +154,18 @@ - (NSUInteger)hash {
////////////////////////////////////////////////////////////////////////////////
@implementation FlutterMethodCall
+ (instancetype)methodCallWithMethodName:(NSString*)method arguments:(id)arguments {
return [[[FlutterMethodCall alloc] initWithMethodName:method arguments:arguments] autorelease];
return [[FlutterMethodCall alloc] initWithMethodName:method arguments:arguments];
}

- (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;
Copy link
Member

@jmagman jmagman Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_method = method;
_method = [method copy];

_arguments = arguments;
return self;
}

- (void)dealloc {
[_method release];
[_arguments release];
[super dealloc];
}

- (BOOL)isEqual:(id)object {
if (self == object) {
return YES;
Expand Down Expand Up @@ -224,8 +203,7 @@ + (instancetype)methodChannelWithName:(NSString*)name
+ (instancetype)methodChannelWithName:(NSString*)name
binaryMessenger:(NSObject<FlutterBinaryMessenger>*)messenger
codec:(NSObject<FlutterMethodCodec>*)codec {
return [[[FlutterMethodChannel alloc] initWithName:name binaryMessenger:messenger
codec:codec] autorelease];
return [[FlutterMethodChannel alloc] initWithName:name binaryMessenger:messenger codec:codec];
}

- (instancetype)initWithName:(NSString*)name
Expand All @@ -240,21 +218,13 @@ - (instancetype)initWithName:(NSString*)name
taskQueue:(NSObject<FlutterTaskQueue>*)taskQueue {
self = [super init];
NSAssert(self, @"Super init cannot be nil");
_name = [name retain];
_messenger = [messenger retain];
_codec = [codec retain];
_taskQueue = [taskQueue retain];
_name = name;
_messenger = messenger;
_codec = codec;
_taskQueue = taskQueue;
return self;
}

- (void)dealloc {
[_name release];
[_messenger release];
[_codec release];
[_taskQueue release];
[super dealloc];
}

- (void)invokeMethod:(NSString*)method arguments:(id)arguments {
FlutterMethodCall* methodCall = [FlutterMethodCall methodCallWithMethodName:method
arguments:arguments];
Expand Down Expand Up @@ -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;
Copy link
Contributor

@hellohuanlin hellohuanlin Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same q here

Copy link
Contributor Author

@cyanglaz cyanglaz Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

FlutterBinaryMessageHandler messageHandler = ^(NSData* message, FlutterBinaryReply callback) {
FlutterMethodCall* call = [codec decodeMethodCall:message];
handler(call, ^(id result) {
Expand Down Expand Up @@ -328,8 +298,7 @@ + (instancetype)eventChannelWithName:(NSString*)name
+ (instancetype)eventChannelWithName:(NSString*)name
binaryMessenger:(NSObject<FlutterBinaryMessenger>*)messenger
codec:(NSObject<FlutterMethodCodec>*)codec {
return [[[FlutterEventChannel alloc] initWithName:name binaryMessenger:messenger
codec:codec] autorelease];
return [[FlutterEventChannel alloc] initWithName:name binaryMessenger:messenger codec:codec];
}

- (instancetype)initWithName:(NSString*)name
Expand All @@ -344,21 +313,13 @@ - (instancetype)initWithName:(NSString*)name
taskQueue:(NSObject<FlutterTaskQueue>* _Nullable)taskQueue {
self = [super init];
NSAssert(self, @"Super init cannot be nil");
_name = [name retain];
_messenger = [messenger retain];
_codec = [codec retain];
_taskQueue = [taskQueue retain];
_name = name;
Copy link
Member

@jmagman jmagman Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

etc

_messenger = messenger;
_codec = codec;
_taskQueue = taskQueue;
return self;
}

- (void)dealloc {
[_name release];
[_codec release];
[_messenger release];
[_taskQueue release];
[super dealloc];
}

static FlutterBinaryMessengerConnection SetStreamHandlerMessageHandlerOnChannel(
NSObject<FlutterStreamHandler>* handler,
NSString* name,
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/darwin/common/framework/Source/FlutterCodecs.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ - (NSString*)decode:(NSData*)message {
if (message == nil) {
return nil;
}
return [[[NSString alloc] initWithData:message encoding:NSUTF8StringEncoding] autorelease];
return [[NSString alloc] initWithData:message encoding:NSUTF8StringEncoding];
}
@end

Expand Down
Loading