fix(router): guard StackedPage pop completer and remove popped page by identity

[Eimen2018]

Summary

Fixes #1181: on Android back gestures with the Navigator 2 router, users hit Bad state: Future already completed and see the forward transition flash before the back animation.

Root cause

StackedPage holds a single _popCompleter per page instance and createRoute() chained popped.then(_popCompleter.complete) on every call. Flutter can call createRoute() more than once for the same Page instance. route.popped completes when the pop starts, but onDidRemovePage only fires once the route is disposed, which for an animated route is after the exit animation. If the Navigator rebuilds in between, router.stack still contains the popped page, so the Navigator creates a second route for it. That second route plays an enter animation (the flash) and later completes the same completer again (the crash).

Fix

1. StackedPage tracks the latest route it created. Only that route's pop result is forwarded, and only once.

2. When the latest route pops, the page is removed from the router stack right away instead of waiting for onDidRemovePage.

3. Removal is by page instance, not by routeKey. Pages pushed from the same route share a key (ValueKey(config.name)), so removing by key from two places could remove a sibling page. New internal StackRouter.removePageInstance(page) removes that exact instance and does nothing if it is already gone. RouteNavigator.onDidRemovePage uses it too, and when the instance is already gone it only runs the same cleanup master ran in that case, without removing anything by key.

4. Child routers are detached by page identity too. _removeTopRouterOf matched by key, so when the cleanup ran twice for one pop it removed the nested router of a sibling page from the same route. The fallback cleanup in onDidRemovePage now only detaches what is still attached to that exact page.

5. The eager removal is skipped for the last page on the stack. Emptying the stack at pop start swapped the Navigator for the placeholder mid-animation.

The public removeRoute(RouteData) is unchanged.

Flutter version note

When onDidRemovePage fires depends on the Flutter version: at pop start on 3.35 and 3.44+, but after the exit animation on 3.38 and 3.41. On the latter the eager removal runs first and the onDidRemovePage fallback runs second, which is why items 4 and 5 matter. Both orders are handled. CI runs on the latest stable only, so it cannot exercise the 3.38 to 3.41 order.

The eager-removal idea comes from #1188 by @Vinsho. #1182 is not adopted because it switches to the deprecated onPopPage.

Tests

• test/router/stacked_page_test.dart: completer is completed once, by the latest route only.
• test/router/remove_page_identity_test.dart, real router, scenarios from review:
  • [Home, Detail(1), Detail(2)], back press leaves [Home, Detail(1)]
  • [Home, Detail(1), Detail(2)], replace(Settings) leaves [Home, Detail(1), Settings]
  • replace(Search?q=x) from [Login] keeps the query params

All three failed before the identity-based change and pass now.

• test/router/nested_sibling_router_test.dart: [Home, Detail(1), Detail(2)] with a nested router in each Detail, pop Detail(2), Detail(1) keeps its nested router. Failed on Flutter 3.41 before item 4.
• stacked_page_test.dart: popping the last page does not eagerly remove it.

Verification

• flutter test: 103 passed, 1 pre-existing skip
• dart analyze lib test: no new issues

Note: #1198 also touches _removeRoute. Whichever merges second will need a small rebase.

🤖 Generated with Claude Code

[github-actions]

LCOV of commit b06f841 during Test Package #162

📊 Summary Coverage

Metric	Coverage	Details
📈 Lines	🔴 25.2%	937 / 3717 lines
🔧 Functions	⚪ N/A	no data found
🌿 Branches	⚪ N/A	no data found

📁 Files Changed Coverage

File	Lines	Functions	Branches
lib/src/code_generation/router_annotation/extended_navigator.dart	⚪ – (0)	⚪ – (0)	⚪ – (0)
lib/src/router/controller/routing_controller.dart	🔴 0.0% (189)	⚪ – (0)	⚪ – (0)
lib/src/router/stacked_page.dart	🔴 0.0% (38)	⚪ – (0)	⚪ – (0)
lib/src/router/widgets/route_navigator.dart	🔴 0.0% (29)	⚪ – (0)	⚪ – (0)

🟢 ≥ 90% · 🟡 ≥ 70% · 🔴 below 70% · ⚪ no data
Full coverage report

[ferrarafer]

Three issues with the eager removeRoute call in createRoute. They share one cause: _removeRoute is not idempotent. It matches pages by routeKey, which defaults to ValueKey(config.name) (route_matcher.dart:214), so pages pushed from the same route share a key. It also always runs _updateSharedPathData(includeAncestors: true) with default (empty) query params and fragment, plus _removeTopRouterOf and notifyAll(forceUrlRebuild: true), even when nothing is removed.

