Form needs to clean up its scopedWillPopCallback (#7936)

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

@@ -64,29 +64,36 @@ class Form extends StatefulWidget {
 class FormState extends State<Form> {
   int _generation = 0;
   Set<FormFieldState<dynamic>> _fields = new Set<FormFieldState<dynamic>>();
+  ModalRoute<dynamic> _route;
 
   @override
   void dependenciesChanged() {
     super.dependenciesChanged();
-    final ModalRoute<dynamic> route = ModalRoute.of(context);
-    if (route != null && config.onWillPop != null) {
-      // Avoid adding our callback twice by removing it first.
-      route.removeScopedWillPopCallback(config.onWillPop);
-      route.addScopedWillPopCallback(config.onWillPop);
-    }
+    if (_route != null && config.onWillPop != null)
+      _route.removeScopedWillPopCallback(config.onWillPop);
+    _route = ModalRoute.of(context);
+    if (_route != null && config.onWillPop != null)
+      _route.addScopedWillPopCallback(config.onWillPop);
   }
 
   @override
   void didUpdateConfig(Form oldConfig) {
-    final ModalRoute<dynamic> route = ModalRoute.of(context);
-    if (config.onWillPop != oldConfig.onWillPop && route != null) {
+    assert(_route == ModalRoute.of(context));
+    if (config.onWillPop != oldConfig.onWillPop && _route != null) {
       if (oldConfig.onWillPop != null)
-        route.removeScopedWillPopCallback(oldConfig.onWillPop);
+        _route.removeScopedWillPopCallback(oldConfig.onWillPop);
       if (config.onWillPop != null)
-        route.addScopedWillPopCallback(config.onWillPop);
+        _route.addScopedWillPopCallback(config.onWillPop);
     }
   }
 
+  @override
+  void dispose() {
+    if (_route != null && config.onWillPop != null)
+      _route.removeScopedWillPopCallback(config.onWillPop);
+    super.dispose();
+  }
+
   // Called when a form field has changed. This will cause all form fields
   // to rebuild, useful if form fields have interdependencies.
   void _fieldDidChange() {

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

@@ -665,10 +665,14 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T
   /// `dependenciesChanged` method:
   ///
   /// ```dart
+  /// ModalRoute<dynamic> _route;
+  ///
   /// @override
   /// void dependenciesChanged() {
   ///  super.dependenciesChanged();
-  ///  ModalRoute.of(context).addScopedWillPopCallback(askTheUserIfTheyAreSure);
+  ///  _route?.removeScopedWillPopCallback(askTheUserIfTheyAreSure);
+  ///  _route = ModalRoute.of(context);
+  ///  _route?.addScopedWillPopCallback(askTheUserIfTheyAreSure);
   /// }
   /// ```
   ///
@@ -679,6 +683,19 @@ abstract class ModalRoute<T> extends TransitionRoute<T> with LocalHistoryRoute<T
   /// after its route has been disposed. The callback should check [mounted] before
   /// doing anything.
   ///
+  /// A widget that adds a scopedWillPopCallback must ensure that the callback
+  /// is removed with [removeScopedWillPopCallback] by the time the widget has
+  /// been disposed. A stateful widget can do this in its dispose method
+  /// (continuing the previous example):
+  ///
+  /// ```dart
+  /// @override
+  /// void dispose() {
+  ///   _route?.removeScopedWillPopCallback(askTheUserIfTheyAreSure);
+  ///   super.dispose();
+  /// }
+  /// ```
+  ///
   /// See also:
   ///
   /// * [Form], which provides an `onWillPop` callback that uses this mechanism.

packages/flutter/test/material/will_pop_test.dart

@@ -53,6 +53,15 @@ class SampleForm extends StatelessWidget {
   }
 }
 
+// Expose the protected hasScopedWillPopCallback getter
+class TestPageRoute<T> extends MaterialPageRoute<T> {
+  TestPageRoute({ WidgetBuilder builder })
+    : super(builder: builder, maintainState: true, settings: const RouteSettings());
+
+  bool get hasCallback => super.hasScopedWillPopCallback;
+}
+
+
 void main() {
   testWidgets('ModalRoute scopedWillPopupCallback can inhibit back button', (WidgetTester tester) async {
     await tester.pumpWidget(
@@ -243,4 +252,63 @@ void main() {
     expect(find.text('Sample Form'), findsNothing);
   });
 
+  testWidgets('Route.scopedWillPop callbacks do not accumulate', (WidgetTester tester) async {
+    StateSetter contentsSetState; // call this to rebuild the route's SampleForm contents
+    bool contentsEmpty = false; // when true, don't include the SampleForm in the route
+
+    TestPageRoute<Null> route = new TestPageRoute<Null>(
+      builder: (BuildContext context) {
+        return new StatefulBuilder(
+          builder: (BuildContext context, StateSetter setState) {
+            contentsSetState = setState;
+            return contentsEmpty ? new Container() : new SampleForm(key: new UniqueKey());
+          }
+        );
+      },
+    );
+
+    Widget buildFrame() {
+      return new MaterialApp(
+        home: new Scaffold(
+          appBar: new AppBar(title: new Text('Home')),
+          body: new Builder(
+            builder: (BuildContext context) {
+              return new Center(
+                child: new FlatButton(
+                  child: new Text('X'),
+                  onPressed: () {
+                    Navigator.of(context).push(route);
+                  },
+                ),
+              );
+            },
+          ),
+        ),
+      );
+    }
+
+    await tester.pumpWidget(buildFrame());
+
+    await tester.tap(find.text('X'));
+    await tester.pump();
+    await tester.pump(const Duration(seconds: 1));
+
+    expect(find.text('Sample Form'), findsOneWidget);
+    expect(route.hasCallback, isTrue);
+
+    // Rebuild the route's SampleForm child an additional 3x for good measure.
+    contentsSetState(() { });
+    await tester.pump();
+    contentsSetState(() { });
+    await tester.pump();
+    contentsSetState(() { });
+    await tester.pump();
+
+    // Now build the route's contents without the sample form.
+    contentsEmpty = true;
+    contentsSetState(() { });
+    await tester.pump();
+
+    expect(route.hasCallback, isFalse);
+  });
 }