Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reland fix memory leaks for tab selector #147689

Conversation

ValentinVignal
Copy link
Contributor

Part of #141198

Relands #147403

We need to make sure the bot 'Google testing' passes.

Pre-launch Checklist

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 2, 2024
@ValentinVignal
Copy link
Contributor Author

cc @polina-c

@ValentinVignal ValentinVignal force-pushed the flutter/leak-tracking/Reland-fix-memories-for-tab-selectors branch from 1f74d7d to 9606fd3 Compare May 2, 2024 01:24
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label May 2, 2024
@polina-c polina-c requested a review from chingjun May 2, 2024 22:35
@polina-c
Copy link
Contributor

polina-c commented May 2, 2024

@chingjun , can you add your review to enable autosubmit to enable google testing?

Copy link
Contributor

@chingjun chingjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM to trigger Google Testing

@polina-c polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label May 3, 2024
Copy link
Contributor

auto-submit bot commented May 3, 2024

auto label is removed for flutter/flutter/147689, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 3, 2024
Copy link
Contributor

auto-submit bot commented May 3, 2024

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

@polina-c
Copy link
Contributor

polina-c commented May 3, 2024

Here is the error:

The following assertion was thrown building ...:
		dependOnInheritedWidgetOfExactType<_TabControllerScope>() or dependOnInheritedElement() was called
		before _TabPageSelectorState.initState() completed.
17:32:14.000	U	When an inherited widget changes, for example if the value of Theme.of() changes, its dependent
		widgets are rebuilt. If the dependent widget's reference to the inherited widget is in a constructor
		or an initState() method, then the rebuilt dependent widget will not reflect the changes in the
		inherited widget.
		Typically references to inherited widgets should occur in widget build() methods. Alternatively,
		initialization based on inherited widgets can be placed in the didChangeDependencies method, which
		is called after initState and whenever the dependencies change thereafter.
		The relevant error-causing widget was:
		...
		When the exception was thrown, this was the stack:
		#0      StatefulElement.dependOnInheritedElement.<anonymous closure> (third_party/dart/flutter/lib/src/widgets/framework.dart:5714:9)
		#1      StatefulElement.dependOnInheritedElement (third_party/dart/flutter/lib/src/widgets/framework.dart:5757:6)
		#2      Element.dependOnInheritedWidgetOfExactType (third_party/dart/flutter/lib/src/widgets/framework.dart:4773:14)
		#3      DefaultTabController.maybeOf (third_party/dart/flutter/lib/src/material/tab_controller.dart:409:20)
		#4      _TabPageSelectorState._tabController (third_party/dart/flutter/lib/src/material/tabs.dart:2266:84)
		#5      _TabPageSelectorState._setAnimation (third_party/dart/flutter/lib/src/material/tabs.dart:2315:15)
		#6      _TabPageSelectorState.initState (third_party/dart/flutter/lib/src/material/tabs.dart:2287:5)
		#7      StatefulElement._firstBuild (third_party/dart/flutter/lib/src/widgets/framework.dart:5612:55)
		#8      ComponentElement.mount (third_party/dart/flutter/lib/src/widgets/framework.dart:5457:5)

May be we need to make things resilient to usage after disposal?

@ValentinVignal
Copy link
Contributor Author

@polina-c I think I just need to not set the animation in the initState but in the didChangeDependencies only. Hopefully fix: Don't use dependOnInheritedWidgetOfExactType in the initState fixes it

@polina-c
Copy link
Contributor

polina-c commented May 3, 2024

Great!
Can you add test coverage that would catch the issue that you just fixed, in public testing, before internal testing?

@ValentinVignal
Copy link
Contributor Author

Great! Can you add test coverage that would catch the issue that you just fixed, in public testing, before internal testing?

Sure, the test I added in test: Add a test that uses DefaultTabController fails without my fix in fix: Don't use dependOnInheritedWidgetOfExactType in the initState

@polina-c polina-c merged commit 02d239a into flutter:master May 5, 2024
71 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 6, 2024
flutter/flutter@f1037a0...04424e1

