From 1363ff75c24c21416819f1db4539cceb199b52a2 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 18 Dec 2018 09:01:15 -0800 Subject: [PATCH 1/7] fix memory leak in a11y bridge --- .../framework/Source/accessibility_bridge.mm | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index d744e5fc06605..de10408e8aab7 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -82,13 +82,14 @@ @implementation FlutterCustomAccessibilityAction { */ @interface SemanticsObjectContainer : NSObject - (instancetype)init __attribute__((unavailable("Use initWithSemanticsObject instead"))); -- (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject +- (instancetype)initWithSemanticsObject:(fml::WeakPtr)semanticsObject bridge:(fml::WeakPtr)bridge NS_DESIGNATED_INITIALIZER; @end @implementation SemanticsObject { - SemanticsObjectContainer* _container; + fml::scoped_nsobject _container; + std::unique_ptr> _weakFactory; } #pragma mark - Override base class designated initializers @@ -111,6 +112,7 @@ - (instancetype)initWithBridge:(fml::WeakPtr)bridge _bridge = bridge; _uid = uid; self.children = [[[NSMutableArray alloc] init] autorelease]; + _weakFactory = std::make_unique>(self); } return self; @@ -123,8 +125,6 @@ - (void)dealloc { [_children removeAllObjects]; [_children dealloc]; _parent = nil; - [_container release]; - _container = nil; [super dealloc]; } @@ -268,9 +268,10 @@ - (CGRect)globalRect { - (id)accessibilityContainer { if ([self hasChildren] || [self uid] == kRootNodeId) { if (_container == nil) - _container = [[SemanticsObjectContainer alloc] initWithSemanticsObject:self - bridge:[self bridge]]; - return _container; + _container.reset([[SemanticsObjectContainer alloc] + initWithSemanticsObject:_weakFactory->GetWeakPtr() + bridge:[self bridge]]); + return _container.get(); } if ([self parent] == nil) { // This can happen when we have released the accessibility tree but iOS is @@ -395,7 +396,7 @@ - (UIAccessibilityTraits)accessibilityTraits { @end @implementation SemanticsObjectContainer { - SemanticsObject* _semanticsObject; + fml::WeakPtr _semanticsObject; fml::WeakPtr _bridge; } @@ -408,47 +409,40 @@ - (instancetype)init { return nil; } -- (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject +- (instancetype)initWithSemanticsObject:(fml::WeakPtr)semanticsObject bridge:(fml::WeakPtr)bridge { - FML_DCHECK(semanticsObject != nil) << "semanticsObject must be set"; + FML_DCHECK(semanticsObject.get() != nil) << "semanticsObject must be set"; self = [super init]; if (self) { _semanticsObject = semanticsObject; - // The pointer is managed manually. - [_semanticsObject retain]; _bridge = bridge; } return self; } -- (void)dealloc { - [_semanticsObject release]; - [super dealloc]; -} - #pragma mark - UIAccessibilityContainer overrides - (NSInteger)accessibilityElementCount { - return [[_semanticsObject children] count] + 1; + return [[_semanticsObject.get() children] count] + 1; } - (nullable id)accessibilityElementAtIndex:(NSInteger)index { if (index < 0 || index >= [self accessibilityElementCount]) return nil; if (index == 0) - return _semanticsObject; - SemanticsObject* child = [_semanticsObject children][index - 1]; + return _semanticsObject.get(); + SemanticsObject* child = [_semanticsObject.get() children][index - 1]; if ([child hasChildren]) return [child accessibilityContainer]; return child; } - (NSInteger)indexOfAccessibilityElement:(id)element { - if (element == _semanticsObject) + if (element == _semanticsObject.get()) return 0; - NSMutableArray* children = [_semanticsObject children]; + NSMutableArray* children = [_semanticsObject.get() children]; for (size_t i = 0; i < [children count]; i++) { SemanticsObject* child = children[i]; if ((![child hasChildren] && child == element) || @@ -465,22 +459,22 @@ - (BOOL)isAccessibilityElement { } - (CGRect)accessibilityFrame { - return [_semanticsObject accessibilityFrame]; + return [_semanticsObject.get() accessibilityFrame]; } - (id)accessibilityContainer { if (!_bridge) { return nil; } - return ([_semanticsObject uid] == kRootNodeId) + return ([_semanticsObject.get() uid] == kRootNodeId) ? _bridge->view() - : [[_semanticsObject parent] accessibilityContainer]; + : [[_semanticsObject.get() parent] accessibilityContainer]; } #pragma mark - UIAccessibilityAction overrides - (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction { - return [_semanticsObject accessibilityScroll:direction]; + return [_semanticsObject.get() accessibilityScroll:direction]; } @end @@ -506,6 +500,7 @@ - (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction { } AccessibilityBridge::~AccessibilityBridge() { + clearState(); view_.accessibilityElements = nil; [accessibility_channel_.get() setMessageHandler:nil]; } From 373f96b3285d91bb85335e684d5e9c90ef7bfa58 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 18 Dec 2018 09:41:34 -0800 Subject: [PATCH 2/7] fix merge --- .../darwin/ios/framework/Source/accessibility_bridge.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 02ced9ba0ad76..3f1f739fcfe45 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -111,7 +111,7 @@ - (instancetype)initWithBridge:(fml::WeakPtr)bridge if (self) { _bridge = bridge; _uid = uid; - self.children = [[[NSMutableArray alloc] init] autorelease]; + _children = [[NSMutableArray alloc] init]; _weakFactory = std::make_unique>(self); } From e862289bf530fbbecc9fae52523dfaf846fb2c63 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 14 Jan 2019 13:56:31 -0800 Subject: [PATCH 3/7] std::weak_ptr --- .../darwin/ios/framework/Source/accessibility_bridge.mm | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 3f1f739fcfe45..c6c1033dff321 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -82,14 +82,13 @@ @implementation FlutterCustomAccessibilityAction { */ @interface SemanticsObjectContainer : NSObject - (instancetype)init __attribute__((unavailable("Use initWithSemanticsObject instead"))); -- (instancetype)initWithSemanticsObject:(fml::WeakPtr)semanticsObject +- (instancetype)initWithSemanticsObject:(std::weak_ptr)semanticsObject bridge:(fml::WeakPtr)bridge NS_DESIGNATED_INITIALIZER; @end @implementation SemanticsObject { fml::scoped_nsobject _container; - std::unique_ptr> _weakFactory; } #pragma mark - Override base class designated initializers @@ -112,7 +111,6 @@ - (instancetype)initWithBridge:(fml::WeakPtr)bridge _bridge = bridge; _uid = uid; _children = [[NSMutableArray alloc] init]; - _weakFactory = std::make_unique>(self); } return self; @@ -396,7 +394,7 @@ - (UIAccessibilityTraits)accessibilityTraits { @end @implementation SemanticsObjectContainer { - fml::WeakPtr _semanticsObject; + std::weak_ptr _semanticsObject; fml::WeakPtr _bridge; } @@ -409,7 +407,7 @@ - (instancetype)init { return nil; } -- (instancetype)initWithSemanticsObject:(fml::WeakPtr)semanticsObject +- (instancetype)initWithSemanticsObject:(std::weak_ptr)semanticsObject bridge:(fml::WeakPtr)bridge { FML_DCHECK(semanticsObject.get() != nil) << "semanticsObject must be set"; self = [super init]; From 8ef05c219486fb4267503f78ed8a96ab6b8897fd Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 14 Jan 2019 14:49:20 -0800 Subject: [PATCH 4/7] scoped_nsobject --- .../framework/Source/accessibility_bridge.mm | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index c6c1033dff321..e425666af0cdf 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -82,7 +82,7 @@ @implementation FlutterCustomAccessibilityAction { */ @interface SemanticsObjectContainer : NSObject - (instancetype)init __attribute__((unavailable("Use initWithSemanticsObject instead"))); -- (instancetype)initWithSemanticsObject:(std::weak_ptr)semanticsObject +- (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject bridge:(fml::WeakPtr)bridge NS_DESIGNATED_INITIALIZER; @end @@ -266,9 +266,8 @@ - (CGRect)globalRect { - (id)accessibilityContainer { if ([self hasChildren] || [self uid] == kRootNodeId) { if (_container == nil) - _container.reset([[SemanticsObjectContainer alloc] - initWithSemanticsObject:_weakFactory->GetWeakPtr() - bridge:[self bridge]]); + _container.reset([[SemanticsObjectContainer alloc] initWithSemanticsObject:self + bridge:[self bridge]]); return _container.get(); } if ([self parent] == nil) { @@ -394,7 +393,7 @@ - (UIAccessibilityTraits)accessibilityTraits { @end @implementation SemanticsObjectContainer { - std::weak_ptr _semanticsObject; + fml::scoped_nsobject _semanticsObject; fml::WeakPtr _bridge; } @@ -407,13 +406,13 @@ - (instancetype)init { return nil; } -- (instancetype)initWithSemanticsObject:(std::weak_ptr)semanticsObject +- (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject bridge:(fml::WeakPtr)bridge { - FML_DCHECK(semanticsObject.get() != nil) << "semanticsObject must be set"; + FML_DCHECK(semanticsObject) << "semanticsObject must be set"; self = [super init]; if (self) { - _semanticsObject = semanticsObject; + _semanticsObject.reset([semanticsObject retain]); _bridge = bridge; } @@ -429,8 +428,9 @@ - (NSInteger)accessibilityElementCount { - (nullable id)accessibilityElementAtIndex:(NSInteger)index { if (index < 0 || index >= [self accessibilityElementCount]) return nil; - if (index == 0) + if (index == 0) { return _semanticsObject.get(); + } SemanticsObject* child = [_semanticsObject.get() children][index - 1]; if ([child hasChildren]) return [child accessibilityContainer]; From 85433d60462d3c94b99ac51324c1469ece4ce801 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 14 Jan 2019 15:50:42 -0800 Subject: [PATCH 5/7] autorelease --- .../darwin/ios/framework/Source/accessibility_bridge.mm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index e425666af0cdf..8c44162960325 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -266,8 +266,9 @@ - (CGRect)globalRect { - (id)accessibilityContainer { if ([self hasChildren] || [self uid] == kRootNodeId) { if (_container == nil) - _container.reset([[SemanticsObjectContainer alloc] initWithSemanticsObject:self - bridge:[self bridge]]); + _container.reset( + [[[SemanticsObjectContainer alloc] initWithSemanticsObject:self + bridge:[self bridge]] autorelease]); return _container.get(); } if ([self parent] == nil) { From 516271bede0bdee7b87399ec533e0cab57fb5176 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 14 Jan 2019 16:16:10 -0800 Subject: [PATCH 6/7] make it a weak property --- .../framework/Source/accessibility_bridge.mm | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 8c44162960325..1984f5f204788 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -85,6 +85,9 @@ - (instancetype)init __attribute__((unavailable("Use initWithSemanticsObject ins - (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject bridge:(fml::WeakPtr)bridge NS_DESIGNATED_INITIALIZER; + +@property(nonatomic, weak) SemanticsObject* semanticsObject; + @end @implementation SemanticsObject { @@ -266,9 +269,8 @@ - (CGRect)globalRect { - (id)accessibilityContainer { if ([self hasChildren] || [self uid] == kRootNodeId) { if (_container == nil) - _container.reset( - [[[SemanticsObjectContainer alloc] initWithSemanticsObject:self - bridge:[self bridge]] autorelease]); + _container.reset([[SemanticsObjectContainer alloc] initWithSemanticsObject:self + bridge:[self bridge]]); return _container.get(); } if ([self parent] == nil) { @@ -394,7 +396,7 @@ - (UIAccessibilityTraits)accessibilityTraits { @end @implementation SemanticsObjectContainer { - fml::scoped_nsobject _semanticsObject; + SemanticsObject* _semanticsObject; fml::WeakPtr _bridge; } @@ -413,7 +415,7 @@ - (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject self = [super init]; if (self) { - _semanticsObject.reset([semanticsObject retain]); + _semanticsObject = semanticsObject; _bridge = bridge; } @@ -423,25 +425,25 @@ - (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject #pragma mark - UIAccessibilityContainer overrides - (NSInteger)accessibilityElementCount { - return [[_semanticsObject.get() children] count] + 1; + return [[_semanticsObject children] count] + 1; } - (nullable id)accessibilityElementAtIndex:(NSInteger)index { if (index < 0 || index >= [self accessibilityElementCount]) return nil; if (index == 0) { - return _semanticsObject.get(); + return _semanticsObject; } - SemanticsObject* child = [_semanticsObject.get() children][index - 1]; + SemanticsObject* child = [_semanticsObject children][index - 1]; if ([child hasChildren]) return [child accessibilityContainer]; return child; } - (NSInteger)indexOfAccessibilityElement:(id)element { - if (element == _semanticsObject.get()) + if (element == _semanticsObject) return 0; - NSMutableArray* children = [_semanticsObject.get() children]; + NSMutableArray* children = [_semanticsObject children]; for (size_t i = 0; i < [children count]; i++) { SemanticsObject* child = children[i]; if ((![child hasChildren] && child == element) || @@ -458,22 +460,22 @@ - (BOOL)isAccessibilityElement { } - (CGRect)accessibilityFrame { - return [_semanticsObject.get() accessibilityFrame]; + return [_semanticsObject accessibilityFrame]; } - (id)accessibilityContainer { if (!_bridge) { return nil; } - return ([_semanticsObject.get() uid] == kRootNodeId) + return ([_semanticsObject uid] == kRootNodeId) ? _bridge->view() - : [[_semanticsObject.get() parent] accessibilityContainer]; + : [[_semanticsObject parent] accessibilityContainer]; } #pragma mark - UIAccessibilityAction overrides - (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction { - return [_semanticsObject.get() accessibilityScroll:direction]; + return [_semanticsObject accessibilityScroll:direction]; } @end From 4ea610d26ca184c18301f36d12880ce33a44a90c Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 14 Jan 2019 16:36:02 -0800 Subject: [PATCH 7/7] explicitly nil out container ref --- .../platform/darwin/ios/framework/Source/accessibility_bridge.mm | 1 + 1 file changed, 1 insertion(+) diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm index 1984f5f204788..7fe516ac8097e 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm @@ -126,6 +126,7 @@ - (void)dealloc { [_children removeAllObjects]; [_children release]; _parent = nil; + _container.get().semanticsObject = nil; [super dealloc]; }