diff --git a/examples/layers/rendering/src/sector_layout.dart b/examples/layers/rendering/src/sector_layout.dart index f37f48afc9..4234d7e93d 100644 --- a/examples/layers/rendering/src/sector_layout.dart +++ b/examples/layers/rendering/src/sector_layout.dart @@ -455,7 +455,6 @@ class RenderSectorSlice extends RenderSectorWithChildren { } class RenderBoxToRenderSectorAdapter extends RenderBox with RenderObjectWithChildMixin { - RenderBoxToRenderSectorAdapter({ double innerRadius = 0.0, RenderSector? child }) : _innerRadius = innerRadius { this.child = child; @@ -567,7 +566,6 @@ class RenderBoxToRenderSectorAdapter extends RenderBox with RenderObjectWithChil result.add(BoxHitTestEntry(this, position)); return true; } - } class RenderSolidColor extends RenderDecoratedSector { diff --git a/examples/layers/widgets/sectors.dart b/examples/layers/widgets/sectors.dart index 2ed2aae3a3..af96dc7cac 100644 --- a/examples/layers/widgets/sectors.dart +++ b/examples/layers/widgets/sectors.dart @@ -90,6 +90,13 @@ class SectorAppState extends State { }); } + void recursivelyDisposeChildren(RenderObject parent) { + parent.visitChildren((RenderObject child) { + recursivelyDisposeChildren(child); + child.dispose(); + }); + } + Widget buildBody() { return Column( mainAxisAlignment: MainAxisAlignment.spaceBetween, @@ -107,7 +114,12 @@ class SectorAppState extends State { Container( padding: const EdgeInsets.all(4.0), margin: const EdgeInsets.only(right: 10.0), - child: WidgetToRenderBoxAdapter(renderBox: sectorAddIcon), + child: WidgetToRenderBoxAdapter( + renderBox: sectorAddIcon, + onUnmount: () { + recursivelyDisposeChildren(sectorAddIcon); + }, + ), ), const Text('ADD SECTOR'), ], @@ -122,7 +134,12 @@ class SectorAppState extends State { Container( padding: const EdgeInsets.all(4.0), margin: const EdgeInsets.only(right: 10.0), - child: WidgetToRenderBoxAdapter(renderBox: sectorRemoveIcon), + child: WidgetToRenderBoxAdapter( + renderBox: sectorRemoveIcon, + onUnmount: () { + recursivelyDisposeChildren(sectorRemoveIcon); + }, + ), ), const Text('REMOVE SECTOR'), ], @@ -142,6 +159,9 @@ class SectorAppState extends State { child: WidgetToRenderBoxAdapter( renderBox: sectors, onBuild: doUpdates, + onUnmount: () { + recursivelyDisposeChildren(sectors); + }, ), ), ), diff --git a/packages/flutter/lib/src/rendering/editable.dart b/packages/flutter/lib/src/rendering/editable.dart index 913d311db4..e3036b33fa 100644 --- a/packages/flutter/lib/src/rendering/editable.dart +++ b/packages/flutter/lib/src/rendering/editable.dart @@ -283,6 +283,15 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin { _RenderEditableCustomPaint? _foregroundRenderObject; _RenderEditableCustomPaint? _backgroundRenderObject; + @override + void dispose() { + _foregroundRenderObject?.dispose(); + _foregroundRenderObject = null; + _backgroundRenderObject?.dispose(); + _backgroundRenderObject = null; + super.dispose(); + } + void _updateForegroundPainter(RenderEditablePainter? newPainter) { final _CompositeRenderEditablePainter effectivePainter = newPainter == null ? _builtInForegroundPainters diff --git a/packages/flutter/lib/src/rendering/image.dart b/packages/flutter/lib/src/rendering/image.dart index 98c2d90ec4..fe0d73343a 100644 --- a/packages/flutter/lib/src/rendering/image.dart +++ b/packages/flutter/lib/src/rendering/image.dart @@ -438,6 +438,13 @@ class RenderImage extends RenderBox { ); } + @override + void dispose() { + _image?.dispose(); + _image = null; + super.dispose(); + } + @override void debugFillProperties(DiagnosticPropertiesBuilder properties) { super.debugFillProperties(properties); diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart index f842febef0..2dec3d247a 100644 --- a/packages/flutter/lib/src/rendering/object.dart +++ b/packages/flutter/lib/src/rendering/object.dart @@ -1111,6 +1111,19 @@ class PipelineOwner { /// The [RenderBox] subclass introduces the opinion that the layout /// system uses Cartesian coordinates. /// +/// ## Lifecycle +/// +/// A [RenderObject] must [dispose] when it is no longer needed. The creator +/// of the object is responsible for disposing of it. Typically, the creator is +/// a [RenderObjectElement], and that element will dispose the object it creates +/// when it is unmounted. +/// +/// [RenderObject]s are responsible for cleaning up any expensive resources +/// they hold when [dispose] is called, such as [Picture] or [Image] objects. +/// This includes any [Layer]s that the render object has directly created. The +/// base implementation of dispose will nullify the [layer] property. Subclasses +/// must also nullify any other layer(s) it directly creates. +/// /// ## Writing a RenderObject subclass /// /// In most cases, subclassing [RenderObject] itself is overkill, and @@ -1230,6 +1243,52 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im }); } + /// Whether this has been disposed. + /// + /// If asserts are disabled, this property is always null. + bool? get debugDisposed { + bool? disposed; + assert(() { + disposed = _debugDisposed; + return true; + }()); + return disposed; + } + + bool _debugDisposed = false; + + /// Release any resources held by this render object. + /// + /// The object that creates a RenderObject is in charge of disposing it. + /// If this render object has created any children directly, it must dispose + /// of those children in this method as well. It must not dispose of any + /// children that were created by some other object, such as + /// a [RenderObjectElement]. Those children will be disposed when that + /// element unmounts, which may be delayed if the element is moved to another + /// part of the tree. + /// + /// Implementations of this method must end with a call to the inherited + /// method, as in `super.dispose()`. + /// + /// The object is no longer usable after calling dispose. + @mustCallSuper + void dispose() { + assert(!_debugDisposed); + _layer = null; + assert(() { + // TODO(dnfield): Enable this assert once clients have had a chance to + // migrate. + // visitChildren((RenderObject child) { + // assert( + // child.debugDisposed!, + // '${child.runtimeType} (child of $runtimeType) must be disposed before calling super.dispose().', + // ); + // }); + _debugDisposed = true; + return true; + }()); + } + // LAYOUT /// Data for use by the parent render object. @@ -1367,6 +1426,10 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im bool get _debugCanPerformMutations { late bool result; assert(() { + if (_debugDisposed) { + result = false; + return true; + } if (owner != null && !owner!.debugDoingLayout) { result = true; return true; @@ -1401,6 +1464,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im @override void attach(PipelineOwner owner) { + assert(!_debugDisposed); super.attach(owner); // If the node was dirtied in some way while unattached, make sure to add // it to the appropriate dirty list now that an owner is available @@ -1612,6 +1676,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im /// /// See [RenderView] for an example of how this function is used. void scheduleInitialLayout() { + assert(!_debugDisposed); assert(attached); assert(parent is! RenderObject); assert(!owner!._debugDoingLayout); @@ -1681,6 +1746,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im /// work to update its layout information. @pragma('vm:notify-debugger-on-exception') void layout(Constraints constraints, { bool parentUsesSize = false }) { + assert(!_debugDisposed); if (!kReleaseMode && debugProfileLayoutsEnabled) Timeline.startSync('$runtimeType', arguments: timelineArgumentsIndicatingLandmarkEvent); @@ -2040,6 +2106,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im /// it cannot be the case that _only_ the compositing bits changed, /// something else will have scheduled a frame for us. void markNeedsCompositingBitsUpdate() { + assert(!_debugDisposed); if (_needsCompositingBitsUpdate) return; _needsCompositingBitsUpdate = true; @@ -2138,6 +2205,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im /// layer, thus limiting the number of nodes that [markNeedsPaint] must mark /// dirty. void markNeedsPaint() { + assert(!_debugDisposed); assert(owner == null || !owner!.debugDoingPaint); if (_needsPaint) return; @@ -2222,6 +2290,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im /// /// This might be called if, e.g., the device pixel ratio changed. void replaceRootLayer(OffsetLayer rootLayer) { + assert(!_debugDisposed); assert(rootLayer.attached); assert(attached); assert(parent is! RenderObject); @@ -2234,6 +2303,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im } void _paintWithContext(PaintingContext context, Offset offset) { + assert(!_debugDisposed); assert(() { if (_debugDoingThisPaint) { throw FlutterError.fromParts([ @@ -2457,6 +2527,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im /// /// See [RendererBinding] for an example of how this function is used. void scheduleInitialSemantics() { + assert(!_debugDisposed); assert(attached); assert(parent is! RenderObject); assert(!owner!._debugDoingSemantics); @@ -2579,6 +2650,7 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im /// [RenderObject] as annotated by [describeSemanticsConfiguration] changes in /// any way to update the semantics tree. void markNeedsSemanticsUpdate() { + assert(!_debugDisposed); assert(!attached || !owner!._debugDoingSemantics); if (!attached || owner!._semanticsOwner == null) { _cachedSemanticsConfiguration = null; @@ -2824,6 +2896,10 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im @override String toStringShort() { String header = describeIdentity(this); + if (_debugDisposed) { + header += ' DISPOSED'; + return header; + } if (_relayoutBoundary != null && _relayoutBoundary != this) { int count = 1; RenderObject? target = parent as RenderObject?; @@ -3209,6 +3285,7 @@ mixin ContainerRenderObjectMixin renderBox; @@ -6264,6 +6292,12 @@ class WidgetToRenderBoxAdapter extends LeafRenderObjectWidget { void updateRenderObject(BuildContext context, RenderBox renderObject) { onBuild?.call(); } + + @override + void didUnmountRenderObject(RenderObject renderObject) { + assert(renderObject == renderBox); + onUnmount?.call(); + } } diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index c2c160dbd2..bf0029c805 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -3200,10 +3200,13 @@ abstract class Element extends DiagnosticableTree implements BuildContext { RenderObject? result; void visit(Element element) { assert(result == null); // this verifies that there's only one child - if (element is RenderObjectElement) + if (element._lifecycleState == _ElementLifecycle.defunct) { + return; + } else if (element is RenderObjectElement) { result = element.renderObject; - else + } else { element.visitChildren(visit); + } } visit(this); return result; @@ -3842,6 +3845,10 @@ abstract class Element extends DiagnosticableTree implements BuildContext { /// After this function is called, the element will not be incorporated into /// the tree again. /// + /// Any resources this element holds should be released at this point. For + /// example, [RenderObjectElement.unmount] calls [RenderObject.dispose] and + /// nulls out its reference to the render object. + /// /// See the lifecycle documentation for [Element] for additional information. /// /// Implementations of this method should end with a call to the inherited @@ -5441,8 +5448,13 @@ abstract class RenderObjectElement extends Element { RenderObjectWidget get widget => super.widget as RenderObjectWidget; /// The underlying [RenderObject] for this element. + /// + /// If this element has been [unmount]ed, this getter will throw. @override - RenderObject get renderObject => _renderObject!; + RenderObject get renderObject { + assert(_renderObject != null, '$runtimeType unmounted'); + return _renderObject!; + } RenderObject? _renderObject; bool _debugDoingBuild = false; @@ -5510,6 +5522,7 @@ abstract class RenderObjectElement extends Element { return true; }()); _renderObject = widget.createRenderObject(this); + assert(!_renderObject!.debugDisposed!); assert(() { _debugDoingBuild = false; return true; @@ -5787,6 +5800,11 @@ abstract class RenderObjectElement extends Element { @override void unmount() { + assert( + !renderObject.debugDisposed!, + 'A RenderObject was disposed prior to its owning element being unmounted: ' + '$renderObject', + ); final RenderObjectWidget oldWidget = widget; super.unmount(); assert( @@ -5795,6 +5813,8 @@ abstract class RenderObjectElement extends Element { 'RenderObjectElement: $renderObject', ); oldWidget.didUnmountRenderObject(renderObject); + _renderObject!.dispose(); + _renderObject = null; } void _updateParentData(ParentDataWidget parentDataWidget) { diff --git a/packages/flutter/lib/src/widgets/sliver_persistent_header.dart b/packages/flutter/lib/src/widgets/sliver_persistent_header.dart index 0464d8d637..431ee3b1a0 100644 --- a/packages/flutter/lib/src/widgets/sliver_persistent_header.dart +++ b/packages/flutter/lib/src/widgets/sliver_persistent_header.dart @@ -268,8 +268,8 @@ class _SliverPersistentHeaderElement extends RenderObjectElement { @override void unmount() { - super.unmount(); renderObject._element = null; + super.unmount(); } @override diff --git a/packages/flutter/test/rendering/image_test.dart b/packages/flutter/test/rendering/image_test.dart index d7e8e1f2cb..fae48396f3 100644 --- a/packages/flutter/test/rendering/image_test.dart +++ b/packages/flutter/test/rendering/image_test.dart @@ -174,7 +174,7 @@ Future main() async { expect(image.colorBlendMode, BlendMode.color); }); - test('Render image disposes its image', () async { + test('RenderImage disposes its image', () async { final ui.Image image = await createTestImage(width: 10, height: 10, cache: false); expect(image.debugGetOpenHandleStackTraces()!.length, 1); @@ -191,7 +191,7 @@ Future main() async { expect(image.debugGetOpenHandleStackTraces()!.length, 0); }, skip: kIsWeb); // Web doesn't track open image handles. - test('Render image does not dispose its image if setting the same image twice', () async { + test('RenderImage does not dispose its image if setting the same image twice', () async { final ui.Image image = await createTestImage(width: 10, height: 10, cache: false); expect(image.debugGetOpenHandleStackTraces()!.length, 1); @@ -207,4 +207,19 @@ Future main() async { image.dispose(); expect(image.debugGetOpenHandleStackTraces()!.length, 0); }, skip: kIsWeb); // Web doesn't track open image handles. + + test('Render image disposes its image when it is disposed', () async { + final ui.Image image = await createTestImage(width: 10, height: 10, cache: false); + expect(image.debugGetOpenHandleStackTraces()!.length, 1); + + final RenderImage renderImage = RenderImage(image: image.clone()); + expect(image.debugGetOpenHandleStackTraces()!.length, 2); + + renderImage.dispose(); + expect(image.debugGetOpenHandleStackTraces()!.length, 1); + expect(renderImage.image, null); + + image.dispose(); + expect(image.debugGetOpenHandleStackTraces()!.length, 0); + }, skip: kIsWeb); // Web doesn't track open image handles. } diff --git a/packages/flutter/test/rendering/object_test.dart b/packages/flutter/test/rendering/object_test.dart index d6b8b23958..59fe13cfd1 100644 --- a/packages/flutter/test/rendering/object_test.dart +++ b/packages/flutter/test/rendering/object_test.dart @@ -144,6 +144,38 @@ void main() { return context.pushOpacity(offset, 100, painter, oldLayer: oldLayer as OpacityLayer?); }); }); + + test('RenderObject.dispose sets debugDisposed to true', () { + final TestRenderObject renderObject = TestRenderObject(); + expect(renderObject.debugDisposed, false); + renderObject.dispose(); + expect(renderObject.debugDisposed, true); + expect(renderObject.toStringShort(), contains('DISPOSED')); + }); + + test('RenderObject.dispose null the layer on repaint boundaries', () { + final TestRenderObject renderObject = TestRenderObject(allowPaintBounds: true); + // Force a layer to get set. + renderObject.isRepaintBoundary = true; + PaintingContext.repaintCompositedChild(renderObject, debugAlsoPaintedParent: true); + expect(renderObject.debugLayer, isA()); + + // Dispose with repaint boundary still being true. + renderObject.dispose(); + expect(renderObject.debugLayer, null); + }); + + test('RenderObject.dispose nulls the layer on non-repaint boundaries', () { + final TestRenderObject renderObject = TestRenderObject(allowPaintBounds: true); + // Force a layer to get set. + renderObject.isRepaintBoundary = true; + PaintingContext.repaintCompositedChild(renderObject, debugAlsoPaintedParent: true); + + // Dispose with repaint boundary being false. + renderObject.isRepaintBoundary = false; + renderObject.dispose(); + expect(renderObject.debugLayer, null); + }); } // Tests the create-update cycle by pumping two frames. The first frame has no @@ -188,12 +220,19 @@ class _TestCustomLayerBox extends RenderBox { class TestParentData extends ParentData with ContainerParentDataMixin { } class TestRenderObject extends RenderObject { + TestRenderObject({this.allowPaintBounds = false}); + + final bool allowPaintBounds; + + @override + bool isRepaintBoundary = false; + @override void debugAssertDoesMeetConstraints() { } @override Rect get paintBounds { - assert(false); // The test shouldn't call this. + assert(allowPaintBounds); // For some tests, this should not get called. return Rect.zero; } diff --git a/packages/flutter/test/widgets/framework_test.dart b/packages/flutter/test/widgets/framework_test.dart index 86067ff4fa..4262b65691 100644 --- a/packages/flutter/test/widgets/framework_test.dart +++ b/packages/flutter/test/widgets/framework_test.dart @@ -1620,6 +1620,18 @@ void main() { expect(states, ['deactivate', 'dispose']); }); + testWidgets('RenderObjectElement.unmount dispsoes of its renderObject', (WidgetTester tester) async { + await tester.pumpWidget(const Placeholder()); + final RenderObjectElement element = tester.allElements.whereType().first; + final RenderObject renderObject = element.renderObject; + expect(renderObject.debugDisposed, false); + + await tester.pumpWidget(Container()); + + expect(() => element.renderObject, throwsAssertionError); + expect(renderObject.debugDisposed, true); + }); + testWidgets('Getting the render object of an unmounted element throws', (WidgetTester tester) async { await tester.pumpWidget(const _StatefulLeaf()); final StatefulElement element = tester.element(find.byType(_StatefulLeaf));