Fix issue viewdidload call while init FlutterViewController #16672

ngminhduong · 2020-02-19T02:27:30Z

Fix issue reported from ticket
flutter/flutter#50949

chinmaygarde

The existing behavior is to launch the engine only when UIKit attempts to load the Flutter view. With this patch, the engine will be loaded eagerly even when there is no need to display the same. If the application wants to launch the engine irrespective of the whether its view is being displayed, it can use the engine reference directly to launch the Flutter isolate.

dnfield

This will break existing usages. If you need this behavior we should probably just create a new initializer.

We should also have a test for it.

chinmaygarde

I apologize. I was incorrect with my previous comment.

With this patch, the engine will be loaded eagerly even when there is no need to display the same.

This is exactly what you are avoiding with this patch.

The patch looks good but please add a test.

dnfield · 2020-02-19T03:19:21Z

But now the viewcontroller won't get attached to the engine right?

chinmaygarde · 2020-02-19T03:25:14Z

But now the viewcontroller won't get attached to the engine right?

Yeah. You're right. We need to figure out why setting the view controller is implicitly loading the view. This seems like an issue where some other method missed the UIViewController.isViewLoaded check and directly accessed the view (implicitly loading it). We should figure out what that call is and fix that.

ngminhduong · 2020-02-19T03:32:42Z

This will break existing usages. If you need this behavior we should probably just create a new initializer.

Yeah. But I think that current initializer doesn't follow iOS app lifecycle. We should correct it.

ngminhduong · 2020-02-19T06:01:45Z

But now the viewcontroller won't get attached to the engine right?

Viewcontroller will attached to the engine in viewWillAppear

- (void)viewWillAppear:(BOOL)animated {
  TRACE_EVENT0("flutter", "viewWillAppear");

  if (_engineNeedsLaunch) {
    [_engine.get() launchEngine:nil libraryURI:nil];
    [_engine.get() setViewController:self];
    _engineNeedsLaunch = NO;
  }

  // Send platform settings to Flutter, e.g., platform brightness.
  [self onUserSettingsChanged:nil];

  // Only recreate surface on subsequent appearances when viewport metrics are known.
  // First time surface creation is done on viewDidLayoutSubviews.
  if (_viewportMetrics.physical_width) {
    [self surfaceUpdated:YES];
  }
  [[_engine.get() lifecycleChannel] sendMessage:@"AppLifecycleState.inactive"];

  [super viewWillAppear:animated];
}

Does it make something broken?

dnfield · 2020-02-19T06:19:04Z

Took a little more time to read through some code here.

I'm concerned that this change would lead to calling launchEngine on an already launched engine. The call is meant to be safe for re-entrancy, but it involves jumping around some threads even to check that it's already running.

I believe our problem is that when you set the owner view controller, we access its view, which will cause it to load,here:

engine/shell/platform/darwin/ios/platform_view_ios.mm

Line 69 in 80f73ee

    
           [static_cast<FlutterView*>(owner_controller.get().view) createSurface:gl_context_];

We should somehow instead only create the surface when the view is actually available, instead of forcing it to load there. That will avoid making unnecessary re-entrant calls to launchEngine, and also make it safe for users to call setViewController on the engine without forcing the VC to load.

dnfield · 2020-02-19T06:21:56Z

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

    _ongoingTouches = [[NSMutableSet alloc] init];

    [self performCommonViewControllerInitialization];
-    [engine setViewController:self];


So this line here is what's causing the view to load.

Yes. That right.

dnfield · 2020-02-19T06:24:55Z

A simpler way to handle this for now (without fixing the underlying problem of setting the VC causing it to load the view) would be to avoid setting the VC in the intializer, but conditionally setting it in viewWillAppear if the engine's view is nil.

That seems a little hacky though, and likely to break something in a difficult to understand way. A better resolution here should be to just have the platform view more lazily instantiate the surface it wants, only after some other action has caused the view to load (unless isViewLoaded is true).

ngminhduong · 2020-02-19T10:46:32Z

@dnfield, @chinmaygarde
I updated pull request with new approach.
Can you help to review it again.
Please review the solution.
I will update unittest later.
Thanks

dnfield · 2020-02-19T15:47:34Z

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

+  if (!_isAttchedView) {
+    [_engine.get() attachView];
+    _isAttchedView = YES;
+  }


Do we really need this guard? It seems like if we actually loaded a new instance of the view, we'd want to let the engine know about it to update the surface/a11y bridge.

But we would not want to let it know if the view was actually the same view.

I think that we this guard.
Make sure that we don't create another instance of ios_surface_ in platform_view_ios.mm
I think that this is the same with old logic.

That's fine. If this is a bug it's existing and we can address it separately

We should probably FML_DCHECK that owner controller is not nil here though

I added FML_DCHECK.
Thanks

dnfield · 2020-02-19T15:49:13Z

also /cc @gaaclarke (GitHub is slow right now and failing to let me add you as a reviewer).

