don't clear the dirty bit of an inactive node (#5614)

Fixes https://github.com/flutter/flutter/issues/5588

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

@@ -1283,11 +1283,6 @@ class BuildOwner {
         int dirtyCount = _dirtyElements.length;
         int index = 0;
         while (index < dirtyCount) {
-          assert(() {
-            if (debugPrintRebuildDirtyWidgets)
-              debugPrint('Rebuilding ${_dirtyElements[index].widget}');
-            return true;
-          });
           _dirtyElements[index].rebuild();
           index += 1;
           if (dirtyCount < _dirtyElements.length) {
@@ -1295,7 +1290,7 @@ class BuildOwner {
             dirtyCount = _dirtyElements.length;
           }
         }
-        assert(!_dirtyElements.any((BuildableElement element) => element.dirty));
+        assert(!_dirtyElements.any((BuildableElement element) => element._active && element.dirty));
       }, building: true);
     } finally {
       _dirtyElements.clear();
@@ -1913,10 +1908,13 @@ abstract class BuildableElement extends Element {
   /// changed.
   void rebuild() {
     assert(_debugLifecycleState != _ElementLifecycle.initial);
-    if (!_active || !_dirty) {
-      _dirty = false;
+    if (!_active || !_dirty)
       return;
-    }
+    assert(() {
+      if (debugPrintRebuildDirtyWidgets)
+        debugPrint('Rebuilding $this');
+      return true;
+    });
     assert(_debugLifecycleState == _ElementLifecycle.active);
     assert(owner._debugStateLocked);
     BuildableElement debugPreviousBuildTarget;
@@ -2179,7 +2177,6 @@ class StatefulElement extends ComponentElement {
         'that all the resources used by the widget are fully released.'
       );
     });
-    assert(!dirty); // See BuildableElement.unmount for why this is important.
     _state._element = null;
     _state = null;
   }

