Modify the updateChildren method deep copy _children #120773

yiiim · 2023-02-15T09:10:27Z

This PR modifies newChildren to deep copy _children in updateChildren method.

Fixes #120762

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.

flutter-dashboard · 2023-02-15T09:10:30Z

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.

xu-baolin

Changes looks good with some small nits.
Thank you for your great work!

packages/flutter/test/widgets/framework_test.dart

xu-baolin · 2023-02-16T07:28:19Z

packages/flutter/lib/src/widgets/framework.dart

@@ -5945,7 +5945,7 @@ abstract class RenderObjectElement extends Element {
    int oldChildrenBottom = oldChildren.length - 1;

    final List<Element> newChildren = oldChildren.length == newWidgets.length ?
-        oldChildren : List<Element>.filled(newWidgets.length, _NullElement.instance);
+        <Element>[...oldChildren] : List<Element>.filled(newWidgets.length, _NullElement.instance);


We indeed should not modify the input parameters directly, which is dangerous.

yiiim · 2023-02-16T07:54:47Z

nits have been fixed

xu-baolin

LGTM, Thanks!

auto-submit · 2023-02-16T08:56:19Z

auto label is removed for flutter/flutter, pr: 120773, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2023-02-16T08:56:20Z

auto label is removed for flutter/flutter, pr: 120773, due to Validations Fail.

xu-baolin · 2023-02-16T09:19:56Z

@goderbauer Hi could you take a second review for this patch? :)

packages/flutter/lib/src/widgets/framework.dart

goderbauer

LGTM

…er/flutter#120773)

* df98689c9 2be7253c9 Roll Fuchsia Linux SDK from q7u2WyX2BSRBIzyTW... to yT4JLKTCWWwbRwB0l... (flutter/engine#39679) (flutter/flutter#120898) * cacef57b6 [flutter_tools] Skip over "Resolving dependencies..." text in integration tests (flutter/flutter#120077) * 34102ca3b Migrate channels to pkg:integration _test (flutter/flutter#120833) * df13ea248 Roll Flutter Engine from 2be7253c9b10 to e4cb80e22ee1 (2 revisions) (flutter/flutter#120903) * a2e65f7c3 Roll Flutter Engine from e4cb80e22ee1 to 4a90fbcd6901 (2 revisions) (flutter/flutter#120911) * e00241a06 Enable Windows plugin tests (flutter/flutter#119345) * 09ad9f3cd Document ScrollPhysics invariant requiring ballistic motion (flutter/flutter#120400) * 6029de2fb Update switch template (flutter/flutter#120919) * 229d70ea3 Roll Flutter Engine from 4a90fbcd6901 to bddfc1c4dcaa (5 revisions) (flutter/flutter#120920) * 206c6ae99 roll packages (flutter/flutter#120922) * 9fcaaebb5 Roll Flutter Engine from bddfc1c4dcaa to 6602fc753525 (3 revisions) (flutter/flutter#120928) * 00c0a07fa Increase Linux docs_test timeout (flutter/flutter#120899) * e29a79975 946b29198 [dart:ui] Introduce `PlatformDispatcher.implicitView` (flutter/engine#39553) (flutter/flutter#120939) * 081cd5776 650db7a72 [macOS] Eliminate mirrors support (flutter/engine#39694) (flutter/flutter#120943) * 875e48c69 52a4fb4c5 Roll Skia from b1800a8b9595 to d0df677ffd5e (13 revisions) (flutter/engine#39699) (flutter/flutter#120947) * 78d058f46 6e92c0c28 Roll Fuchsia Mac SDK from xl9Y8o-9FDyvPogki... to haDvcC5VzWVdQs9Rs... (flutter/engine#39700) (flutter/flutter#120950) * 298d8c76b Revert "Remove references to Observatory (#118577)" (flutter/flutter#120929) * 674254c03 Always use the testbed in web_test.dart so `environment` is populated. (flutter/flutter#120984) * c4d40cc15 Modify the updateChildren method deep copy _children (flutter/flutter#120773) * 9367641ce clean up (flutter/flutter#120934) * 51712b90a Roll Plugins from d699b4a to 8f3419b (7 revisions) (flutter/flutter#120993) * c3587c62e Add `InheritedTheme` support to `ScrollbarTheme` (flutter/flutter#120970) * 08b409ab0 Roll Flutter Engine from 6e92c0c28410 to bd37a3992b50 (16 revisions) (flutter/flutter#121004) * f78513685 [web] Temporarily disable a line boundary test (flutter/flutter#121005) * 9fe556705 Print sub process that failed to run in tool (flutter/flutter#120999) * 6205c110d Remove "note that" in our documentation (as per style guide) (flutter/flutter#120842) * 1daa0be4f Fix scrollable to clear inner semantics node if it does not use two p… (flutter/flutter#120996) * 7f19b7485 0a27673d7 Roll Skia from 02890036028e to 0e444e355607 (9 revisions) (flutter/engine#39723) (flutter/flutter#121008) * 48d2dfc72 e7fde3f72 [web] Make glassPaneElement and glassPaneShadow non-nullable (flutter/engine#39692) (flutter/flutter#121009) * 610450523 2b2780185 Roll Skia from 0e444e355607 to 4b79e398dfe0 (5 revisions) (flutter/engine#39725) (flutter/flutter#121016) * f99f47280 Remove the deprecated accentColor from ThemeData (flutter/flutter#120932) * 2b4c96088 Remove more references to dart:ui.window (flutter/flutter#120994) * 0fa652752 Roll Flutter Engine from 2b2780185dd5 to a37e27b77008 (2 revisions) (flutter/flutter#121020) * 9281114fb Roll Flutter Engine from a37e27b77008 to 2fdce9a96367 (2 revisions) (flutter/flutter#121023) * 4dd555d32 Roll Flutter Engine from 2fdce9a96367 to 9a3c3e462fce (3 revisions) (flutter/flutter#121025) * 66dce657f Roll Flutter Engine from 9a3c3e462fce to 3777ed51774f (2 revisions) (flutter/flutter#121029) * a5b53a6d2 a9db42c3e Roll Skia from 733a19f6a625 to 2f05923f825e (3 revisions) (flutter/engine#39744) (flutter/flutter#121030) * 0be7c3f30 Roll Flutter Engine from a9db42c3edc2 to c22c64812243 (2 revisions) (flutter/flutter#121041)

…er/flutter#120773)

* 674254c03 Always use the testbed in web_test.dart so `environment` is populated. (flutter/flutter#120984) * c4d40cc15 Modify the updateChildren method deep copy _children (flutter/flutter#120773) * 9367641ce clean up (flutter/flutter#120934) * 51712b90a Roll Plugins from d699b4a to 8f3419b (7 revisions) (flutter/flutter#120993) * c3587c62e Add `InheritedTheme` support to `ScrollbarTheme` (flutter/flutter#120970) * 08b409ab0 Roll Flutter Engine from 6e92c0c28410 to bd37a3992b50 (16 revisions) (flutter/flutter#121004) * f78513685 [web] Temporarily disable a line boundary test (flutter/flutter#121005) * 9fe556705 Print sub process that failed to run in tool (flutter/flutter#120999) * 6205c110d Remove "note that" in our documentation (as per style guide) (flutter/flutter#120842) * 1daa0be4f Fix scrollable to clear inner semantics node if it does not use two p… (flutter/flutter#120996) * 7f19b7485 0a27673d7 Roll Skia from 02890036028e to 0e444e355607 (9 revisions) (flutter/engine#39723) (flutter/flutter#121008) * 48d2dfc72 e7fde3f72 [web] Make glassPaneElement and glassPaneShadow non-nullable (flutter/engine#39692) (flutter/flutter#121009) * 610450523 2b2780185 Roll Skia from 0e444e355607 to 4b79e398dfe0 (5 revisions) (flutter/engine#39725) (flutter/flutter#121016) * f99f47280 Remove the deprecated accentColor from ThemeData (flutter/flutter#120932) * 2b4c96088 Remove more references to dart:ui.window (flutter/flutter#120994) * 0fa652752 Roll Flutter Engine from 2b2780185dd5 to a37e27b77008 (2 revisions) (flutter/flutter#121020) * 9281114fb Roll Flutter Engine from a37e27b77008 to 2fdce9a96367 (2 revisions) (flutter/flutter#121023) * 4dd555d32 Roll Flutter Engine from 2fdce9a96367 to 9a3c3e462fce (3 revisions) (flutter/flutter#121025) * 66dce657f Roll Flutter Engine from 9a3c3e462fce to 3777ed51774f (2 revisions) (flutter/flutter#121029) * a5b53a6d2 a9db42c3e Roll Skia from 733a19f6a625 to 2f05923f825e (3 revisions) (flutter/engine#39744) (flutter/flutter#121030) * 0be7c3f30 Roll Flutter Engine from a9db42c3edc2 to c22c64812243 (2 revisions) (flutter/flutter#121041)

…er/flutter#120773)

Modify the updateChildren method deep copy _children

703c06a

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 15, 2023

add test

4644333

xu-baolin self-requested a review February 16, 2023 06:56

xu-baolin requested changes Feb 16, 2023

View reviewed changes

xu-baolin reviewed Feb 16, 2023

View reviewed changes

fix some small nits

53b293f

xu-baolin approved these changes Feb 16, 2023

View reviewed changes

xu-baolin added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 16, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 16, 2023

xu-baolin requested a review from goderbauer February 16, 2023 09:09

goderbauer reviewed Feb 16, 2023

View reviewed changes

packages/flutter/lib/src/widgets/framework.dart Outdated Show resolved Hide resolved

Simplified newChildren declaration in updateChildren

81a5698

goderbauer approved these changes Feb 17, 2023

View reviewed changes

goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 17, 2023

auto-submit bot merged commit c4d40cc into flutter:master Feb 17, 2023

engine-flutter-autoroll mentioned this pull request Feb 17, 2023

Roll Flutter from 170539f83dc8 to c4d40cc1528e (19 revisions) flutter/packages#3222

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 17, 2023

c4d40cc15 Modify the updateChildren method deep copy _children (flutt…

7a752e5

…er/flutter#120773)

engine-flutter-autoroll mentioned this pull request Feb 17, 2023

Roll Flutter from 170539f83dc8 to 9367641ce2ac (20 revisions) flutter/packages#3223

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 17, 2023

c4d40cc15 Modify the updateChildren method deep copy _children (flutt…

067edca

…er/flutter#120773)

This was referenced Feb 18, 2023

Roll Flutter from 170539f83dc8 to 0be7c3f30d64 (38 revisions) flutter/packages#3225

Merged

Roll Flutter from 298d8c76ba78 to 0be7c3f30d64 (21 revisions) flutter/plugins#7193

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 18, 2023

c4d40cc15 Modify the updateChildren method deep copy _children (flutt…

18bed57

…er/flutter#120773)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 18, 2023

c4d40cc15 Modify the updateChildren method deep copy _children (flutt…

56cd16c

…er/flutter#120773)

engine-flutter-autoroll mentioned this pull request Feb 18, 2023

Roll Flutter from 298d8c76ba78 to 0be7c3f30d64 (21 revisions) flutter/plugins#7194

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 18, 2023

c4d40cc15 Modify the updateChildren method deep copy _children (flutt…

9eaeb7e

…er/flutter#120773)

HansMuller mentioned this pull request Mar 2, 2023

Flutter Framework Team: Notable Commits #121415

Open

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

c4d40cc15 Modify the updateChildren method deep copy _children (flutt…

c44a726

…er/flutter#120773)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

c4d40cc15 Modify the updateChildren method deep copy _children (flutt…

7d3a642

…er/flutter#120773)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify the updateChildren method deep copy _children #120773

Modify the updateChildren method deep copy _children #120773

yiiim commented Feb 15, 2023 •

edited

flutter-dashboard bot commented Feb 15, 2023

xu-baolin left a comment •

edited

xu-baolin Feb 16, 2023

yiiim commented Feb 16, 2023

xu-baolin left a comment

auto-submit bot commented Feb 16, 2023

auto-submit bot commented Feb 16, 2023

xu-baolin commented Feb 16, 2023

goderbauer left a comment

Modify the updateChildren method deep copy _children #120773

Modify the updateChildren method deep copy _children #120773

Conversation

yiiim commented Feb 15, 2023 • edited

Pre-launch Checklist

flutter-dashboard bot commented Feb 15, 2023

xu-baolin left a comment • edited

Choose a reason for hiding this comment

xu-baolin Feb 16, 2023

Choose a reason for hiding this comment

yiiim commented Feb 16, 2023

xu-baolin left a comment

Choose a reason for hiding this comment

auto-submit bot commented Feb 16, 2023

auto-submit bot commented Feb 16, 2023

xu-baolin commented Feb 16, 2023

goderbauer left a comment

Choose a reason for hiding this comment

yiiim commented Feb 15, 2023 •

edited

xu-baolin left a comment •

edited