Fix ImageCropper crash and resource leaks

CHANGELOG.md

@@ -46,6 +46,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [SIL.Windows.Forms] Updated ImageToolbox UI to consistently use "image" (not "picture").
 - [SIL.Windows.Forms.Keyboarding] Removed Timer-based deferred IME conversion status restore from WindowsKeyboardSwitchingAdapter, which disrupted active Chinese Pinyin IME compositions (LT-22442). Added diagnostic tracing for keyboard switching and IME state.
 - [SIL.DictionaryServices] Fix memory leak in LiftWriter
+- [SIL.Windows.Forms] Fixed ImageCropper crash when switching between Crop and Choose tabs: `Application.Idle` handler was never unsubscribed, causing it to fire on a disposed object
+- [SIL.Windows.Forms] Fixed ImageCropper `GetCroppedImage` re-encoding the crop through a `MemoryStream` that had to outlive the returned bitmap; the stream could not be disposed and leaked. `GetCroppedImage` now returns the cropped bitmap directly and the caller chooses the save format (via the file extension), as `PalasoImage.Save` already does
+- [SIL.Windows.Forms] Fixed ImageCropper `Image` setter leaking the previous temp file and cropping image on re-set; fields are now nulled after disposal so a mid-setter failure does not leave disposed-but-non-null references
+- [SIL.Windows.Forms] Fixed ImageCropper not downscaling tall images before cropping (height condition was checking width)
+- [SIL.Windows.Forms] Fixed ImageCropper `NullReferenceException` when the control is resized before an image has been set
 - [SIL.WritingSystems] Fix IetfLanguageTag.GetGeneralCode to handle cases when zh-CN or zh-TW is a prefix and not the whole string.
 - [SIL.WritingSystems] More fixes to consistently use 繁体中文 and 简体中文 for Traditional and Simplified Chinese native language names, and Chinese (Traditional) and Chinese (Simplified) for their English names.
 - [SIL.Windows.Forms] Prevent BetterLabel from responding to OnTextChanged when it has been disposed.

SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs

