Added new lifecycle enum #11913

chunhtai · 2019-09-05T23:56:19Z

This is one of the step toward fixing add to app warm up issue
design doc

This pr adds a new AppLifecycleState.detached, and make sure flutter embedder sends out the correct signal when engine is initialized.

This pr also includes removes AppLifecycleState.suspend, It seems it is not longer used

This will be a breaking change and require manual engine roll.
framework pr https://github.com/flutter/flutter/pull/39945/files

flutter/flutter#39832

Here is the breaking change email draft

dnfield · 2019-09-06T17:31:03Z

@chunhtai - I'm using your PR to test building on LUCI infra presubmit. Failures may not indicate a real failure and might be safe to ignore (although the Cirrus failure shoul dbe addressed).

chunhtai · 2019-09-06T17:37:32Z

@chunhtai - I'm using your PR to test building on LUCI infra presubmit. Failures may not indicate a real failure and might be safe to ignore (although the Cirrus failure shoul dbe addressed).

ok thanks for letting me know, the cirrus test is expected to fail, this require framework change.

dnfield · 2019-09-06T18:09:29Z

All worked as expected - woohoo!

chunhtai · 2019-09-06T18:43:38Z

lib/ui/window.dart

+  /// The application is detached from view.
  ///
-  /// On iOS, this state is currently unused.
-  suspending,


@cbracken
It doesn't seem like we are still using this enum any where in our code. Since this pr will be a breaking change anyway. Is it ok to remove this?

Is there any reason not to deprecate for one stable release , document as unused, then delete it in order to make the transition slightly less painful to users?

I think it is less painful to remove the enum and add the new enum in one pr. If you think it is better to add a document warning ahead of time, I can do a document only change pr first and wait for some release cycle before merge this one

I'm for doing it in one PR as well. Adding an enum value is already a non-gracefully-failable breaking change. We already can't make a soft transition.

This enum is still used in the framework. It looks like flutter/flutter#39945 will remove it (from scheduler/binding.dart), but that PR hasn't been merged yet. As a result, this PR will block engine rolls moving forward.

chunhtai · 2019-09-06T18:47:43Z

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

 }

 - (void)notifyViewControllerDeallocated {
+  [[self lifecycleChannel] sendMessage:@"AppLifecycleState.detached"];


I don't know how to test this.

@xster has been working on some tests related to these and can probably help

I think #11890 might make some part of your test a bit easier. You'd have to change add a bit to push another VC after the Flutter VC and change the assert value after you pop the Flutter VC.

I expect to have that one reworked (retranslate to obj-c) by monday

Updated existing test

I'm tempted to suggest that we keep the logic that does all this stuff in the same place. i.e. all the rest of the lifecycle system channel transitions are sourced in flutterviewcontroller (as is the source of the signal that triggers this call). We might as well keep them all in the same place.

umm that's a good point. I think that's also generally true for all other references to the engine from the VC in the first place.

Maybe there isn't any immediate, great solutions. I'm tempted to say, since there's already ~40 "incorrect" _engine.get() in the flutter vc, we might as well keep this one in the same place (for easier refactor in the future since they're all together) and leave a bug for us to clean this up together when the engine sets a flutter view controller that's not the current one.

WDYT @dnfield, @gaaclarke?

For this specific one, this seems like the right move. If I understand correctly, this is specifically to let us know about a detatched state of the VC - the VC can't necessarily tell us that because it might try to tell us too late (at a point where it's reference to the engine is lost).

It would be worth auditing other places where we try to send information via the engine, but as long as it's information that only a makes sense in an attached state it should be fine to keep it in the VC. If not, we should move it to the engine.

For instance, there are a few lifecycle messages we send based on whether the view is coming or going about the Application state - those probably should get refactored so they work more sensibly in add-to-app contexts if that's possible.

The other usages of _engine.get() all look like they are correct to me.

I will leave this as is.

tangential to this PR now:

as long as it's information that only a makes sense in an attached state it should be fine to keep it in the VC

the point @chunhtai was pointing to originally I think is that if the VC did get detached from the engine, the VC would never know and would crash at some point. It's not happening right now because removing the engine from a currently active foreground vc is not something people do commonly. But we should audit.

