Migrate ios_surface files to ARC

[jmagman]

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

Migrate ios_surface classes from MRC to ARC.

Decorate C functions that take or return Objective-C objects or structs containing Objective-C objects with cf_audited_transfer.

Part of flutter/flutter#137801.

[stuartmorgan-g]

LGTM

[stuartmorgan-g]

Couldn't we remove the scoped_nsobject from layer_?

But it's also fine to defer that to a second-pass cleanup when you eliminate scoped_nsobject completely if you'd rather.

[jmagman]

Let's do a cleanup sweep at the end. I'll keep removing them where it's easy though.

[auto-submit]

auto label is removed for flutter/engine/52139, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

[gaaclarke]

😨

[stuartmorgan-g]

@gaaclarke I'm not following your comments here; why would we need special annotations when a function that doesn't use magic words (e.g., Create) isn't returning a retained object?

[gaaclarke]

@gaaclarke I'm not following your comments here; why would we need special annotations when a function that doesn't use magic words (e.g., Create) isn't returning a retained object?

1. We have unmatched retains when creating the scoped_nsobject, I've outlined it here in this pr: Migrate PlatformMessageHandlerIos to ARC #52226 (comment)
2. We are changing the semantics of the get methods. Previously they were returning unretained references, now they are returning retained references. When they are called from MRC that could lead to unbalanced retain/releases. If we can assert it is only called from ARC that shouldn't be a problem.
3. A minor quibble, not using ns_returns_not_retained will change the performance profile.

[stuartmorgan-g]

2. We are changing the semantics of the get methods. Previously they were returning unretained references, now they are returning retained references.

What get methods are you referring to here? scoped_* gets are explicitly annotated as returning unretained. None of the semantics of scoped_* change between ARC and non-ARC. If that were not true, then everything would have undefined behavior, as it did in the original version you had tried to enable. See extensive previous discussion on that.

[gaaclarke]

What get methods are you referring to here?

I'm talking about IOSSurfaceMetalSkia::GetCAMetalLayer and IOSSurfaceMetalImpeller::GetCAMetalLayer.

[stuartmorgan-g]

Okay, so that's the same basic question as #52226 (comment)

[gaaclarke]

@gaaclarke I'm not following your comments here; why would we need special annotations when a function that doesn't use magic words (e.g., Create) isn't returning a retained object?

1. We have unmatched retains when creating the scoped_nsobject, I've outlined it here in this pr: Migrate PlatformMessageHandlerIos to ARC #52226 (comment)

This was resolved to be okay. There is conditional compilation that switches the behavior based on arc or mrc, at the scoped_nsprotocol level.

2. We are changing the semantics of the get methods. Previously they were returning unretained references, now they are returning retained references. When they are called from MRC that could lead to unbalanced retain/releases. If we can assert it is only called from ARC that shouldn't be a problem.

This was shown that the default behavior is returning autoreleased objects so should be good. I thought they were returning retained objects and the call site would release them. I think that may be an optimization that arc can do, but if it doesn't know the call site it can't.

3. A minor quibble, not using ns_returns_not_retained will change the performance profile.

This is still a thing since we are now adding extra autorelease pool entries where there were none. I'm not sure how heavily this code is used but my professional opinon would be to not add the entries to the autorelease pool and to maintain the old semantics.

This can be observed compiling

@interface Foo : NSObject
@end

@implementation Foo
@end

class Bar {
public:
  Foo *GetFoo() { return foo_; }

private:
  Foo *foo_;
};

generates

__ZN3Bar6GetFooEv:                      ; @_ZN3Bar6GetFooEv
	.cfi_startproc
; %bb.0:
	sub	sp, sp, #16
	.cfi_def_cfa_offset 16
	str	x0, [sp, #8]
	ldr	x8, [sp, #8]
	ldr	x0, [x8]
	add	sp, sp, #16
	b	_objc_retainAutoreleaseReturnValue
	.cfi_endproc
               

[stuartmorgan-g]

I'm not sure how heavily this code is used but my professional opinon would be to not add the entries to the autorelease pool and to maintain the old semantics.

Returning an unretained object without autoreleasing it is generally an anti-pattern, as it can lead to subtle crash bugs. I would strongly recommend that we follow standard advice relating to optimization: follow standard behaviors until/unless there's clear evidence that a particular piece of code is critical to overall performance, rather than trying to manual pre-optimize just in case.

[flutter-dashboard]

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

[stuartmorgan-g]

test-exempt: code refactor with no semantic change