From ea5cfe09fd688acc019dca088873dc6b4a05c9b7 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Thu, 18 May 2023 17:32:26 -0700 Subject: [PATCH] Properly cleans up routes (#126453) fixes https://github.com/flutter/flutter/issues/126100 --- packages/flutter/lib/src/widgets/heroes.dart | 69 +++++----- .../flutter/lib/src/widgets/navigator.dart | 123 +++++++++++++----- packages/flutter/lib/src/widgets/routes.dart | 7 +- .../flutter/test/material/about_test.dart | 8 +- .../flutter/test/widgets/navigator_test.dart | 2 +- .../flutter/test/widgets/routes_test.dart | 2 +- 6 files changed, 144 insertions(+), 67 deletions(-) diff --git a/packages/flutter/lib/src/widgets/heroes.dart b/packages/flutter/lib/src/widgets/heroes.dart index 7e38cd7c78b..ca5e6d5f230 100644 --- a/packages/flutter/lib/src/widgets/heroes.dart +++ b/packages/flutter/lib/src/widgets/heroes.dart @@ -849,40 +849,47 @@ class HeroController extends NavigatorObserver { HeroFlightDirection flightType, bool isUserGestureTransition, ) { - if (toRoute != fromRoute && toRoute is PageRoute && fromRoute is PageRoute) { - final PageRoute from = fromRoute; - final PageRoute to = toRoute; + if (toRoute == fromRoute || + toRoute is! PageRoute || + fromRoute is! PageRoute) { + return; + } - // A user gesture may have already completed the pop, or we might be the initial route - switch (flightType) { - case HeroFlightDirection.pop: - if (from.animation!.value == 0.0) { - return; - } - case HeroFlightDirection.push: - if (to.animation!.value == 1.0) { - return; - } - } + final PageRoute from = fromRoute; + final PageRoute to = toRoute; - // For pop transitions driven by a user gesture: if the "to" page has - // maintainState = true, then the hero's final dimensions can be measured - // immediately because their page's layout is still valid. - if (isUserGestureTransition && flightType == HeroFlightDirection.pop && to.maintainState) { + // A user gesture may have already completed the pop, or we might be the initial route + switch (flightType) { + case HeroFlightDirection.pop: + if (from.animation!.value == 0.0) { + return; + } + case HeroFlightDirection.push: + if (to.animation!.value == 1.0) { + return; + } + } + + // For pop transitions driven by a user gesture: if the "to" page has + // maintainState = true, then the hero's final dimensions can be measured + // immediately because their page's layout is still valid. + if (isUserGestureTransition && flightType == HeroFlightDirection.pop && to.maintainState) { + _startHeroTransition(from, to, flightType, isUserGestureTransition); + } else { + // Otherwise, delay measuring until the end of the next frame to allow + // the 'to' route to build and layout. + + // Putting a route offstage changes its animation value to 1.0. Once this + // frame completes, we'll know where the heroes in the `to` route are + // going to end up, and the `to` route will go back onstage. + to.offstage = to.animation!.value == 0.0; + + WidgetsBinding.instance.addPostFrameCallback((Duration value) { + if (from.navigator == null || to.navigator == null) { + return; + } _startHeroTransition(from, to, flightType, isUserGestureTransition); - } else { - // Otherwise, delay measuring until the end of the next frame to allow - // the 'to' route to build and layout. - - // Putting a route offstage changes its animation value to 1.0. Once this - // frame completes, we'll know where the heroes in the `to` route are - // going to end up, and the `to` route will go back onstage. - to.offstage = to.animation!.value == 0.0; - - WidgetsBinding.instance.addPostFrameCallback((Duration value) { - _startHeroTransition(from, to, flightType, isUserGestureTransition); - }); - } + }); } } diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index 58007829560..353b0614fcc 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -2786,6 +2786,7 @@ class Navigator extends StatefulWidget { // \ | // dispose* // | +// disposing // | // disposed // | @@ -2821,6 +2822,9 @@ enum _RouteLifecycle { removing, // we are waiting for subsequent routes to be done animating, then will switch to dispose // routes that are completely removed from the navigator and overlay. dispose, // we will dispose the route momentarily + disposing, // The entry is waiting for its widget subtree to be disposed + // first. It is stored in _entryWaitingForSubTreeDisposal while + // awaiting that. disposed, // we have disposed the route } @@ -3047,9 +3051,26 @@ class _RouteEntry extends RouteTransitionRecord { currentState = _RouteLifecycle.dispose; } - void dispose() { + /// Disposes this route entry and its [route] immediately. + /// + /// This method does not wait for the widget subtree of the [route] to unmount + /// before disposing. + void forcedDispose() { assert(currentState.index < _RouteLifecycle.disposed.index); currentState = _RouteLifecycle.disposed; + route.dispose(); + } + + /// Disposes this route entry and its [route]. + /// + /// This method waits for the widget subtree of the [route] to unmount before + /// disposing. If subtree is already unmounted, this method calls + /// [forcedDispose] immediately. + /// + /// Use [forcedDispose] if the [route] need to be disposed immediately. + void dispose() { + assert(currentState.index < _RouteLifecycle.disposing.index); + currentState = _RouteLifecycle.disposing; // If the overlay entries are still mounted, widgets in the route's subtree // may still reference resources from the route and we delay disposal of @@ -3060,24 +3081,43 @@ class _RouteEntry extends RouteTransitionRecord { final Iterable mountedEntries = route.overlayEntries.where((OverlayEntry e) => e.mounted); if (mountedEntries.isEmpty) { - route.dispose(); - } else { - int mounted = mountedEntries.length; - assert(mounted > 0); - for (final OverlayEntry entry in mountedEntries) { - late VoidCallback listener; - listener = () { - assert(mounted > 0); - assert(!entry.mounted); - mounted--; - entry.removeListener(listener); - if (mounted == 0) { - assert(route.overlayEntries.every((OverlayEntry e) => !e.mounted)); - route.dispose(); - } - }; - entry.addListener(listener); - } + forcedDispose(); + return; + } + + int mounted = mountedEntries.length; + assert(mounted > 0); + final NavigatorState navigator = route._navigator!; + navigator._entryWaitingForSubTreeDisposal.add(this); + for (final OverlayEntry entry in mountedEntries) { + late VoidCallback listener; + listener = () { + assert(mounted > 0); + assert(!entry.mounted); + mounted--; + entry.removeListener(listener); + if (mounted == 0) { + assert(route.overlayEntries.every((OverlayEntry e) => !e.mounted)); + // This is a listener callback of one of the overlayEntries in this + // route. Disposing the route also disposes its overlayEntries and + // violates the rule that a change notifier can't be disposed during + // its notifying callback. + // + // Use a microtask to ensure the overlayEntries have finished + // notifying their listeners before disposing. + return scheduleMicrotask(() { + if (!navigator._entryWaitingForSubTreeDisposal.remove(this)) { + // This route must have been destroyed as a result of navigator + // force dispose. + assert(route._navigator == null && !navigator.mounted); + return; + } + assert(currentState == _RouteLifecycle.disposing); + forcedDispose(); + }); + } + }; + entry.addListener(listener); } } @@ -3257,6 +3297,15 @@ class _NavigatorReplaceObservation extends _NavigatorObservation { class NavigatorState extends State with TickerProviderStateMixin, RestorationMixin { late GlobalKey _overlayKey; List<_RouteEntry> _history = <_RouteEntry>[]; + /// A set for entries that are waiting to dispose until their subtrees are + /// disposed. + /// + /// These entries are not considered to be in the _history and will usually + /// remove themselves from this set once they can dispose. + /// + /// The navigator keep track of these entries so that, in case the navigator + /// itself is disposed, it can dispose these entries immediately. + final Set<_RouteEntry> _entryWaitingForSubTreeDisposal = <_RouteEntry>{}; final _HistoryProperty _serializableHistory = _HistoryProperty(); final Queue<_NavigatorObservation> _observedRouteAdditions = Queue<_NavigatorObservation>(); final Queue<_NavigatorObservation> _observedRouteDeletions = Queue<_NavigatorObservation>(); @@ -3338,9 +3387,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res registerForRestoration(_serializableHistory, 'history'); // Delete everything in the old history and clear the overlay. - while (_history.isNotEmpty) { - _history.removeLast().dispose(); - } + _forcedDisposeAllRouteEntries(); assert(_history.isEmpty); _overlayKey = GlobalKey(); @@ -3423,6 +3470,28 @@ class NavigatorState extends State with TickerProviderStateMixin, Res } } + /// Dispose all lingering router entries immediately. + void _forcedDisposeAllRouteEntries() { + _entryWaitingForSubTreeDisposal.removeWhere((_RouteEntry entry) { + entry.forcedDispose(); + return true; + }); + while (_history.isNotEmpty) { + _disposeRouteEntry(_history.removeLast(), graceful: false); + } + } + + static void _disposeRouteEntry(_RouteEntry entry, {required bool graceful}) { + for (final OverlayEntry overlayEntry in entry.route.overlayEntries) { + overlayEntry.remove(); + } + if (graceful) { + entry.dispose(); + } else { + entry.forcedDispose(); + } + } + void _updateHeroController(HeroController? newHeroController) { if (_heroControllerFromScope != newHeroController) { if (newHeroController != null) { @@ -3595,9 +3664,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res }()); _updateHeroController(null); focusNode.dispose(); - for (final _RouteEntry entry in _history) { - entry.dispose(); - } + _forcedDisposeAllRouteEntries(); _rawNextPagelessRestorationScopeId.dispose(); _serializableHistory.dispose(); userGestureInProgressNotifier.dispose(); @@ -4022,6 +4089,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res // Delay disposal until didChangeNext/didChangePrevious have been sent. toBeDisposed.add(_history.removeAt(index)); entry = next; + case _RouteLifecycle.disposing: case _RouteLifecycle.disposed: case _RouteLifecycle.staging: assert(false); @@ -4051,10 +4119,7 @@ class NavigatorState extends State with TickerProviderStateMixin, Res // Lastly, removes the overlay entries of all marked entries and disposes // them. for (final _RouteEntry entry in toBeDisposed) { - for (final OverlayEntry overlayEntry in entry.route.overlayEntries) { - overlayEntry.remove(); - } - entry.dispose(); + _disposeRouteEntry(entry, graceful: true); } if (rearrangeOverlay) { overlay?.rearrange(_allRouteOverlayEntries); diff --git a/packages/flutter/lib/src/widgets/routes.dart b/packages/flutter/lib/src/widgets/routes.dart index 7cd4ead6ef0..6c3ddf881c4 100644 --- a/packages/flutter/lib/src/widgets/routes.dart +++ b/packages/flutter/lib/src/widgets/routes.dart @@ -84,6 +84,9 @@ abstract class OverlayRoute extends Route { @override void dispose() { + for (final OverlayEntry entry in _overlayEntries) { + entry.dispose(); + } _overlayEntries.clear(); super.dispose(); } @@ -704,7 +707,9 @@ mixin LocalHistoryRoute on Route { // elements during finalizeTree. The state is locked at this moment, and // we can only notify state has changed in the next frame. SchedulerBinding.instance.addPostFrameCallback((Duration duration) { - changedInternalState(); + if (isActive) { + changedInternalState(); + } }); } else { changedInternalState(); diff --git a/packages/flutter/test/material/about_test.dart b/packages/flutter/test/material/about_test.dart index 71b756c3dd1..191fa8eaf4b 100644 --- a/packages/flutter/test/material/about_test.dart +++ b/packages/flutter/test/material/about_test.dart @@ -427,7 +427,7 @@ void main() { final FakeLicenseEntry licenseEntry = FakeLicenseEntry(); licenseCompleter.complete(licenseEntry); expect(licenseEntry.packagesCalled, false); - }, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: {'ValueNotifier<_OverlayEntryWidgetState?>': null})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100. + }); testWidgetsWithLeakTracking('LicensePage returns late if unmounted', (WidgetTester tester) async { final Completer licenseCompleter = Completer(); @@ -452,7 +452,7 @@ void main() { await tester.pumpAndSettle(); expect(licenseEntry.packagesCalled, true); - }, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: {'ValueNotifier<_OverlayEntryWidgetState?>': null})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100. + }); testWidgetsWithLeakTracking('LicensePage logic defaults to executable name for app name', (WidgetTester tester) async { await tester.pumpWidget( @@ -1128,7 +1128,7 @@ void main() { // Configure to show the default layout. await tester.binding.setSurfaceSize(defaultSize); - }, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: {'ValueNotifier<_OverlayEntryWidgetState?>': null})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100. + }); testWidgetsWithLeakTracking('LicensePage master view layout position - rtl', (WidgetTester tester) async { const TextDirection textDirection = TextDirection.rtl; @@ -1191,7 +1191,7 @@ void main() { // Configure to show the default layout. await tester.binding.setSurfaceSize(defaultSize); - }, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: {'ValueNotifier<_OverlayEntryWidgetState?>': null})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100. + }); testWidgetsWithLeakTracking('License page title in lateral UI does not use AppBarTheme.foregroundColor', (WidgetTester tester) async { // This is a regression test for https://github.com/flutter/flutter/issues/108991 diff --git a/packages/flutter/test/widgets/navigator_test.dart b/packages/flutter/test/widgets/navigator_test.dart index d60215ebc84..26d14636392 100644 --- a/packages/flutter/test/widgets/navigator_test.dart +++ b/packages/flutter/test/widgets/navigator_test.dart @@ -4171,7 +4171,7 @@ class RouteAnnouncementSpy extends Route { final AnnouncementCallBack? onDidPopNext; @override - List get overlayEntries => [ + final List overlayEntries = [ OverlayEntry( builder: (BuildContext context) => const Placeholder(), ), diff --git a/packages/flutter/test/widgets/routes_test.dart b/packages/flutter/test/widgets/routes_test.dart index fff423ba925..8d802f67e25 100644 --- a/packages/flutter/test/widgets/routes_test.dart +++ b/packages/flutter/test/widgets/routes_test.dart @@ -401,7 +401,7 @@ void main() { ], ); await tester.pumpWidget(Container()); - expect(results, equals(['A: dispose', 'b: dispose'])); + expect(results, equals(['b: dispose', 'A: dispose'])); expect(routes.isEmpty, isTrue); results.clear(); });