Skip to content

Fix memory leak in CupertinoActionSheet #134885

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.

Sign up for GitHub

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

Jump to bottom
Merged
polina-c merged 1 commit into from
Sep 16, 2023
Merged

Fix memory leak in CupertinoActionSheet #134885

polina-c merged 1 commit into from
Sep 16, 2023

Conversation

@ksokolovskyi
Copy link
Contributor

@ksokolovskyi ksokolovskyi commented Sep 16, 2023

This PR fixes memory leaks in the CupertinoActionSheet widget, which did not dispose ScrollControllers created internally.

Description

Tests

  • Updates cupertino/action_sheet_test.dart to use testWidgetsWithLeakTracking.

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: cupertino flutter/packages/flutter/cupertino repository labels Sep 16, 2023
@ksokolovskyi
Copy link
Contributor Author

ksokolovskyi commented Sep 16, 2023

cc @polina-c

@polina-c
Copy link
Contributor

polina-c commented Sep 16, 2023 •
edited
Loading

I think we duplicated each other: #13479
And the changes are almost identical.

I will merge yours, because in this case I do not need approver from Flutter team.

@polina-c polina-c merged commit 854bbb9 into flutter:master Sep 16, 2023
This was referenced Sep 16, 2023
Closed
Closed
@derdilla derdilla mentioned this pull request Sep 17, 2023
Closed
8 tasks
@ksokolovskyi
Copy link
Contributor Author

ksokolovskyi commented Sep 17, 2023

I think we duplicated each other: #13479 And the changes are almost identical.

I will merge yours, because in this case I do not need approver from Flutter team.

Thanks a lot for the review, and sorry for the duplication of your fix. Next time, I will be more attentive.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 17, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 17, 2023
@engine-flutter-autoroll
854bbb9 Fix memory leak in CupertinoActionSheet (flutter/flutter#134885)
db403db
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Sep 17, 2023
@engine-flutter-autoroll
Roll Flutter from e5e36ad to 1b18b13 (8 revisions) (#4940)
a26d8a8 
flutter/flutter@e5e36ad...1b18b13

2023-09-17 [email protected] Roll Flutter Engine from e07bb975ee85 to 57a4ff3c7ff8 (1 revision) (flutter/flutter#134895)
2023-09-17 [email protected] Roll Flutter Engine from 444689fd4060 to e07bb975ee85 (1 revision) (flutter/flutter#134889)
2023-09-16 [email protected] Roll Flutter Engine from 8c2203bb6c3b to 444689fd4060 (1 revision) (flutter/flutter#134886)
2023-09-16 [email protected] Fix memory leak in CupertinoActionSheet (flutter/flutter#134885)
2023-09-16 [email protected] Add disposal mechanism for created Layers to TestRecordingPaintingContext. (flutter/flutter#134768)
2023-09-16 [email protected] Manual roll Flutter Engine from 490925676b91 to 8c2203bb6c3b (6 revisions) (flutter/flutter#134882)
2023-09-16 [email protected] Manual roll Flutter Engine from cdcbdcc7ccba to 490925676b91 (5 revisions) (flutter/flutter#134879)
2023-09-16 [email protected] Manual roll Flutter Engine from 30b7e9ded7a0 to cdcbdcc7ccba (5 revisions) (flutter/flutter#134877)

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
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
@ksokolovskyi @Mairramer
Fix memory leak in CupertinoActionSheet (flutter#134885)
efb62a8
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
@engine-flutter-autoroll
854bbb9 Fix memory leak in CupertinoActionSheet (flutter/flutter#134885)
c00e862
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@polina-c polina-c polina-c approved these changes

Assignees

No one assigned

Labels

f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ScrollController not disposed in tests

2 participants

@ksokolovskyi @polina-c