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

* 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 9ad74f76f6.

src/ImageSharp/Image.cs

@@ -80,8 +80,22 @@ namespace SixLabors.ImageSharp
         /// </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 @@ namespace SixLabors.ImageSharp
         {
             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 @@ namespace SixLabors.ImageSharp
         /// <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;

src/ImageSharp/ImageExtensions.cs

@@ -119,5 +119,16 @@ namespace SixLabors.ImageSharp
                 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.");
+            }
+        }
     }
-}
+}

src/ImageSharp/Image{TPixel}.cs

@@ -162,6 +162,8 @@ namespace SixLabors.ImageSharp
         /// <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 @@ namespace SixLabors.ImageSharp
         /// <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);
         }
 

src/ImageSharp/Processing/Extensions/ProcessingExtensions.cs

@@ -21,6 +21,10 @@ namespace SixLabors.ImageSharp.Processing
         /// <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 @@ namespace SixLabors.ImageSharp.Processing
         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 @@ namespace SixLabors.ImageSharp.Processing
         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 @@ namespace SixLabors.ImageSharp.Processing
         /// <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 @@ namespace SixLabors.ImageSharp.Processing
         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 @@ namespace SixLabors.ImageSharp.Processing
         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 @@ namespace SixLabors.ImageSharp.Processing
             }
         }
     }
-}
+}

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 @@ namespace SixLabors.ImageSharp.Tests
             }
         }
     }
-}
+}

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 @@ namespace SixLabors.ImageSharp.Tests
                     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));
+                }
+            }
         }
     }
 }

tests/ImageSharp.Tests/ImageOperationTests.cs

@@ -23,19 +23,22 @@ namespace SixLabors.ImageSharp.Tests
         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 @@ namespace SixLabors.ImageSharp.Tests
         }
 
         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);
+        }
     }
 }