Skip to content

Conversation

@ksokolovskyi
Copy link
Contributor

There were a couple of bugs in ListWheelScrollView:

  1. Internal fallback ScrollController was not disposed if no external controller was provided;
  2. There was a case when an external controller was still attached even if it was not passed into the widget anymore;
  3. ListWheelScrollView was able to dispose external controller in didUpdateWidget method.

Description

Tests

  • Updates controller hot swappable test to ensure that external controllers are detached and attached properly;
  • Adds controller can be reused test to ensure that the controller can be reused.

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Sep 14, 2023
@ksokolovskyi ksokolovskyi changed the title Fix leak in ListWheelScrollView Fix memory leak in ListWheelScrollView Sep 14, 2023
@ksokolovskyi
Copy link
Contributor Author

cc @polina-c

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM
Thank you for the contribution!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 14, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 14, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 14, 2023

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

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

@polina-c polina-c self-requested a review September 15, 2023 03:51
@polina-c polina-c merged commit 09acfe6 into flutter:master Sep 15, 2023
@ksokolovskyi
Copy link
Contributor Author

@polina-c @Piinks, thanks for the review!

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 15, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Sep 15, 2023
flutter/flutter@58ba6c2...72b69f9

2023-09-15 polinach@google.com Date picker dialog state should dispose members. (flutter/flutter#134804)
2023-09-15 engine-flutter-autoroll@skia.org Roll Packages from 275b76c to bc8c2f2 (7 revisions) (flutter/flutter#134823)
2023-09-15 leroux_bruno@yahoo.fr Fix navigation rail hover misplaced when direction is RTL and extended is true (flutter/flutter#134815)
2023-09-15 k_hayashi@yumemi.co.jp Applied the logo to the Discord badge. (flutter/flutter#134339)
2023-09-15 sokolovskyi.konstantin@gmail.com Fix memory leak in ListWheelScrollView (flutter/flutter#134732)
2023-09-15 zanderso@users.noreply.github.com Move two tests on Pixel 7 from staging to prod (flutter/flutter#134784)
2023-09-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 683bca53d4d7 to 45bc4307cda3 (2 revisions) (flutter/flutter#134789)
2023-09-14 pavel.mazhnik@gmail.com [web] provide serviceWorkerVersion to the getNewServiceWorker function (flutter/flutter#131240)
2023-09-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3a3a2807c3b6 to 683bca53d4d7 (3 revisions) (flutter/flutter#134778)
2023-09-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 035932d64017 to 3a3a2807c3b6 (5 revisions) (flutter/flutter#134769)
2023-09-14 47866232+chunhtai@users.noreply.github.com Allows page removal that contains Localhistoryentry (flutter/flutter#134757)
2023-09-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from e0b5b6c4eb76 to 035932d64017 (2 revisions) (flutter/flutter#134763)
2023-09-14 sokolovskyi.konstantin@gmail.com Cover more test/widgets tests with leak tracking #3 (flutter/flutter#134576)
2023-09-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2cd34d23c1a2 to e0b5b6c4eb76 (2 revisions) (flutter/flutter#134755)
2023-09-14 15619084+vashworth@users.noreply.github.com Set xcode version to older compatible version for microbenchmarks_ios_xcode_debug test (flutter/flutter#134693)
2023-09-14 82763757+NobodyForNothing@users.noreply.github.com Cover some Services tests with leak tracing (flutter/flutter#134381)
2023-09-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4160ebacdae2 to 2cd34d23c1a2 (18 revisions) (flutter/flutter#134749)
2023-09-14 github@alexv525.com � Setup color tween for `RefreshIndicator` in a better way (flutter/flutter#134492)
2023-09-14 leroux_bruno@yahoo.fr Fix NavigationRail hover misplaced when using large icons (flutter/flutter#134719)
2023-09-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from cd90cc8469fb to 4160ebacdae2 (5 revisions) (flutter/flutter#134695)
2023-09-14 30870216+gaaclarke@users.noreply.github.com Added a devicelab test for vulkan validation layers (flutter/flutter#134685)

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://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
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory Leak: ListWheelScrollView does not dispose fallback FixedExtentScrollController

3 participants