Two details of Flutter's navigator.dart matter here. On an imperative pop, onDidRemovePage runs synchronously in handlePop, before the route.popped.then microtask, so this call is always a second removal. And route.popped also completes when a page is dropped from pages (replace/replaceAll/removeWhere/navigate), a case where onDidRemovePage is not called.

Possible fix: remove only when this exact page instance is still in the router's stack (compare by identity, not by key), and skip the call when onDidRemovePage has already handled the page.

[ferrarafer]

One back press can remove two pages. Push DetailRoute(id: 1), then DetailRoute(id: 2), giving [Home, Detail1, Detail2], then press back. onDidRemovePage removes Detail2 synchronously. This callback then calls removeRoute(Detail2.routeData), and lastIndexWhere(p.routeKey == route.key) matches Detail1, which shares the key, so it is removed too. The user lands on Home instead of Detail1. The new tests don't cover pages that share a key.

[ferrarafer]

Page-list removals now remove a different page with the same key. With [Home, Detail1, Detail2], call router.replace(SettingsRoute()). _removeLast drops Detail2, and the Navigator then disposes Detail2's route, which completes popped. This callback calls removeRoute(Detail2.routeData), which matches Detail1 by key. The result is [Home, Settings] instead of [Home, Detail1, Settings]. removeWhere can go through the same path. Before this PR, these paths never called removeRoute.

[ferrarafer]

replace and navigate lose the new route's query params and fragment. Example: replace(SearchRoute(queryParams: {'q': 'x'})) from Login. After the Navigator removes Login's route, this callback calls removeRoute(Login.routeData). Login is no longer in _pages, but _removeRoute still calls _updateSharedPathData(includeAncestors: true) with the default {} and '', which clears query params and fragment on every page in this router and its ancestors, then forces a URL rebuild. On web, ?q=x drops from the URL and routeData.queryParams is empty. The same applies to navigate/_popUntilOrPushAll and replaceAll whenever a route is dropped.

[ferrarafer]

Crash: _finishRouteRemoval now runs twice per animated pop, and the redirect-guard cleanup is not idempotent.

On a normal animated pop the eager path in StackedPage.createRoute already ran removePageInstance -> _finishRouteRemoval. When the exit animation ends, onDidRemovePage fires, removePageInstance returns false, and this line runs _finishRouteRemoval a second time for the same RouteMatch.

The guard loop inside it is not idempotent:

void _removeRedirectGuard(RedirectGuardBase guard) {
  guard.removeListener(_redirectGuardsListeners[guard]!); // <-- null check
  _redirectGuardsListeners.remove(guard);
}

Scenario: a route protected by a RedirectGuard (e.g. AuthGuard extends RedirectGuard) is the only page carrying that guard, and the user pops it. First pass: the guard is absent from the remaining stack, so _removeRedirectGuard removes it from _redirectGuardsListeners. Second pass (this line, ~300ms later when the route is disposed): the guard is still absent from the stack, so _removeRedirectGuard is called again and _redirectGuardsListeners[guard]! throws Null check operator used on a null value.

Before this PR _removeRoute ran once per removal, so this path was not reachable on an ordinary pop. Suggest guarding removal (_redirectGuardsListeners.remove(guard)?.let(guard.removeListener)-style) and/or making finishRemovedRouteCleanup skip work it already did.

[ferrarafer]

Checked this against Flutter 3.47.2 and it does not reproduce — the ordering is the other way around.

In packages/flutter/lib/src/widgets/navigator.dart, _RouteEntry.handlePop:

route.didPop(pendingResult);                     // completes Route.popped
route.onPopInvokedWithResult(true, pendingResult);
if (pageBased && imperativeRemoval) {
  final page = route.settings as Page<Object?>;
  navigator.widget.onDidRemovePage?.call(page);  // synchronous, same call
}

Route.popped.then(...) callbacks only run on a later microtask, so onDidRemovePage always runs first. That means this handler is the one that removes the page and runs the cleanup, and the eager removePageInstance in StackedPage.createRoute is the call that returns false.

Verified empirically: I added a print at the top of _finishRouteRemoval and ran the full suite — exactly one call per removal, never two, and finishRemovedRouteCleanup never fired at all across 101 tests.

That said, relying on Flutter's internal ordering for idempotency is fragile, so I made _removeRedirectGuard tolerate a missing listener instead of using _redirectGuardsListeners[guard]! — pushed as f1c31b5.

[Eimen2018]

Your ordering holds on Flutter 3.44 and later (and on 3.35), but not on every supported version. I checked navigator.dart per tag:

Flutter	onDidRemovePage is called from
3.35	handlePop, at pop start
3.38, 3.41	_flushHistoryUpdates, at the dispose stage, after the exit animation
3.44, 3.47	handlePop, at pop start

