Fix provider unmount logic not correctly cleaning up the full Provide…

…rContainer tree if an intermediate ProviderContainer does not use the provider (#2341)

Close #1943

packages/riverpod/CHANGELOG.md

@@ -1,3 +1,7 @@
+## Unreleased fix
+
+- Fixes an issue with nested ProviderScope (thanks to @jeiea)
+
 ## 2.3.3 - 2023-04-06
 
 - The debugger no-longer pauses on uncaught exceptions inside providers.

packages/riverpod/lib/src/framework/container.dart

@@ -179,11 +179,6 @@ class ProviderContainer implements Node {
   final _overrideForFamily = HashMap<Family<Object?>, _FamilyOverrideRef>();
   final Map<ProviderBase<Object?>, _StateReader> _stateReaders;
 
-  /// Awaits for providers to rebuild/be disposed and for listeners to be notified.
-  Future<void> pump() async {
-    return _scheduler.pendingFuture;
-  }
-
   final List<ProviderObserver> _observers;
 
   /// A debug utility used by `flutter_riverpod`/`hooks_riverpod` to check
@@ -200,6 +195,22 @@ class ProviderContainer implements Node {
   /// a [StateError] when attempting to use them.
   bool _disposed = false;
 
+  /// An internal utility for checking if a [ProviderContainer] has a fast
+  /// path for reading a provider.
+  ///
+  /// This should not be used and is an implementation detail of [ProviderContainer].
+  /// It could be removed at any time without a major version bump.
+  @internal
+  @visibleForTesting
+  bool hasStateReaderFor(ProviderListenable<Object?> provider) {
+    return _stateReaders.containsKey(provider);
+  }
+
+  /// Awaits for providers to rebuild/be disposed and for listeners to be notified.
+  Future<void> pump() async {
+    return _scheduler.pendingFuture;
+  }
+
   /// Reads a provider without listening to it and returns the currently
   /// exposed value.
   ///
@@ -316,10 +327,11 @@ class ProviderContainer implements Node {
       // on provider dispose, to avoid memory leak
 
       void removeStateReaderFrom(ProviderContainer container) {
-        if (container._stateReaders[element._origin] == reader) {
+        final reader = container._stateReaders[element._origin];
+        if (reader?.override == provider) {
           container._stateReaders.remove(element._origin);
-          container._children.forEach(removeStateReaderFrom);
         }
+        container._children.forEach(removeStateReaderFrom);
       }
 
       removeStateReaderFrom(this);

packages/riverpod/test/framework/provider_container_test.dart

@@ -16,6 +16,60 @@ void main() {
       child.dispose();
     });
 
+    group('when unmounting providders', () {
+      test(
+          'cleans up all the StateReaders of a provider in the entire ProviderContainer tree',
+          () async {
+        // Regression test for https://github.com/rrousselGit/riverpod/issues/1943
+        final a = createContainer();
+        // b/c voluntarily do not use the Provider, but a/d do. This is to test
+        // that the disposal logic correctly cleans up the StateReaders
+        // in all ProviderContainers associated with the provider, even if
+        // some links between two ProviderContainers are not using the provider.
+        final b = createContainer(parent: a);
+        final c = createContainer(parent: b);
+        final d = createContainer(parent: c);
+
+        final provider = Provider.autoDispose((ref) => 3);
+
+        final subscription = d.listen(
+          provider,
+          (previous, next) {},
+          fireImmediately: true,
+        );
+
+        expect(a.hasStateReaderFor(provider), true);
+        expect(b.hasStateReaderFor(provider), false);
+        expect(c.hasStateReaderFor(provider), false);
+        expect(d.hasStateReaderFor(provider), true);
+
+        subscription.close();
+
+        expect(a.hasStateReaderFor(provider), true);
+        expect(b.hasStateReaderFor(provider), false);
+        expect(c.hasStateReaderFor(provider), false);
+        expect(d.hasStateReaderFor(provider), true);
+
+        await a.pump();
+
+        expect(a.hasStateReaderFor(provider), false);
+        expect(b.hasStateReaderFor(provider), false);
+        expect(c.hasStateReaderFor(provider), false);
+        expect(d.hasStateReaderFor(provider), false);
+
+        d.listen(
+          provider,
+          (previous, next) {},
+          fireImmediately: true,
+        );
+
+        expect(a.hasStateReaderFor(provider), true);
+        expect(b.hasStateReaderFor(provider), false);
+        expect(c.hasStateReaderFor(provider), false);
+        expect(d.hasStateReaderFor(provider), true);
+      });
+    });
+
     group('exists', () {
       test('simple use-case', () {
         final container = createContainer();