[vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] #8373

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

@@ -746,7 +746,8 @@ class FlutterVectorGraphicsListener extends VectorGraphicsCodecListener {
    listener = ImageStreamListener(
      (ImageInfo image, bool synchronousCall) {
        cacheCompleter.removeListener(listener);
-        _images[imageId] = image.image;
+        _images[imageId] = image.image.clone();


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

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

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

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

flutter/packages@02c6fef...e6ce02c 2025-02-05 65381000+raju8000@users.noreply.github.com [vector_graphics] Allow transition between placeholder and loaded image to have an animation (flutter/packages#8195) 2025-02-04 andrechalella@gmail.com [flutter_markdown] Make custom table column alignments work when text wraps (flutter/packages#8340) 2025-02-04 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds internal wrapper for iOS native `IMAAdPodInfo` (flutter/packages#8429) 2025-02-03 tarrinneal@gmail.com [pigeon] reorg generator files (flutter/packages#8532) 2025-02-03 byoungchan.lee@gmx.com [pigeon] [swift] Fix `PigeonError` sendability conformance in Swift 6 (flutter/packages#8302) 2025-02-03 engine-flutter-autoroll@skia.org Roll Flutter from b007899 to 8e2a6fc (61 revisions) (flutter/packages#8556) 2025-02-03 20989940+aednlaxer@users.noreply.github.com [google_maps_flutter] Support for Ground Overlay - platform interface (flutter/packages#8518) 2025-01-31 737941+loic-sharma@users.noreply.github.com [tool] Add --xcode-warnings-exceptions flag (flutter/packages#8524) 2025-01-31 stuartmorgan@google.com [tool] Ensure that injected dependency overrides are sorted (flutter/packages#8542) 2025-01-31 jonahwilliams@google.com [vector_graphics] Revert leak tracker change (flutter/packages#8544) 2025-01-31 parlough@gmail.com [shared_preferences_tool] Loosen vm_service constraint to allow for 15 (flutter/packages#8539) 2025-01-31 32538273+ValentinVignal@users.noreply.github.com [in_app_purchase] Activate leak testing for android (flutter/packages#8369) 2025-01-31 cunderw@gmail.com [flutter_markdown] Allow tables to be scrollable with IntrinsicColumnWidth (flutter/packages#8526) 2025-01-30 goderbauer@google.com Update CODEOWNERS for pkg:animations (flutter/packages#8534) 2025-01-30 engine-flutter-autoroll@skia.org Roll Flutter from c1ffaa9 to b007899 (43 revisions) (flutter/packages#8527) 2025-01-30 pawel.jakubowski@leancode.pl [video_player_web] Adjust Web implementation to the new platform interface (flutter/packages#8528) 2025-01-30 tarrinneal@gmail.com [shared_preferences] Exposed SharedPreferencesOptions. (flutter/packages#8530) 2025-01-29 stuartmorgan@google.com Re-land [shared_preferences] Add shared preferences devtool (flutter/packages#8531) 2025-01-29 84978733+alejandro-all-win-software@users.noreply.github.com [in_app_purchase_storekit] Add Swift Package Manager compatibility (flutter/packages#8469) 2025-01-29 stuartmorgan@google.com Revert "Re-land [shared_preferences] Add shared preferences devtool" (flutter/packages#8529) 2025-01-29 32538273+ValentinVignal@users.noreply.github.com [go_router_builder] Fixes trailing `?` by comparing iterables (flutter/packages#8521) 2025-01-29 737941+loic-sharma@users.noreply.github.com [tool] Refactor args of strings or YAML file lists (flutter/packages#8513) 2025-01-28 48155875+Michae1Weiss@users.noreply.github.com [go_router] Add missing await keyword to onTap callback in the code example in `navigation.md` (flutter/packages#8343) 2025-01-28 stuartmorgan@google.com Re-land [shared_preferences] Add shared preferences devtool (flutter/packages#8519) 2025-01-28 32538273+ValentinVignal@users.noreply.github.com [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 flutter-ecosystem@google.com 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
77 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

4e0111097 [vector_graphics] Fix memory leaks and activate leak testin…

d6348f0

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

jonahwilliams added 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

4e0111097 [vector_graphics] Fix memory leaks and activate leak testin…

1e78840

…g [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

4e0111097 [vector_graphics] Fix memory leaks and activate leak testin…

9639cab

…g [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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] #8373

[vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] #8373

Uh oh!

ValentinVignal commented Jan 6, 2025

Uh oh!

ValentinVignal commented Jan 6, 2025

Uh oh!

jonahwilliams Jan 6, 2025

Uh oh!

ValentinVignal Jan 7, 2025

Uh oh!

ValentinVignal Jan 18, 2025

Uh oh!

jonahwilliams Jan 18, 2025

Uh oh!

ValentinVignal Jan 19, 2025

Uh oh!

jonahwilliams Jan 27, 2025

Uh oh!

ValentinVignal Jan 28, 2025

Uh oh!

ValentinVignal Jan 28, 2025

Uh oh!

jonahwilliams Jan 28, 2025

Uh oh!

jonahwilliams Jan 28, 2025

Uh oh!

ValentinVignal Jan 28, 2025

Uh oh!

jonahwilliams left a comment

Uh oh!

Uh oh!

Uh oh!

	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);

[vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] #8373

[vector_graphics] Fix memory leaks and activate leak testing [prod-leak-fix] #8373

Uh oh!

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!

Uh oh!