AnimationController should dispatch creation in constructor. [prod-leak-fix] #134839

ksokolovskyi · 2023-09-15T20:10:07Z

Description

Adds AnimationController creation and disposal dispatching for memory leak tracking as part of Clean up memory leaks in Flutter Framework, phase 2. #137311;
Fixes Memory Leak: _MergeableMaterialState does not dispose AnimationController #134838.

Tests

Updates animation_controller_test.dart to test AnimationController object creation and disposal dispatching;
Fixes memory leak in _RenderCupertinoSlider;
Fixes memory leak in _CheckedPopupMenuItemState;
Fixes memory leak in ScaffoldMessengerState;
Fixes memory leak in _MergeableMaterialState;
Fixes a couple of leaks in tests.

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.

ksokolovskyi · 2023-09-15T20:42:59Z

cc @polina-c

polina-c · 2023-09-15T20:58:01Z

@ksokolovskyi ,
While AnimationController is disposable, tracking for it and cleanup is not included in first round of cleanup, because we want to keep scope of first iteration not very big.
I suggest to convert this PR to draft, and return to it after releasing leak_tracker MVP.

See details in the design doc: https://docs.google.com/document/d/1hlGY6HnsH5EJhpQDFm-_e-Sr9euQf6jE74TxkqDxhGY/edit?resourcekey=0--7J08lEADd6HpaTGC1B-Pw

polina-c · 2023-09-15T21:42:32Z

Also, there are thoughts to introduce mixin Disposable, that will implement the instrumentation, so there will not be need to duplicate the code.

ksokolovskyi · 2023-09-15T21:51:43Z

@ksokolovskyi , While AnimationController is disposable, tracking for it and cleanup is not included in first round of cleanup, because we want to keep scope of first iteration not very big. I suggest to convert this PR to draft, and return to it after releasing leak_tracker MVP.

Got it, I was not aware of the first-round plans. I have converted this PR to the draft.
Could you please mention this PR somewhere so we don't forget about it?

polina-c · 2023-09-16T17:55:55Z

@ksokolovskyi , While AnimationController is disposable, tracking for it and cleanup is not included in first round of cleanup, because we want to keep scope of first iteration not very big. I suggest to convert this PR to draft, and return to it after releasing leak_tracker MVP.

Got it, I was not aware of the first-round plans. I have converted this PR to the draft. Could you please mention this PR somewhere so we don't forget about it?

Sure: #134787

ksokolovskyi · 2023-09-16T18:35:11Z

@ksokolovskyi , While AnimationController is disposable, tracking for it and cleanup is not included in first round of cleanup, because we want to keep scope of first iteration not very big. I suggest to convert this PR to draft, and return to it after releasing leak_tracker MVP.

Got it, I was not aware of the first-round plans. I have converted this PR to the draft. Could you please mention this PR somewhere so we don't forget about it?

Sure: #134787

Thanks!

ksokolovskyi · 2023-10-26T16:13:53Z

@polina-c, am I supposed to close this PR considering #137309, or can I merge your changes later?

polina-c · 2023-10-26T17:28:22Z

@polina-c, am I supposed to close this PR considering #137309, or can I merge your changes later?

Actually, you can adjust it to match new structure. It may be faster that I will get code review for my PR.
Draft for new process: #137311

And this PR also contains good fixes.
It is up to you if you want to push this PR or copy the fixes to new one.

ksokolovskyi · 2023-10-26T17:41:00Z

@polina-c, am I supposed to close this PR considering #137309, or can I merge your changes later?

Actually, you can adjust it to match new structure. It may be faster that I will get code review for my PR. Draft for new process: #137311

And this PR also contains good fixes. It is up to you if you want to push this PR or copy the fixes to new one.

If you don't mind, I will update this PR as soon as possible. Ok?

polina-c · 2023-10-26T17:52:47Z

@polina-c, am I supposed to close this PR considering #137309, or can I merge your changes later?

Actually, you can adjust it to match new structure. It may be faster that I will get code review for my PR. Draft for new process: #137311
And this PR also contains good fixes. It is up to you if you want to push this PR or copy the fixes to new one.

If you don't mind, I will update this PR as soon as possible. Ok?

Sure, then I will close mine. Thank you!!!

ksokolovskyi · 2023-10-26T20:09:03Z

