Skip to content

Fix SliverGrid garbage collection issue - #138915

Merged
auto-submit[bot] merged 3 commits into
flutter:masterfrom
chunhtai:issues/138749
Nov 23, 2023
Merged

Fix SliverGrid garbage collection issue#138915
auto-submit[bot] merged 3 commits into
flutter:masterfrom
chunhtai:issues/138749

Conversation

@chunhtai

@chunhtai chunhtai commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

The code doesn't consider indices of children may not be continuous after rebuild.

fixes #138749

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Nov 22, 2023
chunhtai
chunhtai commented Nov 22, 2023
View reviewed changes
Comment thread packages/flutter/lib/src/rendering/sliver_grid.dart Outdated

@chunhtai chunhtai Nov 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I copy over the code from RenderSliverFixedExtentBoxAdaptor

@goderbauer goderbauer Nov 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could the logic live in a common place (e.g. the RenderSliverMultiBoxAdaptor base class) and be shared between the two instead of duplicating it?

@chunhtai chunhtai Nov 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't want to expose these method because they are implementation detail. since this are only two instance, I prefer to keep it as is. We can revisit this if there are more use case for these logic

@goderbauer

goderbauer commented Nov 22, 2023

Copy link
Copy Markdown
Member

Would you mind reverting some of the dartfmt changes that snuck into this PR?

@chunhtai

chunhtai commented Nov 22, 2023

Copy link
Copy Markdown
Contributor Author

Would you mind reverting some of the dartfmt changes that snuck into this PR?

done.

goderbauer
goderbauer approved these changes Nov 22, 2023
View reviewed changes

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread packages/flutter/lib/src/rendering/sliver_grid.dart Outdated

@goderbauer goderbauer Nov 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could the logic live in a common place (e.g. the RenderSliverMultiBoxAdaptor base class) and be shared between the two instead of duplicating it?

Comment thread packages/flutter/test/widgets/slivers_test.dart Outdated
@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit

auto-submit Bot commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

auto label is removed for flutter/flutter/138915, due to - The status or check suite Mac_build_test flutter_gallery__transition_perf_e2e_ios has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Mac framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac build_tests_2_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux build_tests_2_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux build_tests_1_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows customer_testing has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac_arm64 flutter_packaging_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux flutter_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 22, 2023
@auto-submit
auto-submit Bot merged commit d92d2c1 into flutter:master Nov 23, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 24, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 24, 2023
@engine-flutter-autoroll
d92d2c1 Fix SliverGrid garbage collection issue (flutter/flutter#138915)
ec956df
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 24, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 24, 2023
@engine-flutter-autoroll
d92d2c1 Fix SliverGrid garbage collection issue (flutter/flutter#138915)
577a6c3
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Nov 24, 2023
@engine-flutter-autoroll
Roll Flutter from 106667e to cf05f3c (10 revisions) (#5474)
a20fc3a 
flutter/flutter@106667e...cf05f3c

2023-11-24 [email protected] Roll Flutter Engine from 7e640faa6adf to b1e0bf62fa35 (1 revision) (flutter/flutter#138979)
2023-11-24 [email protected] Roll Flutter Engine from d67ac2c561ca to 7e640faa6adf (2 revisions) (flutter/flutter#138969)
2023-11-24 [email protected] Roll Flutter Engine from 152790b5811b to d67ac2c561ca (1 revision) (flutter/flutter#138965)
2023-11-23 [email protected] Roll Flutter Engine from 4a344e050401 to 152790b5811b (1 revision) (flutter/flutter#138959)
2023-11-23 [email protected] Roll Flutter Engine from 6e71c3642abf to 4a344e050401 (1 revision) (flutter/flutter#138958)
2023-11-23 [email protected] Roll Flutter Engine from bec0dacaae41 to 6e71c3642abf (2 revisions) (flutter/flutter#138956)
2023-11-23 [email protected] [flutter_tools] Fix bad state future already completed in flutter logs (flutter/flutter#138517)
2023-11-23 [email protected] Fix SliverGrid garbage collection issue (flutter/flutter#138915)
2023-11-23 [email protected] Roll Flutter Engine from f331e0afaf2e to bec0dacaae41 (6 revisions) (flutter/flutter#138954)
2023-11-23 [email protected] Roll Packages from 2102327 to 6c5ad9e (1 revision) (flutter/flutter#138950)

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] 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@HansMuller HansMuller mentioned this pull request Dec 1, 2023
Closed
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
@chunhtai @caseycrogers
Fix SliverGrid garbage collection issue (flutter#138915)
a7fbe62 
The code doesn't consider indices of children may not be continuous after rebuild.

fixes flutter#138749
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
d92d2c1 Fix SliverGrid garbage collection issue (flutter/flutter#138915)
40a6bdb
creatorpiyush pushed a commit to creatorpiyush/packages that referenced this pull request Jun 10, 2026
@engine-flutter-autoroll
Roll Flutter from 106667e to cf05f3c (10 revisions) (flutter#5474)
4f7e5b8 
flutter/flutter@106667e...cf05f3c

2023-11-24 [email protected] Roll Flutter Engine from 7e640faa6adf to b1e0bf62fa35 (1 revision) (flutter/flutter#138979)
2023-11-24 [email protected] Roll Flutter Engine from d67ac2c561ca to 7e640faa6adf (2 revisions) (flutter/flutter#138969)
2023-11-24 [email protected] Roll Flutter Engine from 152790b5811b to d67ac2c561ca (1 revision) (flutter/flutter#138965)
2023-11-23 [email protected] Roll Flutter Engine from 4a344e050401 to 152790b5811b (1 revision) (flutter/flutter#138959)
2023-11-23 [email protected] Roll Flutter Engine from 6e71c3642abf to 4a344e050401 (1 revision) (flutter/flutter#138958)
2023-11-23 [email protected] Roll Flutter Engine from bec0dacaae41 to 6e71c3642abf (2 revisions) (flutter/flutter#138956)
2023-11-23 [email protected] [flutter_tools] Fix bad state future already completed in flutter logs (flutter/flutter#138517)
2023-11-23 [email protected] Fix SliverGrid garbage collection issue (flutter/flutter#138915)
2023-11-23 [email protected] Roll Flutter Engine from f331e0afaf2e to bec0dacaae41 (6 revisions) (flutter/flutter#138954)
2023-11-23 [email protected] Roll Packages from 2102327 to 6c5ad9e (1 revision) (flutter/flutter#138950)

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] 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
bisor0627 pushed a commit to bisor0627/packages that referenced this pull request Jun 19, 2026
@engine-flutter-autoroll
Roll Flutter from 106667e to cf05f3c (10 revisions) (flutter#5474)
29b9a0f 
flutter/flutter@106667e...cf05f3c

2023-11-24 [email protected] Roll Flutter Engine from 7e640faa6adf to b1e0bf62fa35 (1 revision) (flutter/flutter#138979)
2023-11-24 [email protected] Roll Flutter Engine from d67ac2c561ca to 7e640faa6adf (2 revisions) (flutter/flutter#138969)
2023-11-24 [email protected] Roll Flutter Engine from 152790b5811b to d67ac2c561ca (1 revision) (flutter/flutter#138965)
2023-11-23 [email protected] Roll Flutter Engine from 4a344e050401 to 152790b5811b (1 revision) (flutter/flutter#138959)
2023-11-23 [email protected] Roll Flutter Engine from 6e71c3642abf to 4a344e050401 (1 revision) (flutter/flutter#138958)
2023-11-23 [email protected] Roll Flutter Engine from bec0dacaae41 to 6e71c3642abf (2 revisions) (flutter/flutter#138956)
2023-11-23 [email protected] [flutter_tools] Fix bad state future already completed in flutter logs (flutter/flutter#138517)
2023-11-23 [email protected] Fix SliverGrid garbage collection issue (flutter/flutter#138915)
2023-11-23 [email protected] Roll Flutter Engine from f331e0afaf2e to bec0dacaae41 (6 revisions) (flutter/flutter#138954)
2023-11-23 [email protected] Roll Packages from 6d23bed to 398db1b (1 revision) (flutter/flutter#138950)

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] 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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
@Piinks Piinks Awaiting requested review from Piinks

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. 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.

Crash when using SliverGrid, findChildIndexCallback, a sparsely populated grid and updating the children

2 participants

@chunhtai @goderbauer