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

[ValentinVignal]

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

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[ValentinVignal]

cc @polina-c

[jonahwilliams]

I don't really understand what this is doing?

[ValentinVignal]

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?

[ValentinVignal]

What are your thoughts on this @jonahwilliams ?

[jonahwilliams]

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

[ValentinVignal]

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

1. 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?
2. 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?

[jonahwilliams]

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.

[ValentinVignal]

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 {
	constString bluePngPixel =
	'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPj/HwADBwIAMCbHYQAAAABJRU5ErkJggg==';
	finalVectorGraphicsBuffer buffer =VectorGraphicsBuffer();
	constVectorGraphicsCodec codec =VectorGraphicsCodec();
	codec.writeSize(buffer, 100, 100);
	codec.writeDrawImage(
	buffer,
	codec.writeImage(buffer, 0, base64Decode(bluePngPixel)),
	0,
	0,
	100,
	100,
	null,
	);
	finalUniqueKey key =UniqueKey();
	finalTestBytesLoader loader =TestBytesLoader(buffer.done());
	// See listener.dart.
	finalint 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.
	awaitexpectLater(
	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.
	awaitexpectLater(
	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?

[ValentinVignal]

I pushed the change in Store and dispose ImageInfo

[jonahwilliams]

That makes more sense

[jonahwilliams]

Please add a more descriptive changelog entry.

[ValentinVignal]

Sure, what do you think of More description changelog?

[jonahwilliams]

LGTM