Unverified Commit ea5cfe09 authored by chunhtai's avatar chunhtai Committed by GitHub

Properly cleans up routes (#126453)

fixes https://github.com/flutter/flutter/issues/126100
parent af83c767
......@@ -849,40 +849,47 @@ class HeroController extends NavigatorObserver {
HeroFlightDirection flightType,
bool isUserGestureTransition,
) {
if (toRoute != fromRoute && toRoute is PageRoute<dynamic> && fromRoute is PageRoute<dynamic>) {
final PageRoute<dynamic> from = fromRoute;
final PageRoute<dynamic> to = toRoute;
if (toRoute == fromRoute ||
toRoute is! PageRoute<dynamic> ||
fromRoute is! PageRoute<dynamic>) {
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<dynamic> from = fromRoute;
final PageRoute<dynamic> 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) {
_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.
// 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;
}
}
// 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;
// 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.
WidgetsBinding.instance.addPostFrameCallback((Duration value) {
_startHeroTransition(from, to, flightType, isUserGestureTransition);
});
}
// 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);
});
}
}
......
......@@ -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<OverlayEntry> 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<Navigator> with TickerProviderStateMixin, RestorationMixin {
late GlobalKey<OverlayState> _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<Navigator> 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<OverlayState>();
......@@ -3423,6 +3470,28 @@ class NavigatorState extends State<Navigator> 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<Navigator> 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<Navigator> 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<Navigator> 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);
......
......@@ -84,6 +84,9 @@ abstract class OverlayRoute<T> extends Route<T> {
@override
void dispose() {
for (final OverlayEntry entry in _overlayEntries) {
entry.dispose();
}
_overlayEntries.clear();
super.dispose();
}
......@@ -704,7 +707,9 @@ mixin LocalHistoryRoute<T> on Route<T> {
// 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();
......
......@@ -427,7 +427,7 @@ void main() {
final FakeLicenseEntry licenseEntry = FakeLicenseEntry();
licenseCompleter.complete(licenseEntry);
expect(licenseEntry.packagesCalled, false);
}, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: <String, int?>{'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<LicenseEntry> licenseCompleter = Completer<LicenseEntry>();
......@@ -452,7 +452,7 @@ void main() {
await tester.pumpAndSettle();
expect(licenseEntry.packagesCalled, true);
}, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: <String, int?>{'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: <String, int?>{'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: <String, int?>{'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
......
......@@ -4171,7 +4171,7 @@ class RouteAnnouncementSpy extends Route<void> {
final AnnouncementCallBack? onDidPopNext;
@override
List<OverlayEntry> get overlayEntries => <OverlayEntry>[
final List<OverlayEntry> overlayEntries = <OverlayEntry>[
OverlayEntry(
builder: (BuildContext context) => const Placeholder(),
),
......
......@@ -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']));
expect(routes.isEmpty, isTrue);
results.clear();
});
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment