Fix memory leak in RenderAnimatedSize #133653

ksokolovskyi · 2023-08-30T10:31:48Z

AnimationController and CurvedAnimation objects were not disposed in RenderAnimatedSize.

Description

Fixes Memory Leak: RenderAnimatedSize doesn't dispose AnimationController and CurvedAnimation #133903;
Adds the missing dispose() calls for AnimationController and CurvedAnimation in RenderAnimatedSize.

Tests

Updates animated_size_test.dart to test that AnimationController and CurvedAnimation are disposed after RenderSize disposal.

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 this PR is [test-exempt].
All existing and new tests are passing.

flutter-dashboard · 2023-08-30T10:31:52Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Piinks

Hi @ksokolovskyi thanks for contributing! Is there an open issue this PR would resolve? Also, the bot comment provides instructions for seeking test exemption.

ksokolovskyi · 2023-08-30T19:43:15Z

Hi @Piinks, thank you for the review!

I don't have a specific issue for this PR. I've found out that there is a memory leak in the RenderAnimatedSize and made a PR with a fix. Do I need to create an issue?
Asked for a test exemption in #hackers chat.

Hixie · 2023-08-30T19:50:11Z

test-exempt: will be tested later when we can catch these leaks

xu-baolin · 2023-08-31T09:24:58Z

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

