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

iOS: Migrate accessibility_bridge to ARC #55570

Merged
auto-submit merged 1 commit into from
Oct 2, 2024
Merged

iOS: Migrate accessibility_bridge to ARC #55570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions shell/platform/darwin/ios/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ source_set("flutter_framework_source_arc") {
"framework/Source/TextInputSemanticsObject.mm",
"framework/Source/UIViewController+FlutterScreenAndSceneIfLoaded.h",
"framework/Source/UIViewController+FlutterScreenAndSceneIfLoaded.mm",
"framework/Source/accessibility_bridge.h",
"framework/Source/accessibility_bridge.mm",
"framework/Source/connection_collection.h",
"framework/Source/connection_collection.mm",
"framework/Source/overlay_layer_pool.h",
Expand Down Expand Up @@ -188,8 +190,6 @@ source_set("flutter_framework_source") {
"framework/Source/FlutterPlatformPlugin.mm",
"framework/Source/FlutterViewController.mm",
"framework/Source/FlutterViewController_Internal.h",
"framework/Source/accessibility_bridge.h",
"framework/Source/accessibility_bridge.mm",
"platform_view_ios.h",
"platform_view_ios.mm",
]
Expand Down
3 changes: 3 additions & 0 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 @@ -97,6 +97,9 @@ class AccessibilityBridge final : public AccessibilityBridgeIos {
// been focused or the focus is currently outside of the flutter application
// (i.e. the status bar or keyboard)
int32_t last_focused_semantics_object_id_;

// TODO(cbracken): https://github.com/flutter/flutter/issues/137801
// Eliminate use of fml::scoped_* wrappers here.
fml::scoped_nsobject<NSMutableDictionary<NSNumber*, SemanticsObject*>> objects_;
fml::scoped_nsprotocol<FlutterBasicMessageChannel*> accessibility_channel_;
int32_t previous_route_id_ = 0;
Expand Down
42 changes: 19 additions & 23 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,7 +14,7 @@

#pragma GCC diagnostic error "-Wundeclared-selector"

FLUTTER_ASSERT_NOT_ARC
FLUTTER_ASSERT_ARC

namespace flutter {
namespace {
Expand Down Expand Up @@ -101,14 +101,13 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
needsAnnouncement = [object nodeShouldTriggerAnnouncement:&node];
[object setSemanticsNode:&node];
NSUInteger newChildCount = node.childrenInTraversalOrder.size();
NSMutableArray* newChildren =
[[[NSMutableArray alloc] initWithCapacity:newChildCount] autorelease];
NSMutableArray* newChildren = [[NSMutableArray alloc] initWithCapacity:newChildCount];
for (NSUInteger i = 0; i < newChildCount; ++i) {
SemanticsObject* child = GetOrCreateObject(node.childrenInTraversalOrder[i], nodes);
[newChildren addObject:child];
}
NSMutableArray* newChildrenInHitTestOrder =
[[[NSMutableArray alloc] initWithCapacity:newChildCount] autorelease];
[[NSMutableArray alloc] initWithCapacity:newChildCount];
for (NSUInteger i = 0; i < newChildCount; ++i) {
SemanticsObject* child = GetOrCreateObject(node.childrenInHitTestOrder[i], nodes);
[newChildrenInHitTestOrder addObject:child];
Expand All @@ -117,7 +116,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
object.childrenInHitTestOrder = newChildrenInHitTestOrder;
if (!node.customAccessibilityActions.empty()) {
NSMutableArray<FlutterCustomAccessibilityAction*>* accessibilityCustomActions =
[[[NSMutableArray alloc] init] autorelease];
[[NSMutableArray alloc] init];
for (int32_t action_id : node.customAccessibilityActions) {
flutter::CustomAccessibilityAction& action = actions_[action_id];
if (action.overrideId != -1) {
Expand All @@ -128,9 +127,9 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
NSString* label = @(action.label.data());
SEL selector = @selector(onCustomAccessibilityAction:);
FlutterCustomAccessibilityAction* customAction =
[[[FlutterCustomAccessibilityAction alloc] initWithName:label
target:object
selector:selector] autorelease];
[[FlutterCustomAccessibilityAction alloc] initWithName:label
target:object
selector:selector];
customAction.uid = action_id;
[accessibilityCustomActions addObject:customAction];
}
Expand All @@ -143,14 +142,13 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
// interrupting system notifications or other elements.
// Expectation: roughly match the behavior of polite announcements on
// Android.
NSString* announcement =
[[[NSString alloc] initWithUTF8String:object.node.label.c_str()] autorelease];
NSString* announcement = [[NSString alloc] initWithUTF8String:object.node.label.c_str()];
UIAccessibilityPostNotification(
UIAccessibilityAnnouncementNotification,
[[[NSAttributedString alloc] initWithString:announcement
attributes:@{
UIAccessibilitySpeechAttributeQueueAnnouncement : @YES
}] autorelease]);
[[NSAttributedString alloc] initWithString:announcement
attributes:@{
UIAccessibilitySpeechAttributeQueueAnnouncement : @YES
}]);
}
}

Expand All @@ -164,7 +162,7 @@ void PostAccessibilityNotification(UIAccessibilityNotifications notification,
view_controller_.view.accessibilityElements =
@[ [root accessibilityContainer] ?: [NSNull null] ];
}
NSMutableArray<SemanticsObject*>* newRoutes = [[[NSMutableArray alloc] init] autorelease];
NSMutableArray<SemanticsObject*>* newRoutes = [[NSMutableArray alloc] init];
[root collectRoutes:newRoutes];
// Finds the last route that is not in the previous routes.
for (SemanticsObject* route in newRoutes) {
Expand Down Expand Up @@ -255,7 +253,6 @@ static void ReplaceSemanticsObject(SemanticsObject* oldObject,
FML_DCHECK(oldObject.node.id == newObject.uid);
NSNumber* nodeId = @(oldObject.node.id);
NSUInteger positionInChildlist = [oldObject.parent.children indexOfObject:oldObject];
[[oldObject retain] autorelease];
Copy link
Contributor

@hellohuanlin hellohuanlin Oct 1, 2024

Choose a reason for hiding this comment

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

This was odd practice here before. Did we add this line to fix a crash before? If so, this line didn't 100% fix that crash since it's still used in the previous 3 lines.

Copy link
Member Author

@cbracken cbracken Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes! I noticed this line too and was going to leave a comment here referencing the commit here: #31351

This bumps the refcount to ensure that oldObject isn't released before it's used again below, if the refcount is decremented in this method itself. It's a relatively common pattern in old non-ARC tree-related Obj-C code.

I didn't mentally work through the gymnastics of how oldObject's refcount could be indirectly knocked down one in this method, but we are manipulating the children of our parent node here, which seems like the exact sort of case where that can happen.

This is the sort of case that motivated the creation of ARC -- and IIRC a very old WWDC presentation where ARC was introduced covered exactly this scenario (as well as the more basic cases of incorrect setter implementations that don't check for assignment of the same value already held by the ivar.

Copy link
Member Author

@cbracken cbracken Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's try mentally working through it.

  1. We get the position the list of oldObject within its parent's children into positionInChildlist.
  2. We remove oldObject from objects, which releases oldObject. If that was the only thing that had retained it, then oldObject is dead and is dealloc'ed.
  3. We call oldObject.parent ... on the next line. Boom.

To fix this, we need to ensure that we retain it for the duration we need it. We could have retained it, then manually released it after the call to oldObject.parent, but autorelease is much more future-proof.

Copy link
Member Author

@cbracken cbracken Oct 1, 2024

Choose a reason for hiding this comment

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

Ah in fact there's a comment that @stuartmorgan left pointing out this exact scenario if you scroll down far enough on the patch.

Copy link
Member Author

@cbracken cbracken Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's 11 years old, but I think this is the talk I was thinking of that covers a simpler version of this scenario, though I swear there was a talk where they went through more complex scenarios like trees.

https://www.youtube.com/watch?v=ANlpDUIF-1o#t=9m19s

Edit: might have been this one around 48 minutes or so:
https://devstreaming-cdn.apple.com/videos/wwdc/2011/323/ipad_c.m3u8

Copy link
Member Author

@cbracken cbracken Oct 1, 2024

Choose a reason for hiding this comment

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

I think the TL;DR here is that this line is an excellent argument in favour of ARC migration.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the sort of case that motivated the creation of ARC

Yes, I think it's a great sign of how successful ARC was that there are now a lot of people whose reaction to this line is "that's weird" rather than immediately knowing exactly why it's there and what it's doing. Everyone learning this pattern (usually the hard way) is definitely something we can do without 🙂

I think the TL;DR here is that this line is an excellent argument in favour of ARC migration.

Both the line and the discussion! The line because not having this kind of foot gun in the first place is clearly a safety win, but also the discussion because it illustrates that being able to correctly maintain non-ARC code is a shrinking/fading knowledge domain, so any non-ARC code we have will be an increasing liability over time.

oldObject.children = @[];
[oldObject.parent replaceChildAtIndex:positionInChildlist withChild:newObject];
[objects removeObjectForKey:nodeId];
Expand All @@ -267,22 +264,21 @@ static void ReplaceSemanticsObject(SemanticsObject* oldObject,
if (node.HasFlag(flutter::SemanticsFlags::kIsTextField) &&
!node.HasFlag(flutter::SemanticsFlags::kIsReadOnly)) {
// Text fields are backed by objects that implement UITextInput.
return [[[TextInputSemanticsObject alloc] initWithBridge:weak_ptr uid:node.id] autorelease];
return [[TextInputSemanticsObject alloc] initWithBridge:weak_ptr uid:node.id];
} else if (!node.HasFlag(flutter::SemanticsFlags::kIsInMutuallyExclusiveGroup) &&
(node.HasFlag(flutter::SemanticsFlags::kHasToggledState) ||
node.HasFlag(flutter::SemanticsFlags::kHasCheckedState))) {
return [[[FlutterSwitchSemanticsObject alloc] initWithBridge:weak_ptr uid:node.id] autorelease];
return [[FlutterSwitchSemanticsObject alloc] initWithBridge:weak_ptr uid:node.id];
} else if (node.HasFlag(flutter::SemanticsFlags::kHasImplicitScrolling)) {
return [[[FlutterScrollableSemanticsObject alloc] initWithBridge:weak_ptr
uid:node.id] autorelease];
return [[FlutterScrollableSemanticsObject alloc] initWithBridge:weak_ptr uid:node.id];
} else if (node.IsPlatformViewNode()) {
return [[[FlutterPlatformViewSemanticsContainer alloc]
return [[FlutterPlatformViewSemanticsContainer alloc]
initWithBridge:weak_ptr
uid:node.id
platformView:weak_ptr->GetPlatformViewsController()->GetFlutterTouchInterceptingViewByID(
node.platformViewId)] autorelease];
node.platformViewId)];
} else {
return [[[FlutterSemanticsObject alloc] initWithBridge:weak_ptr uid:node.id] autorelease];
return [[FlutterSemanticsObject alloc] initWithBridge:weak_ptr uid:node.id];
}
}

Expand Down