On 3.38 and 3.41 the order flips: the eager path does the real removal and this fallback runs the cleanup a second time. I replayed your print experiment on 3.41.7 against the same suite: the eager path removed the page 3 times and finishRemovedRouteCleanup fired once. The pubspec only requires Dart 3.5, so those Flutter versions are in range.

Thanks for the idempotent guard change, kept as is. In b06f841 the fallback now only detaches what is still attached to that exact page (guards and its own child router), so a second pass is a no-op. CI only runs on the latest stable, so it cannot see this; a second job on an older Flutter would catch this class of issue.

[ferrarafer]

The double _finishRouteRemoval also re-runs _removeTopRouterOf(route.key), which removes the sibling's nested router — the exact class of bug this PR is fixing.

_finishRouteRemoval ends with _removeTopRouterOf(route.key), and _removeTopRouterOf is still key-based:

void _removeTopRouterOf(Key? key) {
  for (final ctr in List.unmodifiable(_childControllers).reversed) {
    if (ctr.key == key) { _childControllers.remove(ctr); break; }
  }
}

Because the popped page's cleanup now runs twice (eagerly from StackedPage.createRoute, then again here via finishRemovedRouteCleanup), two child controllers with that key get dropped.

Scenario, using the PR's own fixture shape: stack [Home, Detail(1), Detail(2)] where DetailRoute hosts a nested router (AutoTabsRouter / nested StackRouter), so both Detail pages register a child controller under ValueKey('DetailRoute'). Pop Detail(2): the eager pass removes Detail(2)'s child controller; the onDidRemovePage pass removes Detail(1)'s child controller, even though Detail(1) is still on screen. Detail(1)'s nested tab/stack state is then lost and innerRouterOf('DetailRoute') returns the wrong controller or null.

The page-identity fix does not help here — _removeTopRouterOf needs the same identity treatment, and/or the cleanup must not run twice.

[ferrarafer]

Not reproducible — I wrote this exact scenario as a widget test.

Stack [Home, Detail(1), Detail(2)], both Detail pages sharing ValueKey('DetailRoute'), each rendering an EmptyRouterPage so each owns a nested controller. After popping Detail(2):

expect(router.childControllers, hasLength(1));      // passes
expect(router.childControllers.single.routeData.args, 1);  // passes

Two reasons it holds:

1. _finishRouteRemoval runs once, not twice — see the reply on route_navigator.dart; onDidRemovePage fires synchronously inside handlePop, before the popped.then microtask, so the eager path is the no-op.
2. _removeTopRouterOf iterates List.unmodifiable(_childControllers).reversed and breaks on the first match, so even on a shared key it takes the last-registered controller — Detail(2)'s, which is the correct one.

[Eimen2018]

Reproduced on Flutter 3.41.7 with your exact scenario, because of the ordering in the thread above. Stack [Home, Detail(1), Detail(2)], each Detail hosting a nested router, pop Detail(2):

• master: childControllers has length 1, Detail(1)'s.
• PR head at f1c31b5: childControllers is empty. Both nested routers gone, including the one still on screen.

Fixed in b06f841. Child routers are now detached by page identity (identical(c.routeData, routeData)), and the key-based fallback only runs when the call itself removed a page. Your scenario is added as test/router/nested_sibling_router_test.dart. It fails on f1c31b5 and passes now on 3.41, and should pass on 3.47 as your version did.

[ferrarafer]

The identical(route, _latestRouteBox[0]) guard is the wrong way round for the very race it is meant to cover: if the phantom route is created before the real route pops, the pop is dropped forever.

The PR description's own root cause is that the Navigator can rebuild with the stale page and create a second route for it. The guard assumes the live route is always the latest one created. If the order is reversed — route A is on screen and mid-gesture, the Navigator rebuilds and createRoute makes route B (so _latestRouteBox[0] == B), and only then does A's predictive-back gesture commit — A's callback hits !identical(route, _latestRouteBox[0]) and returns early.

Consequences in that ordering:

• _popCompleter never completes, so await router.push<T>(...) / page.popped hangs forever (a silent deadlock, which is arguably worse than the Bad state it replaces).
• the eager removePageInstance(this) below never runs, so the page stays in router.stack while its route is gone; currentUrl and canPop() are then wrong.
• B is a phantom route nobody will ever pop, so nothing recovers.

_popCompleter.isCompleted alone is sufficient to stop the double-complete and is order-independent — first pop wins, later ones are ignored. Recommend dropping the identity check and keeping only the isCompleted guard, and making the eager removal run on whichever route completes first.

