- Fixes _DropdownMenuState leaking text controller [prod-leak-fix] #146571

Dimilkalathiya · 2024-04-10T13:26:14Z

Fixes state leaking text controller _DropdownMenuState is used after disposal that causes creation of new controller that is never disposed. #145622
Fixes particular test not handling text controller dispose

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

- Fixes particular test not handling text controller dispose

Dimilkalathiya · 2024-04-10T14:01:17Z

cc: @polina-c

polina-c · 2024-04-10T17:21:19Z

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

              currentHighlight = widget.enableSearch ? i : null;
              widget.onSelected?.call(entry.value);
+              _localTextEditingController?.dispose();
+              _localTextEditingController = null;


Why we need to dispose _localTextEditingController on every click? It will cause recreation of the controller.

This getter recreates controller if there is no controller:

TextEditingController get _textEditingController { return widget.controller ?? (_localTextEditingController ??= TextEditingController()); }

Yes, having getter which creates disposable class is quite troublesome even if we fix this issue if bounds to cause issue if new feature is introduced

i'll try to remove getter and minimise instantiation limited to initState and didUpdateWidget

polina-c · 2024-04-10T17:26:10Z

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

                    selection: TextSelection.collapsed(offset: entry.label.length),
                  );
                  widget.onSelected?.call(entry.value);
+                      _localTextEditingController?.dispose();


The same comment here.

It will cause recreate of controller on each onEditingComplete.

Dimilkalathiya · 2024-04-12T18:21:34Z

Hii @polina-c, Have updated PR as per discussed, let me know if this is good or you'd have some improvement in mind 🤔

polina-c · 2024-04-12T18:52:45Z

Thanks.
It would be great to understand what was the reason for original leak.

Originally the getter was creating local controller if one in widget is not provided.
If widget's controller was changed, local controller was disposed and nulled
Then local controller was disposed in dispose.
Why this was not working as expected?

If we understand what was wrong, we may find smaller code change for fix. Or, we may discover some bigger bug in Flutter Framework, that is always a good thing.

Dimilkalathiya · 2024-04-14T08:58:05Z

Hii @polina-c Yes, its nice idea to understand what is causing this bug, so i did some test and it turns out that MenuItemButton calls onSelected event after post frame callback.

which is there because of copy-paste issue #118731 so it can change focus.

which causes onSelected to called after dispose which creates TextController thanks to getter we had.

if we simply remove post frame callback function and call onSelected function directly it fixes issue but that mean it would reopen #118731 again.

some additional details that might help:-

If controller is passed from outside user it doesn't cause leak since we are not disposing controller provided by user
and since we are using null aware operator ? while using on onPressed on MenuItemButton on current PR it just ignores adding last event on TextEditingController and simply calls onSelected if user has not provided their own controller which make outside behaviour stay same and fixes issue, if user did have passed controller it will add event on both TextEditingController and call onSelected since we aren't disposing user provided controller.
i think MenuItemButton is only made and used for DropdownMenu only so it makes sense to have post frame callback to fix issue mentioned earlier and adding changes we made to in this PR would be good fix for leak.

but what do you think? we should merge this PR? or we should be changing MenuItemButton logic?

polina-c · 2024-04-14T14:58:39Z

Thank you for research!
It is not good that onSelected is called after dispose. And it is even worse that it triggers creation of a new controller.
This PR does not fix the first problem, but fixes the second. So, yes, let's merge it.

…lutter#146571)

