Fix Avalonia SvgImage reload resource leak

src/Svg.Controls.Skia.Avalonia/SvgImage.cs

@@ -202,10 +202,20 @@ private bool HasStyleOverrides()
     private static bool HasSameParameters(SvgParameters? left, SvgParameters? right)
     {
         return ReferenceEquals(left?.Entities, right?.Entities) &&
-               string.Equals(left?.Css, right?.Css, StringComparison.Ordinal) &&
+               HasSameCss(left?.Css, right?.Css) &&
                Nullable.Equals(left?.CurrentColor, right?.CurrentColor);
     }
 
+    private static bool HasSameCss(string? left, string? right)
+    {
+        if (string.IsNullOrWhiteSpace(left) && string.IsNullOrWhiteSpace(right))
+        {
+            return true;
+        }
+
+        return string.Equals(left, right, StringComparison.Ordinal);
+    }
+
     private static string? CombineCss(string? baseCss, string? css, string? currentCss)
     {
         var combinedCss = CombineCss(css, currentCss);

src/Svg.Controls.Skia.Avalonia/SvgSource.cs

@@ -39,6 +39,7 @@ public sealed class SvgSource : IDisposable
     private Uri? _originalBaseUri;
     private int _activeRenders;
     private readonly ThreadLocal<int> _renderDepth = new(() => 0);
+    private List<ResourceDisposal>? _deferredDisposals;
     private bool _disposePending;
     private bool _disposed;
 
@@ -152,6 +153,22 @@ public void Dispose()
 
     public object Sync { get; } = new();
 
+    private readonly struct ResourceDisposal
+    {
+        public ResourceDisposal(SKPicture? picture, SKSvg? skSvg, Stream? originalStream)
+        {
+            Picture = picture;
+            SkSvg = skSvg;
+            OriginalStream = originalStream;
+        }
+
+        public SKPicture? Picture { get; }
+
+        public SKSvg? SkSvg { get; }
+
+        public Stream? OriginalStream { get; }
+    }
+
     static SvgSource()
     {
         s_skiaModel = new SkiaModel(new SKSvgSettings());
@@ -160,37 +177,50 @@ static SvgSource()
 
     private static SKPicture? Load(SvgSource source, string? path, SvgParameters? parameters)
     {
+        SKPicture? oldPicture = null;
+        SKSvg? oldSkSvg = null;
+        Stream? oldOriginalStream = null;
+
         if (path is null)
         {
-            lock (source.Sync)
+            if (source.ReplaceResources(
+                    picture: null,
+                    skSvg: null,
+                    originalStream: null,
+                    originalPath: null,
+                    parameters: parameters,
+                    originalBaseUri: null,
+                    out oldPicture,
+                    out oldSkSvg,
+                    out oldOriginalStream))
             {
-                source._originalPath = null;
-                source._originalStream?.Dispose();
-                source._originalStream = null;
-                source._originalParameters = parameters;
-                source._originalBaseUri = null;
-                source._skSvg = null;
-                source._picture = null;
+                DisposeResources(oldPicture, oldSkSvg, oldOriginalStream);
             }
+
             return null;
         }
 
         var skSvg = CreateSkSvg();
         skSvg.Load(path, parameters);
         var picture = skSvg.Picture;
 
-        lock (source.Sync)
+        if (source.ReplaceResources(
+                picture,
+                skSvg,
+                originalStream: null,
+                originalPath: path,
+                parameters: parameters,
+                originalBaseUri: null,
+                out oldPicture,
+                out oldSkSvg,
+                out oldOriginalStream))
         {
-            source._originalPath = path;
-            source._originalStream?.Dispose();
-            source._originalStream = null;
-            source._originalParameters = parameters;
-            source._originalBaseUri = null;
-            source._skSvg = skSvg;
-            source._picture = picture;
+            DisposeResources(oldPicture, oldSkSvg, oldOriginalStream);
+            return picture;
         }
 
-        return picture;
+        DisposeResources(picture, skSvg, originalStream: null);
+        return null;
     }
 
     private static SKPicture? Load(SvgSource source, Stream stream, SvgParameters? parameters = null, Uri? baseUri = null)
@@ -206,19 +236,27 @@ static SvgSource()
         var skSvg = CreateSkSvg();
         skSvg.Load(cachedStream, parameters, baseUri);
         var picture = skSvg.Picture;
-
-        lock (source.Sync)
+        SKPicture? oldPicture;
+        SKSvg? oldSkSvg;
+        Stream? oldOriginalStream;
+
+        if (source.ReplaceResources(
+                picture,
+                skSvg,
+                cachedStream,
+                originalPath: null,
+                parameters: parameters,
+                originalBaseUri: baseUri,
+                out oldPicture,
+                out oldSkSvg,
+                out oldOriginalStream))
         {
-            source._originalStream?.Dispose();
-            source._originalStream = cachedStream;
-            source._originalPath = null;
-            source._originalParameters = parameters;
-            source._originalBaseUri = baseUri;
-            source._skSvg = skSvg;
-            source._picture = picture;
+            DisposeResources(oldPicture, oldSkSvg, oldOriginalStream);
+            return picture;
         }
 
-        return picture;
+        DisposeResources(picture, skSvg, cachedStream);
+        return null;
     }
 
     private static MemoryStream CreateStream(string svg)
@@ -362,17 +400,26 @@ public static SvgSource LoadFromSvgDocument(SvgDocument document, SvgParameters?
             var skSvg = CreateSkSvg();
             skSvg.FromSvgDocument(document);
             var picture = skSvg.Picture;
-
-            lock (source.Sync)
+            SKPicture? oldPicture;
+            SKSvg? oldSkSvg;
+            Stream? oldOriginalStream;
+
+            if (source.ReplaceResources(
+                    picture,
+                    skSvg,
+                    originalStream,
+                    originalPath: null,
+                    parameters: null,
+                    originalBaseUri: document.BaseUri,
+                    out oldPicture,
+                    out oldSkSvg,
+                    out oldOriginalStream))
             {
-                source._originalStream?.Dispose();
-                source._originalStream = originalStream;
-                source._originalPath = null;
-                source._originalParameters = null;
-                source._originalBaseUri = document.BaseUri;
-                source._skSvg = skSvg;
-                source._picture = picture;
+                DisposeResources(oldPicture, oldSkSvg, oldOriginalStream);
+                return source;
             }
+
+            DisposeResources(picture, skSvg, originalStream);
             return source;
         }
 
@@ -535,15 +582,16 @@ public void ReLoad(SvgParameters? parameters)
             if (_originalStream is { } originalStream)
             {
                 streamCopy = new MemoryStream();
+                var position = originalStream.Position;
                 originalStream.Position = 0;
                 originalStream.CopyTo(streamCopy);
                 streamCopy.Position = 0;
+                originalStream.Position = position;
             }
             originalPath = _originalPath;
             originalBaseUri = _originalBaseUri;
             path = Path;
             baseUri = _baseUri;
-            _originalParameters = parameters;
         }
 
         if (streamCopy is { })
