Throw ObjectDisposedException when trying to operate on a disposed image (#968)

antonfirsov · JimBobSquarePants · commit 29099949ca90 · 2019-08-12T18:22:45.000+10:00
* disable multitargeting + TreatWarningsAsErrors to for fast development * Check if image is disposed in significant Image and Image<T> methods * Mutate / Clone: ensure image is not disposed * Revert "disable multitargeting + TreatWarningsAsErrors to for fast development" This reverts commit 9ad74f7.
diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs
@@ -80,8 +80,22 @@ internal Image(
         /// </summary>
         Configuration IConfigurable.Configuration => this.Configuration;
 
+        /// <summary>
+        /// Gets a value indicating whether the image instance is disposed.
+        /// </summary>
+        public bool IsDisposed { get; private set; }
+
         /// <inheritdoc />
-        public abstract void Dispose();
+        public void Dispose()
+        {
+            if (this.IsDisposed)
+            {
+                return;
+            }
+
+            this.IsDisposed = true;
+            this.DisposeImpl();
+        }
 
         /// <summary>
         /// Saves the image to the given stream using the given image encoder.
@@ -93,6 +107,7 @@ public void Save(Stream stream, IImageEncoder encoder)
         {
             Guard.NotNull(stream, nameof(stream));
             Guard.NotNull(encoder, nameof(encoder));
+            this.EnsureNotDisposed();
 
             EncodeVisitor visitor = new EncodeVisitor(encoder, stream);
             this.AcceptVisitor(visitor);
@@ -128,6 +143,11 @@ public abstract Image<TPixel2> CloneAs<TPixel2>(Configuration configuration)
         /// <param name="size">The <see cref="Size"/>.</param>
         protected void UpdateSize(Size size) => this.size = size;
 
+        /// <summary>
+        /// Implements the Dispose logic.
+        /// </summary>
+        protected abstract void DisposeImpl();
+
         private class EncodeVisitor : IImageVisitor
         {
             private readonly IImageEncoder encoder;
diff --git a/src/ImageSharp/ImageExtensions.cs b/src/ImageSharp/ImageExtensions.cs
@@ -119,5 +119,16 @@ public static string ToBase64String<TPixel>(this Image<TPixel> source, IImageFor
                 return $"data:{format.DefaultMimeType};base64,{Convert.ToBase64String(stream.ToArray())}";
             }
         }
+
+        /// <summary>
+        /// Throws <see cref="ObjectDisposedException"/> if the image is disposed.
+        /// </summary>
+        internal static void EnsureNotDisposed(this Image image)
+        {
+            if (image.IsDisposed)
+            {
+                throw new ObjectDisposedException(nameof(image), "Trying to execute an operation on a disposed image.");
+            }
+        }
     }
-}
+}
diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs
@@ -162,6 +162,8 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable<
         /// <returns>Returns a new <see cref="Image{TPixel}"/> with all the same pixel data as the original.</returns>
         public Image<TPixel> Clone(Configuration configuration)
         {
+            this.EnsureNotDisposed();
+
             IEnumerable<ImageFrame<TPixel>> clonedFrames =
                 this.Frames.Select<ImageFrame<TPixel>, ImageFrame<TPixel>>(x => x.Clone(configuration));
             return new Image<TPixel>(configuration, this.Metadata.DeepClone(), clonedFrames);
@@ -175,17 +177,21 @@ public Image<TPixel> Clone(Configuration configuration)
         /// <returns>The <see cref="Image{TPixel2}"/>.</returns>
         public override Image<TPixel2> CloneAs<TPixel2>(Configuration configuration)
         {
+            this.EnsureNotDisposed();
+
             IEnumerable<ImageFrame<TPixel2>> clonedFrames =
                 this.Frames.Select<ImageFrame<TPixel>, ImageFrame<TPixel2>>(x => x.CloneAs<TPixel2>(configuration));
             return new Image<TPixel2>(configuration, this.Metadata.DeepClone(), clonedFrames);
         }
 
         /// <inheritdoc/>
-        public override void Dispose() => this.Frames.Dispose();
+        protected override void DisposeImpl() => this.Frames.Dispose();
 
         /// <inheritdoc />
         internal override void AcceptVisitor(IImageVisitor visitor)
         {
+            this.EnsureNotDisposed();
+
             visitor.Visit(this);
         }
 
diff --git a/src/ImageSharp/Processing/Extensions/ProcessingExtensions.cs b/src/ImageSharp/Processing/Extensions/ProcessingExtensions.cs
@@ -21,6 +21,10 @@ public static class ProcessingExtensions
         /// <param name="operation">The operation to perform on the source.</param>
         public static void Mutate(this Image source, Action<IImageProcessingContext> operation)
         {
+            Guard.NotNull(source, nameof(source));
+            Guard.NotNull(operation, nameof(operation));
+            source.EnsureNotDisposed();
+
             ProcessingVisitor visitor = new ProcessingVisitor(operation, true);
             source.AcceptVisitor(visitor);
         }
@@ -34,8 +38,9 @@ public static void Mutate(this Image source, Action<IImageProcessingContext> ope
         public static void Mutate<TPixel>(this Image<TPixel> source, Action<IImageProcessingContext> operation)
             where TPixel : struct, IPixel<TPixel>
         {
-            Guard.NotNull(operation, nameof(operation));
             Guard.NotNull(source, nameof(source));
+            Guard.NotNull(operation, nameof(operation));
+            source.EnsureNotDisposed();
 
             IInternalImageProcessingContext<TPixel> operationsRunner = source.GetConfiguration().ImageOperationsProvider
                 .CreateImageProcessingContext(source, true);
@@ -51,8 +56,9 @@ public static void Mutate<TPixel>(this Image<TPixel> source, Action<IImageProces
         public static void Mutate<TPixel>(this Image<TPixel> source, params IImageProcessor[] operations)
             where TPixel : struct, IPixel<TPixel>
         {
-            Guard.NotNull(operations, nameof(operations));
             Guard.NotNull(source, nameof(source));
+            Guard.NotNull(operations, nameof(operations));
+            source.EnsureNotDisposed();
 
             IInternalImageProcessingContext<TPixel> operationsRunner = source.GetConfiguration().ImageOperationsProvider
                 .CreateImageProcessingContext(source, true);
@@ -67,6 +73,10 @@ public static void Mutate<TPixel>(this Image<TPixel> source, params IImageProces
         /// <returns>The new <see cref="SixLabors.ImageSharp.Image"/>.</returns>
         public static Image Clone(this Image source, Action<IImageProcessingContext> operation)
         {
+            Guard.NotNull(source, nameof(source));
+            Guard.NotNull(operation, nameof(operation));
+            source.EnsureNotDisposed();
+
             ProcessingVisitor visitor = new ProcessingVisitor(operation, false);
             source.AcceptVisitor(visitor);
             return visitor.ResultImage;
@@ -82,8 +92,9 @@ public static Image Clone(this Image source, Action<IImageProcessingContext> ope
         public static Image<TPixel> Clone<TPixel>(this Image<TPixel> source, Action<IImageProcessingContext> operation)
             where TPixel : struct, IPixel<TPixel>
         {
-            Guard.NotNull(operation, nameof(operation));
             Guard.NotNull(source, nameof(source));
+            Guard.NotNull(operation, nameof(operation));
+            source.EnsureNotDisposed();
 
             IInternalImageProcessingContext<TPixel> operationsRunner = source.GetConfiguration().ImageOperationsProvider
                 .CreateImageProcessingContext(source, false);
@@ -101,8 +112,9 @@ public static Image<TPixel> Clone<TPixel>(this Image<TPixel> source, Action<IIma
         public static Image<TPixel> Clone<TPixel>(this Image<TPixel> source, params IImageProcessor[] operations)
             where TPixel : struct, IPixel<TPixel>
         {
-            Guard.NotNull(operations, nameof(operations));
             Guard.NotNull(source, nameof(source));
+            Guard.NotNull(operations, nameof(operations));
+            source.EnsureNotDisposed();
 
             IInternalImageProcessingContext<TPixel> operationsRunner = source.GetConfiguration().ImageOperationsProvider
                 .CreateImageProcessingContext(source, false);
@@ -152,4 +164,4 @@ public void Visit<TPixel>(Image<TPixel> image)
             }
         }
     }
-}
+}
diff --git a/tests/ImageSharp.Tests/Image/ImageCloneTests.cs b/tests/ImageSharp.Tests/Image/ImageCloneTests.cs
@@ -7,6 +7,24 @@ namespace SixLabors.ImageSharp.Tests
 {
     public class ImageCloneTests
     {
+        [Fact]
+        public void CloneAs_WhenDisposed_Throws()
+        {
+            Image<Rgba32> image = new Image<Rgba32>(5, 5);
+            image.Dispose();
+
+            Assert.Throws<ObjectDisposedException>(() => image.CloneAs<Bgra32>());
+        }
+
+        [Fact]
+        public void Clone_WhenDisposed_Throws()
+        {
+            Image<Rgba32> image = new Image<Rgba32>(5, 5);
+            image.Dispose();
+
+            Assert.Throws<ObjectDisposedException>(() => image.Clone());
+        }
+
         [Theory]
         [WithTestPatternImages(9, 9, PixelTypes.Rgba32)]
         public void CloneAs_ToBgra32(TestImageProvider<Rgba32> provider)
@@ -109,4 +127,4 @@ public void CloneAs_ToRgb24(TestImageProvider<Rgba32> provider)
             }
         }
     }
-}
+}
diff --git a/tests/ImageSharp.Tests/Image/ImageTests.Save.cs b/tests/ImageSharp.Tests/Image/ImageTests.Save.cs
@@ -4,6 +4,10 @@
 // ReSharper disable InconsistentNaming
 
 using System;
+using System.IO;
+
+using Moq;
+
 using SixLabors.ImageSharp.Formats.Png;
 using SixLabors.ImageSharp.PixelFormats;
 using Xunit;
@@ -65,6 +69,18 @@ public void SetEncoding()
                     Assert.Equal("image/png", mime.DefaultMimeType);
                 }
             }
+
+            [Fact]
+            public void ThrowsWhenDisposed()
+            {
+                var image = new Image<Rgba32>(5, 5);
+                image.Dispose();
+                IImageEncoder encoder = Mock.Of<IImageEncoder>();
+                using (MemoryStream stream = new MemoryStream())
+                {
+                    Assert.Throws<ObjectDisposedException>(() => image.Save(stream, encoder));
+                }
+            }
         }
     }
 }
