Improve Route lifecycle (#7526)

Previously the navigator wouldn't always call Route.dispose when it was
removed from the tree. After this patch, the navigator remembers popped
routes so that it can call dispose on them when it is removed from the
tree.

Also, improve some error messages around calling dispose() more than
once on routes and AnimationControllers.

Fixes #7457

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

@@ -210,7 +210,7 @@ class AnimationController extends Animation<double>
   /// controller's ticker might get muted, in which case the animation
   /// controller's callbacks will no longer fire even though time is continuing
   /// to pass. See [Ticker.muted] and [TickerMode].
-  bool get isAnimating => _ticker.isActive;
+  bool get isAnimating => _ticker != null && _ticker.isActive;
 
   _AnimationDirection _direction;
 
@@ -361,7 +361,17 @@ class AnimationController extends Animation<double>
   /// after this method is called.
   @override
   void dispose() {
+    assert(() {
+      if (_ticker == null) {
+        throw new FlutterError(
+          'AnimationController.dispose() called more than once.\n'
+          'A given AnimationController cannot be disposed more than once.'
+        );
+      }
+      return true;
+    });
     _ticker.dispose();
+    _ticker = null;
     super.dispose();
   }
 
@@ -392,10 +402,10 @@ class AnimationController extends Animation<double>
   @override
   String toStringDetails() {
     String paused = isAnimating ? '' : '; paused';
-    String silenced = _ticker.muted ? '; silenced' : '';
+    String ticker = _ticker == null ? '; DISPOSED' : (_ticker.muted ? '; silenced' : '');
     String label = debugLabel == null ? '' : '; for $debugLabel';
     String more = '${super.toStringDetails()} ${value.toStringAsFixed(3)}';
-    return '$more$paused$silenced$label';
+    return '$more$paused$ticker$label';
   }
 }
 

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

@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 import 'package:flutter/widgets.dart';
+import 'package:meta/meta.dart';
+
 import 'material.dart';
 import 'theme.dart';
 
@@ -109,29 +111,45 @@ class _CupertinoTransitionCurve extends Curve {
 // animation progress. Used for iOS back gesture.
 class _CupertinoBackGestureController extends NavigationGestureController {
   _CupertinoBackGestureController({
-    NavigatorState navigator,
-    this.controller,
-    this.onDisposed,
-  }) : super(navigator);
+    @required NavigatorState navigator,
+    @required this.controller,
+    @required this.onDisposed,
+  }) : super(navigator) {
+    assert(controller != null);
+    assert(onDisposed != null);
+  }
 
   AnimationController controller;
-  VoidCallback onDisposed;
+  final VoidCallback onDisposed;
 
   @override
   void dispose() {
-    super.dispose();
-    onDisposed();
     controller.removeStatusListener(handleStatusChanged);
     controller = null;
+    onDisposed();
+    super.dispose();
   }
 
   @override
   void dragUpdate(double delta) {
+    // This assert can be triggered the Scaffold is reparented out of the route
+    // associated with this gesture controller and continues to feed it events.
+    // TODO(abarth): Change the ownership of the gesture controller so that the
+    // object feeding it these events (e.g., the Scaffold) is responsible for
+    // calling dispose on it as well.
+    assert(controller != null);
     controller.value -= delta;
   }
 
   @override
   bool dragEnd(double velocity) {
+    // This assert can be triggered the Scaffold is reparented out of the route
+    // associated with this gesture controller and continues to feed it events.
+    // TODO(abarth): Change the ownership of the gesture controller so that the
+    // object feeding it these events (e.g., the Scaffold) is responsible for
+    // calling dispose on it as well.
+    assert(controller != null);
+
     if (velocity.abs() >= _kMinFlingVelocity) {
       controller.fling(velocity: -velocity);
     } else if (controller.value <= 0.5) {
@@ -142,17 +160,28 @@ class _CupertinoBackGestureController extends NavigationGestureController {
 
     // Don't end the gesture until the transition completes.
     final AnimationStatus status = controller.status;
-    handleStatusChanged(controller.status);
+    handleStatusChanged(status);
     controller?.addStatusListener(handleStatusChanged);
 
     return (status == AnimationStatus.reverse || status == AnimationStatus.dismissed);
   }
 
   void handleStatusChanged(AnimationStatus status) {
-    if (status == AnimationStatus.dismissed)
+    // 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();
-    if (status == AnimationStatus.dismissed || status == AnimationStatus.completed)
+      assert(controller == null);
+    } else if (status == AnimationStatus.completed) {
       dispose();
+      assert(controller == null);
+    }
   }
 }
 
