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

Migrate PlatformMessageHandlerIos to ARC #52226

Merged
merged 3 commits into from
May 1, 2024
Merged

Migrate PlatformMessageHandlerIos to ARC #52226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e7382b
Migrate PlatformMessageHandlerIos to ARC
jmagman Apr 18, 2024
465946e
readonly
jmagman Apr 18, 2024
c80f585
scoped_nsprotocol
jmagman Apr 19, 2024
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
4 changes: 2 additions & 2 deletions shell/platform/darwin/ios/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ source_set("flutter_framework_source_arc") {
"ios_surface_metal_skia.mm",
"ios_surface_software.h",
"ios_surface_software.mm",
"platform_message_handler_ios.h",
"platform_message_handler_ios.mm",
"rendering_api_selection.h",
"rendering_api_selection.mm",
]
Expand Down Expand Up @@ -180,8 +182,6 @@ source_set("flutter_framework_source") {
"framework/Source/accessibility_text_entry.mm",
"ios_external_view_embedder.h",
"ios_external_view_embedder.mm",
"platform_message_handler_ios.h",
"platform_message_handler_ios.mm",
"platform_view_ios.h",
"platform_view_ios.mm",
]
Expand Down
5 changes: 1 addition & 4 deletions shell/platform/darwin/ios/platform_message_handler_ios.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,10 @@
#ifndef FLUTTER_SHELL_PLATFORM_DARWIN_IOS_PLATFORM_MESSAGE_HANDLER_IOS_H_
#define FLUTTER_SHELL_PLATFORM_DARWIN_IOS_PLATFORM_MESSAGE_HANDLER_IOS_H_

#include <unordered_map>

#include "flutter/common/task_runners.h"
#include "flutter/fml/platform/darwin/scoped_block.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/fml/task_runner.h"
#include "flutter/shell/common/platform_message_handler.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h"
#import "flutter/shell/platform/darwin/ios/flutter_task_queue_dispatch.h"

namespace flutter {
Expand Down
23 changes: 10 additions & 13 deletions shell/platform/darwin/ios/platform_message_handler_ios.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@

#import "flutter/shell/platform/darwin/ios/platform_message_handler_ios.h"

#import "flutter/fml/trace_event.h"
#import "flutter/lib/ui/window/platform_message.h"
#import "flutter/shell/platform/darwin/common/buffer_conversions.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h"
#include "flutter/fml/trace_event.h"
#include "flutter/lib/ui/window/platform_message.h"
#include "flutter/lib/ui/window/platform_message_response.h"
#include "flutter/shell/platform/darwin/common/buffer_conversions.h"

FLUTTER_ASSERT_ARC

static uint64_t platform_message_counter = 1;

@interface FLTSerialTaskQueue : NSObject <FlutterTaskQueueDispatch>
@property(nonatomic, strong) dispatch_queue_t queue;
@property(nonatomic, readonly) dispatch_queue_t queue;
@end

@implementation FLTSerialTaskQueue
Expand All @@ -24,11 +26,6 @@ - (instancetype)init {
return self;
}

- (void)dealloc {
dispatch_release(_queue);
[super dealloc];
}

- (void)dispatch:(dispatch_block_t)block {
dispatch_async(self.queue, block);
}
Expand All @@ -37,7 +34,7 @@ - (void)dispatch:(dispatch_block_t)block {
namespace flutter {

NSObject<FlutterTaskQueue>* PlatformMessageHandlerIos::MakeBackgroundTaskQueue() {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, do we need a __attribute__((ns_returns_autoreleased)) for this method in case it is ever used from MRC? Are we sure that it isn't? Maybe we could add an arc assert in the header.

Without it I think ARC reserves the right to return a retained value, expecting the call site to release it. A MRC client would expect it to be autoreleased.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought ARC followed the standard Create/Copy naming rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#conversion-to-retainable-object-pointer-type-of-expressions-with-known-semantics discusses what happens with audited functions, which are new since I had last looked at this in detail. We could add the audit annotation to all of our helpers that follow standard semantics (which I hope is all of them, but we should check) as we go to make it clearer that we know what the semantics are.

So far I can't find discussion of what happens when functions aren't audited, but I'm almost positive that when ARC first rolled out the rule was that it would follow the naming rules (i.e., that everything acted audited). It seems unlikely they would have retroactively changed that since it would have been massively breaking, but if we mark as audited it'll be moot.

Copy link
Member

Choose a reason for hiding this comment

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

Here's the hypothetical I was thinking of. This is how the code will look for those that are using this function from MRC.

// MRC
void Foo::foo() {
  bar_ = [platform_message_handler_.MakeBackgroundTaskQueue() retain];
}

I checked the output for the following:

@interface Foo : NSObject
@end

@implementation Foo
@end

Foo* MakeFoo() {
  return [[Foo alloc] init];
}

The output is

__Z7MakeFoov:                           ; @_Z7MakeFoov
	.cfi_startproc
; %bb.0:
	stp	x29, x30, [sp, #-16]!           ; 16-byte Folded Spill
	.cfi_def_cfa_offset 16
	mov	x29, sp
	.cfi_def_cfa w29, 16
	.cfi_offset w30, -8
	.cfi_offset w29, -16
	adrp	x8, _OBJC_CLASSLIST_REFERENCES_$_@PAGE
	ldr	x0, [x8, _OBJC_CLASSLIST_REFERENCES_$_@PAGEOFF]
	bl	_objc_alloc_init
	ldp	x29, x30, [sp], #16             ; 16-byte Folded Reload
	b	_objc_autoreleaseReturnValue
	.cfi_endproc
                                        ; -- End function
	.globl	__Z3Barv                        ; -- Begin function _Z3Barv
	.p2align	2

So it looks like autorelease is the default behavior, ns_returns_autoreleased seems superfluous in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it looks like autorelease is the default behavior

That matches my recollection that ARC will honor CF-style naming conventions (since your method doesn't start with Create or Copy). But it's weird that I can't find anything that explicitly says that, and sprinkling cf_audited_transfer on our C functions as we go should be very easy.

return [[[FLTSerialTaskQueue alloc] init] autorelease];
return [[FLTSerialTaskQueue alloc] init];
}

PlatformMessageHandlerIos::PlatformMessageHandlerIos(
Expand Down Expand Up @@ -127,8 +124,8 @@ - (void)dispatch:(dispatch_block_t)block {
message_handlers_.erase(channel);
if (handler) {
message_handlers_[channel] = {
.task_queue = fml::scoped_nsprotocol(
[static_cast<NSObject<FlutterTaskQueueDispatch>*>(task_queue) retain]),
.task_queue =
fml::scoped_nsprotocol(static_cast<NSObject<FlutterTaskQueueDispatch>*>(task_queue)),
Copy link
Member

Choose a reason for hiding this comment

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

This is similar to the comment on the other PR. ARC doesn't insert an unbalanced retain at a call site and the constructor for the scoped_nsprotocol uses __unsafe_unretained, assuming that it is coming in retained. So I think you need to add a retain here. Looks like ScopedTypeRef has a second parameter that you can use to specify that you want it to retain. See fml::scoped_policy::OwnershipPolicy

Copy link
Contributor

Choose a reason for hiding this comment

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

This is similar to the comment on the other PR. ARC doesn't insert an unbalanced retain at a call site and the constructor for the scoped_nsprotocol uses __unsafe_unretained, assuming that it is coming in retained.

As noted in my other responses: the version of scoped_* that we re-imported from Chromium was specifically, carefully constructed to have correct, equivalent ARC and non-ARC behaviors. In ARC there is no assuming anything about the retain state of the pointer being passed in, everything is annotated so that ARC can reason correctly about retain counts and adjust them accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

This one is slightly different since the scoped_nsprotocol isn't consuming a ns_returns_not_retained return value.

scoped_nsprotocol is expected to consume a +1 object as seen here:

  explicit ScopedTypeRef(
      __unsafe_unretained T object = Traits::InvalidValue(),
      fml::scoped_policy::OwnershipPolicy policy = fml::scoped_policy::kAssume)
      : object_(object) {
    if (object_ && policy == fml::scoped_policy::kRetain) {
      object_ = Traits::Retain(object_);
    }
  }

In this new code, where would the extra retain come from that will match the one in the destructor for the scoped pointer? I don't think there is one.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, i see it now:

#if !defined(__has_feature) || !__has_feature(objc_arc)
  explicit scoped_nsprotocol(NST object = Traits::InvalidValue(),
                             scoped_policy::OwnershipPolicy policy =
                                 scoped_policy::OwnershipPolicy::kAssume)
      : ScopedTypeRef<NST, Traits>(object, policy) {}
#else
  explicit scoped_nsprotocol(NST object = Traits::InvalidValue())
      : ScopedTypeRef<NST, Traits>(object,
                                   scoped_policy::OwnershipPolicy::kRetain) {}
#endif

.handler =
fml::ScopedBlock<FlutterBinaryMessageHandler>{
handler, fml::scoped_policy::OwnershipPolicy::kRetain},
Expand Down