diff --git a/tests/ImageSharp.Tests/ImageOperationTests.cs b/tests/ImageSharp.Tests/ImageOperationTests.cs
@@ -23,19 +23,22 @@ public class ImageOperationTests : IDisposable
         private readonly FakeImageOperationsProvider provider;
         private readonly IImageProcessor processorDefinition;
 
+        private static readonly string ExpectedExceptionMessage = GetExpectedExceptionText();
+
         public ImageOperationTests()
         {
             this.provider = new FakeImageOperationsProvider();
 
             Mock<IImageProcessor> processorMock = new Mock<IImageProcessor>();
             this.processorDefinition = processorMock.Object;
-            
+
             this.image = new Image<Rgba32>(new Configuration
             {
                 ImageOperationsProvider = this.provider
             }, 1, 1);
         }
 
+
         [Fact]
         public void MutateCallsImageOperationsProvider_Func_OriginalImage()
         {
@@ -109,5 +112,65 @@ public void ApplyProcessors_ListOfProcessors_AppliesAllProcessorsToOperation()
         }
 
         public void Dispose() => this.image.Dispose();
+
+        [Fact]
+        public void GenericMutate_WhenDisposed_Throws()
+        {
+            this.image.Dispose();
+
+            CheckThrowsCorrectObjectDisposedException(
+                () => this.image.Mutate(x => x.ApplyProcessor(this.processorDefinition)));
+        }
+
+        [Fact]
+        public void GenericClone_WhenDisposed_Throws()
+        {
+            this.image.Dispose();
+
+            CheckThrowsCorrectObjectDisposedException(
+                () => this.image.Clone(x => x.ApplyProcessor(this.processorDefinition)));
+        }
+
+        [Fact]
+        public void AgnosticMutate_WhenDisposed_Throws()
+        {
+            this.image.Dispose();
+            Image img = this.image;
+
+            CheckThrowsCorrectObjectDisposedException(
+                () => img.Mutate(x => x.ApplyProcessor(this.processorDefinition)));
+        }
+
+        [Fact]
+        public void AgnosticClone_WhenDisposed_Throws()
+        {
+            this.image.Dispose();
+            Image img = this.image;
+
+            CheckThrowsCorrectObjectDisposedException(
+                () => img.Clone(x => x.ApplyProcessor(this.processorDefinition)));
+        }
+
+        private static string GetExpectedExceptionText()
+        {
+            try
+            {
+                Image<Rgba32> img = new Image<Rgba32>(1, 1);
+                img.Dispose();
+                img.EnsureNotDisposed();
+            }
+            catch (ObjectDisposedException ex)
+            {
+                return ex.Message;
+            }
+
+            return "?";
+        }
+
+        private static void CheckThrowsCorrectObjectDisposedException(Action action)
+        {
+            var ex = Assert.Throws<ObjectDisposedException>(action);
+            Assert.Equal(ExpectedExceptionMessage, ex.Message);
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -119,5 +119,16 @@ public static string ToBase64String<TPixel>(this Image<TPixel> source, IImageFor`
`119`	`119`	`return $"data:{format.DefaultMimeType};base64,{Convert.ToBase64String(stream.ToArray())}";`
`120`	`120`	`}`
`121`	`121`	`}`
	`122`	`+`
	`123`	`+ /// <summary>`
	`124`	`+ /// Throws <see cref="ObjectDisposedException"/> if the image is disposed.`
	`125`	`+ /// </summary>`
	`126`	`+ internal static void EnsureNotDisposed(this Image image)`
	`127`	`+ {`
	`128`	`+ if (image.IsDisposed)`
	`129`	`+ {`
	`130`	`+ throw new ObjectDisposedException(nameof(image), "Trying to execute an operation on a disposed image.");`
	`131`	`+ }`
	`132`	`+ }`
`122`	`133`	`}`
`123`		`-}`
	`134`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -162,6 +162,8 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable<`
`162`	`162`	`/// <returns>Returns a new <see cref="Image{TPixel}"/> with all the same pixel data as the original.</returns>`
`163`	`163`	`public Image<TPixel> Clone(Configuration configuration)`
`164`	`164`	`{`
	`165`	`+ this.EnsureNotDisposed();`
	`166`	`+`
`165`	`167`	`IEnumerable<ImageFrame<TPixel>> clonedFrames =`
`166`	`168`	`this.Frames.Select<ImageFrame<TPixel>, ImageFrame<TPixel>>(x => x.Clone(configuration));`
`167`	`169`	`return new Image<TPixel>(configuration, this.Metadata.DeepClone(), clonedFrames);`
`@@ -175,17 +177,21 @@ public Image<TPixel> Clone(Configuration configuration)`
`175`	`177`	`/// <returns>The <see cref="Image{TPixel2}"/>.</returns>`
`176`	`178`	`public override Image<TPixel2> CloneAs<TPixel2>(Configuration configuration)`
`177`	`179`	`{`
	`180`	`+ this.EnsureNotDisposed();`
	`181`	`+`
`178`	`182`	`IEnumerable<ImageFrame<TPixel2>> clonedFrames =`
`179`	`183`	`this.Frames.Select<ImageFrame<TPixel>, ImageFrame<TPixel2>>(x => x.CloneAs<TPixel2>(configuration));`
`180`	`184`	`return new Image<TPixel2>(configuration, this.Metadata.DeepClone(), clonedFrames);`
`181`	`185`	`}`
`182`	`186`
`183`	`187`	`/// <inheritdoc/>`
`184`		`- public override void Dispose() => this.Frames.Dispose();`
	`188`	`+ protected override void DisposeImpl() => this.Frames.Dispose();`