@@ -564,6 +612,62 @@ public void ReLoad(SvgParameters? parameters)
         }
     }
 
+    private bool ReplaceResources(
+        SKPicture? picture,
+        SKSvg? skSvg,
+        Stream? originalStream,
+        string? originalPath,
+        SvgParameters? parameters,
+        Uri? originalBaseUri,
+        out SKPicture? oldPicture,
+        out SKSvg? oldSkSvg,
+        out Stream? oldOriginalStream)
+    {
+        oldPicture = null;
+        oldSkSvg = null;
+        oldOriginalStream = null;
+
+        lock (Sync)
+        {
+            if (_disposed || _disposePending)
+            {
+                return false;
+            }
+
+            oldPicture = _picture;
+            oldSkSvg = _skSvg;
+            oldOriginalStream = _originalStream;
+
+            _picture = picture;
+            _skSvg = skSvg;
+            _originalStream = originalStream;
+            _originalPath = originalPath;
+            _originalParameters = parameters;
+            _originalBaseUri = originalBaseUri;
+
+            if (_activeRenders > 0)
+            {
+                QueueDeferredDisposalLocked(oldPicture, oldSkSvg, oldOriginalStream);
+                oldPicture = null;
+                oldSkSvg = null;
+                oldOriginalStream = null;
+            }
+        }
+
+        return true;
+    }
+
+    private void QueueDeferredDisposalLocked(SKPicture? picture, SKSvg? skSvg, Stream? originalStream)
+    {
+        if (picture is null && skSvg is null && originalStream is null)
+        {
+            return;
+        }
+
+        _deferredDisposals ??= new List<ResourceDisposal>();
+        _deferredDisposals.Add(new ResourceDisposal(picture, skSvg, originalStream));
+    }
+
     internal bool BeginRender()
     {
         lock (Sync)
@@ -584,6 +688,7 @@ internal void EndRender()
         SKPicture? picture = null;
         SKSvg? skSvg = null;
         Stream? originalStream = null;
+        List<ResourceDisposal>? deferredDisposals = null;
 
         lock (Sync)
         {
@@ -594,6 +699,9 @@ internal void EndRender()
 
             if (_activeRenders > 0 && --_activeRenders == 0)
             {
+                deferredDisposals = _deferredDisposals;
+                _deferredDisposals = null;
+
                 if (_disposePending)
                 {
                     DisposeCoreLocked(out picture, out skSvg, out originalStream);
@@ -607,6 +715,8 @@ internal void EndRender()
         {
             DisposeResources(picture, skSvg, originalStream);
         }
+
+        DisposeDeferredResources(deferredDisposals);
     }
 
     private void DisposeCoreLocked(out SKPicture? picture, out SKSvg? skSvg, out Stream? originalStream)
@@ -642,4 +752,17 @@ private static void DisposeResources(SKPicture? picture, SKSvg? skSvg, Stream? o
 
         originalStream?.Dispose();
     }
+
+    private static void DisposeDeferredResources(List<ResourceDisposal>? disposals)
+    {
+        if (disposals is null)
+        {
+            return;
+        }
+
+        foreach (var disposal in disposals)
+        {
+            DisposeResources(disposal.Picture, disposal.SkSvg, disposal.OriginalStream);
+        }
+    }
 }

tests/Svg.Controls.Skia.Avalonia.UnitTests/SvgImageTests.cs

@@ -1,4 +1,5 @@
-using System.Linq;
+using System;
+using System.Linq;
 using Avalonia;
 using Avalonia.Controls;
 using Avalonia.Headless.XUnit;
@@ -151,4 +152,40 @@ private static SKColor GetFirstFillColor(SvgSource source)
         Assert.NotNull(command?.Paint?.Color);
         return command!.Paint!.Color!.Value;
     }
+
+    [AvaloniaFact]
+    public void SvgImage_Does_Not_Reload_When_Effective_Css_Is_Unchanged()
+    {
+        using var source = SvgSource.LoadFromSvg(SampleSvg, new SvgParameters(null, " "));
+        var originalSvg = source.Svg;
+        var originalPicture = source.Picture;
+        var svgImage = new SvgImage
+        {
+            Source = source
+        };
+
+        svgImage.Css = string.Empty;
+        svgImage.CurrentCss = string.Empty;
+
+        Assert.Same(originalSvg, source.Svg);
+        Assert.Same(originalPicture, source.Picture);
+    }
+
+    [AvaloniaFact]
+    public void SvgImage_Css_Reload_Disposes_Previous_Picture()
+    {
+        using var source = SvgSource.LoadFromSvg(SampleSvg, new SvgParameters(null, " "));
+        var originalPicture = source.Picture;
+        var svgImage = new SvgImage
+        {
+            Source = source
+        };
+
+        Assert.NotNull(originalPicture);
+
+        svgImage.Css = "rect { fill: blue; }";
+
+        Assert.NotSame(originalPicture, source.Picture);
+        Assert.Equal(IntPtr.Zero, originalPicture.Handle);
+    }
 }