[webview_flutter_android] Updates Dart and Java InstanceManagers (#3282)

[webview_flutter_android] Updates Dart and Java InstanceManagers

Author: bparrishMines

packages/webview_flutter/webview_flutter_android/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 3.4.3
+
+* Updates internal Java InstanceManager to be cleared on hot restart.
+
 ## 3.4.2
 
 * Clarifies explanation of endorsement in README.

packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/GeneratedAndroidWebView.java

@@ -439,6 +439,51 @@ public interface Result<T> {
 
     void error(Throwable error);
   }
+  /**
+   * Host API for managing the native `InstanceManager`.
+   *
+   * <p>Generated interface from Pigeon that represents a handler of messages from Flutter.
+   */
+  public interface InstanceManagerHostApi {
+    /**
+     * Clear the native `InstanceManager`.
+     *
+     * <p>This is typically only used after a hot restart.
+     */
+    void clear();
+
+    /** The codec used by InstanceManagerHostApi. */
+    static MessageCodec<Object> getCodec() {
+      return new StandardMessageCodec();
+    }
+    /**
+     * Sets up an instance of `InstanceManagerHostApi` to handle messages through the
+     * `binaryMessenger`.
+     */
+    static void setup(BinaryMessenger binaryMessenger, InstanceManagerHostApi api) {
+      {
+        BasicMessageChannel<Object> channel =
+            new BasicMessageChannel<>(
+                binaryMessenger, "dev.flutter.pigeon.InstanceManagerHostApi.clear", getCodec());
+        if (api != null) {
+          channel.setMessageHandler(
+              (message, reply) -> {
+                ArrayList<Object> wrapped = new ArrayList<Object>();
+                try {
+                  api.clear();
+                  wrapped.add(0, null);
+                } catch (Error | RuntimeException exception) {
+                  ArrayList<Object> wrappedError = wrapError(exception);
+                  wrapped = wrappedError;
+                }
+                reply.reply(wrapped);
+              });
+        } else {
+          channel.setMessageHandler(null);
+        }
+      }
+    }
+  }
   /**
    * Handles methods calls to the native Java Object class.
    *

packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java

@@ -30,6 +30,9 @@
  */
 @SuppressWarnings("unchecked")
 public class InstanceManager {
+  /// Constant returned from #addHostCreatedInstance() if the manager is closed.
+  public static final int INSTANCE_CLOSED = -1;
+
   // Identifiers are locked to a specific range to avoid collisions with objects
   // created simultaneously from Dart.
   // Host uses identifiers >= 2^16 and Dart is expected to use values n where,
@@ -87,8 +90,7 @@ private InstanceManager(FinalizationListener finalizationListener) {
    */
   @Nullable
   public <T> T remove(long identifier) {
-    if (isClosed()) {
-      Log.w(TAG, CLOSED_WARNING);
+    if (assertNotClosed()) {
       return null;
     }
     return (T) strongInstances.remove(identifier);
@@ -97,17 +99,22 @@ public <T> T remove(long identifier) {
   /**
    * Retrieves the identifier paired with an instance.
    *
-   * <p>If the manager contains `instance`, as a strong or weak reference, the strong reference to
-   * `instance` will be recreated and will need to be removed again with {@link #remove(long)}.
+   * <p>If the manager contains a strong reference to `instance`, it will return the identifier
+   * associated with `instance`. If the manager contains only a weak reference to `instance`, a new
+   * strong reference to `instance` will be added and will need to be removed again with {@link
+   * #remove(long)}.
+   *
+   * <p>If this method returns a nonnull identifier, this method also expects the Dart
+   * `InstanceManager` to have, or recreate, a weak reference to the Dart instance the identifier is
+   * associated with.
    *
    * @param instance an instance that may be stored in the manager.
    * @return the identifier associated with `instance` if the manager contains the value, otherwise
    *     null if the manager doesn't contain the value or the manager is closed.
    */
   @Nullable
   public Long getIdentifierForStrongReference(Object instance) {
-    if (isClosed()) {
-      Log.w(TAG, CLOSED_WARNING);
+    if (assertNotClosed()) {
       return null;
     }
     final Long identifier = identifiers.get(instance);
@@ -120,18 +127,18 @@ public Long getIdentifierForStrongReference(Object instance) {
   /**
    * Adds a new instance that was instantiated from Dart.
    *
-   * <p>If an instance or identifier has already been added, it will be replaced by the new values.
-   * The Dart InstanceManager is considered the source of truth and has the capability to overwrite
-   * stored pairs in response to hot restarts.
+   * <p>The same instance can be added multiple times, but each identifier must be unique. This
+   * allows two objects that are equivalent (e.g. the `equals` method returns true and their
+   * hashcodes are equal) to both be added.
    *
-   * <p>If the manager is closed, the addition is ignored.
+   * <p>If the manager is closed, the addition is ignored and a warning is logged.
    *
    * @param instance the instance to be stored.
-   * @param identifier the identifier to be paired with instance. This value must be >= 0.
+   * @param identifier the identifier to be paired with instance. This value must be >= 0 and
+   *     unique.
    */
   public void addDartCreatedInstance(Object instance, long identifier) {
-    if (isClosed()) {
-      Log.w(TAG, CLOSED_WARNING);
+    if (assertNotClosed()) {
       return;
     }
     addInstance(instance, identifier);
@@ -140,13 +147,18 @@ public void addDartCreatedInstance(Object instance, long identifier) {
   /**
    * Adds a new instance that was instantiated from the host platform.
    *
-   * @param instance the instance to be stored.
+   * @param instance the instance to be stored. This must be unique to all other added instances.
    * @return the unique identifier stored with instance. If the manager is closed, returns -1.
+   *     Otherwise, returns a value >= 0.
    */
   public long addHostCreatedInstance(Object instance) {
-    if (isClosed()) {
-      Log.w(TAG, CLOSED_WARNING);
-      return -1;
+    if (assertNotClosed()) {
+      return INSTANCE_CLOSED;
+    }
+
+    if (containsInstance(instance)) {
+      throw new IllegalArgumentException(
+          String.format("Instance of `%s` has already been added.", instance.getClass()));
     }
     final long identifier = nextIdentifier++;
     addInstance(instance, identifier);
@@ -156,22 +168,22 @@ public long addHostCreatedInstance(Object instance) {
   /**
    * Retrieves the instance associated with identifier.
    *
-   * @param identifier the identifier paired to an instance.
+   * @param identifier the identifier associated with an instance.
    * @param <T> the expected return type.
    * @return the instance associated with `identifier` if the manager contains the value, otherwise
    *     null if the manager doesn't contain the value or the manager is closed.
    */
   @Nullable
   public <T> T getInstance(long identifier) {
-    if (isClosed()) {
-      Log.w(TAG, CLOSED_WARNING);
+    if (assertNotClosed()) {
       return null;
     }
+
     final WeakReference<T> instance = (WeakReference<T>) weakInstances.get(identifier);
     if (instance != null) {
       return instance.get();
     }
-    return (T) strongInstances.get(identifier);
+    return null;
   }
 
   /**
@@ -182,8 +194,7 @@ public <T> T getInstance(long identifier) {
    *     `false`.
    */
   public boolean containsInstance(Object instance) {
-    if (isClosed()) {
-      Log.w(TAG, CLOSED_WARNING);
+    if (assertNotClosed()) {
       return false;
     }
     return identifiers.containsKey(instance);
@@ -197,14 +208,23 @@ public boolean containsInstance(Object instance) {
   public void close() {
     handler.removeCallbacks(this::releaseAllFinalizedInstances);
     isClosed = true;
+    clear();
+  }
+
+  /**
+   * Removes all of the instances from this manager.
+   *
+   * <p>The manager will be empty after this call returns.
+   */
+  public void clear() {
     identifiers.clear();
     weakInstances.clear();
     strongInstances.clear();
     weakReferencesToIdentifiers.clear();
   }
 
   /**
-   * Whether the manager has released resources and is not longer usable.
+   * Whether the manager has released resources and is no longer usable.
    *
    * <p>See {@link #close()}.
    */
@@ -228,12 +248,24 @@ private void releaseAllFinalizedInstances() {
 
   private void addInstance(Object instance, long identifier) {
     if (identifier < 0) {
-      throw new IllegalArgumentException("Identifier must be >= 0.");
+      throw new IllegalArgumentException(String.format("Identifier must be >= 0: %d", identifier));
+    }
+    if (weakInstances.containsKey(identifier)) {
+      throw new IllegalArgumentException(
+          String.format("Identifier has already been added: %d", identifier));
     }
     final WeakReference<Object> weakReference = new WeakReference<>(instance, referenceQueue);
     identifiers.put(instance, identifier);
     weakInstances.put(identifier, weakReference);
     weakReferencesToIdentifiers.put(weakReference, identifier);
     strongInstances.put(identifier, instance);
   }
+
+  private boolean assertNotClosed() {
+    if (isClosed()) {
+      Log.w(TAG, CLOSED_WARNING);
+      return true;
+    }
+    return false;
+  }
 }

packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterPlugin.java

@@ -17,6 +17,7 @@
 import io.flutter.plugins.webviewflutter.GeneratedAndroidWebView.CookieManagerHostApi;
 import io.flutter.plugins.webviewflutter.GeneratedAndroidWebView.DownloadListenerHostApi;
 import io.flutter.plugins.webviewflutter.GeneratedAndroidWebView.FlutterAssetManagerHostApi;
+import io.flutter.plugins.webviewflutter.GeneratedAndroidWebView.InstanceManagerHostApi;
 import io.flutter.plugins.webviewflutter.GeneratedAndroidWebView.JavaObjectHostApi;
 import io.flutter.plugins.webviewflutter.GeneratedAndroidWebView.JavaScriptChannelHostApi;
 import io.flutter.plugins.webviewflutter.GeneratedAndroidWebView.WebChromeClientHostApi;
@@ -84,6 +85,8 @@ private void setUp(
                 new GeneratedAndroidWebView.JavaObjectFlutterApi(binaryMessenger)
                     .dispose(identifier, reply -> {}));
 
+    InstanceManagerHostApi.setup(binaryMessenger, () -> instanceManager.clear());
+
     viewRegistry.registerViewFactory(
         "plugins.flutter.io/webview", new FlutterWebViewFactory(instanceManager));
 

packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java

@@ -108,4 +108,67 @@ public void containsInstanceReturnsFalseWhenClosed() {
 
     assertFalse(instanceManager.containsInstance(object));
   }
+
+  @Test
+  public void clear() {
+    final InstanceManager instanceManager = InstanceManager.open(identifier -> {});
+
+    final Object instance = new Object();
+
+    instanceManager.addDartCreatedInstance(instance, 0);
+    assertTrue(instanceManager.containsInstance(instance));
+
+    instanceManager.clear();
+    assertFalse(instanceManager.containsInstance(instance));
+
+    instanceManager.close();
+  }
+
+  @Test
+  public void canAddSameObjectWithAddDartCreatedInstance() {
+    final InstanceManager instanceManager = InstanceManager.open(identifier -> {});
+
+    final Object instance = new Object();
+
+    instanceManager.addDartCreatedInstance(instance, 0);
+    instanceManager.addDartCreatedInstance(instance, 1);
+
+    assertTrue(instanceManager.containsInstance(instance));
+
+    assertEquals(instanceManager.getInstance(0), instance);
+    assertEquals(instanceManager.getInstance(1), instance);
+
+    instanceManager.close();
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void cannotAddSameObjectsWithAddHostCreatedInstance() {
+    final InstanceManager instanceManager = InstanceManager.open(identifier -> {});
+
+    final Object instance = new Object();
+
+    instanceManager.addHostCreatedInstance(instance);
+    instanceManager.addHostCreatedInstance(instance);
+
+    instanceManager.close();
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void cannotUseIdentifierLessThanZero() {
+    final InstanceManager instanceManager = InstanceManager.open(identifier -> {});
+
+    instanceManager.addDartCreatedInstance(new Object(), -1);
+
+    instanceManager.close();
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void identifiersMustBeUnique() {
+    final InstanceManager instanceManager = InstanceManager.open(identifier -> {});
+
+    instanceManager.addDartCreatedInstance(new Object(), 0);
+    instanceManager.addDartCreatedInstance(new Object(), 0);
+
+    instanceManager.close();
+  }
 }

packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/WebViewClientTest.java

@@ -118,8 +118,8 @@ public WebViewClient createWebViewClient(WebViewClientFlutterApiImpl flutterApi)
             },
             mockFlutterApi);
 
-    instanceManager.addDartCreatedInstance(mockWebViewClient, 0);
-    webViewClientHostApi.setSynchronousReturnValueForShouldOverrideUrlLoading(0L, false);
+    instanceManager.addDartCreatedInstance(mockWebViewClient, 2);
+    webViewClientHostApi.setSynchronousReturnValueForShouldOverrideUrlLoading(2L, false);
 
     verify(mockWebViewClient).setReturnValueForShouldOverrideUrlLoading(false);
   }

packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/WebViewTest.java

@@ -310,9 +310,9 @@ public void destroy() {
           }
         };
 
-    testInstanceManager.addDartCreatedInstance(webView, 0);
+    testInstanceManager.addDartCreatedInstance(webView, 1);
     final JavaObjectHostApiImpl javaObjectHostApi = new JavaObjectHostApiImpl(testInstanceManager);
-    javaObjectHostApi.dispose(0L);
+    javaObjectHostApi.dispose(1L);
 
     assertTrue(destroyCalled[0]);
   }

packages/webview_flutter/webview_flutter_android/example/integration_test/webview_flutter_test.dart

@@ -18,6 +18,7 @@ import 'package:flutter/services.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:integration_test/integration_test.dart';
 import 'package:webview_flutter_android/src/android_webview.dart' as android;
+import 'package:webview_flutter_android/src/android_webview.g.dart';
 import 'package:webview_flutter_android/src/android_webview_api_impls.dart';
 import 'package:webview_flutter_android/src/instance_manager.dart';
 import 'package:webview_flutter_android/src/weak_reference_utils.dart';
@@ -111,6 +112,11 @@ Future<void> main() async {
         }
       });
 
+      // Since the InstanceManager of the apis are being changed, the native
+      // InstanceManager needs to be cleared otherwise an exception will be
+      // thrown that an identifier is being reused.
+      await InstanceManagerHostApi().clear();
+
       android.WebView.api = WebViewHostApiImpl(
         instanceManager: instanceManager,
       );