Automatically destroy FlutterEngine when created by FlutterActivity or FlutterFragment. #13423

matthew-carroll · 2019-10-29T22:22:13Z

Automatically destroy FlutterEngine when created by FlutterActivity or FlutterFragment.

I believe this resolves: flutter/flutter#43688

I did a memory profile locally and got the following results:

Memory dump before, during, and after the gallery was opened using existing engine:
130,258
287,161
282,553

Memory dump before, during, and after the gallery was opened using changes in this PR:
118,115
282,177
52,930

I'm now attempting to reproduce these results by running the actual device lab test locally. I'm waiting for a local build of profile mode to finish so that I can execute the test, but so far the evidence looks pretty strong.

…r FlutterFragment.

blasten · 2019-10-29T22:49:26Z

Are we able to add tests at this point?

matthew-carroll · 2019-10-29T22:51:06Z

@blasten what did you have in mind?

matthew-carroll · 2019-10-29T23:06:07Z

I realized after looking at the tests that the root cause is that our tests do not use the dedicated methods for configuring an Intent for FlutterActivity, which lead to the unexpected retention of the FlutterEngine. This PR essentially handles the missing Intent extra. I have now also added a test to verify that even such an illegal intent does not retain the FlutterEngine.

shell/platform/android/io/flutter/embedding/android/FlutterFragment.java

xster · 2019-10-30T01:09:07Z

shell/platform/android/test/io/flutter/embedding/android/FlutterActivityTest.java

+
+    assertTrue(flutterActivity.shouldDestroyEngineWithHost());
+  }
+


Have more tests where a mockito engine is passed into the activity/fragment via the cache or by subclassing and provideFlutterEngine and check that flutterEngine.destroy is not called?

Testing any lifecycle methods is outside what we seem to be able to do with our current testing infrastructure. Maybe we could assemble some e2e tests for that purpose, but even those seem to take a while to create. I did add some testing logic to cover the use-case you mentioned.

Robolectric in general can test lifecycle events. We should have the infra for this. But maybe the problem is something other than just the raw ability for the test harness to drive lifecycle events?

…destroy it.

xster · 2019-10-30T05:02:26Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+   * removed.
+   */
+  @VisibleForTesting
+  /* package */ void setDelegate(@NonNull FlutterActivityAndFragmentDelegate delegate) {


Being able to put in a delegate factory was the reason why I made the FlutterInjector :D

Yeah, I think the desired outcome here is to better support common testing mechanisms. Then we would probably have alternative approaches that don't require this or an injector.

+1. We don't necessarily need to bring in a full DI framework (and probably shouldn't) but it would help testing a ton in general if the classes used constructor injection as a pattern instead of defaulting to instantiating everything as silent side effects of being constructed, or having certain callbacks triggered.

Yeah, the combination of problems here is that:

We can't constructor inject framework classes

The very 1st lifecycle method in FlutterActivity and FlutterFragment attempt to connect to JNI

We should eventually probably introduce a static dependency resolver for FlutterActivity and FlutterFragment. @xster had something like that in his original PR. The reason I removed it was because I think it was also being passed to plain Java classes, which I think should always utilize constructor injection unless there is an exceptional situation. But I probably should not have removed it entirely - I should have left it in specifically for FlutterActivity and FlutterFragment

At this point my recommendation is that we do what is necessary to button up the embedding for public consumption, and then it would be nice to schedule a dedicated project to resolve outstanding issues related to testing including: testing dep resolution, testing dep versioning, and embedding tools for testing.

ya, this is now tangential to this PR to some extent. But ya, that's why DI is so common on Android, because you can't construct and inject stuff generally.

Let's clean these up at some point in your TODO issue.

xster · 2019-10-30T05:04:30Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+  /**
+   * This method exists so that JVM tests can ensure that a delegate exists without
+   * putting this Activity through any lifecycle events, because JVM tests cannot handle
+   * executing any lifecycle methods, at the time of writing this.


Should be possible. Does http://robolectric.org/androidx_test/ help any?

I'm not sure, but we don't have support for the latest testing infra. I think we're still on robolectric v1, while robolectric is now on v3 or v4. If Flutter brings this testing infra completely up to date with the ecosystem then we can probably write a lot of better tests.

We should be able to put activities through lifecycle events on our current Robolectric version (3.8). What's missing in 3 that you need in 4 that we don't have here?

xster · 2019-10-30T05:14:31Z

I think you probably don't need the setter by calling activitycontroller.create etc.

Otherwise LGTM

matthew-carroll · 2019-10-30T05:16:49Z

@xster the first thing I tried was calling create(). That crashes right off the bat by trying to attach to JNI. So then I tried to subclass and override onCreate() to only instantiate the delegate but then it blew up because it couldn't find some version of appcompat. That means there's probably some transitive dependency that we're missing in our testing infra, but every time I've tried to solve those problems it costs me a number of days to track it down. So I opted for exposing a setter. Is it OK if we continue as-is?

xster · 2019-10-30T05:20:49Z

@mklim looks like https://chrome-infra-packages.appspot.com/p/flutter/android/robolectric_bundle/+/last_updated:2019-09-09T16:47:38-0700 is on robolectric 3.8 right?

This is fine. Though we should leave an issue to clean this up at some point.

matthew-carroll · 2019-10-30T05:23:39Z

Maybe I'm thinking of some other library version then. I think one of these major pieces is on 1 instead of 3. Oh, maybe I'm thinking of mockito? In any event, I think if we could write tests the way that tests are exemplified in standard Android docs, then we'd probably be in a position to write better tests.

matthew-carroll · 2019-10-30T05:28:05Z

Added a TODO with a github issue in source code to remove the setter.

xster · 2019-10-30T05:32:09Z

Mockito doesn't affect this right? It's a purely robolectric issue.

matthew-carroll · 2019-10-30T05:36:17Z

Yeah, that's correct. But really it's that we haven't included every possible transitive dependency that would typically be loaded. I'm also not sure how to even discover that set of dependencies. The dependency tree is both very broad and very deep. My overarching point is that pretty much every new test that's written seems to lead to some missing piece, so each new test can end up with days of plumbing work. If we could reach the status-quo, which is Gradle resolved dependencies with the latest and most convenient APIs, then we could probably do a lot better with tests in general, including this specific case, too.

xster · 2019-10-30T05:42:41Z

https://github.com/flutter/engine/pull/12069/files#diff-f928557f2d60773a8435366400fa42edR93 was meant to have a universally test accessible way of not crashing on JNI. Is this somewhere else now?

This PR is fine but if the only issue left was the crash on ActivityController.create, it would be nice to spend some time-bound effort to fix it since it seems fixable.

matthew-carroll · 2019-10-30T05:44:49Z

That sounds good to me. Can we wait until after the code freeze since these are package private, visible for testing exposures?

xster · 2019-10-30T05:47:09Z

ya, that's fine

mklim · 2019-10-30T16:38:43Z

This is fine. Though we should leave an issue to clean this up at some point.

Yeah, tracked at flutter/flutter#38471. We were blocked for a little while on a Robolectric issue but the latest release should fix it. The upgrade is a little complicated because it also involves switching CI to use Java 9.

matthew-carroll · 2019-10-30T21:55:48Z

I finally got the device lab test running locally with the patch. Here is the comparison of the results. It looks good to me.

Before patch:

Task result:
{
"success": true,
"data": {
"start-min": 37628,
"start-max": 43147,
"start-median": 37677,
"end-min": 126056,
"end-max": 131671,
"end-median": 126819,
"diff-min": 88313,
"diff-max": 89793,
"diff-median": 88524
},
"benchmarkScoreKeys": [
"start-min",
"start-max",
"start-median",
"end-min",
"end-max",
"end-median",
"diff-min",
"diff-max",
"diff-median"
]
}

After patch:

Task result:
{
"success": true,
"data": {
"start-min": 51088,
"start-max": 57177,
"start-median": 51593,
"end-min": 56913,
"end-max": 60748,
"end-median": 57558,
"diff-min": 3571,
"diff-max": 6470,
"diff-median": 5727
},
"benchmarkScoreKeys": [
"start-min",
"start-max",
"start-median",
"end-min",
"end-max",
"end-median",
"diff-min",
"diff-max",
"diff-median"
]
}

matthew-carroll · 2019-10-30T22:01:44Z

Merging on red LUCI due to message in chat that says it's not really broken.

…tivity or FlutterFragment. (flutter/engine#13423)

git@github.com:flutter/engine.git/compare/e609577d1201...97807b5 git log e609577..97807b5 --no-merges --oneline 2019-10-30 matthew-carroll@users.noreply.github.com Automatically destroy FlutterEngine when created by FlutterActivity or FlutterFragment. (flutter/engine#13423) 2019-10-30 bkonyi@google.com Roll src/third_party/dart f30b494035..d50c158d42 (5 commits) (flutter/engine#13443) 2019-10-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 7da048b5e8f1..7df14d055703 (7 commits) (flutter/engine#13441) 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 cbracken@google.com 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

Automatically destroy FlutterEngine when created by FlutterActivity o…

d1f89f8

…r FlutterFragment.

matthew-carroll requested review from blasten, chinmaygarde, jason-simmons and xster October 29, 2019 22:22

googlebot added cla: yes labels Oct 29, 2019

jason-simmons approved these changes Oct 29, 2019

View reviewed changes

Fixed implementation for tests.

2281efe

Added test for engine destruction when Intent is missing the parameter.

0155b60

xster reviewed Oct 30, 2019

View reviewed changes

shell/platform/android/io/flutter/embedding/android/FlutterFragment.java Outdated Show resolved Hide resolved

xster reviewed Oct 30, 2019

View reviewed changes

Account for a provided FlutterEngine when deciding whether or not to …

3bf9db9

…destroy it.

xster reviewed Oct 30, 2019

View reviewed changes

Added issue to remove setter.

a478a80

matthew-carroll merged commit 97807b5 into flutter:master Oct 30, 2019

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

97807b5 Automatically destroy FlutterEngine when created by FlutterAc…

a7caa04

…tivity or FlutterFragment. (flutter/engine#13423)

engine-flutter-autoroll mentioned this pull request Oct 30, 2019

Roll engine e609577d1201..97807b534122 (3 commits) flutter/flutter#43860

Merged

blasten mentioned this pull request Oct 31, 2019

Reland: Migrate examples to the Android embedding v2 flutter/flutter#43868

Merged

Automatically destroy FlutterEngine when created by FlutterActivity or FlutterFragment. #13423

Automatically destroy FlutterEngine when created by FlutterActivity or FlutterFragment. #13423

Uh oh!

Conversation

matthew-carroll commented Oct 29, 2019

Uh oh!

blasten commented Oct 29, 2019

Uh oh!

matthew-carroll commented Oct 29, 2019

Uh oh!

matthew-carroll commented Oct 29, 2019

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!

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!

xster commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

xster commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

xster commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

xster commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

xster commented Oct 30, 2019

Uh oh!

mklim commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

matthew-carroll commented Oct 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants