Skip to content

Commit 6bafbf8

Browse files
chunhtaichunhtai
authored
Fixes crash when adding and removing mulitple page-based route (flutter#177338)
<!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> fixes flutter#177322 ## 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]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
1 parent 2b7e563 commit 6bafbf8

File tree

2 files changed

+78
-25
lines changed

2 files changed

+78
-25
lines changed

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

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,9 @@ abstract class Route<T> extends _RoutePlaceholder {
184184
NavigatorState? get navigator => _navigator;
185185
NavigatorState? _navigator;
186186

187+
bool get _installed => _navigator != null;
188+
bool _isInstalledIn(NavigatorState state) => _navigator == state;
189+
187190
/// The settings for this route.
188191
///
189192
/// See [RouteSettings] for details.
@@ -220,7 +223,7 @@ abstract class Route<T> extends _RoutePlaceholder {
220223
void _updateSettings(RouteSettings newSettings) {
221224
if (_settings != newSettings) {
222225
_settings = newSettings;
223-
if (_navigator != null) {
226+
if (_installed) {
224227
changedInternalState();
225228
}
226229
}
@@ -579,7 +582,7 @@ abstract class Route<T> extends _RoutePlaceholder {
579582
///
580583
/// If this is true, then [isActive] is also true.
581584
bool get isCurrent {
582-
if (_navigator == null) {
585+
if (!_installed) {
583586
return false;
584587
}
585588
final _RouteEntry? currentRouteEntry = _navigator!._lastRouteEntryWhereOrNull(
@@ -596,7 +599,7 @@ abstract class Route<T> extends _RoutePlaceholder {
596599
/// If [isFirst] and [isCurrent] are both true then this is the only route on
597600
/// the navigator (and [isActive] will also be true).
598601
bool get isFirst {
599-
if (_navigator == null) {
602+
if (!_installed) {
600603
return false;
601604
}
602605
final _RouteEntry? currentRouteEntry = _navigator!._firstRouteEntryWhereOrNull(
@@ -611,7 +614,7 @@ abstract class Route<T> extends _RoutePlaceholder {
611614
/// Whether there is at least one active route underneath this route.
612615
@protected
613616
bool get hasActiveRouteBelow {
614-
if (_navigator == null) {
617+
if (!_installed) {
615618
return false;
616619
}
617620
for (final _RouteEntry entry in _navigator!._history) {
@@ -3213,7 +3216,7 @@ class _RouteEntry extends RouteTransitionRecord {
32133216
);
32143217
assert(navigator._debugLocked);
32153218
assert(
3216-
route._navigator == null,
3219+
!route._installed,
32173220
'The pushed route has already been used. When pushing a route, a new '
32183221
'Route object must be provided.',
32193222
);
@@ -3292,7 +3295,7 @@ class _RouteEntry extends RouteTransitionRecord {
32923295
/// refuses to be popped.
32933296
bool handlePop({required NavigatorState navigator, required Route<dynamic>? previousPresent}) {
32943297
assert(navigator._debugLocked);
3295-
assert(route._navigator == navigator);
3298+
assert(route._isInstalledIn(navigator));
32963299
currentState = _RouteLifecycle.popping;
32973300
if (route._popCompleter.isCompleted) {
32983301
// This is a page-based route popped through the Navigator.pop. The
@@ -3322,7 +3325,7 @@ class _RouteEntry extends RouteTransitionRecord {
33223325
required Route<dynamic>? previousPresent,
33233326
}) {
33243327
assert(navigator._debugLocked);
3325-
if (route._navigator == navigator) {
3328+
if (route._isInstalledIn(navigator)) {
33263329
currentState = _RouteLifecycle.removing;
33273330
} else {
33283331
// This route is still waiting to be added while a top-most push or pop
@@ -3336,7 +3339,7 @@ class _RouteEntry extends RouteTransitionRecord {
33363339
}
33373340

33383341
void didAdd({required NavigatorState navigator, required bool isNewFirst}) {
3339-
assert(route._navigator == null);
3342+
assert(!route._installed);
33403343
route._navigator = navigator;
33413344
route.install();
33423345
assert(route.overlayEntries.isNotEmpty);
@@ -3436,7 +3439,7 @@ class _RouteEntry extends RouteTransitionRecord {
34363439
if (!navigator._entryWaitingForSubTreeDisposal.remove(this)) {
34373440
// This route must have been destroyed as a result of navigator
34383441
// force dispose.
3439-
assert(route._navigator == null && !navigator.mounted);
3442+
assert(!route._installed && !navigator.mounted);
34403443
return;
34413444
}
34423445
assert(currentState == _RouteLifecycle.disposing);
@@ -4484,9 +4487,12 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
44844487
assert(entry.currentState == _RouteLifecycle.remove);
44854488
continue;
44864489
case _RouteLifecycle.remove:
4487-
if (!seenTopActiveRoute) {
4490+
// Routes that are not yet added (_installed == false) will exit as if
4491+
// they have never been on the navigator. They shouldn't announce
4492+
// their presence.
4493+
if (!seenTopActiveRoute && entry.route._installed) {
44884494
if (poppedRoute != null) {
4489-
entry.route.didPopNext(poppedRoute);
4495+
entry.handleDidPopNext(poppedRoute);
44904496
}
44914497
poppedRoute = null;
44924498
}
@@ -5049,7 +5055,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
50495055
_debugLocked = true;
50505056
return true;
50515057
}());
5052-
assert(entry.route._navigator == null);
5058+
assert(!entry.route._installed);
50535059
assert(entry.currentState == _RouteLifecycle.push);
50545060
_history.add(entry);
50555061
_flushHistoryUpdates();
@@ -5127,7 +5133,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
51275133
Route<T> newRoute, {
51285134
TO? result,
51295135
}) {
5130-
assert(newRoute._navigator == null);
5136+
assert(!newRoute._installed);
51315137
_pushReplacementEntry(
51325138
_RouteEntry(newRoute, pageBased: false, initialState: _RouteLifecycle.pushReplace),
51335139
result,
@@ -5181,7 +5187,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
51815187
_debugLocked = true;
51825188
return true;
51835189
}());
5184-
assert(entry.route._navigator == null);
5190+
assert(!entry.route._installed);
51855191
assert(_history.isNotEmpty);
51865192
assert(
51875193
_history.any(_RouteEntry.isPresentPredicate),
@@ -5228,7 +5234,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
52285234
/// restored during state restoration.
52295235
@optionalTypeArgs
52305236
Future<T?> pushAndRemoveUntil<T extends Object?>(Route<T> newRoute, RoutePredicate predicate) {
5231-
assert(newRoute._navigator == null);
5237+
assert(!newRoute._installed);
52325238
assert(newRoute.overlayEntries.isEmpty);
52335239
_pushEntryAndRemoveUntil(
52345240
_RouteEntry(newRoute, pageBased: false, initialState: _RouteLifecycle.push),
@@ -5282,7 +5288,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
52825288
_debugLocked = true;
52835289
return true;
52845290
}());
5285-
assert(entry.route._navigator == null);
5291+
assert(!entry.route._installed);
52865292
assert(entry.route.overlayEntries.isEmpty);
52875293
assert(entry.currentState == _RouteLifecycle.push);
52885294
int index = _history.length - 1;
@@ -5315,7 +5321,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
53155321
@optionalTypeArgs
53165322
void replace<T extends Object?>({required Route<dynamic> oldRoute, required Route<T> newRoute}) {
53175323
assert(!_debugLocked);
5318-
assert(oldRoute._navigator == this);
5324+
assert(oldRoute._isInstalledIn(this));
53195325
_replaceEntry(
53205326
_RouteEntry(newRoute, pageBased: false, initialState: _RouteLifecycle.replace),
53215327
oldRoute,
@@ -5337,7 +5343,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
53375343
required RestorableRouteBuilder<T> newRouteBuilder,
53385344
Object? arguments,
53395345
}) {
5340-
assert(oldRoute._navigator == this);
5346+
assert(oldRoute._isInstalledIn(this));
53415347
assert(
53425348
_debugIsStaticCallback(newRouteBuilder),
53435349
'The provided routeBuilder must be a static function.',
@@ -5365,7 +5371,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
53655371
return true;
53665372
}());
53675373
assert(entry.currentState == _RouteLifecycle.replace);
5368-
assert(entry.route._navigator == null);
5374+
assert(!entry.route._installed);
53695375
final int index = _history.indexWhere(_RouteEntry.isRoutePredicate(oldRoute));
53705376
assert(index >= 0, 'This Navigator does not contain the specified oldRoute.');
53715377
assert(
@@ -5401,8 +5407,8 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
54015407
required Route<dynamic> anchorRoute,
54025408
required Route<T> newRoute,
54035409
}) {
5404-
assert(newRoute._navigator == null);
5405-
assert(anchorRoute._navigator == this);
5410+
assert(!newRoute._installed);
5411+
assert(anchorRoute._isInstalledIn(this));
54065412
_replaceEntryBelow(
54075413
_RouteEntry(newRoute, pageBased: false, initialState: _RouteLifecycle.replace),
54085414
anchorRoute,
@@ -5425,7 +5431,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
54255431
required RestorableRouteBuilder<T> newRouteBuilder,
54265432
Object? arguments,
54275433
}) {
5428-
assert(anchorRoute._navigator == this);
5434+
assert(anchorRoute._isInstalledIn(this));
54295435
assert(
54305436
_debugIsStaticCallback(newRouteBuilder),
54315437
'The provided routeBuilder must be a static function.',
@@ -5515,7 +5521,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
55155521
if (lastEntry == null) {
55165522
return false;
55175523
}
5518-
assert(lastEntry.route._navigator == this);
5524+
assert(lastEntry.route._isInstalledIn(this));
55195525

55205526
// TODO(justinmc): When the deprecated willPop method is removed, delete
55215527
// this code and use only popDisposition, below.
@@ -5636,7 +5642,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
56365642
_debugLocked = true;
56375643
return true;
56385644
}());
5639-
assert(route._navigator == this);
5645+
assert(route._isInstalledIn(this));
56405646
final bool wasCurrent = route.isCurrent;
56415647
final _RouteEntry entry = _history.firstWhere(_RouteEntry.isRoutePredicate(route));
56425648
entry.complete(result, isReplaced: false, imperativeRemoval: true);
@@ -5661,7 +5667,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
56615667
_debugLocked = true;
56625668
return true;
56635669
}());
5664-
assert(anchorRoute._navigator == this);
5670+
assert(anchorRoute._isInstalledIn(this));
56655671
final int anchorIndex = _history.indexWhere(_RouteEntry.isRoutePredicate(anchorRoute));
56665672
assert(anchorIndex >= 0, 'This Navigator does not contain the specified anchorRoute.');
56675673
assert(

‎packages/flutter/test/widgets/navigator_test.dart‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,7 @@ void main() {
280280
expect(tester.widget<Overlay>(find.byType(Overlay)).clipBehavior, Clip.none);
281281
});
282282

283+
// Regression test for https://github.com/flutter/flutter/issues/170250.
283284
testWidgets('Can remove page before they are added', (WidgetTester tester) async {
284285
final GlobalKey<NavigatorState> key = GlobalKey<NavigatorState>();
285286
Future<void> buildPages(List<Page<void>> pages) {
@@ -337,6 +338,52 @@ void main() {
337338
expect(tester.takeException(), isNull);
338339
});
339340

341+
// Regression test for https://github.com/flutter/flutter/issues/177322.
342+
testWidgets('Can remove page before they are added - case 2', (WidgetTester tester) async {
343+
final GlobalKey<NavigatorState> key = GlobalKey<NavigatorState>();
344+
Future<void> buildPages(List<Page<void>> pages) {
345+
return tester.pumpWidget(
346+
MediaQuery(
347+
data: MediaQueryData.fromView(tester.view),
348+
child: Directionality(
349+
textDirection: TextDirection.ltr,
350+
child: Navigator(key: key, pages: pages, onDidRemovePage: (_) {}),
351+
),
352+
),
353+
);
354+
}
355+
356+
const MaterialPage<void> page = MaterialPage<void>(
357+
key: ValueKey<String>('page'),
358+
child: Text('page'),
359+
);
360+
const MaterialPage<void> page1 = MaterialPage<void>(
361+
key: ValueKey<String>('page1'),
362+
child: Text('page1'),
363+
);
364+
const MaterialPage<void> page2 = MaterialPage<void>(
365+
key: ValueKey<String>('page2'),
366+
child: Text('page2'),
367+
);
368+
const MaterialPage<void> page3 = MaterialPage<void>(
369+
key: ValueKey<String>('page3'),
370+
child: Text('page3'),
371+
);
372+
await buildPages(<Page<void>>[page]);
373+
374+
expect(find.text('page'), findsOneWidget);
375+
376+
await buildPages(<Page<void>>[page, page1, page2, page3]);
377+
378+
// In mid transition;
379+
await tester.pump(const Duration(microseconds: 150));
380+
// Cause page1 and page2 to be removed before they are added.
381+
await buildPages(<Page<void>>[page]);
382+
await tester.pumpAndSettle();
383+
expect(find.text('page'), findsOneWidget);
384+
expect(tester.takeException(), isNull);
385+
});
386+
340387
testWidgets('Zero transition page-based route correctly notifies observers when it is popped', (
341388
WidgetTester tester,
342389
) async {

0 commit comments

Comments
 (0)