Re-land fix for not disposed TabController [prod-leak-fix] #146745

polina-c · 2024-04-14T22:37:29Z

goderbauer · 2024-04-16T17:12:37Z

packages/flutter/lib/src/animation/animation_controller.dart

@@ -843,6 +843,10 @@ class AnimationController extends Animation<double>
  }

  void _tick(Duration elapsed) {
+    if (_ticker == null) {


This looks fishy. Why is _tick still being called when the associated ticker has been disposed?

Because ticker uses SchedulerBinding.instance.scheduleFrameCallback to invoke tick.
There is number of cases of usage after disposal in Flutter, as it is validated only for small subset. It is not in scope of current cleanup to fix all these cases.
Current clean up makes sure to dispose all disposables, even when usage after disposal happens.

We can detect and clean up usage after disposal in future phases. The macro @disposable, that I am constructing to instrument disposables, will have this parameter.

Yeah, but it is unscheduled when the ticker is disposed. So, if _tick is still called after dispose there is something else that is not working as intended and - on the surface - this just seems to paper over that problem.

I could not repro the problem I was trying to solve with this. Removed.

goderbauer · 2024-04-16T17:14:51Z

packages/flutter/lib/src/material/tab_controller.dart

@@ -276,7 +281,9 @@ class TabController extends ChangeNotifier {

  @override
  void dispose() {
-    _animationController?.dispose();
+    if (_disposeAnimationController) {


This is also a strange pattern. Either the TabController owns the AnimationController (then it needs to be disposed here) or it doesn't own it and should never dispose it.

Added doc comment. Does it help?

The pattern is still strange. How about: copyWith will move the animationController to the new TabController and null out _animationController on the old one (this should be added to the documentation of _copyWith). Then, instead of doing an internal dispose call in _copyWith whoever calls _copyWith should follow up with a dispose call if they no longer need the old controller.

I removed the flag and nulled controller. Thanks for suggesting this.

It is strange for me though to declare that whoever called _copyWith must call dispose. It is easy to miss. If it is always this way, why _copyWith cannot do it?

goderbauer

LGTM

Fixes flutter#144910

…46745)

flutter/flutter@140edb9...77043ba 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from ffd3911b1ff7 to 79f49954cce8 (2 revisions) (flutter/flutter#147235) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7d9deb66bf7 to ffd3911b1ff7 (1 revision) (flutter/flutter#147227) 2024-04-23 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147220) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 075d834489d0 to c7d9deb66bf7 (2 revisions) (flutter/flutter#147215) 2024-04-23 leroux_bruno@yahoo.fr Mention visualDensity impact on ButtonStyle.padding documentation (flutter/flutter#147048) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9c0d24ff1cb6 to 075d834489d0 (1 revision) (flutter/flutter#147214) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `CupertinoTextMagnifier` (flutter/flutter#147208) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 004e98839ed7 to 9c0d24ff1cb6 (1 revision) (flutter/flutter#147211) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 79f753650c6e to 004e98839ed7 (1 revision) (flutter/flutter#147209) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from f8e373da5227 to 79f753650c6e (1 revision) (flutter/flutter#147206) 2024-04-23 102401667+Dimilkalathiya@users.noreply.github.com fixes cupertino page transition leak (flutter/flutter#147133) 2024-04-23 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `PopupMenu` (flutter/flutter#147174) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 62c9f17169cf to f8e373da5227 (2 revisions) (flutter/flutter#147205) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 33c828fb3ff5 to 62c9f17169cf (4 revisions) (flutter/flutter#147203) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.2 to 4.3.3 (flutter/flutter#147192) 2024-04-23 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.1 to 3.25.2 (flutter/flutter#147193) 2024-04-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from a4b71f02a1c7 to 33c828fb3ff5 (9 revisions) (flutter/flutter#147190) 2024-04-22 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#147094) 2024-04-22 polinach@google.com Re-land fix for not disposed TabController (flutter/flutter#146745) 2024-04-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 75ca2195c936 to a4b71f02a1c7 (1 revision) (flutter/flutter#147175) 2024-04-22 lamnhandev@gmail.com Update `examples/api` for android platform (flutter/flutter#147102) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

-

aefb038

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. labels Apr 14, 2024

polina-c mentioned this pull request Apr 14, 2024

_DefaultTabControllerState should dispose TabContoller instances. #144910

Closed

3 tasks

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Apr 14, 2024

-

2053b64

polina-c requested a review from goderbauer April 14, 2024 22:42

polina-c marked this pull request as draft April 14, 2024 22:42

polina-c removed the request for review from goderbauer April 14, 2024 22:42

polina-c added 2 commits April 15, 2024 09:09

Update tabs_test.dart

31168ca

Merge branch 'master' of github.com:flutter/flutter into tabs-fix

fffab1b

polina-c marked this pull request as ready for review April 15, 2024 16:11

Merge branch 'master' into tabs-fix

99552f2

polina-c requested a review from goderbauer April 16, 2024 01:25

goderbauer reviewed Apr 16, 2024

View reviewed changes

polina-c added 3 commits April 16, 2024 15:36

Update tab_controller.dart

5d12edd

Merge branch 'tabs-fix' of github.com:polina-c/flutter into tabs-fix

944bebb

Merge branch 'master' of github.com:flutter/flutter into tabs-fix

d304ed2

polina-c requested a review from goderbauer April 16, 2024 22:37

polina-c and others added 5 commits April 17, 2024 10:40

Merge branch 'master' into tabs-fix

0225f70

Merge branch 'master' into tabs-fix

d204be2

Update flutter_test_config.dart

9f255b4

Merge branch 'master' of github.com:flutter/flutter into tabs-fix

b41ad47

-

c7d87c9

github-actions bot removed the a: animation Animation APIs label Apr 20, 2024

Update tab_controller.dart

c570868

goderbauer approved these changes Apr 22, 2024

View reviewed changes

polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 22, 2024

auto-submit bot merged commit 1d7c680 into flutter:master Apr 22, 2024
75 checks passed

gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024

Re-land fix for not disposed TabController (flutter#146745)

abbb4e2

Fixes flutter#144910

engine-flutter-autoroll mentioned this pull request Apr 23, 2024

Roll Flutter from 140edb988312 to 77043bae1a5e (21 revisions) flutter/packages#6599

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 23, 2024

1d7c680 Re-land fix for not disposed TabController (flutter/flutter#1…

f7df073

…46745)

polina-c deleted the tabs-fix branch April 26, 2024 04:37

polina-c changed the title ~~Re-land fix for not disposed TabController~~ Re-land fix for not disposed TabController [prod-leak-fix] Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-land fix for not disposed TabController [prod-leak-fix] #146745

Re-land fix for not disposed TabController [prod-leak-fix] #146745

polina-c commented Apr 14, 2024 •

edited

Loading

goderbauer Apr 16, 2024

polina-c Apr 16, 2024 •

edited

Loading

goderbauer Apr 19, 2024

polina-c Apr 20, 2024

goderbauer Apr 16, 2024

polina-c Apr 16, 2024

goderbauer Apr 19, 2024

polina-c Apr 20, 2024 •

edited

Loading

goderbauer left a comment

Re-land fix for not disposed TabController [prod-leak-fix] #146745

Re-land fix for not disposed TabController [prod-leak-fix] #146745

Conversation

polina-c commented Apr 14, 2024 • edited Loading

goderbauer Apr 16, 2024

Choose a reason for hiding this comment

polina-c Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

goderbauer Apr 19, 2024

Choose a reason for hiding this comment

polina-c Apr 20, 2024

Choose a reason for hiding this comment

goderbauer Apr 16, 2024

Choose a reason for hiding this comment

polina-c Apr 16, 2024

Choose a reason for hiding this comment

goderbauer Apr 19, 2024

Choose a reason for hiding this comment

polina-c Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

polina-c commented Apr 14, 2024 •

edited

Loading

polina-c Apr 16, 2024 •

edited

Loading

polina-c Apr 20, 2024 •

edited

Loading