Migrate PlatformMessageHandlerIos to ARC

[jmagman]

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

Migrate PlatformMessageHandlerIos from MRC to ARC. Clean up the #includes.

Part of flutter/flutter#137801.

[gaaclarke]

This is possibly unsafe. You are storing an objc pointer inside of a c++ data structure and relying on ARC to keep it alive. I'm like 60% sure this isn't okay, maybe it's fine if the structs and c++ data structures are only ever used from objc++ with ARC?

If you haven't already, you could write a tiny sample program to demonstrate that it works. You should probably add a comment if it is safe, you could even throw in a the link to the gist that proves it's safe. You may still have to use a smart pointer that uses bridge directives to manually do the reference counting.

[stuartmorgan-g]

maybe it's fine if the structs and c++ data structures are only ever used from objc++ with ARC?

It is, but since the struct is part of the public part of the header and doesn't assert ARC I agree that this isn't currently safe.

Let's leave this as a scoped_nsprotocol for now, and unwind it when the ARC migration is complete and it's easier to reason about the safety.

[jmagman]

Thanks for keeping this on the rails, @gaaclarke. I am out of my depth with the parts of this migration that touch C++.

[stuartmorgan-g]

LGTM

[gaaclarke]

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.

[stuartmorgan-g]

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

[stuartmorgan-g]

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.

[gaaclarke]

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.

[stuartmorgan-g]

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.

[gaaclarke]

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

[stuartmorgan-g]

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.

[gaaclarke]

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.

[gaaclarke]

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