@polina-c could you please take a look at the latest commit? It seems like leak_tracker has false-positive results.
Maybe I am doing something wrong?

polina-c · 2023-10-26T20:30:03Z

@polina-c could you please take a look at the latest commit? It seems like leak_tracker has false-positive results. Maybe I am doing something wrong?

Yes, you removed ignore for allNotGCed: true, and now all notGCed leaks for all instrumented classes are visible. Return it.
It should be a separate effort to investigate and clean up notGCed leaks.

ksokolovskyi · 2023-10-26T20:31:56Z

@polina-c could you please take a look at the latest commit? It seems like leak_tracker has false-positive results. Maybe I am doing something wrong?

Yes, you removed ignore for allNotGCed: true, and now all not GCed leaks are visible. Return it.

But previously there were no leaks in all not marked testWidgetsWithLeakTracking.

polina-c · 2023-10-26T20:34:53Z

@polina-c could you please take a look at the latest commit? It seems like leak_tracker has false-positive results. Maybe I am doing something wrong?

Yes, you removed ignore for allNotGCed: true, and now all not GCed leaks are visible. Return it.

But previously there were no leaks in all not marked testWidgetsWithLeakTracking.

Sent you IM in twitter.

ksokolovskyi · 2023-10-26T22:06:21Z

@polina-c seems like all tests are fixed now :)

ksokolovskyi · 2023-10-27T05:56:31Z

@polina-c, thanks a lot for the review!

…lutter/flutter#134839)

