markNeedsBuild should handle case where the element is in the dirty list (#7448)

Previously we asserted that the element was not in the dirty list, but there
are scenarios where the element can be in the dirty list already. This patch
makes markNeedsBuild handle those cases by simply resorting the dirty list.

Fixes #6119

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

@@ -43,7 +43,7 @@ class WidgetsApp extends StatefulWidget {
     @required this.onGenerateRoute,
     this.title,
     this.textStyle,
-    this.color,
+    @required this.color,
     this.navigatorObserver,
     this.initialRoute,
     this.onLocaleChanged,
@@ -52,6 +52,7 @@ class WidgetsApp extends StatefulWidget {
     this.showSemanticsDebugger: false,
     this.debugShowCheckedModeBanner: true
   }) : super(key: key) {
+    assert(color != null);
     assert(onGenerateRoute != null);
     assert(showPerformanceOverlay != null);
     assert(checkerboardRasterCacheImages != null);

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

@@ -1601,26 +1601,6 @@ class BuildOwner {
   bool _scheduledFlushDirtyElements = false;
   bool _dirtyElementsNeedsResorting; // null means we're not in a buildScope
 
-  /// Flags the dirty elements list as needing resorting.
-  ///
-  /// This should only be called while a [buildScope] is actively rebuilding the
-  /// widget tree.
-  void markNeedsToResortDirtyElements() {
-    assert(() {
-      if (debugPrintScheduleBuildForStacks)
-        debugPrintStack(label: 'markNeedsToResortDirtyElements() called; _dirtyElementsNeedsResorting was $_dirtyElementsNeedsResorting (now true); dirty list is: $_dirtyElements');
-      if (_dirtyElementsNeedsResorting == null) {
-        throw new FlutterError(
-          'markNeedsToResortDirtyElements() called inappropriately.\n'
-          'The markNeedsToResortDirtyElements() method should only be called while the '
-          'buildScope() method is actively rebuilding the widget tree.'
-        );
-      }
-      return true;
-    });
-    _dirtyElementsNeedsResorting = true;
-  }
-
   /// Adds an element to the dirty elements list so that it will be rebuilt
   /// when [WidgetsBinding.beginFrame] calls [buildScope].
   void scheduleBuildFor(BuildableElement element) {
@@ -1630,22 +1610,6 @@ class BuildOwner {
     assert(() {
       if (debugPrintScheduleBuildForStacks)
         debugPrintStack(label: 'scheduleBuildFor() called for $element${_dirtyElements.contains(element) ? " (ALREADY IN LIST)" : ""}');
-      if (element._inDirtyList) {
-        throw new FlutterError(
-          'scheduleBuildFor() called for a widget for which a build was already scheduled.\n'
-          'The method was called for the following element:\n'
-          '  $element\n'
-          'The current dirty list consists of:\n'
-          '  $_dirtyElements\n'
-          'This usually indicates that a widget was rebuilt outside the build phase (thus '
-          'marking the element as clean even though it is still in the dirty list). '
-          'This should not be possible and is probably caused by a bug in the widgets framework. '
-          'Please report it: https://github.com/flutter/flutter/issues/new\n'
-          'To debug this issue, consider setting the debugPrintScheduleBuildForStacks and '
-          'debugPrintBuildDirtyElements flags to true and looking for a call to scheduleBuildFor '
-          'for a widget that is labeled "ALREADY IN LIST".'
-        );
-      }
       if (!element.dirty) {
         throw new FlutterError(
           'scheduleBuildFor() called for a widget that is not marked as dirty.\n'
@@ -1660,6 +1624,22 @@ class BuildOwner {
       }
       return true;
     });
+    if (element._inDirtyList) {
+      assert(() {
+        if (debugPrintScheduleBuildForStacks)
+          debugPrintStack(label: 'markNeedsToResortDirtyElements() called; _dirtyElementsNeedsResorting was $_dirtyElementsNeedsResorting (now true); dirty list is: $_dirtyElements');
+        if (_dirtyElementsNeedsResorting == null) {
+          throw new FlutterError(
+              'markNeedsToResortDirtyElements() called inappropriately.\n'
+                  'The markNeedsToResortDirtyElements() method should only be called while the '
+                  'buildScope() method is actively rebuilding the widget tree.'
+          );
+        }
+        return true;
+      });
+      _dirtyElementsNeedsResorting = true;
+      return;
+    }
     if (!_scheduledFlushDirtyElements && onBuildScheduled != null) {
       _scheduledFlushDirtyElements = true;
       onBuildScheduled();
@@ -2807,13 +2787,8 @@ abstract class BuildableElement extends Element {
   void activate() {
     final bool hadDependencies = ((_dependencies != null && _dependencies.isNotEmpty) || _hadUnsatisfiedDependencies);
     super.activate(); // clears _dependencies, and sets active to true
-    if (_dirty) {
-      if (_inDirtyList) {
-        owner.markNeedsToResortDirtyElements();
-      } else {
-        owner.scheduleBuildFor(this);
-      }
-    }
+    if (_dirty)
+      owner.scheduleBuildFor(this);
     if (hadDependencies)
       dependenciesChanged();
   }

packages/flutter/test/widgets/reparent_state_with_layout_builder_test.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:ui' as ui show window;
+
 import 'package:flutter/material.dart';
 import 'package:flutter_test/flutter_test.dart' hide TypeMatcher;
 
@@ -28,12 +30,16 @@ class BarState extends State<Bar> {
     if (_mode) {
       return new SizedBox(
         child: new LayoutBuilder(
-          builder: (BuildContext context, BoxConstraints constraints) => new StatefulCreationCounter(key: _fooKey),
+          builder: (BuildContext context, BoxConstraints constraints) {
+            return new StatefulCreationCounter(key: _fooKey);
+          },
         ),
       );
     } else {
       return new LayoutBuilder(
-        builder: (BuildContext context, BoxConstraints constraints) => new StatefulCreationCounter(key: _fooKey),
+        builder: (BuildContext context, BoxConstraints constraints) {
+          return new StatefulCreationCounter(key: _fooKey);
+        },
       );
     }
   }
@@ -60,7 +66,8 @@ class StatefulCreationCounterState extends State<StatefulCreationCounter> {
 }
 
 void main() {
-  testWidgets('reparent state with layout builder', (WidgetTester tester) async {
+  testWidgets('reparent state with layout builder',
+      (WidgetTester tester) async {
     expect(StatefulCreationCounterState.creationCount, 0);
     await tester.pumpWidget(new Bar());
     expect(StatefulCreationCounterState.creationCount, 1);
@@ -69,4 +76,83 @@ void main() {
     await tester.pump();
     expect(StatefulCreationCounterState.creationCount, 1);
   });
+
+  testWidgets('Clean then reparent with dependencies',
+      (WidgetTester tester) async {
+    GlobalKey key = new GlobalKey();
+
+    StateSetter keyedSetState;
+
+    Widget keyedWidget = new StatefulBuilder(
+      key: key,
+      builder: (BuildContext context, StateSetter setState) {
+        keyedSetState = setState;
+        MediaQuery.of(context);
+        return new Container();
+      },
+    );
+
+    Widget layoutBuilderChild = keyedWidget;
+    StateSetter layoutBuilderSetState;
+
+    StateSetter childSetState;
+    Widget deepChild = new Container();
+
+    int layoutBuilderBuildCount = 0;
+
+    await tester.pumpWidget(new MediaQuery(
+      data: new MediaQueryData.fromWindow(ui.window),
+      child: new Column(
+        children: <Widget>[
+          new StatefulBuilder(
+              builder: (BuildContext context, StateSetter setState) {
+            layoutBuilderSetState = setState;
+            return new LayoutBuilder(
+              builder: (BuildContext context, BoxConstraints constraints) {
+                ++layoutBuilderBuildCount;
+                return layoutBuilderChild;
+              },
+            );
+          }),
+          new Container(
+            child: new Container(
+              child: new Container(
+                child: new Container(
+                  child: new Container(
+                    child: new Container(
+                      child: new StatefulBuilder(builder:
+                          (BuildContext context, StateSetter setState) {
+                        childSetState = setState;
+                        return deepChild;
+                      }),
+                    ),
+                  ),
+                ),
+              ),
+            ),
+          ),
+        ],
+      ),
+    ));
+
+    expect(layoutBuilderBuildCount, 1);
+
+    // This call adds the element ot the dirty list.
+    keyedSetState(() {});
+
+    childSetState(() {
+      deepChild = keyedWidget;
+    });
+
+    // The layout builder will build in a separate build scope. This delays the
+    // removal of the keyed child until this build scope.
+    layoutBuilderSetState(() {
+      layoutBuilderChild = new Container();
+    });
+
+    // The essential part of this test is that this call to pump doesn't throw.
+    await tester.pump();
+
+    expect(layoutBuilderBuildCount, 2);
+  });
 }