From 09acfe6341e51d306b44dcd047f4bc041f702122 Mon Sep 17 00:00:00 2001 From: Kostia Sokolovskyi Date: Fri, 15 Sep 2023 05:52:22 +0200 Subject: [PATCH] Fix memory leak in ListWheelScrollView (#134732) --- .../src/widgets/list_wheel_scroll_view.dart | 21 ++-- .../widgets/list_wheel_scroll_view_test.dart | 103 ++++++++++++++++-- 2 files changed, 100 insertions(+), 24 deletions(-) diff --git a/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart b/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart index d8d43ef7bf..96f56b2d02 100644 --- a/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart +++ b/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart @@ -7,7 +7,6 @@ import 'dart:math' as math; import 'package:flutter/physics.dart'; import 'package:flutter/rendering.dart'; -import 'package:flutter/scheduler.dart'; import 'basic.dart'; import 'framework.dart'; @@ -709,12 +708,14 @@ class ListWheelScrollView extends StatefulWidget { class _ListWheelScrollViewState extends State { int _lastReportedItemIndex = 0; - ScrollController? scrollController; + ScrollController? _backupController; + + ScrollController get _effectiveController => + widget.controller ?? (_backupController ??= FixedExtentScrollController()); @override void initState() { super.initState(); - scrollController = widget.controller ?? FixedExtentScrollController(); if (widget.controller is FixedExtentScrollController) { final FixedExtentScrollController controller = widget.controller! as FixedExtentScrollController; _lastReportedItemIndex = controller.initialItem; @@ -722,15 +723,9 @@ class _ListWheelScrollViewState extends State { } @override - void didUpdateWidget(ListWheelScrollView oldWidget) { - super.didUpdateWidget(oldWidget); - if (widget.controller != null && widget.controller != scrollController) { - final ScrollController? oldScrollController = scrollController; - SchedulerBinding.instance.addPostFrameCallback((_) { - oldScrollController!.dispose(); - }); - scrollController = widget.controller; - } + void dispose() { + _backupController?.dispose(); + super.dispose(); } bool _handleScrollNotification(ScrollNotification notification) { @@ -754,7 +749,7 @@ class _ListWheelScrollViewState extends State { return NotificationListener( onNotification: _handleScrollNotification, child: _FixedExtentScrollable( - controller: scrollController, + controller: _effectiveController, physics: widget.physics, itemExtent: widget.itemExtent, restorationId: widget.restorationId, diff --git a/packages/flutter/test/widgets/list_wheel_scroll_view_test.dart b/packages/flutter/test/widgets/list_wheel_scroll_view_test.dart index a0e14026f8..ef2361d500 100644 --- a/packages/flutter/test/widgets/list_wheel_scroll_view_test.dart +++ b/packages/flutter/test/widgets/list_wheel_scroll_view_test.dart @@ -11,6 +11,7 @@ import 'package:flutter/foundation.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart'; import '../rendering/rendering_tester.dart' show TestCallbackPainter, TestClipPaintingContext; @@ -1285,7 +1286,7 @@ void main() { expect(controller.selectedItem, 10); }); - testWidgets('controller hot swappable', (WidgetTester tester) async { + testWidgetsWithLeakTracking('controller hot swappable', (WidgetTester tester) async { await tester.pumpWidget( Directionality( textDirection: TextDirection.ltr, @@ -1302,14 +1303,16 @@ void main() { await tester.drag(find.byType(ListWheelScrollView), const Offset(0.0, -500.0)); await tester.pump(); - final FixedExtentScrollController newController = + final FixedExtentScrollController controller1 = FixedExtentScrollController(initialItem: 30); + addTearDown(controller1.dispose); + // Attaching first controller. await tester.pumpWidget( Directionality( textDirection: TextDirection.ltr, child: ListWheelScrollView( - controller: newController, + controller: controller1, itemExtent: 100.0, children: List.generate(100, (int index) { return const Placeholder(); @@ -1320,13 +1323,41 @@ void main() { // initialItem doesn't do anything since the scroll position was already // created. - expect(newController.selectedItem, 5); + expect(controller1.selectedItem, 5); - newController.jumpToItem(50); - expect(newController.selectedItem, 50); - expect(newController.position.pixels, 5000.0); + controller1.jumpToItem(50); + expect(controller1.selectedItem, 50); + expect(controller1.position.pixels, 5000.0); - // Now remove the controller + final FixedExtentScrollController controller2 = + FixedExtentScrollController(initialItem: 33); + addTearDown(controller2.dispose); + + // Attaching the second controller. + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: ListWheelScrollView( + controller: controller2, + itemExtent: 100.0, + children: List.generate(100, (int index) { + return const Placeholder(); + }), + ), + ), + ); + + // First controller is now detached. + expect(controller1.hasClients, isFalse); + // initialItem doesn't do anything since the scroll position was already + // created. + expect(controller2.selectedItem, 50); + + controller2.jumpToItem(40); + expect(controller2.selectedItem, 40); + expect(controller2.position.pixels, 4000.0); + + // Now, use the internal controller. await tester.pumpWidget( Directionality( textDirection: TextDirection.ltr, @@ -1339,9 +1370,59 @@ void main() { ), ); - // Internally, that same controller is still attached and still at the - // same place. - expect(newController.selectedItem, 50); + // Both controllers are now detached. + expect(controller1.hasClients, isFalse); + expect(controller2.hasClients, isFalse); + }); + + testWidgetsWithLeakTracking('controller can be reused', (WidgetTester tester) async { + final FixedExtentScrollController controller = + FixedExtentScrollController(initialItem: 3); + addTearDown(controller.dispose); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: ListWheelScrollView( + controller: controller, + itemExtent: 100.0, + children: List.generate(100, (int index) { + return const Placeholder(); + }), + ), + ), + ); + + // selectedItem is equal to the initialItem. + expect(controller.selectedItem, 3); + expect(controller.position.pixels, 300.0); + + controller.jumpToItem(10); + expect(controller.selectedItem, 10); + expect(controller.position.pixels, 1000.0); + + await tester.pumpWidget(const Center()); + + // Controller is now detached. + expect(controller.hasClients, isFalse); + + await tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: ListWheelScrollView( + controller: controller, + itemExtent: 100.0, + children: List.generate(100, (int index) { + return const Placeholder(); + }), + ), + ), + ); + + // Controller is now attached again. + expect(controller.hasClients, isTrue); + expect(controller.selectedItem, 3); + expect(controller.position.pixels, 300.0); }); });