Dispose overlay entries by tgucio · Pull Request #132826 · flutter/flutter

tgucio · 2023-08-18T06:46:15Z

As OverlayEntry implements a Listenable that is e.g. subscribed to by the Navigator, it should be disposed when no longer used. This PR adds the missing dispose() calls in all the places I could find they were missing.

Test-exempt: adding missing dispose() methods without other functional changes

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-18T06:46:17Z

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.

navaronbracke · 2023-08-18T07:08:14Z

Would this unblock removing the leak tracking config overrides on some testWidgetsWithLeakTracking() usages?

This PR does not modify any tests, hence why I'm asking.

tgucio · 2023-08-18T13:17:29Z

@navaronbracke This might be a good question for @polina-c :)

justinmc

LGTM 👍

Thanks for cleaning this up!

justinmc · 2023-08-24T22:39:40Z

    );

-   _overlayEntry = OverlayEntry(
+    _overlayEntry = OverlayEntry(


Good catch on the space 👾

justinmc · 2023-08-24T22:40:58Z

@polina-c Is there a way to test these changes using your leak tracking tests? Or should it be test exempt?

polina-c · 2023-08-24T23:47:10Z

OverlayEntry is not instrumented to be tracked for leaks yet. So, not yet. Added it to list of classes to instrument.

Hixie · 2023-08-25T09:58:57Z

test-exempt: will be tracked

is there a bug# to follow that work?

flutter/flutter@deeb811...61d9f55 2023-08-25 [email protected] Update flutter packages to pick up latest vm_service (flutter/flutter#133335) 2023-08-25 [email protected] Roll Flutter Engine from 00f532dcaef4 to 33fca02451ef (1 revision) (flutter/flutter#133337) 2023-08-25 [email protected] Roll Flutter Engine from cb58abd77326 to 00f532dcaef4 (3 revisions) (flutter/flutter#133333) 2023-08-25 [email protected] Fix `PopupMenuItem` with a `ListTile` doesn't use the correct style. (flutter/flutter#133141) 2023-08-25 [email protected] Fix `Chip.shape`'s side is not used when provided in Material 3 (flutter/flutter#132941) 2023-08-25 [email protected] Dispose overlay entries (flutter/flutter#132826) 2023-08-25 [email protected] Roll Flutter Engine from 0f8962208a44 to cb58abd77326 (3 revisions) (flutter/flutter#133320) 2023-08-25 [email protected] Roll Flutter Engine from 09e620d26834 to 0f8962208a44 (2 revisions) (flutter/flutter#133309) 2023-08-25 [email protected] Roll Flutter Engine from 9bcefc74b772 to 09e620d26834 (1 revision) (flutter/flutter#133307) 2023-08-25 [email protected] Roll Flutter Engine from 1382d6d79408 to 9bcefc74b772 (2 revisions) (flutter/flutter#133305) 2023-08-25 [email protected] Allow passing verbose log from flutter daemon. (flutter/flutter#132828) 2023-08-25 [email protected] Roll Flutter Engine from b8ec4da8866c to 1382d6d79408 (1 revision) (flutter/flutter#133298) 2023-08-25 [email protected] Roll Flutter Engine from 965501a25d92 to b8ec4da8866c (11 revisions) (flutter/flutter#133296) 2023-08-24 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.5.3 to 3.6.0 (flutter/flutter#133281) 2023-08-24 [email protected] Update the tool to know about all our new platforms (flutter/flutter#132423) 2023-08-24 [email protected] l10n-related documentation improvements (flutter/flutter#133114) 2023-08-24 [email protected] Add hover duration for `Inkwell` widget (flutter/flutter#132176) 2023-08-24 [email protected] [web] benchmark the benchmark harness overhead (flutter/flutter#132999) 2023-08-24 [email protected] Users of ChangeNotifier should dispatch event of object creation in constructor. (flutter/flutter#133210) 2023-08-24 49699333+dependabot[bot]@users.noreply.github.com Bump activesupport from 6.1.7.3 to 6.1.7.6 in /dev/ci/mac (flutter/flutter#133225) 2023-08-24 [email protected] Remove `ImageProvider.load`, `DecoderCallback` and `PaintingBinding.instantiateImageCodec` (flutter/flutter#132679) 2023-08-24 [email protected] Roll Flutter Engine from aa98a9d2e86f to 965501a25d92 (24 revisions) (flutter/flutter#133272) 2023-08-24 [email protected] handle exceptions raised while searching for configured android studio (flutter/flutter#133180) 2023-08-24 [email protected] Fix mac tool_integration_tests with Xcode 15 (flutter/flutter#133217) 2023-08-24 [email protected] Roll Packages from 3060b1a to 383bffa (3 revisions) (flutter/flutter#133256) 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 [email protected],[email protected],[email protected] 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

flutter/flutter@deeb811...61d9f55 2023-08-25 [email protected] Update flutter packages to pick up latest vm_service (flutter/flutter#133335) 2023-08-25 [email protected] Roll Flutter Engine from 00f532dcaef4 to 33fca02451ef (1 revision) (flutter/flutter#133337) 2023-08-25 [email protected] Roll Flutter Engine from cb58abd77326 to 00f532dcaef4 (3 revisions) (flutter/flutter#133333) 2023-08-25 [email protected] Fix `PopupMenuItem` with a `ListTile` doesn't use the correct style. (flutter/flutter#133141) 2023-08-25 [email protected] Fix `Chip.shape`'s side is not used when provided in Material 3 (flutter/flutter#132941) 2023-08-25 [email protected] Dispose overlay entries (flutter/flutter#132826) 2023-08-25 [email protected] Roll Flutter Engine from 0f8962208a44 to cb58abd77326 (3 revisions) (flutter/flutter#133320) 2023-08-25 [email protected] Roll Flutter Engine from 09e620d26834 to 0f8962208a44 (2 revisions) (flutter/flutter#133309) 2023-08-25 [email protected] Roll Flutter Engine from 9bcefc74b772 to 09e620d26834 (1 revision) (flutter/flutter#133307) 2023-08-25 [email protected] Roll Flutter Engine from 1382d6d79408 to 9bcefc74b772 (2 revisions) (flutter/flutter#133305) 2023-08-25 [email protected] Allow passing verbose log from flutter daemon. (flutter/flutter#132828) 2023-08-25 [email protected] Roll Flutter Engine from b8ec4da8866c to 1382d6d79408 (1 revision) (flutter/flutter#133298) 2023-08-25 [email protected] Roll Flutter Engine from 965501a25d92 to b8ec4da8866c (11 revisions) (flutter/flutter#133296) 2023-08-24 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.5.3 to 3.6.0 (flutter/flutter#133281) 2023-08-24 [email protected] Update the tool to know about all our new platforms (flutter/flutter#132423) 2023-08-24 [email protected] l10n-related documentation improvements (flutter/flutter#133114) 2023-08-24 [email protected] Add hover duration for `Inkwell` widget (flutter/flutter#132176) 2023-08-24 [email protected] [web] benchmark the benchmark harness overhead (flutter/flutter#132999) 2023-08-24 [email protected] Users of ChangeNotifier should dispatch event of object creation in constructor. (flutter/flutter#133210) 2023-08-24 49699333+dependabot[bot]@users.noreply.github.com Bump activesupport from 6.1.7.3 to 6.1.7.6 in /dev/ci/mac (flutter/flutter#133225) 2023-08-24 [email protected] Remove `ImageProvider.load`, `DecoderCallback` and `PaintingBinding.instantiateImageCodec` (flutter/flutter#132679) 2023-08-24 [email protected] Roll Flutter Engine from aa98a9d2e86f to 965501a25d92 (24 revisions) (flutter/flutter#133272) 2023-08-24 [email protected] handle exceptions raised while searching for configured android studio (flutter/flutter#133180) 2023-08-24 [email protected] Fix mac tool_integration_tests with Xcode 15 (flutter/flutter#133217) 2023-08-24 [email protected] Roll Packages from 6f55c61 to abdcaab (3 revisions) (flutter/flutter#133256) 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 [email protected],[email protected],[email protected] 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

tgucio added 8 commits August 18, 2023 08:41

ContextMenu

821f1f1

RangeSlider

3b0d1a7

Autocomplete

b860e48

Hero

28ffe66

ReorderableList

6d59e47

Magnifier

0161f5c

Slider

28bc78a

Example

eb3b679

Merge branch 'flutter:master' into dispose-overlay-entries

3a65557

justinmc approved these changes Aug 24, 2023

View reviewed changes

Merge branch 'flutter:master' into dispose-overlay-entries

c1e90ab

tgucio merged commit 5c17a37 into flutter:master Aug 25, 2023

tgucio deleted the dispose-overlay-entries branch August 25, 2023 12:35

engine-flutter-autoroll mentioned this pull request Aug 25, 2023

Roll Flutter from deeb811ef0bb to ea0052193942 (22 revisions) flutter/packages#4772

Closed

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

5c17a37 Dispose overlay entries (flutter/flutter#132826)

a17690d

engine-flutter-autoroll mentioned this pull request Aug 25, 2023

Roll Flutter from deeb811ef0bb to db22617b7292 (23 revisions) flutter/packages#4773

Closed

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

5c17a37 Dispose overlay entries (flutter/flutter#132826)

11f1289

engine-flutter-autoroll mentioned this pull request Aug 25, 2023

Roll Flutter from deeb811ef0bb to 61d9f5566559 (25 revisions) flutter/packages#4774

Merged

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

5c17a37 Dispose overlay entries (flutter/flutter#132826)

8b1f4bb

HansMuller mentioned this pull request Aug 25, 2023

Flutter Framework Team: Notable Commits #121415

Closed

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

5c17a37 Dispose overlay entries (flutter/flutter#132826)

4f10bd6

Navigation Menu

Repository navigation

Dispose overlay entries - #132826

Dispose overlay entries#132826
tgucio merged 10 commits into
flutter:masterfrom
tgucio:dispose-overlay-entries

tgucio commented Aug 18, 2023

Uh oh!

flutter-dashboard Bot commented Aug 18, 2023

Uh oh!

navaronbracke commented Aug 18, 2023

Uh oh!

tgucio commented Aug 18, 2023

Uh oh!

justinmc left a comment

Uh oh!

justinmc Aug 24, 2023

Uh oh!

justinmc commented Aug 24, 2023

Uh oh!

polina-c commented Aug 24, 2023

Uh oh!

Hixie commented Aug 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

tgucio commented Aug 18, 2023

Pre-launch Checklist

Uh oh!

flutter-dashboard Bot commented Aug 18, 2023

Uh oh!

navaronbracke commented Aug 18, 2023

Uh oh!

tgucio commented Aug 18, 2023

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

justinmc commented Aug 24, 2023

Uh oh!

polina-c commented Aug 24, 2023

Uh oh!

Hixie commented Aug 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants