Fix: Memory leak in UndoHistory widget because it never de-registered…

… itself as global UndoManager client (Resolves flutter#148291) (flutter#150661) Unsets a global `client` variable that was missed.
andrewkolos · Jun 22, 2024 · 1cb003b · 1cb003b
1 parent 88e6f62
commit 1cb003b
Show file tree

Hide file tree

Showing 2 changed files with 122 additions and 0 deletions.
diff --git a/packages/flutter/lib/src/widgets/undo_history.dart b/packages/flutter/lib/src/widgets/undo_history.dart
@@ -199,6 +199,10 @@ class UndoHistoryState<T> extends State<UndoHistory<T>> with UndoManagerClient {
 
   void _handleFocus() {
     if (!widget.focusNode.hasFocus) {
+      if (UndoManager.client == this) {
+        UndoManager.client = null;
+      }
+
       return;
     }
     UndoManager.client = this;
@@ -257,6 +261,10 @@ class UndoHistoryState<T> extends State<UndoHistory<T>> with UndoManagerClient {
 
   @override
   void dispose() {
+    if (UndoManager.client == this) {
+      UndoManager.client = null;
+    }
+
     widget.value.removeListener(_push);
     widget.focusNode.removeListener(_handleFocus);
     _effectiveController.onUndo.removeListener(undo);

diff --git a/packages/flutter/test/widgets/undo_history_test.dart b/packages/flutter/test/widgets/undo_history_test.dart
@@ -30,6 +30,120 @@ void main() {
     Future<void> sendUndo(WidgetTester tester) => sendUndoRedo(tester);
     Future<void> sendRedo(WidgetTester tester) => sendUndoRedo(tester, true);
 
+    testWidgets('UndoHistory widget registers as global undo/redo client', (WidgetTester tester) async {
+      final FocusNode focusNode = FocusNode(debugLabel: 'UndoHistory Node');
+      final GlobalKey undoHistoryGlobalKey = GlobalKey();
+      final ValueNotifier<int> value = ValueNotifier<int>(0);
+      addTearDown(value.dispose);
+
+      await tester.pumpWidget(
+        MaterialApp(
+          home: UndoHistory<int>(
+            key: undoHistoryGlobalKey,
+            value: value,
+            onTriggered: (_) {},
+            focusNode: focusNode,
+            child: Focus(
+              focusNode: focusNode,
+              child: Container(),
+            ),
+          ),
+        ),
+      );
+
+      // Initially the UndoHistory doesn't have focus, therefore it should
+      // not be the global undo/redo client. Ensure that's the case.
+      expect(UndoManager.client, isNull);
+
+      // Give focus to the UndoHistory widget.
+      focusNode.requestFocus();
+      await tester.pump();
+
+      // Now that the UndoHistory widget has focus, it should have registered
+      // itself as the global undo/redo client.
+      final State? undoHistoryState = undoHistoryGlobalKey.currentState;
+      expect(UndoManager.client, undoHistoryState);
+    });
+
+    testWidgets('UndoHistory widget deregisters as global undo/redo client when it loses focus',
+            (WidgetTester tester) async {
+          final FocusNode focusNode = FocusNode(debugLabel: 'UndoHistory Node');
+          final GlobalKey undoHistoryGlobalKey = GlobalKey();
+          final ValueNotifier<int> value = ValueNotifier<int>(0);
+          addTearDown(value.dispose);
+
+          await tester.pumpWidget(
+            MaterialApp(
+              home: UndoHistory<int>(
+                key: undoHistoryGlobalKey,
+                value: value,
+                onTriggered: (_) {},
+                focusNode: focusNode,
+                child: Focus(
+                  focusNode: focusNode,
+                  child: Container(),
+                ),
+              ),
+            ),
+          );
+
+          // Give focus to the UndoHistory widget.
+          focusNode.requestFocus();
+          await tester.pump();
+
+          // Ensure that UndoHistory is the global undo/redo client.
+          final State? undoHistoryState = undoHistoryGlobalKey.currentState;
+          expect(UndoManager.client, undoHistoryState);
+
+          // Remove focus from UndoHistory widget.
+          focusNode.unfocus();
+          await tester.pump();
+
+          // Ensure the UndoHistory widget is no longer the global client
+          expect(UndoManager.client, null);
+        });
+
+    testWidgets('UndoHistory widget deregisters as global undo/redo client when disposed', (WidgetTester tester) async {
+      final FocusNode focusNode = FocusNode(debugLabel: 'UndoHistory Node');
+      final GlobalKey undoHistoryGlobalKey = GlobalKey();
+      final ValueNotifier<int> value = ValueNotifier<int>(0);
+      addTearDown(value.dispose);
+
+      await tester.pumpWidget(
+        MaterialApp(
+          home: UndoHistory<int>(
+            key: undoHistoryGlobalKey,
+            value: value,
+            onTriggered: (_) {},
+            focusNode: focusNode,
+            child: Focus(
+              focusNode: focusNode,
+              child: Container(),
+            ),
+          ),
+        ),
+      );
+
+      // Give focus to the UndoHistory widget.
+      focusNode.requestFocus();
+      await tester.pump();
+
+      // Ensure that UndoHistory is the global undo/redo client.
+      final State? undoHistoryState = undoHistoryGlobalKey.currentState;
+      expect(UndoManager.client, undoHistoryState);
+
+      // Cause the UndoHistory widget to dispose its state.
+      await tester.pumpWidget(
+        const MaterialApp(
+          home: SizedBox(),
+        ),
+      );
+
+      // Ensure that the disposed UndoHistory state is not still the global
+      // undo/redo history client.
+      expect(UndoManager.client, isNull);
+    });
+
     testWidgets('allows undo and redo to be called programmatically from the UndoHistoryController', (WidgetTester tester) async {
       final ValueNotifier<int> value = ValueNotifier<int>(0);
       addTearDown(value.dispose);