From 1cb003b8fb45321421af69f22162c3e45f12679d Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Sat, 22 Jun 2024 16:41:39 -0700 Subject: [PATCH] Fix: Memory leak in UndoHistory widget because it never de-registered itself as global UndoManager client (Resolves #148291) (#150661) Unsets a global `client` variable that was missed. --- .../flutter/lib/src/widgets/undo_history.dart | 8 ++ .../test/widgets/undo_history_test.dart | 114 ++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/packages/flutter/lib/src/widgets/undo_history.dart b/packages/flutter/lib/src/widgets/undo_history.dart index ab37737cfcd70..d9bfd44a02612 100644 --- a/packages/flutter/lib/src/widgets/undo_history.dart +++ b/packages/flutter/lib/src/widgets/undo_history.dart @@ -199,6 +199,10 @@ class UndoHistoryState extends State> 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 extends State> 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 index 6cbb3a377c1c9..d4f5b083e896b 100644 --- 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 sendUndo(WidgetTester tester) => sendUndoRedo(tester); Future 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 value = ValueNotifier(0); + addTearDown(value.dispose); + + await tester.pumpWidget( + MaterialApp( + home: UndoHistory( + 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 value = ValueNotifier(0); + addTearDown(value.dispose); + + await tester.pumpWidget( + MaterialApp( + home: UndoHistory( + 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 value = ValueNotifier(0); + addTearDown(value.dispose); + + await tester.pumpWidget( + MaterialApp( + home: UndoHistory( + 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 value = ValueNotifier(0); addTearDown(value.dispose);