[ferrarafer]

This one is a fair concern, but dropping the identical check trades it for a worse failure.

_popCompleter.isCompleted alone is order-independent, agreed — but it also lets a stale route win. If createRoute ran twice and the discarded route completes while the page is still live, we would complete popped with that route's result and eagerly removePageInstance a page that is still on screen. That is exactly what the guard was added for in 9c7d631, and the regression test in stacked_page_test.dart asserts it: routeA.didPop('stale-result') then routeB.didPop('latest-result') must resolve to 'latest-result'.

I also tried a middle ground — ignore the superseded route only when the newer one is actually installed (_latestRouteBox[0]?.navigator != null) — which would fix the phantom case while keeping stale-route protection. It fails that same test, because in the test harness neither _TestRoute is installed in a Navigator.

The hang needs a route that is created, never installed, and never popped. I could not construct it. Leaving the guard as-is unless someone has a reproduction.

[Eimen2018]

Agree, keeping the identity check. The phantom route can only be created once the real route's history entry has been disposed, which happens in a later frame, while this callback runs as a microtask right after didPop. So the real route always sees itself as the latest one. The stale-route protection is worth more than a case I could not construct either.

[ferrarafer]

Eager removal of the last page empties _pages mid-animation, which swaps the whole Navigator out for the placeholder.

RouteNavigator.build is:

final navigator = widget.router.hasEntries ? Navigator(...) : widget.placeholder?.call(context) ?? Container(color: ...);

and hasEntries => _pages.isNotEmpty. Removing the page at pop start (rather than after the route is disposed) plus notifyAll(forceUrlRebuild: true) means that when the popped page is the only one left, the next rebuild replaces the Navigator with the placeholder Container while the exit animation is still running.

Scenario: a nested router whose stack is [Details] inside a tab, and something calls router.pop() (or the nested PopScope path in this same file forwards a back gesture). Instead of the route animating out, the whole nested navigator vanishes in one frame — and because the Navigator is disposed rather than told the page went away, onDidRemovePage never fires, so widget.didPop?.call(routeData.route, null) is never invoked and any pageless routes (dialogs/bottom sheets) on that navigator are torn down without their observers running.

Worth deferring the eager removal to after the route's exit animation when it would empty the stack, or keeping the Navigator mounted while a pop is in flight.

[ferrarafer]

This behaviour is real, but it predates the PR — the eager removal is not the actor.

Same test on both revisions: root router with a single page, then popForced(), sampling immediately after the pop.

• PR head 1e71dc1: stack=0 hasEntries=false navigators=0
• Pre-PR base b0bc379: stack=0 hasEntries=false navigators=0

Identical. The old handler already called widget.router.removeRoute(routeData) from onDidRemovePage, which Flutter invokes synchronously inside handlePop at pop start (Route.didComplete is documented to resolve popped "before the pop animation, if any, takes place"). So _pages was already emptied at the same moment before this change.

The eager removePageInstance cannot cause it, because it runs on a later microtask, by which point onDidRemovePage has already removed the page and it returns false.

Worth fixing on its own, but it is out of scope for this PR.

[Eimen2018]

Pre-existing on 3.47 and 3.35, agreed, but on 3.38 and 3.41 it is new behaviour from this PR. Same probe on 3.41.7, single-page root, popForced(), sampled one frame in:

• master: stack=1 hasEntries=true navigators=1, the page animates out.
• PR head: stack=0 hasEntries=false navigators=0, the Navigator is gone immediately.

In b06f841 the eager removal is skipped when the page is the last one on the stack, so onDidRemovePage handles that case as before. Covered by a unit test in stacked_page_test.dart.

[ferrarafer]

Thanks for this, @Eimen2018. The identity-based removal is a real fix: it covers popping or replacing two pages of the same route, and popAndPush to the same route, where the old key-based lastIndexWhere could remove the page that was just pushed. I didn't find any correctness issues. Two small things:

1. Description vs. current Flutter behavior. The description says onDidRemovePage fires after the exit animation. On Flutter 3.47.2, _RouteEntry.handlePop completes route.popped and calls onDidRemovePage in the same step, so the page is already gone by the time the .then in StackedPage runs, and the early removal there does nothing. It's harmless and a reasonable safeguard, but could you mention which Flutter version shows the delayed callback? Otherwise, reword it so it doesn't read as the main mechanism.

2. query params set by a replace survive the disposal of the replaced page (test/router/remove_page_identity_test.dart). The default TransitionDelegate doesn't call onDidRemovePage for a declarative replace, so I believe this test passes without the change too. If you want it to guard the fix, you could pop the replaced page instead, or drive removal through onDidRemovePage directly.

Already fixed