From a2b1f2697920112d80f80fdfd1b948b7fc7addbf Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Thu, 23 Jan 2020 14:06:08 -0800 Subject: [PATCH 1/3] Register plugins at the right time, once Currently we're automatically registering plugins both when the FlutterEngine is constructed and in the `flutter create` template, when FlutterActivity#configureFlutterEngine is called. The initial registration is too early to contain a reference to the activity and the second registration can cause problems in some plugins. This alters the flow so automatic registration happens in two discrete places, and contains the `activity` in its first and only call for most apps. 1. We're no longer automatically registering plugins on `FlutterEngine` in any of our activities/fragments at construction time. But since the FlutterEngine default constructor still automatically registers plugins, anyone constructing the engine themselves (for example, in a service) is still going to get automatic registration at `FlutterEngine` instantiation time. 2. We now automatically register plugins in the base `FlutterActivity`'s `configureFlutterEngine` hook. Anyone using `FlutterActivity` (or `FlutterFragment`) should be automatically registered once that hook is called. Right now the `flutter create` template overrides the base class method with a subclass that registers everything manually in the same spot. But with this in place we can safely recommend to remove the subclass and rely on this hook to automatically register going forward. Registering at this time means `activity` is set correctly. --- shell/platform/android/BUILD.gn | 1 + .../embedding/android/FlutterActivity.java | 41 ++++++++++++++-- .../FlutterActivityAndFragmentDelegate.java | 4 +- .../embedding/engine/FlutterEngine.java | 30 +++++++++++- .../android/FlutterActivityTest.java | 29 +++++++++++- .../embedding/engine/FlutterEngineTest.java | 47 ++++++++++++++++--- 6 files changed, 136 insertions(+), 16 deletions(-) diff --git a/shell/platform/android/BUILD.gn b/shell/platform/android/BUILD.gn index 943b04ca4c71a..9b7ee297e1094 100644 --- a/shell/platform/android/BUILD.gn +++ b/shell/platform/android/BUILD.gn @@ -435,6 +435,7 @@ action("robolectric_tests") { "test/io/flutter/plugin/common/StandardMessageCodecTest.java", "test/io/flutter/plugin/editing/TextInputPluginTest.java", "test/io/flutter/plugin/platform/SingleViewPresentationTest.java", + "test/io/flutter/plugins/GeneratedPluginRegistrant.java", "test/io/flutter/util/PreconditionsTest.java", ] diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterActivity.java b/shell/platform/android/io/flutter/embedding/android/FlutterActivity.java index 6e5ada1d8d77c..8a7a8475e71f8 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterActivity.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterActivity.java @@ -33,6 +33,9 @@ import io.flutter.plugin.platform.PlatformPlugin; import io.flutter.view.FlutterMain; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + import static io.flutter.embedding.android.FlutterActivityLaunchConfigs.DART_ENTRYPOINT_META_DATA_KEY; import static io.flutter.embedding.android.FlutterActivityLaunchConfigs.DEFAULT_BACKGROUND_MODE; import static io.flutter.embedding.android.FlutterActivityLaunchConfigs.DEFAULT_DART_ENTRYPOINT; @@ -873,14 +876,18 @@ public PlatformPlugin providePlatformPlugin(@Nullable Activity activity, @NonNul } /** - * Hook for subclasses to easily configure a {@code FlutterEngine}, e.g., register - * plugins. - *

