[platform_view] Only dispose view when it is removed from the composition order #41521

cyanglaz · 2023-04-26T18:46:49Z

DisposeView happens every frame before laying out PlatformViews, with the order specified in composition_order_. Because view_to_dispose_ is determined on UI thread and composition_order_ is determined on the platform thread, there could be a race if the dart code on the UI thread runs faster than the rasterizer on the Platform thread, causing a view in view_to_dispose_ is still in the composition_order_.

This PR delays the views in the composition_order_ being disposed and wait for the next frame to dispose them.

fixes flutter/flutter#125223

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.

cyanglaz · 2023-04-26T18:47:26Z

@saintmac-google Do you mind to test this patch again :) thank you

saintmac-google

tested on our app, it works (also with std::move)

saintmac-google · 2023-04-26T20:42:41Z

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

  }
-  views_to_dispose_.clear();
+
+  views_to_dispose_ = views_to_dispose_next_frame;


this could be std::move(views_to_dispose_next_frame)

# This is the 1st commit message: fix remove from slice test # This is the commit message flutter#2: remove comment # This is the commit message flutter#3: dispose view after it is removed from the compositon order

cyanglaz · 2023-04-28T05:41:38Z

Friendly ping @hellohuanlin :)

hellohuanlin · 2023-04-28T17:53:55Z

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

@@ -867,7 +867,14 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,

  FML_DCHECK([[NSThread currentThread] isMainThread]);


thx for the chat just now. just to check if i understand it correctly (let me know if i miss anything):

Dart layer sends separate messages (info about views_to_dispose and composition_order) to the engine. The composition_order contains what should be rendered on the current frame, and views_to_dispose contains views that needs to be disposed. We need separate messages because a platform view could be just hidden and not disposed, so it won't show up in composition_order.

The crash happens when views_to_dispose message is delivered earlier than the new composition_order (due to the async nature of the message channel). So when rendering the current frame, a platform view is already disposed, but it still exists in composition_order. The fix here is simply to delay the views_to_dispose if it's still part of composition_order, and try again next frame. Alternative fix is to remove the view from composition_order, but we decided that composition_order should be the source of truth of the current frame.

