Fixes accessibility issue that bridge fails to clean up FlutterScrollableSemanticsObject by chunhtai · Pull Request #28495 · flutter-team-archive/engine

chunhtai · 2021-09-07T23:39:58Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…ableSemanticsObject

gaaclarke · 2021-09-08T20:44:41Z

+- (void)willRemoveFromAccessibilityBridge {
+  [self removeFromSuperview];
+}
+


removeFromSuperview is called in the dealloc of the FlutterScrollableSemanticsObject, are these objects leaking, that's why they aren't getting removed from the superview? If so we should fix the leak instead.

it seems like the superview will hold a reference on this object. The object was never dealloc, so it just keeps adding more UIScrollable :D

Makes sense, I think we should investigate what I've suggested in my last review before accepting this change.

gaaclarke

Alternatively, instead of making FlutterScrollableSemanticsObject subclass UIScrollView, it could have a UIScrollView. That way when the FlutterScrollableSemanticsObject is deleted, it can remove the UIScrollView from its superview (thus breaking the cycle).

That would be preferable because, while the solution you are proposing will work, it is much more costly to maintain than if everything just works with standard object memory management.

chunhtai · 2021-09-09T19:31:58Z

existing test should cover the refactoring PTAL @gaaclarke

gaaclarke

Awesome, LGTM with the caveat that you've tested it locally too. Despite our work to improve the testing on this code it still makes me a bit nervous to change it.

chunhtai · 2021-09-10T18:30:44Z

Awesome, LGTM with the caveat that you've tested it locally too. Despite our work to improve the testing on this code it still makes me a bit nervous to change it.

I have tested it locally. I think this is one step toward the right direction, and we just need to watch out for regression...

…erScrollableSemanticsObject (flutter-team-archive/engine#28495)

dnfield · 2021-11-01T19:30:59Z

+- (instancetype)initWithSemanticsObject:(SemanticsObject*)semanticsObject {
+  self = [super initWithFrame:CGRectZero];
+  if (self) {
+    _semanticsObject = semanticsObject;


I believe we're missing a retain here, which is the cause of a crash noted in flutter/flutter#92853

It was not that we're missing a retain, but that we're missing niling this out when the parent object is deallocated.

dnfield · 2021-11-01T19:34:03Z

+                           uid:(int32_t)uid {
+  self = [super initWithBridge:bridge uid:uid];
  if (self) {
-    _semanticsObject = [semanticsObject retain];


Yes. It used to live here.

zjrwdl · 2022-05-27T04:30:37Z

these commits have been merged in flutter 2.8 ，but this crash is also exist.

stack belows:

flutter::AccessibilityBridge::GetOrCreateObject(int, std::__1::unordered_map<int, flutter::SemanticsNode, std::__1::hash, std::__1::equal_to, std::__1::allocator<std::__1::pair<int const, flutter::SemanticsNode> > >&) (in Flutter) (accessibility_bridge.mm:303)
flutter::PlatformViewIOS::UpdateSemantics(std::__1::unordered_map<int, flutter::SemanticsNode, std::__1::hash, std::__1::equal_to, std::__1::allocator<std::__1::pair<int const, flutter::SemanticsNode> > >, std::__1::unordered_map<int, flutter::CustomAccessibilityAction, std::__1::hash, std::__1::equal_to, std::__1::allocator<std::__1::pair<int const, flutter::CustomAccessibilityAction> > >) (in Flutter) (platform_view_ios.mm:191)
std::__1::__function::__func<flutter::Shell::OnEngineUpdateSemantics(std::__1::unordered_map<int, flutter::SemanticsNode, std::__1::hash, std::__1::equal_to, std::__1::allocator<std::__1::pair<int const, flutter::SemanticsNode> > >, std::__1::unordered_map<int, flutter::CustomAccessibilityAction, std::__1::hash, std::__1::equal_to, std::__1::allocator<std::__1::pair<int const, flutter::CustomAccessibilityAction> > >)::$_38, std::__1::allocator<flutter::Shell::OnEngineUpdateSemantics(std::__1::unordered_map<int, flutter::SemanticsNode, std::__1::hash, std::__1::equal_to, std::__1::allocator<std::__1::pair<int const, flutter::SemanticsNode> > >, std::__1::unordered_map<int, flutter::CustomAccessibilityAction, std::__1::hash, std::__1::equal_to, std::__1::allocator<std::__1::pair<int const, flutter::CustomAccessibilityAction> > >)::$_38>, void ()>::operator()() (in Flutter) (functional:1720)
fml::MessageLoopImpl::FlushTasks(fml::FlushType) (in Flutter) (message_loop_impl.cc:132)
fml::MessageLoopDarwin::OnTimerFire(__CFRunLoopTimer*, fml::MessageLoopDarwin*) (in Flutter) (message_loop_darwin.mm:81)

chunhtai · 2022-05-27T16:33:28Z

@zjrwdl if you are still seeing the same crash it may be a different cause, can you file a new issue with repro steps?

flutter-dashboard Bot added the platform-ios label Sep 7, 2021

google-cla Bot added the cla: yes label Sep 7, 2021

Fixes accessibility issue that bridge fails to clean up FlutterScroll…

f1489f6

…ableSemanticsObject

chunhtai force-pushed the issues/89613 branch from c2ebb0d to f1489f6 Compare September 8, 2021 19:08

chunhtai changed the title ~~Issues/89613~~ Fixes accessibility issue that bridge fails to clean up FlutterScrollableSemanticsObject Sep 8, 2021

fix comment

2a75885

chunhtai requested a review from gaaclarke September 8, 2021 19:17

gaaclarke reviewed Sep 8, 2021

View reviewed changes

gaaclarke suggested changes Sep 8, 2021

View reviewed changes

chunhtai added 3 commits September 9, 2021 11:57

refactor

6d243b4

update comment

f184b10

update

46a625d

chunhtai requested a review from gaaclarke September 9, 2021 19:31

gaaclarke suggested changes Sep 9, 2021

View reviewed changes

addressing comments

549d441

chunhtai requested a review from gaaclarke September 9, 2021 22:35

gaaclarke approved these changes Sep 10, 2021

View reviewed changes

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 10, 2021

fluttergithubbot merged commit 48ce284 into flutter-team-archive:master Sep 10, 2021

engine-flutter-autoroll mentioned this pull request Sep 10, 2021

Roll Engine from c86d581441bf to 382553f6c29f (7 revisions) flutter/flutter#89890

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 10, 2021

48ce284 Fixes accessibility issue that bridge fails to clean up Flutt…

74f3c8f

…erScrollableSemanticsObject (flutter-team-archive/engine#28495)

chunhtai mentioned this pull request Sep 24, 2021

[A11y] iOS VoiceOver trapped in scrollable. flutter/flutter#90681

Closed

dnfield reviewed Nov 1, 2021

View reviewed changes

chunhtai mentioned this pull request Jan 24, 2022

iOS Flutter 2.8.1 got crash because of EXC_BAD_ACCESS KERN_INVALID_ADDRESS flutter/flutter#96850

Closed

zjrwdl mentioned this pull request May 27, 2022

iOS Flutter 2.8 got crash because of EXC_BAD_ACCESS KERN_INVALID_ADDRESS flutter/flutter#104795

Closed

Conversation

chunhtai commented Sep 7, 2021

Pre-launch Checklist

Uh oh!

gaaclarke Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

gaaclarke Sep 8, 2021

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Sep 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Sep 10, 2021

Uh oh!

dnfield Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

zjrwdl commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai commented May 27, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

zjrwdl commented May 27, 2022 •

edited

Loading