`185`	`189`
`186`	`190`	`/// <inheritdoc />`
`187`	`191`	`internal override void AcceptVisitor(IImageVisitor visitor)`
`188`	`192`	`{`
	`193`	`+ this.EnsureNotDisposed();`
	`194`	`+`
`189`	`195`	`visitor.Visit(this);`
`190`	`196`	`}`
`191`	`197`
Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,10 @@`
`4`	`4`	`// ReSharper disable InconsistentNaming`
`5`	`5`
`6`	`6`	`using System;`
	`7`	`+using System.IO;`
	`8`	`+`
	`9`	`+using Moq;`
	`10`	`+`
`7`	`11`	`using SixLabors.ImageSharp.Formats.Png;`
`8`	`12`	`using SixLabors.ImageSharp.PixelFormats;`
`9`	`13`	`using Xunit;`
`@@ -65,6 +69,18 @@ public void SetEncoding()`
`65`	`69`	`Assert.Equal("image/png", mime.DefaultMimeType);`
`66`	`70`	`}`
`67`	`71`	`}`
	`72`	`+`
	`73`	`+ [Fact]`
	`74`	`+ public void ThrowsWhenDisposed()`
	`75`	`+ {`
	`76`	`+ var image = new Image<Rgba32>(5, 5);`
	`77`	`+ image.Dispose();`
	`78`	`+ IImageEncoder encoder = Mock.Of<IImageEncoder>();`
	`79`	`+ using (MemoryStream stream = new MemoryStream())`
	`80`	`+ {`
	`81`	`+ Assert.Throws<ObjectDisposedException>(() => image.Save(stream, encoder));`
	`82`	`+ }`
	`83`	`+ }`
`68`	`84`	`}`
`69`	`85`	`}`
`70`	`86`	`}`