[vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] by ValentinVignal · Pull Request #8373 · flutter/packages

ValentinVignal · 2025-01-06T15:05:56Z

The package manipulates disposable objects. This PR

Fixes some memory leaks.
Activates leak testing to make sure disposable objects are correctly disposed.

See the documentation: https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ValentinVignal · 2025-01-06T15:06:07Z

cc @polina-c

jonahwilliams · 2025-01-06T19:29:42Z

packages/vector_graphics/lib/src/listener.dart

I don't really understand what this is doing?

For the test Can render VG with image from test/vector_graphics_test.dart, leak_tracker_flutter_testing was not happy because the ImageInfo was not disposed from this callback was never disposed.

So here I am disposing it. However, disposing it also disposes it image.image, that's why I'm cloning it first.

Expected: leak free Actual: <Instance of 'Leaks'> Which: contains leaks: # The text is generated by leak_tracker. # For leak troubleshooting tips open: # https://github.com/dart-lang/leak_tracker/blob/main/doc/leak_tracking/TROUBLESHOOT.md notDisposed: total: 1 objects: ImageInfo: test: Can render VG with image identityHashCode: 137082098 context: start: > #6_______dispatchFlutterEventToLeakTracker_(package:leak_tracker_flutter_testing/src/testing.dart:74:23) #7______FlutterMemoryAllocations.dispatchObjectEvent_(package:flutter/src/foundation/memory_allocations.dart:249:23) #8______FlutterMemoryAllocations.dispatchObjectCreated_(package:flutter/src/foundation/memory_allocations.dart:283:5) #9______new_ImageInfo_(package:flutter/src/painting/image_stream.dart:51:34) #10_____ImageInfo.clone_(package:flutter/src/painting/image_stream.dart:72:12) #11_____ImageStreamCompleter.setImage_(package:flutter/src/painting/image_stream.dart:755:32) #12_____StackZoneSpecification._registerUnaryCallback.<anonymous_closure>.<anonymous_closure>_(package:stack_trace/src/stack_zone_specification.dart:127:36) #13_____StackZoneSpecification._run_(package:stack_trace/src/stack_zone_specification.dart:207:15) #14_____StackZoneSpecification._registerUnaryCallback.<anonymous_closure>_(package:stack_trace/src/stack_zone_specification.dart:127:24) #15______rootRunUnary_(dart:async/zone.dart:1422:47) #16______CustomZone.runUnary_(dart:async/zone.dart:1324:19) #17_____Future._propagateToListeners.handleValueCallback_(dart:async/future_impl.dart:902:45) #18_____Future._propagateToListeners_(dart:async/future_impl.dart:931:13) #19_____Future._completeWithValue_(dart:async/future_impl.dart:707:5) <asynchronous_suspension> package:matcher expect package:leak_tracker_testing/src/leak_testing.dart 69:24 LeakTesting.collectedLeaksReporter.<fn> package:leak_tracker_flutter_testing/src/testing.dart 70:37 maybeTearDownLeakTrackingForAll ===== asynchronous gap =========================== dart:async _CustomZone.registerBinaryCallback package:flutter_test/src/test_compat.dart 287:3 _tearDownForTestFile

Is there a better way you have in mind?

What are your thoughts on this @jonahwilliams ?

Making changes to the library to work around leak tracker is not reasonable.

The way I understood it was that the leak tracker detected a memory leak in the library (ImageInfo is never being disposed) and what I did here was an attempt to fix this memory leak.

Are you saying this is

Not a memory leak but a false positive? In that cases where is the ImageInfo usually disposed in production and would you know what is missing to the test to get it disposed too?

This is a memory leak but my fix is not the proper way to do it? In that case, would you have suggestions of what I could do instead?

I don't know if this is a memory leak or a false positive (likely the latter), but even if this was memory leak I don't understand how to proposed change fixes it. it seems like what you're actually doing is working around leak tracker.

For context, this is the failing test:

packages/packages/vector_graphics/test/vector_graphics_test.dart

Lines 533 to 592 in d6705de

testWidgets('Can render VG with image', (WidgetTester tester) async {

const String bluePngPixel =

'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPj/HwADBwIAMCbHYQAAAABJRU5ErkJggg==';

final VectorGraphicsBuffer buffer = VectorGraphicsBuffer();

const VectorGraphicsCodec codec = VectorGraphicsCodec();

codec.writeSize(buffer, 100, 100);

codec.writeDrawImage(

buffer,

codec.writeImage(buffer, 0, base64Decode(bluePngPixel)),

0,

0,

100,

100,

null,

);

final UniqueKey key = UniqueKey();

final TestBytesLoader loader = TestBytesLoader(buffer.done());

// See listener.dart.

final int imageKey = Object.hash(loader.hashCode, 0, 0);

expect(imageCache.currentSize, 0);

expect(imageCache.statusForKey(imageKey).untracked, true);

await tester.pumpWidget(RepaintBoundary(

key: key,

child: VectorGraphic(

loader: loader,

width: 100,

height: 100,

),

));

expect(imageCache.currentSize, 0);

expect(imageCache.statusForKey(imageKey).pending, true);

// A blank image, because the image hasn't loaded yet.

await expectLater(

find.byKey(key),

matchesGoldenFile('vg_with_image_blank.png'),

);

expect(imageCache.currentSize, 1);

expect(imageCache.statusForKey(imageKey).live, false);

expect(imageCache.statusForKey(imageKey).keepAlive, true);

await tester.runAsync(() => vg.waitForPendingDecodes());

await tester.pump();

expect(imageCache.currentSize, 1);

expect(imageCache.statusForKey(imageKey).live, false);

expect(imageCache.statusForKey(imageKey).keepAlive, true);

// A blue square, becuase the image is available now.

await expectLater(

find.byKey(key),

matchesGoldenFile('vg_with_image_blue.png'),

);

}, skip: kIsWeb);

At the end of this test, an ImageInfo remains undisposed. At first glance, I don't see anything wrong with the test itself, so this appears to be a memory leak.

Currently, FlutterVectorGraphicsListener only stores image.image and disposes of it in its toPicture method, leaving the outer ImageInfo unmanaged. I attempted to address this by disposing of the ImageInfo immediately.

Would it make sense to store the ImageInfo instead of just the Image to ensure everything gets properly disposed of in FlutterVectorGraphicsListener.toPicture?

I pushed the change in Store and dispose ImageInfo

That makes more sense

jonahwilliams · 2025-01-28T02:42:32Z

packages/vector_graphics/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 1.1.16
+
+* Fixes some memory leaks.


Please add a more descriptive changelog entry.

Sure, what do you think of More description changelog?

jonahwilliams

LGTM

…[prod-leak-fix] (flutter/packages#8373)

…[prod-leak-fix] (#8373)" This reverts commit 4e01110.

…[prod-leak-fix] (flutter/packages#8373)

flutter/packages@02c6fef...e6ce02c 2025-02-05 [email protected] [vector_graphics] Allow transition between placeholder and loaded image to have an animation (flutter/packages#8195) 2025-02-04 [email protected] [flutter_markdown] Make custom table column alignments work when text wraps (flutter/packages#8340) 2025-02-04 [email protected] [interactive_media_ads] Adds internal wrapper for iOS native `IMAAdPodInfo` (flutter/packages#8429) 2025-02-03 [email protected] [pigeon] reorg generator files (flutter/packages#8532) 2025-02-03 [email protected] [pigeon] [swift] Fix `PigeonError` sendability conformance in Swift 6 (flutter/packages#8302) 2025-02-03 [email protected] Roll Flutter from b007899 to 8e2a6fc (61 revisions) (flutter/packages#8556) 2025-02-03 [email protected] [google_maps_flutter] Support for Ground Overlay - platform interface (flutter/packages#8518) 2025-01-31 [email protected] [tool] Add --xcode-warnings-exceptions flag (flutter/packages#8524) 2025-01-31 [email protected] [tool] Ensure that injected dependency overrides are sorted (flutter/packages#8542) 2025-01-31 [email protected] [vector_graphics] Revert leak tracker change (flutter/packages#8544) 2025-01-31 [email protected] [shared_preferences_tool] Loosen vm_service constraint to allow for 15 (flutter/packages#8539) 2025-01-31 [email protected] [in_app_purchase] Activate leak testing for android (flutter/packages#8369) 2025-01-31 [email protected] [flutter_markdown] Allow tables to be scrollable with IntrinsicColumnWidth (flutter/packages#8526) 2025-01-30 [email protected] Update CODEOWNERS for pkg:animations (flutter/packages#8534) 2025-01-30 [email protected] Roll Flutter from c1ffaa9 to b007899 (43 revisions) (flutter/packages#8527) 2025-01-30 [email protected] [video_player_web] Adjust Web implementation to the new platform interface (flutter/packages#8528) 2025-01-30 [email protected] [shared_preferences] Exposed SharedPreferencesOptions. (flutter/packages#8530) 2025-01-29 [email protected] Re-land [shared_preferences] Add shared preferences devtool (flutter/packages#8531) 2025-01-29 [email protected] [in_app_purchase_storekit] Add Swift Package Manager compatibility (flutter/packages#8469) 2025-01-29 [email protected] Revert "Re-land [shared_preferences] Add shared preferences devtool" (flutter/packages#8529) 2025-01-29 [email protected] [go_router_builder] Fixes trailing `?` by comparing iterables (flutter/packages#8521) 2025-01-29 [email protected] [tool] Refactor args of strings or YAML file lists (flutter/packages#8513) 2025-01-28 [email protected] [go_router] Add missing await keyword to onTap callback in the code example in `navigation.md` (flutter/packages#8343) 2025-01-28 [email protected] Re-land [shared_preferences] Add shared preferences devtool (flutter/packages#8519) 2025-01-28 [email protected] [vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] (flutter/packages#8373) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ak-fix] (flutter#8373) The package manipulates disposable objects. This PR - Fixes some memory leaks. - Activates leak testing to make sure disposable objects are correctly disposed. See the documentation: https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md

ValentinVignal requested a review from jonahwilliams as a code owner January 6, 2025 15:05

github-actions bot added the p: vector_graphics label Jan 6, 2025

jonahwilliams reviewed Jan 6, 2025

View reviewed changes

ValentinVignal requested a review from jonahwilliams January 7, 2025 01:17

polina-c changed the title ~~[vector_graphics] Fix memory leaks and activate leak testing~~ [vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] Jan 7, 2025

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Jan 7, 2025

ValentinVignal force-pushed the vector-graphics/activate-leak-testing branch from a456583 to 49637d0 Compare January 15, 2025 13:47

ValentinVignal added 4 commits January 18, 2025 13:23

fix memory leaks

e6789c8

Activate leak testing

5c997e1

Fix the tests

cce339e

Update version number

08792f3

ValentinVignal force-pushed the vector-graphics/activate-leak-testing branch from 49637d0 to 08792f3 Compare January 18, 2025 05:23

Store and dispose ImageInfo

86fc536

jonahwilliams reviewed Jan 28, 2025

View reviewed changes

More description changelog

f1a0d21

ValentinVignal requested a review from jonahwilliams January 28, 2025 03:32

jonahwilliams approved these changes Jan 28, 2025

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 28, 2025

auto-submit bot merged commit 4e01110 into flutter:main Jan 28, 2025
76 checks passed

engine-flutter-autoroll mentioned this pull request Jan 29, 2025

Roll Packages from 02c6fef4ef63 to dc5707949719 (5 revisions) flutter/flutter#162384

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 29, 2025

4e01110 [vector_graphics] Fix memory leaks and activate leak testing …

d6348f0

…[prod-leak-fix] (flutter/packages#8373)

jonahwilliams pushed a commit that referenced this pull request Jan 31, 2025

Revert "[vector_graphics] Fix memory leaks and activate leak testing …

31cdde3

…[prod-leak-fix] (#8373)" This reverts commit 4e01110.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2025

4e01110 [vector_graphics] Fix memory leaks and activate leak testing …

1e78840

…[prod-leak-fix] (flutter/packages#8373)

engine-flutter-autoroll mentioned this pull request Feb 5, 2025

Roll Packages from 02c6fef4ef63 to e6ce02c2a859 (25 revisions) flutter/flutter#162744

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2025

4e01110 [vector_graphics] Fix memory leaks and activate leak testing …

9639cab

…[prod-leak-fix] (flutter/packages#8373)

engine-flutter-autoroll mentioned this pull request Feb 5, 2025

Roll Packages from 02c6fef4ef63 to e6ce02c2a859 (25 revisions) flutter/flutter#162760

Merged

	testWidgets('Can render VG with image', (WidgetTester tester) async {
	const String bluePngPixel =
	'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPj/HwADBwIAMCbHYQAAAABJRU5ErkJggg==';

	final VectorGraphicsBuffer buffer = VectorGraphicsBuffer();
	const VectorGraphicsCodec codec = VectorGraphicsCodec();
	codec.writeSize(buffer, 100, 100);

	codec.writeDrawImage(
	buffer,
	codec.writeImage(buffer, 0, base64Decode(bluePngPixel)),
	0,
	0,
	100,
	100,
	null,
	);
	final UniqueKey key = UniqueKey();
	final TestBytesLoader loader = TestBytesLoader(buffer.done());
	// See listener.dart.
	final int imageKey = Object.hash(loader.hashCode, 0, 0);

	expect(imageCache.currentSize, 0);
	expect(imageCache.statusForKey(imageKey).untracked, true);

	await tester.pumpWidget(RepaintBoundary(
	key: key,
	child: VectorGraphic(
	loader: loader,
	width: 100,
	height: 100,
	),
	));

	expect(imageCache.currentSize, 0);
	expect(imageCache.statusForKey(imageKey).pending, true);

	// A blank image, because the image hasn't loaded yet.
	await expectLater(
	find.byKey(key),
	matchesGoldenFile('vg_with_image_blank.png'),
	);

	expect(imageCache.currentSize, 1);
	expect(imageCache.statusForKey(imageKey).live, false);
	expect(imageCache.statusForKey(imageKey).keepAlive, true);

	await tester.runAsync(() => vg.waitForPendingDecodes());
	await tester.pump();

	expect(imageCache.currentSize, 1);
	expect(imageCache.statusForKey(imageKey).live, false);
	expect(imageCache.statusForKey(imageKey).keepAlive, true);

	// A blue square, becuase the image is available now.
	await expectLater(
	find.byKey(key),
	matchesGoldenFile('vg_with_image_blue.png'),
	);
	}, skip: kIsWeb);

Conversation

ValentinVignal commented Jan 6, 2025

Pre-launch Checklist

Uh oh!

ValentinVignal commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants