Fix iOS platform view not deallocated

[zhongwuzw]

Fixes flutter/flutter#54937

There has a retain cycle like below:
SemanticsObject->platformViewSemanticsContainer->platformViewSemanticsContainer.accessibilityElements->SemanticsObject

Another two PRs for memory leaks:
#17954
#18107

[cyanglaz]

We can add this in the dealloc of SemanticsObject

[zhongwuzw]

Emm, actually we did it in dealloc of SemanticsObject,

engine/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm

Line 198 in fb208b4

[_platformViewSemanticsContainer release];

, but cause of retain cycle, dealloc not executed.

[gaaclarke]

Please add a test. Previously we haven't been able to easily write tests for this code but now we are able to, see also https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm

I'll add a new test file for you in another PR.

[gaaclarke]

Once #18281 is landed you will be able to write a unit test against accessibility bridge. It's hung up in review currently.

[gaaclarke]

@zhongwuzw Apologies for the delay. I landed that some unit tests for that class so it should be easy to write tests =)

[zhongwuzw]

@gaaclarke 👍 ,I added test, please review.

[gaaclarke]

Nice, thanks! I've looked through this for a few hours and I think I've come up with a better solution.

If you look at this code right here:

        object.platformViewSemanticsContainer = [[[FlutterPlatformViewSemanticsContainer alloc]
            initWithSemanticsObject:object] autorelease];

We can see the obvious cycle.

The better solution is to make FlutterPlatformViewSemanticsContainer have a weak reference to the SemanticsObject, since the SemanticsObject already has a strong reference to him.

The solution originally proposed left the platformViewSemanticsContainer property in a weird state where it was somewhat strong but required outside manipulation like it was weak.

I've given some feedback on this PR, but I'll clean it all up for you so we can get this landed.

[gaaclarke]

Your example is using autorelease (which generally is proper) but other plugins are not doing this: https://github.com/flutter/plugins/blob/531ff2bc80afb3181218a1ff46e195303348ae0c/packages/google_maps_flutter/google_maps_flutter/ios/Classes/GoogleMapController.m#L37

[gaaclarke]

done

[zhongwuzw]

@gaaclarke Emm, I think they are the same thing, they also return autorelease object actually by ARC.

[gaaclarke]

You're right. Sorry, half of our codebase is in ARC half is not. I thought I was looking at non-arc code.

[gaaclarke]

@zhongwuzw Ok, the bug is fixed if you want to give it a look. The problem with memory leaks is that sometimes people can start depending on them. Let me double check this still works with the webview plugin.

[gaaclarke]

@zhongwuzw Sorry, what I tried seemed correct but broke the plugins. I reverted it. Your unit test and thus your patch is still wrong because it relies on autoreleasing the result of createWithFrame:viewIdentifier:arguments: which isn't how it's suppose to work.

[zhongwuzw]

@zhongwuzw Sorry, what I tried seemed correct but broke the plugins. I reverted it. Your unit test and thus your patch is still wrong because it relies on autoreleasing the result of createWithFrame:viewIdentifier:arguments: which isn't how it's suppose to work.

createWithFrame:viewIdentifier:arguments: actually always return autorelease object, please refer to https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html.

[gaaclarke]

@zhongwuzw thanks for pointing out my mistake with the ARC code. I switched the code back to making FlutterPlatformViewSemanticsContainer.semanticsObject weak. I got stuck yesterday because of the ARC issue.

This way is preferred because it adheres to the contract better. The way it was originally posted the property was strong, but relied in outside manipulation to work. That was fragile because people had to remember to update that property whenever the object_ instance variable is modified. This way that maintenance doesn't have to happen.

I've tested the webview plugin and it works and your test still passes.