Fix ImageCropper crash when switching Crop/Choose tabs

CHANGELOG.md

@@ -44,6 +44,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` returning a JPEG-format bitmap backed by a prematurely disposed `MemoryStream`
+- [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 `GetCroppedImage` throwing `NullReferenceException` when `Image` property is set directly rather than via `SetImage`
 - [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,171 @@
+using System.Drawing;
+using System.Drawing.Imaging;
+using System.IO;
+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_CalledTwice_DoesNotThrow()
+		{
+			var cropper = new ImageCropper();
+			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_ReturnsJpegBitmapUsableAfterReturn()
+		{
+			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);
+
+					Image result;
+					// GetCroppedImage returns a Bitmap backed by a MemoryStream for JPEG.
+					// The stream must still be alive after the method returns.
+					result = cropper.GetCroppedImage();
+					try
+					{
+						Assert.IsNotNull(result);
+						Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
+						// Re-encode to force GDI+ to re-read the pixel data from the backing stream.
+						using (var ms = new MemoryStream())
+							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png));
+					}
+					finally
+					{
+						result?.Dispose();
+					}
+				}
+			}
+		}
+		[Test]
+		public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsJpegBitmap()
+		{
+			// Setting Image directly (not via SetImage) must still initialize _originalFormat so
+			// GetCroppedImage does not throw NullReferenceException on JPEG re-encoding.
+			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.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
+						Assert.Greater(result.Width, 0);
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow()
+		{
+			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);
+					cropper.SetImage(img2);
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow()
+		{
+			// GetImage() returns a JPEG Bitmap backed by a MemoryStream; feeding that result
+			// back into a new cropper re-saves it in the Image setter (value.Image.Save), which
+			// crashed once the backing stream had been disposed.
+			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));
+					}
+				}
+			}
+		}
+	}
+}

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

@@ -145,6 +145,12 @@
 				if (value == null)
 					return;
 
+				_savedOriginalImage?.Dispose();
+				_savedOriginalImage = null;
+				_croppingImage?.Dispose();
+				_croppingImage = null;
+				_originalFormat = value.Image.RawFormat;
+
 				//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 +159,7 @@
 				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 +273,8 @@
 
 		private void CalculateSourceImageArea()
 		{
+			if (_croppingImage == null)
+				return;
 			float imageToCanvaseScaleFactor = GetImageToCanvasScaleFactor(_croppingImage);
 			_sourceImageArea = new Rectangle(GripThickness, GripThickness,
 											 (int)(_croppingImage.Width*imageToCanvaseScaleFactor),
@@ -421,14 +429,24 @@
 					if (_originalFormat.Guid == ImageFormat.Jpeg.Guid)
 					{
 						//We've sadly lost our jpeg formatting, so now we encode a new image in jpeg
-						using (var stream = new MemoryStream())
+						var stream = new MemoryStream();
+						try
 						{
 							cropped.Save(stream, ImageFormat.Jpeg);
+							stream.Position = 0;
+							// Do not dispose stream on success: GDI+ bitmaps reference the stream for lazy
+							// decoding. The stream is collected when the returned Bitmap is disposed.
 							var oldCropped = cropped;
-							cropped = System.Drawing.Image.FromStream(stream) as Bitmap;
+							cropped = (Bitmap)System.Drawing.Image.FromStream(stream);
 							oldCropped.Dispose();
-							Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting");
 						}
+						catch
+						{
+							stream.Dispose();
+							cropped.Dispose();
+							throw;
+						}
+						Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting");
 					}
 					return cropped;
 				}
@@ -449,7 +467,6 @@
 			}
 			else
 			{
-				_originalFormat = image.Image.RawFormat;
 				Image = image;
 			}
 		}
@@ -482,6 +499,8 @@
 					components = null;
 				}
 
+				Application.Idle -= Application_Idle;
+
 				try
 				{
 					if (_savedOriginalImage != null)