Started clearing out the parent of orphaned semantic objects.

[gaaclarke]

Relevant issue: flutter/flutter#53827

The actual bug was here: https://github.com/gaaclarke/engine/blob/3185a9fca1386f5ba062f7adafa4d102f2d7e036/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm#L759:L759.
We weren't clearing out the parent.

The following changes have been made to make the code more robust:

1. SemanticsObject.parent is no longer settable publicly. Editing of that field now happens only in SemanticsObject in reaction to editing its children.
2. SemanticsObject.children no longer returns a mutable collection. All edits must go through -[SemanticsObject setChildren:] or -[SemanticsObject replaceChildAtIndex:withChild:].

[dnfield]

This was added previously to prevent a different memory leak. Are we sure we're not leaking a11y objects with this?

[gaaclarke]

It is redundant, the next line deletes the whole collection which will release all of its items. If this appeared to fix something, there is a deeper problem somewhere else.

[dnfield]

It may have just been paranoia :)

[gaaclarke]

The most likely scenario is that if this fixed something, [_children release] is not actually deleting _children. I've audited it and the code looks fine. Maybe this was not the case in the past. This was the most likely culprit, but it correctly has an autorelease on it: https://github.com/gaaclarke/engine/blob/da3ffbf8562299dbfa5b4b2ab42c142b79a5b0af/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm#L770:L770

[gaaclarke]

Another possibility is that someone is holding onto the SemanticsObject.children, if they were doing that, I don't think we should be clearing it out. I'll put it back in since it doesn't have direct impact on the issue.

[dnfield]

I don't see how this really helps us vs. having the parent property be writable. It's not like there were other TUs setting the property. Can you explain a bit?

[gaaclarke]

Yes, the point is we want to control the editing of the parent property. We want to enforce that editing it happens in conjunction with editing children. If we were using another language I could make the setter private. Here i just do it by convention and naming. We could split SemanticsObject into its own file and make it truly private if you aren't happy with how it's setup.

[dnfield]

Why can't we just leave it as _parent = parent? Am I just forgetting how Objective C works?

[gaaclarke]

Because you are calling it on someone else:

void SemanticObject.addChild(SemanticObject child) {
  _children.append(child);
  child.parent = this;
}

We want only SemanticObject to have access to the setter, we don't want the accessibility bridge or whoever fiddling with it (private in a c++ sense). My hope was that when people see a method called "privateSetParent" they might think twice about who is suppose to be calling it and why.

[dnfield]

LGTM. Might be nice to expand the comment on privateSetParent to explain when it's valid to use it. We should just refactor this whole thing to make it more testable - one option might be a C++ tree impl that could be unit tested and then just used here.

[gaaclarke]

@dnfield and I decided in chat to refactor SemanticsObject into its own class. I'll follow up with another PR.

[magicleon94]

Has this reached the stable branch? We are experiencing this issue in our production app...

[gaaclarke]

@magicleon94 You can find where commits are by looking at their roll commit into flutter/flutter: flutter/flutter@46ce54d

So, it looks like 1.18.0-6.0.pre and later have it.