@@ -0,0 +1,230 @@
+using System;
+using System.Drawing;
+using System.Drawing.Imaging;
+using System.IO;
+using System.Reflection;
+using System.Threading;
+using NUnit.Framework;
+using SIL.IO;
+using SIL.Windows.Forms.ImageToolbox;
+using SIL.Windows.Forms.ImageToolbox.Cropping;
+
+namespace SIL.Windows.Forms.Tests.ImageToolbox
+{
+	[Apartment(ApartmentState.STA)]
+	[TestFixture]
+	public class ImageCropperTests
+	{
+		[Test]
+		public void Dispose_CalledTwiceAfterSettingImage_DoesNotThrow()
+		{
+			// Exercises double-dispose after the cropper actually holds state to clean up
+			// (saved-original temp file, cropping image, Application.Idle subscription),
+			// not just on a freshly-constructed, empty instance.
+			using (var tempFile = TempFile.WithExtension(".png"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+					bmp.Save(tempFile.Path, ImageFormat.Png);
+
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				{
+					var cropper = new ImageCropper();
+					cropper.Size = new Size(400, 300);
+					cropper.SetImage(palasoImage);
+
+					Assert.DoesNotThrow(() => cropper.Dispose());
+					Assert.DoesNotThrow(() => cropper.Dispose());
+				}
+			}
+		}
+
+		[Test]
+		public void GetCroppedImage_PngImage_ReturnsUsableBitmap()
+		{
+			using (var tempFile = TempFile.WithExtension(".png"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+					bmp.Save(tempFile.Path, ImageFormat.Png);
+
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.SetImage(palasoImage);
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						Assert.Greater(result.Width, 0);
+						// Re-encode to force GDI+ to re-read the pixel data from the backing store.
+						using (var ms = new MemoryStream())
+							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png));
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void GetCroppedImage_JpegImage_ReturnsUsableBitmap()
+		{
+			using (var tempFile = TempFile.WithExtension(".jpg"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+					bmp.Save(tempFile.Path, ImageFormat.Jpeg);
+
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.SetImage(palasoImage);
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						// The crop is a stand-alone in-memory bitmap, not backed by a file or stream, so
+						// it reports MemoryBmp format even for a JPEG source; the caller chooses the actual
+						// save format via the file extension.
+						Assert.AreEqual(ImageFormat.MemoryBmp.Guid, result.RawFormat.Guid);
+						// Re-encode to force GDI+ to re-read the pixel data from the backing store.
+						using (var ms = new MemoryStream())
+							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png));
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsUsableBitmap()
+		{
+			// Setting Image directly (not via SetImage) must still yield a usable crop.
+			using (var tempFile = TempFile.WithExtension(".jpg"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+					bmp.Save(tempFile.Path, ImageFormat.Jpeg);
+
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.Image = palasoImage; // bypass SetImage intentionally
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						Assert.Greater(result.Width, 0);
+						using (var ms = new MemoryStream())
+							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png));
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void SetImage_Reassign_DisposesPreviousSavedOriginalAndCroppingImage()
+		{
+			using (var tempFile1 = TempFile.WithExtension(".png"))
+			using (var tempFile2 = TempFile.WithExtension(".jpg"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+				{
+					bmp.Save(tempFile1.Path, ImageFormat.Png);
+					bmp.Save(tempFile2.Path, ImageFormat.Jpeg);
+				}
+
+				using (var img1 = PalasoImage.FromFile(tempFile1.Path))
+				using (var img2 = PalasoImage.FromFile(tempFile2.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.SetImage(img1);
+
+					var firstSavedOriginal = (TempFile)GetPrivateField(cropper, "_savedOriginalImage");
+					var firstCroppingImage = (Image)GetPrivateField(cropper, "_croppingImage");
+					var firstSavedOriginalPath = firstSavedOriginal.Path;
+					Assert.That(File.Exists(firstSavedOriginalPath), "Sanity check: first saved-original temp file should exist before reassignment");
+
+					cropper.SetImage(img2);
+
+					// Regression test: the Image setter used to overwrite _savedOriginalImage/_croppingImage
+					// without disposing the previous instances, leaking the temp file and the cropping bitmap.
+					Assert.That(File.Exists(firstSavedOriginalPath), Is.False,
+						"Previous saved-original temp file should have been disposed (and deleted) on reassignment");
+					Assert.Throws<ArgumentException>(() => { var _ = firstCroppingImage.Width; },
+						"Previous cropping image should have been disposed on reassignment");
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						Assert.Greater(result.Width, 0);
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void SetImage_TallImage_DownscalesCroppingImage()
+		{
+			// Regression test for the height/width typo (BL-1275): the downscale-threshold check
+			// tested Width > 1000 twice instead of Width > 1000 and Height > 1000, so a tall image
+			// (height > 1000, width <= 1000) skipped downscaling entirely.
+			using (var tempFile = TempFile.WithExtension(".png"))
+			{
+				using (var bmp = new Bitmap(100, 1200))
+					bmp.Save(tempFile.Path, ImageFormat.Png);
+
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.SetImage(palasoImage);
+
+					// Not owned by this test -- the cropper still holds and will dispose this itself.
+					var croppingImage = (Image)GetPrivateField(cropper, "_croppingImage");
+					Assert.Less(croppingImage.Height, 1200,
+						"Tall image should have been downscaled before cropping");
+				}
+			}
+		}
+
+		[Test]
+		public void SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow()
+		{
+			// Regression test for issue #1275: cropping a JPEG, then feeding the result back into a
+			// new cropper (which re-saves it in the Image setter via value.Image.Save) crashed when
+			// the cropped bitmap was backed by a disposed stream. The crop is now a stand-alone
+			// bitmap, so the round-trip must not throw.
+			using (var tempFile = TempFile.WithExtension(".jpg"))
+			{
+				using (var bmp = new Bitmap(1200, 900))
+					bmp.Save(tempFile.Path, ImageFormat.Jpeg);
+
+				// GetImage() returns the same PalasoImage instance, now holding the cropped JPEG,
+				// so the outer using disposes it exactly once.
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				{
+					PalasoImage cropped;
+					using (var cropper1 = new ImageCropper())
+					{
+						cropper1.Size = new Size(400, 300);
+						cropper1.SetImage(palasoImage);
+						cropped = cropper1.GetImage();
+					}
+					Assert.That(cropped, Is.Not.Null);
+
+					using (var cropper2 = new ImageCropper())
+					{
+						cropper2.Size = new Size(400, 300);
+						Assert.DoesNotThrow(() => cropper2.SetImage(cropped));
+					}
+				}
+			}
+		}
+
+		private static object GetPrivateField(ImageCropper cropper, string fieldName)
+		{
+			return typeof(ImageCropper)
+				.GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance)
+				.GetValue(cropper);
+		}
+	}
+}

SIL.Windows.Forms.Tests/ImageToolbox/ImageToolboxTests.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Diagnostics;
-using System.Drawing.Imaging;
 using System.IO;
 using System.Reflection;
 using System.Threading;
@@ -89,20 +88,6 @@ public void ShowToolboxWith_PreExisting_Image_WithMetadata()
 			}
 		}
 
-		[Test]
-		[Explicit("By hand only")]
-		public void ShowToolboxWith_PreExisting_EnsureRawFormatUnchanged()
-		{
-			Application.EnableVisualStyles();
-			PalasoImage i = PalasoImage.FromImage(TestImages.logo);
-
-			using (var dlg = new ImageToolboxDialog(i, ""))
-				{
-					dlg.ShowDialog();
-					Assert.AreEqual(ImageFormat.Jpeg.Guid, dlg.ImageInfo.Image.RawFormat.Guid);
-				}
-		}
-
 		[Test]
 		public void DoubleCheckFileFilterWorks()
 		{

SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs

@@ -4,7 +4,6 @@
 using System.Drawing.Imaging;
 using System.IO;
 using System.Windows.Forms;
-using SIL.Code;
 using SIL.IO;
 using SIL.Reporting;
 
@@ -30,7 +29,6 @@ public partial class ImageCropper : UserControl, IImageToolboxControl
 		private Point _startOfDrag = default(Point);
 
 		//we will be cropping the image, so we need to keep the original lest we be cropping the crop, so to speak
-		private ImageFormat _originalFormat;
 		private TempFile _savedOriginalImage;
 		private Image _croppingImage;
 
@@ -145,6 +143,11 @@ public PalasoImage Image
 				if (value == null)
 					return;
 
+				_savedOriginalImage?.Dispose();
+				_savedOriginalImage = null;
+				_croppingImage?.Dispose();
+				_croppingImage = null;
+
 				//other code changes the image of this palaso image, at which time the PI disposes of its copy,
 				//so we better keep our own.
 
@@ -153,7 +156,7 @@ public PalasoImage Image
 				value.Image.Save(_savedOriginalImage.Path, ImageFormat.Png);
 
 				// make a reasonable sized copy to crop
-				if ((value.Image.Width > 1000) || (value.Image.Width > 1000))
+				if ((value.Image.Width > 1000) || (value.Image.Height > 1000))
 				{
 					_croppingImage = CreateCroppingImage(value.Image.Height, value.Image.Width);
 
@@ -267,6 +270,8 @@ private void ImageCropper_Resize(object sender, EventArgs e)
 
 		private void CalculateSourceImageArea()
 		{
+			if (_croppingImage == null)
+				return;
 			float imageToCanvaseScaleFactor = GetImageToCanvasScaleFactor(_croppingImage);
 			_sourceImageArea = new Rectangle(GripThickness, GripThickness,
 											 (int)(_croppingImage.Width*imageToCanvaseScaleFactor),
@@ -385,16 +390,7 @@ public Image GetCroppedImage()
 
 			try
 			{
-				//jpeg = b96b3c  *AE* -0728-11d3-9d7b-0000f81ef32e
-				//bitmap = b96b3c  *AA* -0728-11d3-9d7b-0000f81ef32e
-
-				//NB: this worked for tiff and png, but would crash with Out Of Memory for jpegs.
-				//This may be because I closed the stream? THe doc says you have to keep that stream open.
-				//Also, note that this method, too, lost our jpeg encoding:
-				//          return bmp.Clone(selection, _image.PixelFormat);
-				//So now, I first copy it, then clone with the bounds of our crop:
-
-				using (var originalImage = new Bitmap(_savedOriginalImage.Path)) //**** here we lose the jpeg rawimageformat, if it's a jpeg. Grrr.
+				using (var originalImage = new Bitmap(_savedOriginalImage.Path))
 				{
 					double z = 1.0 / GetImageToCanvasScaleFactor(originalImage);
 
@@ -416,21 +412,13 @@ public Image GetCroppedImage()
 						selectionHeight = originalImage.Height - top;
 					var selection = new Rectangle(left, top, selectionWidth, selectionHeight);
 
-					var cropped = originalImage.Clone(selection, originalImage.PixelFormat); //do the actual cropping
-
-					if (_originalFormat.Guid == ImageFormat.Jpeg.Guid)
+					using (var cropped = originalImage.Clone(selection, originalImage.PixelFormat)) //do the actual cropping
 					{
-						//We've sadly lost our jpeg formatting, so now we encode a new image in jpeg
-						using (var stream = new MemoryStream())
-						{
-							cropped.Save(stream, ImageFormat.Jpeg);
-							var oldCropped = cropped;
-							cropped = System.Drawing.Image.FromStream(stream) as Bitmap;
-							oldCropped.Dispose();
-							Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting");
-						}
+						// Copy into a fresh, stand-alone Bitmap so the result has no lazy reference to a
+						// stream or file we'd otherwise need to keep open. The caller picks the save
+						// format via file extension, so the resulting MemoryBmp format is fine.
+						return new Bitmap(cropped);
 					}
-					return cropped;
 				}
 			}
 			catch (Exception e)
@@ -449,7 +437,6 @@ public void SetImage(PalasoImage image)
 			}
 			else
 			{
-				_originalFormat = image.Image.RawFormat;
 				Image = image;
 			}
 		}
@@ -482,6 +469,8 @@ protected override void Dispose(bool disposing)
 					components = null;
 				}
 
+				Application.Idle -= Application_Idle;
+
 				try
 				{
 					if (_savedOriginalImage != null)