Skip to content
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

CupertinoDialogRoute leak fix #148774

Merged
merged 25 commits into from
May 22, 2024
Merged

CupertinoDialogRoute leak fix #148774

merged 25 commits into from
May 22, 2024

Conversation

Dimilkalathiya
Copy link
Contributor

@Dimilkalathiya Dimilkalathiya commented May 21, 2024 •
edited
Loading

part of #141198

Reattempts to fix CupertinoDialogRoute the only thing left close CurvedAnimation leaks.

Pre-launch Checklist

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

Dimilkalathiya and others added 23 commits May 4, 2024 12:20
@Dimilkalathiya
fixes CupertinoModalPopupRoute and CupertinoDialogRoute leaks
2ae9408
@polina-c
Merge branch 'master' into cupertino_popup_leak
8cfa578
@Dimilkalathiya
fix test case and make buildTransitions less invasive
fd1ae18
@Dimilkalathiya
add function to old position for better review
f9affe9
@Dimilkalathiya
indentation fix
61509e8
@Dimilkalathiya
indentation fix
da7a9a8
@polina-c
Merge branch 'master' into cupertino_popup_leak
884d5e6
@Dimilkalathiya
Merge remote-tracking branch 'origin/master' into cupertino_popup_leak
0a5828a
@Dimilkalathiya
init animation once
6ea72f2
@Dimilkalathiya
avoid behavior change if user passed custom transition
2beda7c
@Dimilkalathiya
making transitionBuilder final
9e97987
@Dimilkalathiya
Merge remote-tracking branch 'upstream/master' into cupertino_popup_leak
7e133ad
@Dimilkalathiya
revert conflicting changes
984cd41
@Dimilkalathiya
fix space issue
2bb9df7
@Dimilkalathiya
fix test case
6bf1f0d
@Dimilkalathiya
remove test to check cause of failure
4fb47bf
@Dimilkalathiya
remove everything except test-case
382917b
@Dimilkalathiya
remove everything and minor change on text
d8a8f4c
@Dimilkalathiya
fix CupertinoModalPopupRoute leak
6414b89
@Dimilkalathiya
revert spell change
ee9d6c8
@Dimilkalathiya
fix CupertinoDialogRoute leaks
bb72ad5
@Dimilkalathiya
Merge remote-tracking branch 'upstream/master' into cupertino_popup_leak
31ebc85
@Dimilkalathiya
spacing issue fix
0ff539a
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. labels May 21, 2024
@Dimilkalathiya Dimilkalathiya marked this pull request as ready for review May 21, 2024 17:03
@Dimilkalathiya
Copy link
Contributor Author

@polina-c Looks like previous changes for CupertinoDialogRoute were indeed correct since its passing test, probably failed last time because of tree status being red?

hopefully this closes CurvedAnimation chapter

polina-c
polina-c reviewed May 21, 2024
View reviewed changes
packages/flutter/lib/src/cupertino/route.dart
curve: Curves.easeInOut,
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you assert here that _fadeAnimation != null.
It will improve code readability and verify our assumptions are correct even after future changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it may be better to create local:

final fadeAnimation = _fadeAnimation!

And then use it without !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would like me to only add local variable change

final fadeAnimation = _fadeAnimation!

And then use it without !

Or both assert and local variable change

It's a bit confusing because of the below line:

Actually it may be better to create a local:

Copy link
Contributor

Choose a reason for hiding this comment

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

With local variable assert is not needed, because the expression contains '!', that will fail in case of null.
So, just local.

@polina-c
Copy link
Contributor

Approved to kick off google testing. The comments still need to be addressed.

@Dimilkalathiya
Copy link
Contributor Author

Dimilkalathiya commented May 22, 2024 •
edited
Loading

@polina-c Added change as mentioned and it seems google test have passed as well 🥂

@polina-c polina-c merged commit 73bf206 into flutter:master May 22, 2024
72 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 22, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 22, 2024
@engine-flutter-autoroll
73bf206f3 CupertinoDialogRoute leak fix (flutter/flutter#148774)
3a54e49
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 23, 2024
@engine-flutter-autoroll
Roll Flutter from d02292dbc409 to 73bf206f35e7 (31 revisions) (#6780)
1c4eaf3 
flutter/flutter@d02292d...73bf206

2024-05-22 102401667+Dimilkalathiya@users.noreply.github.com `CupertinoDialogRoute` leak fix (flutter/flutter#148774)
2024-05-22 fluttergithubbot@gmail.com Marks Windows plugin_test to be flaky (flutter/flutter#148835)
2024-05-22 sokolovskyi.konstantin@gmail.com Add tests for actions.0.dart API example. (flutter/flutter#148678)
2024-05-22 tessertaha@gmail.com Introduce `WidgetStateBorderSide.lerp` (flutter/flutter#148122)
2024-05-22 holzgeist@users.noreply.github.com add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter/flutter#147968)
2024-05-22 katelovett@google.com [wiki migration] Pages under docs/postmortems/ (flutter/flutter#148798)
2024-05-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter/flutter#148812)
2024-05-22 victorsanniay@gmail.com Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter/flutter#148808)
2024-05-21 amirpanahandeh@yahoo.com Fix two dimensional viewport unexpected null exception when no child is laid out (flutter/flutter#148256)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter/flutter#148807)
2024-05-21 32538273+ValentinVignal@users.noreply.github.com Add test for undo_history_controller.0.dart (flutter/flutter#148205)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter/flutter#148802)
2024-05-21 polinach@google.com Fix test that leaks images. (flutter/flutter#148494)
2024-05-21 34871572+gmackall@users.noreply.github.com Fix warnings in `dependency_version_checker.gradle.kts` (flutter/flutter#148699)
2024-05-21 katelovett@google.com [wiki migration] Android team pages (flutter/flutter#148585)
2024-05-21 polinach@google.com Fix leaky test. (flutter/flutter#148788)
2024-05-21 bruno.leroux@gmail.com Add DropdownButton.menuWidth (flutter/flutter#148125)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Add test for focus example 2 (flutter/flutter#147624)
2024-05-21 34871572+gmackall@users.noreply.github.com Add a migrator to remove `FlutterMultiDexApplication.java` (flutter/flutter#148515)
2024-05-21 katelovett@google.com [wiki migration] Infra team pages (flutter/flutter#148718)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter/flutter#148776)
2024-05-21 jacksongardner@google.com Fix the output of the CDN test. (flutter/flutter#148730)
2024-05-21 katelovett@google.com [wiki migration] Release team pages (flutter/flutter#148723)
2024-05-21 ian@hixie.ch Remove hidden dependencies on LocalProcessManager (flutter/flutter#148096)
2024-05-21 pateltirth454@gmail.com Adds Missing `onHover` & `onFocusChange` for `OutlinedButton.icon` (flutter/flutter#144374)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Adds tests to NestedScrollView examples (flutter/flutter#148170)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from c2ef01f6f1ab to 8a352f01e503 (18 revisions) (flutter/flutter#148766)
2024-05-21 nate.w5687@gmail.com `switch` expressions: finale (flutter/flutter#148711)
2024-05-21 matej.knopp@gmail.com [iOS] specify minimum os version for native asset frameworks (flutter/flutter#148504)
2024-05-21 59215665+davidhicks980@users.noreply.github.com Removed brand references from MenuAnchor.dart (flutter/flutter#148760)
2024-05-21 zanderso@users.noreply.github.com Skip flaky test in expression_evaluation_test.dart (flutter/flutter#148737)

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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@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://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
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 23, 2024
@hello-coder-xu
Merge branch 'fix/_floatingActionButtonVisibilityValue-update' of htt…
396e78f 
…ps://github.com/hello-coder-xu/flutter into fix/_floatingActionButtonVisibilityValue-update

* 'fix/_floatingActionButtonVisibilityValue-update' of https://github.com/hello-coder-xu/flutter:
  fix: update _floatingActionButtonVisibility only if floatingActionButton is not null
  [wiki migration] Remaining pages under docs/about/ (flutter#148782)
  Roll Flutter Engine from b6971cdf14f8 to 8b094fbb94d8 (3 revisions) (flutter#148883)
  Fix the second TextFormField to trigger onTapOutside (flutter#148206)
  Try removing robolectric from `integration_test` tests (flutter#148803)
  Prevent test folder deletion on running `flutter create --empty` on an existing app project (flutter#147160)
  [wiki migration] Tool team pages (flutter#148779)
  Roll Flutter Engine from c89defa55801 to b6971cdf14f8 (6 revisions) (flutter#148819)
  [native_assets] Add support for link hooks (flutter#148474)
  Roll Packages from ba19b247d0ba to 65254411e677 (12 revisions) (flutter#148864)
  Update tokens to 4.0.0 (flutter#148789)
  Move Linux web_long_running_tests_2_5 to bringup (flutter#148854)
  `CupertinoDialogRoute` leak fix (flutter#148774)
  Marks Windows plugin_test to be flaky (flutter#148835)
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
@Dimilkalathiya @victorsanni
CupertinoDialogRoute leak fix (flutter#148774)
58371e2
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Jun 5, 2024
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@engine-flutter-autoroll @arc-yong
Roll Flutter from d02292dbc409 to 73bf206f35e7 (31 revisions) (flutte…
fc111a2 
…r#6780)

flutter/flutter@d02292d...73bf206

2024-05-22 102401667+Dimilkalathiya@users.noreply.github.com `CupertinoDialogRoute` leak fix (flutter/flutter#148774)
2024-05-22 fluttergithubbot@gmail.com Marks Windows plugin_test to be flaky (flutter/flutter#148835)
2024-05-22 sokolovskyi.konstantin@gmail.com Add tests for actions.0.dart API example. (flutter/flutter#148678)
2024-05-22 tessertaha@gmail.com Introduce `WidgetStateBorderSide.lerp` (flutter/flutter#148122)
2024-05-22 holzgeist@users.noreply.github.com add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter/flutter#147968)
2024-05-22 katelovett@google.com [wiki migration] Pages under docs/postmortems/ (flutter/flutter#148798)
2024-05-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter/flutter#148812)
2024-05-22 victorsanniay@gmail.com Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter/flutter#148808)
2024-05-21 amirpanahandeh@yahoo.com Fix two dimensional viewport unexpected null exception when no child is laid out (flutter/flutter#148256)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter/flutter#148807)
2024-05-21 32538273+ValentinVignal@users.noreply.github.com Add test for undo_history_controller.0.dart (flutter/flutter#148205)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter/flutter#148802)
2024-05-21 polinach@google.com Fix test that leaks images. (flutter/flutter#148494)
2024-05-21 34871572+gmackall@users.noreply.github.com Fix warnings in `dependency_version_checker.gradle.kts` (flutter/flutter#148699)
2024-05-21 katelovett@google.com [wiki migration] Android team pages (flutter/flutter#148585)
2024-05-21 polinach@google.com Fix leaky test. (flutter/flutter#148788)
2024-05-21 bruno.leroux@gmail.com Add DropdownButton.menuWidth (flutter/flutter#148125)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Add test for focus example 2 (flutter/flutter#147624)
2024-05-21 34871572+gmackall@users.noreply.github.com Add a migrator to remove `FlutterMultiDexApplication.java` (flutter/flutter#148515)
2024-05-21 katelovett@google.com [wiki migration] Infra team pages (flutter/flutter#148718)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter/flutter#148776)
2024-05-21 jacksongardner@google.com Fix the output of the CDN test. (flutter/flutter#148730)
2024-05-21 katelovett@google.com [wiki migration] Release team pages (flutter/flutter#148723)
2024-05-21 ian@hixie.ch Remove hidden dependencies on LocalProcessManager (flutter/flutter#148096)
2024-05-21 pateltirth454@gmail.com Adds Missing `onHover` & `onFocusChange` for `OutlinedButton.icon` (flutter/flutter#144374)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Adds tests to NestedScrollView examples (flutter/flutter#148170)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from c2ef01f6f1ab to 8a352f01e503 (18 revisions) (flutter/flutter#148766)
2024-05-21 nate.w5687@gmail.com `switch` expressions: finale (flutter/flutter#148711)
2024-05-21 matej.knopp@gmail.com [iOS] specify minimum os version for native asset frameworks (flutter/flutter#148504)
2024-05-21 59215665+davidhicks980@users.noreply.github.com Removed brand references from MenuAnchor.dart (flutter/flutter#148760)
2024-05-21 zanderso@users.noreply.github.com Skip flaky test in expression_evaluation_test.dart (flutter/flutter#148737)

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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@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://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

@polina-c polina-c polina-c approved these changes

Assignees
No one assigned
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. 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.

None yet
2 participants
@Dimilkalathiya @polina-c