Fix bugs in picture disposal

CHANGELOG.md

@@ -1,5 +1,9 @@
 # CHANGES
 
+## 1.0.3+1
+
+- Fix bugs in picture disposal.
+
 ## 1.0.3
 
 - Use `longestLine` rather than `minIntrinsicWidth` to place text.

lib/src/picture_stream.dart

@@ -57,16 +57,62 @@ class PictureInfo {
 
   /// Creates a [PictureLayer] that will suitably manage the lifecycle of the
   /// [picture].
+  ///
+  /// This must not be called if all created handles have been disposed.
   PictureLayer createLayer() {
-    return _NonOwningPictureLayer(viewport)
-      ..picture = picture
-      ..isComplexHint = true;
+    assert(picture != null);
+    return _NonOwningComplexPictureLayer(this);
+  }
+
+  final Set<int> _handles = <int>{};
+
+  /// Creates a [PictureHandle] that keeps the [picture] from being disposed.
+  ///
+  /// Once all created handles are disposed, the underlying [picture] must not
+  /// be used again.
+  PictureHandle createHandle() {
+    final PictureHandle handle = PictureHandle._(this);
+    _handles.add(handle._id);
+    return handle;
   }
 
-  void _dispose() {
+  void _disposeHandle(PictureHandle handle) {
+    assert(_handles.isNotEmpty);
     assert(_picture != null);
-    _picture!.dispose();
-    _picture = null;
+    final bool removed = _handles.remove(handle._id);
+    assert(removed);
+    if (_handles.isEmpty) {
+      _picture!.dispose();
+      _picture = null;
+    }
+  }
+}
+
+/// An opaque handle used by [PictureInfo] to track the lifecycle of a
+/// [Picture].
+///
+/// Create handles using [PictureInfo.createHandle]. Dispose of them using
+/// [dispose].
+@immutable
+class PictureHandle {
+  PictureHandle._(this._owner);
+
+  static int _counter = 1;
+  final int _id = _counter++;
+
+  final PictureInfo _owner;
+
+  /// Disposes of this handle. Must not be called more than once.
+  void dispose() {
+    _owner._disposeHandle(this);
+  }
+
+  @override
+  int get hashCode => _id;
+
+  @override
+  bool operator ==(Object other) {
+    return other is PictureHandle && other._id == _id;
   }
 }
 
@@ -205,6 +251,7 @@ class PictureStream with Diagnosticable {
 abstract class PictureStreamCompleter with Diagnosticable {
   final List<_PictureListenerPair> _listeners = <_PictureListenerPair>[];
   PictureInfo? _current;
+  PictureHandle? _handle;
 
   bool _cached = false;
 
@@ -215,7 +262,8 @@ abstract class PictureStreamCompleter with Diagnosticable {
       return;
     }
     if (!value && _listeners.isEmpty) {
-      _current?._dispose();
+      _handle?.dispose();
+      _handle = null;
       _current = null;
     }
     _cached = value;
@@ -253,8 +301,9 @@ abstract class PictureStreamCompleter with Diagnosticable {
       (_PictureListenerPair pair) => pair.listener == listener,
     );
     if (_listeners.isEmpty && !cached) {
-      _current?._dispose();
+      _handle?.dispose();
       _current = null;
+      _handle = null;
     }
   }
 
@@ -267,8 +316,9 @@ abstract class PictureStreamCompleter with Diagnosticable {
   /// Calls all the registered listeners to notify them of a new picture.
   @protected
   void setPicture(PictureInfo? picture) {
-    _current?._dispose();
+    _handle?.dispose();
     _current = picture;
+    _handle = _current?.createHandle();
     if (_listeners.isEmpty) {
       return;
     }
@@ -282,7 +332,10 @@ abstract class PictureStreamCompleter with Diagnosticable {
           listenerPair.errorListener!(exception, stack);
         } else {
           _handleImageError(
-              ErrorDescription('by a picture listener'), exception, stack);
+            ErrorDescription('by a picture listener'),
+            exception,
+            stack,
+          );
         }
       }
     }
@@ -351,18 +404,30 @@ class OneFramePictureStreamCompleter extends PictureStreamCompleter {
   }
 }
 
