From e683bcace9a7cf77f5b0919606bd9d721427c155 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Wed, 22 Sep 2021 14:03:07 -0400 Subject: [PATCH 1/3] [web] Fix disposal of browser history on hot restart --- .../lib/src/engine/navigation/history.dart | 21 +++- lib/web_ui/lib/src/engine/window.dart | 4 + lib/web_ui/test/engine/history_test.dart | 103 ++++++++++++++++++ 3 files changed, 124 insertions(+), 4 deletions(-) diff --git a/lib/web_ui/lib/src/engine/navigation/history.dart b/lib/web_ui/lib/src/engine/navigation/history.dart index 9c6de07e95edf..f0eee478308e1 100644 --- a/lib/web_ui/lib/src/engine/navigation/history.dart +++ b/lib/web_ui/lib/src/engine/navigation/history.dart @@ -4,6 +4,7 @@ import 'dart:html' as html; +import 'package:meta/meta.dart'; import 'package:ui/ui.dart' as ui; import '../platform_dispatcher.dart'; @@ -55,6 +56,20 @@ abstract class BrowserHistory { ); } + /// Release any resources held by this [BrowserHistory] instance. + /// + /// This method has no effect on the browser history entries. Use [tearDown] + /// instead to revert this instance's modifications to browser history + /// entries. + @mustCallSuper + void dispose() { + if (_isDisposed || urlStrategy == null) { + return; + } + _isDisposed = true; + _unsubscribe(); + } + /// Exit this application and return to the previous page. Future exit() async { if (urlStrategy != null) { @@ -199,8 +214,7 @@ class MultiEntriesBrowserHistory extends BrowserHistory { if (_isDisposed || urlStrategy == null) { return; } - _isDisposed = true; - _unsubscribe(); + dispose(); // Restores the html browser history. assert(_hasSerialCount(currentState)); @@ -370,8 +384,7 @@ class SingleEntryBrowserHistory extends BrowserHistory { if (_isDisposed || urlStrategy == null) { return; } - _isDisposed = true; - _unsubscribe(); + dispose(); // We need to remove the flutter entry that we pushed in setup. await urlStrategy!.go(-1); diff --git a/lib/web_ui/lib/src/engine/window.dart b/lib/web_ui/lib/src/engine/window.dart index 17c1271513529..b2c636e4b0cbe 100644 --- a/lib/web_ui/lib/src/engine/window.dart +++ b/lib/web_ui/lib/src/engine/window.dart @@ -13,6 +13,7 @@ import 'package:js/js.dart'; import 'package:meta/meta.dart'; import 'package:ui/ui.dart' as ui; +import '../engine.dart'; import 'browser_detection.dart'; import 'navigation/history.dart'; import 'navigation/js_url_strategy.dart'; @@ -51,6 +52,9 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { if (_isUrlStrategySet) { _browserHistory = createHistoryForExistingState(_customUrlStrategy); } + registerHotRestartListener(() { + _browserHistory?.dispose(); + }); } final Object _windowId; diff --git a/lib/web_ui/test/engine/history_test.dart b/lib/web_ui/test/engine/history_test.dart index 7f87324b4091d..c3be9e02f55c8 100644 --- a/lib/web_ui/test/engine/history_test.dart +++ b/lib/web_ui/test/engine/history_test.dart @@ -8,6 +8,7 @@ import 'dart:async'; import 'dart:html' as html; +import 'package:quiver/testing/async.dart'; import 'package:test/bootstrap/browser.dart'; import 'package:test/test.dart'; import 'package:ui/src/engine.dart' show window; @@ -121,6 +122,60 @@ void testMain() { // TODO(mdebbar): https://github.com/flutter/flutter/issues/50836 skip: browserEngine == BrowserEngine.edge); + test('disposes of its listener without touching history', () async { + final TestUrlStrategy strategy = TestUrlStrategy.fromEntry( + const TestHistoryEntry('initial state', null, '/initial'), + ); + expect(strategy.listeners, isEmpty); + + await window.debugInitializeHistory(strategy, useSingle: true); + + final Map wrappedOriginState = _wrapOriginState('initial state'); + + // There should be one `popstate` listener and two history entries. + expect(strategy.listeners, hasLength(1)); + expect(strategy.history, hasLength(2)); + expect(strategy.history[0].state, wrappedOriginState); + expect(strategy.history[0].url, '/initial'); + expect(strategy.history[1].state, flutterState); + expect(strategy.history[1].url, '/initial'); + + FakeAsync().run((FakeAsync fakeAsync) { + window.browserHistory.dispose(); + // The `TestUrlStrategy` implementation uses microtasks to schedule the + // removal of event listeners. + fakeAsync.flushMicrotasks(); + }); + + // After disposing, there should no listeners, and the history entries + // remain unaffected. + expect(strategy.listeners, isEmpty); + expect(strategy.history, hasLength(2)); + expect(strategy.history[0].state, wrappedOriginState); + expect(strategy.history[0].url, '/initial'); + expect(strategy.history[1].state, flutterState); + expect(strategy.history[1].url, '/initial'); + + // An extra call to dispose should be safe. + FakeAsync().run((FakeAsync fakeAsync) { + expect(() => window.browserHistory.dispose(), returnsNormally); + fakeAsync.flushMicrotasks(); + }); + + // Same expectations should remain true after the second dispose. + expect(strategy.listeners, isEmpty); + expect(strategy.history, hasLength(2)); + expect(strategy.history[0].state, wrappedOriginState); + expect(strategy.history[0].url, '/initial'); + expect(strategy.history[1].state, flutterState); + expect(strategy.history[1].url, '/initial'); + }); + + test('disposes gracefully when url strategy is null', () async { + await window.debugInitializeHistory(null, useSingle: true); + expect(() => window.browserHistory.dispose(), returnsNormally); + }); + test('browser back button pops routes correctly', () async { final TestUrlStrategy strategy = TestUrlStrategy.fromEntry( const TestHistoryEntry(null, null, '/home'), @@ -326,6 +381,54 @@ void testMain() { // TODO(mdebbar): https://github.com/flutter/flutter/issues/50836 skip: browserEngine == BrowserEngine.edge); + test('disposes of its listener without touching history', () async { + final TestUrlStrategy strategy = TestUrlStrategy.fromEntry( + const TestHistoryEntry('initial state', null, '/initial'), + ); + expect(strategy.listeners, isEmpty); + + await window.debugInitializeHistory(strategy, useSingle: false); + + final Map taggedState = _tagStateWithSerialCount('initial state', 0); + + // There should be one `popstate` listener and one history entry. + expect(strategy.listeners, hasLength(1)); + expect(strategy.history, hasLength(1)); + expect(strategy.history.single.state, taggedState); + expect(strategy.history.single.url, '/initial'); + + FakeAsync().run((FakeAsync fakeAsync) { + window.browserHistory.dispose(); + // The `TestUrlStrategy` implementation uses microtasks to schedule the + // removal of event listeners. + fakeAsync.flushMicrotasks(); + }); + + // After disposing, there should no listeners, and the history entries + // remain unaffected. + expect(strategy.listeners, isEmpty); + expect(strategy.history, hasLength(1)); + expect(strategy.history.single.state, taggedState); + expect(strategy.history.single.url, '/initial'); + + // An extra call to dispose should be safe. + FakeAsync().run((FakeAsync fakeAsync) { + expect(() => window.browserHistory.dispose(), returnsNormally); + fakeAsync.flushMicrotasks(); + }); + + // Same expectations should remain true after the second dispose. + expect(strategy.listeners, isEmpty); + expect(strategy.history, hasLength(1)); + expect(strategy.history.single.state, taggedState); + expect(strategy.history.single.url, '/initial'); + }); + + test('disposes gracefully when url strategy is null', () async { + await window.debugInitializeHistory(null, useSingle: false); + expect(() => window.browserHistory.dispose(), returnsNormally); + }); + test('browser back button push route information correctly', () async { final TestUrlStrategy strategy = TestUrlStrategy.fromEntry( const TestHistoryEntry('initial state', null, '/home'), From f0297299a0132727aea9a85aad6bdb7e53870fb8 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Wed, 22 Sep 2021 15:49:42 -0400 Subject: [PATCH 2/3] make it able to teardown after a dispose --- .../lib/src/engine/navigation/history.dart | 13 ++++++---- lib/web_ui/test/engine/history_test.dart | 24 +++++++++++++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/web_ui/lib/src/engine/navigation/history.dart b/lib/web_ui/lib/src/engine/navigation/history.dart index f0eee478308e1..f472c8a340643 100644 --- a/lib/web_ui/lib/src/engine/navigation/history.dart +++ b/lib/web_ui/lib/src/engine/navigation/history.dart @@ -48,6 +48,7 @@ abstract class BrowserHistory { /// The strategy to interact with html browser history. UrlStrategy? get urlStrategy; + bool _isTornDown = false; bool _isDisposed = false; void _setupStrategy(UrlStrategy strategy) { @@ -211,10 +212,12 @@ class MultiEntriesBrowserHistory extends BrowserHistory { @override Future tearDown() async { - if (_isDisposed || urlStrategy == null) { + dispose(); + + if (_isTornDown || urlStrategy == null) { return; } - dispose(); + _isTornDown = true; // Restores the html browser history. assert(_hasSerialCount(currentState)); @@ -381,10 +384,12 @@ class SingleEntryBrowserHistory extends BrowserHistory { @override Future tearDown() async { - if (_isDisposed || urlStrategy == null) { + dispose(); + + if (_isTornDown || urlStrategy == null) { return; } - dispose(); + _isTornDown = true; // We need to remove the flutter entry that we pushed in setup. await urlStrategy!.go(-1); diff --git a/lib/web_ui/test/engine/history_test.dart b/lib/web_ui/test/engine/history_test.dart index c3be9e02f55c8..2593b42f5e542 100644 --- a/lib/web_ui/test/engine/history_test.dart +++ b/lib/web_ui/test/engine/history_test.dart @@ -123,14 +123,16 @@ void testMain() { skip: browserEngine == BrowserEngine.edge); test('disposes of its listener without touching history', () async { + const String unwrappedOriginState = 'initial state'; + final Map wrappedOriginState = _wrapOriginState(unwrappedOriginState); + final TestUrlStrategy strategy = TestUrlStrategy.fromEntry( - const TestHistoryEntry('initial state', null, '/initial'), + const TestHistoryEntry(unwrappedOriginState, null, '/initial'), ); expect(strategy.listeners, isEmpty); await window.debugInitializeHistory(strategy, useSingle: true); - final Map wrappedOriginState = _wrapOriginState('initial state'); // There should be one `popstate` listener and two history entries. expect(strategy.listeners, hasLength(1)); @@ -169,6 +171,12 @@ void testMain() { expect(strategy.history[0].url, '/initial'); expect(strategy.history[1].state, flutterState); expect(strategy.history[1].url, '/initial'); + + // Can still teardown after being disposed. + await window.browserHistory.tearDown(); + expect(strategy.history, hasLength(2)); + expect(strategy.currentEntry.state, unwrappedOriginState); + expect(strategy.currentEntry.url, '/initial'); }); test('disposes gracefully when url strategy is null', () async { @@ -382,14 +390,16 @@ void testMain() { skip: browserEngine == BrowserEngine.edge); test('disposes of its listener without touching history', () async { + const String untaggedState = 'initial state'; + final Map taggedState = _tagStateWithSerialCount(untaggedState, 0); + final TestUrlStrategy strategy = TestUrlStrategy.fromEntry( - const TestHistoryEntry('initial state', null, '/initial'), + const TestHistoryEntry(untaggedState, null, '/initial'), ); expect(strategy.listeners, isEmpty); await window.debugInitializeHistory(strategy, useSingle: false); - final Map taggedState = _tagStateWithSerialCount('initial state', 0); // There should be one `popstate` listener and one history entry. expect(strategy.listeners, hasLength(1)); @@ -422,6 +432,12 @@ void testMain() { expect(strategy.history, hasLength(1)); expect(strategy.history.single.state, taggedState); expect(strategy.history.single.url, '/initial'); + + // Can still teardown after being disposed. + await window.browserHistory.tearDown(); + expect(strategy.history, hasLength(1)); + expect(strategy.history.single.state, untaggedState); + expect(strategy.history.single.url, '/initial'); }); test('disposes gracefully when url strategy is null', () async { From 2a3e66e80f5191577c76b712d709a942aa6a110c Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Thu, 23 Sep 2021 12:10:12 -0400 Subject: [PATCH 3/3] fix warning --- lib/web_ui/lib/src/engine/window.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web_ui/lib/src/engine/window.dart b/lib/web_ui/lib/src/engine/window.dart index b2c636e4b0cbe..5ebb31825512f 100644 --- a/lib/web_ui/lib/src/engine/window.dart +++ b/lib/web_ui/lib/src/engine/window.dart @@ -13,7 +13,7 @@ import 'package:js/js.dart'; import 'package:meta/meta.dart'; import 'package:ui/ui.dart' as ui; -import '../engine.dart'; +import '../engine.dart' show registerHotRestartListener; import 'browser_detection.dart'; import 'navigation/history.dart'; import 'navigation/js_url_strategy.dart';