Register plugins at the right time, once (#15979)

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.

Author: Michael Klimushyn

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",
   ]
 

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.
-   * <p>
-   * This method is called after {@link #provideFlutterEngine(Context)}.
+   * Hook for subclasses to easily configure a {@code FlutterEngine}.
+   *
+   * <p>This method is called after {@link #provideFlutterEngine(Context)}.
+   *
+   * <p>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.
+   * <p>
+   * 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.
+   * <p>
+   * 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.");
+    }
+  }
 }

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(), host.getFlutterShellArgs().toArray(), /*automaticallyRegisterPlugins=*/false);
     isFlutterEngineFromHost = false;
   }
 

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)}.
    * <p>
-   * 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()}.
    * <p>
    * A new {@code FlutterEngine} does come with all default system channels attached.
    * <p>
@@ -152,6 +151,19 @@ 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 and avoiding automatic plugin registration.
+   * <p>
+   * If the Dart VM has already started, the given arguments will have no effect.
+   */
+  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.

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<FlutterEngine> 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
         );
     }
   }

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<FlutterEngine> 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());
   }
 }

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.
+ *
+ * <p>Used to test engine logic which interacts with the generated class.
+ */
+@VisibleForTesting
+public class GeneratedPluginRegistrant {
+  private static final List<FlutterEngine> registeredEngines = new ArrayList<>();
+
+  /**
+   * The one and only method currently generated by the tool.
+   *
+   * <p>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.
+   *
+   * <p>{@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.
+   *
+   * <p>CAUTION: This list is static and must be manually wiped in between test runs. See
+   * {@link #clearRegisteredEngines()}.
+   */
+  public static List<FlutterEngine> getRegisteredEngines() {
+    return new ArrayList<>(registeredEngines);
+  }
+}