Strengthen animation listener iteration patterns (#7566)

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).

This patch introduces ObserverList to avoid the performance regression that the
previous version of this patch caused.

Fixes #7533

packages/flutter/lib/foundation.dart

@@ -14,6 +14,7 @@ export 'src/foundation/basic_types.dart';
 export 'src/foundation/binding.dart';
 export 'src/foundation/change_notifier.dart';
 export 'src/foundation/licenses.dart';
+export 'src/foundation/observer_list.dart';
 export 'src/foundation/platform.dart';
 export 'src/foundation/print.dart';
 export 'src/foundation/synchronous_future.dart';

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

@@ -34,9 +34,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 +63,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 ObserverList<VoidCallback> _listeners = new ObserverList<VoidCallback>();
 
   /// Calls the listener every time the value of the animation changes.
   ///
@@ -84,9 +86,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)
+    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 +111,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 ObserverList<AnimationStatusListener> _statusListeners = new ObserverList<AnimationStatusListener>();
 
   /// Calls listener every time the status of the animation changes.
   ///
@@ -117,8 +134,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)
+    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

@@ -6,6 +6,7 @@ import 'package:meta/meta.dart';
 
 import 'assertions.dart';
 import 'basic_types.dart';
+import 'observer_list.dart';
 
 /// An object that maintains a list of listeners.
 abstract class Listenable {
@@ -37,11 +38,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;
+  ObserverList<VoidCallback> _listeners = new ObserverList<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 +59,6 @@ class ChangeNotifier extends Listenable {
   @override
   void addListener(VoidCallback listener) {
     assert(_debugAssertNotDisposed);
-    _listeners ??= <VoidCallback>[];
     _listeners.add(listener);
   }
 
@@ -71,7 +71,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 +83,7 @@ class ChangeNotifier extends Listenable {
   @mustCallSuper
   void dispose() {
     assert(_debugAssertNotDisposed);
-    _listeners = const <VoidCallback>[];
+    _listeners = null;
   }
 
   /// Call all the registered listeners.
@@ -101,7 +101,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/foundation/observer_list.dart

+// Copyright 2017 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 'dart:collection';
+
+/// A list optimized for containment queries.
+///
+/// Consider using an [ObserverList] instead of a [List] when the number of
+/// [contains] calls dominates the number of [add] and [remove] calls.
+class ObserverList<T> extends Iterable<T> {
+  final List<T> _list = new List<T>();
+  bool _isDirty = false;
+  HashSet<T> _set;
+
+  /// Adds an item to the end of this list.
+  ///
+  /// The given item must not already be in the list.
+  void add(T item) {
+    _isDirty = true;
+    _list.add(item);
+  }
+
+  /// Removes an item from the list.
+  ///
+  /// Returns whether the item was present in the list.
+  bool remove(T item) {
+    _isDirty = true;
+    return _list.remove(item);
+  }
+
+  @override
+  bool contains(Object item) {
+    if (_list.length < 3)
+      return _list.contains(item);
+
+    if (_isDirty) {
+      if (_set == null) {
+        _set = new HashSet<T>.from(_list);
+      } else {
+        _set.clear();
+        _set.addAll(_list);
+      }
+      _isDirty = false;
+    }
+
+    return _set.contains(item);
+  }
+
+  @override
+  Iterator<T> get iterator => _list.iterator;
+}

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

@@ -178,16 +178,12 @@ class _MergeableMaterialState extends State<MergeableMaterial> with TickerProvid
       parent: controller,
       curve: Curves.fastOutSlowIn
     );
-
-    startAnimation.addListener(_handleTick);
-    endAnimation.addListener(_handleTick);
-
     final CurvedAnimation gapAnimation = new CurvedAnimation(
       parent: controller,
       curve: Curves.fastOutSlowIn
     );
 
-    gapAnimation.addListener(_handleTick);
+    controller.addListener(_handleTick);
 
     _animationTuples[gap.key] = new _AnimationTuple(
       controller: controller,

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

+// 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', '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', '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();
+  });
+}