Dispose render objects when owning element is unmounted. #82883

dnfield · 2021-05-19T04:01:31Z

When a RenderObjectElement is unmounted, the associated RenderObject should not be retained.

This patch adds a dispose method to RenderObject, which is called in RenderObjectElement.unmount. It also updates RenderImage to use this to dispose of its image, along with updating a single instance where we accessed a renderObject after calling super.unmount(). I've checked internal repo, and don't see any other instances that this should fail on.

This will help with #81514 (because in addition to nulling out a repaing boundary's layer, we'll want to dispose it and whatever pictures it holds) as well as making some cases of #79605 better.

/cc @jacob314

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

dnfield · 2021-05-19T04:04:39Z

Something is missing here. Need to add it back and make sure it still passes tests.

jacob314 · 2021-05-19T16:21:49Z

packages/flutter/lib/src/rendering/editable.dart

+  void dispose() {
+    _foregroundRenderObject?.dispose();
+    _backgroundRenderObject?.dispose();
+    super.dispose();


why not null out the references as well as disposing?

jacob314 · 2021-05-19T16:28:16Z

packages/flutter/lib/src/rendering/object.dart

@@ -2841,6 +2887,8 @@ abstract class RenderObject extends AbstractNode with DiagnosticableTreeMixin im
      header += ' NEEDS-COMPOSITING-BITS-UPDATE';
    if (!attached)
      header += ' DETACHED';
+    if (_debugDisposed)


maybe move this higher up in the list and short circuit all the other checks if the object is disposed? NEEDS-LAYOUT, NEEDS-PAINT, etc are probably all misleading if the object has been disposed.

jacob314 · 2021-05-19T16:32:08Z

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

  @override
-  RenderObject get renderObject => _renderObject!;
+  RenderObject get renderObject {
+    assert(_renderObject != null, 'Object disposed');


Object disposed --> RenderObject disposed? or '$runtimeType unmounted'?

Going with $runtimeType disposed`, since ROs don't have a notion of unmounting.

But isn't this the case where this RenderObjectElement has been unmounted not really the case that the RenderObject has been disposed? The RenderObject probably has been disposed given it is null.

Ahh I missed which thing I was in. I see what you're saying now. Will change to unmounted.

jacob314 · 2021-05-19T16:34:36Z

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

+        // Don't use the getter, the _renderObject may be null if the element
+        // is defunct, and the getter asserts that it is not null. This method
+        // does not guarantee a non-null return anyway.
+        result = element._renderObject;


can we instead return null immediately at the top of this method if the element is defunct?
Seems like a bug if you ever had non-defunct elements in a tree with defunct elements.

Done. We have other assertions around element that make sure you don't end up in that kind of state.

jacob314 · 2021-05-19T16:36:18Z

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

@@ -5768,6 +5778,11 @@ abstract class RenderObjectElement extends Element {

  @override
  void unmount() {
+    assert(
+      !renderObject.debugDisposed!,


nit: this will also blow up if there is a double unmount causing the _renderObject to already be null. do we care about that case?

You should not unmount an element more than once. If you do, asserts will fire in the unmount method itself.

goderbauer · 2021-05-19T16:38:28Z

packages/flutter/lib/src/rendering/object.dart

+    if (isRepaintBoundary) {
+      _layer = null;
+    }


Why only null this out if we are a repaint boundary? Shouldn't we release all resources?

What other resources does the top level RenderObject have?

Children will get dropped or disposed by elements holding them, unless we ourselves created the children, which I'll add to the top level documentation on RO.

What I meant is: "Why only null our _layer if it is a repaint boundary? If you're not a repaint boundary, who is responsible for freeing up this resource?

Ahhh, I guess we could null out our reference. In another branch, this code also calls dispose on thelayer, which we should only do if we're a repaint boundary. But yeah we can unconditionally null it out.

goderbauer · 2021-05-19T16:39:15Z

packages/flutter/lib/src/rendering/object.dart

+      });
+      return true;
+    }());
+    assert(() {


nit: to reduce clutter, could this just be part of the previous assert?

goderbauer · 2021-05-19T16:40:07Z

packages/flutter/lib/src/rendering/object.dart

+
+  /// Release any resources held by this render object.
+  ///
+  /// If this render object has created any children directly, it should dispose


should -> must dispose them before calling super.dispose() ? (since the method asserts that children are disposed?)

goderbauer · 2021-05-19T16:40:51Z

packages/flutter/lib/src/rendering/object.dart

+  /// If this render object has created any children directly, it should dispose
+  /// of those children in this method as well. However, it should not
+  /// dispose of children that were created by some other object, such as
+  /// a [RenderObjectElement].


Not sure I understand what is meant by this "However"?

Tried to clarify. Basically, only dispose ROs you created directly, which might include your children but does not automatically include them.

goderbauer · 2021-05-19T16:41:59Z

packages/flutter/lib/src/rendering/object.dart

+
+  bool _debugDisposed = false;
+
+  /// Release any resources held by this render object.


It's probably worth mentioning in the overall class doc on RenderObjects when to dispose them?

Added some docs at the top.

goderbauer · 2021-05-19T16:44:45Z

packages/flutter/test/rendering/image_test.dart

@@ -208,4 +208,19 @@ Future<void> 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 {


nit: RenderImage

goderbauer · 2021-05-19T16:50:27Z

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

@@ -5768,6 +5778,11 @@ abstract class RenderObjectElement extends Element {

  @override
  void unmount() {


Update the docs on unmount to indicate that that's also the place where resources may get released?

goderbauer · 2021-05-19T16:53:06Z

packages/flutter/lib/src/rendering/object.dart

+  /// Whether this has been disposed.
+  ///
+  /// If asserts are disabled, this property is always null.
+  bool? get debugDisposed {


Should there be more methods guarded with an assert on this? e.g. presumably, you shouldn't re-attach a RenderObject that has been disposed?

Added to attach, scheduleIniitialPaint, layout, markNeedsCompositingBitsUpdate, markNeedsPaint, replaceRootLayer, scheduleInitialSemantics, markNeedsSemanticsUpdate.

Other things are covered by _debugCanPerformMutations. I was surprised more of these weren't.

goderbauer · 2021-05-19T16:54:16Z

packages/flutter/lib/src/rendering/object.dart

+    }
+    assert(() {
+      visitChildren((RenderObject child) {
+        assert(child._debugDisposed);


nit: access via regular getter?

Done - I was avoiding the ! but it doesn't really matter much for asserts and will be less confusing if someone modifies the getter

goderbauer · 2021-05-19T16:55:54Z

packages/flutter/lib/src/rendering/object.dart

+
+  bool _debugDisposed = false;
+
+  /// Release any resources held by this render object.


We usually also say that the object is no longer usable after calling dispose.

dnfield · 2021-05-19T19:06:39Z

Added a fix for sectors as well, something in there needed to dispose children it created.

goderbauer · 2021-05-19T20:25:14Z

examples/layers/rendering/src/sector_layout.dart

+  @override
+  void dispose() {
+    visitChildren((RenderObject child) {
+      child.dispose();


This contradicts the documentation on dispose that you should only dispose children that you created yourself?

This particular render object does create its own children. We'd get an assertion error in the tests otherwise when the element tried to dispose them. It's just kind of confusing to me as to exactly where this happens...

I don't think it does create its own children. They are added from the outside here:

flutter/examples/layers/rendering/custom_coordinate_systems.dart

Line 13 in e7b7ebc

rootCircle.add(RenderSolidColor(const Color(0xFF00FFFF), desiredDeltaTheta: kTwoPi * 0.15));

is there a way to enumerate just the children that the selector created itself? Would help clarify why the children are disposed and avoid bugs if say someone added a subclass that added more children.

@goderbauer is right, but the test that's failing is actually failing from SectorApp rather than this file. I'm looking into it.

goderbauer · 2021-05-19T20:25:20Z

examples/layers/rendering/src/sector_layout.dart

@@ -461,6 +469,12 @@ class RenderBoxToRenderSectorAdapter extends RenderBox with RenderObjectWithChil
    this.child = child;
  }

+  @override
+  void dispose() {
+    child?.dispose();


goderbauer · 2021-05-19T20:33:06Z

packages/flutter/lib/src/rendering/object.dart

+    if (isRepaintBoundary) {
+      _layer = null;
+    }


What I meant is: "Why only null our _layer if it is a repaint boundary? If you're not a repaint boundary, who is responsible for freeing up this resource?

goderbauer · 2021-05-19T20:37:28Z

packages/flutter/lib/src/rendering/object.dart

+///
+/// [RenderObject]s are responsible for cleaning up any expensive resources
+/// they hold when [dispose] is called, such as [Picture] or [Image] objects.
+/// They must _only_ dispose of their [layer] object if [isRepaintBoundary] is


This is not super-clear to me. If I own the layer (e.g. because I set it in my paint method) why am I not in charge of releasing it as well? How can my parent still use it if I (the owner) are no longer arround?

@dnfield and I haven't finished this discussion, so I'm also interested in the ownership model here. I think there's shared ownership going on, but we lack means to express it. In a ref-counted language I'd be thinking about it as follows: a layer's ref count is bumped by its parent layer and its render object. The count can be in the following states:

2 - when a layer has both a parent layer and an owning RenderObject
1 - when a layer has either a parent layer or an owning RenderObject, but not both
0 - when nothing references the layer, this is when I'd clear the resources of this layer, this is also when recursively this layer's descendants' ref-counts are dropped by one and, possibly, reach 0, get their resources cleaned up, and so on.

This way we don't need to treat repaint boundaries as special. Any render object can own a sub-tree of layers, and the recursive disposal of render objects introduced by this PR makes sure we don't leak layers.

Yes, this doc is incorrect and I have updated it.

I may have to revisit this as I continue to work out the details about layer disposal, but I think it makes more sense now. PTAL.

Thanks. The doc comment is clear, but I'm not sure if what it describes is the right thing to do. Perhaps it is, and I just don't have a full picture in my head. Here's a couple of examples that I'm not sure are handled properly:

A repaint boundary has a RenderOpacity child. The repaint boundary disposes of its layer. Is the RepaintBoundary pointing to a disposed layer (i.e. a dangling pointer)?

A repaint boundary has another repaint boundary as a child. The parent disposes of its layer. Do we recursively delete pictures of the child repaint boundary? If not, how do we know to not delete them? If yes, how does the child repaint boundary know what its resources got cleaned-up and it needs to repaint?

I've changed this wording because it's no longer relevant to this patch. This patch doesn't add anything for disposing of layers, that will be coming in a separate patch.

To start to answer these questions though - if a repaint boundary disposes its layer, it means that repaint boundary is going way and won't be drawn again without getting completely recreated/repainted. And so if it has any child repaint boundaries that have not been moved to some active part of the tree, they will not get drawn again either unless recreated/repainted.

goderbauer · 2021-06-01T20:41:11Z

packages/flutter/lib/src/rendering/object.dart

+/// [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. By
+/// default, the base implementation of dispose will dispose of the [layer], and


I don't think subclasses can change this "default" behavior since they have to call super.dispose? Should we maybe just say that Layers created by the render object must be disposes manually unless they have been set to the layer property, which is automatically disposed?

Also: This patch doesn't contain any layer disposing mechanics (it just nulls out the property). Do you plan to add that logic to this PR or will that be added later? If it is latter, maybe we should just document what's implemented for now?

Reworded this a bit -layer.dispose will be in a separate patch, instead just talking about nulling the property.

goderbauer · 2021-06-01T20:42:58Z

packages/flutter/lib/src/rendering/object.dart

+  /// If [isRepaintBoundary] returns true, the layer tree rooted at this
+  /// object's layer will also dispose.


Is this still valid? Looks like _layer is now nulled regardless of isRepaintBoundary?

(Also note my comment above regarding that layers do not implement a disposal concept yet...)

Yes, removing this for now.

goderbauer · 2021-06-01T20:44:35Z

packages/flutter/lib/src/rendering/object.dart

+      visitChildren((RenderObject child) {
+        assert(
+          child.debugDisposed!,
+          '${child.runtimeType} (child of $runtimeType) must be disposed before calling super.dispose().',


Maybe add a note about this condition to the doc comment?

This corresponds to the doc comment

/// Implementations of this method must end with a call to the inherited /// method, as in `super.dispose()`.

goderbauer · 2021-06-01T20:46:38Z

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

@@ -6221,13 +6221,18 @@ class DefaultAssetBundle extends InheritedWidget {
 /// A given render object can be placed at most once in the widget tree. This
 /// widget enforces that restriction by keying itself using a [GlobalObjectKey]
 /// for the given render object.
+///
+/// This widget will call [RenderObject.dispose] on the [renderBox] when it is


It's somewhat surprising that the adaptor takes ownership of the passed in renderBox and disposes it, no?

I originally expected that you'd be in charge of doing that in onUnmount if you don't want to reuse it?

(Maybe also document this in the renderBox property?

My thought is to keep this as a less-breaking change. Otherwise users will have to update their code to use the unmount property now where they didn't before. Updated the renderBox property.

Perhaps at some point it would be worth changing this, but my assumption is that most users use this widget as if it will dispose the render object, rather than caching a render object to use repeatedly with this widget.

I see. I was thinking that maybe the default onUnmount handler would just dispose the renderBox, but if you don't want that you could provide a custom one and make it to do something else (e.g. keep the renderObject alive longer).

But maybe that can be added later. Maybe we should however specifically call out in the doc here that the provided renderBox becomes unusable when the adapter is unmounted from the tree?

Added another line explaining that.

goderbauer · 2021-06-01T20:48:46Z

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

+  /// Called when it is safe to dispose of the render object and its
+  /// descendants.
+  ///
+  /// Typically, this should dispose of any children attached to the


The two paragraphs sound a little contradictory: One the one hand its safe to dispose the render object (first paragraph) on the other hand it must not be disposed (second paragraph)?

You're right, I've updated this to try to clarify it and added links to some relevant areas.

goderbauer

LGTM

goderbauer · 2021-06-01T22:37:42Z

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

@@ -6221,13 +6221,18 @@ class DefaultAssetBundle extends InheritedWidget {
 /// A given render object can be placed at most once in the widget tree. This
 /// widget enforces that restriction by keying itself using a [GlobalObjectKey]
 /// for the given render object.
+///
+/// This widget will call [RenderObject.dispose] on the [renderBox] when it is


I see. I was thinking that maybe the default onUnmount handler would just dispose the renderBox, but if you don't want that you could provide a custom one and make it to do something else (e.g. keep the renderObject alive longer).

But maybe that can be added later. Maybe we should however specifically call out in the doc here that the provided renderBox becomes unusable when the adapter is unmounted from the tree?

jacob314

)" This reverts commit c75edc2.

)" (#83790) This reverts commit c75edc2.

…tter#82883)" (flutter#83790) This reverts commit 63c49c3.

)" (#83790) (#83920)

Dispose render objects when owning element is unmounted.

0ef8684

dnfield requested review from yjbanov and goderbauer May 19, 2021 04:01

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 19, 2021

google-cla bot added the cla: yes label May 19, 2021

dnfield marked this pull request as draft May 19, 2021 04:04

dnfield added 2 commits May 18, 2021 21:27

assert that children are disposed

c06c9a9

analysis

f00086e

dnfield marked this pull request as ready for review May 19, 2021 04:58

jacob314 reviewed May 19, 2021

View reviewed changes

goderbauer reviewed May 19, 2021

View reviewed changes

reviews

15e1743

dnfield added 2 commits May 19, 2021 12:47

unmounted -> defunct

4f4d439

disposed -> unmounted

19ecff3

goderbauer reviewed May 19, 2021

View reviewed changes

unconditionally null layer

fc51f6d

goderbauer reviewed May 19, 2021

View reviewed changes

dnfield added 5 commits May 20, 2021 10:17

fix test, update WidgetToRenderBoxAdapter

ccfe5e5

Merge remote-tracking branch 'upstream/master' into disp_ro

a64759f

update doc

35833e8

!

0ee8e1e

fix doc

10bdb0f

goderbauer reviewed Jun 1, 2021

View reviewed changes

docs/goderbauer review

e3a441a

goderbauer approved these changes Jun 1, 2021

View reviewed changes

jacob314 approved these changes Jun 1, 2021

View reviewed changes

one more doc

ba00aef

dnfield added the waiting for tree to go green label Jun 2, 2021

fluttergithubbot merged commit c75edc2 into flutter:master Jun 2, 2021

dnfield deleted the disp_ro branch June 2, 2021 05:36

renyou added a commit that referenced this pull request Jun 2, 2021

Revert "Dispose render objects when owning element is unmounted. (#82883

cfdec52

)" This reverts commit c75edc2.

renyou mentioned this pull request Jun 2, 2021

Revert "Dispose render objects when owning element is unmounted." #83790

Merged

renyou added a commit that referenced this pull request Jun 2, 2021

Revert "Dispose render objects when owning element is unmounted. (#82883

63c49c3

)" (#83790) This reverts commit c75edc2.

dnfield added a commit to dnfield/flutter that referenced this pull request Jun 3, 2021

Reland "Dispose render objects when owning element is unmounted. (flu…

ad1212e

…tter#82883)" (flutter#83790) This reverts commit 63c49c3.

fluttergithubbot pushed a commit that referenced this pull request Jun 8, 2021

Reland "Dispose render objects when owning element is unmounted. (#82883

c1fe2bd

)" (#83790) (#83920)


		bool _debugDisposed = false;

		/// Release any resources held by this render object.

		@@ -5768,6 +5778,11 @@ abstract class RenderObjectElement extends Element {

		@override
		void unmount() {

		/// If [isRepaintBoundary] returns true, the layer tree rooted at this
		/// object's layer will also dispose.

Dispose render objects when owning element is unmounted. #82883

Dispose render objects when owning element is unmounted. #82883

Conversation

dnfield commented May 19, 2021 • edited Loading

Pre-launch Checklist

dnfield commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 left a comment

Choose a reason for hiding this comment

dnfield commented May 19, 2021 •

edited

Loading