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

Fix a memory leak in AutomaticKeepAlive #124163

Merged
merged 3 commits into from Apr 6, 2023
Merged

Fix a memory leak in AutomaticKeepAlive #124163

merged 3 commits into from Apr 6, 2023

Conversation

GregoryConrad
Copy link
Contributor

Previously, if a KeepAliveHandle outlived an AutomaticKeepAlive (e.g., switching locations in the tree), it'd cause a memory leak by holding onto the AutomaticKeepAlive. This PR fixes that leak by calling removeListener once the listener is fired. See more: #123864 (comment)

This PR was spun off of #123864.

CC @dnfield @Piinks

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 4, 2023
@flutter-dashboard
Copy link

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.

@dnfield
Copy link
Contributor

dnfield commented Apr 4, 2023

Can you please add a test for this to make sure it doesn't get regressed?

@dnfield dnfield self-requested a review April 4, 2023 23:03
@GregoryConrad
Copy link
Contributor Author

@dnfield Should be all set!

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

@dnfield
Copy link
Contributor

dnfield commented Apr 5, 2023

Great! This will need a second review from a contributor to land.

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!

I checked the Google testing failure, it is not related to the PR. A rebase should set it straight.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@goderbauer
Copy link
Member

Did something go wrong with rebasing or updating this branch? Or is it really supposed to touch 158 files?

(removing the auto submit label for now)

@goderbauer goderbauer removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@GregoryConrad
Copy link
Contributor Author

GregoryConrad commented Apr 5, 2023

@goderbauer Looks like GitHub's online UI messed up the rebase. Same thing happened to me yesterday. I'll fix it

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@goderbauer
Copy link
Member

Thanks. Re-added the label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Apr 5, 2023

auto label is removed for flutter/flutter, pr: 124163, due to - The status or check suite Mac framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.

@GregoryConrad
Copy link
Contributor Author

Going to rebase to trigger a re-test. Shouldn't be related to this PR since everything passed before.

@Piinks
Copy link
Contributor

Piinks commented Apr 5, 2023

Next time, just send a ping. We and re-run most tests individually. The Google testing one is a special one. :)

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@GregoryConrad
Copy link
Contributor Author

GregoryConrad commented Apr 5, 2023

Ok! Would you please remove the autosubmit for a second? Looks like a rebase somewhere got messed up and changed the engine.version. I'll sort it out

@goderbauer goderbauer removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2023
@GregoryConrad
Copy link
Contributor Author

GregoryConrad commented Apr 5, 2023

Branch should be fixed & up to date, ready for auto-submit. Sorry about the hassle! Lesson learned: will not be using GitHub's online branch update feature in the future 🙃

@tirth-multipl
Copy link

Branch should be fixed & up to date, ready for auto-submit. Sorry about the hassle! Lesson learned: will not be using GitHub's online branch update feature in the future 🙃

you could use this hack to fix 'update rebase (from github) mess' - https://sirolad.medium.com/other-peoples-commit-in-my-pull-request-github-afc1fa7528ea

@GregoryConrad
Copy link
Contributor Author

@Piinks Hi! Is the google testing error still unrelated to this PR? Would a rebase help? Or should it be manually rerun?

Also, this PR should be all set for autosubmit!

Previously, if a KeepAliveHandle outlived an AutomaticKeepAlive,
it'd cause a memory leak by holding onto the AutomaticKeepAlive.
@Piinks
Copy link
Contributor

Piinks commented Apr 6, 2023

Hey @GregoryConrad sorry our CI has been having some troubles the last few days. I've rebased this to see if we can get this landed. There is another change that caused a lot of failures that has been getting caught up in other tests. Thanks for your patience.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 6, 2023
@tirth-multipl
Copy link

Hey @GregoryConrad sorry our CI has been having some troubles the last few days. I've rebased this to see if we can get this landed. There is another change that caused a lot of failures that has been getting caught up in other tests. Thanks for your patience.

why did number of checks reduced to just 9?!

@auto-submit auto-submit bot merged commit 4fc78b9 into flutter:master Apr 6, 2023
72 checks passed
@GregoryConrad GregoryConrad deleted the keep-alive-leak-fix branch April 6, 2023 18:36
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 7, 2023
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
Fix a memory leak in `AutomaticKeepAlive`
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants