fix crash SemanticsObject dealloc and access the children

[xiaoxiaowesley]

I found a crash in SemanticsObject.mm when [SemanticsObject dealloc] and access property self.children
but the children has called release

Here is the crash logic:

1. [_children release]first called in [SemanticsObject dealloc] but _children not to set nil

- (void)dealloc {
  for (SemanticsObject* child in _children) {
    [child privateSetParent:nil];
  }
  [_children removeAllObjects];
  [_children release];
  _parent = nil;
  _container.get().semanticsObject = nil;
  [_platformViewSemanticsContainer release];
  [super dealloc];
}

2. After [super dealloc] will access [UIAccessibilityElementSuperCategory dealloc] and then [SemanticsObject accessibilityContainer] and access the released _children property, and crash finally

- (id)accessibilityContainer {
  if ([self hasChildren] || [self uid] == kRootNodeId) {
    if (_container == nil)
      _container.reset([[SemanticsObjectContainer alloc] initWithSemanticsObject:self
                                                                          bridge:[self bridge]]);
    return _container.get();
  }
  if ([self parent] == nil) {
    // This can happen when we have released the accessibility tree but iOS is
    // still holding onto our objects. iOS can take some time before it
    // realizes that the tree has changed.
    return nil;
  }
  return [[self parent] accessibilityContainer];
}

- (BOOL)hasChildren {
  if (_node.IsPlatformViewNode()) {
    return YES;
  }
  return [self.children count] != 0;
}

And the issue is here flutter/flutter#87247
flutter/flutter#66032

[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 Hixie on the #hackers channel in Chat.

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]

1. After [super dealloc] will access [UIAccessibilityElementSuperCategory dealloc] and then [SemanticsObject accessibilityContainer]

It is not safe to call method on an object for which dealloc has been called, so this is a bug; niling the ivar is papering over the issue; the correct fix would be to ensure that this call doesn't happen in the first place.

[jmagman]

It is not safe to call method on an object for which dealloc has been called, so this is a bug; niling the ivar is papering over the issue; the correct fix would be to ensure that this call doesn't happen in the first place.

UIAccessibilityElementSuperCategory is in UIKit so the bug is theirs. And unfortunately we don't know and can't rely on the stability of their dealloc implementation.

I wonder if this reproduces if the children getter is implemented instead of synthesized...

[jmagman]

I wonder if this reproduces if the children getter is implemented instead of synthesized...

@xiaoxiaowesley Can you try this? Were you able to reproduce?

[xiaoxiaowesley]

I wonder if this reproduces if the children getter is implemented instead of synthesized...

@xiaoxiaowesley Can you try this? Were you able to reproduce?

Sorry I can't reproduce.But I received many crash cases from my user.

[xiaoxiaowesley]

I wonder if this reproduces if the children getter is implemented instead of synthesized...

@xiaoxiaowesley Can you try this? Were you able to reproduce?

@jmagman I added test but the "needs tests" label still there. Does the code and test look good to you?

[jmagman]

I don't think this is a complete workaround for the UIKit dealloc bug. You've added a guard at the spot where it's crashing, but not in the other places self.children is called. And we don't know and can't rely on their dealloc implementation.

It's unusual, but what if we waited to dealloc _children and _platformViewSemanticsContainer until after [super dealloc]?

- (void)dealloc {
  for (SemanticsObject* child in _children) {
    [child privateSetParent:nil];
  }
  [_children removeAllObjects];
  _parent = nil;
  _container.get().semanticsObject = nil;
  [super dealloc];

  // Due to a bug in -[UIAccessibilityElementSuperCategory dealloc]
  // that calls into self.children, delay releasing ivars until after `[super dealloc]`.
  [_children release];
  [_platformViewSemanticsContainer release];
}

@stuartmorgan can you foresee any problems with this?

[stuartmorgan-g]

I looked at this a bit more. children isn't part of UIAccessibilityElement, so I was confused about how [super dealloc] could be calling it. According to the stack, it's not; we are, in accessibilityContainer. So if the bug is that iOS 9 is calling accessibilityContainer in our dealloc flow, we should target that specifically. E.g., by doing something very explicit like setting an inDealloc bool to true before calling [super dealloc], and short-circuiting accessibilityContainer immediately if that bool is true (with a clear comment explaining why, pointing to the issue).

[xiaoxiaowesley]

I looked at this a bit more. children isn't part of UIAccessibilityElement, so I was confused about how [super dealloc] could be calling it. According to the stack, it's not; we are, in accessibilityContainer. So if the bug is that iOS 9 is calling accessibilityContainer in our dealloc flow, we should target that specifically. E.g., by doing something very explicit like setting an inDealloc bool to true before calling [super dealloc], and short-circuiting accessibilityContainer immediately if that bool is true (with a clear comment explaining why, pointing to the issue).

In apple's documentation UIAccessibilityElement.h said:

UIAccessibilityElement
Instances of this class can be used as "fake" accessibility elements.

Since SemanticsObject which is the subclass of UIAccessibilityElement maintain its own data structure like children and container in its own life cycle. So it must be release all the properties before call [super dealloc]. Though in iOS 9 [super dealloc] will call accessibilityContainer , we should make sure all the properties(children or container) is nil before [super dealloc] called.

[stuartmorgan-g]

we should make sure all the properties(children or container) is nil before [super dealloc] called.

Why? Do you have evidence that anything other than accessibilityContainer is improperly called during [super dealloc]?

[xiaoxiaowesley]

we should make sure all the properties(children or container) is nil before [super dealloc] called.

Why? Do you have evidence that anything other than accessibilityContainer is improperly called during [super dealloc]?

Of course I not sure if other override function been called during [super dealloc] since iOS framework is black box for us.
That is the point we should provent from access the released properties like children or contianer.

Setting all properies to nil is a way and using an inDealloc bool to do this thing a way eather.

@stuartmorgan I add _inDealloc bool to prevent from accessing the children in accessibilityContainer.
Thanks for reviewing.

[jmagman]

we should make sure all the properties(children or container) is nil before [super dealloc] called.

Why? Do you have evidence that anything other than accessibilityContainer is improperly called during [super dealloc]?

I spent all day yesterday yak shaving trying to get the engine tests to run on iOS 9 to see what else-dealloc could be calling. My macOS is too new to run an iOS 9 simulator, and I actually have a 9.3.6 iPhone 4s but I can't run the engine unit tests on a physical device. flutter/flutter#87699

This is as good a hack as any. We can see if any new crashes surface. LGTM