@@ -210,7 +239,7 @@ class MaterialPageRoute<T> extends PageRoute<T> {
   ///  * [hasScopedWillPopCallback], which is true if a `willPop` callback
   ///    is defined for this route.
   @override
-  NavigationGestureController startPopGesture(NavigatorState navigator) {
+  NavigationGestureController startPopGesture() {
     // If attempts to dismiss this route might be vetoed, then do not
     // allow the user to dismiss the route with a swipe.
     if (hasScopedWillPopCallback)

packages/flutter/lib/src/widgets/navigator.dart

@@ -80,8 +80,12 @@ abstract class Route<T> {
   /// return false, otherwise return true. Returning false will prevent the
   /// default behavior of NavigatorState.pop().
   ///
-  /// If this is called, the Navigator will not call dispose(). It is the
-  /// responsibility of the Route to later call dispose().
+  /// When this function returns true, the navigator removes this route from
+  /// the history but does not yet call [dispose]. Instead, it is the route's
+  /// responsibility to call [NavigatorState.finalizeRoute], which will in turn
+  /// call [dispose] on the route. This sequence lets the route perform an
+  /// exit animation (or some other visual effect) after being popped but prior
+  /// to being disposed.
   @protected
   @mustCallSuper
   bool didPop(T result) {
@@ -108,11 +112,21 @@ abstract class Route<T> {
 
   /// The route should remove its overlays and free any other resources.
   ///
-  /// A call to didPop() implies that the Route should call dispose() itself,
-  /// but it is possible for dispose() to be called directly (e.g. if the route
-  /// is replaced, or if the navigator itself is disposed).
+  /// This route is no longer referenced by the navigator.
   @mustCallSuper
-  void dispose() { }
+  @protected
+  void dispose() {
+    assert(() {
+      if (_navigator == null) {
+        throw new FlutterError(
+          '$runtimeType.dipose() called more than once.\n'
+          'A given route cannot be disposed more than once.'
+        );
+      }
+      return true;
+    });
+    _navigator = null;
+  }
 
   /// If the route's transition can be popped via a user gesture (e.g. the iOS
   /// back gesture), this should return a controller object that can be used to
@@ -123,18 +137,13 @@ abstract class Route<T> {
   /// a [WillPopCallback] was defined for the route, then it may make sense
   /// to disable the pop gesture. For example, the iOS back gesture is disabled
   /// when [ModalRoute.hasScopedWillCallback] is true.
-  NavigationGestureController startPopGesture(NavigatorState navigator) {
-    return null;
-  }
+  NavigationGestureController startPopGesture() => null;
 
   /// Whether this route is the top-most route on the navigator.
   ///
   /// If this is true, then [isActive] is also true.
   bool get isCurrent {
-    if (_navigator == null)
-      return false;
-    assert(_navigator._history.contains(this));
-    return _navigator._history.last == this;
+    return _navigator != null && _navigator._history.last == this;
   }
 
   /// Whether this route is on the navigator.
@@ -146,10 +155,7 @@ abstract class Route<T> {
   /// rendered. It is even possible for the route to be active but for the stateful
   /// widgets within the route to not be instatiated. See [ModalRoute.maintainState].
   bool get isActive {
-    if (_navigator == null)
-      return false;
-    assert(_navigator._history.contains(this));
-    return true;
+    return _navigator != null && _navigator._history.contains(this);
   }
 }
 
@@ -208,6 +214,7 @@ abstract class NavigationGestureController {
   /// Configures the NavigationGestureController and tells the given [Navigator] that
   /// a gesture has started.
   NavigationGestureController(this._navigator) {
+    assert(_navigator != null);
     // Disable Hero transitions until the gesture is complete.
     _navigator.didStartUserGesture();
   }
@@ -223,6 +230,7 @@ abstract class NavigationGestureController {
   /// Must be called when the gesture is done.
   ///
   /// Calling this method notifies the navigator that the gesture has completed.
+  @mustCallSuper
   void dispose() {
     _navigator.didStopUserGesture();
     _navigator = null;
@@ -596,6 +604,7 @@ class Navigator extends StatefulWidget {
 class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
   final GlobalKey<OverlayState> _overlayKey = new GlobalKey<OverlayState>();
   final List<Route<dynamic>> _history = new List<Route<dynamic>>();
+  final Set<Route<dynamic>> _poppedRoutes = new Set<Route<dynamic>>();
 
   @override
   void initState() {
@@ -622,10 +631,11 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
     assert(!_debugLocked);
     assert(() { _debugLocked = true; return true; });
     config.observer?._navigator = null;
-    for (Route<dynamic> route in _history) {
+    final List<Route<dynamic>> doomed = _poppedRoutes.toList()..addAll(_history);
+    for (Route<dynamic> route in doomed)
       route.dispose();
-      route._navigator = null;
-    }
+    _poppedRoutes.clear();
+    _history.clear();
     super.dispose();
     assert(() { _debugLocked = false; return true; });
   }
@@ -726,7 +736,6 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
       if (index > 0)
         _history[index - 1].didChangeNext(newRoute);
       oldRoute.dispose();
-      oldRoute._navigator = null;
     });
     assert(() { _debugLocked = false; return true; });
   }
@@ -766,7 +775,6 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
       if (index > 0)
         _history[index - 1].didChangeNext(newRoute);
       targetRoute.dispose();
-      targetRoute._navigator = null;
     });
     assert(() { _debugLocked = false; return true; });
   }
@@ -815,9 +823,13 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
           // can't do that for themselves, even if they have changed their own
           // state (e.g. ModalScope.isCurrent).
           _history.removeLast();
+          // If route._navigator is null, the route called finalizeRoute from
+          // didPop, which means the route has already been disposed and doesn't
+          // need to be added to _poppedRoutes for later disposal.
+          if (route._navigator != null)
+            _poppedRoutes.add(route);
           _history.last.didPopNext(route);
           config.observer?.didPop(route, _history.last);
-          route._navigator = null;
         });
       } else {
         assert(() { _debugLocked = false; return true; });
@@ -831,6 +843,22 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
     return true;
   }
 
+  /// Complete the lifecycle for a route that has been popped off the navigator.
+  ///
+  /// When the navigator pops a route, the navigator retains a reference to the
+  /// route in order to call [Route.dispose] if the navigator itself is removed
+  /// from the tree. When the route is finished with any exit animation, the
+  /// route should call this function to complete its lifecycle (e.g., to
+  /// receive a call to [Route.dispose]).
+  ///
+  /// The given `route` must have already received a call to [Route.didPop].
+  /// This function may be called directly from [Route.didPop] if [Route.didPop]
+  /// will return `true`.
+  void finalizeRoute(Route<dynamic> route) {
+    _poppedRoutes.remove(route);
+    route.dispose();
+  }
+
   /// Repeatedly calls [pop] until the given `predicate` returns true.
   ///
   /// The predicate may be applied to the same route more than once if
@@ -855,7 +883,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
   /// Starts a gesture that results in popping the navigator.
   NavigationGestureController startPopGesture() {
     if (canPop())
-      return _history.last.startPopGesture(this);
+      return _history.last.startPopGesture();
     return null;
   }
 

packages/flutter/lib/src/widgets/routes.dart

@@ -35,39 +35,32 @@ abstract class OverlayRoute<T> extends Route<T> {
     super.install(insertionPoint);
   }
 
-  /// Controls whether [didPop] calls [finished].
+  /// Controls whether [didPop] calls [NavigatorState.finalizeRoute].
   ///
   /// If true, this route removes its overlay entries during [didPop].
-  /// Subclasses can override this getter if they want to delay the [finished]
-  /// call (for example to animate the route's exit before removing it from the
+  /// Subclasses can override this getter if they want to delay finalization
+  /// (for example to animate the route's exit before removing it from the
   /// overlay).
+  ///
+  /// Subclasses that return false from [finishedWhenPopped] are responsible for
+  /// calling [NavigatorState.finalizeRoute] themselves.
   @protected
   bool get finishedWhenPopped => true;
 
   @override
   bool didPop(T result) {
+    final bool returnValue = super.didPop(result);
+    assert(returnValue);
     if (finishedWhenPopped)
-      finished();
-    return super.didPop(result);
+      navigator.finalizeRoute(this);
+    return returnValue;
   }
 
-  /// Clears out the overlay entries.
-  ///
-  /// This method is intended to be used by subclasses who don't call
-  /// super.didPop() because they want to have control over the timing of the
-  /// overlay removal.
-  ///
-  /// Do not call this method outside of this context.
-  @protected
-  void finished() {
+  @override
+  void dispose() {
     for (OverlayEntry entry in _overlayEntries)
       entry.remove();
     _overlayEntries.clear();
-  }
-
-  @override
-  void dispose() {
-    finished();
     super.dispose();
   }
 }
@@ -93,7 +86,7 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
   bool get opaque;
 
   @override
-  bool get finishedWhenPopped => false;
+  bool get finishedWhenPopped => _controller.status == AnimationStatus.dismissed;
 
   /// The animation that drives the route's transition and the previous route's
   /// forward transition.
@@ -143,8 +136,14 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
         break;
       case AnimationStatus.dismissed:
         assert(!overlayEntries.first.opaque);
-        finished(); // clear the overlays
-        assert(overlayEntries.isEmpty);
+        // We might still be the current route if a subclass is controlling the
+        // the transition and hits the dismissed status. For example, the iOS
+        // back gesture drives this animation to the dismissed status before
+        // popping the navigator.
+        if (!isCurrent) {
+          navigator.finalizeRoute(this);
+          assert(overlayEntries.isEmpty);
+        }
         break;
     }
   }
