[pigeon] Prevents premature finalization from unused Dart instance for ProxyApis

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/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 is 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/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: 25.3.1 # This must match the version in lib/src/generator_tools.dart
+version: 25.3.2 # This must match the version in lib/src/generator_tools.dart
 
 environment:
   sdk: ^3.4.0

script/configs/allowed_unpinned_deps.yaml

@@ -41,6 +41,7 @@
 - io
 - js
 - json_serializable
+- leak_tracker
 - leak_tracker_flutter_testing
 - lints
 - logging