From ae74f22f24fbf22323e4233881540c63e21a53e4 Mon Sep 17 00:00:00 2001 From: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com> Date: Wed, 11 Jan 2023 12:47:40 -0500 Subject: [PATCH 1/6] dont throw errors on all instance manager methods --- .../webviewflutter/InstanceManager.java | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java index fefd577ee9b5..6693948a1736 100644 --- a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java +++ b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java @@ -95,11 +95,13 @@ public T remove(long identifier) { * * @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. + * null if the manager doesn't contain the value or the manager is closed. */ @Nullable public Long getIdentifierForStrongReference(Object instance) { - assertManagerIsNotClosed(); + if (isClosed()) { + return null; + } final Long identifier = identifiers.get(instance); if (identifier != null) { strongInstances.put(identifier, instance); @@ -126,10 +128,12 @@ 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. - * @return the unique identifier stored with instance. + * @return the unique identifier stored with instance. If the manager is closed, returns -1. */ public long addHostCreatedInstance(Object instance) { - assertManagerIsNotClosed(); + if (isClosed()) { + return -1; + } final long identifier = nextIdentifier++; addInstance(instance, identifier); return identifier; @@ -157,10 +161,13 @@ public T getInstance(long identifier) { * Returns whether this manager contains the given `instance`. * * @param instance the instance whose presence in this manager is to be tested. - * @return whether this manager contains the given `instance`. + * @return whether this manager contains the given `instance`. If the manager is closed, returns + * `false`. */ public boolean containsInstance(Object instance) { - assertManagerIsNotClosed(); + if (isClosed()) { + return false; + } return identifiers.containsKey(instance); } @@ -173,6 +180,19 @@ public boolean containsInstance(Object instance) { public void close() { handler.removeCallbacks(this::releaseAllFinalizedInstances); isClosed = true; + identifiers.clear(); + weakInstances.clear(); + strongInstances.clear(); + weakReferencesToIdentifiers.clear(); + } + + /** + * Whether the manager has released resources and is not longer usable. + * + *

See {@link #close()}. + */ + public boolean isClosed() { + return isClosed; } private void releaseAllFinalizedInstances() { From 779c53ad4aae1f0224d41fbbd4c8d7dc2de6773a Mon Sep 17 00:00:00 2001 From: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com> Date: Wed, 11 Jan 2023 12:57:39 -0500 Subject: [PATCH 2/6] tests --- .../webviewflutter/InstanceManager.java | 2 +- .../webviewflutter/InstanceManagerTest.java | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java index 6693948a1736..3579d6b575b2 100644 --- a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java +++ b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java @@ -162,7 +162,7 @@ public T getInstance(long identifier) { * * @param instance the instance whose presence in this manager is to be tested. * @return whether this manager contains the given `instance`. If the manager is closed, returns - * `false`. + * `false`. */ public boolean containsInstance(Object instance) { if (isClosed()) { diff --git a/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java b/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java index 4731e2a4beb1..c2a3396eac0f 100644 --- a/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java +++ b/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java @@ -5,6 +5,7 @@ package io.flutter.plugins.webviewflutter; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -59,4 +60,32 @@ public void remove() { instanceManager.close(); } + + @Test + public void getIdentifierForStrongReferenceReturnsNullWhenClosed() { + final Object object = new Object(); + final InstanceManager instanceManager = InstanceManager.open(identifier -> {}); + instanceManager.addDartCreatedInstance(object, 0); + instanceManager.close(); + + assertNull(instanceManager.getIdentifierForStrongReference(object)); + } + + @Test + public void addHostCreatedInstanceReturnsNegativeOneWhenClosed() { + final InstanceManager instanceManager = InstanceManager.open(identifier -> {}); + instanceManager.close(); + + assertEquals(instanceManager.addHostCreatedInstance(new Object()), -1L); + } + + @Test + public void containsInstanceReturnsFalseWhenClosed() { + final Object object = new Object(); + final InstanceManager instanceManager = InstanceManager.open(identifier -> {}); + instanceManager.addDartCreatedInstance(object, 0); + instanceManager.close(); + + assertFalse(instanceManager.containsInstance(object)); + } } From d7ad191e75af80ff08ba820bcd07a4f1bd7b7269 Mon Sep 17 00:00:00 2001 From: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com> Date: Wed, 11 Jan 2023 13:04:06 -0500 Subject: [PATCH 3/6] version bump --- packages/webview_flutter/webview_flutter_android/CHANGELOG.md | 4 ++++ packages/webview_flutter/webview_flutter_android/pubspec.yaml | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/webview_flutter/webview_flutter_android/CHANGELOG.md b/packages/webview_flutter/webview_flutter_android/CHANGELOG.md index b5502ecd2577..cc5384d541f3 100644 --- a/packages/webview_flutter/webview_flutter_android/CHANGELOG.md +++ b/packages/webview_flutter/webview_flutter_android/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.1.2 + +* Fixes crash when the Java `InstanceManager` was used after plugin was removed from engine. + ## 3.1.1 * Fixes bug where a `AndroidNavigationDelegate` was required to load a request. diff --git a/packages/webview_flutter/webview_flutter_android/pubspec.yaml b/packages/webview_flutter/webview_flutter_android/pubspec.yaml index 72bc2e43414a..57346e147e7a 100644 --- a/packages/webview_flutter/webview_flutter_android/pubspec.yaml +++ b/packages/webview_flutter/webview_flutter_android/pubspec.yaml @@ -2,7 +2,7 @@ name: webview_flutter_android description: A Flutter plugin that provides a WebView widget on Android. repository: https://github.com/flutter/plugins/tree/main/packages/webview_flutter/webview_flutter_android issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+webview%22 -version: 3.1.1 +version: 3.1.2 environment: sdk: ">=2.17.0 <3.0.0" From caaf806d42cfcae16212c8bd753702c7275dd19f Mon Sep 17 00:00:00 2001 From: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com> Date: Wed, 11 Jan 2023 13:52:45 -0500 Subject: [PATCH 4/6] change to logging a warning and ignore calls in other methods --- .../webviewflutter/InstanceManager.java | 30 ++++++++++++------- .../webviewflutter/InstanceManagerTest.java | 20 +++++++++++++ 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java index 3579d6b575b2..efcd3251bb19 100644 --- a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java +++ b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java @@ -6,6 +6,7 @@ import android.os.Handler; import android.os.Looper; +import android.util.Log; import androidx.annotation.Nullable; import java.lang.ref.ReferenceQueue; import java.lang.ref.WeakReference; @@ -35,6 +36,8 @@ public class InstanceManager { // 0 <= n < 2^16. private static final long MIN_HOST_CREATED_IDENTIFIER = 65536; private static final long CLEAR_FINALIZED_WEAK_REFERENCES_INTERVAL = 30000; + private static final String TAG = "InstanceManager"; + private static final String CLOSED_WARNING = "Method was called while the manager was closed."; /** Interface for listening when a weak reference of an instance is removed from the manager. */ public interface FinalizationListener { @@ -83,7 +86,10 @@ private InstanceManager(FinalizationListener finalizationListener) { */ @Nullable public T remove(long identifier) { - assertManagerIsNotClosed(); + if (isClosed()) { + Log.w(TAG, CLOSED_WARNING); + return null; + } return (T) strongInstances.remove(identifier); } @@ -100,6 +106,7 @@ public T remove(long identifier) { @Nullable public Long getIdentifierForStrongReference(Object instance) { if (isClosed()) { + Log.w(TAG, CLOSED_WARNING); return null; } final Long identifier = identifiers.get(instance); @@ -120,7 +127,10 @@ public Long getIdentifierForStrongReference(Object instance) { * @param identifier the identifier to be paired with instance. This value must be >= 0. */ public void addDartCreatedInstance(Object instance, long identifier) { - assertManagerIsNotClosed(); + if (isClosed()) { + Log.w(TAG, CLOSED_WARNING); + return; + } addInstance(instance, identifier); } @@ -132,6 +142,7 @@ public void addDartCreatedInstance(Object instance, long identifier) { */ public long addHostCreatedInstance(Object instance) { if (isClosed()) { + Log.w(TAG, CLOSED_WARNING); return -1; } final long identifier = nextIdentifier++; @@ -149,7 +160,10 @@ public long addHostCreatedInstance(Object instance) { */ @Nullable public T getInstance(long identifier) { - assertManagerIsNotClosed(); + if (isClosed()) { + Log.w(TAG, CLOSED_WARNING); + return null; + } final WeakReference instance = (WeakReference) weakInstances.get(identifier); if (instance != null) { return instance.get(); @@ -166,6 +180,7 @@ public T getInstance(long identifier) { */ public boolean containsInstance(Object instance) { if (isClosed()) { + Log.w(TAG, CLOSED_WARNING); return false; } return identifiers.containsKey(instance); @@ -174,8 +189,7 @@ public boolean containsInstance(Object instance) { /** * Closes the manager and releases resources. * - *

Calling a method after calling this one will throw an {@link AssertionError}. This method - * excluded. + *

Methods called after this one will be ignored and log a warning. */ public void close() { handler.removeCallbacks(this::releaseAllFinalizedInstances); @@ -219,10 +233,4 @@ private void addInstance(Object instance, long identifier) { weakReferencesToIdentifiers.put(weakReference, identifier); strongInstances.put(identifier, instance); } - - private void assertManagerIsNotClosed() { - if (isClosed) { - throw new AssertionError("Manager has already been closed."); - } - } } diff --git a/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java b/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java index c2a3396eac0f..6a19c883548a 100644 --- a/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java +++ b/packages/webview_flutter/webview_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/InstanceManagerTest.java @@ -61,6 +61,16 @@ public void remove() { instanceManager.close(); } + @Test + public void removeReturnsNullWhenClosed() { + final Object object = new Object(); + final InstanceManager instanceManager = InstanceManager.open(identifier -> {}); + instanceManager.addDartCreatedInstance(object, 0); + instanceManager.close(); + + assertNull(instanceManager.remove(0)); + } + @Test public void getIdentifierForStrongReferenceReturnsNullWhenClosed() { final Object object = new Object(); @@ -79,6 +89,16 @@ public void addHostCreatedInstanceReturnsNegativeOneWhenClosed() { assertEquals(instanceManager.addHostCreatedInstance(new Object()), -1L); } + @Test + public void getInstanceReturnsNullWhenClosed() { + final Object object = new Object(); + final InstanceManager instanceManager = InstanceManager.open(identifier -> {}); + instanceManager.addDartCreatedInstance(object, 0); + instanceManager.close(); + + assertNull(instanceManager.getInstance(0)); + } + @Test public void containsInstanceReturnsFalseWhenClosed() { final Object object = new Object(); From bd30156b02e3df6473247df65a1ff7ba22c23931 Mon Sep 17 00:00:00 2001 From: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com> Date: Wed, 11 Jan 2023 13:55:49 -0500 Subject: [PATCH 5/6] the --- packages/webview_flutter/webview_flutter_android/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/webview_flutter/webview_flutter_android/CHANGELOG.md b/packages/webview_flutter/webview_flutter_android/CHANGELOG.md index cc5384d541f3..40cdaf21da7c 100644 --- a/packages/webview_flutter/webview_flutter_android/CHANGELOG.md +++ b/packages/webview_flutter/webview_flutter_android/CHANGELOG.md @@ -1,6 +1,6 @@ ## 3.1.2 -* Fixes crash when the Java `InstanceManager` was used after plugin was removed from engine. +* Fixes crash when the Java `InstanceManager` was used after plugin was removed from the engine. ## 3.1.1 From 40402f07895941f9d2b4a64e4fcec960a20c6e6f Mon Sep 17 00:00:00 2001 From: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com> Date: Wed, 11 Jan 2023 14:03:13 -0500 Subject: [PATCH 6/6] documentation --- .../io/flutter/plugins/webviewflutter/InstanceManager.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java index efcd3251bb19..55775a914c56 100644 --- a/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java +++ b/packages/webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java @@ -82,7 +82,8 @@ private InstanceManager(FinalizationListener finalizationListener) { * * @param identifier the identifier paired to an instance. * @param the expected return type. - * @return the removed instance if the manager contains the given identifier, otherwise null. + * @return the removed instance if the manager contains the given identifier, otherwise null if + * the manager doesn't contain the value or the manager is closed. */ @Nullable public T remove(long identifier) { @@ -123,6 +124,8 @@ public Long getIdentifierForStrongReference(Object instance) { * The Dart InstanceManager is considered the source of truth and has the capability to overwrite * stored pairs in response to hot restarts. * + *

If the manager is closed, the addition is ignored. + * * @param instance the instance to be stored. * @param identifier the identifier to be paired with instance. This value must be >= 0. */ @@ -156,7 +159,7 @@ public long addHostCreatedInstance(Object instance) { * @param identifier the identifier paired to an instance. * @param the expected return type. * @return the instance associated with `identifier` if the manager contains the value, otherwise - * null. + * null if the manager doesn't contain the value or the manager is closed. */ @Nullable public T getInstance(long identifier) {