From 4f0eff31bcea3a6acf8ad84bc24e8d58eadf7988 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Tue, 20 Sep 2016 14:14:05 -0700 Subject: [PATCH] Fix crash when a RenderObject tree is rooted in a non-RenderObject environment (#5933) --- dev/devicelab/test/run_test.dart | 2 +- .../flutter/lib/src/rendering/binding.dart | 20 ++++- packages/flutter/lib/src/rendering/node.dart | 6 ++ .../flutter/lib/src/rendering/object.dart | 86 +++++++++++++------ .../non_render_object_root_test.dart | 62 +++++++++++++ 5 files changed, 143 insertions(+), 33 deletions(-) create mode 100644 packages/flutter/test/rendering/non_render_object_root_test.dart diff --git a/dev/devicelab/test/run_test.dart b/dev/devicelab/test/run_test.dart index 382f32a000b..2a75476408a 100644 --- a/dev/devicelab/test/run_test.dart +++ b/dev/devicelab/test/run_test.dart @@ -35,7 +35,7 @@ void main() { test('Exits with code 1 when fails to connect', () async { expect(await runScript(['smoke_test_setup_failure']), 1); - }); + }, skip: true); // https://github.com/flutter/flutter/issues/5901 test('Exits with code 1 when results are mixed', () async { expect( diff --git a/packages/flutter/lib/src/rendering/binding.dart b/packages/flutter/lib/src/rendering/binding.dart index 4e407409441..169ab8b43bf 100644 --- a/packages/flutter/lib/src/rendering/binding.dart +++ b/packages/flutter/lib/src/rendering/binding.dart @@ -26,7 +26,11 @@ abstract class RendererBinding extends BindingBase implements SchedulerBinding, void initInstances() { super.initInstances(); _instance = this; - _pipelineOwner = new PipelineOwner(onNeedVisualUpdate: ensureVisualUpdate); + _pipelineOwner = new PipelineOwner( + onNeedVisualUpdate: ensureVisualUpdate, + onScheduleInitialSemantics: _scheduleInitialSemantics, + onClearSemantics: _clearSemantics, + ); ui.window.onMetricsChanged = handleMetricsChanged; initRenderView(); initSemantics(); @@ -96,12 +100,12 @@ abstract class RendererBinding extends BindingBase implements SchedulerBinding, PipelineOwner _pipelineOwner; /// The render tree that's attached to the output surface. - RenderView get renderView => _pipelineOwner.rootRenderObject; + RenderView get renderView => _pipelineOwner.rootNode; /// Sets the given [RenderView] object (which must not be null), and its tree, to /// be the new render tree to display. The previous tree, if any, is detached. set renderView(RenderView value) { assert(value != null); - _pipelineOwner.rootRenderObject = value; + _pipelineOwner.rootNode = value; } /// Called when the system metrics change. @@ -210,7 +214,7 @@ abstract class RendererBinding extends BindingBase implements SchedulerBinding, @override void reassembleApplication() { super.reassembleApplication(); - pipelineOwner.reassemble(); + renderView.reassemble(); handleBeginFrame(null); } @@ -229,6 +233,14 @@ abstract class RendererBinding extends BindingBase implements SchedulerBinding, }; instance?.renderView?.visitChildren(visitor); } + + void _scheduleInitialSemantics() { + renderView.scheduleInitialSemantics(); + } + + void _clearSemantics() { + renderView.clearSemantics(); + } } /// Prints a textual representation of the entire render tree. diff --git a/packages/flutter/lib/src/rendering/node.dart b/packages/flutter/lib/src/rendering/node.dart index 3adf5fe799f..7ec742db94a 100644 --- a/packages/flutter/lib/src/rendering/node.dart +++ b/packages/flutter/lib/src/rendering/node.dart @@ -76,6 +76,9 @@ class AbstractNode { /// /// Typically called only from the parent's attach(), and to mark the root of /// a tree attached. + /// + /// Subclasses with children should attach all their children to the same + /// [owner] whenever this method is called. @mustCallSuper void attach(Object owner) { assert(owner != null); @@ -87,6 +90,9 @@ class AbstractNode { /// /// Typically called only from the parent's detach(), and to mark the root of /// a tree detached. + /// + /// Subclasses with children should detach all their children whenever this + /// method is called. @mustCallSuper void detach() { assert(_owner != null); diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart index 88183802a5d..d5b79f5cc59 100644 --- a/packages/flutter/lib/src/rendering/object.dart +++ b/packages/flutter/lib/src/rendering/object.dart @@ -789,7 +789,11 @@ class PipelineOwner { /// Typically created by the binding (e.g., [RendererBinding]), but can be /// created separately from the binding to drive off-screen render objects /// through the rendering pipeline. - PipelineOwner({ this.onNeedVisualUpdate }); + PipelineOwner({ + this.onNeedVisualUpdate, + this.onScheduleInitialSemantics, + this.onClearSemantics, + }); /// Called when a render object associated with this pipeline owner wishes to /// update its visual appearance. @@ -800,6 +804,21 @@ class PipelineOwner { /// duplicate calls quickly. final VoidCallback onNeedVisualUpdate; + /// Called when [addSemanticsListener] is called when there was no + /// [SemanticsOwner] present, to request that the + /// [RenderObject.scheduleInitialSemantics] method be called on the + /// appropriate object(s). + /// + /// For example, the [RendererBinding] calls it on the [RenderView] object. + final VoidCallback onScheduleInitialSemantics; + + /// Called when the last [SemanticsListener] is removed from the + /// [SemanticsOwner], to request that the [RenderObject.clearSemantics] method + /// be called on the appropriate object(s). + /// + /// For example, the [RendererBinding] calls it on the [RenderView] object. + final VoidCallback onClearSemantics; + /// Calls [onNeedVisualUpdate] if [onNeedVisualUpdate] is not null. /// /// Used to notify the pipeline owner that an associated render object wishes @@ -809,15 +828,17 @@ class PipelineOwner { onNeedVisualUpdate(); } - /// The unique render object managed by this pipeline that has no parent. - RenderObject get rootRenderObject => _rootRenderObject; - RenderObject _rootRenderObject; - set rootRenderObject(RenderObject value) { - if (_rootRenderObject == value) + /// The unique object managed by this pipeline that has no parent. + /// + /// This object does not have to be a [RenderObject]. + AbstractNode get rootNode => _rootNode; + AbstractNode _rootNode; + set rootNode(AbstractNode value) { + if (_rootNode == value) return; - _rootRenderObject?.detach(); - _rootRenderObject = value; - _rootRenderObject?.attach(this); + _rootNode?.detach(); + _rootNode = value; + _rootNode?.attach(this); } /// Calls the given listener whenever the semantics of the render tree change. @@ -829,7 +850,8 @@ class PipelineOwner { initialListener: listener, onLastListenerRemoved: _handleLastSemanticsListenerRemoved ); - _rootRenderObject.scheduleInitialSemantics(); + if (onScheduleInitialSemantics != null) + onScheduleInitialSemantics(); } else { _semanticsOwner.addListener(listener); } @@ -839,7 +861,8 @@ class PipelineOwner { void _handleLastSemanticsListenerRemoved() { assert(!_debugDoingSemantics); - rootRenderObject._clearSemantics(); + if (onClearSemantics != null) + onClearSemantics(); _semanticsOwner.dispose(); _semanticsOwner = null; } @@ -993,16 +1016,6 @@ class PipelineOwner { } _semanticsOwner.sendSemanticsTree(); } - - /// Cause the entire render tree rooted at [rootRenderObject] to be entirely - /// reprocessed. This is used by development tools when the application code - /// has changed, to cause the rendering tree to pick up any changed - /// implementations. - /// - /// This is expensive and should not be called except during development. - void reassemble() { - _rootRenderObject?._reassemble(); - } } // See _performLayout. @@ -1038,14 +1051,25 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { _performLayout = performLayout; } - void _reassemble() { + /// Cause the entire subtree rooted at the given [RenderObject] to be marked + /// dirty for layout, paint, etc. This is called by the [RendererBinding] in + /// response to the `ext.flutter.reassemble` hook, which is used by + /// development tools when the application code has changed, to cause the + /// widget tree to pick up any changed implementations. + /// + /// This is expensive and should not be called except during development. + /// + /// See also: + /// + /// * [BindingBase.reassembleApplication]. + void reassemble() { _performLayout = performLayout; markNeedsLayout(); markNeedsCompositingBitsUpdate(); markNeedsPaint(); markNeedsSemanticsUpdate(); visitChildren((RenderObject child) { - child._reassemble(); + child.reassemble(); }); } @@ -1490,12 +1514,13 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { )); assert(!_debugDoingThisResize); assert(!_debugDoingThisLayout); - final RenderObject parent = this.parent; RenderObject relayoutBoundary; - if (!parentUsesSize || sizedByParent || constraints.isTight || parent is! RenderObject) + if (!parentUsesSize || sizedByParent || constraints.isTight || parent is! RenderObject) { relayoutBoundary = this; - else + } else { + final RenderObject parent = this.parent; relayoutBoundary = parent._relayoutBoundary; + } assert(parent == this.parent); assert(() { _debugCanParentUseSize = parentUsesSize; @@ -1992,12 +2017,17 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { } /// Removes all semantics from this render object and its descendants. - void _clearSemantics() { + /// + /// Should only be called in response to the [PipelineOwner] calling its + /// [PipelineOwner.onClearSemantics] callback. + /// + /// Should only be called on objects whose [parent] is not a [RenderObject]. + void clearSemantics() { _needsSemanticsUpdate = true; _needsSemanticsGeometryUpdate = true; _semantics = null; visitChildren((RenderObject child) { - child._clearSemantics(); + child.clearSemantics(); }); } diff --git a/packages/flutter/test/rendering/non_render_object_root_test.dart b/packages/flutter/test/rendering/non_render_object_root_test.dart new file mode 100644 index 00000000000..728a7448bf8 --- /dev/null +++ b/packages/flutter/test/rendering/non_render_object_root_test.dart @@ -0,0 +1,62 @@ +// Copyright 2015 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/material.dart'; +import 'package:flutter/rendering.dart'; +import 'package:test/test.dart'; + +import 'rendering_tester.dart'; + +class RealRoot extends AbstractNode { + RealRoot(this.child) { + if (child != null) + adoptChild(child); + } + + final RenderObject child; + + @override + void redepthChildren() { + if (child != null) + redepthChild(child); + } + + @override + void attach(Object owner) { + super.attach(owner); + child?.attach(owner); + } + + @override + void detach() { + super.detach(); + child?.detach(); + } + + @override + PipelineOwner get owner => super.owner; + + void layout() { + child?.layout(new BoxConstraints.tight(const Size(500.0, 500.0))); + } +} + +void main() { + test("non-RenderObject roots", () { + RenderPositionedBox child; + RealRoot root = new RealRoot( + child = new RenderPositionedBox( + alignment: FractionalOffset.center, + child: new RenderSizedBox(new Size(100.0, 100.0)) + ) + ); + root.attach(new PipelineOwner()); + + child.scheduleInitialLayout(); + root.layout(); + + child.markNeedsLayout(); + root.layout(); + }); +}