gaaclarke · 2020-02-19T18:08:53Z

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

    _engineNeedsLaunch = NO;
  }

+  if (!_isAttchedView) {


Wouldn't this be more appropriate in loadView? Wouldn't that timing more match how it was previously and you wouldn't have to keep an extra bool _isViewAttached?

Will loadView ever get called more than once in the lifecycle of a UIViewController?

Talked offline - the answer should be no

It should only be called once. We should actually use viewDidLoad since it's a bit safer.

So need also call setViewController in viewDidLoad. Is that right?

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

gaaclarke

You can add a unit test here: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm

shell/platform/darwin/ios/platform_view_ios.mm

…d instead of viewWillAppear

ngminhduong · 2020-02-21T02:07:09Z

Hi @dnfield , @gaaclarke
I have some update.
Can you help to review again.

ngminhduong · 2020-02-24T16:15:41Z

Hi have anything issues should be fixed with this patch?
Can you help to review it.
Thanks,

…lutter/engine#16672)

* 7c612de Roll fuchsia/sdk/core/linux-amd64 from cXgMr... to cTw2C... (flutter/engine#16970) * 6cfa7fc fix shadows and mask filter blurs (flutter/engine#16963) * bfebadf Roll src/third_party/skia 012f8497802e..93a2a6b8badb (4 commits) (flutter/engine#16974) * 47963a5 Roll src/third_party/skia 93a2a6b8badb..74055566bd14 (2 commits) (flutter/engine#16981) * 98f9941 [fuchsia] fix broken flows when under high load (flutter/engine#16834) * fe051e0 Fix issue viewdidload call while init FlutterViewController (flutter/engine#16672) * 0ad54c2 [web] Fixes IE11 crash due to missing canvas ellipse support and font polyfill failure (flutter/engine#16965) * f6435de Roll fuchsia/sdk/core/mac-amd64 from J6ct_... to 95geB... (flutter/engine#16982) * 43971ca Roll src/third_party/skia 74055566bd14..54de2fa48d85 (3 commits) (flutter/engine#16983) * 45e61a6 Roll fuchsia/sdk/core/linux-amd64 from cTw2C... to K1wwe... (flutter/engine#16984) * 1ab5c36 Roll src/third_party/skia 54de2fa48d85..beaaf4700f50 (3 commits) (flutter/engine#16987) * e2c0454 remove 10s timeouts from tests (flutter/engine#16988) * dfc9c12 Roll src/third_party/skia beaaf4700f50..6e58290ba639 (9 commits) (flutter/engine#16990) * eddda80 fushia licenses fix (flutter/engine#16992) * c15f239 documented fluttertexture.h (flutter/engine#16950) * e1ba7a1 Roll src/third_party/skia 6e58290ba639..24a8e9e170f7 (5 commits) (flutter/engine#16996) * fc5963d [web] Engine integration test (flutter/engine#16930) * d323bac doxygen tooling updates and doxygen for FlutterCodecs.h (flutter/engine#16947) * 03ddc1d Started deleting .DS_Store files so licenses can run on mac os x. (flutter/engine#16998) * 30a8292 Roll src/third_party/skia 24a8e9e170f7..cf573d844da6 (4 commits) (flutter/engine#17004) * d031963 Roll fuchsia/sdk/core/mac-amd64 from 95geB... to hW33F... (flutter/engine#17006) * 41b371d Roll fuchsia/sdk/core/linux-amd64 from K1wwe... to FGMpI... (flutter/engine#17007) * 619acd5 Revert "fix shadows and mask filter blurs (#16963)" (flutter/engine#17008)

…16672) Co-authored-by: Aaron Clarke <[email protected]>

xster · 2020-04-03T21:14:05Z

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


 #pragma mark - UIViewController lifecycle notifications

- (void)viewWillAppear:(BOOL)animated {


I get that we moved a bunch of stuff out of PlatformViewIOS::SetOwnerViewController which is definitely the right thing to do. But I'm not sure I understood why the change from viewWillAppear to viewDidLoad. Strictly speaking, viewDidLoad doesn't signal intent. It'll be called even if you just do flutterViewController.view without ever trying to present the FlutterViewController and we'll end up with an IOSSurface and an AccessibilityBridge.

Is it just a convenient way of avoiding having to keep track of whether viewWillAppear was already called once per instance?

viewDidLoad means we have a valid view we can use. It's up to the developer to make sure they avoid loading the view earlier than they should, right?

probably. The side effects is not very obvious however. Might as well take away another foot gun.

googlebot added the cla: yes label Feb 19, 2020

ngminhduong requested a review from dnfield February 19, 2020 02:44

chinmaygarde suggested changes Feb 19, 2020

View reviewed changes

dnfield suggested changes Feb 19, 2020

View reviewed changes

chinmaygarde reviewed Feb 19, 2020

View reviewed changes

chinmaygarde self-requested a review February 19, 2020 03:17

dnfield reviewed Feb 19, 2020

View reviewed changes

ngminhduong force-pushed the dev/bugfix_50949 branch from b2c7cda to 8e669bf Compare February 19, 2020 10:43

ngminhduong force-pushed the dev/bugfix_50949 branch 2 times, most recently from ace18f3 to 46def80 Compare February 19, 2020 14:47

dnfield reviewed Feb 19, 2020

View reviewed changes

Fix issue viewdidload call while init FlutterViewController

17dd9d0

ngminhduong force-pushed the dev/bugfix_50949 branch from 46def80 to 17dd9d0 Compare February 19, 2020 16:52

gaaclarke reviewed Feb 19, 2020

View reviewed changes

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

gaaclarke reviewed Feb 19, 2020

View reviewed changes

dnfield reviewed Feb 19, 2020

View reviewed changes

shell/platform/darwin/ios/platform_view_ios.mm Outdated Show resolved Hide resolved

gaaclarke and others added 3 commits February 19, 2020 11:06

added unit test

03056f8

AttachView while SetOwnerViewController if viewLoaded and fix Spelling

aac5bac

Move attach viewcontroller to the engine and attachView to viewDidLoa…

321d6b9

…d instead of viewWillAppear

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

37f0a4a

…lutter/engine#16672)

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

Roll engine 5aff31194809..43971caf04af (9 commits) flutter/flutter#52139

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

8e61884

…lutter/engine#16672)

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

Roll engine 5aff31194809..45e61a6ff777 (10 commits) flutter/flutter#52143

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

61cd2cc

…lutter/engine#16672)

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

Roll engine 5aff31194809..1ab5c3641caa (11 commits) flutter/flutter#52148

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

beae0a2

…lutter/engine#16672)

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

Roll engine 5aff31194809..dfc9c126d0dd (13 commits) flutter/flutter#52153

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

e6c68ed

…lutter/engine#16672)

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

Roll engine 5aff31194809..eddda803279c (14 commits) flutter/flutter#52154

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

5ec7334

…lutter/engine#16672)

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

Roll engine 5aff31194809..c15f239c1e9e (15 commits) flutter/flutter#52155

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

8e3e9bf

…lutter/engine#16672)

This was referenced Mar 7, 2020

Roll engine 5aff31194809..e1ba7a18f456 (16 commits) flutter/flutter#52156

Closed

Roll engine 5aff31194809..d323bace5f6f (18 commits) flutter/flutter#52159

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

b53232a

…lutter/engine#16672)

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

9ebd8b5

…lutter/engine#16672)

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

Roll engine 5aff31194809..30a82927be82 (20 commits) flutter/flutter#52162

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

caa93fe

…lutter/engine#16672)

This was referenced Mar 7, 2020

Roll engine 5aff31194809..d0319631bfb8 (21 commits) flutter/flutter#52164

Closed

Roll engine 5aff31194809..41b371d0b0be (22 commits) flutter/flutter#52165

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

b02c6bf

…lutter/engine#16672)

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

Roll engine 5aff31194809..41b371d0b0be (22 commits) flutter/flutter#52176

Closed

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

ab0059a

…lutter/engine#16672)

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

fe051e0 Fix issue viewdidload call while init FlutterViewController (f…

27e16af

…lutter/engine#16672)

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

Roll engine 5aff31194809..619acd58ca55 (23 commits) flutter/flutter#52177

Merged

blasten pushed a commit to blasten/engine that referenced this pull request Mar 10, 2020

Fix issue viewdidload call while init FlutterViewController (flutter#…

282b3eb

…16672) Co-authored-by: Aaron Clarke <[email protected]>

xster reviewed Apr 4, 2020

View reviewed changes

jmagman mentioned this pull request Jun 27, 2022

[flutter/engine] PlatformViewIOS::attachVIew() is called multiple times during flutter engine startup on iOS flutter/flutter#106283

Closed


		#pragma mark - UIViewController lifecycle notifications

		- (void)viewWillAppear:(BOOL)animated {

Fix issue viewdidload call while init FlutterViewController #16672

Fix issue viewdidload call while init FlutterViewController #16672

Uh oh!

Conversation

ngminhduong commented Feb 19, 2020

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Feb 19, 2020

Uh oh!

chinmaygarde commented Feb 19, 2020

Uh oh!

ngminhduong commented Feb 19, 2020

Uh oh!

ngminhduong commented Feb 19, 2020

Uh oh!

dnfield commented Feb 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Feb 19, 2020

Uh oh!

ngminhduong commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dnfield commented Feb 19, 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!

Uh oh!

gaaclarke left a comment • 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!

Uh oh!

ngminhduong commented Feb 21, 2020

Uh oh!

ngminhduong commented Feb 24, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ngminhduong commented Feb 19, 2020 •

edited

Loading

gaaclarke left a comment •

edited

Loading