@@ -351,6 +351,8 @@ class RenderAnimatedSize extends RenderAligningShiftedBox {
  @override
  void dispose() {
    _clipRectLayer.layer = null;
+    _controller.dispose();


Maybe we can add a test to check the state of controller after the widgets removed.

@xu-baolin thank you for the advice. I've added the test. Could you please take a look?

This change looks good to me, thanks for your nice job.
Could you file a new issue to track this problem and link it to this?

Yes, of course, I will create a new issue.

ksokolovskyi · 2023-09-02T10:07:06Z

@xu-baolin Here is the issue: #133903

xu-baolin · 2023-09-04T09:02:02Z

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

+  ///
+  /// The animation itself is exposed by the [animation] property.
+  @visibleForTesting
+  AnimationController get controller => _controller;


This is a public API not only for testing, right?

We can refer to the implementation of DragGestureRecognizer.debugLastPendingEventTimestamp and document that it is only available for testing.

Previously _controller and _animation were private because AnimatedSize is a kind of implicit animation, so users don't need to have access to the animation controller.
Are you sure that we need to expose them to the users and create additional getters for testing only?

I mean we should not add new APIs to AnimatedSize.

If I'm not mistaken, the @visibleForTesting annotation is not meant to be visible only to testing.

I got your point, thanks.
Could you please review the latest commit?

There is a problem with the CI infra, let's wait for it to be fixed.

xu-baolin

LGTM, Thanks for finding and fixing this bug.
@Piinks Hi, could you take a second review on this : )

XilaiZhang · 2023-09-08T20:48:24Z

hopefully people wouldn't mind me test some Google Testing logic on this PR (only involves a few extra clicks on "update branch" button) while Kate is OOO. choosing this PR since it is from an external contributor and has gotten approval from a flutter hacker member.

update: indeed found a bug and sent out cl/563861992 for review.

Piinks

This LGTM, thank you for contributing @ksokolovskyi!

auto-submit · 2023-09-11T19:29:19Z

auto label is removed for flutter/flutter/133653, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

…derAnimatedSize

XilaiZhang · 2023-09-11T20:33:37Z

Umm quick question, there seems to be 1k+ failures reported in Google Testing in previous runs from the commits, are those failures considered to be unrelated to this PR? thank you!

Piinks · 2023-09-11T20:36:11Z

are those failures considered to be unrelated to this PR?

I think so

Piinks · 2023-09-11T20:36:51Z

When I checked, the 1k+ failures looked like infra failures. Like the tests failed to build and run, which would not be due to this change.

XilaiZhang · 2023-09-11T20:43:55Z

ah cool, thanks for verifying!

Piinks · 2023-09-11T20:48:43Z

Looks like it merged without running Google testing this time @XilaiZhang

XilaiZhang · 2023-09-11T21:00:59Z

yeah sorry i was testing out a few things on this PR today and temporarily made it disappear (sent cl/564489978 as fix)

last time i checked this PR it created cl/564478862 and the results at test/564478862 was green so I merged it.

ksokolovskyi · 2023-09-11T21:03:32Z

@xu-baolin @Piinks @XilaiZhang thanks for the review!

`AnimationController` and `CurvedAnimation` objects were not disposed in `RenderAnimatedSize`. ### Description - Fixes flutter#133903; - Adds the missing `dispose()` calls for `AnimationController` and `CurvedAnimation` in `RenderAnimatedSize`. ### Tests - Updates `animated_size_test.dart` to test that `AnimationController` and `CurvedAnimation` are disposed after `RenderSize` disposal.

flutter/flutter@219efce...4e7a07a 2023-09-12 katelovett@google.com Remove chip tooltip deprecations (flutter/flutter#134486) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8a8ddaeecf8a to d4698c65aa8d (2 revisions) (flutter/flutter#134553) 2023-09-12 katelovett@google.com Remove deprecated TextSelectionOverlay.fadeDuration (flutter/flutter#134485) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 25be03186d82 to 8a8ddaeecf8a (1 revision) (flutter/flutter#134545) 2023-09-12 engine-flutter-autoroll@skia.org Roll Packages from ef0c65e to e04ba88 (5 revisions) (flutter/flutter#134546) 2023-09-12 47866232+chunhtai@users.noreply.github.com Revert "Adds a parent scope TraversalEdgeBehavior and fixes modal rouâ�¦ (flutter/flutter#134550) 2023-09-12 fluttergithubbot@gmail.com Marks Windows_android channels_integration_test_win to be unflaky (flutter/flutter#133129) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4091167e402d to 25be03186d82 (1 revision) (flutter/flutter#134536) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 54175da7ba90 to 4091167e402d (1 revision) (flutter/flutter#134532) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 85f3f02e5c37 to 54175da7ba90 (2 revisions) (flutter/flutter#134531) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1369ab35aaa7 to 85f3f02e5c37 (1 revision) (flutter/flutter#134519) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2f1efe5967f3 to 1369ab35aaa7 (1 revision) (flutter/flutter#134512) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4d8265cbc133 to 2f1efe5967f3 (3 revisions) (flutter/flutter#134511) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from ee7215553deb to 4d8265cbc133 (1 revision) (flutter/flutter#134506) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6ddee4423d2c to ee7215553deb (2 revisions) (flutter/flutter#134505) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from a98348946d61 to 6ddee4423d2c (1 revision) (flutter/flutter#134490) 2023-09-12 xubaolin@oppo.com [New feature] Allowing the `ListView` slivers to have different extents while still having scrolling performance (flutter/flutter#131393) 2023-09-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8389ad914b21 to a98348946d61 (4 revisions) (flutter/flutter#134487) 2023-09-12 kouhei.seino@woven-planet.global ScaleGestureRecognizer: make pointerCount public (flutter/flutter#127310) 2023-09-11 chinmay@blend.to Fix DataTable example not being scrollable (flutter/flutter#131556) 2023-09-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 06696e768c1b to 8389ad914b21 (2 revisions) (flutter/flutter#134469) 2023-09-11 christopherfujino@gmail.com [flutter_tools] disallow -O0 for flutter build web (flutter/flutter#134185) 2023-09-11 sokolovskyi.konstantin@gmail.com Cover focus tests with leak tracking (flutter/flutter#134457) 2023-09-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0774ddcda9b0 to 06696e768c1b (4 revisions) (flutter/flutter#134462) 2023-09-11 sokolovskyi.konstantin@gmail.com Fix memory leak in RenderAnimatedSize (flutter/flutter#133653) 2023-09-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from e42fd367adcb to 0774ddcda9b0 (1 revision) (flutter/flutter#134447) 2023-09-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from d593002b7159 to e42fd367adcb (1 revision) (flutter/flutter#134444) 2023-09-11 engine-flutter-autoroll@skia.org Roll Packages from aaae5ef to ef0c65e (6 revisions) (flutter/flutter#134445) 2023-09-11 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2b5961cbc40d to d593002b7159 (3 revisions) (flutter/flutter#134439) 2023-09-11 polinach@google.com Mark leak: instances of OpacityLayer, created by _RenderChip, should be disposed. (flutter/flutter#134395) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 30, 2023

Piinks reviewed Aug 30, 2023

View reviewed changes

xu-baolin reviewed Aug 31, 2023

View reviewed changes

ksokolovskyi requested review from xu-baolin and Piinks August 31, 2023 14:17

xu-baolin reviewed Sep 4, 2023

View reviewed changes

xu-baolin approved these changes Sep 5, 2023

View reviewed changes

Piinks approved these changes Sep 11, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023

ksokolovskyi added 3 commits September 11, 2023 15:04

Add disposal of an AnimationController and CurvedAnimation in the Ren…

db4c20a

…derAnimatedSize

Add test to ensure that animation and controller are disposed

15cebca

Add debugController and debugAnimation to RenderAnimatedSize

227a586

Piinks force-pushed the animated-size-leaks branch from b8cf1a2 to 227a586 Compare September 11, 2023 20:04

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023

XilaiZhang removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023

XilaiZhang added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023

auto-submit bot merged commit e7058f5 into flutter:master Sep 11, 2023
66 checks passed

polina-c mentioned this pull request Sep 11, 2023

Clean up notDisposed leaks in Flutter Framework, phase 1. dart-lang/leak_tracker#134

Closed

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter from 219efce7f16d to 127c76b91bef (22 revisions) flutter/packages#4905

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2023

e7058f568 Fix memory leak in RenderAnimatedSize (flutter/flutter#133653)

d2858d0

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter from 219efce7f16d to bdd96032a053 (24 revisions) flutter/packages#4906

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2023

e7058f568 Fix memory leak in RenderAnimatedSize (flutter/flutter#133653)

cd19d09

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter from 219efce7f16d to 5900c4baa751 (25 revisions) flutter/packages#4907

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2023

e7058f568 Fix memory leak in RenderAnimatedSize (flutter/flutter#133653)

9700faa

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter from 219efce7f16d to f9aef4f21e31 (26 revisions) flutter/packages#4908

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2023

e7058f568 Fix memory leak in RenderAnimatedSize (flutter/flutter#133653)

4f1c666

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter from 219efce7f16d to 90ae98f57642 (28 revisions) flutter/packages#4909

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2023

e7058f568 Fix memory leak in RenderAnimatedSize (flutter/flutter#133653)

1e5a8db

engine-flutter-autoroll mentioned this pull request Sep 12, 2023

Roll Flutter from 219efce7f16d to 4e7a07af882c (30 revisions) flutter/packages#4910

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2023

e7058f568 Fix memory leak in RenderAnimatedSize (flutter/flutter#133653)

15f3316

HansMuller mentioned this pull request Sep 15, 2023

Flutter Framework Team: Notable Commits #121415

Open

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023

e7058f568 Fix memory leak in RenderAnimatedSize (flutter/flutter#133653)

1c2f88d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in RenderAnimatedSize #133653

Fix memory leak in RenderAnimatedSize #133653

ksokolovskyi commented Aug 30, 2023 •

edited

flutter-dashboard bot commented Aug 30, 2023

Piinks left a comment

ksokolovskyi commented Aug 30, 2023 •

edited

Hixie commented Aug 30, 2023

xu-baolin Aug 31, 2023

ksokolovskyi Aug 31, 2023

xu-baolin Sep 1, 2023

ksokolovskyi Sep 2, 2023

ksokolovskyi commented Sep 2, 2023

xu-baolin Sep 4, 2023

ksokolovskyi Sep 4, 2023

xu-baolin Sep 4, 2023

ksokolovskyi Sep 4, 2023

xu-baolin Sep 5, 2023

xu-baolin left a comment

XilaiZhang commented Sep 8, 2023 •

edited

Piinks left a comment

auto-submit bot commented Sep 11, 2023

XilaiZhang commented Sep 11, 2023

Piinks commented Sep 11, 2023

Piinks commented Sep 11, 2023

XilaiZhang commented Sep 11, 2023

Piinks commented Sep 11, 2023

XilaiZhang commented Sep 11, 2023

ksokolovskyi commented Sep 11, 2023

Fix memory leak in RenderAnimatedSize #133653

Fix memory leak in RenderAnimatedSize #133653

Conversation

ksokolovskyi commented Aug 30, 2023 • edited

Description

Tests

Pre-launch Checklist

flutter-dashboard bot commented Aug 30, 2023

Piinks left a comment

Choose a reason for hiding this comment

ksokolovskyi commented Aug 30, 2023 • edited

Hixie commented Aug 30, 2023

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

ksokolovskyi commented Sep 2, 2023

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

xu-baolin left a comment

Choose a reason for hiding this comment

XilaiZhang commented Sep 8, 2023 • edited

Piinks left a comment

Choose a reason for hiding this comment

auto-submit bot commented Sep 11, 2023

XilaiZhang commented Sep 11, 2023

Piinks commented Sep 11, 2023

Piinks commented Sep 11, 2023

XilaiZhang commented Sep 11, 2023

Piinks commented Sep 11, 2023

XilaiZhang commented Sep 11, 2023

ksokolovskyi commented Sep 11, 2023

ksokolovskyi commented Aug 30, 2023 •

edited

ksokolovskyi commented Aug 30, 2023 •

edited

XilaiZhang commented Sep 8, 2023 •

edited