firka/flutter
firka/flutter
5
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix leaks (#126144)

Browse Source 
Fixes https://github.com/flutter/flutter/issues/126096.
Fixes https://github.com/flutter/flutter/issues/126097.
Fixes https://github.com/flutter/flutter/issues/126102.
Fixes https://github.com/flutter/flutter/issues/126098.
Fixes https://github.com/flutter/flutter/issues/126147.
Work towards https://github.com/flutter/flutter/issues/126100.

Does not fix the OverlyEntry/ModalRoute leak (https://github.com/flutter/flutter/issues/126100).
This commit is contained in:
Michael Goderbauer 2023-05-08 11:11:23 +02:00 committed by GitHub
parent 65dfb555c0
commit 9b230d239a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 115 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
packages/flutter/lib/src/material/page_transitions_theme.dart
View File

@ -297,7 +297,7 @@ class _ZoomEnterTransition extends StatefulWidget {
State<_ZoomEnterTransition> createState() => _ZoomEnterTransitionState();
}
class _ZoomEnterTransitionState extends State<_ZoomEnterTransition> with _ZoomTransitionBase {
class _ZoomEnterTransitionState extends State<_ZoomEnterTransition> with _ZoomTransitionBase<_ZoomEnterTransition> {
// See SnapshotWidget doc comment, this is disabled on web because the HTML backend doesn't
// support this functionality and the canvaskit backend uses a single thread for UI and raster
// work which diminishes the impact of this performance improvement.
@ -406,7 +406,7 @@ class _ZoomExitTransition extends StatefulWidget {
State<_ZoomExitTransition> createState() => _ZoomExitTransitionState();
}
class _ZoomExitTransitionState extends State<_ZoomExitTransition> with _ZoomTransitionBase {
class _ZoomExitTransitionState extends State<_ZoomExitTransition> with _ZoomTransitionBase<_ZoomExitTransition> {
late _ZoomExitTransitionPainter delegate;
// See SnapshotWidget doc comment, this is disabled on web because the HTML backend doesn't
@ -830,7 +830,7 @@ void _updateScaledTransform(Matrix4 transform, double scale, Size size) {
transform.translate(-dx, -dy);
}
mixin _ZoomTransitionBase {
mixin _ZoomTransitionBase<S extends StatefulWidget> on State<S> {
bool get useSnapshot;
// Don't rasterize if:
@ -863,6 +863,12 @@ mixin _ZoomTransitionBase {
controller.allowSnapshotting = useSnapshot;
}
}
@override
void dispose() {
controller.dispose();
super.dispose();
}
}
class _ZoomEnterTransitionPainter extends SnapshotPainter {

1
packages/flutter/lib/src/widgets/drag_target.dart
View File

@ -885,6 +885,7 @@ class _DragAvatar<T extends Object> extends Drag {
_leaveAllEntered();
_activeTarget = null;
_entry!.remove();
_entry!.dispose();
_entry = null;
// TODO(ianh): consider passing _entry as well so the client can perform an animation.
onDragEnd?.call(velocity ?? Velocity.zero, _lastOffset!, wasAccepted);

4
packages/flutter/lib/src/widgets/navigator.dart
View File

@ -454,6 +454,7 @@ abstract class Route<T> {
@protected
void dispose() {
_navigator = null;
_restorationScopeId.dispose();
}
/// Whether this route is the top-most route on the navigator.
@ -3606,6 +3607,9 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
for (final _RouteEntry entry in _history) {
entry.dispose();
}
_rawNextPagelessRestorationScopeId.dispose();
_serializableHistory.dispose();
userGestureInProgressNotifier.dispose();
super.dispose();
// don't unlock, so that the object becomes unusable
assert(_debugLocked);

41
packages/flutter/lib/src/widgets/ticker_provider.dart
View File

@ -64,13 +64,13 @@ class TickerMode extends StatefulWidget {
return widget?.enabled ?? true;
}
/// Obtains a [ValueNotifier] from the [TickerMode] surrounding the `context`,
/// Obtains a [ValueListenable] from the [TickerMode] surrounding the `context`,
/// which indicates whether tickers are enabled in the given subtree.
///
/// When that [TickerMode] enabled or disabled tickers, the notifier notifies
/// When that [TickerMode] enabled or disabled tickers, the listenable notifies
/// its listeners.
///
/// While the [ValueNotifier] is stable for the lifetime of the surrounding
/// While the [ValueListenable] is stable for the lifetime of the surrounding
/// [TickerMode], calling this method does not establish a dependency between
/// the `context` and the [TickerMode] and the widget owning the `context`
/// does not rebuild when the ticker mode changes from true to false or vice
@ -79,7 +79,7 @@ class TickerMode extends StatefulWidget {
/// [Ticker]. Since no dependency is established, the widget owning the
/// `context` is also not informed when it is moved to a new location in the
/// tree where it may have a different [TickerMode] ancestor. When this
/// happens, the widget must manually unsubscribe from the old notifier,
/// happens, the widget must manually unsubscribe from the old listenable,
/// obtain a new one from the new ancestor [TickerMode] by calling this method
/// again, and re-subscribe to it. [StatefulWidget]s can, for example, do this
/// in [State.activate], which is called after the widget has been moved to
@ -93,10 +93,10 @@ class TickerMode extends StatefulWidget {
/// potential unnecessary rebuilds.
///
/// In the absence of a [TickerMode] widget, this function returns a
/// [ValueNotifier], whose [ValueNotifier.value] is always true.
static ValueNotifier<bool> getNotifier(BuildContext context) {
/// [ValueListenable], whose [ValueListenable.value] is always true.
static ValueListenable<bool> getNotifier(BuildContext context) {
final _EffectiveTickerMode? widget = context.getInheritedWidgetOfExactType<_EffectiveTickerMode>();
return widget?.notifier ?? ValueNotifier<bool>(true);
return widget?.notifier ?? const _ConstantValueListenable<bool>(true);
}
@override
@ -228,7 +228,7 @@ mixin SingleTickerProviderStateMixin<T extends StatefulWidget> on State<T> imple
super.dispose();
}
ValueNotifier<bool>? _tickerModeNotifier;
ValueListenable<bool>? _tickerModeNotifier;
@override
void activate() {
@ -245,7 +245,7 @@ mixin SingleTickerProviderStateMixin<T extends StatefulWidget> on State<T> imple
}
void _updateTickerModeNotifier() {
final ValueNotifier<bool> newNotifier = TickerMode.getNotifier(context);
final ValueListenable<bool> newNotifier = TickerMode.getNotifier(context);
if (newNotifier == _tickerModeNotifier) {
return;
}
@ -307,7 +307,7 @@ mixin TickerProviderStateMixin<T extends StatefulWidget> on State<T> implements
_tickers!.remove(ticker);
}
ValueNotifier<bool>? _tickerModeNotifier;
ValueListenable<bool>? _tickerModeNotifier;
@override
void activate() {
@ -327,7 +327,7 @@ mixin TickerProviderStateMixin<T extends StatefulWidget> on State<T> implements
}
void _updateTickerModeNotifier() {
final ValueNotifier<bool> newNotifier = TickerMode.getNotifier(context);
final ValueListenable<bool> newNotifier = TickerMode.getNotifier(context);
if (newNotifier == _tickerModeNotifier) {
return;
}
@ -395,3 +395,22 @@ class _WidgetTicker extends Ticker {
super.dispose();
}
}
class _ConstantValueListenable<T> implements ValueListenable<T> {
const _ConstantValueListenable(this.value);
@override
void addListener(VoidCallback listener) {
// Intentionally left empty: Value cannot change, so we never have to
// notify registered listeners.
}
@override
void removeListener(VoidCallback listener) {
// Intentionally left empty: Value cannot change, so we never have to
// notify registered listeners.
}
@override
final T value;
}

31
packages/flutter/test/animation/animation_sheet_test.dart
View File

@ -7,18 +7,20 @@
@Tags(<String>['reduced-test-set'])
library;
import 'dart:ui' as ui;
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import '../foundation/leak_tracking.dart';
void main() {
/*
* Here lies tests for packages/flutter_test/lib/src/animation_sheet.dart
* because [matchesGoldenFile] does not use Skia Gold in its native package.
*/
// TODO(polina-c): fix ValueNotifier not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('correctly records frames using display', (WidgetTester tester) async {
testWidgetsWithLeakTracking('correctly records frames using display', (WidgetTester tester) async {
final AnimationSheetBuilder builder = AnimationSheetBuilder(frameSize: _DecuplePixels.size);
await tester.pumpFrames(
@ -54,9 +56,7 @@ void main() {
await expectLater(find.byWidget(display), matchesGoldenFile('test.animation_sheet_builder.records.png'));
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001
// TODO(polina-c): fix ValueNotifier not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('correctly wraps a row', (WidgetTester tester) async {
testWidgetsWithLeakTracking('correctly wraps a row', (WidgetTester tester) async {
final AnimationSheetBuilder builder = AnimationSheetBuilder(frameSize: _DecuplePixels.size);
const Duration duration = Duration(seconds: 2);
@ -74,9 +74,7 @@ void main() {
await expectLater(find.byWidget(display), matchesGoldenFile('test.animation_sheet_builder.wraps.png'));
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001
// TODO(polina-c): fix Picture and Image not disposed and and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('correctly records frames using collate', (WidgetTester tester) async {
testWidgetsWithLeakTracking('correctly records frames using collate', (WidgetTester tester) async {
final AnimationSheetBuilder builder = AnimationSheetBuilder(frameSize: _DecuplePixels.size);
await tester.pumpFrames(
@ -104,15 +102,16 @@ void main() {
const Duration(milliseconds: 100),
);
final ui.Image image = await builder.collate(5);
await expectLater(
builder.collate(5),
image,
matchesGoldenFile('test.animation_sheet_builder.collate.png'),
);
image.dispose();
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001
// TODO(polina-c): fix Picture and Image not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('use allLayers to record out-of-subtree contents', (WidgetTester tester) async {
testWidgetsWithLeakTracking('use allLayers to record out-of-subtree contents', (WidgetTester tester) async {
final AnimationSheetBuilder builder = AnimationSheetBuilder(
frameSize: const Size(8, 2),
allLayers: true,
@ -137,12 +136,14 @@ void main() {
const Duration(milliseconds: 100),
);
final ui.Image image = await builder.collate(5);
await expectLater(
builder.collate(5),
image,
matchesGoldenFile('test.animation_sheet_builder.out_of_tree.png'),
);
image.dispose();
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001
}
// An animation of a yellow pixel moving from left to right, in a container of

11
packages/flutter/test/animation/live_binding_test.dart
View File

@ -7,6 +7,8 @@
@Tags(<String>['reduced-test-set'])
library;
import 'dart:ui' as ui;
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
@ -78,9 +80,7 @@ void main() {
// Currently skipped due to daily flake: https://github.com/flutter/flutter/issues/87588
}, skip: true); // Typically skip: isBrowser https://github.com/flutter/flutter/issues/42767
// TODO(polina-c): fix ValueNotifier not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('Should show event indicator for pointer events with setSurfaceSize', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Should show event indicator for pointer events with setSurfaceSize', (WidgetTester tester) async {
final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(200, 200), allLayers: true);
final List<Offset> taps = <Offset>[];
Widget target({bool recording = true}) => Container(
@ -132,9 +132,12 @@ void main() {
await tester.pumpFrames(target(), const Duration(milliseconds: 50));
expect(taps, isEmpty);
final ui.Image image = await animationSheet.collate(6);
await expectLater(
animationSheet.collate(6),
image,
matchesGoldenFile('LiveBinding.press.animation.2.png'),
);
image.dispose();
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001
}

2
packages/flutter/test/foundation/leak_tracking.dart
View File

@ -7,6 +7,8 @@ import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker/leak_tracker.dart';
import 'package:meta/meta.dart';
export 'package:leak_tracker/leak_tracker.dart' show LeakTrackingTestConfig, StackTraceCollectionConfig;
/// Set of objects, that does not hold the objects from garbage collection.
///
/// The objects are referenced by hash codes and can duplicate with low probability.

4
packages/flutter/test/gestures/gesture_config_regression_test.dart
View File

@ -114,9 +114,7 @@ void main() {
expect(result.dragUpdate, true);
});
// TODO(polina-c): fix ValueNotifier not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('Scroll Views get the same ScrollConfiguration as Draggables', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Scroll Views get the same ScrollConfiguration as Draggables', (WidgetTester tester) async {
tester.view.gestureSettings = const ui.GestureSettings(physicalTouchSlop: 4);
addTearDown(tester.view.reset);

32
packages/flutter/test/material/about_test.dart
View File

@ -150,9 +150,7 @@ void main() {
expect(find.text('About flutter_tester'), findsOneWidget);
});
// TODO(polina-c): fix SnapshotController not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('LicensePage control test', (WidgetTester tester) async {
testWidgetsWithLeakTracking('LicensePage control test', (WidgetTester tester) async {
LicenseRegistry.addLicense(() {
return Stream<LicenseEntry>.fromIterable(<LicenseEntry>[
const LicenseEntryWithLineBreaks(<String>['AAA'], 'BBB'),
@ -203,9 +201,7 @@ void main() {
expect(find.text('Another license'), findsOneWidget);
});
// TODO(polina-c): fix SnapshotController not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('LicensePage control test with all properties', (WidgetTester tester) async {
testWidgetsWithLeakTracking('LicensePage control test with all properties', (WidgetTester tester) async {
const FlutterLogo logo = FlutterLogo();
LicenseRegistry.addLicense(() {
@ -408,9 +404,7 @@ void main() {
);
});
// TODO(polina-c): fix SnapshotController not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('LicensePage returns early if unmounted', (WidgetTester tester) async {
testWidgetsWithLeakTracking('LicensePage returns early if unmounted', (WidgetTester tester) async {
final Completer<LicenseEntry> licenseCompleter = Completer<LicenseEntry>();
LicenseRegistry.addLicense(() {
return Stream<LicenseEntry>.fromFuture(licenseCompleter.future);
@ -433,11 +427,9 @@ void main() {
final FakeLicenseEntry licenseEntry = FakeLicenseEntry();
licenseCompleter.complete(licenseEntry);
expect(licenseEntry.packagesCalled, false);
});
}, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: <String>{'ValueNotifier<_OverlayEntryWidgetState?>'})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100.
// TODO(polina-c): fix SnapshotController not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('LicensePage returns late if unmounted', (WidgetTester tester) async {
testWidgetsWithLeakTracking('LicensePage returns late if unmounted', (WidgetTester tester) async {
final Completer<LicenseEntry> licenseCompleter = Completer<LicenseEntry>();
LicenseRegistry.addLicense(() {
return Stream<LicenseEntry>.fromFuture(licenseCompleter.future);
@ -460,7 +452,7 @@ void main() {
await tester.pumpAndSettle();
expect(licenseEntry.packagesCalled, true);
});
}, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: <String>{'ValueNotifier<_OverlayEntryWidgetState?>'})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100.
testWidgetsWithLeakTracking('LicensePage logic defaults to executable name for app name', (WidgetTester tester) async {
await tester.pumpWidget(
@ -1075,9 +1067,7 @@ void main() {
expect(find.text('Exception: Injected failure'), findsOneWidget);
});
// TODO(polina-c): fix SnapshotController not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('LicensePage master view layout position - ltr', (WidgetTester tester) async {
testWidgetsWithLeakTracking('LicensePage master view layout position - ltr', (WidgetTester tester) async {
const TextDirection textDirection = TextDirection.ltr;
const Size defaultSize = Size(800.0, 600.0);
const Size wideSize = Size(1200.0, 600.0);
@ -1138,11 +1128,9 @@ void main() {
// Configure to show the default layout.
await tester.binding.setSurfaceSize(defaultSize);
});
}, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: <String>{'ValueNotifier<_OverlayEntryWidgetState?>'})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100.
// TODO(polina-c): fix SnapshotController not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('LicensePage master view layout position - rtl', (WidgetTester tester) async {
testWidgetsWithLeakTracking('LicensePage master view layout position - rtl', (WidgetTester tester) async {
const TextDirection textDirection = TextDirection.rtl;
const Size defaultSize = Size(800.0, 600.0);
const Size wideSize = Size(1200.0, 600.0);
@ -1203,7 +1191,7 @@ void main() {
// Configure to show the default layout.
await tester.binding.setSurfaceSize(defaultSize);
});
}, leakTrackingConfig: const LeakTrackingTestConfig(notDisposedAllowList: <String>{'ValueNotifier<_OverlayEntryWidgetState?>'})); // TODO(goderbauer): Fix leak, https://github.com/flutter/flutter/issues/126100.
testWidgetsWithLeakTracking('License page title in lateral UI does not use AppBarTheme.foregroundColor', (WidgetTester tester) async {
// This is a regression test for https://github.com/flutter/flutter/issues/108991

6
packages/flutter/test/material/flutter_logo_test.dart
View File

@ -10,10 +10,10 @@ library;
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import '../foundation/leak_tracking.dart';
void main() {
// TODO(polina-c): fix Image not disposed and switch to testWidgetsWithLeakTracking.
// https://github.com/flutter/devtools/issues/3951
testWidgets('Flutter Logo golden test', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Flutter Logo golden test', (WidgetTester tester) async {
final Key logo = UniqueKey();
await tester.pumpWidget(FlutterLogo(key: logo));

7
packages/flutter_test/lib/src/_goldens_io.dart
View File

@ -201,13 +201,16 @@ Future<ComparisonResult> compareLists(List<int>? test, List<int>? master) async
final int height = testImage.height;
if (width != masterImage.width || height != masterImage.height) {
return ComparisonResult(
final ComparisonResult result = ComparisonResult(
passed: false,
diffPercent: 1.0,
error: 'Pixel test failed, image sizes do not match.\n'
'Master Image: ${masterImage.width} X ${masterImage.height}\n'
'Test Image: ${testImage.width} X ${testImage.height}',
);
masterImage.dispose();
testImage.dispose();
return result;
}
int pixelDiffCount = 0;
@ -264,6 +267,8 @@ Future<ComparisonResult> compareLists(List<int>? test, List<int>? master) async
},
);
}
masterImage.dispose();
testImage.dispose();
return ComparisonResult(passed: true, diffPercent: 0.0);
}

34
packages/flutter_test/lib/src/_matchers_io.dart
View File

@ -78,10 +78,13 @@ class MatchesGoldenFile extends AsyncMatcher {
}
}
Future<ui.Image?> imageFuture;
final bool disposeImage; // set to true if the matcher created and owns the image and must therefore dispose it.
if (item is Future<ui.Image?>) {
imageFuture = item;
disposeImage = false;
} else if (item is ui.Image) {
imageFuture = Future<ui.Image>.value(item);
disposeImage = false;
} else if (item is Finder) {
final Iterable<Element> elements = item.evaluate();
if (elements.isEmpty) {
@ -90,6 +93,7 @@ class MatchesGoldenFile extends AsyncMatcher {
return 'matched too many widgets';
}
imageFuture = captureImage(elements.single);
disposeImage = true;
} else {
throw AssertionError('must provide a Finder, Image, Future<Image>, List<int>, or Future<List<int>>');
}
@ -100,19 +104,25 @@ class MatchesGoldenFile extends AsyncMatcher {
if (image == null) {
throw AssertionError('Future<Image> completed to null');
}
final ByteData? bytes = await image.toByteData(format: ui.ImageByteFormat.png);
if (bytes == null) {
return 'could not encode screenshot.';
}
if (autoUpdateGoldenFiles) {
await goldenFileComparator.update(testNameUri, bytes.buffer.asUint8List());
return null;
}
try {
final bool success = await goldenFileComparator.compare(bytes.buffer.asUint8List(), testNameUri);
return success ? null : 'does not match';
} on TestFailure catch (ex) {
return ex.message;
final ByteData? bytes = await image.toByteData(format: ui.ImageByteFormat.png);
if (bytes == null) {
return 'could not encode screenshot.';
}
if (autoUpdateGoldenFiles) {
await goldenFileComparator.update(testNameUri, bytes.buffer.asUint8List());
return null;
}
try {
final bool success = await goldenFileComparator.compare(bytes.buffer.asUint8List(), testNameUri);
return success ? null : 'does not match';
} on TestFailure catch (ex) {
return ex.message;
}
} finally {
if (disposeImage) {
image.dispose();
}
}
}, additionalTime: const Duration(minutes: 1));
}

5
packages/flutter_test/lib/src/animation_sheet.dart
View File

@ -437,10 +437,13 @@ Future<ui.Image> _collateFrames(List<ui.Image> frames, Size frameSize, int cells
Paint(),
);
}
return recorder.endRecording().toImage(
final ui.Picture picture = recorder.endRecording();
final ui.Image image = await picture.toImage(
(frameSize.width * cellsPerRow).toInt(),
(frameSize.height * rowNum).toInt(),
);
picture.dispose();
return image;
}
// Layout children in a grid of fixed-sized cells.