-class _NonOwningPictureLayer extends PictureLayer {
-  _NonOwningPictureLayer(Rect canvasBounds) : super(canvasBounds);
+class _NonOwningComplexPictureLayer extends PictureLayer {
+  _NonOwningComplexPictureLayer(this._owner)
+      : _handle = _owner.createHandle(),
+        super(_owner.viewport);
+
+  final PictureInfo _owner;
+  PictureHandle? _handle;
 
   @override
-  Picture? get picture => _picture;
+  bool get isComplexHint => true;
 
-  Picture? _picture;
+  @override
+  Picture? get picture => _owner.picture;
 
   @override
   set picture(Picture? picture) {
-    markNeedsAddToScene();
-    // Do not dispose the picture, it's owned by the stream/cache.
-    _picture = picture;
+    // Should only get called from dispose.
+    assert(picture == null);
+    assert(_handle != null);
+    if (picture != null) {
+      markNeedsAddToScene();
+    } else {
+      _handle?.dispose();
+      _handle = null;
+    }
   }
 }

lib/src/render_picture.dart

@@ -202,6 +202,8 @@ class RenderPicture extends RenderBox {
   @override
   void dispose() {
     _transformHandle.layer = null;
+    _clipHandle.layer = null;
+    _pictureHandle.layer = null;
     super.dispose();
   }
 

pubspec.yaml

@@ -1,7 +1,7 @@
 name: flutter_svg
 description: An SVG rendering and widget library for Flutter, which allows painting and displaying Scalable Vector Graphics 1.1 files.
 homepage: https://github.com/dnfield/flutter_svg
-version: 1.0.3
+version: 1.0.3+1
 
 dependencies:
   flutter:

test/picture_provider_test.dart

@@ -8,9 +8,24 @@ import 'package:flutter_svg/flutter_svg.dart';
 import 'package:test/fake.dart';
 import 'package:test/test.dart';
 
+// ignore: must_be_immutable
+class FakePictureHandle extends Fake implements PictureHandle {
+  bool disposed = false;
+
+  @override
+  void dispose() {
+    disposed = true;
+  }
+}
+
 class FakePictureInfo extends Fake implements PictureInfo {
   @override
   late CacheCompatibilityTester compatibilityTester;
+
+  @override
+  PictureHandle createHandle() {
+    return FakePictureHandle();
+  }
 }
 
 class FakeFile extends Fake implements File {}

test/picture_stream_test.dart

@@ -1,9 +1,42 @@
 import 'dart:ui';
 
+import 'package:flutter/rendering.dart';
 import 'package:flutter_svg/flutter_svg.dart';
 import 'package:test/test.dart';
 
 void main() {
+  test('Picture does not get disposed if there are outstanding undisposed layers', () async {
+    final PictureRecorder recorder = PictureRecorder();
+    final Canvas canvas = Canvas(recorder);
+    canvas.drawPaint(Paint()..color = const Color(0xFFFA0000));
+    final Picture picture = recorder.endRecording();
+
+    final PictureInfo info = PictureInfo(
+      picture: picture,
+      viewport: Rect.zero,
+      size: Size.zero,
+      compatibilityTester: const CacheCompatibilityTester(),
+    );
+
+    final OneFramePictureStreamCompleter completer =
+        OneFramePictureStreamCompleter(Future<PictureInfo>.value(info));
+
+    await null; // wait an event turn for future to resolve.
+
+    expect(info.picture, isNotNull);
+    final PictureLayer layer = info.createLayer();
+    expect(info.picture, isNotNull);
+
+    void listener(PictureInfo? image, bool synchronousCall) {}
+
+    completer.addListener(listener);
+    completer.removeListener(listener);
+    expect(info.picture, isNotNull);
+
+    layer.dispose();
+    expect(info.picture, null);
+  });
+
   test('Completer disposes layer when removed from cache and no listeners',
       () async {
     final PictureRecorder recorder = PictureRecorder();