Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction #30835

jason-simmons · 2022-01-13T03:02:33Z

PlatformViewIOS uses an AccessibilityBridge wrapper that calls
PlatformView::SetSemanticsEnabled whenever the AccessibilityBridge is created
or destroyed. However, the SetSemanticsEnabled call should not happen
during destruction of the PlatformView. SetSemanticsEnabled calls into a
Shell API, and the PlatformView is deleted while the Shell is being
destructed.

Fixes flutter/flutter#95844

gaaclarke

Looks good for the most part. I have a few comments. It bugs me a bit that we are using raw pointers outside of a smart pointer class. We can get rid of that raw pointer parameter with a unique_ptr. We could also get rid of get() by adding those methods to AccessibilityBridgeManager, but I'm not sure that better.

gaaclarke · 2022-01-13T17:38:00Z

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

+  class TestDelegate : public flutter::MockDelegate {
+   public:
+    void OnPlatformViewSetSemanticsEnabled(bool enabled) override {
+      set_semantics_enabled_called = true;


nit: this test would be a bit more robust if instead of having a bool here you had an int that increments. That way you could assert that the count is "1" instead of "2" after its deconstructed. This test would also pass if we somehow messed up setting the delegate.

gaaclarke · 2022-01-13T17:50:10Z

shell/platform/darwin/ios/platform_view_ios.h

+    void Enable(AccessibilityBridge* bridge);
+    void Disable();


I think "Set" and "Clear" would be a bit easier to understand. At face value it isn't clear why "Enable" takes a pointer as a parameter. (at a minimum a docstring could clear that up)

gaaclarke · 2022-01-13T17:51:50Z

shell/platform/darwin/ios/platform_view_ios.h

-    AccessibilityBridge* operator->() const noexcept { return accessibility_bridge_.get(); }
-    void reset(AccessibilityBridge* bridge = nullptr);
+    AccessibilityBridge* get() const noexcept { return accessibility_bridge_.get(); }
+    void Enable(AccessibilityBridge* bridge);


Now that this class isn't a smart pointer we should change the parameter to a unique_ptr to match Google C++ Style.

…uring destruction PlatformViewIOS uses an AccessibilityBridge wrapper that calls PlatformView::SetSemanticsEnabled whenever the AccessibilityBridge is created or destroyed. However, the SetSemanticsEnabled call should not happen during destruction of the PlatformView. SetSemanticsEnabled calls into a Shell API, and the PlatformView is deleted while the Shell is being destructed. Fixes flutter/flutter#95844

jason-simmons · 2022-01-13T20:09:18Z

Done - PTAL

gaaclarke

LGTM thanks!

gaaclarke · 2022-01-13T21:14:44Z

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

+  class TestDelegate : public flutter::MockDelegate {
+   public:
+    void OnPlatformViewSetSemanticsEnabled(bool enabled) override { set_semantics_enabled_calls++; }
+    int set_semantics_enabled_calls = 0;


nit: google c++ style says this should have a postfix _ (only structs have fields without them).

I was thinking that you'd show it called through once so the final assert would be 1, not 2, not a huge deal.

…uring destruction (flutter#30835)

…s APIs during destruction (flutter/engine#30835)

flutter/engine@fab1982...83d99a5 2022-01-14 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 35I2K_Bou... to V541xkYVr... (flutter/engine#30879) 2022-01-14 skia-flutter-autoroll@skia.org Roll Dart SDK from dcab0d0b2f6c to d0d4dbfc6e69 (1 revision) (flutter/engine#30878) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from 1f0e64acd621 to f260a297c68d (1 revision) (flutter/engine#30877) 2022-01-14 638538+chaselatta@users.noreply.github.com [fuchsia] stamp package with target api level (flutter/engine#30857) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from dd9e165ef7d0 to 1f0e64acd621 (1 revision) (flutter/engine#30876) 2022-01-14 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from BQ2l2096A... to bGW3xlB1D... (flutter/engine#30875) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from 7aec4b164e79 to dd9e165ef7d0 (1 revision) (flutter/engine#30874) 2022-01-14 skia-flutter-autoroll@skia.org Roll Dart SDK from 68ccd13498be to dcab0d0b2f6c (1 revision) (flutter/engine#30872) 2022-01-14 skia-flutter-autoroll@skia.org Roll Dart SDK from aa6fbac07951 to 68ccd13498be (1 revision) (flutter/engine#30870) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from 76e62d32d979 to 7aec4b164e79 (1 revision) (flutter/engine#30869) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from a6f2ebf30fea to 76e62d32d979 (1 revision) (flutter/engine#30868) 2022-01-14 skia-flutter-autoroll@skia.org Roll Dart SDK from ba044d5e9c03 to aa6fbac07951 (1 revision) (flutter/engine#30867) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from 7c80b2f08ead to a6f2ebf30fea (1 revision) (flutter/engine#30863) 2022-01-14 dkwingsmt@users.noreply.github.com [Windows, Keyboard] Lift key event redispatching to KeyboardManagerWin32 (flutter/engine#30702) 2022-01-14 jason-simmons@users.noreply.github.com Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (flutter/engine#30835) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from 119fb6bb2568 to 7c80b2f08ead (1 revision) (flutter/engine#30862) 2022-01-14 skia-flutter-autoroll@skia.org Roll Dart SDK from b36d861d5487 to ba044d5e9c03 (1 revision) (flutter/engine#30861) 2022-01-14 skia-flutter-autoroll@skia.org Roll Skia from 62b32180c192 to 119fb6bb2568 (1 revision) (flutter/engine#30860) 2022-01-14 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from Uvw9UoGSm... to BQ2l2096A... (flutter/engine#30859) 2022-01-13 gw280@google.com Move SoftwareCanvasProvider into its own source file (flutter/engine#30856) 2022-01-13 skia-flutter-autoroll@skia.org Roll Skia from c505bdca9d60 to 62b32180c192 (1 revision) (flutter/engine#30855) 2022-01-13 gw280@google.com Create a DisplayList benchmarks dylib on iOS (flutter/engine#30833) 2022-01-13 gw280@google.com Don't retain the MTLTexture or MTLDevice in TestMetalContext (flutter/engine#30832) 2022-01-13 skia-flutter-autoroll@skia.org Roll Skia from 455e580b9c78 to c505bdca9d60 (1 revision) (flutter/engine#30852) 2022-01-13 74682667+chandarrengoog@users.noreply.github.com Roll buildroot to 7effd69. (flutter/engine#30851) 2022-01-13 scheglov@google.com Remove unused field initializing formal parameters. (flutter/engine#30822) 2022-01-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f1c98a571d1a to b36d861d5487 (1 revision) (flutter/engine#30850) 2022-01-13 skia-flutter-autoroll@skia.org Roll Skia from 759bc62a06d2 to 455e580b9c78 (2 revisions) (flutter/engine#30849) 2022-01-13 43054281+camsim99@users.noreply.github.com Remove usages of deprecated setSystemUiVisibility() (flutter/engine#29493) 2022-01-13 skia-flutter-autoroll@skia.org Roll Skia from b21c4af0f670 to 759bc62a06d2 (3 revisions) (flutter/engine#30848) 2022-01-13 akbiggs@users.noreply.github.com [fuchsia] Switch from core-jit to core snapshots. (flutter/engine#30744) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 35I2K_BouXUN to V541xkYVrdUC fuchsia/sdk/core/mac-amd64 from Uvw9UoGSmIjy to bGW3xlB1DoAm

…s APIs during destruction (flutter/engine#30835)

* a193f08 [fuchsia] Switch from core-jit to core snapshots. (flutter/engine#30744) * 7e50462 Roll Skia from b21c4af0f670 to 759bc62a06d2 (3 revisions) (flutter/engine#30848) * 8db5038 Remove usages of deprecated setSystemUiVisibility() (flutter/engine#29493) * c05d0df Roll Skia from 759bc62a06d2 to 455e580b9c78 (2 revisions) (flutter/engine#30849) * 1fab2fb Roll Dart SDK from f1c98a571d1a to b36d861d5487 (1 revision) (flutter/engine#30850) * f9385e7 Remove unused field initializing formal parameters. (flutter/engine#30822) * b6e5d99 Roll buildroot to 7effd69. (flutter/engine#30851) * 742eaf8 Roll Skia from 455e580b9c78 to c505bdca9d60 (1 revision) (flutter/engine#30852) * d0f2beb Don't retain the MTLTexture or MTLDevice in TestMetalContext (flutter/engine#30832) * f121c1f Create a DisplayList benchmarks dylib on iOS (flutter/engine#30833) * 4499797 Roll Skia from c505bdca9d60 to 62b32180c192 (1 revision) (flutter/engine#30855) * fcf7458 Move SoftwareCanvasProvider into its own source file (flutter/engine#30856) * 794a833 Roll Fuchsia Mac SDK from Uvw9UoGSm... to BQ2l2096A... (flutter/engine#30859) * 4b32e1c Roll Skia from 62b32180c192 to 119fb6bb2568 (1 revision) (flutter/engine#30860) * facfa74 Roll Dart SDK from b36d861d5487 to ba044d5e9c03 (1 revision) (flutter/engine#30861) * f6613c9 Roll Skia from 119fb6bb2568 to 7c80b2f08ead (1 revision) (flutter/engine#30862) * fb3ee7f Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (flutter/engine#30835) * 073e6c5 [Windows, Keyboard] Lift key event redispatching to KeyboardManagerWin32 (flutter/engine#30702) * 88e67a2 Roll Skia from 7c80b2f08ead to a6f2ebf30fea (1 revision) (flutter/engine#30863) * 1f4e7fa Roll Dart SDK from ba044d5e9c03 to aa6fbac07951 (1 revision) (flutter/engine#30867) * 3fbd427 Roll Skia from a6f2ebf30fea to 76e62d32d979 (1 revision) (flutter/engine#30868) * b6db081 Roll Skia from 76e62d32d979 to 7aec4b164e79 (1 revision) (flutter/engine#30869) * b92fd27 Roll Dart SDK from aa6fbac07951 to 68ccd13498be (1 revision) (flutter/engine#30870) * 8961366 Roll Dart SDK from 68ccd13498be to dcab0d0b2f6c (1 revision) (flutter/engine#30872) * 18ea2ce Roll Skia from 7aec4b164e79 to dd9e165ef7d0 (1 revision) (flutter/engine#30874) * 9d660d9 Roll Fuchsia Mac SDK from BQ2l2096A... to bGW3xlB1D... (flutter/engine#30875) * ad68b1b Roll Skia from dd9e165ef7d0 to 1f0e64acd621 (1 revision) (flutter/engine#30876) * b61a6f5 [fuchsia] stamp package with target api level (flutter/engine#30857) * 5787489 Roll Skia from 1f0e64acd621 to f260a297c68d (1 revision) (flutter/engine#30877) * 87ba2d8 Roll Dart SDK from dcab0d0b2f6c to d0d4dbfc6e69 (1 revision) (flutter/engine#30878) * 83d99a5 Roll Fuchsia Linux SDK from 35I2K_Bou... to V541xkYVr... (flutter/engine#30879) * 50adf4c Revert "Remove usages of deprecated setSystemUiVisibility()" (flutter/engine#30880)

…uring destruction (flutter#30835)

…0918) * Impl and test (#30488) * Fix missing shcore.dll error on Windows 7 (#30699) * Remove glitch when displaying platform views (#30724) * Win32: Fix Korean text input (#30805) Fixes an issue with Korean IMEs wherein a text input state update may be sent to the framework that misleads the framework into assuming that IME composing has ended. When inputting Korean text, characters are built up keystroke by keystroke until the point that either: * the user presses space/enter to terminate composing and commit the character, or; * the user presses a key such that the character currently being composed cannot be modified further, and the IME determines that the user has begun composing the next character. The following is an example sequence of events for the latter case: 1. User presses ㅂ. GCS_COMPSTR event received with ㅂ. Embedder sends state update to framework. 2. User presses ㅏ. GCS_COMPSTR event received with 바. Embedder sends state update to framework. 3. User presses ㄴ. GCS_COMPSTR event received with 반. Embedder sends state update to framework. 4. User presses ㅏ. At this point, the current character being composed (반) cannot be modified in a meaningful way, and the IME determines that the user is typing 바 followed by 나. GCS_RESULTSTR event received with 바, immediately followed by GCS_COMPSTR event with 나. In step 4, we previously sent two events to the framework, one immediately after the other: * GCS_RESULTSTR triggers the text input model to commit the current composing region to the string under edit. This causes the composing region to collapse to an empty range. * GCS_COMPSTR triggers the text input model to insert the new composing character and set the composing region to that character. Conceptually, this is an atomic operation. The fourth keystroke causes the 반 character to be broken into two (바 and ㄴ) and the latter to be modified to 나. From the user's point of view, as well as from the IME's point of view, the user has NOT stopped composing, and the composing region has simply moved on to the next character. Flutter has no concept of whether the user is composing or not other that whether a non-empty composing region exists. As such, sending a state update after the GCS_RESULTSTR event misleads the framework into believing that composing has ended. This triggers a serious bug: Text fields with input formatters applied do not perform input formatting updates while composing is active; instead they wait until composing has ended to apply any formatting. The previous behaviour would thus trigger input formatters to be applied each time the user input caused a new character to be input. This has the add-on negative effect that once formatting has been applied, it sends an update back to the embedder so that the native OS text input state can be updated. However, since the GCS_RESULTSTR event is _immediately_ followed by a GCS_COMPSTR, the state has changed in the meantime, and the embedder is left processing an update (the intermediate state sent after the GCS_RESULTSTR) which is now out of date (i.e. missing the new state from the GCS_COMPSTR event). Since GCS_RESULTR events are always immediately followed by a subsequent GCS_COMPSTR (in the case where composing continues) or a WM_IME_ENDCOMPOSITION (in the case where composing is finished), and because the event handlers for both of those send updated state to the framework, this change eliminates sending the (intermediate) state in response to GCS_COMPSTR events. Issue: flutter/flutter#96209 (full fix) Issue: flutter/flutter#88645 (partial fix) * Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (#30835) * Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (#30835) Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com> Co-authored-by: Chris Bracken <chris@bracken.jp> Co-authored-by: Emmanuel Garcia <egarciad@google.com> Co-authored-by: Jason Simmons <jason-simmons@users.noreply.github.com>

…uring destruction (flutter#30835)

jmagman

Sorry wrong PR please ignore.

* a193f08 [fuchsia] Switch from core-jit to core snapshots. (flutter/engine#30744) * 7e50462 Roll Skia from b21c4af0f670 to 759bc62a06d2 (3 revisions) (flutter/engine#30848) * 8db5038 Remove usages of deprecated setSystemUiVisibility() (flutter/engine#29493) * c05d0df Roll Skia from 759bc62a06d2 to 455e580b9c78 (2 revisions) (flutter/engine#30849) * 1fab2fb Roll Dart SDK from f1c98a571d1a to b36d861d5487 (1 revision) (flutter/engine#30850) * f9385e7 Remove unused field initializing formal parameters. (flutter/engine#30822) * b6e5d99 Roll buildroot to 7effd69. (flutter/engine#30851) * 742eaf8 Roll Skia from 455e580b9c78 to c505bdca9d60 (1 revision) (flutter/engine#30852) * d0f2beb Don't retain the MTLTexture or MTLDevice in TestMetalContext (flutter/engine#30832) * f121c1f Create a DisplayList benchmarks dylib on iOS (flutter/engine#30833) * 4499797 Roll Skia from c505bdca9d60 to 62b32180c192 (1 revision) (flutter/engine#30855) * fcf7458 Move SoftwareCanvasProvider into its own source file (flutter/engine#30856) * 794a833 Roll Fuchsia Mac SDK from Uvw9UoGSm... to BQ2l2096A... (flutter/engine#30859) * 4b32e1c Roll Skia from 62b32180c192 to 119fb6bb2568 (1 revision) (flutter/engine#30860) * facfa74 Roll Dart SDK from b36d861d5487 to ba044d5e9c03 (1 revision) (flutter/engine#30861) * f6613c9 Roll Skia from 119fb6bb2568 to 7c80b2f08ead (1 revision) (flutter/engine#30862) * fb3ee7f Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (flutter/engine#30835) * 073e6c5 [Windows, Keyboard] Lift key event redispatching to KeyboardManagerWin32 (flutter/engine#30702) * 88e67a2 Roll Skia from 7c80b2f08ead to a6f2ebf30fea (1 revision) (flutter/engine#30863) * 1f4e7fa Roll Dart SDK from ba044d5e9c03 to aa6fbac07951 (1 revision) (flutter/engine#30867) * 3fbd427 Roll Skia from a6f2ebf30fea to 76e62d32d979 (1 revision) (flutter/engine#30868) * b6db081 Roll Skia from 76e62d32d979 to 7aec4b164e79 (1 revision) (flutter/engine#30869) * b92fd27 Roll Dart SDK from aa6fbac07951 to 68ccd13498be (1 revision) (flutter/engine#30870) * 8961366 Roll Dart SDK from 68ccd13498be to dcab0d0b2f6c (1 revision) (flutter/engine#30872) * 18ea2ce Roll Skia from 7aec4b164e79 to dd9e165ef7d0 (1 revision) (flutter/engine#30874) * 9d660d9 Roll Fuchsia Mac SDK from BQ2l2096A... to bGW3xlB1D... (flutter/engine#30875) * ad68b1b Roll Skia from dd9e165ef7d0 to 1f0e64acd621 (1 revision) (flutter/engine#30876) * b61a6f5 [fuchsia] stamp package with target api level (flutter/engine#30857) * 5787489 Roll Skia from 1f0e64acd621 to f260a297c68d (1 revision) (flutter/engine#30877) * 87ba2d8 Roll Dart SDK from dcab0d0b2f6c to d0d4dbfc6e69 (1 revision) (flutter/engine#30878) * 83d99a5 Roll Fuchsia Linux SDK from 35I2K_Bou... to V541xkYVr... (flutter/engine#30879) * 50adf4c Revert "Remove usages of deprecated setSystemUiVisibility()" (flutter/engine#30880)

jason-simmons requested a review from gaaclarke January 13, 2022 03:02

flutter-dashboard bot added the platform-ios label Jan 13, 2022

gaaclarke reviewed Jan 13, 2022

View reviewed changes

jason-simmons force-pushed the bug_95844 branch from b347278 to 5417ce5 Compare January 13, 2022 20:00

jason-simmons force-pushed the bug_95844 branch from 5417ce5 to 8318d1b Compare January 13, 2022 20:08

gaaclarke approved these changes Jan 13, 2022

View reviewed changes

jason-simmons 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 Jan 14, 2022

fluttergithubbot merged commit fb3ee7f into flutter:main Jan 14, 2022

JsouLiang pushed a commit to JsouLiang/engine that referenced this pull request Jan 14, 2022

Ensure that PlatformViewIOS does not call into Shell semantics APIs d…

a85f986

…uring destruction (flutter#30835)

engine-flutter-autoroll mentioned this pull request Jan 14, 2022

Roll Engine from fab19829949d to 83d99a58097b (31 revisions) flutter/flutter#96673

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 14, 2022

fb3ee7f Ensure that PlatformViewIOS does not call into Shell semantic…

746810a

…s APIs during destruction (flutter/engine#30835)

iskakaushik mentioned this pull request Jan 15, 2022

Roll Engine from fab19829949d to 83d99a58097b (31 revisions) flutter/flutter#96687

Closed

zhongwuzw mentioned this pull request Jan 17, 2022

[Production][iOS]: EXC_BAD_ACCESS - UISystemKeyboardDockController - Attempted to dereference garbage pointer flutter/flutter#96348

Closed

engine-flutter-autoroll mentioned this pull request Jan 18, 2022

Roll Engine from fab19829949d to 50adf4c1f79e (32 revisions) flutter/flutter#96778

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2022

fb3ee7f Ensure that PlatformViewIOS does not call into Shell semantic…

a96712e

…s APIs during destruction (flutter/engine#30835)

itsjustkevin pushed a commit to itsjustkevin/engine that referenced this pull request Jan 19, 2022

Ensure that PlatformViewIOS does not call into Shell semantics APIs d…

3726d1e

…uring destruction (flutter#30835)

Levi-Lesches mentioned this pull request Jan 19, 2022

Crashed: com.apple.main-thread EXC_BAD_ACCESS KERN_INVALID_ADDRESS flutter/flutter#95568

Closed

mkay-42069 mentioned this pull request Jan 20, 2022

Flutter 2.8.1 crashed on iOS because of EXC_BAD_ACCESS KERN_INVALID_ADDRESS flutter/flutter#96851

Closed

fbcouch pushed a commit to twinsunllc/engine that referenced this pull request Jan 27, 2022

Ensure that PlatformViewIOS does not call into Shell semantics APIs d…

7c7c811

…uring destruction (flutter#30835)

jmagman reviewed Mar 1, 2022

View reviewed changes

cailianqing mentioned this pull request Mar 8, 2022

Flutter 2.8.1 background Crash flutter/flutter#99536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction #30835

Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction #30835

jason-simmons commented Jan 13, 2022 •

edited

Loading

gaaclarke left a comment

gaaclarke Jan 13, 2022

gaaclarke Jan 13, 2022 •

edited

Loading

gaaclarke Jan 13, 2022

jason-simmons commented Jan 13, 2022

gaaclarke left a comment

gaaclarke Jan 13, 2022

jmagman left a comment •

edited

Loading

Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction #30835

Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction #30835

Conversation

jason-simmons commented Jan 13, 2022 • edited Loading

gaaclarke left a comment

Choose a reason for hiding this comment

gaaclarke Jan 13, 2022

Choose a reason for hiding this comment

gaaclarke Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

gaaclarke Jan 13, 2022

Choose a reason for hiding this comment

jason-simmons commented Jan 13, 2022

gaaclarke left a comment

Choose a reason for hiding this comment

gaaclarke Jan 13, 2022

Choose a reason for hiding this comment

jmagman left a comment • edited Loading

Choose a reason for hiding this comment

jason-simmons commented Jan 13, 2022 •

edited

Loading

gaaclarke Jan 13, 2022 •

edited

Loading

jmagman left a comment •

edited

Loading