…he composition order (flutter/engine#41521)

…125714) flutter/engine@98b6fab...2a84ea5 2023-04-28 [email protected] Revert "Replace deprecated and internal Skia EncodedImage calls with public versions" (flutter/engine#41595) 2023-04-28 [email protected] Roll buildroot to c96c9d4641714301fab450a5f3b0f3c42712e1e3 (flutter/engine#41589) 2023-04-28 [email protected] [platform_view] Only dispose view when it is removed from the composition order (flutter/engine#41521) 2023-04-28 [email protected] Determine lifecycle by looking at window focus also (flutter/engine#41094) 2023-04-28 [email protected] Increase timeout for clang-tidy on mac (flutter/engine#41588) 2023-04-28 [email protected] [Impeller] Always enable validation for goldens (flutter/engine#41574) 2023-04-28 [email protected] fuchsia: do not read version_history.json (flutter/engine#41585) 2023-04-28 [email protected] Roll Dart SDK from c7160d4ea0b7 to 8e9bf2bb9878 (5 revisions) (flutter/engine#41586) 2023-04-28 [email protected] Replace deprecated and internal Skia EncodedImage calls with public versions (flutter/engine#41368) 2023-04-28 [email protected] Roll Skia from 05d09f28250a to 9867fa253064 (18 revisions) (flutter/engine#41584) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…tion order (flutter#41521) DisposeView happens every frame before laying out PlatformViews, with the order specified in `composition_order_`. Because `view_to_dispose_` is determined on UI thread and `composition_order_` is determined on the platform thread, there could be a race if the dart code on the UI thread runs faster than the rasterizer on the Platform thread, causing a view in `view_to_dispose_` is still in the `composition_order_`. This PR delays the views in the `composition_order_` being disposed and wait for the next frame to dispose them. fixes flutter/flutter#125223 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

minseok-jeong-gn · 2023-11-22T07:52:25Z

Is this only for iOS?

Can you guys check below issue too?

flutter/flutter#112542

I think in android side, app crash still happening.

Only iOS side after flutter stable version 3.13.x upgrade, app doesn't crash.

But in android still happening..

Please help me to examine the above issue...
(cc: @cyanglaz )

flutter-dashboard bot added the platform-ios label Apr 26, 2023

cyanglaz marked this pull request as ready for review April 26, 2023 18:46

cyanglaz mentioned this pull request Apr 26, 2023

[platform_view] Remove the views from composition_order if they are being disposed #41371

Closed

8 tasks

cyanglaz requested a review from hellohuanlin April 26, 2023 18:47

saintmac-google reviewed Apr 26, 2023

View reviewed changes

Chris Yang added 2 commits April 27, 2023 22:40

# This is a combination of 3 commits.

2eaeffe

# This is the 1st commit message: fix remove from slice test # This is the commit message flutter#2: remove comment # This is the commit message flutter#3: dispose view after it is removed from the compositon order

std move

a113c4b

cyanglaz force-pushed the platform_view_crash_2 branch from 4daaaed to a113c4b Compare April 28, 2023 05:41

renamefix

c8aa713

hellohuanlin approved these changes Apr 28, 2023

View reviewed changes

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2023

auto-submit bot merged commit d6bc5c5 into flutter:main Apr 28, 2023

engine-flutter-autoroll mentioned this pull request Apr 28, 2023

Roll Flutter Engine from 98b6fabc66bb to d6bc5c55d4dd (8 revisions) flutter/flutter#125706

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2023

d6bc5c55d [platform_view] Only dispose view when it is removed from t…

0c0095d

…he composition order (flutter/engine#41521)

engine-flutter-autoroll mentioned this pull request Apr 28, 2023

Roll Flutter Engine from 98b6fabc66bb to 4b380b9e52bf (9 revisions) flutter/flutter#125708

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2023

d6bc5c55d [platform_view] Only dispose view when it is removed from t…

ab4f122

…he composition order (flutter/engine#41521)

engine-flutter-autoroll mentioned this pull request Apr 28, 2023

Roll Flutter Engine from 98b6fabc66bb to 2a84ea55e4ef (10 revisions) flutter/flutter#125714

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 28, 2023

d6bc5c55d [platform_view] Only dispose view when it is removed from t…

c3899b3

…he composition order (flutter/engine#41521)

cyanglaz deleted the platform_view_crash_2 branch April 28, 2023 23:13

cyanglaz mentioned this pull request Sep 28, 2023

Crash -[__NSCFString characterAtIndex:].cold.1 in FlutterPlatformViews.mm flutter/flutter#134106

Closed

2 tasks

minseok-jeong-gn mentioned this pull request Nov 29, 2023

[Android, Engine issue] 지도 위젯이 있는 페이지에서 다른 페이지를 push할 경우, 트랜지션 후 지도 페이지가 잠시 깜빡이듯 노출됨 note11g/flutter_naver_map#84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[platform_view] Only dispose view when it is removed from the composition order #41521

[platform_view] Only dispose view when it is removed from the composition order #41521

Uh oh!

cyanglaz commented Apr 26, 2023

Uh oh!

cyanglaz commented Apr 26, 2023

Uh oh!

saintmac-google left a comment

Uh oh!

saintmac-google Apr 26, 2023

Uh oh!

cyanglaz commented Apr 28, 2023

Uh oh!

hellohuanlin Apr 28, 2023

Uh oh!

cyanglaz Apr 28, 2023

Uh oh!

minseok-jeong-gn commented Nov 22, 2023 •

edited

Loading

Uh oh!

Uh oh!

		@@ -867,7 +867,14 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,

		FML_DCHECK([[NSThread currentThread] isMainThread]);

[platform_view] Only dispose view when it is removed from the composition order #41521

[platform_view] Only dispose view when it is removed from the composition order #41521

Uh oh!

Conversation

cyanglaz commented Apr 26, 2023

Pre-launch Checklist

Uh oh!

cyanglaz commented Apr 26, 2023

Uh oh!

saintmac-google left a comment

Choose a reason for hiding this comment

Uh oh!

saintmac-google Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Apr 28, 2023

Uh oh!

hellohuanlin Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

cyanglaz Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

minseok-jeong-gn commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

minseok-jeong-gn commented Nov 22, 2023 •

edited

Loading