Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+34 −8
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ca789b
fix memory leaks
ValentinVignal Jan 6, 2025
737916a
Activate leak testing
ValentinVignal Jan 6, 2025
1a640a0
Fix the tests
ValentinVignal Jan 6, 2025
a456583
Update version number
ValentinVignal Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/vector_graphics/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 1.1.16

* Fixes some memory leaks.

## 1.1.15

* Updates error handling in VectorGraphicWidget to handle errors when the bytes of the graphic cannot be loaded.
Expand Down
3 changes: 2 additions & 1 deletion packages/vector_graphics/lib/src/listener.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand what this is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

image.dispose();
completer.complete();
},
onError: (Object exception, StackTrace? stackTrace) {
Expand Down
1 change: 1 addition & 0 deletions packages/vector_graphics/lib/src/render_vector_graphic.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ class RenderVectorGraphic extends RenderBox {

final ui.Image pending =
rasterPicture.toImageSync(scaledWidth, scaledHeight);
rasterPicture.dispose();
return RasterData(pending, 0, key);
}

Expand Down
8 changes: 5 additions & 3 deletions packages/vector_graphics/lib/src/vector_graphics.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,10 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
return;
}
data.count -= 1;
if (data.count == 0 && _livePictureCache.containsKey(data.key)) {
_livePictureCache.remove(data.key);
if (data.count == 0) {
if (_livePictureCache.containsKey(data.key)) {
_livePictureCache.remove(data.key);
}
data.pictureInfo.picture.dispose();
}
}
Expand Down Expand Up @@ -382,7 +384,7 @@ class _VectorGraphicWidgetState extends State<VectorGraphic> {
}

Future<void> _loadAssetBytes() async {
// First check if we have an avilable picture and use this immediately.
// First check if we have an available picture and use this immediately.
final Object loaderKey = widget.loader.cacheKey(context);
final _PictureKey key =
_PictureKey(loaderKey, locale, textDirection, widget.clipViewbox);
Expand Down
3 changes: 2 additions & 1 deletion packages/vector_graphics/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: vector_graphics
description: A vector graphics rendering package for Flutter using a binary encoding.
repository: https://github.com/flutter/packages/tree/main/packages/vector_graphics
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+vector_graphics%22
version: 1.1.15
version: 1.1.16

environment:
sdk: ^3.4.0
Expand All @@ -17,6 +17,7 @@ dependencies:
dev_dependencies:
flutter_test:
sdk: flutter
leak_tracker_flutter_testing: any
vector_graphics_compiler: ^1.1.11+1

platforms:
Expand Down
13 changes: 13 additions & 0 deletions packages/vector_graphics/test/flutter_test_config.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';

import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';

Future<void> testExecutable(FutureOr<void> Function() testMain) async {
LeakTesting.enable();
LeakTracking.warnForUnsupportedPlatforms = false;
await testMain();
}
10 changes: 7 additions & 3 deletions packages/vector_graphics/test/vector_graphics_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,9 @@ void main() {
);
await tester.pumpAndSettle();

expect(await completer.future, isA<PictureInfo>());
final PictureInfo picture = await completer.future;
addTearDown(picture.picture.dispose);
expect(picture, isA<PictureInfo>());
expect(debugLastLocale, const Locale('fr', 'CH'));
expect(debugLastTextDirection, TextDirection.rtl);
});
Expand Down Expand Up @@ -475,7 +477,9 @@ void main() {
);
await tester.pumpAndSettle();

expect(await completer.future, isA<PictureInfo>());
final PictureInfo picture = await completer.future;
addTearDown(picture.picture.dispose);
expect(picture, isA<PictureInfo>());
expect(debugLastLocale, PlatformDispatcher.instance.locale);
expect(debugLastTextDirection, TextDirection.ltr);
});
Expand Down Expand Up @@ -584,7 +588,7 @@ void main() {
expect(imageCache.statusForKey(imageKey).live, false);
expect(imageCache.statusForKey(imageKey).keepAlive, true);

// A blue square, becuase the image is available now.
// A blue square, because the image is available now.
await expectLater(
find.byKey(key),
matchesGoldenFile('vg_with_image_blue.png'),
Expand Down
Loading