Block should work inside LazyBlock (#3546)

Previously we were locking down the state even when calling layout in
LazyBlock. Now we lock only when building children. Making this work well
involved moving the catch out of lockState and into the few callers who
actually wanted it.

Fixes #3534

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

@@ -285,7 +285,7 @@ class RenderObjectToWidgetAdapter<T extends RenderObject> extends RenderObjectWi
       } else {
         element.update(this);
       }
-    }, building: true, context: 'while attaching root widget to rendering tree');
+    }, building: true);
     return element;
   }
 

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

@@ -710,7 +710,7 @@ class BuildOwner {
   ///
   /// The context argument is used to describe the scope in case an exception is
   /// caught while invoking the callback.
-  void lockState(void callback(), { bool building: false, String context }) {
+  void lockState(void callback(), { bool building: false }) {
     bool debugPreviouslyBuilding;
     assert(_debugStateLockLevel >= 0);
     assert(() {
@@ -723,8 +723,6 @@ class BuildOwner {
     });
     try {
       callback();
-    } catch (e, stack) {
-      _debugReportException(context, e, stack);
     } finally {
       assert(() {
         _debugStateLockLevel -= 1;
@@ -758,23 +756,25 @@ class BuildOwner {
     if (_dirtyElements.isEmpty)
       return;
     Timeline.startSync('Build');
-    lockState(() {
-      _dirtyElements.sort(_elementSort);
-      int dirtyCount = _dirtyElements.length;
-      int index = 0;
-      while (index < dirtyCount) {
-        _dirtyElements[index].rebuild();
-        index += 1;
-        if (dirtyCount < _dirtyElements.length) {
-          _dirtyElements.sort(_elementSort);
-          dirtyCount = _dirtyElements.length;
+    try {
+      lockState(() {
+        _dirtyElements.sort(_elementSort);
+        int dirtyCount = _dirtyElements.length;
+        int index = 0;
+        while (index < dirtyCount) {
+          _dirtyElements[index].rebuild();
+          index += 1;
+          if (dirtyCount < _dirtyElements.length) {
+            _dirtyElements.sort(_elementSort);
+            dirtyCount = _dirtyElements.length;
+          }
         }
-      }
-      assert(!_dirtyElements.any((BuildableElement element) => element.dirty));
+        assert(!_dirtyElements.any((BuildableElement element) => element.dirty));
+      }, building: true);
+    } finally {
       _dirtyElements.clear();
-    }, building: true, context: 'while rebuilding dirty elements');
-    assert(_dirtyElements.isEmpty);
-    Timeline.finishSync();
+      Timeline.finishSync();
+    }
   }
 
   /// Complete the element build pass by unmounting any elements that are no
@@ -789,12 +789,17 @@ class BuildOwner {
   /// about changes to global keys will run.
   void finalizeTree() {
     Timeline.startSync('Finalize tree');
-    lockState(() {
-      _inactiveElements._unmountAll();
-    }, context: 'while finalizing the widget tree');
-    assert(GlobalKey._debugCheckForDuplicates);
-    scheduleMicrotask(GlobalKey._notifyListeners);
-    Timeline.finishSync();
+    try {
+      lockState(() {
+        _inactiveElements._unmountAll();
+      });
+      assert(GlobalKey._debugCheckForDuplicates);
+      scheduleMicrotask(GlobalKey._notifyListeners);
+    } catch (e, stack) {
+      _debugReportException('while finalizing the widget tree', e, stack);
+    } finally {
+      Timeline.finishSync();
+    }
   }
 
   /// Cause the entire subtree rooted at the given [Element] to

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

@@ -494,165 +494,184 @@ class _LazyBlockElement extends RenderObjectElement {
   void _layout(BoxConstraints constraints) {
     final double blockExtent = _getMainAxisExtent(renderObject.size);
 
-    owner.lockState(() {
-      final IndexedBuilder builder = widget.delegate.buildItem;
-      final double startLogicalOffset = widget.startOffset;
-      final double endLogicalOffset = startLogicalOffset + blockExtent;
-      final _RenderLazyBlock block = renderObject;
-      final BoxConstraints innerConstraints = _getInnerConstraints(constraints);
-
-      // A high watermark for which children have been through layout this pass.
-      int firstLogicalIndexNeedingLayout = _firstChildLogicalIndex;
-
-      // The index of the current child we're examining. The index is the same one
-      // used for the builder (as opposed to the physical index in the _children
-      // list).
-      int currentLogicalIndex = _firstChildLogicalIndex;
-
-      // The offset of the current child we're examining from the start of the
-      // entire block (in the direction of the main axis). As we compute layout
-      // information, we use dead reckoning to keep track of where all the
-      // children are based on this quantity.
-      double currentLogicalOffset = _firstChildLogicalOffset;
-
-      // First, we check if we need to inflate any children before the start of
-      // the viewport. Because we're dead reckoning from the current viewport, we
-      // inflate the children in reverse tree order.
-
-      if (currentLogicalIndex > 0 && currentLogicalOffset > startLogicalOffset) {
-        final List<Element> newChildren = <Element>[];
-
-        while (currentLogicalIndex > 0 && currentLogicalOffset > startLogicalOffset) {
-          currentLogicalIndex -= 1;
+    final IndexedBuilder builder = widget.delegate.buildItem;
+    final double startLogicalOffset = widget.startOffset;
+    final double endLogicalOffset = startLogicalOffset + blockExtent;
+    final _RenderLazyBlock block = renderObject;
+    final BoxConstraints innerConstraints = _getInnerConstraints(constraints);
+
+    // A high watermark for which children have been through layout this pass.
+    int firstLogicalIndexNeedingLayout = _firstChildLogicalIndex;
+
+    // The index of the current child we're examining. The index is the same one
+    // used for the builder (as opposed to the physical index in the _children
+    // list).
+    int currentLogicalIndex = _firstChildLogicalIndex;
+
+    // The offset of the current child we're examining from the start of the
+    // entire block (in the direction of the main axis). As we compute layout
+    // information, we use dead reckoning to keep track of where all the
+    // children are based on this quantity.
+    double currentLogicalOffset = _firstChildLogicalOffset;
+
+    // First, we check if we need to inflate any children before the start of
+    // the viewport. Because we're dead reckoning from the current viewport, we
+    // inflate the children in reverse tree order.
+
+    if (currentLogicalIndex > 0 && currentLogicalOffset > startLogicalOffset) {
+      final List<Element> newChildren = <Element>[];
+
+      while (currentLogicalIndex > 0 && currentLogicalOffset > startLogicalOffset) {
+        currentLogicalIndex -= 1;
+        Element newElement;
+        owner.lockState(() {
+          // TODO(abarth): Handle exceptions from builder gracefully.
           Widget newWidget = builder(this, currentLogicalIndex);
-          assert(newWidget != null);
+          if (newWidget == null) {
+            throw new FlutterError(
+              'buildItem must not return null after returning non-null.\n'
+              'If buildItem for a LazyBlockDelegate returns a non-null widget for a given '
+              'index, it must return non-null widgets for every smaller index as well. The '
+              'buildItem function for ${widget.delegate.runtimeType} returned null for '
+              'index $currentLogicalIndex after having returned a non-null value for index '
+              '${currentLogicalIndex - 1}.'
+            );
+          }
           newWidget = new RepaintBoundary.wrap(newWidget, currentLogicalIndex);
-          newChildren.add(inflateWidget(newWidget, null));
-          RenderBox child = block.firstChild;
-          assert(child == newChildren.last.renderObject);
-          child.layout(innerConstraints, parentUsesSize: true);
-          currentLogicalOffset -= _getMainAxisExtent(child.size);
-        }
-
-        final int numberOfNewChildren = newChildren.length;
-        _children.insertAll(0, newChildren.reversed);
-        _firstChildLogicalIndex = currentLogicalIndex;
-        _firstChildLogicalOffset = currentLogicalOffset;
-        firstLogicalIndexNeedingLayout = currentLogicalIndex + numberOfNewChildren;
-      } else if (currentLogicalOffset < startLogicalOffset) {
-        // If we didn't need to inflate more children before the viewport, we
-        // might need to deactivate children that have left the viewport from the
-        // top. We repeatedly check whether the first child overlaps the viewport
-        // and deactivate it if it's outside the viewport.
-        int currentPhysicalIndex = 0;
-        while (block.firstChild != null) {
-          RenderBox child = block.firstChild;
-          child.layout(innerConstraints, parentUsesSize: true);
-          firstLogicalIndexNeedingLayout += 1;
-          double childExtent = _getMainAxisExtent(child.size);
-          if (currentLogicalOffset + childExtent >= startLogicalOffset)
-            break;
-          deactivateChild(_children[currentPhysicalIndex]);
-          _children[currentPhysicalIndex] = null;
-          currentPhysicalIndex += 1;
-          currentLogicalIndex += 1;
-          currentLogicalOffset += childExtent;
-        }
-
-        if (currentPhysicalIndex > 0) {
-          _children.removeRange(0, currentPhysicalIndex);
-          _firstChildLogicalIndex = currentLogicalIndex;
-          _firstChildLogicalOffset = currentLogicalOffset;
-        }
+          newElement = inflateWidget(newWidget, null);
+        }, building: true);
+        newChildren.add(newElement);
+        RenderBox child = block.firstChild;
+        assert(child == newChildren.last.renderObject);
+        child.layout(innerConstraints, parentUsesSize: true);
+        currentLogicalOffset -= _getMainAxisExtent(child.size);
+      }
+
+      final int numberOfNewChildren = newChildren.length;
+      _children.insertAll(0, newChildren.reversed);
+      _firstChildLogicalIndex = currentLogicalIndex;
+      _firstChildLogicalOffset = currentLogicalOffset;
+      firstLogicalIndexNeedingLayout = currentLogicalIndex + numberOfNewChildren;
+    } else if (currentLogicalOffset < startLogicalOffset) {
+      // If we didn't need to inflate more children before the viewport, we
+      // might need to deactivate children that have left the viewport from the
+      // top. We repeatedly check whether the first child overlaps the viewport
+      // and deactivate it if it's outside the viewport.
+      int currentPhysicalIndex = 0;
+      while (block.firstChild != null) {
+        RenderBox child = block.firstChild;
+        child.layout(innerConstraints, parentUsesSize: true);
+        firstLogicalIndexNeedingLayout += 1;
+        double childExtent = _getMainAxisExtent(child.size);
+        if (currentLogicalOffset + childExtent >= startLogicalOffset)
+          break;
+        deactivateChild(_children[currentPhysicalIndex]);
+        _children[currentPhysicalIndex] = null;
+        currentPhysicalIndex += 1;
+        currentLogicalIndex += 1;
+        currentLogicalOffset += childExtent;
       }
 
-      // We've now established the invariant that the first physical child in the
-      // block is the first child that ought to be visible in the viewport. Now we
-      // need to walk forward until we've filled up the viewport. We might have
-      // already called layout for some of the children we encounter in this phase
-      // of the algorithm, we we'll need to be careful not to call layout on them again.
-
-      if (currentLogicalOffset >= startLogicalOffset) {
-        // The first element is visible. We need to update our reckoning of where
-        // the min scroll offset is.
-        _minScrollOffset = currentLogicalOffset;
-        _startOffsetLowerLimit = double.NEGATIVE_INFINITY;
-      } else {
-        // The first element is not visible. Ensure that we have one blockExtent
-        // of headroom so we don't hit the min scroll offset prematurely.
-        _minScrollOffset = currentLogicalOffset - blockExtent;
-        _startOffsetLowerLimit = currentLogicalOffset;
+      if (currentPhysicalIndex > 0) {
+        _children.removeRange(0, currentPhysicalIndex);
+        _firstChildLogicalIndex = currentLogicalIndex;
+        _firstChildLogicalOffset = currentLogicalOffset;
       }
+    }
 
-      // Materialize new children until we fill the viewport (or run out of
-      // children to materialize).
+    // We've now established the invariant that the first physical child in the
+    // block is the first child that ought to be visible in the viewport. Now we
+    // need to walk forward until we've filled up the viewport. We might have
+    // already called layout for some of the children we encounter in this phase
+    // of the algorithm, we we'll need to be careful not to call layout on them again.
+
+    if (currentLogicalOffset >= startLogicalOffset) {
+      // The first element is visible. We need to update our reckoning of where
+      // the min scroll offset is.
+      _minScrollOffset = currentLogicalOffset;
+      _startOffsetLowerLimit = double.NEGATIVE_INFINITY;
+    } else {
+      // The first element is not visible. Ensure that we have one blockExtent
+      // of headroom so we don't hit the min scroll offset prematurely.
+      _minScrollOffset = currentLogicalOffset - blockExtent;
+      _startOffsetLowerLimit = currentLogicalOffset;
+    }
 
-      RenderBox child;
-      while (currentLogicalOffset < endLogicalOffset) {
-        int physicalIndex = currentLogicalIndex - _firstChildLogicalIndex;
-        if (physicalIndex >= _children.length) {
-          assert(physicalIndex == _children.length);
+    // Materialize new children until we fill the viewport (or run out of
+    // children to materialize).
+
+    RenderBox child;
+    while (currentLogicalOffset < endLogicalOffset) {
+      int physicalIndex = currentLogicalIndex - _firstChildLogicalIndex;
+      if (physicalIndex >= _children.length) {
+        assert(physicalIndex == _children.length);
+        Element newElement;
+        owner.lockState(() {
+          // TODO(abarth): Handle exceptions from builder gracefully.
           Widget newWidget = builder(this, currentLogicalIndex);
           if (newWidget == null)
-            break;
+            return;
           newWidget = new RepaintBoundary.wrap(newWidget, currentLogicalIndex);
           Element previousChild = _children.isEmpty ? null : _children.last;
-          _children.add(inflateWidget(newWidget, previousChild));
-        }
-        child = _getNextWithin(block, child);
-        assert(child != null);
-        if (currentLogicalIndex >= firstLogicalIndexNeedingLayout) {
-          assert(currentLogicalIndex == firstLogicalIndexNeedingLayout);
-          child.layout(innerConstraints, parentUsesSize: true);
-          firstLogicalIndexNeedingLayout += 1;
-        }
-        currentLogicalOffset += _getMainAxisExtent(child.size);
-        currentLogicalIndex += 1;
+          newElement = inflateWidget(newWidget, previousChild);
+        }, building: true);
+        if (newElement == null)
+          return;
+        _children.add(newElement);
       }
-
-      // We now have all the physical children we ought to have to fill the
-      // viewport. The currentLogicalIndex is the index of the first child that
-      // we don't need.
-
-      if (currentLogicalOffset < endLogicalOffset) {
-        // The last element is visible. We need to update our reckoning of where
-        // the max scroll offset is.
-        _maxScrollOffset = currentLogicalOffset + widget._mainAxisPadding - blockExtent;
-        _startOffsetUpperLimit = double.INFINITY;
-      } else {
-        // The last element is not visible. Ensure that we have one blockExtent
-        // of headroom so we don't hit the max scroll offset prematurely.
-        _maxScrollOffset = currentLogicalOffset;
-        _startOffsetUpperLimit = currentLogicalOffset - blockExtent;
+      child = _getNextWithin(block, child);
+      assert(child != null);
+      if (currentLogicalIndex >= firstLogicalIndexNeedingLayout) {
+        assert(currentLogicalIndex == firstLogicalIndexNeedingLayout);
+        child.layout(innerConstraints, parentUsesSize: true);
+        firstLogicalIndexNeedingLayout += 1;
       }
+      currentLogicalOffset += _getMainAxisExtent(child.size);
+      currentLogicalIndex += 1;
+    }
 
-      // Remove any unneeded children.
+    // We now have all the physical children we ought to have to fill the
+    // viewport. The currentLogicalIndex is the index of the first child that
+    // we don't need.
 
-      int currentPhysicalIndex = currentLogicalIndex - _firstChildLogicalIndex;
-      final int numberOfRequiredPhysicalChildren = currentPhysicalIndex;
-      while (currentPhysicalIndex < _children.length) {
-        deactivateChild(_children[currentPhysicalIndex]);
-        _children[currentPhysicalIndex] = null;
-        currentPhysicalIndex += 1;
-      }
-      _children.length = numberOfRequiredPhysicalChildren;
-
-      // We now have the correct physical children, each of which has gone through
-      // layout exactly once. We still need to position them correctly. We
-      // position the first physical child at Offset.zero and use the paintOffset
-      // on the render object to adjust the final paint location of the children.
-
-      Offset currentChildOffset = _initialChildOffset;
-      child = block.firstChild;
-      while (child != null) {
-        final _LazyBlockParentData childParentData = child.parentData;
-        childParentData.offset = currentChildOffset;
-        currentChildOffset += _getMainAxisOffsetForSize(child.size);
-        child = childParentData.nextSibling;
-      }
+    if (currentLogicalOffset < endLogicalOffset) {
+      // The last element is visible. We need to update our reckoning of where
+      // the max scroll offset is.
+      _maxScrollOffset = currentLogicalOffset + widget._mainAxisPadding - blockExtent;
+      _startOffsetUpperLimit = double.INFINITY;
+    } else {
+      // The last element is not visible. Ensure that we have one blockExtent
+      // of headroom so we don't hit the max scroll offset prematurely.
+      _maxScrollOffset = currentLogicalOffset;
+      _startOffsetUpperLimit = currentLogicalOffset - blockExtent;
+    }
 
-      _updatePaintOffset();
-    }, building: true, context: 'during $runtimeType layout');
+    // Remove any unneeded children.
+
+    int currentPhysicalIndex = currentLogicalIndex - _firstChildLogicalIndex;
+    final int numberOfRequiredPhysicalChildren = currentPhysicalIndex;
+    while (currentPhysicalIndex < _children.length) {
+      deactivateChild(_children[currentPhysicalIndex]);
+      _children[currentPhysicalIndex] = null;
+      currentPhysicalIndex += 1;
+    }
+    _children.length = numberOfRequiredPhysicalChildren;
+
+    // We now have the correct physical children, each of which has gone through
+    // layout exactly once. We still need to position them correctly. We
+    // position the first physical child at Offset.zero and use the paintOffset
+    // on the render object to adjust the final paint location of the children.
+
+    Offset currentChildOffset = _initialChildOffset;
+    child = block.firstChild;
+    while (child != null) {
+      final _LazyBlockParentData childParentData = child.parentData;
+      childParentData.offset = currentChildOffset;
+      currentChildOffset += _getMainAxisOffsetForSize(child.size);
+      child = childParentData.nextSibling;
+    }
+
+    _updatePaintOffset();
 
     LazyBlockExtentsChangedCallback onExtentsChanged = widget.onExtentsChanged;
     if (onExtentsChanged != null) {

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

@@ -164,7 +164,7 @@ abstract class VirtualViewportElement extends RenderObjectElement {
     assert(startOffsetBase != null);
     assert(startOffsetLimit != null);
     _updatePaintOffset();
-    owner.lockState(_materializeChildren, building: true, context: 'during $runtimeType layout');
+    owner.lockState(_materializeChildren, building: true);
   }
 
   void _materializeChildren() {

packages/flutter/test/widget/lazy_block_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/material.dart';
+import 'package:test/test.dart';
+
+void main() {
+  test('Block inside LazyBlock', () {
+    testWidgets((WidgetTester tester) {
+      tester.pumpWidget(new LazyBlock(
+        delegate: new LazyBlockChildren(
+          children: <Widget>[
+            new Block(
+              children: <Widget>[
+                new Text('1'),
+                new Text('2'),
+                new Text('3'),
+              ]
+            ),
+            new Block(
+              children: <Widget>[
+                new Text('4'),
+                new Text('5'),
+                new Text('6'),
+              ]
+            ),
+          ]
+        )
+      ));
+    });
+  });
+}