Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

Started clearing out the parent of orphaned semantic objects. #17499

Merged
merged 5 commits into from
Apr 3, 2020
Merged

Started clearing out the parent of orphaned semantic objects. #17499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3185a9f
Started clearing out the parent of orphaned semantic objects.
gaaclarke Apr 3, 2020
dd64273
Made "SemanticsObject.children" return an immutable collection.
gaaclarke Apr 3, 2020
534845d
Made "SemanticsObject.parent" not publicly settable (must go through
gaaclarke Apr 3, 2020
da3ffbf
fixed signature on setChildren
gaaclarke Apr 3, 2020
c32f9ac
replaced removeAllObjects call, changed order of settting the parent
gaaclarke Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class AccessibilityBridge;
* The parent of this node in the node tree. Will be nil for the root node and
* during transient state changes.
*/
@property(nonatomic, assign) SemanticsObject* parent;
@property(nonatomic, readonly) SemanticsObject* parent;

/**
* The accessibility bridge that this semantics object is attached to. This
Expand Down Expand Up @@ -85,13 +85,15 @@ class AccessibilityBridge;
* Direct children of this semantics object. Each child's `parent` property must
* be equal to this object.
*/
@property(nonatomic, strong) NSMutableArray<SemanticsObject*>* children;
@property(nonatomic, strong) NSArray<SemanticsObject*>* children;

/**
* Used if this SemanticsObject is for a platform view.
*/
@property(strong, nonatomic) FlutterPlatformViewSemanticsContainer* platformViewSemanticsContainer;

- (void)replaceChildAtIndex:(NSInteger)index withChild:(SemanticsObject*)child;

- (BOOL)nodeWillCauseLayoutChange:(const flutter::SemanticsNode*)node;

#pragma mark - Designated initializers
Expand Down
39 changes: 33 additions & 6 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h"
#include "flutter/shell/platform/darwin/ios/platform_view_ios.h"

FLUTTER_ASSERT_NOT_ARC

namespace {

constexpr int32_t kRootNodeId = 0;
Expand Down Expand Up @@ -162,8 +164,14 @@ - (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject

@end

@interface SemanticsObject ()
/** Should only be called in conjunction with setting child/parent relationship. */
- (void)privateSetParent:(SemanticsObject*)parent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@gaaclarke gaaclarke Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@end

@implementation SemanticsObject {
fml::scoped_nsobject<SemanticsObjectContainer> _container;
NSMutableArray<SemanticsObject*>* _children;
}

#pragma mark - Override base class designated initializers
Expand Down Expand Up @@ -197,7 +205,7 @@ - (instancetype)initWithBridge:(fml::WeakPtr<flutter::AccessibilityBridge>)bridg

- (void)dealloc {
for (SemanticsObject* child in _children) {
child.parent = nil;
[child privateSetParent:nil];
}
[_children removeAllObjects];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may have just been paranoia :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@gaaclarke gaaclarke Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[_children release];
Expand Down Expand Up @@ -239,6 +247,28 @@ - (BOOL)hasChildren {
return [self.children count] != 0;
}

- (void)privateSetParent:(SemanticsObject*)parent {
_parent = parent;
}

- (void)setChildren:(NSArray<SemanticsObject*>*)children {
for (SemanticsObject* child in _children) {
[child privateSetParent:nil];
}
[_children release];
_children = [[NSMutableArray alloc] initWithArray:children];
for (SemanticsObject* child in _children) {
[child privateSetParent:self];
}
}

- (void)replaceChildAtIndex:(NSInteger)index withChild:(SemanticsObject*)child {
SemanticsObject* oldChild = _children[index];
[oldChild privateSetParent:nil];
[child privateSetParent:self];
[_children replaceObjectAtIndex:index withObject:child];
}

#pragma mark - UIAccessibility overrides

- (BOOL)isAccessibilityElement {
Expand Down Expand Up @@ -653,7 +683,7 @@ - (NSInteger)indexOfAccessibilityElement:(id)element {
return ((FlutterPlatformViewSemanticsContainer*)element).index;
}

NSMutableArray<SemanticsObject*>* children = [_semanticsObject children];
NSArray<SemanticsObject*>* children = [_semanticsObject children];
for (size_t i = 0; i < [children count]; i++) {
SemanticsObject* child = children[i];
if ((![child hasChildren] && child == element) ||
Expand Down Expand Up @@ -741,7 +771,6 @@ - (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction {
[[[NSMutableArray alloc] initWithCapacity:newChildCount] autorelease];
for (NSUInteger i = 0; i < newChildCount; ++i) {
SemanticsObject* child = GetOrCreateObject(node.childrenInTraversalOrder[i], nodes);
child.parent = object;
[newChildren addObject:child];
}
object.children = newChildren;
Expand Down Expand Up @@ -847,10 +876,8 @@ static void ReplaceSemanticsObject(SemanticsObject* oldObject,
assert(oldObject.node.id == newObject.node.id);
NSNumber* nodeId = @(oldObject.node.id);
NSUInteger positionInChildlist = [oldObject.parent.children indexOfObject:oldObject];
SemanticsObject* parent = oldObject.parent;
[objects removeObjectForKey:nodeId];
newObject.parent = parent;
[newObject.parent.children replaceObjectAtIndex:positionInChildlist withObject:newObject];
[oldObject.parent replaceChildAtIndex:positionInChildlist withChild:newObject];
objects[nodeId] = newObject;
}

Expand Down