Fix iOS accessibility bridge leak #14155
Conversation
* dart-lang/sdk@3f32196 Make LateInitialziationError abstract and uninstantiable * dart-lang/sdk@f9cedb6 [vm/ffi] Fix source information in generated AST nodes * dart-lang/sdk@8023acc [vm/ffi] Fix source information in generated AST nodes - part 2 * dart-lang/sdk@1392d61 [infra] Modify 2.7.0 make_version hack so that it also works on other branches
* 3e6d6bc add pointer data santizing in flutter web engine
* 73da385 Disable a11y on detach (flutter#14142)
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
To test it, use my example in flutter/flutter#45599, and turn on the voice over. Click push flutter page, and open second route repeatedly. Without this fix, it will crash due to leaked semantics object has no bridge, which has been reset by SetOwnerViewController |
|
Thank you @xujim for reporting the issue and finding a fix! It would be nice if you can also transform your test in flutter/flutter#45599 into a test either in the flutter/engine or flutter/flutter repo, so we won't accidentally break it again in the future. CC @goderbauer @cbracken for accessibility review, @xster for add-to-app, and @chinmaygarde for iOS and engine unit tests. |
|
/cc @darrenaustin, who was recently investigating a similar problem. |
|
@liyuqian see my code: https://github.com/xujim/testaccessibility.git |
* 57afd86 Remove specificity on Android and iOS
* 253851e Move Fuchsia unit test runners into engine repo
|
Thank you @xujim for the effort of making https://github.com/xujim/testaccessibility.git ! Making a nice test is often more tricky and time consuming than the fix itself so we really appreciate your contributions here. It seems Flutter doesn't currently have any real-device iOS integration tests with accessibility. The closest test I can find is android_semantics_integration_test. In order to test flutter/flutter#45599 automatically, we probably have to write some test for devicelab that automatically turns the VoiceOver on for a real iOS device. CC one of our test guru @dnfield who is going to give a talk in accessibility in Flutter Interact tomorrow. Dan: do you have any thought on the best way to write a test for this? |
xster
changed the title
|
Also cc @gaaclarke on review and/or testing thoughts |
There was a problem hiding this comment.
I don't recommend this fix. Introducing a global c++ data structure to to store objective-c objects is perilous. The correct fix is in the dealloc of the AccessibilityBridge. You need to either issue a command that gets __UIAccessibilityFocusedElements to forget about the object, or your need to nil out the bridge parameter of outstanding SemanticObjects.
|
How do we end up with multiple focused accessibility elements? |
|
@gaaclarke , yes, the ideal fix is let the __UIAccessibilityFocusedElements forget the focused element, However seems there is no such API to do it. I have tried a lot of method, but none of them works. For example: UIAccessibilityPostNotification(Layout/Screenchange). Besides, I don't know how to nil out the semantics object in the bridge if it is string referenced by __UIAccessibilityFocusedElements. If I call release directly, it will go to crash, because __UIAccessibilityFocusedElements then access a wild pointer. |
|
This appears to be dealing with a similar issue that PR #13857 was trying to fix. Not sure if it is the same thing, but in that case the problem was that when the a11y bridge shut down it correctly released everything it was referencing. However, for some reason iOS was still holding on to the a11y focused semantic object. Like @xujim I was unable to find a way to get it to release these objects with the publicly available APIs. My solution was just guard the bridge reference in the objects so that if they tried to access a dead bridge it would no longer crash. Once VoiceOver comes up again these objects would be released by iOS. |
|
@darrenaustin , it is similar, however #13857 cannot fix the problem cleanly. At the beginning, I used the similar way, it can avoid crash, but will let the second flutter page unable to receive accessibility events. |
* google/skia@a640745 Disable QCOM_tiled_rendering while we wait for test devices * bcb8267 Revert "Do not default to downstream affinity on iOS insertText"
Co-authored-by: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com>
…lutter#16344) pick up a dart-sdk merge commit (no source changed) so that a new build of the engine can be hotfixed into flutter stable v1.12.13.
# Conflicts: # BUILD.gn # DEPS # ci/licenses_golden/licenses_flutter # ci/licenses_golden/licenses_skia # ci/licenses_golden/licenses_third_party # lib/web_ui/lib/src/engine.dart # lib/web_ui/lib/src/engine/pointer_binding.dart # lib/web_ui/lib/src/engine/pointer_converter.dart # lib/web_ui/test/engine/pointer_binding_test.dart # testing/fuchsia/run_tests.sh # testing/fuchsia/test_fars # testing/scenario_app/ios/Scenarios/Scenarios.xcodeproj/project.pbxproj # testing/scenario_app/run_ios_tests.sh
# Conflicts: # BUILD.gn # DEPS # ci/licenses_golden/licenses_flutter # ci/licenses_golden/licenses_skia # ci/licenses_golden/licenses_third_party # lib/web_ui/lib/src/engine/pointer_binding.dart # lib/web_ui/lib/src/engine/pointer_converter.dart # lib/web_ui/test/engine/pointer_binding_test.dart # shell/platform/darwin/ios/platform_view_ios.mm # testing/fuchsia/run_tests.sh # testing/fuchsia/test_fars # testing/scenario_app/ios/Scenarios/Scenarios.xcodeproj/project.pbxproj # tools/luci/force_luci_build.sh
|
cc @cbracken |
|
@cbracken advises that this PR is stale, though may have some salvageable bits. He will take a look. |
see issue: flutter/flutter#45599
When accessibility element got focus, it will be kept by __UIAccessibilityFocusedElements.
Consequently, accessibility bridge will fail to dealloc the kept semantics objects when FlutterEngine need to reset its FlutterViewController( in the function: FlutterEngine::setViewController) and reset the platform view's accesssiblity bridge( int the function: PlatformViewIOS::SetOwnerViewController).
To fix it, we use focusedAccessibilityElements( a vector) to keep those transiently focused elements, and reset its bridge to the new one. As long as __UIAccessibilityFocusedElements release them, those semantics objects will be dealloced accordingly.