Skip to content

Conversation

@ksokolovskyi
Copy link
Contributor

@ksokolovskyi ksokolovskyi commented Sep 7, 2023

Description

  • Fixes a memory leak in _DraggableScrollableSheetState where _DraggableSheetExtent wasn't disposed when DraggableScrollableController wasn't provided.

Related Issues

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 7, 2023
@ksokolovskyi
Copy link
Contributor Author

cc @polina-c

@github-actions github-actions bot added the f: material design flutter/packages/flutter/material repository. label Sep 8, 2023
Copy link
Contributor

@polina-c polina-c left a comment

Choose a reason for hiding this comment

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

One comment then LGTM

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

auto-submit bot commented Sep 8, 2023

auto label is removed for flutter/flutter/134212, 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.

@override
void dispose() {
widget.controller?._detach(disposeExtent: true);
if (widget.controller != null) {
Copy link
Contributor

@polina-c polina-c Sep 8, 2023

Choose a reason for hiding this comment

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

Can you revert it to make more readable:

if (widget.controller == null) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@polina-c thanks a lot for the review!
Added the fix.

@HansMuller HansMuller requested a review from chunhtai September 8, 2023 21:35
@polina-c polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 8, 2023

auto label is removed for flutter/flutter/134212, 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 merged commit 5d5da38 into flutter:master Sep 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Sep 9, 2023
flutter/flutter@da676f7...7c28e8e

2023-09-09 [email protected] Roll Flutter Engine from 4e3231af6efc to d1913cb6a276 (1 revision) (flutter/flutter#134355)
2023-09-09 [email protected] Roll Flutter Engine from 348e3a376807 to 4e3231af6efc (1 revision) (flutter/flutter#134353)
2023-09-09 [email protected] Day picker should dispose created MaterialStatesController's. (flutter/flutter#133884)
2023-09-09 [email protected] Roll Flutter Engine from 00ef109b845e to 348e3a376807 (1 revision) (flutter/flutter#134349)
2023-09-09 [email protected] Roll Flutter Engine from 7af8a5d8d556 to 00ef109b845e (1 revision) (flutter/flutter#134336)
2023-09-09 [email protected] Roll Flutter Engine from 1f2da3d69da7 to 7af8a5d8d556 (2 revisions) (flutter/flutter#134332)
2023-09-09 [email protected] Roll Flutter Engine from 3a5f3ad1d054 to 1f2da3d69da7 (1 revision) (flutter/flutter#134328)
2023-09-09 [email protected] Roll Flutter Engine from d6aa2d9061c1 to 3a5f3ad1d054 (3 revisions) (flutter/flutter#134327)
2023-09-09 [email protected] Roll Flutter Engine from 66bec85d5005 to d6aa2d9061c1 (1 revision) (flutter/flutter#134324)
2023-09-08 [email protected] Roll Flutter Engine from 8d2892211366 to 66bec85d5005 (3 revisions) (flutter/flutter#134321)
2023-09-08 [email protected] Roll Flutter Engine from 2da727e23518 to 8d2892211366 (1 revision) (flutter/flutter#134316)
2023-09-08 [email protected] Fix memory leak in _DraggableScrollableSheetState (flutter/flutter#134212)
2023-09-08 [email protected] RestorationManager should dispatch creation in constructor. (flutter/flutter#133911)
2023-09-08 [email protected] Remove TextPainter migration flag from the framework (flutter/flutter#134274)
2023-09-08 [email protected] InputDecoration.error should activate error state (flutter/flutter#134001)
2023-09-08 [email protected] Roll Flutter Engine from b2cb1d271a88 to 2da727e23518 (1 revision) (flutter/flutter#134314)
2023-09-08 [email protected] Update chip docs to clarify how to specify a shape with no border & explain default values for Material 3 (flutter/flutter#134298)
2023-09-08 [email protected] Add ios analyzer command for universal links (flutter/flutter#134155)
2023-09-08 [email protected] Roll Flutter Engine from 47a79306eed3 to b2cb1d271a88 (5 revisions) (flutter/flutter#134313)
2023-09-08 [email protected] Roll Flutter Engine from 6d6b44886175 to 47a79306eed3 (2 revisions) (flutter/flutter#134310)

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
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: material design flutter/packages/flutter/material repository. 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: _DraggableScrollableSheetState doesn't dispose _DraggableSheetExtent _DraggableScrollableSheetState does not dispose extent.

2 participants