From 6554b4087a788e288f25ab24c9b20b09b31203f3 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 13 Jul 2026 11:18:03 -0400 Subject: [PATCH 1/3] Fix ImageCropper crash and resource leaks (BL-1275) Return the cropped bitmap as a stand-alone copy instead of round-tripping through a MemoryStream that had to outlive it; unsubscribe the disposed Application.Idle handler; dispose and null out prior image state before re-assignment; and fix a height-vs-width typo that skipped downscaling of tall images. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 4 + .../ImageToolbox/ImageCropperTests.cs | 168 ++++++++++++++++++ .../ImageToolbox/Cropping/ImageCropper.cs | 37 ++-- 3 files changed, 192 insertions(+), 17 deletions(-) create mode 100644 SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d2b8374c..1a88576f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,10 @@ 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.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. diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs new file mode 100644 index 000000000..868b0ee6c --- /dev/null +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -0,0 +1,168 @@ +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_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_DoesNotThrow() + { + 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.Greater(result.Width, 0); + } + } + } + } + + [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)); + } + } + } + } + } +} diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 4ded520a6..40a4360f5 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/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), @@ -416,21 +421,18 @@ 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"); - } + // Return a stand-alone copy. We used to round-trip the crop through a MemoryStream + // to restore the JPEG RawFormat, but that stream had to outlive the returned bitmap + // (GDI+ decodes lazily), so it could not be disposed and leaked. Cloning from the + // file-backed originalImage likewise keeps a lazy reference to its source, which can + // leave the caller's temp file locked when the crop is later saved (as ImageCropper's + // own Image setter does). Copying into a fresh Bitmap severs both ties. Nothing reads + // the returned bitmap's RawFormat -- PalasoImage.Save picks the format from the file + // extension -- so the resulting MemoryBmp format is fine. + return new Bitmap(cropped); } - return cropped; } } catch (Exception e) @@ -449,7 +451,6 @@ public void SetImage(PalasoImage image) } else { - _originalFormat = image.Image.RawFormat; Image = image; } } @@ -482,6 +483,8 @@ protected override void Dispose(bool disposing) components = null; } + Application.Idle -= Application_Idle; + try { if (_savedOriginalImage != null) From e588f15e306e86fbcb07600c12f79f537e48b433 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 13 Jul 2026 11:39:56 -0400 Subject: [PATCH 2/3] Trim stale comment and remove obsolete RawFormat test (BL-1275) ShowToolboxWith_PreExisting_EnsureRawFormatUnchanged asserted that cropping preserves the original RawFormat, which no longer holds now that GetCroppedImage intentionally returns a stand-alone bitmap. Co-Authored-By: Claude Sonnet 5 --- .../ImageToolbox/ImageToolboxTests.cs | 15 --------------- .../ImageToolbox/Cropping/ImageCropper.cs | 11 +++-------- 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageToolboxTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageToolboxTests.cs index a0cad7a27..771c2151f 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageToolboxTests.cs +++ b/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() { diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 40a4360f5..30d140298 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -423,14 +423,9 @@ public Image GetCroppedImage() using (var cropped = originalImage.Clone(selection, originalImage.PixelFormat)) //do the actual cropping { - // Return a stand-alone copy. We used to round-trip the crop through a MemoryStream - // to restore the JPEG RawFormat, but that stream had to outlive the returned bitmap - // (GDI+ decodes lazily), so it could not be disposed and leaked. Cloning from the - // file-backed originalImage likewise keeps a lazy reference to its source, which can - // leave the caller's temp file locked when the crop is later saved (as ImageCropper's - // own Image setter does). Copying into a fresh Bitmap severs both ties. Nothing reads - // the returned bitmap's RawFormat -- PalasoImage.Save picks the format from the file - // extension -- so the resulting MemoryBmp format is fine. + // 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); } } From 384aae8c6d4d0de7171a1d036674bb8254e57976 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 13 Jul 2026 12:41:13 -0400 Subject: [PATCH 3/3] Expand ImageCropper test coverage and document CalculateSourceImageArea fix (BL-1275) Strengthen the double-dispose and reassignment tests to actually exercise and verify cleanup (temp file and cropping image disposal) instead of only asserting no exception is thrown, and add a regression test for the height/width downscaling typo. Add the missing CHANGELOG entry for the null-guard in CalculateSourceImageArea, and trim stale comments in GetCroppedImage describing the removed JPEG re-encoding approach. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 1 + .../ImageToolbox/ImageCropperTests.cs | 72 +++++++++++++++++-- .../ImageToolbox/Cropping/ImageCropper.cs | 11 +-- 3 files changed, 69 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a88576f3..07199c116 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [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. diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index 868b0ee6c..41a9dccba 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -1,6 +1,8 @@ +using System; using System.Drawing; using System.Drawing.Imaging; using System.IO; +using System.Reflection; using System.Threading; using NUnit.Framework; using SIL.IO; @@ -14,11 +16,26 @@ namespace SIL.Windows.Forms.Tests.ImageToolbox public class ImageCropperTests { [Test] - public void Dispose_CalledTwice_DoesNotThrow() + public void Dispose_CalledTwiceAfterSettingImage_DoesNotThrow() { - var cropper = new ImageCropper(); - Assert.DoesNotThrow(() => cropper.Dispose()); - Assert.DoesNotThrow(() => cropper.Dispose()); + // 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] @@ -103,7 +120,7 @@ public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsUsableBitmap } [Test] - public void SetImage_Reassign_DoesNotThrow() + public void SetImage_Reassign_DisposesPreviousSavedOriginalAndCroppingImage() { using (var tempFile1 = TempFile.WithExtension(".png")) using (var tempFile2 = TempFile.WithExtension(".jpg")) @@ -120,8 +137,21 @@ public void SetImage_Reassign_DoesNotThrow() { 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(() => { var _ = firstCroppingImage.Width; }, + "Previous cropping image should have been disposed on reassignment"); + using (var result = cropper.GetCroppedImage()) { Assert.IsNotNull(result); @@ -131,6 +161,31 @@ public void SetImage_Reassign_DoesNotThrow() } } + [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() { @@ -164,5 +219,12 @@ public void SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow() } } } + + private static object GetPrivateField(ImageCropper cropper, string fieldName) + { + return typeof(ImageCropper) + .GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance) + .GetValue(cropper); + } } } diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 30d140298..5ae4bf553 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -390,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);