From 8bfe5e5430baf0e22cf55256068ea6bfc39e9eac Mon Sep 17 00:00:00 2001 From: Oleksandr Klymenko Date: Fri, 29 May 2026 13:58:30 -0700 Subject: [PATCH] [go_router] Make ImperativeRouteMatch.complete idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an `isCompleted` guard inside `ImperativeRouteMatch.complete` so calling it a second time after the future has already resolved is silently ignored instead of throwing `Bad state: Future already completed`. `GoRouteData` ships a default `FutureOr onExit(...) => true`, and `go_router_builder` wires it into every generated `$route` factory, so `routeBase.onExit != null` is true for every route declared via `@TypedGoRoute` / `GoRouteData`. That forces every pop through the `scheduleMicrotask` path in `GoRouterDelegate._handlePopPageWithRouteMatch`: scheduleMicrotask(() async { final bool onExitResult = await routeBase.onExit!(...); if (onExitResult) { _completeRouteMatch(result, match); } }); return false; If the framework reaches `_handlePopPageWithRouteMatch` a second time for the same `ImperativeRouteMatch` before either microtask runs (iOS interactive back-edge gesture firing `Navigator.onPopPage` during the commit/animation window, chained `context.pop(result)` calls for nested modals, etc.), both microtasks call `_completeRouteMatch`. The first one completes the Completer and removes the match from `currentConfiguration`; the second one calls `completer.complete(value)` against an already-completed Completer and throws. The pop itself succeeds — `currentConfiguration` is already updated and `notifyListeners` rebuilds the Navigator without the popped page before the second microtask fires — so user-visible behaviour is unchanged. The error only shows up in apps' global error handlers (`PlatformDispatcher.onError` / `FlutterError.onError`), typically forwarded to crash reporting as fatal. Issue: https://github.com/flutter/flutter/issues/187326 (also covers the chained-pop / async-modal flavors of the same defect previously filed as #160696, #176493, #163813, #146938, #123369). Test: `match_test.dart` now asserts that calling `complete` twice does not throw and the second call is a no-op. --- packages/go_router/CHANGELOG.md | 10 ++++++++++ packages/go_router/lib/src/match.dart | 17 +++++++++++++++++ packages/go_router/pubspec.yaml | 2 +- packages/go_router/test/match_test.dart | 24 ++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index 9aad45c2da0c..77864a57e5eb 100644 --- a/packages/go_router/CHANGELOG.md +++ b/packages/go_router/CHANGELOG.md @@ -1,3 +1,13 @@ +## 17.2.4 + +- Fixes `Bad state: Future already completed` thrown by + `ImperativeRouteMatch.complete` when `_handlePopPageWithRouteMatch` is + invoked twice for the same imperative match before either scheduled + microtask runs (e.g. iOS interactive back-edge pop gesture, chained + `context.pop(result)` calls for nested modals). The completer is now + idempotent — a second `complete` call after the future has resolved is + silently ignored. + ## 17.2.3 - Fixes an assertion failure when navigating to URLs with hash fragments missing a leading slash. diff --git a/packages/go_router/lib/src/match.dart b/packages/go_router/lib/src/match.dart index 87f588a3eeef..4bdc4d9aa3f5 100644 --- a/packages/go_router/lib/src/match.dart +++ b/packages/go_router/lib/src/match.dart @@ -485,7 +485,24 @@ class ImperativeRouteMatch extends RouteMatch { /// Called when the corresponding [Route] associated with this route match is /// completed. + /// + /// This is idempotent: a second call after the future has already + /// completed is silently ignored. `GoRouteData`'s default + /// `onExit => true` forces every pop through the + /// `scheduleMicrotask` path in + /// `GoRouterDelegate._handlePopPageWithRouteMatch`, which makes it + /// possible for two microtasks to be scheduled for the same match + /// before either runs (e.g., the iOS interactive back-edge gesture + /// firing `Navigator.onPopPage` twice, or chained + /// `context.pop(result)` calls for nested modals). Without this + /// guard the second microtask throws + /// `Bad state: Future already completed`. The page is already gone + /// from `currentConfiguration` and the `.push()` future has already + /// resolved by then, so a second complete has no useful meaning. void complete([dynamic value]) { + if (completer.isCompleted) { + return; + } completer.complete(value); } diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index d4da05afae9f..306fe24daf0e 100644 --- a/packages/go_router/pubspec.yaml +++ b/packages/go_router/pubspec.yaml @@ -1,7 +1,7 @@ name: go_router description: A declarative router for Flutter based on Navigation 2 supporting deep linking, data-driven routes and more -version: 17.2.3 +version: 17.2.4 repository: https://github.com/flutter/packages/tree/main/packages/go_router issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22 diff --git a/packages/go_router/test/match_test.dart b/packages/go_router/test/match_test.dart index 1b12d2e71d7c..fa77dd0f3322 100644 --- a/packages/go_router/test/match_test.dart +++ b/packages/go_router/test/match_test.dart @@ -248,6 +248,30 @@ void main() { expect(match1 == match2, isFalse); expect(match1.hashCode == match2.hashCode, isFalse); }); + + test('complete is idempotent — second call is a no-op', () async { + // Regression test for the `Bad state: Future already completed` + // throw observed when `GoRouterDelegate._handlePopPageWithRouteMatch` + // fires twice for the same imperative match before either + // scheduled microtask runs `_completeRouteMatch` (e.g. the iOS + // interactive back-edge gesture, or chained `context.pop(result)` + // calls for nested modals). See + // https://github.com/flutter/flutter/issues/187326 and the + // duplicates cross-referenced there. + final completer = Completer(); + final match = ImperativeRouteMatch( + pageKey: const ValueKey('idempotent'), + matches: matchList1, + completer: completer, + ); + + match.complete('first'); + // Must not throw — the page is already gone from + // currentConfiguration and the future has already resolved. + match.complete('second'); + + expect(await completer.future, 'first'); + }); }); }