From 0e6105f1fb69353e64741249aa8110b001fd02cd Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Fri, 10 May 2024 15:52:57 -0700 Subject: [PATCH 1/2] Migrate SemanticsObject and FlutterSemanticsScrollView to ARC --- shell/platform/darwin/ios/BUILD.gn | 8 ++-- .../Source/FlutterSemanticsScrollView.h | 2 +- .../Source/FlutterSemanticsScrollView.mm | 3 ++ .../ios/framework/Source/SemanticsObject.h | 4 +- .../ios/framework/Source/SemanticsObject.mm | 40 +++++-------------- 5 files changed, 19 insertions(+), 38 deletions(-) diff --git a/shell/platform/darwin/ios/BUILD.gn b/shell/platform/darwin/ios/BUILD.gn index 800025c14a61d..39c23e8c0de54 100644 --- a/shell/platform/darwin/ios/BUILD.gn +++ b/shell/platform/darwin/ios/BUILD.gn @@ -83,6 +83,8 @@ source_set("flutter_framework_source_arc") { "framework/Source/FlutterPluginAppLifeCycleDelegate.mm", "framework/Source/FlutterRestorationPlugin.h", "framework/Source/FlutterRestorationPlugin.mm", + "framework/Source/FlutterSemanticsScrollView.h", + "framework/Source/FlutterSemanticsScrollView.mm", "framework/Source/FlutterSpellCheckPlugin.h", "framework/Source/FlutterSpellCheckPlugin.mm", "framework/Source/FlutterTextInputDelegate.h", @@ -100,6 +102,8 @@ source_set("flutter_framework_source_arc") { "framework/Source/FlutterViewResponder.h", "framework/Source/KeyCodeMap.g.mm", "framework/Source/KeyCodeMap_Internal.h", + "framework/Source/SemanticsObject.h", + "framework/Source/SemanticsObject.mm", "framework/Source/UIViewController+FlutterScreenAndSceneIfLoaded.h", "framework/Source/UIViewController+FlutterScreenAndSceneIfLoaded.mm", "framework/Source/connection_collection.h", @@ -173,12 +177,8 @@ source_set("flutter_framework_source") { "framework/Source/FlutterHeadlessDartRunner.mm", "framework/Source/FlutterPlatformPlugin.h", "framework/Source/FlutterPlatformPlugin.mm", - "framework/Source/FlutterSemanticsScrollView.h", - "framework/Source/FlutterSemanticsScrollView.mm", "framework/Source/FlutterViewController.mm", "framework/Source/FlutterViewController_Internal.h", - "framework/Source/SemanticsObject.h", - "framework/Source/SemanticsObject.mm", "framework/Source/accessibility_bridge.h", "framework/Source/accessibility_bridge.mm", "framework/Source/accessibility_text_entry.h", diff --git a/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.h b/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.h index f83047f28b562..28b9b5636296b 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.h +++ b/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.h @@ -20,7 +20,7 @@ NS_ASSUME_NONNULL_BEGIN */ @interface FlutterSemanticsScrollView : UIScrollView -@property(nonatomic, assign, nullable) SemanticsObject* semanticsObject; +@property(nonatomic, weak, nullable) SemanticsObject* semanticsObject; - (instancetype)init NS_UNAVAILABLE; - (instancetype)initWithFrame:(CGRect)frame NS_UNAVAILABLE; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.mm b/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.mm index 2a0083a22d1f3..9b74e2ddabc79 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.mm @@ -4,8 +4,11 @@ #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.h" +#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h" #import "flutter/shell/platform/darwin/ios/framework/Source/SemanticsObject.h" +FLUTTER_ASSERT_ARC + @implementation FlutterSemanticsScrollView - (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject { diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObject.h b/shell/platform/darwin/ios/framework/Source/SemanticsObject.h index 792113c501636..d3441ec0058b1 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObject.h +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObject.h @@ -38,7 +38,7 @@ constexpr float kScrollExtentMaxForInf = 1000; * The parent of this node in the node tree. Will be nil for the root node and * during transient state changes. */ -@property(nonatomic, assign, readonly) SemanticsObject* parent; +@property(nonatomic, weak, readonly) SemanticsObject* parent; /** * The accessibility bridge that this semantics object is attached to. This @@ -231,7 +231,7 @@ constexpr float kScrollExtentMaxForInf = 1000; bridge:(fml::WeakPtr)bridge NS_DESIGNATED_INITIALIZER; -@property(nonatomic, assign) SemanticsObject* semanticsObject; +@property(nonatomic, weak) SemanticsObject* semanticsObject; @end diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm index ef97d929f05ba..2405b49b78638 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm @@ -7,6 +7,8 @@ #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h" #import "flutter/shell/platform/darwin/ios/framework/Source/FlutterSemanticsScrollView.h" +FLUTTER_ASSERT_ARC + namespace { flutter::SemanticsAction GetSemanticsActionForScrollDirection( @@ -107,11 +109,6 @@ - (instancetype)initWithBridge:(fml::WeakPtr)br return self; } -- (void)dealloc { - [_nativeSwitch release]; - [super dealloc]; -} - - (NSMethodSignature*)methodSignatureForSelector:(SEL)sel { NSMethodSignature* result = [super methodSignatureForSelector:sel]; if (!result) { @@ -145,7 +142,7 @@ - (UIAccessibilityTraits)accessibilityTraits { @end // FlutterSwitchSemanticsObject @interface FlutterScrollableSemanticsObject () -@property(nonatomic, retain) FlutterSemanticsScrollView* scrollView; +@property(nonatomic) FlutterSemanticsScrollView* scrollView; @end @implementation FlutterScrollableSemanticsObject @@ -164,9 +161,6 @@ - (instancetype)initWithBridge:(fml::WeakPtr)br - (void)dealloc { [_scrollView removeFromSuperview]; - _scrollView.semanticsObject = nil; - [_scrollView release]; - [super dealloc]; } - (void)accessibilityBridgeDidFinishUpdate { @@ -249,10 +243,10 @@ @implementation FlutterCustomAccessibilityAction { @end @interface SemanticsObject () -@property(nonatomic, retain) SemanticsObjectContainer* container; +@property(nonatomic) SemanticsObjectContainer* container; /** Should only be called in conjunction with setting child/parent relationship. */ -@property(nonatomic, assign, readwrite) SemanticsObject* parent; +@property(nonatomic, weak, readwrite) SemanticsObject* parent; @end @@ -265,7 +259,6 @@ @implementation SemanticsObject { // Method declared as unavailable in the interface - (instancetype)init { - [self release]; [super doesNotRecognizeSelector:_cmd]; return nil; } @@ -296,14 +289,9 @@ - (void)dealloc { for (SemanticsObject* child in _children) { child.parent = nil; } - [_children removeAllObjects]; - [_children release]; - [_childrenInHitTestOrder release]; _parent = nil; - [_container release]; _inDealloc = YES; - [super dealloc]; } #pragma mark - Semantic object property accesser @@ -312,7 +300,6 @@ - (void)setChildren:(NSArray*)children { for (SemanticsObject* child in _children) { child.parent = nil; } - [_children release]; _children = [children mutableCopy]; for (SemanticsObject* child in _children) { child.parent = self; @@ -323,7 +310,6 @@ - (void)setChildrenInHitTestOrder:(NSArray*)childrenInHitTestO for (SemanticsObject* child in _childrenInHitTestOrder) { child.parent = nil; } - [_childrenInHitTestOrder release]; _childrenInHitTestOrder = [childrenInHitTestOrder copy]; for (SemanticsObject* child in _childrenInHitTestOrder) { child.parent = self; @@ -416,7 +402,7 @@ - (NSAttributedString*)createAttributedStringFromString:(NSString*)string withAttributes: (const flutter::StringAttributes&)attributes { NSMutableAttributedString* attributedString = - [[[NSMutableAttributedString alloc] initWithString:string] autorelease]; + [[NSMutableAttributedString alloc] initWithString:string]; for (const auto& attribute : attributes) { NSRange range = NSMakeRange(attribute->start, attribute->end - attribute->start); switch (attribute->type) { @@ -686,9 +672,8 @@ - (id)accessibilityContainer { if ([self hasChildren] || self.uid == kRootNodeId) { if (self.container == nil) { - self.container = - [[[SemanticsObjectContainer alloc] initWithSemanticsObject:self - bridge:self.bridge] autorelease]; + self.container = [[SemanticsObjectContainer alloc] initWithSemanticsObject:self + bridge:[self bridge]]; } return self.container; } @@ -794,7 +779,6 @@ @implementation FlutterSemanticsObject { // Method declared as unavailable in the interface - (instancetype)init { - [self release]; [super doesNotRecognizeSelector:_cmd]; return nil; } @@ -853,7 +837,7 @@ - (UIAccessibilityTraits)accessibilityTraits { @end @interface FlutterPlatformViewSemanticsContainer () -@property(nonatomic, assign) UIView* platformView; +@property(nonatomic, weak) UIView* platformView; @end @implementation FlutterPlatformViewSemanticsContainer @@ -868,11 +852,6 @@ - (instancetype)initWithBridge:(fml::WeakPtr)br return self; } -- (void)dealloc { - _platformView = nil; - [super dealloc]; -} - - (id)nativeAccessibility { return self.platformView; } @@ -887,7 +866,6 @@ @implementation SemanticsObjectContainer { // Method declared as unavailable in the interface - (instancetype)init { - [self release]; [super doesNotRecognizeSelector:_cmd]; return nil; } From 82e84dcc55fb436fd3ef75353a23e0fc9b1416c1 Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Tue, 14 May 2024 20:43:52 -0700 Subject: [PATCH 2/2] Review comments --- .../ios/framework/Source/SemanticsObject.mm | 34 +++++-------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm index 2405b49b78638..2fe38516f4f5f 100644 --- a/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm +++ b/shell/platform/darwin/ios/framework/Source/SemanticsObject.mm @@ -255,14 +255,6 @@ @implementation SemanticsObject { BOOL _inDealloc; } -#pragma mark - Override base class designated initializers - -// Method declared as unavailable in the interface -- (instancetype)init { - [super doesNotRecognizeSelector:_cmd]; - return nil; -} - #pragma mark - Designated initializers - (instancetype)initWithBridge:(fml::WeakPtr)bridge @@ -286,9 +278,16 @@ - (instancetype)initWithBridge:(fml::WeakPtr)br } - (void)dealloc { + // Set parent and children parents to nil explicitly in dealloc. + // -[UIAccessibilityElement dealloc] has in the past called into -accessibilityContainer + // and self.children. There have also been crashes related to iOS + // accessing methods during dealloc, and there's a lag before the tree changes. + // See https://github.com/flutter/engine/pull/4602 and + // https://github.com/flutter/engine/pull/27786. for (SemanticsObject* child in _children) { child.parent = nil; } + [_children removeAllObjects]; _parent = nil; _inDealloc = YES; @@ -673,7 +672,7 @@ - (id)accessibilityContainer { if ([self hasChildren] || self.uid == kRootNodeId) { if (self.container == nil) { self.container = [[SemanticsObjectContainer alloc] initWithSemanticsObject:self - bridge:[self bridge]]; + bridge:self.bridge]; } return self.container; } @@ -772,16 +771,7 @@ - (void)accessibilityElementDidLoseFocus { @end -@implementation FlutterSemanticsObject { -} - -#pragma mark - Override base class designated initializers - -// Method declared as unavailable in the interface -- (instancetype)init { - [super doesNotRecognizeSelector:_cmd]; - return nil; -} +@implementation FlutterSemanticsObject #pragma mark - Designated initializers @@ -864,12 +854,6 @@ @implementation SemanticsObjectContainer { #pragma mark - initializers -// Method declared as unavailable in the interface -- (instancetype)init { - [super doesNotRecognizeSelector:_cmd]; - return nil; -} - - (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject bridge:(fml::WeakPtr)bridge { FML_DCHECK(semanticsObject) << "semanticsObject must be set";