Unverified Commit 301577a3 authored by Greg Spencer's avatar Greg Spencer Committed by GitHub

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.
parent f0e7c518
...@@ -2861,11 +2861,11 @@ class _RouteEntry extends RouteTransitionRecord { ...@@ -2861,11 +2861,11 @@ class _RouteEntry extends RouteTransitionRecord {
final _RestorationInformation? restorationInformation; final _RestorationInformation? restorationInformation;
final bool pageBased; final bool pageBased;
static Route<dynamic> notAnnounced = _NotAnnounced(); static final Route<dynamic> notAnnounced = _NotAnnounced();
_RouteLifecycle currentState; _RouteLifecycle currentState;
Route<dynamic>? lastAnnouncedPreviousRoute = notAnnounced; // last argument to Route.didChangePrevious Route<dynamic>? lastAnnouncedPreviousRoute = notAnnounced; // last argument to Route.didChangePrevious
Route<dynamic> lastAnnouncedPoppedNextRoute = notAnnounced; // last argument to Route.didPopNext WeakReference<Route<dynamic>> lastAnnouncedPoppedNextRoute = WeakReference<Route<dynamic>>(notAnnounced); // last argument to Route.didPopNext
Route<dynamic>? lastAnnouncedNextRoute = notAnnounced; // last argument to Route.didChangeNext Route<dynamic>? lastAnnouncedNextRoute = notAnnounced; // last argument to Route.didChangeNext
/// Restoration ID to be used for the encapsulating route when restoration is /// Restoration ID to be used for the encapsulating route when restoration is
...@@ -2954,7 +2954,7 @@ class _RouteEntry extends RouteTransitionRecord { ...@@ -2954,7 +2954,7 @@ class _RouteEntry extends RouteTransitionRecord {
void handleDidPopNext(Route<dynamic> poppedRoute) { void handleDidPopNext(Route<dynamic> poppedRoute) {
route.didPopNext(poppedRoute); route.didPopNext(poppedRoute);
lastAnnouncedPoppedNextRoute = poppedRoute; lastAnnouncedPoppedNextRoute = WeakReference<Route<dynamic>>(poppedRoute);
} }
/// Process the to-be-popped route. /// Process the to-be-popped route.
...@@ -3153,7 +3153,7 @@ class _RouteEntry extends RouteTransitionRecord { ...@@ -3153,7 +3153,7 @@ class _RouteEntry extends RouteTransitionRecord {
// already announced this change by calling didPopNext. // already announced this change by calling didPopNext.
return !( return !(
nextRoute == null && nextRoute == null &&
lastAnnouncedPoppedNextRoute == lastAnnouncedNextRoute lastAnnouncedPoppedNextRoute.target == lastAnnouncedNextRoute
); );
} }
......
...@@ -135,20 +135,20 @@ class OverlayEntry implements Listenable { ...@@ -135,20 +135,20 @@ class OverlayEntry implements Listenable {
/// Whether the [OverlayEntry] is currently mounted in the widget tree. /// Whether the [OverlayEntry] is currently mounted in the widget tree.
/// ///
/// The [OverlayEntry] notifies its listeners when this value changes. /// 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]. /// The currently mounted `_OverlayEntryWidgetState` built using this [OverlayEntry].
final ValueNotifier<_OverlayEntryWidgetState?> _overlayEntryStateNotifier = ValueNotifier<_OverlayEntryWidgetState?>(null); ValueNotifier<_OverlayEntryWidgetState?>? _overlayEntryStateNotifier = ValueNotifier<_OverlayEntryWidgetState?>(null);
@override @override
void addListener(VoidCallback listener) { void addListener(VoidCallback listener) {
assert(!_disposedByOwner); assert(!_disposedByOwner);
_overlayEntryStateNotifier.addListener(listener); _overlayEntryStateNotifier?.addListener(listener);
} }
@override @override
void removeListener(VoidCallback listener) { void removeListener(VoidCallback listener) {
_overlayEntryStateNotifier.removeListener(listener); _overlayEntryStateNotifier?.removeListener(listener);
} }
OverlayState? _overlay; OverlayState? _overlay;
...@@ -194,7 +194,8 @@ class OverlayEntry implements Listenable { ...@@ -194,7 +194,8 @@ class OverlayEntry implements Listenable {
void _didUnmount() { void _didUnmount() {
assert(!mounted); assert(!mounted);
if (_disposedByOwner) { if (_disposedByOwner) {
_overlayEntryStateNotifier.dispose(); _overlayEntryStateNotifier?.dispose();
_overlayEntryStateNotifier = null;
} }
} }
...@@ -217,7 +218,11 @@ class OverlayEntry implements Listenable { ...@@ -217,7 +218,11 @@ class OverlayEntry implements Listenable {
assert(_overlay == null, 'An OverlayEntry must first be removed from the Overlay before dispose is called.'); assert(_overlay == null, 'An OverlayEntry must first be removed from the Overlay before dispose is called.');
_disposedByOwner = true; _disposedByOwner = true;
if (!mounted) { 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> { ...@@ -315,7 +320,7 @@ class _OverlayEntryWidgetState extends State<_OverlayEntryWidget> {
@override @override
void initState() { void initState() {
super.initState(); super.initState();
widget.entry._overlayEntryStateNotifier.value = this; widget.entry._overlayEntryStateNotifier!.value = this;
_theater = context.findAncestorRenderObjectOfType<_RenderTheater>()!; _theater = context.findAncestorRenderObjectOfType<_RenderTheater>()!;
assert(_sortedTheaterSiblings == null); assert(_sortedTheaterSiblings == null);
} }
...@@ -335,7 +340,7 @@ class _OverlayEntryWidgetState extends State<_OverlayEntryWidget> { ...@@ -335,7 +340,7 @@ class _OverlayEntryWidgetState extends State<_OverlayEntryWidget> {
@override @override
void dispose() { void dispose() {
widget.entry._overlayEntryStateNotifier.value = null; widget.entry._overlayEntryStateNotifier?.value = null;
widget.entry._didUnmount(); widget.entry._didUnmount();
_sortedTheaterSiblings = null; _sortedTheaterSiblings = null;
super.dispose(); super.dispose();
...@@ -917,9 +922,9 @@ class _TheaterParentData extends StackParentData { ...@@ -917,9 +922,9 @@ class _TheaterParentData extends StackParentData {
// _overlayStateMounted is set to null in _OverlayEntryWidgetState's dispose // _overlayStateMounted is set to null in _OverlayEntryWidgetState's dispose
// method. This property is only accessed during layout, paint and hit-test so // method. This property is only accessed during layout, paint and hit-test so
// the `value!` should be safe. // the `value!` should be safe.
Iterator<RenderBox>? get paintOrderIterator => overlayEntry?._overlayEntryStateNotifier.value!._paintOrderIterable.iterator; Iterator<RenderBox>? get paintOrderIterator => overlayEntry?._overlayEntryStateNotifier?.value!._paintOrderIterable.iterator;
Iterator<RenderBox>? get hitTestOrderIterator => overlayEntry?._overlayEntryStateNotifier.value!._hitTestOrderIterable.iterator; Iterator<RenderBox>? get hitTestOrderIterator => overlayEntry?._overlayEntryStateNotifier?.value!._hitTestOrderIterable.iterator;
void visitChildrenOfOverlayEntry(RenderObjectVisitor visitor) => overlayEntry?._overlayEntryStateNotifier.value!._paintOrderIterable.forEach(visitor); void visitChildrenOfOverlayEntry(RenderObjectVisitor visitor) => overlayEntry?._overlayEntryStateNotifier?.value!._paintOrderIterable.forEach(visitor);
} }
class _RenderTheater extends RenderBox with ContainerRenderObjectMixin<RenderBox, StackParentData>, _RenderTheaterMixin { class _RenderTheater extends RenderBox with ContainerRenderObjectMixin<RenderBox, StackParentData>, _RenderTheaterMixin {
......
...@@ -11,6 +11,7 @@ import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart'; ...@@ -11,6 +11,7 @@ import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';
class TestResult { class TestResult {
bool dragStarted = false; bool dragStarted = false;
bool dragUpdate = false; bool dragUpdate = false;
bool dragEnd = false;
} }
class NestedScrollableCase extends StatelessWidget { class NestedScrollableCase extends StatelessWidget {
...@@ -77,7 +78,9 @@ class NestedDraggableCase extends StatelessWidget { ...@@ -77,7 +78,9 @@ class NestedDraggableCase extends StatelessWidget {
onDragUpdate: (DragUpdateDetails details){ onDragUpdate: (DragUpdateDetails details){
testResult.dragUpdate = true; testResult.dragUpdate = true;
}, },
onDragEnd: (_) {}, onDragEnd: (_) {
testResult.dragEnd = true;
},
), ),
); );
}, },
...@@ -134,5 +137,6 @@ void main() { ...@@ -134,5 +137,6 @@ void main() {
expect(result.dragStarted, true); expect(result.dragStarted, true);
expect(result.dragUpdate, true); expect(result.dragUpdate, true);
expect(result.dragEnd, true);
}); });
} }
...@@ -282,15 +282,7 @@ void main() { ...@@ -282,15 +282,7 @@ void main() {
await tester.tap(find.text('Another package')); await tester.tap(find.text('Another package'));
await tester.pumpAndSettle(); await tester.pumpAndSettle();
expect(find.text('Another license'), findsOneWidget); expect(find.text('Another license'), findsOneWidget);
}, });
leakTrackingTestConfig: const LeakTrackingTestConfig(
// TODO(polina-c): remove after fixing
// https://github.com/flutter/flutter/issues/130354
notGCedAllowList: <String, int?>{
'ValueNotifier<_OverlayEntryWidgetState?>': 2,
'ValueNotifier<String?>': 1,
},
));
testWidgetsWithLeakTracking('LicensePage control test with all properties', (WidgetTester tester) async { testWidgetsWithLeakTracking('LicensePage control test with all properties', (WidgetTester tester) async {
const FlutterLogo logo = FlutterLogo(); const FlutterLogo logo = FlutterLogo();
...@@ -366,15 +358,7 @@ void main() { ...@@ -366,15 +358,7 @@ void main() {
await tester.tap(find.text('Another package')); await tester.tap(find.text('Another package'));
await tester.pumpAndSettle(); await tester.pumpAndSettle();
expect(find.text('Another license'), findsOneWidget); expect(find.text('Another license'), findsOneWidget);
}, });
leakTrackingTestConfig: const LeakTrackingTestConfig(
// TODO(polina-c): remove after fixing
// https://github.com/flutter/flutter/issues/130354
notGCedAllowList: <String, int?>{
'ValueNotifier<_OverlayEntryWidgetState?>':2,
'ValueNotifier<String?>': 1,
},
));
testWidgetsWithLeakTracking('Material2 - _PackageLicensePage title style without AppBarTheme', (WidgetTester tester) async { testWidgetsWithLeakTracking('Material2 - _PackageLicensePage title style without AppBarTheme', (WidgetTester tester) async {
LicenseRegistry.addLicense(() { LicenseRegistry.addLicense(() {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment