fixes `CupertinoFullscreenDialogTransition` leaks [prod-leak-fix] by Dimilkalathiya · Pull Request #147168 · flutter/flutter

Dimilkalathiya · 2024-04-22T15:45:01Z

Part of #141198

FixesCupertinoFullscreenDialogTransition widget leaks

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-04-22T16:02:24Z

cc: @polina-c this one reduces leaks by 330 :)

Dimilkalathiya · 2024-04-22T16:33:55Z

@polina-c framework_tests_impeller seems to be failing and from looking at logs editable_text_cursor_test.dart has something to do with this, i ran tests but except of leaks logs test-cases are passing.

does this has something to do with current changes in PR?

polina-c · 2024-04-22T21:40:21Z

Looks good. Can you opt-in some test, to make bot stop complaining about test coverage?

ValentinVignal

I have the same comments as for #147133

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

Dimilkalathiya · 2024-04-23T16:05:49Z

@ValentinVignal Again, thanks for review :)
i'll fix things as per your suggestions

Dimilkalathiya · 2024-04-23T16:09:40Z

@polina-c Do we need test here? since it does not change outside behaviour, if so what kind of test would be good fit for this?

polina-c · 2024-04-23T18:49:05Z

@polina-c Do we need test here? since it does not change outside behaviour, if so what kind of test would be good fit for this?

Since this PR fixes a leak, it would be great to opt-in a test to track this fixed leak.

Dimilkalathiya · 2024-04-25T17:51:15Z

@polina-c I have added test-case as discussed, let me know we are good go.

At first i tried to add opt-in into existing testcase but it seems all CupertinoPageRoute test are all paired with CupertinoApp which it self leaks CurvedAnimation so i had to create new testcase with MaterialApp would that be okay?

Also, added null assigned to curve after disposal

packages/flutter/test/cupertino/route_test.dart

polina-c · 2024-04-25T18:11:22Z

@polina-c framework_tests_impeller seems to be failing and from looking at logs editable_text_cursor_test.dart has something to do with this, i ran tests but except of leaks logs test-cases are passing.

does this has something to do with current changes in PR?

It seems they passed now.

Dimilkalathiya · 2024-04-25T18:43:44Z

@polina-c added empty line

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

packages/flutter/test/cupertino/route_test.dart

Dimilkalathiya · 2024-04-26T17:08:04Z

@polina-c @ValentinVignal Added mentioned changes 👀

polina-c · 2024-04-26T18:11:00Z

This is not addressed yet: #147168 (comment)

Dimilkalathiya · 2024-04-26T18:26:26Z

@polina-c please check now

…utter#147168)

flutter/flutter@2e80670...f9933b6 2024-04-27 [email protected] Roll Flutter Engine from 5205e3683a0a to 20fb62ba1455 (1 revision) (flutter/flutter#147449) 2024-04-27 [email protected] Roll Flutter Engine from e14649ea0c80 to 5205e3683a0a (1 revision) (flutter/flutter#147448) 2024-04-27 [email protected] Roll Flutter Engine from 87f489c1bed4 to e14649ea0c80 (1 revision) (flutter/flutter#147446) 2024-04-27 [email protected] Roll Flutter Engine from cecf5aa8a778 to 87f489c1bed4 (1 revision) (flutter/flutter#147445) 2024-04-27 [email protected] Roll Flutter Engine from 8af10eba3ef3 to cecf5aa8a778 (1 revision) (flutter/flutter#147444) 2024-04-27 [email protected] [macOS] Eliminate flutter_gallery_macos__start_up benchmark (flutter/flutter#147442) 2024-04-27 [email protected] Roll Flutter Engine from bc055398f42a to 8af10eba3ef3 (1 revision) (flutter/flutter#147441) 2024-04-27 [email protected] Roll Flutter Engine from c410180e5bba to bc055398f42a (7 revisions) (flutter/flutter#147440) 2024-04-26 [email protected] Add tests for character_activator.0.dart API example. (flutter/flutter#147384) 2024-04-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.2 to 3.25.3 (flutter/flutter#147437) 2024-04-26 [email protected] Add integration test for asset transformation feature (flutter/flutter#145715) 2024-04-26 [email protected] Added missing tests for Table api example `table.0.dart`. (flutter/flutter#147318) 2024-04-26 [email protected] Catch any `FileSystemException` thrown when trying to read the template manifest during `flutter create` (flutter/flutter#145620) 2024-04-26 [email protected] fixes `CupertinoFullscreenDialogTransition` leaks (flutter/flutter#147168) 2024-04-26 [email protected] Fix helperMaxLines and errorMaxLines documentation (flutter/flutter#147409) 2024-04-26 [email protected] Refactor route focus node creation (flutter/flutter#147390) 2024-04-26 [email protected] Roll Packages from fde908d to dd01140 (5 revisions) (flutter/flutter#147420) 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://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

…utter#147168)

fixes CupertinoFullscreenDialogTransition leaks

828a84e

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 Apr 22, 2024

add linearTransition condition

f04f5e9

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

ValentinVignal reviewed Apr 23, 2024

View reviewed changes

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

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

ValentinVignal mentioned this pull request Apr 24, 2024

fixes cupertino page transition leak [prod-leak-fix] #147133

Merged

9 tasks

changes as per suggestion and new testcase

f13ced7

polina-c reviewed Apr 25, 2024

View reviewed changes

packages/flutter/test/cupertino/route_test.dart Show resolved Hide resolved

flutter deleted a comment from flutter-dashboard bot Apr 25, 2024

Dimilkalathiya added 2 commits April 25, 2024 23:46

add space before test-case

eea4bed

revert flutter_test_config.dart

dbd67a6

remove space 🫣

6941b92

polina-c reviewed Apr 25, 2024

View reviewed changes

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

polina-c reviewed Apr 25, 2024

View reviewed changes

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

polina-c reviewed Apr 25, 2024

View reviewed changes

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

ValentinVignal reviewed Apr 26, 2024

View reviewed changes

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

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

packages/flutter/test/cupertino/route_test.dart Show resolved Hide resolved

changes as per review

94e911e

added indention

593752c

polina-c approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'master' into fix_full_dialg_transition_leak

9fcdc29

polina-c merged commit cc9ac7d into flutter:master Apr 26, 2024

engine-flutter-autoroll mentioned this pull request Apr 27, 2024

Roll Flutter from 2e806700b928 to f9933b6994e4 (17 revisions) flutter/packages#6619

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2024

cc9ac7d fixes CupertinoFullscreenDialogTransition leaks (flutter/fl…

f8bf6c2

…utter#147168)

engine-flutter-autoroll mentioned this pull request Apr 27, 2024

Roll Flutter from 2e806700b928 to f9933b6994e4 (17 revisions) flutter/packages#6620

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 27, 2024

cc9ac7d fixes CupertinoFullscreenDialogTransition leaks (flutter/fl…

20dc63f

…utter#147168)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

cc9ac7d fixes CupertinoFullscreenDialogTransition leaks (flutter/fl…

3ccaaca

…utter#147168)

polina-c changed the title ~~fixes CupertinoFullscreenDialogTransition leaks~~ fixes CupertinoFullscreenDialogTransition leaks [prod-leak-fix] Aug 29, 2024

Conversation

Dimilkalathiya commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Dimilkalathiya commented Apr 22, 2024

Uh oh!

Dimilkalathiya commented Apr 22, 2024

Uh oh!

polina-c commented Apr 22, 2024

Uh oh!

ValentinVignal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dimilkalathiya commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dimilkalathiya commented Apr 23, 2024

Uh oh!

polina-c commented Apr 23, 2024

Uh oh!

Dimilkalathiya commented Apr 25, 2024

Uh oh!

Uh oh!

polina-c commented Apr 25, 2024

Uh oh!

Dimilkalathiya commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dimilkalathiya commented Apr 26, 2024

Uh oh!

polina-c commented Apr 26, 2024

Uh oh!

Dimilkalathiya commented Apr 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dimilkalathiya commented Apr 22, 2024 •

edited

Loading

Dimilkalathiya commented Apr 23, 2024 •

edited

Loading

Dimilkalathiya commented Apr 25, 2024 •

edited

Loading