remove obsolete check on FlutterPlatformViewsController::OnCreate #19819

dnfield · 2020-07-16T20:00:21Z

Description

When initially landed (92944f7), this check made sense. After #9075 we no longer need to touch the flutter_view_ directly in OnCreate.

A recent change to lifecycle logic means that we sometimes get these calls (particularly on application relaunch for reasons I don't grasp) in different orders sometimes. Specifically, we no longer let viewDidLayoutSubviews create the surface unless the application is active - and sometimes get to that point (it's called before applicationBecameActive).

I'm not able to make this fail in local app that actually calls it in this order. I have to think about how to test this.

Related Issues

Fixes flutter/flutter#23787
Fixes flutter/flutter#61584

Tests

I added the following tests:

Test that we can successfully create a platform view without a flutter_view_.

fluttergithubbot · 2020-07-16T20:00:24Z

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-07-16T20:25:58Z

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

@@ -122,15 +122,6 @@
 }

 void FlutterPlatformViewsController::OnCreate(FlutterMethodCall* call, FlutterResult& result) {
-  if (!flutter_view_.get()) {


Would this race condition be moved to a different part of the code after this change? What about ensuring that flutter_view_ isn't null in the other parts of the code where it's used?

Based on my own auditing of the code, no. We're no longer creating a FlutterView here, we're creating a different UI view that's delegating based on the VC rather than the View.

+1. I think we should make sure we are confident to say that the flutter_view_ is ready when the platform view is going to be displayed.(when Rasterizer:Draw happens)
What about we assert that in CompositeWithParams? Or even earlier in PrerollCompositeEmbeddedView

You can't have a rasterizer without a surface (which the Flutter View creates).

Right, I meant that since we are confident that if the Rasterizer:Draw is called, the flutter_view_ should not nil, we can assert the flutter_view_ in either CompositeWithParams or PrerollCompositeEmbeddedView.

And the animator won't be started unless you have a surface either - so there won't be any composition/preroll calls without a surface (-> view)

gaaclarke

I read through the code for FlutterPlatformViewsController and didn't see an obvious crash that could result from flutter_view_ being nil. It is usually used in identity comparisons and the target for objc messages.

Amir said we could get rid of the error when flutter/flutter#23787 is addressed, which it isn't addressed yet officially, fwiw.

I find this hard to reason about since we can't know when flutter_view_ is getting set by just reading the code. At some point it shouldn't be nil right? It seems like we should push this assert down lower to things logic that assumes it's not nil.

dnfield · 2020-07-16T20:32:42Z

I've written a test case that passes in headless mode. I'm not clear on what's left toa ddress for that.

dnfield · 2020-07-16T20:34:01Z

I'm hoping @cyanglaz can confirm, but I believe some of his previous work has actually fixed #23787 indirectly. I think my test case proves that.

cyanglaz · 2020-07-16T20:48:05Z

@dnfield you are right. We are not relying on the FlutterView when creating the PlatformView. We only needed the FlutterView when we attach the PlatformView to it.
I saw that there is one instance that we are replying on the bounds of the FlutterView to determine the size of the Overlay: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L637
Though I think this line is not necessary as the overlay's frame should be later determined by the RTree work @blasten has done.

dnfield · 2020-07-16T20:53:35Z

The GetLayer call happens from SubmitFrame, which also only is called if the animator is enabled and the rasterizer is working - which require a view/surface. I thnk that should be safe, but happy to add some DCHECKs to it if that would help.

dnfield · 2020-07-16T20:54:39Z

@godofredoc @digiter - I'm not sure why the mac builds are failing here. It's https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8874586729338893104/+/steps/lint_test/0/stdout - seems like Xcode might not be installed on teh bot(s)?

cyanglaz · 2020-07-16T20:57:28Z

The GetLayer call happens from SubmitFrame, which also only is called if the animator is enabled and the rasterizer is working - which require a view/surface. I thnk that should be safe, but happy to add some DCHECKs to it if that would help.

A DCheck in any of the places sounds good. It might help future debugging.

dnfield · 2020-07-16T21:01:25Z

Sprinkled those into the methods that seem to need them :)

cyanglaz · 2020-07-16T21:08:53Z

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

@@ -472,6 +466,7 @@
 bool FlutterPlatformViewsController::SubmitFrame(GrContext* gr_context,
                                                 std::shared_ptr<IOSContext> ios_context,
                                                 std::unique_ptr<SurfaceFrame> frame) {
+  FML_DCHECK(flutter_view_);


I think this one is good enough, I'm ok with removing the other ones.

My thought is that these are free outside debug mode, and this will help clarify things if we refactor any of this in the future. WDYT?

Sure, SG! We probably don't need the one in PrerollCompositeEmbeddedView, as it seems this method doesn't use flutter_view_ at all.

cyanglaz

LGTM

dnfield · 2020-07-16T22:36:55Z

Engine redness is due to bad recipe land that I've reverted. I'm going to land this on red to kick the build.

…eate (flutter/engine#19819)

Remove obsolete check on FlutterPlatformViewsController::OnCreate (flutter#19819)

* Cherry pick 3d104f4 Remove obsolete check on FlutterPlatformViewsController::OnCreate (#19819) * Migrating web_analysis from Cirrus to LUCI (PART 1) (#19726) * explicitly check out framework rc branch Co-authored-by: nturgut <nurhan@google.com> Co-authored-by: Christopher Fujino <christopherfujino@gmail.com>

remove obsolete check

a9aa12a

dnfield requested review from chinmaygarde and cyanglaz July 16, 2020 20:00

fluttergithubbot added the platform-ios label Jul 16, 2020

googlebot added the cla: yes label Jul 16, 2020

dnfield requested a review from gaaclarke July 16, 2020 20:00

blasten reviewed Jul 16, 2020

View reviewed changes

gaaclarke reviewed Jul 16, 2020

View reviewed changes

test

f695ddc

FML_DCHECK flutter_view_

bced7fa

cyanglaz reviewed Jul 16, 2020

View reviewed changes

ci

86edd65

cyanglaz approved these changes Jul 16, 2020

View reviewed changes

Update FlutterPlatformViews.mm

be9c95a

dnfield 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 Jul 16, 2020

dnfield merged commit 3d104f4 into flutter:master Jul 16, 2020

dnfield deleted the ios_fix branch July 16, 2020 22:37

engine-flutter-autoroll mentioned this pull request Jul 17, 2020

Roll Engine from 1493883bf0fc to d32782863249 (18 revisions) flutter/flutter#61743

Merged

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

3d104f4 remove obsolete check on FlutterPlatformViewsController::OnCr…

9808794

…eate (flutter/engine#19819)

dnfield added a commit to dnfield/engine that referenced this pull request Jul 17, 2020

Cherry pick 3d104f4

099641e

Remove obsolete check on FlutterPlatformViewsController::OnCreate (flutter#19819)

dnfield mentioned this pull request Jul 17, 2020

Cherry pick 3d104f43649d333492dfa792968c8a09af93a84c #19848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove obsolete check on FlutterPlatformViewsController::OnCreate #19819

remove obsolete check on FlutterPlatformViewsController::OnCreate #19819

dnfield commented Jul 16, 2020 •

edited

Loading

fluttergithubbot commented Jul 16, 2020

blasten Jul 16, 2020

dnfield Jul 16, 2020

cyanglaz Jul 16, 2020

dnfield Jul 16, 2020

cyanglaz Jul 16, 2020

dnfield Jul 16, 2020

gaaclarke left a comment

dnfield commented Jul 16, 2020

dnfield commented Jul 16, 2020

cyanglaz commented Jul 16, 2020

dnfield commented Jul 16, 2020

dnfield commented Jul 16, 2020

cyanglaz commented Jul 16, 2020

dnfield commented Jul 16, 2020

cyanglaz Jul 16, 2020

dnfield Jul 16, 2020

cyanglaz Jul 16, 2020

dnfield Jul 16, 2020

cyanglaz left a comment

dnfield commented Jul 16, 2020

remove obsolete check on FlutterPlatformViewsController::OnCreate #19819

remove obsolete check on FlutterPlatformViewsController::OnCreate #19819

Conversation

dnfield commented Jul 16, 2020 • edited Loading

Description

Related Issues

Tests

fluttergithubbot commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

dnfield commented Jul 16, 2020

dnfield commented Jul 16, 2020

cyanglaz commented Jul 16, 2020

dnfield commented Jul 16, 2020

dnfield commented Jul 16, 2020

cyanglaz commented Jul 16, 2020

dnfield commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

dnfield commented Jul 16, 2020

dnfield commented Jul 16, 2020 •

edited

Loading