2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from f2bfea5fdecd to a30ae7729c95 (1 revision) (flutter/flutter#147865)
2024-05-06 karel.klic@managry.com Fix Tooltip.decoration comment (flutter/flutter#147858)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2b828f368f6 to f2bfea5fdecd (1 revision) (flutter/flutter#147854)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from c73fd390d10e to e2b828f368f6 (1 revision) (flutter/flutter#147853)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 624dcb987f39 to c73fd390d10e (1 revision) (flutter/flutter#147852)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 900322f23375 to 624dcb987f39 (1 revision) (flutter/flutter#147845)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9080957cd6a0 to 900322f23375 (1 revision) (flutter/flutter#147842)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 66d08d55d788 to 9080957cd6a0 (1 revision) (flutter/flutter#147841)
2024-05-05 polinach@google.com Fix test. (flutter/flutter#147813)
2024-05-05 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `CupertinoSwitch` (flutter/flutter#147821)
2024-05-05 32538273+ValentinVignal@users.noreply.github.com Reland fix memory leaks for tab selector (flutter/flutter#147689)
2024-05-05 sokolovskyi.konstantin@gmail.com Fix memory leak in ExpansionTile. (flutter/flutter#147596)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 135acd5a689a to 66d08d55d788 (2 revisions) (flutter/flutter#147834)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from c937a02c6eb0 to 135acd5a689a (2 revisions) (flutter/flutter#147818)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1d484e57ce2c to c937a02c6eb0 (2 revisions) (flutter/flutter#147812)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2561636602ed to 1d484e57ce2c (1 revision) (flutter/flutter#147808)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 484574426120 to 2561636602ed (1 revision) (flutter/flutter#147805)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from d701f407c8ea to 484574426120 (1 revision) (flutter/flutter#147802)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 250536888a91 to d701f407c8ea (2 revisions) (flutter/flutter#147800)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3eadfd5284c0 to 250536888a91 (1 revision) (flutter/flutter#147796)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 837914f3788b to 3eadfd5284c0 (1 revision) (flutter/flutter#147791)
2024-05-03 102401667+Dimilkalathiya@users.noreply.github.com fixes `SearchAnchor` leak (flutter/flutter#147652)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8cce00433073 to 837914f3788b (1 revision) (flutter/flutter#147780)
2024-05-03 engine-flutter-autoroll@skia.org Roll Packages from aea93d2 to f4719ca (5 revisions) (flutter/flutter#147782)
2024-05-03 leroux_bruno@yahoo.fr Always relies on floatingLabelStyle when FloatingLabelBehavior.always (flutter/flutter#147374)

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 bmparr@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
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
…r#6674)

flutter/flutter@f1037a0...04424e1

2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from f2bfea5fdecd to a30ae7729c95 (1 revision) (flutter/flutter#147865)
2024-05-06 karel.klic@managry.com Fix Tooltip.decoration comment (flutter/flutter#147858)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2b828f368f6 to f2bfea5fdecd (1 revision) (flutter/flutter#147854)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from c73fd390d10e to e2b828f368f6 (1 revision) (flutter/flutter#147853)
2024-05-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 624dcb987f39 to c73fd390d10e (1 revision) (flutter/flutter#147852)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 900322f23375 to 624dcb987f39 (1 revision) (flutter/flutter#147845)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9080957cd6a0 to 900322f23375 (1 revision) (flutter/flutter#147842)
2024-05-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 66d08d55d788 to 9080957cd6a0 (1 revision) (flutter/flutter#147841)
2024-05-05 polinach@google.com Fix test. (flutter/flutter#147813)
2024-05-05 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `CupertinoSwitch` (flutter/flutter#147821)
2024-05-05 32538273+ValentinVignal@users.noreply.github.com Reland fix memory leaks for tab selector (flutter/flutter#147689)
2024-05-05 sokolovskyi.konstantin@gmail.com Fix memory leak in ExpansionTile. (flutter/flutter#147596)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 135acd5a689a to 66d08d55d788 (2 revisions) (flutter/flutter#147834)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from c937a02c6eb0 to 135acd5a689a (2 revisions) (flutter/flutter#147818)
2024-05-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1d484e57ce2c to c937a02c6eb0 (2 revisions) (flutter/flutter#147812)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2561636602ed to 1d484e57ce2c (1 revision) (flutter/flutter#147808)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 484574426120 to 2561636602ed (1 revision) (flutter/flutter#147805)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from d701f407c8ea to 484574426120 (1 revision) (flutter/flutter#147802)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 250536888a91 to d701f407c8ea (2 revisions) (flutter/flutter#147800)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3eadfd5284c0 to 250536888a91 (1 revision) (flutter/flutter#147796)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 837914f3788b to 3eadfd5284c0 (1 revision) (flutter/flutter#147791)
2024-05-03 102401667+Dimilkalathiya@users.noreply.github.com fixes `SearchAnchor` leak (flutter/flutter#147652)
2024-05-03 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8cce00433073 to 837914f3788b (1 revision) (flutter/flutter#147780)
2024-05-03 engine-flutter-autoroll@skia.org Roll Packages from aea93d2 to f4719ca (5 revisions) (flutter/flutter#147782)
2024-05-03 leroux_bruno@yahoo.fr Always relies on floatingLabelStyle when FloatingLabelBehavior.always (flutter/flutter#147374)

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 bmparr@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants