From 301577a34f3f9614e5e866fb2d41372158dc0e89 Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Tue, 15 Aug 2023 14:58:21 -0700 Subject: [PATCH] Fixing a memory leak in About box/dialog overlays (#130842) ## Description Fix three memory leaks detected by `about_test.dart`, but were really in the `Route` and `OverlayEntry` classes. ## Related Issues - Fixes https://github.com/flutter/flutter/issues/130354 ## Tests - Updates about_test.dart to not ignore the leaks anymore. --- .../flutter/lib/src/widgets/navigator.dart | 8 +++--- packages/flutter/lib/src/widgets/overlay.dart | 27 +++++++++++-------- .../gesture_config_regression_test.dart | 6 ++++- .../flutter/test/material/about_test.dart | 20 ++------------ 4 files changed, 27 insertions(+), 34 deletions(-) diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index 43156aa44e26..82c4090037c1 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -2861,11 +2861,11 @@ class _RouteEntry extends RouteTransitionRecord { final _RestorationInformation? restorationInformation; final bool pageBased; - static Route notAnnounced = _NotAnnounced(); + static final Route notAnnounced = _NotAnnounced(); _RouteLifecycle currentState; Route? lastAnnouncedPreviousRoute = notAnnounced; // last argument to Route.didChangePrevious - Route lastAnnouncedPoppedNextRoute = notAnnounced; // last argument to Route.didPopNext + WeakReference> lastAnnouncedPoppedNextRoute = WeakReference>(notAnnounced); // last argument to Route.didPopNext Route? lastAnnouncedNextRoute = notAnnounced; // last argument to Route.didChangeNext /// Restoration ID to be used for the encapsulating route when restoration is @@ -2954,7 +2954,7 @@ class _RouteEntry extends RouteTransitionRecord { void handleDidPopNext(Route poppedRoute) { route.didPopNext(poppedRoute); - lastAnnouncedPoppedNextRoute = poppedRoute; + lastAnnouncedPoppedNextRoute = WeakReference>(poppedRoute); } /// Process the to-be-popped route. @@ -3153,7 +3153,7 @@ class _RouteEntry extends RouteTransitionRecord { // already announced this change by calling didPopNext. return !( nextRoute == null && - lastAnnouncedPoppedNextRoute == lastAnnouncedNextRoute + lastAnnouncedPoppedNextRoute.target == lastAnnouncedNextRoute ); } diff --git a/packages/flutter/lib/src/widgets/overlay.dart b/packages/flutter/lib/src/widgets/overlay.dart index f62b5fddc169..e88aafad20b6 100644 --- a/packages/flutter/lib/src/widgets/overlay.dart +++ b/packages/flutter/lib/src/widgets/overlay.dart @@ -135,20 +135,20 @@ class OverlayEntry implements Listenable { /// Whether the [OverlayEntry] is currently mounted in the widget tree. /// /// The [OverlayEntry] notifies its listeners when this value changes. - bool get mounted => _overlayEntryStateNotifier.value != null; + bool get mounted => _overlayEntryStateNotifier?.value != null; /// The currently mounted `_OverlayEntryWidgetState` built using this [OverlayEntry]. - final ValueNotifier<_OverlayEntryWidgetState?> _overlayEntryStateNotifier = ValueNotifier<_OverlayEntryWidgetState?>(null); + ValueNotifier<_OverlayEntryWidgetState?>? _overlayEntryStateNotifier = ValueNotifier<_OverlayEntryWidgetState?>(null); @override void addListener(VoidCallback listener) { assert(!_disposedByOwner); - _overlayEntryStateNotifier.addListener(listener); + _overlayEntryStateNotifier?.addListener(listener); } @override void removeListener(VoidCallback listener) { - _overlayEntryStateNotifier.removeListener(listener); + _overlayEntryStateNotifier?.removeListener(listener); } OverlayState? _overlay; @@ -194,7 +194,8 @@ class OverlayEntry implements Listenable { void _didUnmount() { assert(!mounted); if (_disposedByOwner) { - _overlayEntryStateNotifier.dispose(); + _overlayEntryStateNotifier?.dispose(); + _overlayEntryStateNotifier = null; } } @@ -217,7 +218,11 @@ class OverlayEntry implements Listenable { assert(_overlay == null, 'An OverlayEntry must first be removed from the Overlay before dispose is called.'); _disposedByOwner = true; if (!mounted) { - _overlayEntryStateNotifier.dispose(); + // If we're still mounted when disposed, then this will be disposed in + // _didUnmount, to allow notifications to occur until the entry is + // unmounted. + _overlayEntryStateNotifier?.dispose(); + _overlayEntryStateNotifier = null; } } @@ -315,7 +320,7 @@ class _OverlayEntryWidgetState extends State<_OverlayEntryWidget> { @override void initState() { super.initState(); - widget.entry._overlayEntryStateNotifier.value = this; + widget.entry._overlayEntryStateNotifier!.value = this; _theater = context.findAncestorRenderObjectOfType<_RenderTheater>()!; assert(_sortedTheaterSiblings == null); } @@ -335,7 +340,7 @@ class _OverlayEntryWidgetState extends State<_OverlayEntryWidget> { @override void dispose() { - widget.entry._overlayEntryStateNotifier.value = null; + widget.entry._overlayEntryStateNotifier?.value = null; widget.entry._didUnmount(); _sortedTheaterSiblings = null; super.dispose(); @@ -917,9 +922,9 @@ class _TheaterParentData extends StackParentData { // _overlayStateMounted is set to null in _OverlayEntryWidgetState's dispose // method. This property is only accessed during layout, paint and hit-test so // the `value!` should be safe. - Iterator? get paintOrderIterator => overlayEntry?._overlayEntryStateNotifier.value!._paintOrderIterable.iterator; - Iterator? get hitTestOrderIterator => overlayEntry?._overlayEntryStateNotifier.value!._hitTestOrderIterable.iterator; - void visitChildrenOfOverlayEntry(RenderObjectVisitor visitor) => overlayEntry?._overlayEntryStateNotifier.value!._paintOrderIterable.forEach(visitor); + Iterator? get paintOrderIterator => overlayEntry?._overlayEntryStateNotifier?.value!._paintOrderIterable.iterator; + Iterator? get hitTestOrderIterator => overlayEntry?._overlayEntryStateNotifier?.value!._hitTestOrderIterable.iterator; + void visitChildrenOfOverlayEntry(RenderObjectVisitor visitor) => overlayEntry?._overlayEntryStateNotifier?.value!._paintOrderIterable.forEach(visitor); } class _RenderTheater extends RenderBox with ContainerRenderObjectMixin, _RenderTheaterMixin { diff --git a/packages/flutter/test/gestures/gesture_config_regression_test.dart b/packages/flutter/test/gestures/gesture_config_regression_test.dart index b238b5b1033c..fd7569c2e548 100644 --- a/packages/flutter/test/gestures/gesture_config_regression_test.dart +++ b/packages/flutter/test/gestures/gesture_config_regression_test.dart @@ -11,6 +11,7 @@ import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart'; class TestResult { bool dragStarted = false; bool dragUpdate = false; + bool dragEnd = false; } class NestedScrollableCase extends StatelessWidget { @@ -77,7 +78,9 @@ class NestedDraggableCase extends StatelessWidget { onDragUpdate: (DragUpdateDetails details){ testResult.dragUpdate = true; }, - onDragEnd: (_) {}, + onDragEnd: (_) { + testResult.dragEnd = true; + }, ), ); }, @@ -134,5 +137,6 @@ void main() { expect(result.dragStarted, true); expect(result.dragUpdate, true); + expect(result.dragEnd, true); }); } diff --git a/packages/flutter/test/material/about_test.dart b/packages/flutter/test/material/about_test.dart index 7585853dc459..ddc3c2fd254f 100644 --- a/packages/flutter/test/material/about_test.dart +++ b/packages/flutter/test/material/about_test.dart @@ -282,15 +282,7 @@ void main() { await tester.tap(find.text('Another package')); await tester.pumpAndSettle(); expect(find.text('Another license'), findsOneWidget); - }, - leakTrackingTestConfig: const LeakTrackingTestConfig( - // TODO(polina-c): remove after fixing - // https://github.com/flutter/flutter/issues/130354 - notGCedAllowList: { - 'ValueNotifier<_OverlayEntryWidgetState?>': 2, - 'ValueNotifier': 1, - }, - )); + }); testWidgetsWithLeakTracking('LicensePage control test with all properties', (WidgetTester tester) async { const FlutterLogo logo = FlutterLogo(); @@ -366,15 +358,7 @@ void main() { await tester.tap(find.text('Another package')); await tester.pumpAndSettle(); expect(find.text('Another license'), findsOneWidget); - }, - leakTrackingTestConfig: const LeakTrackingTestConfig( - // TODO(polina-c): remove after fixing - // https://github.com/flutter/flutter/issues/130354 - notGCedAllowList: { - 'ValueNotifier<_OverlayEntryWidgetState?>':2, - 'ValueNotifier': 1, - }, - )); + }); testWidgetsWithLeakTracking('Material2 - _PackageLicensePage title style without AppBarTheme', (WidgetTester tester) async { LicenseRegistry.addLicense(() {