fixes `CupertinoModalPopupRoute` and `CupertinoDialogRoute` leaks #147823

Dimilkalathiya · 2024-05-04T06:53:01Z

Fixes memory leak on CupertinoModalPopupRoute
Fixes memory leak on CupertinoDialogRoute
Adds new test-case for new changes

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

Dimilkalathiya · 2024-05-04T06:57:47Z

cc: @polina-c
Added new test-case to confirm leak is not happening as every other tests are paired-up with CupertinoApp which leaks CurvedAnimation itself

polina-c · 2024-05-05T01:15:56Z

packages/flutter/lib/src/cupertino/route.dart

@@ -1238,26 +1244,6 @@ Future<T?> showCupertinoModalPopup<T>({
 final Animatable<double> _dialogScaleTween = Tween<double>(begin: 1.3, end: 1.0)
 .chain(CurveTween(curve: Curves.linearToEaseOut));

-Widget _buildCupertinoDialogTransitions(BuildContext context, Animation<double> animation, Animation<double> secondaryAnimation, Widget child) {


You moved this method to other place, that made it hard to see diff. Can you move it back to make code review easier?

Piinks · 2024-05-08T18:27:15Z

@Dimilkalathiya thanks for the contribution, it looks like there are failing checks here, can you take a look?

Dimilkalathiya · 2024-05-08T18:30:31Z

Hii @Piinks

Actually I am working on #147816 which is similar to this, once I make sure it passes Google test and gets merge I'll apply the same changes here as well

packages/flutter/lib/src/cupertino/route.dart

Dimilkalathiya · 2024-05-10T03:48:26Z

@polina-c Not sure why the test-case is failing here, it passes in my pc

Does it have to do with tree status being red?

polina-c · 2024-05-10T17:38:13Z

@polina-c Not sure why the test-case is failing here, it passes in my pc

Does it have to do with tree status being red?

You can check tree status here: https://flutter-dashboard.appspot.com/#/build
But it takes some latency to reflect failures on the tree. If in doubts, you can create draft empty PR, that changes just some doc or comment and check if tests fail for it.

I see many bots fail.
Can you give details about the test? Which exact test you are debugging?

Dimilkalathiya · 2024-05-10T18:03:46Z

@polina-c I tried some of files path that are failed on logs:

flutter/packages/flutter/test/material/app_test.dart
draggable_scrollable_sheet_test.dart

These tests are passing locally.

Dimilkalathiya · 2024-05-11T13:40:26Z

@polina-c test are failing because of changes PR that i confirmed with draft PR but can't figure-out why, i tried few thing but no luck.

There are some things that are bit confusing to me let me know if you have any idea:

Test like star_border_test.dart are failing here which passes in my system.
Can't find connection between this test failure and changes we made star_border_test.dart is something entirely different thing.

edit:- i tried all the test-case failed by bots

Dimilkalathiya · 2024-05-18T10:21:20Z

@Piinks @polina-c I removed everything that was changed and just left some text change (which should not effect test) here yet test-case is failing not sure why, should i create new PR same changes to check?

polina-c · 2024-05-18T19:27:28Z

@Piinks @polina-c I removed everything that was changed and just left some text change (which should not effect test) here yet test-case is failing not sure why, should i create new PR same changes to check?

At the moment flutter tree is red: https://flutter-dashboard.appspot.com/#/build

That means tests are failing at master. No need to create new PR. Just wait for tree to become green and pull updates from master to your branch.

Does it help?

Dimilkalathiya · 2024-05-18T19:51:29Z

I believe the tree went red recently around 2 hours ago.

I did check tree status when tests were failing it was green 10-12 hours ago when I pushed code.

You can see that all those tests have failed as well.

But just in-case if you want I'll check it once tree status is green

polina-c · 2024-05-18T20:19:05Z

There is some latency in tree status.
When tests fails unexpectedly for me, I create separate draft PR without changes in the code just to compare failures, and make sure to refresh both PRs at the same time.

If you want to recreate PR as an experiment, go ahead. There is no penalty for it :)

Dimilkalathiya · 2024-05-19T04:19:40Z

There is some latency in tree status.
When tests fails unexpectedly for me, I create separate draft PR without changes in the code just to compare failures, and make sure to refresh both PRs at the same time.

If you want to recreate PR as an experiment, go ahead. There is no penalty for it :)

I see if it has latency then it makes sense, I will update PR when tree status is green.

Since CupertinoDialogRoute is the one causing an issue before here I'll leave it out and update PR so we can merge CupertinoModalPopupRoute's fix since I think i would be able to help-out in other leaks faster instead of spending too much time in this

fixes CupertinoModalPopupRoute and CupertinoDialogRoute leaks

2ae9408

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 4, 2024

Merge branch 'master' into cupertino_popup_leak

8cfa578

polina-c reviewed May 5, 2024

View reviewed changes

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label May 5, 2024

Dimilkalathiya added 3 commits May 9, 2024 21:48

fix test case and make buildTransitions less invasive

fd1ae18

add function to old position for better review

f9affe9

indentation fix

61509e8

polina-c reviewed May 9, 2024

View reviewed changes

packages/flutter/lib/src/cupertino/route.dart Outdated Show resolved Hide resolved

Dimilkalathiya and others added 2 commits May 9, 2024 22:14

indentation fix

da7a9a8

Merge branch 'master' into cupertino_popup_leak

884d5e6

Merge remote-tracking branch 'origin/master' into cupertino_popup_leak

0a5828a

Dimilkalathiya added 3 commits May 11, 2024 10:27

init animation once

6ea72f2

avoid behavior change if user passed custom transition

2beda7c

making transitionBuilder final

9e97987

Dimilkalathiya added 6 commits May 18, 2024 12:03

Merge remote-tracking branch 'upstream/master' into cupertino_popup_leak

7e133ad

revert conflicting changes

984cd41

fix space issue

2bb9df7

fix test case

6bf1f0d

remove test to check cause of failure

4fb47bf

remove everything except test-case

382917b

remove everything and minor change on text

d8a8f4c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes `CupertinoModalPopupRoute` and `CupertinoDialogRoute` leaks #147823

fixes `CupertinoModalPopupRoute` and `CupertinoDialogRoute` leaks #147823

Dimilkalathiya commented May 4, 2024 •

edited

Dimilkalathiya commented May 4, 2024

polina-c May 5, 2024

Piinks commented May 8, 2024 •

edited

Dimilkalathiya commented May 8, 2024 •

edited

Dimilkalathiya commented May 10, 2024

polina-c commented May 10, 2024 •

edited

Dimilkalathiya commented May 10, 2024

Dimilkalathiya commented May 11, 2024 •

edited

Dimilkalathiya commented May 18, 2024

polina-c commented May 18, 2024

Dimilkalathiya commented May 18, 2024

polina-c commented May 18, 2024 •

edited

Dimilkalathiya commented May 19, 2024

fixes CupertinoModalPopupRoute and CupertinoDialogRoute leaks #147823

Are you sure you want to change the base?

fixes CupertinoModalPopupRoute and CupertinoDialogRoute leaks #147823

Conversation

Dimilkalathiya commented May 4, 2024 • edited

Pre-launch Checklist

Dimilkalathiya commented May 4, 2024

polina-c May 5, 2024

Choose a reason for hiding this comment

Piinks commented May 8, 2024 • edited

Dimilkalathiya commented May 8, 2024 • edited

Dimilkalathiya commented May 10, 2024

polina-c commented May 10, 2024 • edited

Dimilkalathiya commented May 10, 2024

Dimilkalathiya commented May 11, 2024 • edited

Dimilkalathiya commented May 18, 2024

polina-c commented May 18, 2024

Dimilkalathiya commented May 18, 2024

polina-c commented May 18, 2024 • edited

Dimilkalathiya commented May 19, 2024

fixes `CupertinoModalPopupRoute` and `CupertinoDialogRoute` leaks #147823

fixes `CupertinoModalPopupRoute` and `CupertinoDialogRoute` leaks #147823

Dimilkalathiya commented May 4, 2024 •

edited

Piinks commented May 8, 2024 •

edited

Dimilkalathiya commented May 8, 2024 •

edited

polina-c commented May 10, 2024 •

edited

Dimilkalathiya commented May 11, 2024 •

edited

polina-c commented May 18, 2024 •

edited