[pigeon] Fixes support for Kotlin/Java classes that override equals and hashCode for ProxyApis (#10039)

ProxyApis for classes that override `equals()/hashcode` in Kotlin/Java were breaking the `InstanceManager`.

The Kotlin `InstanceManager` would:
1. Return true for `containsInstance`.
2. Then remove the strong reference to the original object in `getIdentifierForStrongReference`. 
3. Erroneously trigger the call to Dart to remove the strong reference of an instance.

This changes the the use of `WeakReference` to a new `IdentityWeakReference` that uses identity to check for equality. Java/Kotlin has an `IdentityHashMap` and a `WeakHashMap`, but not an `IdentityWeakHashMap`. So this is a quick workaround to create an equivalent.

Alternatively, `getIdentifierForStrongReference` could be updated to check `containsInstance` first, but this would leave small timeframes where objects would only have a weak reference. And it seems possible that Kotlin GC could cleanup at any moment. 

Potential fix for flutter/flutter#174134 since the class they were creating a ProxyApi for was a data class: https://github.com/JavesonYehudi/proxy_bug_report/blob/main/android/src/main/kotlin/com/example/proxy_bug_report/auth/Credentials.kt

## Pre-Review Checklist

**Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

[^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

Author: bparrishMines

packages/pigeon/CHANGELOG.md

@@ -1,5 +1,7 @@
-## NEXT
+## 26.0.2
 
+* [kotlin] Fixes support for classes that override equals and hashCode for ProxyApis.
+* [kotlin] Adds error message log when a new Dart proxy instance fails to be created.
 * Updates minimum supported SDK version to Flutter 3.29/Dart 3.7.
 
 ## 26.0.1

packages/pigeon/lib/src/generator_tools.dart

@@ -15,7 +15,7 @@ import 'generator.dart';
 /// The current version of pigeon.
 ///
 /// This must match the version in pubspec.yaml.
-const String pigeonVersion = '26.0.1';
+const String pigeonVersion = '26.0.2';
 
 /// Read all the content from [stdin] to a String.
 String readStdin() {

packages/pigeon/lib/src/kotlin/kotlin_generator.dart

@@ -1031,6 +1031,15 @@ if (wrapped == null) {
             });
             indent.newln();
 
+            indent.format('''
+              fun logNewInstanceFailure(apiName: String, value: Any, exception: Throwable?) {
+                Log.w(
+                  "${proxyApiCodecName(const InternalKotlinOptions(kotlinOut: ''))}",
+                  "Failed to create new Dart proxy instance of \$apiName: \$value. \$exception"
+                )
+              }
+              ''');
+
             enumerate(sortedApis, (int index, AstProxyApi api) {
               final String className =
                   api.kotlinOptions?.fullClassName ?? api.name;
@@ -1043,7 +1052,11 @@ if (wrapped == null) {
 
               indent.format('''
                   ${index > 0 ? ' else ' : ''}if (${versionCheck}value is $className) {
-                    registrar.get$hostProxyApiPrefix${api.name}().${classMemberNamePrefix}newInstance(value) { }
+                    registrar.get$hostProxyApiPrefix${api.name}().${classMemberNamePrefix}newInstance(value) {
+                      if (it.isFailure) {
+                        logNewInstanceFailure("${api.name}", value, it.exceptionOrNull())
+                      }
+                    }
                   }''');
             });
             indent.newln();

packages/pigeon/lib/src/kotlin/templates.dart

@@ -42,11 +42,39 @@ class ${kotlinInstanceManagerClassName(options)}(private val finalizationListene
     fun onFinalize(identifier: Long)
   }
 
-  private val identifiers = java.util.WeakHashMap<Any, Long>()
-  private val weakInstances = HashMap<Long, java.lang.ref.WeakReference<Any>>()
+  // Extends WeakReference and overrides the `equals` and `hashCode` methods using identity rather
+  // than equality.
+  //
+  // Two `IdentityWeakReference`s are equal if either
+  // 1: `get()` returns the identical nonnull value for both references.
+  // 2: `get()` returns null for both references and the references are identical.
+  class IdentityWeakReference<T : Any> : java.lang.ref.WeakReference<T> {
+    private val savedHashCode: Int
+
+    constructor(instance: T) : this(instance, null)
+
+    constructor(instance: T, queue: java.lang.ref.ReferenceQueue<T>?) : super(instance, queue) {
+      savedHashCode = System.identityHashCode(instance)
+    }
+
+    override fun equals(other: Any?): Boolean {
+      val instance = get()
+      if (instance != null) {
+        return other is IdentityWeakReference<*> && other.get() === instance
+      }
+      return other === this
+    }
+
+    override fun hashCode(): Int {
+      return savedHashCode
+    }
+  }
+
+  private val identifiers = java.util.WeakHashMap<IdentityWeakReference<Any>, Long>()
+  private val weakInstances = HashMap<Long, IdentityWeakReference<Any>>()
   private val strongInstances = HashMap<Long, Any>()
   private val referenceQueue = java.lang.ref.ReferenceQueue<Any>()
-  private val weakReferencesToIdentifiers = HashMap<java.lang.ref.WeakReference<Any>, Long>()
+  private val weakReferencesToIdentifiers = HashMap<IdentityWeakReference<Any>, Long>()
   private val handler = android.os.Handler(android.os.Looper.getMainLooper())
   private val releaseAllFinalizedInstancesRunnable = Runnable {
     this.releaseAllFinalizedInstances()
@@ -112,9 +140,12 @@ class ${kotlinInstanceManagerClassName(options)}(private val finalizationListene
    */
   fun getIdentifierForStrongReference(instance: Any?): Long? {
     logWarningIfFinalizationListenerHasStopped()
-    val identifier = identifiers[instance]
+    if (instance == null) {
+      return null
+    }
+    val identifier = identifiers[IdentityWeakReference(instance)]
     if (identifier != null) {
-      strongInstances[identifier] = instance!!
+      strongInstances[identifier] = instance
     }
     return identifier
   }
@@ -151,14 +182,14 @@ class ${kotlinInstanceManagerClassName(options)}(private val finalizationListene
   /** Retrieves the instance associated with identifier, if present, otherwise `null`. */
   fun <T> getInstance(identifier: Long): T? {
     logWarningIfFinalizationListenerHasStopped()
-    val instance = weakInstances[identifier] as java.lang.ref.WeakReference<T>?
+    val instance = weakInstances[identifier] as IdentityWeakReference<T>?
     return instance?.get()
   }
 
   /** Returns whether this manager contains the given `instance`. */
   fun containsInstance(instance: Any?): Boolean {
     logWarningIfFinalizationListenerHasStopped()
-    return identifiers.containsKey(instance)
+    return instance != null && identifiers.containsKey(IdentityWeakReference(instance))
   }
 
   /**
@@ -199,8 +230,8 @@ class ${kotlinInstanceManagerClassName(options)}(private val finalizationListene
     if (hasFinalizationListenerStopped()) {
       return
     }
-    var reference: java.lang.ref.WeakReference<Any>?
-    while ((referenceQueue.poll() as java.lang.ref.WeakReference<Any>?).also { reference = it } != null) {
+    var reference: IdentityWeakReference<Any>?
+    while ((referenceQueue.poll() as IdentityWeakReference<Any>?).also { reference = it } != null) {
       val identifier = weakReferencesToIdentifiers.remove(reference)
       if (identifier != null) {
         weakInstances.remove(identifier)
@@ -216,8 +247,8 @@ class ${kotlinInstanceManagerClassName(options)}(private val finalizationListene
     require(!weakInstances.containsKey(identifier)) {
       "Identifier has already been added: \$identifier"
     }
-    val weakReference = java.lang.ref.WeakReference(instance, referenceQueue)
-    identifiers[instance] = identifier
+    val weakReference = IdentityWeakReference(instance, referenceQueue)
+    identifiers[weakReference] = identifier
     weakInstances[identifier] = weakReference
     weakReferencesToIdentifiers[weakReference] = identifier
     strongInstances[identifier] = instance

packages/pigeon/platform_tests/test_plugin/android/src/main/kotlin/com/example/test_plugin/ProxyApiTests.gen.kt

@@ -75,11 +75,39 @@ class ProxyApiTestsPigeonInstanceManager(
     fun onFinalize(identifier: Long)
   }
 
-  private val identifiers = java.util.WeakHashMap<Any, Long>()
-  private val weakInstances = HashMap<Long, java.lang.ref.WeakReference<Any>>()
+  // Extends WeakReference and overrides the `equals` and `hashCode` methods using identity rather
+  // than equality.
+  //
+  // Two `IdentityWeakReference`s are equal if either
+  // 1: `get()` returns the identical nonnull value for both references.
+  // 2: `get()` returns null for both references and the references are identical.
+  class IdentityWeakReference<T : Any> : java.lang.ref.WeakReference<T> {
+    private val savedHashCode: Int
+
+    constructor(instance: T) : this(instance, null)
+
+    constructor(instance: T, queue: java.lang.ref.ReferenceQueue<T>?) : super(instance, queue) {
+      savedHashCode = System.identityHashCode(instance)
+    }
+
+    override fun equals(other: Any?): Boolean {
+      val instance = get()
+      if (instance != null) {
+        return other is IdentityWeakReference<*> && other.get() === instance
+      }
+      return other === this
+    }
+
+    override fun hashCode(): Int {
+      return savedHashCode
+    }
+  }
+
+  private val identifiers = java.util.WeakHashMap<IdentityWeakReference<Any>, Long>()
+  private val weakInstances = HashMap<Long, IdentityWeakReference<Any>>()
   private val strongInstances = HashMap<Long, Any>()
   private val referenceQueue = java.lang.ref.ReferenceQueue<Any>()
-  private val weakReferencesToIdentifiers = HashMap<java.lang.ref.WeakReference<Any>, Long>()
+  private val weakReferencesToIdentifiers = HashMap<IdentityWeakReference<Any>, Long>()
   private val handler = android.os.Handler(android.os.Looper.getMainLooper())
   private val releaseAllFinalizedInstancesRunnable = Runnable {
     this.releaseAllFinalizedInstances()
@@ -144,9 +172,12 @@ class ProxyApiTestsPigeonInstanceManager(
    */
   fun getIdentifierForStrongReference(instance: Any?): Long? {
     logWarningIfFinalizationListenerHasStopped()
-    val identifier = identifiers[instance]
+    if (instance == null) {
+      return null
+    }
+    val identifier = identifiers[IdentityWeakReference(instance)]
     if (identifier != null) {
-      strongInstances[identifier] = instance!!
+      strongInstances[identifier] = instance
     }
     return identifier
   }
@@ -185,14 +216,14 @@ class ProxyApiTestsPigeonInstanceManager(
   /** Retrieves the instance associated with identifier, if present, otherwise `null`. */
   fun <T> getInstance(identifier: Long): T? {
     logWarningIfFinalizationListenerHasStopped()
-    val instance = weakInstances[identifier] as java.lang.ref.WeakReference<T>?
+    val instance = weakInstances[identifier] as IdentityWeakReference<T>?
     return instance?.get()
   }
 
   /** Returns whether this manager contains the given `instance`. */
   fun containsInstance(instance: Any?): Boolean {
     logWarningIfFinalizationListenerHasStopped()
-    return identifiers.containsKey(instance)
+    return instance != null && identifiers.containsKey(IdentityWeakReference(instance))
   }
 
   /**
@@ -233,9 +264,8 @@ class ProxyApiTestsPigeonInstanceManager(
     if (hasFinalizationListenerStopped()) {
       return
     }
-    var reference: java.lang.ref.WeakReference<Any>?
-    while ((referenceQueue.poll() as java.lang.ref.WeakReference<Any>?).also { reference = it } !=
-        null) {
+    var reference: IdentityWeakReference<Any>?
+    while ((referenceQueue.poll() as IdentityWeakReference<Any>?).also { reference = it } != null) {
       val identifier = weakReferencesToIdentifiers.remove(reference)
       if (identifier != null) {
         weakInstances.remove(identifier)
@@ -251,8 +281,8 @@ class ProxyApiTestsPigeonInstanceManager(
     require(!weakInstances.containsKey(identifier)) {
       "Identifier has already been added: $identifier"
     }
-    val weakReference = java.lang.ref.WeakReference(instance, referenceQueue)
-    identifiers[instance] = identifier
+    val weakReference = IdentityWeakReference(instance, referenceQueue)
+    identifiers[weakReference] = identifier
     weakInstances[identifier] = weakReference
     weakReferencesToIdentifiers[weakReference] = identifier
     strongInstances[identifier] = instance
@@ -460,14 +490,36 @@ private class ProxyApiTestsPigeonProxyApiBaseCodec(
       return
     }
 
+    fun logNewInstanceFailure(apiName: String, value: Any, exception: Throwable?) {
+      Log.w(
+          "PigeonProxyApiBaseCodec",
+          "Failed to create new Dart proxy instance of $apiName: $value. $exception")
+    }
+
     if (value is ProxyApiTestClass) {
-      registrar.getPigeonApiProxyApiTestClass().pigeon_newInstance(value) {}
+      registrar.getPigeonApiProxyApiTestClass().pigeon_newInstance(value) {
+        if (it.isFailure) {
+          logNewInstanceFailure("ProxyApiTestClass", value, it.exceptionOrNull())
+        }
+      }
     } else if (value is com.example.test_plugin.ProxyApiSuperClass) {
-      registrar.getPigeonApiProxyApiSuperClass().pigeon_newInstance(value) {}
+      registrar.getPigeonApiProxyApiSuperClass().pigeon_newInstance(value) {
+        if (it.isFailure) {
+          logNewInstanceFailure("ProxyApiSuperClass", value, it.exceptionOrNull())
+        }
+      }
     } else if (value is ProxyApiInterface) {
-      registrar.getPigeonApiProxyApiInterface().pigeon_newInstance(value) {}
+      registrar.getPigeonApiProxyApiInterface().pigeon_newInstance(value) {
+        if (it.isFailure) {
+          logNewInstanceFailure("ProxyApiInterface", value, it.exceptionOrNull())
+        }
+      }
     } else if (android.os.Build.VERSION.SDK_INT >= 25 && value is ClassWithApiRequirement) {
-      registrar.getPigeonApiClassWithApiRequirement().pigeon_newInstance(value) {}
+      registrar.getPigeonApiClassWithApiRequirement().pigeon_newInstance(value) {
+        if (it.isFailure) {
+          logNewInstanceFailure("ClassWithApiRequirement", value, it.exceptionOrNull())
+        }
+      }
     }
 
     when {

packages/pigeon/platform_tests/test_plugin/android/src/test/kotlin/com/example/test_plugin/InstanceManagerTest.kt

@@ -149,10 +149,97 @@ class InstanceManagerTest {
     assertFalse(finalizerRan)
   }
 
+  @Test
+  fun containsInstanceAndGetIdentifierForStrongReferenceUseIdentityComparison() {
+    val instanceManager: ProxyApiTestsPigeonInstanceManager = createInstanceManager()
+    instanceManager.stopFinalizationListener()
+
+    // Create two objects that are equal.
+    val testString = "aString"
+    val testObject1 = TestDataClass(testString)
+    val testObject2 = TestDataClass(testString)
+    assertEquals(testObject1, testObject2)
+
+    val identifier1 = instanceManager.addHostCreatedInstance(testObject1)
+    assertFalse(instanceManager.containsInstance(testObject2))
+    assertNull(instanceManager.getIdentifierForStrongReference(testObject2))
+
+    val identifier2 = instanceManager.addHostCreatedInstance(testObject2)
+    assertTrue(instanceManager.containsInstance(testObject1))
+    assertTrue(instanceManager.containsInstance(testObject2))
+    assertEquals(identifier1, instanceManager.getIdentifierForStrongReference(testObject1))
+    assertEquals(identifier2, instanceManager.getIdentifierForStrongReference(testObject2))
+  }
+
+  @Test
+  fun addingTwoDartCreatedInstancesThatAreEqual() {
+    val instanceManager: ProxyApiTestsPigeonInstanceManager = createInstanceManager()
+    instanceManager.stopFinalizationListener()
+
+    // Create two objects that are equal.
+    val testString = "aString"
+    val testObject1 = TestDataClass(testString)
+    val testObject2 = TestDataClass(testString)
+    assertEquals(testObject1, testObject2)
+
+    instanceManager.addDartCreatedInstance(testObject1, 0)
+    instanceManager.addDartCreatedInstance(testObject2, 1)
+
+    assertEquals(testObject1, instanceManager.getInstance(0))
+    assertEquals(testObject2, instanceManager.getInstance(1))
+    assertEquals(0L, instanceManager.getIdentifierForStrongReference(testObject1))
+    assertEquals(1L, instanceManager.getIdentifierForStrongReference(testObject2))
+  }
+
+  @Test
+  fun identityWeakReferencesAreEqualWithSameInstance() {
+    val testObject = Any()
+
+    assertEquals(
+        ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject),
+        ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject))
+  }
+
+  @Test
+  fun identityWeakReferenceRemainsEqualAfterGetReturnsNull() {
+    var testObject: Any? = Any()
+
+    val reference = ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject!!)
+
+    // To allow for object to be garbage collected.
+    @Suppress("UNUSED_VALUE")
+    testObject = null
+    Runtime.getRuntime().gc()
+
+    assertNull(reference.get())
+    assertEquals(reference, reference)
+  }
+
+  @Test
+  fun identityWeakReferencesAreNotEqualAfterGetReturnsNull() {
+    var testObject1: Any? = Any()
+    var testObject2: Any? = Any()
+
+    val reference1 = ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject1!!)
+    val reference2 = ProxyApiTestsPigeonInstanceManager.IdentityWeakReference(testObject2!!)
+
+    // To allow for object to be garbage collected.
+    @Suppress("UNUSED_VALUE")
+    testObject1 = null
+    testObject2 = null
+    Runtime.getRuntime().gc()
+
+    assertNull(reference1.get())
+    assertNull(reference2.get())
+    assertFalse(reference1 == reference2)
+  }
+
   private fun createInstanceManager(): ProxyApiTestsPigeonInstanceManager {
     return ProxyApiTestsPigeonInstanceManager.create(
         object : ProxyApiTestsPigeonInstanceManager.PigeonFinalizationListener {
           override fun onFinalize(identifier: Long) {}
         })
   }
+
+  data class TestDataClass(val value: String)
 }

packages/pigeon/pubspec.yaml

@@ -2,7 +2,7 @@ name: pigeon
 description: Code generator tool to make communication between Flutter and the host platform type-safe and easier.
 repository: https://github.com/flutter/packages/tree/main/packages/pigeon
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+pigeon%22
-version: 26.0.1 # This must match the version in lib/src/generator_tools.dart
+version: 26.0.2 # This must match the version in lib/src/generator_tools.dart
 
 environment:
   sdk: ^3.7.0