Skip to content

Mark some leaks. #130470

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 17 commits into from
Jul 17, 2023
Merged

Mark some leaks. #130470

polina-c merged 17 commits into from
Jul 17, 2023

Conversation

@polina-c
Copy link
Contributor

@polina-c polina-c commented Jul 13, 2023

No description provided.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 13, 2023
@polina-c polina-c mentioned this pull request Jul 13, 2023
Closed
@polina-c polina-c marked this pull request as ready for review July 13, 2023 23:38
@polina-c polina-c requested a review from goderbauer July 13, 2023 23:38
@polina-c polina-c changed the title Mark leak in text_selection_theme_test.dart. Mark leak some leaks. Jul 14, 2023
@polina-c polina-c changed the title Mark leak some leaks. Mark some leaks. Jul 14, 2023
goderbauer
goderbauer reviewed Jul 14, 2023
View reviewed changes
packages/flutter/test/material/date_range_picker_test.dart Outdated
await tester.pumpAndSettle();
});
},
// TODO(polina-c): remove after widgets/app.dart/defaultActions stops holding objects.
Copy link
Member

@goderbauer goderbauer Jul 14, 2023

Choose a reason for hiding this comment

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

defaultActions is a static that is supposed to live for the duration of the application. So its expected to never be GCed and therefore is not a leak.

Copy link
Contributor Author

@polina-c polina-c Jul 14, 2023

Choose a reason for hiding this comment

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

defaultActions holds some disposed objects from GC, that is leak. Some object on retaining path should release the references.

Copy link
Member

@goderbauer goderbauer Jul 14, 2023

Choose a reason for hiding this comment

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

What's the disposed object? From what I can tell, the objects in the defaultActions map are not disposable.

Copy link
Contributor Author

@polina-c polina-c Jul 14, 2023

Choose a reason for hiding this comment

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

The disposed object is ValueNotifier<_OverlayEntryWidgetState?>.
Full retaining path is looong and is linked to the issue: #130354

Copy link
Contributor Author

@polina-c polina-c Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This PR just test covers three tests for leaks.
I suggest to merge this PR and move the discussion if the detected leak is true or false positive, to the issue.

@polina-c polina-c mentioned this pull request Jul 14, 2023
Closed
goderbauer
goderbauer approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@polina-c polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 17, 2023
@polina-c polina-c merged commit e337343 into flutter:master Jul 17, 2023
@polina-c polina-c deleted the text_selection_theme branch July 17, 2023 19:49
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
ed8e824
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
9f3cdec
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
c75d8dc
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
2a7a87c
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
2a6e79b
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
77eb493
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
dcf6220
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
7b39d80
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 19, 2023
@engine-flutter-autoroll
e337343 Mark some leaks. (flutter/flutter#130470)
c044f80
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
@polina-c @LouiseHsu
Mark some leaks. (flutter#130470)
8135dcc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material 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.

2 participants

@polina-c @goderbauer