Skip to content

Commit

Permalink
Fix memory leak in ListWheelScrollView (#134732)
Browse files Browse the repository at this point in the history
  • Loading branch information
ksokolovskyi authored Sep 15, 2023
1 parent 1e4a1be commit 09acfe6
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 24 deletions.
21 changes: 8 additions & 13 deletions packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -709,28 +708,24 @@ class ListWheelScrollView extends StatefulWidget {

class _ListWheelScrollViewState extends State<ListWheelScrollView> {
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;
}
}

@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) {
Expand All @@ -754,7 +749,7 @@ class _ListWheelScrollViewState extends State<ListWheelScrollView> {
return NotificationListener<ScrollNotification>(
onNotification: _handleScrollNotification,
child: _FixedExtentScrollable(
controller: scrollController,
controller: _effectiveController,
physics: widget.physics,
itemExtent: widget.itemExtent,
restorationId: widget.restorationId,
Expand Down
103 changes: 92 additions & 11 deletions packages/flutter/test/widgets/list_wheel_scroll_view_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand All @@ -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<Widget>.generate(100, (int index) {
return const Placeholder();
Expand All @@ -1320,17 +1323,94 @@ 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);

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<Widget>.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,
child: ListWheelScrollView(
itemExtent: 100.0,
children: List<Widget>.generate(100, (int index) {
return const Placeholder();
}),
),
),
);

// 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<Widget>.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);

// Now remove the controller
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ListWheelScrollView(
controller: controller,
itemExtent: 100.0,
children: List<Widget>.generate(100, (int index) {
return const Placeholder();
Expand All @@ -1339,9 +1419,10 @@ void main() {
),
);

// Internally, that same controller is still attached and still at the
// same place.
expect(newController.selectedItem, 50);
// Controller is now attached again.
expect(controller.hasClients, isTrue);
expect(controller.selectedItem, 3);
expect(controller.position.pixels, 300.0);
});
});

Expand Down

0 comments on commit 09acfe6

Please sign in to comment.