@@ -239,15 +238,10 @@ abstract class TransitionRoute<T> extends OverlayRoute<T> {
   /// need to coordinate transitions with.
   bool canTransitionFrom(TransitionRoute<dynamic> nextRoute) => true;
 
-  @override
-  void finished() {
-    super.finished();
-    _transitionCompleter.complete(_result);
-  }
-
   @override
   void dispose() {
     _controller.dispose();
+    _transitionCompleter.complete(_result);
     super.dispose();
   }
 

packages/flutter/test/animation/animation_controller_test.dart

@@ -260,4 +260,13 @@ void main() {
 
     controller.stop();
   });
+
+  test('Disposed AnimationController toString works', () {
+    AnimationController controller = new AnimationController(
+      duration: const Duration(milliseconds: 100),
+      vsync: const TestVSync(),
+    );
+    controller.dispose();
+    expect(controller, hasOneLineDescription);
+  });
 }

packages/flutter/test/material/date_picker_test.dart

@@ -142,9 +142,6 @@ void main() {
 
     await tester.pumpUntilNoTransientCallbacks(const Duration(seconds: 1));
     await callback(date);
-
-    // TODO(abarth): Remove this call once https://github.com/flutter/flutter/issues/7457 is fixed.
-    await tester.pumpUntilNoTransientCallbacks(const Duration(seconds: 1));
   }
 
   testWidgets('Initial date is the default', (WidgetTester tester) async {

packages/flutter/test/material/dialog_test.dart

@@ -136,9 +136,5 @@ void main() {
     await tester.tap(find.text('First option'));
 
     expect(await result, equals(42));
-
-    // TODO(abarth): Remove once https://github.com/flutter/flutter/issues/7457
-    // is fixed.
-    await tester.pumpUntilNoTransientCallbacks(const Duration(seconds: 1));
   });
 }

packages/flutter/test/widgets/routes_test.dart

@@ -55,7 +55,7 @@ class TestRoute extends LocalHistoryRoute<String> {
     log('didPop $result');
     bool returnValue;
     if (returnValue = super.didPop(result))
-      dispose();
+      navigator.finalizeRoute(this);
     return returnValue;
   }