Manual roll requested by tarrinneal@google.com flutter/flutter@53cba24...2e748e8 2024-04-15 nate.w5687@gmail.com Implementing control flow collections (flutter/flutter#146601) 2024-04-15 engine-flutter-autoroll@skia.org Roll Packages from 78f684c to 6698b2d (3 revisions) (flutter/flutter#146761) 2024-04-15 32538273+ValentinVignal@users.noreply.github.com test: Fix memory leak in transitions test (flutter/flutter#146747) 2024-04-15 leroux_bruno@yahoo.fr Fix filled text field active indicator overflows container bounds (flutter/flutter#146637) 2024-04-14 102401667+Dimilkalathiya@users.noreply.github.com - Fixes _DropdownMenuState leaking text controller (flutter/flutter#146571) 2024-04-13 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `FloatingActionButton` (flutter/flutter#146711) 2024-04-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0e56e3dffe43 to 1a13c7d1f40e (2 revisions) (flutter/flutter#146703) 2024-04-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146704) 2024-04-12 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.2 to 6.0.3 (flutter/flutter#146702) 2024-04-12 chingjun@google.com Avoid forwarding the data after socket is disconnected. (flutter/flutter#146665) 2024-04-12 christopherfujino@gmail.com [flutter_tools] Fix conductor for package args roll (flutter/flutter#146646) 2024-04-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6b37b170998e to 0e56e3dffe43 (4 revisions) (flutter/flutter#146698) 2024-04-12 katelovett@google.com Light sliver clean up before SliverTree (flutter/flutter#146696) 2024-04-12 leroux_bruno@yahoo.fr Fix label text color is wrong for a focused and hovered TextField (flutter/flutter#146572) 2024-04-12 31859944+LongCatIsLooong@users.noreply.github.com Fix `getOffsetForCaret` crash (flutter/flutter#146669) 2024-04-12 magder@google.com Update gen_keycodes templates (flutter/flutter#146481) 2024-04-12 jacksongardner@google.com Support `flutter run --wasm` and `flutter drive --wasm`. (flutter/flutter#146231) 2024-04-12 32538273+ValentinVignal@users.noreply.github.com Fix curved animation memory leak for scrollbar (flutter/flutter#146670) 2024-04-12 engine-flutter-autoroll@skia.org Roll Packages from e98839a to 78f684c (6 revisions) (flutter/flutter#146691) 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@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

) Manual roll requested by tarrinneal@google.com flutter/flutter@53cba24...2e748e8 2024-04-15 nate.w5687@gmail.com Implementing control flow collections (flutter/flutter#146601) 2024-04-15 engine-flutter-autoroll@skia.org Roll Packages from 78f684c to 6698b2d (3 revisions) (flutter/flutter#146761) 2024-04-15 32538273+ValentinVignal@users.noreply.github.com test: Fix memory leak in transitions test (flutter/flutter#146747) 2024-04-15 leroux_bruno@yahoo.fr Fix filled text field active indicator overflows container bounds (flutter/flutter#146637) 2024-04-14 102401667+Dimilkalathiya@users.noreply.github.com - Fixes _DropdownMenuState leaking text controller (flutter/flutter#146571) 2024-04-13 32538273+ValentinVignal@users.noreply.github.com Fix memory leaks in `FloatingActionButton` (flutter/flutter#146711) 2024-04-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0e56e3dffe43 to 1a13c7d1f40e (2 revisions) (flutter/flutter#146703) 2024-04-12 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#146704) 2024-04-12 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.2 to 6.0.3 (flutter/flutter#146702) 2024-04-12 chingjun@google.com Avoid forwarding the data after socket is disconnected. (flutter/flutter#146665) 2024-04-12 christopherfujino@gmail.com [flutter_tools] Fix conductor for package args roll (flutter/flutter#146646) 2024-04-12 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6b37b170998e to 0e56e3dffe43 (4 revisions) (flutter/flutter#146698) 2024-04-12 katelovett@google.com Light sliver clean up before SliverTree (flutter/flutter#146696) 2024-04-12 leroux_bruno@yahoo.fr Fix label text color is wrong for a focused and hovered TextField (flutter/flutter#146572) 2024-04-12 31859944+LongCatIsLooong@users.noreply.github.com Fix `getOffsetForCaret` crash (flutter/flutter#146669) 2024-04-12 magder@google.com Update gen_keycodes templates (flutter/flutter#146481) 2024-04-12 jacksongardner@google.com Support `flutter run --wasm` and `flutter drive --wasm`. (flutter/flutter#146231) 2024-04-12 32538273+ValentinVignal@users.noreply.github.com Fix curved animation memory leak for scrollbar (flutter/flutter#146670) 2024-04-12 engine-flutter-autoroll@skia.org Roll Packages from e98839a to 78f684c (6 revisions) (flutter/flutter#146691) 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@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

- Fixes state leaking text controller

7d59190

- Fixes particular test not handling text controller dispose

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 10, 2024

Dimilkalathiya changed the title ~~- Fixes state leaking text controller~~ - Fixes _DropdownMenuState leaking text controller Apr 10, 2024

polina-c reviewed Apr 10, 2024

View reviewed changes

Dimilkalathiya and others added 3 commits April 12, 2024 22:51

remove getter and minimize controller instantiation

53cc587

Merge branch 'master' into fix145622

9560f01

control flow statement fix

a388ec3

polina-c approved these changes Apr 14, 2024

View reviewed changes

polina-c merged commit 1002ce4 into flutter:master Apr 14, 2024

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

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

Roll Flutter from 53cba24de3a0 to 1002ce4b03d1 (15 revisions) flutter/packages#6517

Closed

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

1002ce4 - Fixes _DropdownMenuState leaking text controller (flutter/f…

4abdabd

…lutter#146571)

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

Roll Flutter from 53cba24de3a0 to 1002ce4b03d1 (15 revisions) flutter/packages#6519

Closed

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

1002ce4 - Fixes _DropdownMenuState leaking text controller (flutter/f…

de984bd

…lutter#146571)

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

Roll Flutter from 53cba24de3a0 to 63634c25ad41 (16 revisions) flutter/packages#6536

Closed

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

1002ce4 - Fixes _DropdownMenuState leaking text controller (flutter/f…

7acacae

…lutter#146571)

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

Roll Flutter from 53cba24de3a0 to 0099d1a96ac8 (17 revisions) flutter/packages#6538

Closed

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

1002ce4 - Fixes _DropdownMenuState leaking text controller (flutter/f…

ca5588b

…lutter#146571)

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

Roll Flutter from 53cba24de3a0 to 2e748e8598b3 (19 revisions) flutter/packages#6539

Closed

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

1002ce4 - Fixes _DropdownMenuState leaking text controller (flutter/f…

0a7dd64

…lutter#146571)

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

Manual roll Flutter from 53cba24de3a0 to 2e748e8598b3 (19 revisions) flutter/packages#6541

Merged

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

1002ce4 - Fixes _DropdownMenuState leaking text controller (flutter/f…

e4bbda2

…lutter#146571)

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

Roll Flutter from 53cba24de3a0 to 882bcbb9aa44 (20 revisions) flutter/packages#6542

Closed

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

1002ce4 - Fixes _DropdownMenuState leaking text controller (flutter/f…

d2c9278

…lutter#146571)

Dimilkalathiya deleted the fix145622 branch April 21, 2024 07:13

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

- Fixes _DropdownMenuState leaking text controller (flutter#146571)

4821178

HansMuller mentioned this pull request Apr 26, 2024

Flutter Framework Team: Notable Commits #121415

Closed

polina-c changed the title ~~- Fixes _DropdownMenuState leaking text controller~~ - Fixes _DropdownMenuState leaking text controller [prod-leak-fix] Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

- Fixes _DropdownMenuState leaking text controller [prod-leak-fix] #146571

- Fixes _DropdownMenuState leaking text controller [prod-leak-fix] #146571

Uh oh!

Dimilkalathiya commented Apr 10, 2024

Uh oh!

Dimilkalathiya commented Apr 10, 2024

Uh oh!

polina-c Apr 10, 2024

Uh oh!

polina-c Apr 10, 2024

Uh oh!

Dimilkalathiya Apr 12, 2024 •

edited

Loading

Uh oh!

polina-c Apr 10, 2024

Uh oh!

Dimilkalathiya commented Apr 12, 2024

Uh oh!

polina-c commented Apr 12, 2024

Uh oh!

Dimilkalathiya commented Apr 14, 2024 •

edited

Loading

Uh oh!

polina-c commented Apr 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

- Fixes _DropdownMenuState leaking text controller [prod-leak-fix] #146571

- Fixes _DropdownMenuState leaking text controller [prod-leak-fix] #146571

Uh oh!

Conversation

Dimilkalathiya commented Apr 10, 2024

Pre-launch Checklist

Uh oh!

Dimilkalathiya commented Apr 10, 2024

Uh oh!

polina-c Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

polina-c Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

Dimilkalathiya Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

Dimilkalathiya commented Apr 12, 2024

Uh oh!

polina-c commented Apr 12, 2024

Uh oh!

Dimilkalathiya commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polina-c commented Apr 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dimilkalathiya Apr 12, 2024 •

edited

Loading

Dimilkalathiya commented Apr 14, 2024 •

edited

Loading