[iOS] Fix App crash when use WebView with iOS VoiceOver #52484

tacck · 2024-05-01T10:00:50Z

This PR fix the issue flutter/flutter #140528.

This fix cleans up declarations regarding retain and release.
flutterAccessibilityContainer needs to be managed in FlutterPlatformViews.mm, and problems will occur if retain or release is performed in SemanticsObject.mm.

This fix is checked by the testFlutterPlatformViewSemanticsContainer function in the SemanticsObjectTest.mm.
The result is not changed.

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 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.

chinmaygarde · 2024-05-02T20:15:15Z

cc @jmagman

jmagman

This fix is checked by the testFlutterPlatformViewSemanticsContainer function in the SemanticsObjectTest.mm.
The result is not changed.

Are you able to get a test to fail/crash on main but succeed on this PR, that actually exercises the bad behavior? Your fix looks more correct, but in practice I can't see how it's actually changing the retains in a way that would prevent a crash.

This test, for example, passed on both main and this PR:

- (void)testFlutterPlatformViewSemanticsContainer {
  fml::WeakPtrFactory<flutter::testing::MockAccessibilityBridge> factory(
      new flutter::testing::MockAccessibilityBridge());
  fml::WeakPtr<flutter::testing::MockAccessibilityBridge> bridge = factory.GetWeakPtr();
  __weak FlutterTouchInterceptingView* weakPlatformView;
  __weak FlutterPlatformViewSemanticsContainer* weakContainer;
  @autoreleasepool {
    FlutterTouchInterceptingView* platformView = [[FlutterTouchInterceptingView alloc] init];
    weakPlatformView = platformView;

    @autoreleasepool {
      FlutterPlatformViewSemanticsContainer* container =
      [[FlutterPlatformViewSemanticsContainer alloc] initWithBridge:bridge
                                                                uid:1
                                                       platformView:platformView];
      weakContainer = container;
      XCTAssertEqualObjects(platformView.accessibilityContainer, container);
      XCTAssertNotNil(weakPlatformView);
      XCTAssertNotNil(weakContainer);
    }
    XCTAssertNotNil(weakPlatformView);
    XCTAssertNil(weakContainer);
  }
  // Check if there's no more strong references to `platformView` after container and platformView
  // are released.
  XCTAssertNil(weakPlatformView);
  XCTAssertNil(weakContainer);
}

By the way, I'm hoping to migrate FlutterPlatformViews to ARC soon to prevent these kinds of issues 🙂

…fix_ios_voiceover_webview

tacck · 2024-05-07T08:32:44Z

@jmagman
Thank you for your review.
I modified the test code based on the example you wrote.
https://github.com/flutter/engine/pull/52484/files#diff-31e401d2bb8996a130877ac6c34512d746f122368d40946914a592c1648a7f07

Are you able to get a test to fail/crash on main but succeed on this PR, that actually exercises the bad behavior? Your fix looks more correct, but in practice I can't see how it's actually changing the retains in a way that would prevent a crash.

This test verifies that the container object continues to exist even if it moves to a different scope.
This is because the platformView retains the container.
When platformView is destroyed, container will also be destroyed simultaneously.
It is the difference between main and PR.

By the way, I'm hoping to migrate FlutterPlatformViews to ARC soon to prevent these kinds of issues 🙂

That's great.
I hope it goes well 😊

jmagman · 2024-05-08T23:11:09Z

This test verifies that the container object continues to exist even if it moves to a different scope. This is because the platformView retains the container. When platformView is destroyed, container will also be destroyed simultaneously. It is the difference between main and PR.

Hm, this test also passes on main though.

tacck · 2024-05-09T05:45:59Z

I validated my fixes and tests using the following process.
Please let me know if there are any issues.

Download tools and add PATH.
SETTING UP

git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
export PATH=${PATH}:$(pwd)/depot_tools

Download the code and prepare.
Setting up the Engine development environment

mkdir engine; cd engine;
fetch flutter
git -C src/flutter remote rename origin upstream
gclient sync

git pull again at 2024/05/09 11:00+09:00 (2024/05/09 02:00+00:00 UTC)

git -C src/flutter pull upstream main
gclient sync

Build engine.
Compiling the engine

cd src
./flutter/tools/gn --ios --unoptimized --simulator --simulator-cpu=arm64
ninja -C out/ios_debug_sim_unopt_arm64
./flutter/tools/gn --unoptimized --mac-cpu arm64
ninja -C out/host_debug_unopt_arm64

Do test.
Testing the engine

./flutter/testing/run_tests.py --type=objc --ios-variant=ios_debug_sim_unopt_arm64 --variant=host_debug_unopt_arm64

Result:

(snip)
** TEST SUCCEEDED **
(snip)

Now, I think the environment is set up.

Patch the test code and do the below again.

./flutter/tools/gn --ios --unoptimized --simulator --simulator-cpu=arm64
ninja -C out/ios_debug_sim_unopt_arm64
./flutter/tools/gn --unoptimized --mac-cpu arm64
ninja -C out/host_debug_unopt_arm64
./flutter/testing/run_tests.py --type=objc --ios-variant=ios_debug_sim_unopt_arm64 --variant=host_debug_unopt_arm64

Result:

(snip)
Test Case '-[SemanticsObjectTest testFlutterPlatformViewSemanticsContainer]' started.
/../../flutter/shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm:1015: error: -[SemanticsObjectTest testFlutterPlatformViewSemanticsContainer] : ((weakContainer) != nil) failed
Test Case '-[SemanticsObjectTest testFlutterPlatformViewSemanticsContainer]' failed (0.058 seconds).
(snip)
** TEST FAILED **
(snip)

This means that the test code catches the checkpoint in the main branch, I think.

Patch all code and do above again.

Result:

** TEST SUCCEEDED **

This means that the product code fix the probrem, I think.

jmagman

Apologies for wasting your time, you're totally right, I was running the test incorrectly (I was playing around with bin/et and it output to out/ci/ios_debug_unopt_sim_arm64 instead of out/ios_debug_sim_unopt 😓

engine/testing/ios/IosUnitTests/Tests/FlutterEngineConfig.xcconfig

Lines 1 to 3 in 5eaa296

    
           FLUTTER_ENGINE[arch=x86_64]=ios_debug_sim_unopt 
        
           FLUTTER_ENGINE[arch=arm64]=ios_debug_sim_unopt_arm64 
        
           FLUTTER_ENGINE=ios_debug_sim_unopt_arm64

)

LGTM, thanks for the fix!

jmagman · 2024-05-09T19:30:58Z

+ @hellohuanlin for second review.

hellohuanlin

Good catch! Thanks for digging into this.

hellohuanlin · 2024-05-09T21:41:30Z

shell/platform/darwin/ios/framework/Source/SemanticsObject.mm


 - (void)dealloc {
-  [_platformView release];
  _platformView = nil;


nit: it's not necessary to nil out the ivar (can remove the whole dealloc method here).

I'm about to remove this dealloc in #52535, I wouldn't worry about it in this PR.

Oh actually that's not true, the PR doesn't include SemanticsObject. But I'll get to that one hopefully soon 🙂

…utter/engine#52484)

…148137) flutter/engine@c0917b1...1ccd0c3 2024-05-10 30870216+gaaclarke@users.noreply.github.com Fixed constness of display list storage. (flutter/engine#52705) 2024-05-10 zanderso@users.noreply.github.com Revert "Various documentation improvements (#52600)" (flutter/engine#52709) 2024-05-10 skia-flutter-autoroll@skia.org Manual roll Dart SDK from b7cad2edae4b to 01121c008f4d (3 revisions) (flutter/engine#52706) 2024-05-10 gray@tacck.net [iOS] Fix App crash when use WebView with iOS VoiceOver (flutter/engine#52484) 2024-05-09 ian@hixie.ch Various documentation improvements (#52600) (flutter/engine#52623) 2024-05-09 yjbanov@google.com [web] scale semantic text elements to match the desired focus ring size (flutter/engine#52586) 2024-05-09 30870216+gaaclarke@users.noreply.github.com [Impeller] Adds impeller display list golden tests (flutter/engine#52690) 2024-05-09 zanderso@users.noreply.github.com Roll buildroot to 70a42312a688 (flutter/engine#52675) 2024-05-09 matanlurey@users.noreply.github.com When `et` is not attached to a terminal, still split lines for status updates. (flutter/engine#52681) 2024-05-09 30870216+gaaclarke@users.noreply.github.com updated analysis exclusion (flutter/engine#52699) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

agacemi · 2024-05-26T21:42:45Z

@zanderso any idea if this fix will be released as a patch of the current release 3.22 !

jmagman · 2024-05-28T23:44:10Z

This fix is in 3.23.0-10.0.pre which hasn't reached the beta channel yet. See https://github.com/flutter/flutter/blob/master/docs/releases/Hotfixes-to-the-Stable-Channel.md

JDongKhan · 2025-02-10T07:18:21Z

Any progress?

tacck · 2025-02-10T07:47:32Z

@JDongKhan
This PR was incorporated into 3.24.0.
https://docs.flutter.dev/release/release-notes/release-notes-3.24.0

tacck added 2 commits May 1, 2024 17:48

fix: App crash when use WebView with iOS VoiceOver

2143bc5

fix format

2637364

github-actions bot added the platform-ios label May 1, 2024

tacck changed the title ~~Fix ios voiceover webview~~ Fix App crash when use WebView with iOS VoiceOver May 1, 2024

tacck changed the title ~~Fix App crash when use WebView with iOS VoiceOver~~ [iOS] Fix App crash when use WebView with iOS VoiceOver May 1, 2024

tacck added 3 commits May 2, 2024 09:32

Merge branch 'main' into fix_ios_voiceover_webview

558f780

fix lint issue

14c4c17

Merge branch 'main' into fix_ios_voiceover_webview

bb481c3

tacck marked this pull request as ready for review May 2, 2024 07:52

jmagman reviewed May 2, 2024

View reviewed changes

tacck added 4 commits May 7, 2024 10:17

Merge branch 'flutter:main' into fix_ios_voiceover_webview

8aa98bd

Fix test code to match object lifecycle

f1754af

Merge remote-tracking branch 'origin/fix_ios_voiceover_webview' into …

f062e25

…fix_ios_voiceover_webview

Fix indent

fc5447f

tacck requested a review from jmagman May 7, 2024 08:33

jmagman mentioned this pull request May 8, 2024

Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC #52535

Merged

jmagman approved these changes May 9, 2024

View reviewed changes

jmagman requested a review from hellohuanlin May 9, 2024 19:30

hellohuanlin approved these changes May 9, 2024

View reviewed changes

jmagman merged commit ea83156 into flutter:main May 10, 2024

jmagman added a commit to jmagman/engine that referenced this pull request May 10, 2024

Rebase with flutter#52484

baa72bc

engine-flutter-autoroll mentioned this pull request May 10, 2024

Roll Flutter Engine from c0917b14fc36 to 1ccd0c308b3a (10 revisions) flutter/flutter#148137

Merged

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

ea831560f [iOS] Fix App crash when use WebView with iOS VoiceOver (fl…

98b2b8e

…utter/engine#52484)

haitaoshangguan mentioned this pull request May 17, 2024

Disposing of a map when pin focused using accessibility crashes the app on iOS flutter/flutter#142892

Open

jmagman mentioned this pull request May 28, 2024

App crashes upon selecting iOS webview link when voice over is on flutter/flutter#140528

Closed

jmagman mentioned this pull request Feb 26, 2025

UIAccessibility -[NSObject(AXPrivCategory) _accessibilityIterateScrollParentsUsingBlock:includeSelf:] + 240 flutter/flutter#162224

Closed

	FLUTTER_ENGINE[arch=x86_64]=ios_debug_sim_unopt
	FLUTTER_ENGINE[arch=arm64]=ios_debug_sim_unopt_arm64
	FLUTTER_ENGINE=ios_debug_sim_unopt_arm64

[iOS] Fix App crash when use WebView with iOS VoiceOver #52484

[iOS] Fix App crash when use WebView with iOS VoiceOver #52484

Uh oh!

Conversation

tacck commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chinmaygarde commented May 2, 2024

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

tacck commented May 7, 2024

Uh oh!

jmagman commented May 8, 2024

Uh oh!

tacck commented May 9, 2024

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

hellohuanlin May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman May 9, 2024

Choose a reason for hiding this comment

Uh oh!

jmagman May 9, 2024

Choose a reason for hiding this comment

Uh oh!

agacemi commented May 26, 2024

Uh oh!

jmagman commented May 28, 2024

Uh oh!

JDongKhan commented Feb 10, 2025

Uh oh!

tacck commented Feb 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tacck commented May 1, 2024 •

edited

Loading

jmagman commented May 9, 2024 •

edited

Loading

hellohuanlin May 9, 2024 •

edited

Loading