Fixing a memory leak in About box/dialog overlays #130842
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
polina-c
previously approved these changes
Jul 19, 2023
Contributor
|
@gspencergoog , can you check the fix is align with the recommendation: dart-lang/leak_tracker#97 |
polina-c
reviewed
Jul 19, 2023
Contributor
There was a problem hiding this comment.
I realized this is anti-pattern to release reference inside dispose. See the updated documentation.
Contributor
Author
There was a problem hiding this comment.
Which part of the documentation? I don't see anything about this pattern.
What is the recommended solution then?
Contributor
Author
There was a problem hiding this comment.
In this case, the owner of the Route (which is really a subclass: MaterialPageRoute) is the Navigator, where it exists in a list of routes until it is popped and disposed. There's nothing to null out on the Navigator, since it's stored in a list.
polina-c
approved these changes
Jul 19, 2023
3d50210 to
0f52015
Compare
Contributor
Author
|
@polina-c So, is this an OK solution now? The leak detector no longer shows a leak. If this solution is "hiding" the leak, then I'm not sure how to prevent the leak without nulling it out in dispose, and I'll need some guidance on how to do that. |
Contributor
How about removing nulling out for _restorationScopeId in disposal and see if leak still disappears? If it does not, I will help with investigation. |
Contributor
Author
Removing the nulling out of the Here are the logs: |
Contributor
|
Thanks! |
Contributor
Author
The leak does indeed seem to involve I tried converting it to nullable and nulling it in Any other ideas? |
Contributor
|
Will it make sense for lastAnnouncedPoppedNextRoute to be converted to WeakRef, so that it can be garbage collected when it is not needed to other objects? |
Contributor
Author
Yes, I think that would make sense. If I read the code right, it doesn't really own the object, it's just using it to compare to a later result. Converting it to a |
64637ed to
14b3f4b
Compare
Contributor
|
auto label is removed for flutter/flutter, pr: 130842, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
Contributor
|
That was exactly going to be my suggestion @jacob314 The flutter supported platforms doc indicates that our support starts from FF 99+. Internally some apps do support FF 85, so I think it would be fair to bump it at least to FF 85, but could be as far as FF 99. In the past, due to issues with Geckodriver, our team did something similar and bumped from FF 63 to FF 72. At that point in time FF 72 was the min version supported for flutter web, which is why I think we are using 72 on those tests at the moment. See cl/456802232. |
Contributor
|
Thanks. Bumped hello_flutter: cl/551925539 |
Contributor
|
CL got blocked by stacktrace parsing issue in Flutter: #131627 |
Contributor
|
Issue for debug mode in Firefox: #132439 |
17bec30 to
24e91c8
Compare
24e91c8 to
2002241
Compare
Contributor
|
auto label is removed for flutter/flutter/130842, due to - The status or check suite docs-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 16, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 16, 2023
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Aug 16, 2023
flutter/flutter@f0e7c51...2502b51 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from f186f1e9dc88 to 70b5700b79f6 (1 revision) (flutter/flutter#132655) 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from e8670f03a9b1 to f186f1e9dc88 (2 revisions) (flutter/flutter#132649) 2023-08-16 whesse@google.com Fix flutter_tools use of --local-engine-host (flutter/flutter#132648) 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7cc6a5832a0e to e8670f03a9b1 (3 revisions) (flutter/flutter#132623) 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from decaccfc421d to 7cc6a5832a0e (1 revision) (flutter/flutter#132621) 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 659cdfc5a568 to decaccfc421d (6 revisions) (flutter/flutter#132618) 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7409ce4ba0a8 to 659cdfc5a568 (1 revision) (flutter/flutter#132612) 2023-08-16 jonahwilliams@google.com Revert "Reorganize and clarify API doc generator" (flutter/flutter#132613) 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from a9da7212eacf to 7409ce4ba0a8 (5 revisions) (flutter/flutter#132609) 2023-08-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 22f03ffdc290 to a9da7212eacf (4 revisions) (flutter/flutter#132608) 2023-08-16 tessertaha@gmail.com [Reland] #131609 (flutter/flutter#132555) 2023-08-15 dkwingsmt@users.noreply.github.com Explain the keyboard manager protocol (flutter/flutter#132533) 2023-08-15 katelovett@google.com Fix extent for null returning builder in GridView (flutter/flutter#132511) 2023-08-15 gspencergoog@users.noreply.github.com Reorganize and clarify API doc generator (flutter/flutter#132353) 2023-08-15 gspencergoog@users.noreply.github.com Fixing a memory leak in About box/dialog overlays (flutter/flutter#130842) 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 bmparr@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
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix three memory leaks detected by
about_test.dart, but were really in theRouteandOverlayEntryclasses.Related Issues
Tests