[web] Fix disposal of browser history on hot restart (flutter#28805)

Author: mdebbar

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';
@@ -47,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) {
@@ -55,6 +57,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<void> exit() async {
     if (urlStrategy != null) {
@@ -196,11 +212,12 @@ class MultiEntriesBrowserHistory extends BrowserHistory {
 
   @override
   Future<void> tearDown() async {
-    if (_isDisposed || urlStrategy == null) {
+    dispose();
+
+    if (_isTornDown || urlStrategy == null) {
       return;
     }
-    _isDisposed = true;
-    _unsubscribe();
+    _isTornDown = true;
 
     // Restores the html browser history.
     assert(_hasSerialCount(currentState));
@@ -367,11 +384,12 @@ class SingleEntryBrowserHistory extends BrowserHistory {
 
   @override
   Future<void> tearDown() async {
-    if (_isDisposed || urlStrategy == null) {
+    dispose();
+
+    if (_isTornDown || urlStrategy == null) {
       return;
     }
-    _isDisposed = true;
-    _unsubscribe();
+    _isTornDown = true;
 
     // We need to remove the flutter entry that we pushed in setup.
     await urlStrategy!.go(-1);

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' show registerHotRestartListener;
 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;

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,68 @@ void testMain() {
         // TODO(mdebbar): https://github.com/flutter/flutter/issues/50836
         skip: browserEngine == BrowserEngine.edge);
 
+    test('disposes of its listener without touching history', () async {
+      const String unwrappedOriginState = 'initial state';
+      final Map<String, dynamic> wrappedOriginState = _wrapOriginState(unwrappedOriginState);
+
+      final TestUrlStrategy strategy = TestUrlStrategy.fromEntry(
+        const TestHistoryEntry(unwrappedOriginState, null, '/initial'),
+      );
+      expect(strategy.listeners, isEmpty);
+
+      await window.debugInitializeHistory(strategy, useSingle: true);
+
+
+      // 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');
+
+      // 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 {
+      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 +389,62 @@ void testMain() {
         // TODO(mdebbar): https://github.com/flutter/flutter/issues/50836
         skip: browserEngine == BrowserEngine.edge);
 
+    test('disposes of its listener without touching history', () async {
+      const String untaggedState = 'initial state';
+      final Map<String, dynamic> taggedState = _tagStateWithSerialCount(untaggedState, 0);
+
+      final TestUrlStrategy strategy = TestUrlStrategy.fromEntry(
+        const TestHistoryEntry(untaggedState, null, '/initial'),
+      );
+      expect(strategy.listeners, isEmpty);
+
+      await window.debugInitializeHistory(strategy, useSingle: false);
+
+
+      // 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');
+
+      // 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 {
+      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'),