goderbauer

Please make sure to also have this reviewed by somebody on the engine team.

goderbauer · 2019-09-09T16:11:04Z

lib/ui/window.dart

-  /// On iOS, this state is currently unused.
-  suspending,
+  /// When the application is in this state, the engine is running without
+  /// a view. It can either in the progress of creating a view, or the view


It can either be...

goderbauer · 2019-09-09T16:11:45Z

lib/ui/window.dart

  ///
-  /// On iOS, this state is currently unused.
-  suspending,
+  /// When the application is in this state, the engine is running without


I think this needs more documentation to understand what this state actually means. Can this state happen in the non add2app case?

This can happens whenever the condition is met during engine life cycle, and yes it also happens in non add2app case. I found it weird to specifically mention add2app/non add2app in the comment, so I didn't include it. I rephrased a little bit to make it more descriptive.

chunhtai · 2019-09-16T22:57:52Z

ready for re-review

xster

LGTM

xster · 2019-09-18T22:53:24Z

lib/ui/window.dart

-  ///
-  /// When the application is in this state, the engine will not call the
-  /// [Window.onBeginFrame] and [Window.onDrawFrame] callbacks.
+  /// The application is still hosted on flutter engine but is detached from


is still hosted on a Flutter engine

xster · 2019-09-18T22:53:54Z

lib/ui/window.dart

-  /// When the application is in this state, the engine will not call the
-  /// [Window.onBeginFrame] and [Window.onDrawFrame] callbacks.
+  /// The application is still hosted on flutter engine but is detached from
+  /// view.


any host views.

xster · 2019-09-18T22:55:03Z

lib/ui/window.dart

-  /// On iOS, this state is currently unused.
-  suspending,
+  /// When the application is in this state, the engine is running without
+  /// a view. It can either be in the progress of creating a view when engine


progress of attaching to a view when the engine was first initialized

(because Flutter main not be the one responsible for creating any views)

xster · 2019-09-18T22:56:53Z

lib/ui/window.dart

-  suspending,
+  /// When the application is in this state, the engine is running without
+  /// a view. It can either be in the progress of creating a view when engine
+  /// first initializes, or the view has been destroyed due to a Navigator pop.


or after the view being destroyed.

(I wouldn't necessarily reference the SystemNavigator API since the user can initiate a pop from the platform side too. A pop also pops the view but might not necessarily destroy the view)

xster · 2019-09-19T00:15:43Z

shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java

      this,
        androidLifecycle
    );
+    lifecycleChannel.appIsDetached();


Are you hedging on the fact that Aaron is supporting channel buffers for this (since no entrypoint/binding is running by this point).

I thought this will be end up stored in

engine/lib/ui/hooks.dart

Line 116 in ca5642c

window._initialLifecycleState = state;

if the binding hasn't initialized yet?

Ah you're right, we had an existing single featured buffer here :)

xster · 2019-09-19T00:16:59Z

...atform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java

    verify(mockFlutterEngine.getLifecycleChannel(), never()).appIsResumed();
    verify(mockFlutterEngine.getLifecycleChannel(), never()).appIsPaused();
    verify(mockFlutterEngine.getLifecycleChannel(), never()).appIsInactive();
+    verify(mockFlutterEngine.getLifecycleChannel(), never()).appIsDetached();


seems like it's getting a bit verbose. Can we just verifyNoMoreInteractions? (it may not work though if there's more things going on in the mock but I suspect not)

is detached will be called at the end. I think we cannot use verifyNoMoreInteractions?

xster · 2019-09-19T00:22:21Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

                                       initWithDescription:@"A second FlutterViewController goes "
                                                           @"through AppLifecycleState.inactive"]];
  [lifecycleExpectations
      addObject:[[XCTestExpectation alloc]


I expect that this test will fail earlier once @gaaclarke's channel buffer comes through right? Right now it's not crashing because of an extraneous detached signal at the beginning because the detached message you're sending on the FlutterEngine construct is just being dropped on the floor because no one is listening by that point on the framework right?

xster · 2019-09-19T00:22:42Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

                    initWithDescription:
                        @"A second FlutterViewController goes through AppLifecycleState.resumed"]];
