Do not involve external_view_embedder in submit frame process if threads are not merged. #22275

cyanglaz · 2020-11-03T19:11:41Z

Alternatively, we can submit frame directly if threads aren't merged.

122f242 need to be cherry-picked before this commit can be cherry-picked. @zanderso
This PR also had conflicts with 0d5f2e9 and I had to rebase. Cherry-picking might be a little tricky

flutter-dashboard · 2020-11-03T19:11:43Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

blasten · 2020-11-03T19:27:07Z

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

 bool FlutterPlatformViewsController::SubmitFrame(GrDirectContext* gr_context,
                                                 std::shared_ptr<IOSContext> ios_context,
                                                 std::unique_ptr<SurfaceFrame> frame) {
+  if (views_to_dispose_.empty() && composition_order_.empty()) {


this won't fix the issue because composition_order_ is mutated by FlutterPlatformViewsController::PrerollCompositeEmbeddedView, PlatformViewLayer::Preroll (raster task queue), which still leaves the flutter_view_ race.

I see, the thread merger check below is probably better.

gaaclarke · 2020-11-03T21:15:30Z

shell/common/rasterizer.cc

      return raster_status;
    }
-    if (external_view_embedder != nullptr) {
+    if (external_view_embedder != nullptr && (!raster_thread_merger_ || raster_thread_merger_->IsMerged())) {


Is raster_thread_merge_ thread-safe? raster_thread_merger_->Enable() is called from the platform thread.

Looks like there are checks for raster_thread_merger_ being null on the platform thread while it is created on the raster thread, whilst not being atomic. That looks like a problem itself.

raster_thread_merge_ is always on raster task runner. raster_thread_merger_->IsMerged() is thread safe.

Looks like enabling the merger is protected by a mutex: https://github.com/gaaclarke/engine/blob/e1e90f2a2be3bbd61237cb794ec2e21ddfeace5e/fml/raster_thread_merger.cc#L101:L102

we still need to address the flutter_view_ race, no?

I think @blasten is right about the race condition. Even with the threads merged we could end up in a SubmitFrame with a null flutter_view_ which the assert says shouldn't' happen. FWIW I ran the code with a null value for flutter_view_ by removing the assert and nothing crashed, so it looks like objc handled the null checks fine. We can just double check that null is handled fine and remove that assert maybe.

gaaclarke

I talked to @cyanglaz, I think this is a good fix. I'm going to run a bit of local testing just to make sure. LGTM assuming it passes automated and some manual testing.

gaaclarke · 2020-11-03T21:48:16Z

shell/common/rasterizer.cc

+      // If raster_thread_merger_ is supported by the external_view_embedder,
+      // we make sure threads are merged before letting external_view_embedder
+      // to submit frame. Otherwise, fallback to `frame->Submit();`


I don't think the comment adds much to the code.

shell/common/rasterizer.cc

gaaclarke · 2020-11-03T22:06:14Z

I have a 100% repro crash on a branch of the engine. When I rebased that branch onto this one, the crash doesn't happen anymore 👍

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

cyanglaz · 2020-11-03T22:22:01Z

This is actually failing some tests that are counting number of calls for external_view_embedder->SubmitFrame.
I'm going to update those tests as well

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

gaaclarke · 2020-11-03T23:05:35Z

I tried to force the reproduction of the crash by delaying [FlutterViewController viewDidLayoutSubviews] to match what I was seeing.

diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
index 7f593583a..59c14ff3d 100644
--- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
+++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
@@ -923,6 +923,10 @@ static flutter::PointerData::DeviceKind DeviceKindFromTouchType(UITouch* touch)
 }
 
 - (void)viewDidLayoutSubviews {
+  [self performSelector:@selector(viewDidLayoutSubviewsReal) withObject:nil afterDelay:5.0];
+}
+
+- (void)viewDidLayoutSubviewsReal {
   CGRect viewBounds = self.view.bounds;
   CGFloat scale = [UIScreen mainScreen].scale;

This didn't crash because it appears the UI thread isn't presenting trees to be rendered. It is possible this crash is practically impossible to reproduce given the aggregate system. When looking at the rasterizer / platforms view controller we can see it's technically possible and it happens in my branch but my usage is atypical.

blasten

LGTM. Assuming there's test for flutter_view_ = nullptr case + shell_unittest.cc changes.

zanderso · 2020-11-05T16:28:33Z

Is the failure on the Mac iOS Engine presubmit related?

zanderso · 2020-11-05T16:30:37Z

Fixes flutter/flutter#69449

…ads are not merged.

blasten · 2020-11-05T23:09:59Z

shell/common/rasterizer_unittests.cc

+                                                /*device_pixel_ratio=*/2.0f);
+  bool result = pipeline->Produce().Complete(std::move(layer_tree));
+  EXPECT_TRUE(result);
+  std::function<bool(LayerTree&)> no_discard = [](LayerTree&) { return false; };


nit: this could just be auto no_discard

blasten · 2020-11-05T23:13:23Z

shell/common/shell_unittests.cc

+  ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount());

+  end_frame_latch.Wait();
  ASSERT_EQ(1, external_view_embedder->GetSubmittedFrameCount());


nit: is it possible to assert that threads are merged before this assert? Same where you have the comment // Threads not merged, can that be an assert?

Actually, we can't assert if threads are not merged here, the threads are merged at the end of the frame. So it is actually merged here.
My comment was misleading, it should say that during the frame, the threads are not merged, they are merged at the end of the frame.
Updated the comment, PTAL

cyanglaz

@gaaclarke @blasten Updated with reviews, PTAL

cyanglaz · 2020-11-06T01:10:24Z

shell/common/rasterizer_unittests.cc

      .WillRepeatedly(Return(&external_view_embedder));
+
+  auto surface_frame = std::make_unique<SurfaceFrame>(
+      nullptr, true, [](const SurfaceFrame&, SkCanvas*) { return true; });


cyanglaz · 2020-11-06T01:11:26Z

shell/common/rasterizer_unittests.cc

+                                                /*device_pixel_ratio=*/2.0f);
+  bool result = pipeline->Produce().Complete(std::move(layer_tree));
+  EXPECT_TRUE(result);
+  std::function<bool(LayerTree&)> no_discard = [](LayerTree&) { return false; };


