[go_router] Fix some leaks discovered by `leak_tracker_flutter_testing` #6210

ValentinVignal · 2024-02-27T10:34:22Z

Fix some memory issues discovered by leak_tracker_flutter_testing.
Activation of leak_tracker_flutter_testing in the tests to detect memory leak issues is not feasible yet, because of leaks in Flutter stable.

cc @polina-c

Pre-launch Checklist

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

ValentinVignal · 2024-02-27T10:34:37Z

@polina-c Is there an issue to link this PR to?

ValentinVignal · 2024-02-27T14:24:53Z

Because go_router depends on flutter_test from sdk which depends on leak_tracker_flutter_testing 3.0.3, leak_tracker_flutter_testing 3.0.3 is required.
So, because go_router depends on leak_tracker_flutter_testing ^2.0.1, version solving failed.

But leak_tracker_flutter_testing ^2.0.1 is the version available on stabe. @polina-c how can I make it work?

polina-c · 2024-02-27T16:19:01Z

Because go_router depends on flutter_test from sdk which depends on leak_tracker_flutter_testing 3.0.3, leak_tracker_flutter_testing 3.0.3 is required.
So, because go_router depends on leak_tracker_flutter_testing ^2.0.1, version solving failed.

But leak_tracker_flutter_testing ^2.0.1 is the version available on stabe. @polina-c how can I make it work?

Will investigate. Thank you. Converted the PR to draft for now.

polina-c · 2024-02-27T22:25:06Z

Because go_router depends on flutter_test from sdk which depends on leak_tracker_flutter_testing 3.0.3, leak_tracker_flutter_testing 3.0.3 is required.
So, because go_router depends on leak_tracker_flutter_testing ^2.0.1, version solving failed.
But leak_tracker_flutter_testing ^2.0.1 is the version available on stabe. @polina-c how can I make it work?
Will investigate. Thank you. Converted the PR to draft for now.

Okay, it should be leak_tracker_flutter_testing: any in pubspec.yaml to pick up the version of leak tracker from sdk. Thanks for raising this. I will update documentation.

…leak-tracker

ValentinVignal · 2024-02-28T02:54:30Z

Great thank you! I changed it in chore: Use any version for leak_tracker_flutter_testing

polina-c · 2024-02-28T22:26:13Z

I see tests are failing. And they are failing because of leaks. And many of them are originated by flutter itself.

For example, leaking "_NotAnnounced" was cleaned up this week in Flutter master.

It seems packages that use stable should not be early adopters of leak tracker. So, let's shelve this PR till leak_tracker is released and recommended for stable.

Thank you for the experiment. It helped:

To understand go_router has leaks.
To document that version should be any.

Converting this to draft.

ValentinVignal · 2024-02-29T01:06:41Z

@chunhtai what do you want me to do? Maybe I can remove leak_tracker_flutter_testing from the pubspec.yaml and revert the flutter_test_config.dart but keep the changes to prepare for when leak_tracker_flutter_testing can be used ?

polina-c · 2024-02-29T16:15:29Z

@chunhtai what do you want me to do? Maybe I can remove leak_tracker_flutter_testing from the pubspec.yaml and revert the flutter_test_config.dart but keep the changes to prepare for when leak_tracker_flutter_testing can be used ?

Yes, I like it: to merge fixes, while postponing regression testing. @chunhtai, how does it sound for you?

polina-c · 2024-03-06T18:10:04Z

@chunhtai what do you want me to do? Maybe I can remove leak_tracker_flutter_testing from the pubspec.yaml and revert the flutter_test_config.dart but keep the changes to prepare for when leak_tracker_flutter_testing can be used ?

Yes, I like it: to merge fixes, while postponing regression testing. @chunhtai, how does it sound for you?

chunhtai is unavailable at the moment
I think we can proceed with merging the change.

…leak-tracker

ValentinVignal · 2024-03-07T01:37:33Z

I removed the leak_tracker_flutter_testing from the pubspec.yaml and revert the flutter_test_config.dart in test: Disable leak tracking

polina-c · 2024-03-07T01:49:39Z

'Linux analyze_legacy N-2' is failing because HeroController did not have method dispose yet.

@johnpryan, is this legacy job fixed forever or we can upgrade it for newer versions of flutter?

johnpryan

LGTM

polina-c · 2024-03-07T17:32:10Z

'Linux analyze_legacy N-2' is failing because HeroController did not have method dispose yet.

@johnpryan, is this legacy job fixed forever or we can upgrade it for newer versions of flutter?

More information: https://github.com/flutter/packages/blob/main/.ci/legacy_project/README.md

@ValentinVignal, invoked dispose does not exist for this job, that is on Flutter 2.0.6. What is minimal version of Flutter that has this method?
We either need to increase oldest supported version of flutter, or not to add this missing dispose (that may cause memory leaks)

@stuartmorgan, are you right person to help here? Is it ok to stop supporting 2.0.6 and increase minimal supported version a little?

stuartmorgan · 2024-03-07T17:41:23Z

@stuartmorgan, are you right person to help here? Is it ok to stop supporting 2.0.6 and increase minimal supported version a little?

The earliest version of Flutter that any package in this repo is allowed to support (enforced by CI) is 3.13, so I don't really understand the question.

If N-2 is failing, then the PR doesn't work in 3.13.9. It's fine to drop 3.13 per the wiki, but that's unrelated to 2.0.6.

polina-c · 2024-03-07T17:57:11Z

Thanks.

I took 2.0.6 from https://github.com/flutter/packages/blob/main/.ci/legacy_project/README.md, but now i see it is just very first supported version.

ValentinVignal · 2024-03-08T01:36:10Z

I'm a bit unsure of what I should do, should I stop using HeroController.dispose or should I make a change to the pipeline?

stuartmorgan · 2024-03-08T14:00:47Z

I took 2.0.6 from https://github.com/flutter/packages/blob/main/.ci/legacy_project/README.md, but now i see it is just very first supported version.

That's actually unrelated to analyze_legacy; it's for the legacy native build step in android_build_all_packages.

polina-c · 2024-03-08T22:16:27Z

I'm a bit unsure of what I should do, should I stop using HeroController.dispose or should I make a change to the pipeline?

To choose between these two options we need to find out what pipeline change is needed. Then we will decide if this change makes sense or if it is better not to use dispose for now and instead create tech debt issue and leave TODO to dispose when all supported versions of flutter have the dispose. Does it help?

…leak-tracker

ValentinVignal · 2024-03-11T11:19:50Z

@polina-c After looking at it, it is because HeroControlle.dispose was released in flutter 3.16.x

_MaterialAppState should dispose MaterialHeroController. flutter#133951
https://docs.flutter.dev/release/release-notes/release-notes-3.16.0
So I had to increase the minimum flutter version of flutter to ^3.16.0

Is that okay? Or is it too soon?

stuartmorgan · 2024-03-11T15:00:32Z

packages/go_router/example/pubspec.yaml

@@ -5,7 +5,7 @@ publish_to: none

 environment:
  sdk: ^3.1.0


You'll need to change this to match (3.2.0), which is why CI is failing.

…leak-tracker

stuartmorgan · 2024-03-14T13:49:15Z

packages/go_router/CHANGELOG.md


-* Updates minimum supported SDK version to Flutter 3.13/Dart 3.1.
+- Updates minimum supported SDK version to Flutter 3.13/Dart 3.1.


This is no longer accurate.

Oops sorry about that.

I updated it in doc: Update the changelog to specify the new flutter and dart versions

…leak-tracker

…ges#6210)

