[pigeon] Prevents premature finalization from unused Dart instance for ProxyApis (#9231)

1. The Dart `InstanceManager.addHostCreatedInstance` is only ever used to respond to native calls that create a new instance right before the instance is returned in a host method call or passed in a flutter method call. So, `addHostCreatedInstance` now only adds a strong referenced instance and not the weak referenced instance. This will ensure the weak referenced instance is not created until it is being passed to a user immediately.
2. The Dart `InstanceManager.remove` now asserts that the strong referenced instance isn't removed before the weak referenced instance. This should prevent future scenarios where the user is holding a weak referenced instance while the native side no longer has any reference.

Fixes flutter/flutter#168531

## Pre-Review Checklist

[^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,3 +1,8 @@
+## 25.3.2
+
+* [dart] Fixes null pointer crashes/exceptions caused by premature finalization of Dart instances
+  for ProxyApis.
+
 ## 25.3.1
 
 * [kotlin] Fixes Kotlin InstanceManager not properly removing callbacks from handler.

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

@@ -110,8 +110,17 @@ class $dartInstanceManagerClassName {
   ///
   /// Returns the randomly generated id of the [instance] added.
   int addDartCreatedInstance($proxyApiBaseClassName instance) {
+    assert(getIdentifier(instance) == null);
+
     final int identifier = _nextUniqueIdentifier();
-    _addInstanceWithIdentifier(instance, identifier);
+    _identifiers[instance] = identifier;
+    _weakInstances[identifier] =
+        WeakReference<$proxyApiBaseClassName>(instance);
+    _finalizer.attach(instance, identifier, detach: instance);
+
+    final $proxyApiBaseClassName copy = instance.pigeon_copy();
+    _identifiers[copy] = identifier;
+    _strongInstances[identifier] = copy;
     return identifier;
   }
 
@@ -143,9 +152,15 @@ class $dartInstanceManagerClassName {
   /// it was removed. Returns `null` if [identifier] was not associated with
   /// any strong reference.
   ///
-  /// This does not remove the weak referenced instance associated with
-  /// [identifier]. This can be done with [removeWeakReference].
+  /// Throws an `AssertionError` if the weak referenced instance associated with
+  /// [identifier] is not removed first. This can be done with
+  /// [removeWeakReference].
   T? remove<T extends $proxyApiBaseClassName>(int identifier) {
+    final T? instance = _weakInstances[identifier]?.target as T?;
+    assert(
+      instance == null,
+      'A strong instance with identifier \$identifier is being removed despite the weak reference still existing: \$instance',
+    );
     return _strongInstances.remove(identifier) as T?;
   }
 
@@ -191,24 +206,13 @@ class $dartInstanceManagerClassName {
   ///
   /// Throws assertion error if the instance or its identifier has already been
   /// added.
-  ///
-  /// Returns unique identifier of the [instance] added.
   void addHostCreatedInstance($proxyApiBaseClassName instance, int identifier) {
-    _addInstanceWithIdentifier(instance, identifier);
-  }
-
-  void _addInstanceWithIdentifier($proxyApiBaseClassName instance, int identifier) {
     assert(!containsIdentifier(identifier));
     assert(getIdentifier(instance) == null);
     assert(identifier >= 0);
 
     _identifiers[instance] = identifier;
-    _weakInstances[identifier] = WeakReference<$proxyApiBaseClassName>(instance);
-    _finalizer.attach(instance, identifier, detach: instance);
-
-    final $proxyApiBaseClassName copy = instance.${classMemberNamePrefix}copy();
-    _identifiers[copy] = identifier;
-    _strongInstances[identifier] = copy;
+    _strongInstances[identifier] = instance;
   }
 
   /// Whether this manager contains the given [identifier].

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 = '25.3.1';
+const String pigeonVersion = '25.3.2';
 
 /// Read all the content from [stdin] to a String.
 String readStdin() {

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

@@ -37,12 +37,12 @@ protocol ${instanceManagerFinalizerDelegateName(options)}: AnyObject {
 String instanceManagerFinalizerTemplate(InternalSwiftOptions options) => '''
 // Attaches to an object to receive a callback when the object is deallocated.
 internal final class ${_instanceManagerFinalizerName(options)} {
-  private static let associatedObjectKey = malloc(1)!
+  internal static let associatedObjectKey = malloc(1)!
 
   private let identifier: Int64
   // Reference to the delegate is weak because the callback should be ignored if the
   // `InstanceManager` is deallocated.
-  private weak var delegate: ${instanceManagerFinalizerDelegateName(options)}?
+  internal weak var delegate: ${instanceManagerFinalizerDelegateName(options)}?
 
   private init(identifier: Int64, delegate: ${instanceManagerFinalizerDelegateName(options)}) {
     self.identifier = identifier
@@ -57,7 +57,11 @@ internal final class ${_instanceManagerFinalizerName(options)} {
   }
 
   static func detach(from instance: AnyObject) {
-    objc_setAssociatedObject(instance, associatedObjectKey, nil, .OBJC_ASSOCIATION_ASSIGN)
+    let finalizer = objc_getAssociatedObject(instance, associatedObjectKey) as? ${_instanceManagerFinalizerName(options)}
+    if let finalizer = finalizer {
+      finalizer.delegate = nil
+      objc_setAssociatedObject(instance, associatedObjectKey, nil, .OBJC_ASSOCIATION_ASSIGN)
+    }
   }
 
   deinit {
@@ -219,6 +223,10 @@ final class ${swiftInstanceManagerClassName(options)} {
   /// The manager will be empty after this call returns.
   func removeAllObjects() throws {
     lockQueue.sync {
+      let weakInstancesEnumerator = weakInstances.objectEnumerator()!
+      while let instance = weakInstancesEnumerator.nextObject() {
+        ${_instanceManagerFinalizerName(options)}.detach(from: instance as AnyObject)
+      }
       identifiers.removeAllObjects()
       weakInstances.removeAllObjects()
       strongInstances.removeAllObjects()

packages/pigeon/platform_tests/shared_test_plugin_code/lib/src/generated/proxy_api_tests.gen.dart

@@ -158,8 +158,17 @@ class PigeonInstanceManager {
   ///
   /// Returns the randomly generated id of the [instance] added.
   int addDartCreatedInstance(PigeonInternalProxyApiBaseClass instance) {
+    assert(getIdentifier(instance) == null);
+
     final int identifier = _nextUniqueIdentifier();
-    _addInstanceWithIdentifier(instance, identifier);
+    _identifiers[instance] = identifier;
+    _weakInstances[identifier] =
+        WeakReference<PigeonInternalProxyApiBaseClass>(instance);
+    _finalizer.attach(instance, identifier, detach: instance);
+
+    final PigeonInternalProxyApiBaseClass copy = instance.pigeon_copy();
+    _identifiers[copy] = identifier;
+    _strongInstances[identifier] = copy;
     return identifier;
   }
 
@@ -191,9 +200,15 @@ class PigeonInstanceManager {
   /// it was removed. Returns `null` if [identifier] was not associated with
   /// any strong reference.
   ///
-  /// This does not remove the weak referenced instance associated with
-  /// [identifier]. This can be done with [removeWeakReference].
+  /// Throws an `AssertionError` if the weak referenced instance associated with
+  /// [identifier] is not removed first. This can be done with
+  /// [removeWeakReference].
   T? remove<T extends PigeonInternalProxyApiBaseClass>(int identifier) {
+    final T? instance = _weakInstances[identifier]?.target as T?;
+    assert(
+      instance == null,
+      'A strong instance with identifier $identifier is being removed despite the weak reference still existing: $instance',
+    );
     return _strongInstances.remove(identifier) as T?;
   }
 
@@ -244,27 +259,14 @@ class PigeonInstanceManager {
   ///
   /// Throws assertion error if the instance or its identifier has already been
   /// added.
-  ///
-  /// Returns unique identifier of the [instance] added.
   void addHostCreatedInstance(
       PigeonInternalProxyApiBaseClass instance, int identifier) {
-    _addInstanceWithIdentifier(instance, identifier);
-  }
-
-  void _addInstanceWithIdentifier(
-      PigeonInternalProxyApiBaseClass instance, int identifier) {
     assert(!containsIdentifier(identifier));
     assert(getIdentifier(instance) == null);
     assert(identifier >= 0);
 
     _identifiers[instance] = identifier;
-    _weakInstances[identifier] =
-        WeakReference<PigeonInternalProxyApiBaseClass>(instance);
-    _finalizer.attach(instance, identifier, detach: instance);
-
-    final PigeonInternalProxyApiBaseClass copy = instance.pigeon_copy();
-    _identifiers[copy] = identifier;
-    _strongInstances[identifier] = copy;
+    _strongInstances[identifier] = instance;
   }
 
   /// Whether this manager contains the given [identifier].

packages/pigeon/platform_tests/shared_test_plugin_code/pubspec.yaml

@@ -19,3 +19,6 @@ dependencies:
   integration_test:
     sdk: flutter
   mockito: ^5.4.4
+
+dev_dependencies:
+  leak_tracker: any

packages/pigeon/platform_tests/shared_test_plugin_code/test/instance_manager_test.dart

@@ -5,6 +5,7 @@
 // This file specifically tests the test PigeonInstanceManager generated by core_tests.
 
 import 'package:flutter_test/flutter_test.dart';
+import 'package:leak_tracker/leak_tracker.dart';
 import 'package:shared_test_plugin_code/src/generated/proxy_api_tests.gen.dart';
 
 void main() {
@@ -22,7 +23,7 @@ void main() {
       expect(instanceManager.getIdentifier(object), 0);
       expect(
         instanceManager.getInstanceWithWeakReference(0),
-        object,
+        isA<CopyableObject>(),
       );
     });
 
@@ -106,7 +107,7 @@ void main() {
       expect(identical(object, copy), isFalse);
     });
 
-    test('removeStrongReference', () {
+    test('remove', () {
       final PigeonInstanceManager instanceManager =
           PigeonInstanceManager(onWeakReferenceRemoved: (_) {});
 
@@ -120,20 +121,16 @@ void main() {
       expect(instanceManager.containsIdentifier(0), isFalse);
     });
 
-    test('removeStrongReference removes only strong reference', () {
+    test('remove throws AssertionError if weak reference still exists', () {
       final PigeonInstanceManager instanceManager =
           PigeonInstanceManager(onWeakReferenceRemoved: (_) {});
 
       final CopyableObject object = CopyableObject(
         pigeon_instanceManager: instanceManager,
       );
 
-      instanceManager.addHostCreatedInstance(object, 0);
-      expect(instanceManager.remove(0), isA<CopyableObject>());
-      expect(
-        instanceManager.getInstanceWithWeakReference(0),
-        object,
-      );
+      instanceManager.addDartCreatedInstance(object);
+      expect(() => instanceManager.remove(0), throwsAssertionError);
     });
 
     test('getInstance can add a new weak reference', () {
@@ -153,6 +150,48 @@ void main() {
       )!;
       expect(identical(object, newWeakCopy), isFalse);
     });
+
+    test('addDartCreatedInstance should add finalizer to original object',
+        () async {
+      bool weakReferencedRemovedCalled = false;
+      final PigeonInstanceManager instanceManager = PigeonInstanceManager(
+        onWeakReferenceRemoved: (_) {
+          weakReferencedRemovedCalled = true;
+        },
+      );
+
+      CopyableObject? object = CopyableObject(
+        pigeon_instanceManager: instanceManager,
+      );
+
+      instanceManager.addDartCreatedInstance(object);
+
+      object = null;
+      await forceGC();
+
+      expect(weakReferencedRemovedCalled, isTrue);
+    });
+
+    test('addHostCreatedInstance should not add finalizer to original object',
+        () async {
+      bool weakReferencedRemovedCalled = false;
+      final PigeonInstanceManager instanceManager = PigeonInstanceManager(
+        onWeakReferenceRemoved: (_) {
+          weakReferencedRemovedCalled = true;
+        },
+      );
+
+      CopyableObject? object = CopyableObject(
+        pigeon_instanceManager: instanceManager,
+      );
+
+      instanceManager.addHostCreatedInstance(object, 0);
+
+      object = null;
+      await forceGC();
+
+      expect(weakReferencedRemovedCalled, isFalse);
+    });
   });
 }
 

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

@@ -120,6 +120,35 @@ class InstanceManagerTest {
     assertTrue(instanceManager.containsInstance(instance))
   }
 
+  @Test
+  fun clearPreventsFinalizationOfWeakInstances() {
+    var finalizerRan = false
+    val instanceManager: ProxyApiTestsPigeonInstanceManager =
+        ProxyApiTestsPigeonInstanceManager.create(
+            object : ProxyApiTestsPigeonInstanceManager.PigeonFinalizationListener {
+              override fun onFinalize(identifier: Long) {
+                finalizerRan = true
+              }
+            })
+
+    var testObject: Any? = Any()
+    instanceManager.addDartCreatedInstance(testObject!!, 0)
+    instanceManager.remove<Any?>(0)
+    instanceManager.clear()
+
+    // To allow for object to be garbage collected.
+    @Suppress("UNUSED_VALUE")
+    testObject = null
+    Runtime.getRuntime().gc()
+
+    // Changing this value triggers the callback.
+    instanceManager.clearFinalizedWeakReferencesInterval = 1000
+    instanceManager.stopFinalizationListener()
+
+    assertNull(instanceManager.getInstance<Any?>(0))
+    assertFalse(finalizerRan)
+  }
+
   private fun createInstanceManager(): ProxyApiTestsPigeonInstanceManager {
     return ProxyApiTestsPigeonInstanceManager.create(
         object : ProxyApiTestsPigeonInstanceManager.PigeonFinalizationListener {

packages/pigeon/platform_tests/test_plugin/darwin/test_plugin/Sources/test_plugin/ProxyApiTests.gen.swift

@@ -82,12 +82,12 @@ protocol ProxyApiTestsPigeonInternalFinalizerDelegate: AnyObject {
 
 // Attaches to an object to receive a callback when the object is deallocated.
 internal final class ProxyApiTestsPigeonInternalFinalizer {
-  private static let associatedObjectKey = malloc(1)!
+  internal static let associatedObjectKey = malloc(1)!
 
   private let identifier: Int64
   // Reference to the delegate is weak because the callback should be ignored if the
   // `InstanceManager` is deallocated.
-  private weak var delegate: ProxyApiTestsPigeonInternalFinalizerDelegate?
+  internal weak var delegate: ProxyApiTestsPigeonInternalFinalizerDelegate?
 
   private init(identifier: Int64, delegate: ProxyApiTestsPigeonInternalFinalizerDelegate) {
     self.identifier = identifier
@@ -103,7 +103,13 @@ internal final class ProxyApiTestsPigeonInternalFinalizer {
   }
 
   static func detach(from instance: AnyObject) {
-    objc_setAssociatedObject(instance, associatedObjectKey, nil, .OBJC_ASSOCIATION_ASSIGN)
+    let finalizer =
+      objc_getAssociatedObject(instance, associatedObjectKey)
+      as? ProxyApiTestsPigeonInternalFinalizer
+    if let finalizer = finalizer {
+      finalizer.delegate = nil
+      objc_setAssociatedObject(instance, associatedObjectKey, nil, .OBJC_ASSOCIATION_ASSIGN)
+    }
   }
 
   deinit {
@@ -261,6 +267,10 @@ final class ProxyApiTestsPigeonInstanceManager {
   /// The manager will be empty after this call returns.
   func removeAllObjects() throws {
     lockQueue.sync {
+      let weakInstancesEnumerator = weakInstances.objectEnumerator()!
+      while let instance = weakInstancesEnumerator.nextObject() {
+        ProxyApiTestsPigeonInternalFinalizer.detach(from: instance as AnyObject)
+      }
       identifiers.removeAllObjects()
       weakInstances.removeAllObjects()
       strongInstances.removeAllObjects()