Strengthen animation listener iteration patterns (#7537)

This patch aligns the iteration patterns used by animations and
ChangeNotifier. They now both respect re-entrant removal of listeners
and coalesce duplication registrations. (Also, ChangeNotifier
notification is no longer N^2).

Fixes #7533

packages/flutter/lib/src/animation/listener_helpers.dart

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:collection';
 import 'dart:ui' show VoidCallback;
 
 import 'package:flutter/foundation.dart';
@@ -34,9 +35,11 @@ abstract class AnimationLazyListenerMixin implements _ListenerMixin {
   }
 
   /// Called when the number of listeners changes from zero to one.
+  @protected
   void didStartListening();
 
   /// Called when the number of listeners changes from one to zero.
+  @protected
   void didStopListening();
 
   /// Whether there are any listeners.
@@ -61,7 +64,7 @@ abstract class AnimationEagerListenerMixin implements _ListenerMixin {
 /// A mixin that implements the addListener/removeListener protocol and notifies
 /// all the registered listeners when notifyListeners is called.
 abstract class AnimationLocalListenersMixin extends _ListenerMixin {
-  final List<VoidCallback> _listeners = <VoidCallback>[];
+  final Set<VoidCallback> _listeners = new LinkedHashSet<VoidCallback>();
 
   /// Calls the listener every time the value of the animation changes.
   ///
@@ -84,9 +87,24 @@ abstract class AnimationLocalListenersMixin extends _ListenerMixin {
   /// If listeners are added or removed during this function, the modifications
   /// will not change which listeners are called during this iteration.
   void notifyListeners() {
-    List<VoidCallback> localListeners = new List<VoidCallback>.from(_listeners);
-    for (VoidCallback listener in localListeners)
-      listener();
+    final List<VoidCallback> localListeners = new List<VoidCallback>.from(_listeners);
+    for (VoidCallback listener in localListeners) {
+      try {
+        if (_listeners.contains(listener))
+          listener();
+      } catch (exception, stack) {
+        FlutterError.reportError(new FlutterErrorDetails(
+          exception: exception,
+          stack: stack,
+          library: 'animation library',
+          context: 'while notifying listeners for $runtimeType',
+          informationCollector: (StringBuffer information) {
+            information.writeln('The $runtimeType notifying listeners was:');
+            information.write('  $this');
+          }
+        ));
+      }
+    }
   }
 }
 
@@ -94,7 +112,7 @@ abstract class AnimationLocalListenersMixin extends _ListenerMixin {
 /// and notifies all the registered listeners when notifyStatusListeners is
 /// called.
 abstract class AnimationLocalStatusListenersMixin extends _ListenerMixin {
-  final List<AnimationStatusListener> _statusListeners = <AnimationStatusListener>[];
+  final Set<AnimationStatusListener> _statusListeners = new LinkedHashSet<AnimationStatusListener>();
 
   /// Calls listener every time the status of the animation changes.
   ///
@@ -117,8 +135,23 @@ abstract class AnimationLocalStatusListenersMixin extends _ListenerMixin {
   /// If listeners are added or removed during this function, the modifications
   /// will not change which listeners are called during this iteration.
   void notifyStatusListeners(AnimationStatus status) {
-    List<AnimationStatusListener> localListeners = new List<AnimationStatusListener>.from(_statusListeners);
-    for (AnimationStatusListener listener in localListeners)
-      listener(status);
+    final List<AnimationStatusListener> localListeners = new List<AnimationStatusListener>.from(_statusListeners);
+    for (AnimationStatusListener listener in localListeners) {
+      try {
+        if (_statusListeners.contains(listener))
+          listener(status);
+      } catch (exception, stack) {
+        FlutterError.reportError(new FlutterErrorDetails(
+          exception: exception,
+          stack: stack,
+          library: 'animation library',
+          context: 'while notifying status listeners for $runtimeType',
+          informationCollector: (StringBuffer information) {
+            information.writeln('The $runtimeType notifying status listeners was:');
+            information.write('  $this');
+          }
+        ));
+      }
+    }
   }
 }

packages/flutter/lib/src/foundation/change_notifier.dart

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:collection';
+
 import 'package:meta/meta.dart';
 
 import 'assertions.dart';
@@ -37,11 +39,11 @@ abstract class Listenable {
 /// It is O(N) for adding and removing listeners and O(N²) for dispatching
 /// notifications (where N is the number of listeners).
 class ChangeNotifier extends Listenable {
-  List<VoidCallback> _listeners;
+  Set<VoidCallback> _listeners = new LinkedHashSet<VoidCallback>();
 
   bool _debugAssertNotDisposed() {
     assert(() {
-      if (_listeners == const <VoidCallback>[]) {
+      if (_listeners == null) {
         throw new FlutterError(
           'A $runtimeType was used after being disposed.\n'
           'Once you have called dispose() on a $runtimeType, it can no longer be used.'
@@ -58,7 +60,6 @@ class ChangeNotifier extends Listenable {
   @override
   void addListener(VoidCallback listener) {
     assert(_debugAssertNotDisposed);
-    _listeners ??= <VoidCallback>[];
     _listeners.add(listener);
   }
 
@@ -71,7 +72,7 @@ class ChangeNotifier extends Listenable {
   @override
   void removeListener(VoidCallback listener) {
     assert(_debugAssertNotDisposed);
-    _listeners?.remove(listener);
+    _listeners.remove(listener);
   }
 
   /// Discards any resources used by the object. After this is called, the
@@ -83,7 +84,7 @@ class ChangeNotifier extends Listenable {
   @mustCallSuper
   void dispose() {
     assert(_debugAssertNotDisposed);
-    _listeners = const <VoidCallback>[];
+    _listeners = null;
   }
 
   /// Call all the registered listeners.
@@ -101,7 +102,7 @@ class ChangeNotifier extends Listenable {
   void notifyListeners() {
     assert(_debugAssertNotDisposed);
     if (_listeners != null) {
-      List<VoidCallback> localListeners = new List<VoidCallback>.from(_listeners);
+      final List<VoidCallback> localListeners = new List<VoidCallback>.from(_listeners);
       for (VoidCallback listener in localListeners) {
         try {
           if (_listeners.contains(listener))

packages/flutter/lib/src/material/page.dart

@@ -167,14 +167,6 @@ class _CupertinoBackGestureController extends NavigationGestureController {
   }
 
   void handleStatusChanged(AnimationStatus status) {
-    // This can happen if an earlier status listener ends up calling dispose()
-    // on this object.
-    // TODO(abarth): Consider changing AnimationController not to call listeners
-    // that were removed while calling other listeners.
-    // See <https://github.com/flutter/flutter/issues/7533>.
-    if (controller == null)
-      return;
-
     if (status == AnimationStatus.dismissed) {
       navigator.pop();
       assert(controller == null);

packages/flutter/test/animation/iteration_patterns_test.dart

@@ -0,0 +1,123 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter_test/flutter_test.dart';
+import 'package:flutter/animation.dart';
+import 'package:flutter/widgets.dart';
+
+void main() {
+  setUp(() {
+    WidgetsFlutterBinding.ensureInitialized();
+    WidgetsBinding.instance.resetEpoch();
+  });
+
+  test('AnimationController with mutating listener', () {
+    final AnimationController controller = new AnimationController(
+      duration: const Duration(milliseconds: 100),
+      vsync: const TestVSync(),
+    );
+    final List<String> log = <String>[];
+
+    final VoidCallback listener1 = () { log.add('listener1'); };
+    final VoidCallback listener3 = () { log.add('listener3'); };
+    final VoidCallback listener4 = () { log.add('listener4'); };
+    final VoidCallback listener2 = () {
+      log.add('listener2');
+      controller.removeListener(listener1);
+      controller.removeListener(listener3);
+      controller.addListener(listener4);
+    };
+
+    controller.addListener(listener1);
+    controller.addListener(listener2);
+    controller.addListener(listener3);
+    controller.value = 0.2;
+    expect(log, <String>['listener1', 'listener2']);
+    log.clear();
+
+    controller.value = 0.3;
+    expect(log, <String>['listener2', 'listener4']);
+    log.clear();
+
+    controller.value = 0.4;
+    expect(log, <String>['listener2', 'listener4']);
+    log.clear();
+  });
+
+  test('AnimationController with mutating status listener', () {
+    final AnimationController controller = new AnimationController(
+      duration: const Duration(milliseconds: 100),
+      vsync: const TestVSync(),
+    );
+    final List<String> log = <String>[];
+
+    final AnimationStatusListener listener1 = (AnimationStatus status) { log.add('listener1'); };
+    final AnimationStatusListener listener3 = (AnimationStatus status) { log.add('listener3'); };
+    final AnimationStatusListener listener4 = (AnimationStatus status) { log.add('listener4'); };
+    final AnimationStatusListener listener2 = (AnimationStatus status) {
+      log.add('listener2');
+      controller.removeStatusListener(listener1);
+      controller.removeStatusListener(listener3);
+      controller.addStatusListener(listener4);
+    };
+
+    controller.addStatusListener(listener1);
+    controller.addStatusListener(listener2);
+    controller.addStatusListener(listener3);
+    controller.forward();
+    expect(log, <String>['listener1', 'listener2']);
+    log.clear();
+
+    controller.reverse();
+    expect(log, <String>['listener2', 'listener4']);
+    log.clear();
+
+    controller.forward();
+    expect(log, <String>['listener2', 'listener4']);
+    log.clear();
+
+    controller.dispose();
+  });
+
+  testWidgets('AnimationController with throwing listener', (WidgetTester tester) async {
+    final AnimationController controller = new AnimationController(
+      duration: const Duration(milliseconds: 100),
+      vsync: const TestVSync(),
+    );
+    final List<String> log = <String>[];
+
+    final VoidCallback listener1 = () { log.add('listener1'); };
+    final VoidCallback badListener = () { log.add('badListener'); throw null; };
+    final VoidCallback listener2 = () { log.add('listener2'); };
+
+    controller.addListener(listener1);
+    controller.addListener(badListener);
+    controller.addListener(listener2);
+    controller.value = 0.2;
+    expect(log, <String>['listener1', 'badListener', 'listener2']);
+    expect(tester.takeException(), isNullThrownError);
+    log.clear();
+  });
+
+  testWidgets('AnimationController with throwing status listener', (WidgetTester tester) async {
+    final AnimationController controller = new AnimationController(
+      duration: const Duration(milliseconds: 100),
+      vsync: const TestVSync(),
+    );
+    final List<String> log = <String>[];
+
+    final AnimationStatusListener listener1 = (AnimationStatus status) { log.add('listener1'); };
+    final AnimationStatusListener badListener = (AnimationStatus status) { log.add('badListener'); throw null; };
+    final AnimationStatusListener listener2 = (AnimationStatus status) { log.add('listener2'); };
+
+    controller.addStatusListener(listener1);
+    controller.addStatusListener(badListener);
+    controller.addStatusListener(listener2);
+    controller.forward();
+    expect(log, <String>['listener1', 'badListener', 'listener2']);
+    expect(tester.takeException(), isNullThrownError);
+    log.clear();
+    controller.dispose();
+  });
+}

packages/flutter/test/foundation/change_notifier_test.dart

@@ -24,12 +24,12 @@ void main() {
     test.addListener(listener);
     test.addListener(listener);
     test.notify();
-    expect(log, <String>['listener', 'listener']);
+    expect(log, <String>['listener']);
     log.clear();
 
     test.removeListener(listener);
     test.notify();
-    expect(log, <String>['listener']);
+    expect(log, <String>[]);
     log.clear();
 
     test.removeListener(listener);
@@ -79,7 +79,7 @@ void main() {
     test.removeListener(listener2);
     test.addListener(listener2);
     test.notify();
-    expect(log, <String>['badListener', 'listener1', 'listener2']);
+    expect(log, <String>['badListener', 'listener2']);
     expect(tester.takeException(), isNullThrownError);
     log.clear();
   });
@@ -110,7 +110,7 @@ void main() {
     log.clear();
 
     test.notify();
-    expect(log, <String>['listener2', 'listener4', 'listener4']);
+    expect(log, <String>['listener2', 'listener4']);
     log.clear();
   });