cyanglaz · 2020-11-06T01:11:29Z

shell/common/rasterizer_unittests.cc

+  EXPECT_CALL(external_view_embedder, BeginFrame).Times(1);
+  EXPECT_CALL(external_view_embedder, SubmitFrame).Times(1);
+  EXPECT_CALL(external_view_embedder, EndFrame).Times(1);


cyanglaz · 2020-11-06T01:15:55Z

shell/common/shell_unittests.cc

+  ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount());

+  end_frame_latch.Wait();
  ASSERT_EQ(1, external_view_embedder->GetSubmittedFrameCount());


cyanglaz · 2020-11-06T01:23:47Z

shell/common/shell_unittests.cc

+  ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount());

+  end_frame_latch.Wait();
  ASSERT_EQ(1, external_view_embedder->GetSubmittedFrameCount());


Actually, we can't assert if threads are not merged here, the threads are merged at the end of the frame. So it is actually merged here.
My comment was misleading, it should say that during the frame, the threads are not merged, they are merged at the end of the frame.
Updated the comment, PTAL

cyanglaz · 2020-11-06T01:23:56Z

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

  flutterPlatformViewsController->Reset();
 }

+- (void)testFlutterPlatformViewControllerSubmitFrameWithoutFlutterViewNotCrashing {


blasten · 2020-11-06T18:12:58Z

shell/common/shell_unittests.cc

-        external_view_embedder->UpdatePostPrerollResult(
-            PostPrerollResult::kSuccess);
+        if (!raster_thread_merger_ref) {
+          raster_thread_merger_ref = raster_thread_merger;


For future improvements: this would be much easier if we were to create, and supply the thread merger in the test. Right now, this dependency is hardcoded in the rasterizer, which creates patterns like this.

blasten

LGTM

… if threads are not merged. (flutter/engine#22275)

)" (#22367) This reverts commit 0d5f2e9. Due to merge conflicts, this also reverts #22275: Revert "Do not involve external_view_embedder in submit frame process if threads are not merged. (#22275)" This reverts commit 016fbde.

… if threads are not merged. (flutter/engine#22275)

… if threads are not merged. #22275" (#22372)

* Update 1.22 engine to use Dart 2.10.4 * [iOS TextInput] Avoid Unnecessary UndateEditingClient Calls (#21303) * added unit tests to the rasterizer (#22282) * Reland "Do not involve external_view_embedder in submit frame process if threads are not merged. #22275" (#22372) Co-authored-by: LongCatIsLooong <[email protected]> Co-authored-by: gaaclarke <[email protected]> Co-authored-by: Chris Yang <[email protected]>

…ads are not merged. (flutter#22275)

…tter#22348)" (flutter#22367) This reverts commit 0d5f2e9. Due to merge conflicts, this also reverts flutter#22275: Revert "Do not involve external_view_embedder in submit frame process if threads are not merged. (flutter#22275)" This reverts commit 016fbde.

… if threads are not merged. flutter#22275" (flutter#22372)

flutter-dashboard bot added the platform-ios label Nov 3, 2020

google-cla bot added the cla: yes label Nov 3, 2020

cyanglaz requested review from blasten and gaaclarke November 3, 2020 19:12

blasten reviewed Nov 3, 2020

View reviewed changes

gaaclarke reviewed Nov 3, 2020

View reviewed changes

gaaclarke approved these changes Nov 3, 2020

View reviewed changes

gaaclarke mentioned this pull request Nov 3, 2020

Made flutter_view_ atomic #22259

Closed

13 tasks

cyanglaz requested a review from chinmaygarde November 3, 2020 21:45

cyanglaz changed the title ~~Submit frame directly if no platform views.~~ Do not involve external_view_embedder in submit frame process if threads are not merged. Nov 3, 2020

gaaclarke reviewed Nov 3, 2020

View reviewed changes

shell/common/rasterizer.cc Show resolved Hide resolved

cyanglaz force-pushed the escape branch from 1d6666d to 5229abd Compare November 3, 2020 22:00

cyanglaz commented Nov 3, 2020

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm Show resolved Hide resolved

blasten reviewed Nov 3, 2020

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm Show resolved Hide resolved

gaaclarke mentioned this pull request Nov 4, 2020

added unit tests to the rasterizer #22282

Merged

13 tasks

blasten approved these changes Nov 4, 2020

View reviewed changes

gaaclarke mentioned this pull request Nov 4, 2020

Skia crash in customer:money flutter/flutter#69642

Closed

cyanglaz force-pushed the escape branch from c351bd0 to 9846840 Compare November 5, 2020 01:17

zanderso requested review from blasten and gaaclarke November 5, 2020 16:29

zanderso mentioned this pull request Nov 5, 2020

platform views on by default introduced a race condition flutter/flutter#69449

Closed

Do not involve external_view_embedder in submit frame process if thre…

e1a9d81

…ads are not merged.

cyanglaz force-pushed the escape branch from 0125bc6 to e1a9d81 Compare November 5, 2020 17:05

blasten reviewed Nov 5, 2020

View reviewed changes

reviews

dbb7b63

cyanglaz commented Nov 6, 2020

View reviewed changes

Chris Yang added 2 commits November 6, 2020 09:15

Merge branch 'master' into escape

ed75ac6

merge mater

ce7e971

cyanglaz requested review from blasten and gaaclarke November 6, 2020 17:31

blasten reviewed Nov 6, 2020

View reviewed changes

blasten approved these changes Nov 6, 2020

View reviewed changes

cyanglaz 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 Nov 6, 2020

fluttergithubbot merged commit 016fbde into flutter:master Nov 6, 2020

cyanglaz deleted the escape branch November 6, 2020 19:07

engine-flutter-autoroll mentioned this pull request Nov 6, 2020

Roll Engine from cf376142ed5f to 016fbde1bb27 (2 revisions) flutter/flutter#69979

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020

016fbde Do not involve external_view_embedder in submit frame process…

abc028f

… if threads are not merged. (flutter/engine#22275)

cbracken mentioned this pull request Nov 6, 2020

Revert "Rasterizer is initialized with an external view embedder (#22348)" #22367

Merged

13 tasks

cyanglaz mentioned this pull request Nov 6, 2020

Reland "Do not involve external_view_embedder in submit frame process if threads are not merged. #22275" #22372

Merged

engine-flutter-autoroll mentioned this pull request Nov 6, 2020

Roll Engine from d21bb634e054 to 2440ad6d39a5 (8 revisions) flutter/flutter#69991

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020

016fbde Do not involve external_view_embedder in submit frame process…

b113335

… if threads are not merged. (flutter/engine#22275)

engine-flutter-autoroll mentioned this pull request Nov 7, 2020

Roll Engine from d21bb634e054 to e98408bc7d8b (13 revisions) flutter/flutter#70001

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 7, 2020

016fbde Do not involve external_view_embedder in submit frame process…

8273234

… if threads are not merged. (flutter/engine#22275)

engine-flutter-autoroll mentioned this pull request Nov 7, 2020

Roll Engine from d21bb634e054 to 7ab39127ee25 (16 revisions) flutter/flutter#70005

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 7, 2020

016fbde Do not involve external_view_embedder in submit frame process…

059f35d

… if threads are not merged. (flutter/engine#22275)

fluttergithubbot pushed a commit that referenced this pull request Nov 7, 2020

Reland "Do not involve external_view_embedder in submit frame process…

fb72023

… if threads are not merged. #22275" (#22372)

Sunbreak mentioned this pull request Nov 17, 2020

Platform views enabled by default flutter/flutter#44602

Closed

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Do not involve external_view_embedder in submit frame process if thre…

943657e

…ads are not merged. (flutter#22275)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Reland "Do not involve external_view_embedder in submit frame process…

68689fb

… if threads are not merged. flutter#22275" (flutter#22372)

Do not involve external_view_embedder in submit frame process if threads are not merged. #22275

Do not involve external_view_embedder in submit frame process if threads are not merged. #22275

Uh oh!

Conversation

cyanglaz commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Nov 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke commented Nov 3, 2020

Uh oh!

Uh oh!

cyanglaz commented Nov 3, 2020

Uh oh!

Uh oh!

gaaclarke commented Nov 3, 2020

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Nov 5, 2020

Uh oh!

zanderso commented Nov 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

cyanglaz commented Nov 3, 2020 •

edited

Loading