-  flutterVC = [rootVC showFlutter];
+  // At the end of second Flutter VC, we want to make sure it deallocs and sends detached signal.


Instead of making this one longer, can you make a different test?

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

chunhtai · 2019-09-23T18:22:30Z

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

  self.iosPlatformView->SetOwnerViewController(_viewController);
  [self maybeSetupPlatformViewChannels];

+  [_flutterViewControllerWillDeallocObserver release];


@xster I am not sure if this the appropriate fix.
I ran into an issue that calling setViewController multiple time will register multiple observer for flutter vc. This happens in one of my test where i do [engine setViewController:nil] at the end which causes notifyViewControllerDeallocated to be called twice.

I think it make more sense to release the previous one in the case of setting multiple times

@gaaclarke, I have a feeling we had this discussion but didn't do it for some reason. You might have a better recollection of the reasoning.

cbracken · 2019-10-21T17:57:58Z

@chunhtai any plans to land this in the near future or should it be closed out?

xster · 2019-10-25T03:09:04Z

I think he's out this week but we should try to merge this in. I'll review this again (I'm back)

chunhtai · 2019-10-28T17:27:11Z

@chunhtai any plans to land this in the near future or should it be closed out?

There were some refactorings to android embedding, i will have to modify the code a little bit.

I will get this merged in these few days

chunhtai · 2019-10-28T20:22:10Z

@xster this should be ready to go. Can you take another look?

lib/ui/window.dart

lib/web_ui/lib/src/ui/window.dart

xster · 2019-10-29T18:07:02Z

lib/web_ui/lib/src/ui/window.dart

-  /// On iOS, this state is currently unused.
-  suspending,
+  /// When the application is in this state, the engine is running without
+  /// a view. It can either in the progress of creating a view, or the view


In the process of creating a view sounds a bit vague and/or overly inclusive. One might assume this would happen with every view creation process of any normal full-flutter apps, which isn't the case. Just say that this happens when the Flutter tree is populated without a platform UI.

It depends on the dart code the the flutter tree might or might not exist. I will just mention the engine is running without a platform UI

xster · 2019-10-29T18:19:47Z

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

                   << entrypoint.UTF8String;
  } else {
    [self setupChannels];
+    [[self lifecycleChannel] sendMessage:@"AppLifecycleState.detached"];


A bit strange that we're imperatively triggering an edge-triggered signal to set a level-triggered signal. Just letting window._initialLifecycleState = detached seems semantically more correct but also a bit riskier.

chunhtai · 2019-10-30T19:38:05Z

runtime/runtime_controller.h

    std::vector<std::string> locale_data;
    std::string user_settings_data = "{}";
-    std::string lifecycle_state;
+    std::string lifecycle_state = "AppLifecycleState.detached";


@xster This will be true as long as dart project is run after engine is initialized

There is no direct api for engine to modify window.dart property. Even if we drill one down, it still have to go through shell, and that will be the same case as before.

This lgtm. Curious why here and not in window.dart's _initialLifecycleState

When the window hook initialized, runtime controller will send these datas to window hook to initialize it. So the data here means to be the default value of the window hook

Got it. Thanks

xster

LGTM

xster · 2019-10-31T05:47:16Z

runtime/runtime_controller.h

    std::vector<std::string> locale_data;
    std::string user_settings_data = "{}";
-    std::string lifecycle_state;
+    std::string lifecycle_state = "AppLifecycleState.detached";


This lgtm. Curious why here and not in window.dart's _initialLifecycleState

xster · 2019-10-31T06:55:58Z

testing/scenario_app/ios/Scenarios/ScenariosTests/AppLifecycleTests.m

  [engine setViewController:nil];
 }

+- (void)testDetachedFlutterViewControllerRespondsToApplicationLifecycle {


The name is not quite right. I think you meant something like testFlutterViewControllerDetachingSendsApplicationLifecycle or something.

xster · 2019-10-31T07:13:07Z

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

 }

 - (void)notifyViewControllerDeallocated {
+  [[self lifecycleChannel] sendMessage:@"AppLifecycleState.detached"];


umm that's a good point. I think that's also generally true for all other references to the engine from the VC in the first place.

Maybe there isn't any immediate, great solutions. I'm tempted to say, since there's already ~40 "incorrect" _engine.get() in the flutter vc, we might as well keep this one in the same place (for easier refactor in the future since they're all together) and leave a bug for us to clean this up together when the engine sets a flutter view controller that's not the current one.

WDYT @dnfield, @gaaclarke?

xster · 2019-10-31T21:00:30Z

LGTM

cbracken · 2019-11-04T19:08:01Z

From build_and_test_linux_unopt_debug:

   +1 ms] Exception: ../../../packages/flutter/lib/src/scheduler/binding.dart:336:30:
           Error: Getter not found: 'suspending'.
                 case AppLifecycleState.suspending:
                                        ^^^^^^^^^^
           ../../../packages/flutter/lib/src/scheduler/binding.dart:356:34:
           Error: Getter not found: 'suspending'.
                   return AppLifecycleState.suspending;
                                            ^^^^^^^^^^

xster · 2019-11-04T19:09:49Z

Thanks for checking Chris. I guess this one we'd have to fix with a different PR on the framework @chunhtai

chunhtai · 2019-11-04T19:10:00Z

From build_and_test_linux_unopt_debug:

   +1 ms] Exception: ../../../packages/flutter/lib/src/scheduler/binding.dart:336:30:
           Error: Getter not found: 'suspending'.
                 case AppLifecycleState.suspending:
                                        ^^^^^^^^^^
           ../../../packages/flutter/lib/src/scheduler/binding.dart:356:34:
           Error: Getter not found: 'suspending'.
                   return AppLifecycleState.suspending;
                                            ^^^^^^^^^^

This is expected, it require an manual roll. I am waiting for luci to be green

This reverts commit 02a4790.