Roll Flutter from c555599 to 5907c97 (45 revisions) flutter/flutter@c555599...5907c97 2023-10-27 [email protected] Add `isLogicalKeyPressed` to `KeyEvent` (flutter/flutter#136856) 2023-10-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Ensure `flutter build apk --release` optimizes+shrinks platform code" (flutter/flutter#137433) 2023-10-27 [email protected] Roll Flutter Engine from bedc49efc85c to a198ad4e740d (20 revisions) (flutter/flutter#137429) 2023-10-27 [email protected] Make `SemanticsNode.isMergedIntoParent` Readonly (flutter/flutter#137304) 2023-10-27 [email protected] Manual roll Flutter Engine from bedc49efc85c to 71e1a0430232 (16 revisions) (flutter/flutter#137422) 2023-10-27 [email protected] Roll Packages from fea24c5 to 2af6954 (5 revisions) (flutter/flutter#137421) 2023-10-27 [email protected] Run tests on either macOS 12 or 13 (flutter/flutter#137365) 2023-10-27 [email protected] Revert "Update `DataTable` test when data row is pressed for Material 3 (#137230)" (flutter/flutter#137407) 2023-10-27 [email protected] Revert "Reland - Update `OutlinedButton` tests for Material 3 (#136809) (#137247)" (flutter/flutter#137406) 2023-10-27 [email protected] Ensure `flutter build apk --release` optimizes+shrinks platform code (flutter/flutter#136880) 2023-10-27 [email protected] Update `DataTable` test when data row is pressed for Material 3 (flutter/flutter#137230) 2023-10-27 [email protected] Reland - Update `OutlinedButton` tests for Material 3 (#136809) (flutter/flutter#137247) 2023-10-27 [email protected] update asset manifest file name referenced in `WebServiceWorker` (flutter/flutter#135954) 2023-10-27 [email protected] give `throwsToolExit` a more useful description (flutter/flutter#136694) 2023-10-27 [email protected] Add ConstrainedLayoutBuilder.updateShouldRebuild() (flutter/flutter#136691) 2023-10-27 [email protected] Fix. typos (flutter/flutter#137325) 2023-10-27 [email protected] Roll Flutter Engine from 87f384c2d70b to bedc49efc85c (1 revision) (flutter/flutter#137382) 2023-10-26 [email protected] Fix Typos (flutter/flutter#137292) 2023-10-26 [email protected] AnimationController should dispatch creation in constructor. (flutter/flutter#134839) 2023-10-26 [email protected] Roll Flutter Engine from 9788bb9ff83e to 87f384c2d70b (4 revisions) (flutter/flutter#137380) 2023-10-26 [email protected] Roll Flutter Engine from ce1c1ee54107 to 9788bb9ff83e (3 revisions) (flutter/flutter#137373) 2023-10-26 [email protected] Marks Windows_arm64 windows_startup_test to be unflaky (flutter/flutter#137228) 2023-10-26 [email protected] Marks Windows_arm64 flutter_tool_startup__windows to be unflaky (flutter/flutter#137229) 2023-10-26 [email protected] Marks Windows_arm64 windows_home_scroll_perf__timeline_summary to be unflaky (flutter/flutter#137221) 2023-10-26 [email protected] Declare dependency on copyFlutterAssetsTask in bundleAarTask (flutter/flutter#137370) 2023-10-26 [email protected] Marks Windows_arm64 complex_layout_win_desktop__start_up to be unflaky (flutter/flutter#137225) 2023-10-26 [email protected] Roll Flutter Engine from 394744d2c4d0 to ce1c1ee54107 (2 revisions) (flutter/flutter#137367) 2023-10-26 [email protected] Marks Windows_arm64 hot_mode_dev_cycle_win_target__benchmark to be unflaky (flutter/flutter#137217) 2023-10-26 [email protected] Marks Windows_arm64 flutter_view_win_desktop__start_up to be unflaky (flutter/flutter#137226) 2023-10-26 [email protected] Marks Windows_arm64 platform_view_win_desktop__start_up to be unflaky (flutter/flutter#137227) 2023-10-26 [email protected] Marks Windows_arm64 flutter_gallery_win_desktop__start_up to be unflaky (flutter/flutter#137224) 2023-10-26 [email protected] Marks Windows_arm64 hello_world_win_desktop__compile to be unflaky (flutter/flutter#137222) 2023-10-26 [email protected] Marks Windows_arm64 flutter_gallery_win_desktop__compile to be unflaky (flutter/flutter#137223) 2023-10-26 [email protected] Marks Windows_arm64 run_release_test_windows to be unflaky (flutter/flutter#137220) 2023-10-26 [email protected] Fix dislocated doc and comment on ThemeData localize cache (flutter/flutter#137315) 2023-10-26 [email protected] Marks Windows_arm64 platform_channel_sample_test_windows to be unflaky (flutter/flutter#137218) 2023-10-26 [email protected] Roll Flutter Engine from 7c5c8f587992 to 394744d2c4d0 (1 revision) (flutter/flutter#137364) 2023-10-26 [email protected] Roll Flutter Engine from 9363fe6ba503 to 7c5c8f587992 (3 revisions) (flutter/flutter#137363) 2023-10-26 [email protected] Roll Flutter Engine from 542f8bc4c019 to 9363fe6ba503 (2 revisions) (flutter/flutter#137354) 2023-10-26 [email protected] Unified analytics events for doctor validators (flutter/flutter#136647) 2023-10-26 [email protected] Roll Flutter Engine from d8132d5070bf to 542f8bc4c019 (2 revisions) (flutter/flutter#137349) 2023-10-26 [email protected] Marks Windows_arm64 run_debug_test_windows to be unflaky (flutter/flutter#137219) 2023-10-26 [email protected] Roll Flutter Engine from 0a6253dbfafd to d8132d5070bf (1 revision) (flutter/flutter#137347) 2023-10-26 [email protected] Roll Flutter Engine from 5da115661f01 to 0a6253dbfafd (1 revision) (flutter/flutter#137341) 2023-10-26 [email protected] Roll Flutter Engine from 61ae5ef94e9c to 5da115661f01 (1 revision) (flutter/flutter#137340) ...

ksokolovskyi added 6 commits September 15, 2023 21:54

Instrumented AnimationController

d11e6df

Fix leak in _RenderCupertinoSlider

937b608

Fix leak in _CheckedPopupMenuItemState

09942ae

Fix leak in ScaffoldMessengerState

a1c2cb1

Mark leak in _MergeableMaterialState

ea10d8f

Fix leaks in tests

7272eeb

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Sep 15, 2023

ksokolovskyi mentioned this pull request Sep 15, 2023

Memory Leak: _MergeableMaterialState does not dispose AnimationController #134838

Closed

2 tasks

ksokolovskyi marked this pull request as draft September 15, 2023 21:49

polina-c mentioned this pull request Sep 16, 2023

Make sure there are no memory leaks in Flutter Framework. #134787

Closed

5 tasks

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Sep 20, 2023

polina-c mentioned this pull request Oct 26, 2023

Instrument more disposables. #137309

Merged

ksokolovskyi added 2 commits October 26, 2023 20:51

Merge branch 'master' into animation-controller-instrumentation

8f7d707

Integrated latest leak_tracker updates

b3dac85

ksokolovskyi added 3 commits October 26, 2023 22:41

Fix mistake

52fb426

Fix _MergeableMaterialState leak

bcde6f2

Fix AnimationController leaks

271b12f

ksokolovskyi marked this pull request as ready for review October 26, 2023 22:03

polina-c approved these changes Oct 26, 2023

View reviewed changes

polina-c merged commit aeb500a into flutter:master Oct 26, 2023

engine-flutter-autoroll mentioned this pull request Oct 27, 2023

Roll Flutter from c555599fbb4a to eba149fcc3dc (38 revisions) flutter/packages#5244

Closed

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

aeb500a AnimationController should dispatch creation in constructor. (f…

1b49890

…lutter/flutter#134839)

engine-flutter-autoroll mentioned this pull request Oct 27, 2023

Roll Flutter from c555599fbb4a to e75f85bffd46 (39 revisions) flutter/packages#5246

Closed

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

aeb500a AnimationController should dispatch creation in constructor. (f…

ad3f689

…lutter/flutter#134839)

engine-flutter-autoroll mentioned this pull request Oct 27, 2023

Roll Flutter from c555599fbb4a to 8cfa9c478c76 (40 revisions) flutter/packages#5247

Closed

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

aeb500a AnimationController should dispatch creation in constructor. (f…

134023e

…lutter/flutter#134839)

engine-flutter-autoroll mentioned this pull request Oct 27, 2023

Roll Flutter from c555599fbb4a to 3ef84471c188 (41 revisions) flutter/packages#5249

Closed

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

aeb500a AnimationController should dispatch creation in constructor. (f…

e6b34e5

…lutter/flutter#134839)

engine-flutter-autoroll mentioned this pull request Oct 27, 2023

Roll Flutter from c555599fbb4a to 4cae1af41d13 (44 revisions) flutter/packages#5251

Closed

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

aeb500a AnimationController should dispatch creation in constructor. (f…

543923e

…lutter/flutter#134839)

engine-flutter-autoroll mentioned this pull request Oct 27, 2023

Roll Flutter from c555599fbb4a to 5907c9757e3f (45 revisions) flutter/packages#5252

Merged

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

aeb500a AnimationController should dispatch creation in constructor. (f…

5f2febb

…lutter/flutter#134839)

HansMuller mentioned this pull request Nov 1, 2023

Flutter Framework Team: Notable Commits #121415

Closed

polina-c mentioned this pull request Jan 14, 2024

Clean up memory leaks in Flutter Framework, phase 2. #137311

Closed

28 tasks

polina-c changed the title ~~AnimationController should dispatch creation in constructor.~~ AnimationController should dispatch creation in constructor. [prod-leak-fix] Aug 29, 2024

AnimationController should dispatch creation in constructor. [prod-leak-fix] #134839

AnimationController should dispatch creation in constructor. [prod-leak-fix] #134839

Uh oh!

Conversation

ksokolovskyi commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

Pre-launch Checklist

Uh oh!

ksokolovskyi commented Sep 15, 2023

Uh oh!

polina-c commented Sep 15, 2023

Uh oh!

polina-c commented Sep 15, 2023

Uh oh!

ksokolovskyi commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polina-c commented Sep 16, 2023

Uh oh!

ksokolovskyi commented Sep 16, 2023

Uh oh!

ksokolovskyi commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polina-c commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksokolovskyi commented Oct 26, 2023

Uh oh!

polina-c commented Oct 26, 2023

Uh oh!

ksokolovskyi commented Oct 26, 2023

Uh oh!

polina-c commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksokolovskyi commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polina-c commented Oct 26, 2023

Uh oh!

ksokolovskyi commented Oct 26, 2023

Uh oh!

ksokolovskyi commented Oct 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ksokolovskyi commented Sep 15, 2023 •

edited

Loading

ksokolovskyi commented Sep 15, 2023 •

edited

Loading

ksokolovskyi commented Oct 26, 2023 •

edited

Loading

polina-c commented Oct 26, 2023 •

edited

Loading

polina-c commented Oct 26, 2023 •

edited

Loading

ksokolovskyi commented Oct 26, 2023 •

edited

Loading