packages/flutter/test/widget/reparent_state_harder_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/src/widgets/basic.dart';
+import 'package:flutter/src/widgets/framework.dart';
+import 'package:flutter/src/widgets/layout_builder.dart';
+import 'package:flutter_test/flutter_test.dart' hide TypeMatcher;
+
+// This is a regression test for https://github.com/flutter/flutter/issues/5588.
+
+class OrderSwitcher extends StatefulWidget {
+  OrderSwitcher({ Key key, this.a, this.b }) : super(key: key);
+
+  final Widget a;
+  final Widget b;
+
+  @override
+  OrderSwitcherState createState() => new OrderSwitcherState();
+}
+
+class OrderSwitcherState extends State<OrderSwitcher> {
+
+  bool _aFirst = true;
+
+  void switchChildren() {
+    setState(() {
+      _aFirst = false;
+    });
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    List<Widget> children = <Widget>[];
+    if (_aFirst) {
+      children.add(new KeyedSubtree(child: config.a));
+      children.add(config.b);
+    } else {
+      children.add(new KeyedSubtree(child: config.b));
+      children.add(config.a);
+    }
+    return new Stack(
+      children: children
+    );
+  }
+}
+
+class DummyStatefulWidget extends StatefulWidget {
+  DummyStatefulWidget(Key key) : super(key: key);
+
+  @override
+  DummyStatefulWidgetState createState() => new DummyStatefulWidgetState();
+}
+
+class DummyStatefulWidgetState extends State<DummyStatefulWidget> {
+  @override
+  Widget build(BuildContext context) => new Text('LEAF');
+}
+
+class RekeyableDummyStatefulWidgetWrapper extends StatefulWidget {
+  RekeyableDummyStatefulWidgetWrapper({ this.child, this.initialKey });
+  final Widget child;
+  final GlobalKey initialKey;
+  @override
+  RekeyableDummyStatefulWidgetWrapperState createState() => new RekeyableDummyStatefulWidgetWrapperState();
+}
+
+class RekeyableDummyStatefulWidgetWrapperState extends State<RekeyableDummyStatefulWidgetWrapper> {
+  GlobalKey _key;
+
+  @override
+  void initState() {
+    super.initState();
+    _key = config.initialKey;
+  }
+
+  void _setChild(GlobalKey value) {
+    setState(() {
+      _key = value;
+    });
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    return new DummyStatefulWidget(_key);
+  }
+}
+
+void main() {
+  testWidgets('Handle GlobalKey reparenting in weird orders', (WidgetTester tester) async {
+
+    // This is a bit of a weird test so let's try to explain it a bit.
+    //
+    // Basically what's happening here is that we have a complicated tree, and
+    // in one frame, we change it to a slightly different tree with a specific
+    // set of mutations:
+    //
+    // * The keyA subtree is regrafted to be one level higher, but later than
+    //   the keyB subtree.
+    // * The keyB subtree is, similarly, moved one level deeper, but earlier, than
+    //   the keyA subtree.
+    // * The keyD subtree is replaced by the previously earlier and shallower
+    //   keyC subtree. This happens during a LayoutBuilder layout callback, so it
+    //   happens long after A and B have finished their dance.
+    //
+    // The net result is that when keyC is moved, it has already been marked
+    // dirty from being removed then reinserted into the tree (redundantly, as
+    // it turns out, though this isn't known at the time), and has already been
+    // visited once by the code that tries to clean nodes (though at that point
+    // nothing happens since it isn't in the tree).
+    //
+    // This test verifies that none of the asserts go off during this dance.
+
+    final GlobalKey<OrderSwitcherState> keyRoot = new GlobalKey(debugLabel: 'Root');
+    final GlobalKey keyA = new GlobalKey(debugLabel: 'A');
+    final GlobalKey keyB = new GlobalKey(debugLabel: 'B');
+    final GlobalKey keyC = new GlobalKey(debugLabel: 'C');
+    final GlobalKey keyD = new GlobalKey(debugLabel: 'D');
+    await tester.pumpWidget(new OrderSwitcher(
+      key: keyRoot,
+      a: new KeyedSubtree(
+        key: keyA,
+        child: new RekeyableDummyStatefulWidgetWrapper(
+          initialKey: keyC
+        ),
+      ),
+      b: new KeyedSubtree(
+        key: keyB,
+        child: new Builder(
+          builder: (BuildContext context) {
+            return new Builder(
+              builder: (BuildContext context) {
+                return new Builder(
+                  builder: (BuildContext context) {
+                    return new LayoutBuilder(
+                      builder: (BuildContext context, BoxConstraints constraints) {
+                        return new RekeyableDummyStatefulWidgetWrapper(
+                          initialKey: keyD
+                        );
+                      }
+                    );
+                  }
+                );
+              }
+            );
+          }
+        )
+      ),
+    ));
+
+    expect(find.byKey(keyA), findsOneWidget);
+    expect(find.byKey(keyB), findsOneWidget);
+    expect(find.byKey(keyC), findsOneWidget);
+    expect(find.byKey(keyD), findsOneWidget);
+    expect(find.byType(RekeyableDummyStatefulWidgetWrapper), findsNWidgets(2));
+    expect(find.byType(DummyStatefulWidget), findsNWidgets(2));
+
+    keyRoot.currentState.switchChildren();
+    List<State> states = tester.stateList(find.byType(RekeyableDummyStatefulWidgetWrapper)).toList();
+    RekeyableDummyStatefulWidgetWrapperState a = states[0]; a._setChild(null);
+    RekeyableDummyStatefulWidgetWrapperState b = states[1]; b._setChild(keyC);
+    await tester.pump();
+
+    expect(find.byKey(keyA), findsOneWidget);
+    expect(find.byKey(keyB), findsOneWidget);
+    expect(find.byKey(keyC), findsOneWidget);
+    expect(find.byKey(keyD), findsNothing);
+    expect(find.byType(RekeyableDummyStatefulWidgetWrapper), findsNWidgets(2));
+    expect(find.byType(DummyStatefulWidget), findsNWidgets(2));
+  });
+}