- * This method is called after {@link #provideFlutterEngine(Context)}. + * Hook for subclasses to easily configure a {@code FlutterEngine}. + * + *

This method is called after {@link #provideFlutterEngine(Context)}. + * + *

All plugins listed in the app's pubspec are registered in the base implementation of this + * method. To avoid automatic plugin registration, override this method without invoking super(). + * To keep automatic plugin registration and further configure the flutterEngine, override this + * method, invoke super(), and then configure the flutterEngine as desired. */ @Override public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) { - // No-op. Hook for subclasses. + registerPlugins(flutterEngine); } /** @@ -950,4 +957,28 @@ public void onFlutterUiNoLongerDisplayed() { // no-op } + /** + * Registers all plugins that an app lists in its pubspec.yaml. + *

+ * The Flutter tool generates a class called GeneratedPluginRegistrant, which includes the code + * necessary to register every plugin in the pubspec.yaml with a given {@code FlutterEngine}. + * The GeneratedPluginRegistrant must be generated per app, because each app uses different sets + * of plugins. Therefore, the Android embedding cannot place a compile-time dependency on this + * generated class. This method uses reflection to attempt to locate the generated file and then + * use it at runtime. + *

+ * This method fizzles if the GeneratedPluginRegistrant cannot be found or invoked. This situation + * should never occur, but if any eventuality comes up that prevents an app from using this + * behavior, that app can still write code that explicitly registers plugins. + */ + private static void registerPlugins(@NonNull FlutterEngine flutterEngine) { + try { + Class generatedPluginRegistrant = Class.forName("io.flutter.plugins.GeneratedPluginRegistrant"); + Method registrationMethod = generatedPluginRegistrant.getDeclaredMethod("registerWith", FlutterEngine.class); + registrationMethod.invoke(null, flutterEngine); + } catch (Exception e) { + Log.w(TAG, "Tried to automatically register plugins with FlutterEngine (" + + flutterEngine + ") but could not find and invoke the GeneratedPluginRegistrant."); + } + } } diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java b/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java index 17bd3c46d5632..4f4333d016b37 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java @@ -27,6 +27,8 @@ import io.flutter.embedding.engine.FlutterShellArgs; import io.flutter.embedding.engine.dart.DartExecutor; import io.flutter.embedding.engine.renderer.FlutterUiDisplayListener; +import io.flutter.embedding.engine.FlutterJNI; +import io.flutter.embedding.engine.loader.FlutterLoader; import io.flutter.plugin.platform.PlatformPlugin; import static android.content.ComponentCallbacks2.TRIM_MEMORY_RUNNING_LOW; @@ -223,7 +225,7 @@ void onAttach(@NonNull Context context) { // FlutterView. Log.v(TAG, "No preferred FlutterEngine was provided. Creating a new FlutterEngine for" + " this FlutterFragment."); - flutterEngine = new FlutterEngine(host.getContext(), host.getFlutterShellArgs().toArray()); + flutterEngine = new FlutterEngine(host.getContext(), /*automaticallyRegisterPlugins=*/true); isFlutterEngineFromHost = false; } diff --git a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java index cd8956a172961..aefa7a8675529 100644 --- a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java +++ b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java @@ -126,8 +126,7 @@ public void onPreEngineRestart() { * {@link RenderSurface} is registered. See * {@link #getRenderer()} and {@link FlutterRenderer#startRenderingToSurface(RenderSurface)}. *

- * A new {@code FlutterEngine} does not come with any Flutter plugins attached. To attach plugins, - * see {@link #getPlugins()}. + * A new {@code FlutterEngine} automatically attaches all plugins. See {@link #getPlugins()}. *

* A new {@code FlutterEngine} does come with all default system channels attached. *

@@ -142,6 +141,33 @@ public FlutterEngine(@NonNull Context context) { this(context, null); } + /** + * Constructs a new {@code FlutterEngine}. + *

+ * A new {@code FlutterEngine} does not execute any Dart code automatically. See + * {@link #getDartExecutor()} and {@link DartExecutor#executeDartEntrypoint(DartExecutor.DartEntrypoint)} + * to begin executing Dart code within this {@code FlutterEngine}. + *

+ * A new {@code FlutterEngine} will not display any UI until a + * {@link RenderSurface} is registered. See + * {@link #getRenderer()} and {@link FlutterRenderer#startRenderingToSurface(RenderSurface)}. + *

+ * Set {@code automaticallyRegisterPlugins} to control whether or not the instance is created with + * all plugins attached. See {@link #getPlugins()}. + *

+ * A new {@code FlutterEngine} does come with all default system channels attached. + *

+ * The first {@code FlutterEngine} instance constructed per process will also load the Flutter + * native library and start a Dart VM. + *

+ * In order to pass Dart VM initialization arguments (see {@link io.flutter.embedding.engine.FlutterShellArgs}) + * when creating the VM, manually set the initialization arguments by calling {@link FlutterLoader#startInitialization(Context)} + * and {@link FlutterLoader#ensureInitializationComplete(Context, String[])}. + */ + public FlutterEngine(@NonNull Context context, boolean automaticallyRegisterPlugins) { + this(context, FlutterLoader.getInstance(), new FlutterJNI(), null, automaticallyRegisterPlugins); + } + /** * Same as {@link #FlutterEngine(Context)} with added support for passing Dart * VM arguments. diff --git a/shell/platform/android/test/io/flutter/embedding/android/FlutterActivityTest.java b/shell/platform/android/test/io/flutter/embedding/android/FlutterActivityTest.java index 64528bfd766cf..e4cfbfdda70d0 100644 --- a/shell/platform/android/test/io/flutter/embedding/android/FlutterActivityTest.java +++ b/shell/platform/android/test/io/flutter/embedding/android/FlutterActivityTest.java @@ -6,6 +6,10 @@ import android.support.annotation.NonNull; import android.support.annotation.Nullable; +import io.flutter.plugins.GeneratedPluginRegistrant; +import java.util.List; +import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.Robolectric; @@ -30,6 +34,16 @@ @Config(manifest=Config.NONE) @RunWith(RobolectricTestRunner.class) public class FlutterActivityTest { + @Before + public void setUp() { + GeneratedPluginRegistrant.clearRegisteredEngines(); + } + + @After + public void tearDown() { + GeneratedPluginRegistrant.clearRegisteredEngines(); + } + @Test public void itCreatesDefaultIntentWithExpectedDefaults() { Intent intent = FlutterActivity.createDefaultIntent(RuntimeEnvironment.application); @@ -122,6 +136,19 @@ public void itCreatesCachedEngineIntentThatDestroysTheEngine() { assertTrue(flutterActivity.shouldDestroyEngineWithHost()); } + @Test + public void itRegistersPluginsAtConfigurationTime() { + FlutterActivity activity = Robolectric.buildActivity(FlutterActivityWithProvidedEngine.class).get(); + activity.onCreate(null); + + assertTrue(GeneratedPluginRegistrant.getRegisteredEngines().isEmpty()); + activity.configureFlutterEngine(activity.getFlutterEngine()); + + List registeredEngines = GeneratedPluginRegistrant.getRegisteredEngines(); + assertEquals(1, registeredEngines.size()); + assertEquals(activity.getFlutterEngine(), registeredEngines.get(0)); + } + static class FlutterActivityWithProvidedEngine extends FlutterActivity { @Override protected void onCreate(@Nullable Bundle savedInstanceState) { @@ -140,7 +167,7 @@ public FlutterEngine provideFlutterEngine(@NonNull Context context) { mock(FlutterLoader.class), flutterJNI, new String[]{}, - true + false ); } } diff --git a/shell/platform/android/test/io/flutter/embedding/engine/FlutterEngineTest.java b/shell/platform/android/test/io/flutter/embedding/engine/FlutterEngineTest.java index 80ec2c9455708..133beb6dd75fc 100644 --- a/shell/platform/android/test/io/flutter/embedding/engine/FlutterEngineTest.java +++ b/shell/platform/android/test/io/flutter/embedding/engine/FlutterEngineTest.java @@ -1,7 +1,13 @@ package test.io.flutter.embedding.engine; +import io.flutter.plugins.GeneratedPluginRegistrant; +import java.util.List; +import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; import org.robolectric.RobolectricTestRunner; import org.robolectric.RuntimeEnvironment; import org.robolectric.annotation.Config; @@ -10,25 +16,52 @@ import io.flutter.embedding.engine.FlutterJNI; import io.flutter.embedding.engine.loader.FlutterLoader; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @Config(manifest=Config.NONE) @RunWith(RobolectricTestRunner.class) public class FlutterEngineTest { - @Test - public void itDoesNotCrashIfGeneratedPluginRegistrantIsUnavailable() { - FlutterJNI flutterJNI = mock(FlutterJNI.class); + @Mock FlutterJNI flutterJNI; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); when(flutterJNI.isAttached()).thenReturn(true); + GeneratedPluginRegistrant.clearRegisteredEngines(); + } + @After + public void tearDown() { + GeneratedPluginRegistrant.clearRegisteredEngines(); + } + + @Test + public void itAutomaticallyRegistersPluginsByDefault() { + assertTrue(GeneratedPluginRegistrant.getRegisteredEngines().isEmpty()); FlutterEngine flutterEngine = new FlutterEngine( + RuntimeEnvironment.application, + mock(FlutterLoader.class), + flutterJNI + ); + + List registeredEngines = GeneratedPluginRegistrant.getRegisteredEngines(); + assertEquals(1, registeredEngines.size()); + assertEquals(flutterEngine, registeredEngines.get(0)); + } + + @Test + public void itCanBeConfiguredToNotAutomaticallyRegisterPlugins() { + new FlutterEngine( RuntimeEnvironment.application, mock(FlutterLoader.class), flutterJNI, - new String[] {}, - true + /*dartVmArgs=*/new String[] {}, + /*automaticallyRegisterPlugins=*/false ); - // The fact that the above constructor executed without error means that - // it dealt with a non-existent GeneratedPluginRegistrant. + + assertTrue(GeneratedPluginRegistrant.getRegisteredEngines().isEmpty()); } } From 9ea419658b7e53d08f0c48a81dbe8c2ff9962157 Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Fri, 24 Jan 2020 15:46:53 -0800 Subject: [PATCH 2/3] Bugfixes 1. Previous commit accidentally ate `dartVmArgs`, change the new `FlutterEngine` constructor so that it still is passed through. 2. `GeneratedPluginRegistrant` is in `.gitignore` so the last commit didn't have the fake, force the test fake to be added into version control and commit it. --- .../FlutterActivityAndFragmentDelegate.java | 2 +- .../embedding/engine/FlutterEngine.java | 34 ++++--------- .../plugins/GeneratedPluginRegistrant.java | 48 +++++++++++++++++++ 3 files changed, 58 insertions(+), 26 deletions(-) create mode 100644 shell/platform/android/test/io/flutter/plugins/GeneratedPluginRegistrant.java diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java b/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java index 4f4333d016b37..5b36dfaafe735 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java @@ -225,7 +225,7 @@ void onAttach(@NonNull Context context) { // FlutterView. Log.v(TAG, "No preferred FlutterEngine was provided. Creating a new FlutterEngine for" + " this FlutterFragment."); - flutterEngine = new FlutterEngine(host.getContext(), /*automaticallyRegisterPlugins=*/true); + flutterEngine = new FlutterEngine(host.getContext(), host.getFlutterShellArgs().toArray(), /*automaticallyRegisterPlugins=*/false); isFlutterEngineFromHost = false; } diff --git a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java index aefa7a8675529..a5c086090ba77 100644 --- a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java +++ b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java @@ -142,42 +142,26 @@ public FlutterEngine(@NonNull Context context) { } /** - * Constructs a new {@code FlutterEngine}. - *

- * A new {@code FlutterEngine} does not execute any Dart code automatically. See - * {@link #getDartExecutor()} and {@link DartExecutor#executeDartEntrypoint(DartExecutor.DartEntrypoint)} - * to begin executing Dart code within this {@code FlutterEngine}. - *

- * A new {@code FlutterEngine} will not display any UI until a - * {@link RenderSurface} is registered. See - * {@link #getRenderer()} and {@link FlutterRenderer#startRenderingToSurface(RenderSurface)}. - *

- * Set {@code automaticallyRegisterPlugins} to control whether or not the instance is created with - * all plugins attached. See {@link #getPlugins()}. - *

- * A new {@code FlutterEngine} does come with all default system channels attached. - *

- * The first {@code FlutterEngine} instance constructed per process will also load the Flutter - * native library and start a Dart VM. + * Same as {@link #FlutterEngine(Context)} with added support for passing Dart + * VM arguments. *

- * In order to pass Dart VM initialization arguments (see {@link io.flutter.embedding.engine.FlutterShellArgs}) - * when creating the VM, manually set the initialization arguments by calling {@link FlutterLoader#startInitialization(Context)} - * and {@link FlutterLoader#ensureInitializationComplete(Context, String[])}. + * If the Dart VM has already started, the given arguments will have no effect. */ - public FlutterEngine(@NonNull Context context, boolean automaticallyRegisterPlugins) { - this(context, FlutterLoader.getInstance(), new FlutterJNI(), null, automaticallyRegisterPlugins); + public FlutterEngine(@NonNull Context context, @Nullable String[] dartVmArgs) { + this(context, FlutterLoader.getInstance(), new FlutterJNI(), dartVmArgs, true); } /** * Same as {@link #FlutterEngine(Context)} with added support for passing Dart - * VM arguments. + * VM arguments and avoiding automatic plugin registration. *

* If the Dart VM has already started, the given arguments will have no effect. */ - public FlutterEngine(@NonNull Context context, @Nullable String[] dartVmArgs) { - this(context, FlutterLoader.getInstance(), new FlutterJNI(), dartVmArgs, true); + public FlutterEngine(@NonNull Context context, @Nullable String[] dartVmArgs, boolean automaticallyRegisterPlugins) { + this(context, FlutterLoader.getInstance(), new FlutterJNI(), dartVmArgs, automaticallyRegisterPlugins); } + /** * Same as {@link #FlutterEngine(Context, FlutterLoader, FlutterJNI, String[])} but with no Dart * VM flags. diff --git a/shell/platform/android/test/io/flutter/plugins/GeneratedPluginRegistrant.java b/shell/platform/android/test/io/flutter/plugins/GeneratedPluginRegistrant.java new file mode 100644 index 0000000000000..8cf817064bbc7 --- /dev/null +++ b/shell/platform/android/test/io/flutter/plugins/GeneratedPluginRegistrant.java @@ -0,0 +1,48 @@ +package io.flutter.plugins; + +import android.support.annotation.VisibleForTesting; +import io.flutter.embedding.engine.FlutterEngine; +import java.util.List; +import java.util.ArrayList; + +/** + * A fake of the {@code GeneratedPluginRegistrant} normally built by the tool into Flutter apps. + * + *

Used to test engine logic which interacts with the generated class. + */ +@VisibleForTesting +public class GeneratedPluginRegistrant { + private static final List registeredEngines = new ArrayList<>(); + + /** + * The one and only method currently generated by the tool. + * + *

Normally it registers all plugins in an app with the given {@code engine}. This fake tracks + * all registered engines instead. + */ + public static void registerWith(FlutterEngine engine) { + registeredEngines.add(engine); + } + + /** + * Clears the mutable static state regrettably stored in this class. + * + *

{@link #registerWith} is a static call with no visible side effects. In order to verify when + * it's been called we also unfortunately need to store the state statically. This should be + * called before and after each test run accessing this class to make sure the state is clear both + * before and after the run. + */ + public static void clearRegisteredEngines() { + registeredEngines.clear(); + } + + /** + * Returns a list of all the engines registered so far. + * + *

CAUTION: This list is static and must be manually wiped in between test runs. See + * {@link #clearRegisteredEngines()}. + */ + public static List getRegisteredEngines() { + return new ArrayList<>(registeredEngines); + } +} From d518d94f149367f7f6744c85ffddbd1222ffcbb0 Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Fri, 24 Jan 2020 17:04:29 -0800 Subject: [PATCH 3/3] Review feedback --- .../android/io/flutter/embedding/engine/FlutterEngine.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java index a5c086090ba77..1d142a0bf9b2c 100644 --- a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java +++ b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java @@ -157,11 +157,13 @@ public FlutterEngine(@NonNull Context context, @Nullable String[] dartVmArgs) { *

* If the Dart VM has already started, the given arguments will have no effect. */ - public FlutterEngine(@NonNull Context context, @Nullable String[] dartVmArgs, boolean automaticallyRegisterPlugins) { + public FlutterEngine( + @NonNull Context context, + @Nullable String[] dartVmArgs, + boolean automaticallyRegisterPlugins) { this(context, FlutterLoader.getInstance(), new FlutterJNI(), dartVmArgs, automaticallyRegisterPlugins); } - /** * Same as {@link #FlutterEngine(Context, FlutterLoader, FlutterJNI, String[])} but with no Dart * VM flags.