Properly cleans up routes [prod-leak-fix] #126453

chunhtai · 2023-05-10T18:28:09Z

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.

chunhtai · 2023-05-10T18:30:34Z

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

-    if (toRoute != fromRoute && toRoute is PageRoute<dynamic> && fromRoute is PageRoute<dynamic>) {
-      final PageRoute<dynamic> from = fromRoute;
-      final PageRoute<dynamic> to = toRoute;
+    if (toRoute == fromRoute) {


this just refactor the if condition to return early instead of nested condition.

the real functional change is in line 889 where it check for whether the route is still active before actually do the fly.

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

goderbauer · 2023-05-11T13:08:01Z

Looks like many checks are failing. is that related to this change?

goderbauer · 2023-05-11T13:09:09Z

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

+    if (toRoute == fromRoute) {
+      return;
+    }
+    if (toRoute is! PageRoute<dynamic> || fromRoute is! PageRoute<dynamic>) {


can you combine this with the previous if?

goderbauer · 2023-05-11T13:11:19Z

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

@@ -214,7 +214,9 @@ class OverlayEntry implements Listenable {
  /// This method should only be called by the object's owner.
  void dispose() {
    assert(!_disposedByOwner);
-    assert(_overlay == null, 'An OverlayEntry must first be removed from the Overlay before dispose is called.');
+    // In case the owner of this Overlay


this comment seems to be incomplete?

It's not immediately clear to me why the mounted state of the overlay is relevant. I guess that may have been what this comment was supposed to be about?

sorry about this, I should have turn this into draft. The mounted state was because if the owner of this overlay entry is above the overlay widget. It is possible the entry will be disposed without the owner to have a chance to remove the entry. This is because the dispose starts from the child. what the owner would know is the entry's notify the owner it has been disposed.

Things can be even worse if the entry is under another opaque entry and does not maintain state. In this case, owner won't even know the entry is disposed because it was never built

never mind If i try really hard, i can still solve this problem

goderbauer · 2023-05-11T13:15:10Z

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

@@ -3596,6 +3605,9 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
    _updateHeroController(null);
    focusNode.dispose();
    for (final _RouteEntry entry in _history) {
+      // for (final OverlayEntry overlayEntry in entry.route.overlayEntries) {


What about this commented out code?

flutter-dashboard · 2023-05-11T15:43:04Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai · 2023-05-11T19:14:00Z

packages/flutter/test/widgets/routes_test.dart

@@ -401,7 +401,7 @@ void main() {
      ],
    );
    await tester.pumpWidget(Container());
-    expect(results, equals(<String>['A: dispose', 'b: dispose']));
+    expect(results, equals(<String>['b: dispose', 'A: dispose']));


I think this is more correct as it disposed the top most route first.

chunhtai · 2023-05-11T19:15:10Z

packages/flutter/test/widgets/navigator_test.dart

@@ -4171,7 +4171,7 @@ class RouteAnnouncementSpy extends Route<void> {
  final AnnouncementCallBack? onDidPopNext;

  @override
-  List<OverlayEntry> get overlayEntries => <OverlayEntry>[
+  final List<OverlayEntry> overlayEntries = <OverlayEntry>[


the original code will create a new entry every time that will mess up the insert/remove overlay entry logic. previously this is not an issue because we didn't attempt to remove overlay entry when disposing the navigator

chunhtai · 2023-05-11T19:17:23Z

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

@@ -214,7 +214,9 @@ class OverlayEntry implements Listenable {
  /// This method should only be called by the object's owner.
  void dispose() {
    assert(!_disposedByOwner);
-    assert(_overlay == null, 'An OverlayEntry must first be removed from the Overlay before dispose is called.');
+    // In case the owner of this Overlay


never mind If i try really hard, i can still solve this problem

chunhtai · 2023-05-11T21:05:07Z

Several things makes this pr difficult than I think.

Some systems like HeroController have been operated on disposed Route, and they were somehow able to do that because we didn't properly clean up the route

There are also some logic to make sure the Route's dispose needs to wait for its subtree is properly disposed. This because the subtree may be using something like route's animationcontroller.

Therefore a notifier is introduced in overlayentry to let owner to listen for subtree disposal, but the problem with such logic is that you can't dispose the overlayentry when the notifer notify about the subtree disposal because of the changenotifier doesn't allow calling dispose during listener call back. This was previous ok because the route dispose doesn't properly clean up the overlay entry. Thus, I added a microtask to make sure the listener callback is done before disposing the route.

This created another issue that if a navigator is disposed, it expects all route to dispose. Because some of the dispose was scheduled in a microtask, the navigator won't be able to dispose. Therefore, I added a new lifecycle state called disposing. Navigator will keep track of routes in this state and will force these routes to dispose if Navigator itself is about to be disposed. This is ok because we know the subtree of the route must have been disposed already and we can safely clean up the route at this point

chunhtai · 2023-05-17T17:58:38Z

a friendly bump

goderbauer

LGTM

goderbauer · 2023-05-17T19:40:55Z

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

+  disposing, // The entry is waiting for its subtree to disposed first. It should
+             // be in stored in the _entryWaitingForSubTreeDisposal


subtree = widget subtree? Maybe a little clearer:

"The entry is waiting for its widget subtree to be disposed first. It is stored in _entryWaitingForSubTreeDisposal while awaiting that."

goderbauer · 2023-05-17T19:43:08Z

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

+    route.dispose();
+  }
+
+  void dispose() {


Maybe leave a comment here or on forcedDispose explaining the difference...

e.g. "dispose" waits for all overlayEntries to be unmounted before disposing the route.

flutter/flutter@5ae6438...077d644 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2e6b1e6c3458 to a0ea4d2d9ea5 (1 revision) (flutter/flutter#127203) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from e9178e115f14 to 2e6b1e6c3458 (1 revision) (flutter/flutter#127196) 2023-05-19 felangelov@gmail.com fix(flutter_tools): `findBundleFile` w/multiple flavor dimensions (flutter/flutter#127133) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2e8875870f52 to e9178e115f14 (1 revision) (flutter/flutter#127188) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7a209cc40c1 to 2e8875870f52 (1 revision) (flutter/flutter#127185) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5a57ff52f0f7 to c7a209cc40c1 (2 revisions) (flutter/flutter#127175) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from bfbd2e1dafb0 to 5a57ff52f0f7 (1 revision) (flutter/flutter#127172) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from e64084596182 to bfbd2e1dafb0 (2 revisions) (flutter/flutter#127167) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6bc60c8a9877 to e64084596182 (2 revisions) (flutter/flutter#127163) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from bca11a423f9c to 6bc60c8a9877 (1 revision) (flutter/flutter#127162) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9039c2dfb74c to bca11a423f9c (2 revisions) (flutter/flutter#127156) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3c23ddae1d2a to 9039c2dfb74c (2 revisions) (flutter/flutter#127154) 2023-05-19 47866232+chunhtai@users.noreply.github.com Properly cleans up routes (flutter/flutter#126453) 2023-05-19 36861262+QuncCccccc@users.noreply.github.com Remove deprecated `primaryVariant` and `secondaryVariant` from `ColorScheme` (flutter/flutter#127124) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17227c16ca58 to 3c23ddae1d2a (2 revisions) (flutter/flutter#127147) 2023-05-18 36861262+QuncCccccc@users.noreply.github.com Update `useMaterial3` api doc (flutter/flutter#127142) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7c679d6d411 to 17227c16ca58 (1 revision) (flutter/flutter#127143) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2c77c8011d45 to c7c679d6d411 (1 revision) (flutter/flutter#127139) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5fb3179a19c3 to 2c77c8011d45 (2 revisions) (flutter/flutter#127131) 2023-05-18 joshualitt@google.com Migrate benchmarks to package:web (flutter/flutter#126848) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 237c60185ace to 5fb3179a19c3 (3 revisions) (flutter/flutter#127126) 2023-05-18 53684884+mhbdev@users.noreply.github.com Fixing richMessage gesture recognizer in tooltip widget (flutter/flutter#126207) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 843ce0bba356 to 237c60185ace (2 revisions) (flutter/flutter#127122) 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 dit@google.com,rmistry@google.com,stuartmorgan@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

fixes flutter#126100

flutter/flutter@5ae6438...077d644 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2e6b1e6c3458 to a0ea4d2d9ea5 (1 revision) (flutter/flutter#127203) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from e9178e115f14 to 2e6b1e6c3458 (1 revision) (flutter/flutter#127196) 2023-05-19 felangelov@gmail.com fix(flutter_tools): `findBundleFile` w/multiple flavor dimensions (flutter/flutter#127133) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2e8875870f52 to e9178e115f14 (1 revision) (flutter/flutter#127188) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7a209cc40c1 to 2e8875870f52 (1 revision) (flutter/flutter#127185) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5a57ff52f0f7 to c7a209cc40c1 (2 revisions) (flutter/flutter#127175) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from bfbd2e1dafb0 to 5a57ff52f0f7 (1 revision) (flutter/flutter#127172) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from e64084596182 to bfbd2e1dafb0 (2 revisions) (flutter/flutter#127167) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6bc60c8a9877 to e64084596182 (2 revisions) (flutter/flutter#127163) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from bca11a423f9c to 6bc60c8a9877 (1 revision) (flutter/flutter#127162) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9039c2dfb74c to bca11a423f9c (2 revisions) (flutter/flutter#127156) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3c23ddae1d2a to 9039c2dfb74c (2 revisions) (flutter/flutter#127154) 2023-05-19 47866232+chunhtai@users.noreply.github.com Properly cleans up routes (flutter/flutter#126453) 2023-05-19 36861262+QuncCccccc@users.noreply.github.com Remove deprecated `primaryVariant` and `secondaryVariant` from `ColorScheme` (flutter/flutter#127124) 2023-05-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17227c16ca58 to 3c23ddae1d2a (2 revisions) (flutter/flutter#127147) 2023-05-18 36861262+QuncCccccc@users.noreply.github.com Update `useMaterial3` api doc (flutter/flutter#127142) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from c7c679d6d411 to 17227c16ca58 (1 revision) (flutter/flutter#127143) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2c77c8011d45 to c7c679d6d411 (1 revision) (flutter/flutter#127139) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5fb3179a19c3 to 2c77c8011d45 (2 revisions) (flutter/flutter#127131) 2023-05-18 joshualitt@google.com Migrate benchmarks to package:web (flutter/flutter#126848) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 237c60185ace to 5fb3179a19c3 (3 revisions) (flutter/flutter#127126) 2023-05-18 53684884+mhbdev@users.noreply.github.com Fixing richMessage gesture recognizer in tooltip widget (flutter/flutter#126207) 2023-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 843ce0bba356 to 237c60185ace (2 revisions) (flutter/flutter#127122) 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 dit@google.com,rmistry@google.com,stuartmorgan@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. labels May 10, 2023

chunhtai commented May 10, 2023

View reviewed changes

chunhtai requested a review from goderbauer May 10, 2023 18:33

chunhtai commented May 11, 2023

View reviewed changes

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

goderbauer reviewed May 11, 2023

View reviewed changes

chunhtai marked this pull request as draft May 11, 2023 15:41

chunhtai force-pushed the issues/126100 branch from 4b40633 to 479bcce Compare May 11, 2023 19:16

chunhtai commented May 11, 2023

View reviewed changes

chunhtai marked this pull request as ready for review May 11, 2023 20:51

chunhtai requested a review from goderbauer May 11, 2023 21:05

chunhtai force-pushed the issues/126100 branch from ea9102f to 33f2682 Compare May 17, 2023 17:35

goderbauer approved these changes May 17, 2023

View reviewed changes

chunhtai added 4 commits May 18, 2023 15:34

Properly cleans up routes

3921149

typo

786f658

fix comments

fe5398d

updatE

e7444d0

chunhtai force-pushed the issues/126100 branch from e152771 to e7444d0 Compare May 18, 2023 22:34

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label May 18, 2023

auto-submit bot merged commit ea5cfe0 into flutter:master May 19, 2023

engine-flutter-autoroll mentioned this pull request May 19, 2023

Roll Flutter from 5ae64381578c to 0d67c7e10cf9 (20 revisions) flutter/packages#4041

Closed

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

ea5cfe0 Properly cleans up routes (flutter/flutter#126453)

78afd24

engine-flutter-autoroll mentioned this pull request May 19, 2023

Roll Flutter from 5ae64381578c to 54ac355dc11f (22 revisions) flutter/packages#4042

Closed

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

ea5cfe0 Properly cleans up routes (flutter/flutter#126453)

d0f8cd7

engine-flutter-autoroll mentioned this pull request May 19, 2023

Roll Flutter from 5ae64381578c to 077d644bbc5f (23 revisions) flutter/packages#4043

Merged

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

ea5cfe0 Properly cleans up routes (flutter/flutter#126453)

fba5406

HansMuller mentioned this pull request May 19, 2023

Flutter Framework Team: Notable Commits #121415

Closed

CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023

Properly cleans up routes (flutter#126453)

a1d46c1

fixes flutter#126100

This was referenced May 25, 2023

Support minifcation for apps that depend on AGP 8 and integration_test #127628

Merged

i127682 android api 33 test support #127689

Merged

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

ea5cfe0 Properly cleans up routes (flutter/flutter#126453)

fdac987

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

ea5cfe0 Properly cleans up routes (flutter/flutter#126453)

ee3aced

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

ea5cfe0 Properly cleans up routes (flutter/flutter#126453)

9ee5c30

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

ea5cfe0 Properly cleans up routes (flutter/flutter#126453)

973ed67

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Sep 21, 2023

darshankawar mentioned this pull request Sep 26, 2023

[iOS] Wrong routing in the new flutter version (3.13.1) #135418

Closed

2 tasks

polina-c changed the title ~~Properly cleans up routes~~ Properly cleans up routes [prod-leak-fix] Aug 29, 2024

		disposing, // The entry is waiting for its subtree to disposed first. It should
		// be in stored in the _entryWaitingForSubTreeDisposal

Properly cleans up routes [prod-leak-fix] #126453

Properly cleans up routes [prod-leak-fix] #126453

Uh oh!

Conversation

chunhtai commented May 10, 2023

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented May 11, 2023

Uh oh!

chunhtai commented May 17, 2023

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai May 11, 2023 •

edited

Loading