flutter/packages@b21c542...756dcc1 2024-03-15 [email protected] [go_router] Use `leak_tracker_flutter_testing` (flutter/packages#6210) 2024-03-15 [email protected] [camera_web][google_maps_flutter] Fix tests throwing errors after test completion with manual roll (flutter/packages#6318) 2024-03-14 [email protected] [pigeon] Fixes double prefixes added to enum names for Objc HostApis and FlutterApis (flutter/packages#6263) 2024-03-14 [email protected] [webview_flutter_android][webview_flutter_wkwebview] Adds platform implementations for onHttpError (flutter/packages#6149) 2024-03-14 [email protected] [image_picker_android] Fix deprecation warnings by branching based on build version, and suppressing only when needed (flutter/packages#6233) 2024-03-14 [email protected] [google_maps_flutter] Started dispatching platform messages from platform thread (flutter/packages#6069) 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-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

ValentinVignal added 4 commits February 27, 2024 18:29

fix: Dispose disposable

bb74f86

chore: Add leak_tracker_flutter_testing

216661e

test: Fix the tests

1b27f12

doc: Update changelog and version

5d24963

ValentinVignal requested a review from chunhtai as a code owner February 27, 2024 10:34

github-actions bot added the p: go_router label Feb 27, 2024

Merge branch 'main' into go-router/test/add-leak-tracker

05ea74e

polina-c marked this pull request as draft February 27, 2024 16:58

polina-c mentioned this pull request Feb 27, 2024

Instruct not to specify version. dart-lang/leak_tracker#220

Merged

ValentinVignal added 2 commits February 28, 2024 10:53

chore: Use any version for leak_tracker_flutter_testing

f180422

Merge remote-tracking branch 'upstream/main' into go-router/test/add-…

ff92e8f

…leak-tracker

ValentinVignal marked this pull request as ready for review February 28, 2024 02:54

polina-c marked this pull request as draft February 28, 2024 22:26

Merge branch 'main' into go-router/test/add-leak-tracker

b19ec63

ValentinVignal added 2 commits March 7, 2024 09:30

Merge remote-tracking branch 'upstream/main' into go-router/test/add-…

65d563e

…leak-tracker

test: Disable leak tracking

c8d3646

ValentinVignal marked this pull request as ready for review March 7, 2024 01:36

polina-c approved these changes Mar 7, 2024

View reviewed changes

johnpryan approved these changes Mar 7, 2024

View reviewed changes

ValentinVignal added 2 commits March 11, 2024 19:16

chore: Increase version numbers

acab33b

Merge remote-tracking branch 'upstream/main' into go-router/test/add-…

41b8cd3

…leak-tracker

stuartmorgan reviewed Mar 11, 2024

View reviewed changes

ValentinVignal and others added 4 commits March 11, 2024 23:34

chore: Upgrade dart sdk dependency

3474a05

Merge remote-tracking branch 'upstream/main' into go-router/test/add-…

0c94727

…leak-tracker

chore: Remove package_info_plus_web dependency

8fd4c4b

Merge branch 'main' into go-router/test/add-leak-tracker

b6f8dfa

polina-c requested a review from stuartmorgan March 14, 2024 13:38

stuartmorgan reviewed Mar 14, 2024

View reviewed changes

ValentinVignal added 2 commits March 15, 2024 09:39

doc: Update the changelog to specify the new flutter and dart versions

081b183

Merge remote-tracking branch 'upstream/main' into go-router/test/add-…

5e08786

…leak-tracker

ValentinVignal requested a review from stuartmorgan March 15, 2024 01:40

polina-c merged commit 756dcc1 into flutter:main Mar 15, 2024
78 checks passed

polina-c changed the title ~~[go_router] Use leak_tracker_flutter_testing~~ [go_router] Fix some leaks discovered by leak_tracker_flutter_testing Mar 15, 2024

engine-flutter-autoroll mentioned this pull request Mar 15, 2024

Roll Packages from b21c5424783a to 756dcc11180b (6 revisions) flutter/flutter#145214

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2024

756dcc1 [go_router] Use leak_tracker_flutter_testing (flutter/packa…

15d6759

…ges#6210)

reidbaker mentioned this pull request Apr 1, 2024

[camera_android_camerax] Move integration_test dependency to tests reidbaker/packages#4

Closed

11 tasks

arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024

[go_router] Use leak_tracker_flutter_testing (flutter#6210)

bff4835

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[go_router] Fix some leaks discovered by `leak_tracker_flutter_testing` #6210

[go_router] Fix some leaks discovered by `leak_tracker_flutter_testing` #6210

ValentinVignal commented Feb 27, 2024 •

edited by polina-c

Loading

ValentinVignal commented Feb 27, 2024

ValentinVignal commented Feb 27, 2024

polina-c commented Feb 27, 2024 •

edited

Loading

polina-c commented Feb 27, 2024 •

edited

Loading

ValentinVignal commented Feb 28, 2024

polina-c commented Feb 28, 2024

ValentinVignal commented Feb 29, 2024

polina-c commented Feb 29, 2024

polina-c commented Mar 6, 2024

ValentinVignal commented Mar 7, 2024

polina-c commented Mar 7, 2024

johnpryan left a comment

polina-c commented Mar 7, 2024

stuartmorgan commented Mar 7, 2024

polina-c commented Mar 7, 2024 •

edited

Loading

ValentinVignal commented Mar 8, 2024

stuartmorgan commented Mar 8, 2024 •

edited

Loading

polina-c commented Mar 8, 2024

ValentinVignal commented Mar 11, 2024

stuartmorgan Mar 11, 2024

stuartmorgan Mar 14, 2024

ValentinVignal Mar 15, 2024 •

edited

Loading


		* Updates minimum supported SDK version to Flutter 3.13/Dart 3.1.
		- Updates minimum supported SDK version to Flutter 3.13/Dart 3.1.

[go_router] Fix some leaks discovered by leak_tracker_flutter_testing #6210

[go_router] Fix some leaks discovered by leak_tracker_flutter_testing #6210

Conversation

ValentinVignal commented Feb 27, 2024 • edited by polina-c Loading

Pre-launch Checklist

ValentinVignal commented Feb 27, 2024

ValentinVignal commented Feb 27, 2024

polina-c commented Feb 27, 2024 • edited Loading

polina-c commented Feb 27, 2024 • edited Loading

ValentinVignal commented Feb 28, 2024

polina-c commented Feb 28, 2024

ValentinVignal commented Feb 29, 2024

polina-c commented Feb 29, 2024

polina-c commented Mar 6, 2024

ValentinVignal commented Mar 7, 2024

polina-c commented Mar 7, 2024

johnpryan left a comment

Choose a reason for hiding this comment

polina-c commented Mar 7, 2024

stuartmorgan commented Mar 7, 2024

polina-c commented Mar 7, 2024 • edited Loading

ValentinVignal commented Mar 8, 2024

stuartmorgan commented Mar 8, 2024 • edited Loading

polina-c commented Mar 8, 2024

ValentinVignal commented Mar 11, 2024

stuartmorgan Mar 11, 2024

Choose a reason for hiding this comment

stuartmorgan Mar 14, 2024

Choose a reason for hiding this comment

ValentinVignal Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

[go_router] Fix some leaks discovered by `leak_tracker_flutter_testing` #6210

[go_router] Fix some leaks discovered by `leak_tracker_flutter_testing` #6210

ValentinVignal commented Feb 27, 2024 •

edited by polina-c

Loading

polina-c commented Feb 27, 2024 •

edited

Loading

polina-c commented Feb 27, 2024 •

edited

Loading

polina-c commented Mar 7, 2024 •

edited

Loading

stuartmorgan commented Mar 8, 2024 •

edited

Loading

ValentinVignal Mar 15, 2024 •

edited

Loading