[email protected]:flutter/engine.git/compare/e73c9c8f6bd5...7b968ff git log e73c9c8..7b968ff --no-merges --oneline 2019-11-05 [email protected] Ensure that the CAMetalLayer FBO attachments can be read from. (flutter/engine#13643) 2019-11-04 [email protected] Revert "Added new lifecycle enum (#11913)" (flutter/engine#13632) 2019-11-04 [email protected] Added new lifecycle enum (flutter/engine#11913) 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] on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

* Reland "Added new lifecycle enum (#11913)"

[email protected]:flutter/engine.git/compare/e73c9c8f6bd5...7b968ff git log e73c9c8..7b968ff --no-merges --oneline 2019-11-05 [email protected] Ensure that the CAMetalLayer FBO attachments can be read from. (flutter/engine#13643) 2019-11-04 [email protected] Revert "Added new lifecycle enum (flutter#11913)" (flutter/engine#13632) 2019-11-04 [email protected] Added new lifecycle enum (flutter/engine#11913) 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] on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

reference flutter/engine#11913

As reported on [Jira](https://adjustcom.atlassian.net/browse/ES-3620), the `AppLifecycleState.suspending` method is no longer supported by Flutter. This has been replaced by `AppLifecycleState.detached`. [Flutter PR for context](flutter/engine#11913)

googlebot added the cla: yes label Sep 5, 2019

chunhtai mentioned this pull request Sep 5, 2019

added new lifecycle state flutter/flutter#39945

Merged

9 tasks

chunhtai added the prod: API break label Sep 5, 2019

dnfield added the CQ+1 label Sep 6, 2019

chunhtai requested review from goderbauer and xster September 6, 2019 18:42

chunhtai commented Sep 6, 2019

View reviewed changes

goderbauer reviewed Sep 9, 2019

View reviewed changes

chunhtai force-pushed the issues/39832 branch from fccc13d to ad5021a Compare September 16, 2019 22:51

xster reviewed Sep 19, 2019

View reviewed changes

chunhtai force-pushed the issues/39832 branch from ad5021a to 04f2acc Compare September 23, 2019 18:15

chunhtai commented Sep 23, 2019

View reviewed changes

chunhtai force-pushed the issues/39832 branch 2 times, most recently from a0849e7 to dffde3e Compare October 4, 2019 17:21

chunhtai mentioned this pull request Oct 4, 2019

flutter first StatefulWidget dispose method not called when app quit flutter/flutter#28134

Closed

chunhtai force-pushed the issues/39832 branch from dffde3e to e03828b Compare October 7, 2019 21:16

chunhtai force-pushed the issues/39832 branch from e03828b to d797101 Compare October 28, 2019 19:55

xster reviewed Oct 29, 2019

View reviewed changes

lib/ui/window.dart Show resolved Hide resolved

xster reviewed Oct 29, 2019

View reviewed changes

lib/web_ui/lib/src/ui/window.dart Outdated Show resolved Hide resolved

xster reviewed Oct 29, 2019

View reviewed changes

chunhtai commented Oct 30, 2019

View reviewed changes

xster reviewed Oct 31, 2019

View reviewed changes

chunhtai force-pushed the issues/39832 branch from 2b7e86e to c48df33 Compare November 1, 2019 16:08

Added new lifecycle enum

4c0cbf6

chunhtai force-pushed the issues/39832 branch from c48df33 to 4c0cbf6 Compare November 4, 2019 17:45

chunhtai 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 4, 2019

chunhtai merged commit 02a4790 into flutter:master Nov 4, 2019

chunhtai added a commit to chunhtai/engine that referenced this pull request Nov 4, 2019

Revert "Added new lifecycle enum (flutter#11913)"

fcc6063

This reverts commit 02a4790.

chunhtai added a commit that referenced this pull request Nov 4, 2019

Revert "Added new lifecycle enum (#11913)" (#13632)

fe0838e

This reverts commit 02a4790.

chunhtai added a commit to chunhtai/engine that referenced this pull request Nov 4, 2019

Reland "Added new lifecycle enum (flutter#11913)"

83f4f49

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

02a4790 Added new lifecycle enum (flutter/engine#11913)

c7ab23f

engine-flutter-autoroll mentioned this pull request Nov 5, 2019

Roll engine e73c9c8f6bd5..7b968ff95ff1 (3 commits) flutter/flutter#44158

Merged

chunhtai added a commit that referenced this pull request Nov 5, 2019

Issues/39832 reland (#13642)

1bfb928

* Reland "Added new lifecycle enum (#11913)"

sarbagyastha mentioned this pull request Nov 7, 2019

Errors during build on version 6.0.0 Flutter 1.10.15-pre.424 • channel master sarbagyastha/youtube_player_flutter#114

Closed

JohnMusleh mentioned this pull request Dec 4, 2019

AppLifecycleState enum suspending removed amplitude/Amplitude-Flutter-Deprecated#42

Closed

haoliu-amp mentioned this pull request Dec 12, 2019

Error: Getter not found: 'detached' amplitude/Amplitude-Flutter-Deprecated#48

Closed

boyks mentioned this pull request Dec 19, 2019

Getter not found: 'suspending' zino-hofmann/graphql-flutter#499

Closed

ankur014 mentioned this pull request Jan 12, 2020

Error: Getter not found: 'suspending'. case AppLifecycleState.suspending: zino-hofmann/graphql-flutter#535

Closed

ajerni mentioned this pull request Feb 11, 2020

Error: Getter not found: 'suspending' zino-hofmann/graphql-flutter#559

Closed

yunsion added a commit to yunsion/flutter_lifecycle_state that referenced this pull request Nov 10, 2020

fixed bug about suspending

b6dc84d

reference flutter/engine#11913

yunsion mentioned this pull request Nov 10, 2020

fixed bug about suspending tinyvampirepudge/flutter_lifecycle_state#3

Open

Added new lifecycle enum #11913

Added new lifecycle enum #11913

Uh oh!

Conversation

chunhtai commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Sep 6, 2019

Uh oh!

chunhtai commented Sep 6, 2019

Uh oh!

dnfield commented Sep 6, 2019

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!

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!

goderbauer 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!

chunhtai commented Sep 16, 2019

Uh oh!

xster 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!

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!

chunhtai commented Sep 5, 2019 •

edited

Loading

xster Oct 29, 2019 •

edited

Loading