From d9b5d24a804a1c569592abc3406546bd6e6a1e8e Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 15:16:00 -0400 Subject: [PATCH 01/25] Fix ImageCropper Application.Idle leak and premature MemoryStream disposal (#1275) Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 1 + .../ImageToolbox/ImageCropperTests.cs | 90 +++++++++++++++++++ .../ImageToolbox/Cropping/ImageCropper.cs | 19 ++-- 3 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7345a668f..c870997fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ 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 leak + premature MemoryStream disposal) (#1275) - [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..ca1dc7adf --- /dev/null +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -0,0 +1,90 @@ +using System.Drawing; +using System.Drawing.Imaging; +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_DoesNotThrow() + { + var cropper = new ImageCropper(); + Assert.DoesNotThrow(() => cropper.Dispose()); + } + + [Test] + public void Dispose_CalledTwice_DoesNotThrow() + { + var cropper = new ImageCropper(); + 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); + // Accessing Width forces GDI+ to decode the image data; this would throw + // if the backing stream had been prematurely disposed. + Assert.Greater(result.Width, 0); + Assert.Greater(result.Height, 0); + } + } + } + } + + [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); + // Force pixel data access to confirm the stream is still alive. + Assert.Greater(result.Width, 0); + Assert.Greater(result.Height, 0); + } + finally + { + result?.Dispose(); + } + } + } + } + } +} diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 4ded520a6..6d99a946a 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -421,14 +421,15 @@ public Image GetCroppedImage() 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()) - { - 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"); - } + var stream = new MemoryStream(); + cropped.Save(stream, ImageFormat.Jpeg); + var oldCropped = cropped; + // Do not dispose stream here: GDI+ bitmaps reference the stream for lazy decoding. + // The stream will be collected when the returned Bitmap is no longer referenced. + stream.Position = 0; + cropped = System.Drawing.Image.FromStream(stream) as Bitmap; + oldCropped.Dispose(); + Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting"); } return cropped; } @@ -504,6 +505,8 @@ protected override void Dispose(bool disposing) catch (UnauthorizedAccessException) { } + + Application.Idle -= Application_Idle; } base.Dispose(disposing); } From aec870b00837b10d1f9de8f3bfa0112d22a03046 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 15:19:14 -0400 Subject: [PATCH 02/25] Remove issue number from CHANGELOG entry --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c870997fb..37fdfda81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -# Change Log +# Change Log All notable changes to this project will be documented in this file. @@ -43,9 +43,9 @@ 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 leak + premature MemoryStream disposal) (#1275) +- [SIL.Windows.Forms] Fixed ImageCropper crash when switching between Crop and Choose tabs (Application.Idle leak + premature MemoryStream disposal) - [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.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] Prevent ContributorsListControl.GetContributionFromRow from throwing an exception when the DataGridView has no valid rows selected. - [SIL.Media] BREAKING CHANGE (subtle and unlikely): WindowsAudioSession.OnPlaybackStopped now passes itself as the sender instead of a private implementation object, making the event arguments correct. @@ -115,7 +115,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - [SIL.Windows.Forms] In `CustomDropDown.OnOpening`, fixed check that triggers timer to stop. -- [SIL.Windows.Forms] Fixed HtmlBrowserHandled.OnNewWindow to open external URLs (`target="_blank"`) in the system’s default browser instead of Internet Explorer. This improves behavior in SILAboutBox and other components that use the embedded browser. +- [SIL.Windows.Forms] Fixed HtmlBrowserHandled.OnNewWindow to open external URLs (`target="_blank"`) in the system’s default browser instead of Internet Explorer. This improves behavior in SILAboutBox and other components that use the embedded browser. ### Changed From 2123b9d7421e07f693235ccbbfff3120ebef2177 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 15:26:33 -0400 Subject: [PATCH 03/25] Move Application.Idle unsubscription before try-catch in Dispose --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 6d99a946a..7698fcccf 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -483,6 +483,8 @@ protected override void Dispose(bool disposing) components = null; } + Application.Idle -= Application_Idle; + try { if (_savedOriginalImage != null) @@ -505,8 +507,6 @@ protected override void Dispose(bool disposing) catch (UnauthorizedAccessException) { } - - Application.Idle -= Application_Idle; } base.Dispose(disposing); } From 943dd7e0ea0a2b9b091f142d6e8ee3f0bcefee63 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 15:28:37 -0400 Subject: [PATCH 04/25] Use direct cast instead of as-cast for Image.FromStream result --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 7698fcccf..a86c5b258 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -427,7 +427,7 @@ public Image GetCroppedImage() // Do not dispose stream here: GDI+ bitmaps reference the stream for lazy decoding. // The stream will be collected when the returned Bitmap is no longer referenced. stream.Position = 0; - 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"); } From 69d3f3586e90b5af099d9749de5c7cd05e730b4a Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 16:11:26 -0400 Subject: [PATCH 05/25] Dispose MemoryStream on exception path in GetCroppedImage --- .../ImageToolbox/Cropping/ImageCropper.cs | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index a86c5b258..d07144429 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -422,13 +422,21 @@ public Image GetCroppedImage() { //We've sadly lost our jpeg formatting, so now we encode a new image in jpeg var stream = new MemoryStream(); - cropped.Save(stream, ImageFormat.Jpeg); - var oldCropped = cropped; - // Do not dispose stream here: GDI+ bitmaps reference the stream for lazy decoding. - // The stream will be collected when the returned Bitmap is no longer referenced. - stream.Position = 0; - cropped = (Bitmap)System.Drawing.Image.FromStream(stream); - oldCropped.Dispose(); + 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 = (Bitmap)System.Drawing.Image.FromStream(stream); + oldCropped.Dispose(); + } + catch + { + stream.Dispose(); + throw; + } Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting"); } return cropped; From 9b1353198f326355d02cd42a717b543c07216dc6 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 16:18:31 -0400 Subject: [PATCH 06/25] Fix CHANGELOG encoding corruption Restore from base commit to fix double-encoded UTF-8 (Chinese characters and curly apostrophe were corrupted by PowerShell Get-Content/Set-Content mishandling UTF-8-without-BOM), then re-apply our entry using the Edit tool which preserves encoding correctly. Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37fdfda81..449de81bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -# Change Log +# Change Log All notable changes to this project will be documented in this file. @@ -45,7 +45,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [SIL.DictionaryServices] Fix memory leak in LiftWriter - [SIL.Windows.Forms] Fixed ImageCropper crash when switching between Crop and Choose tabs (Application.Idle leak + premature MemoryStream disposal) - [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.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] Prevent ContributorsListControl.GetContributionFromRow from throwing an exception when the DataGridView has no valid rows selected. - [SIL.Media] BREAKING CHANGE (subtle and unlikely): WindowsAudioSession.OnPlaybackStopped now passes itself as the sender instead of a private implementation object, making the event arguments correct. @@ -115,7 +115,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - [SIL.Windows.Forms] In `CustomDropDown.OnOpening`, fixed check that triggers timer to stop. -- [SIL.Windows.Forms] Fixed HtmlBrowserHandled.OnNewWindow to open external URLs (`target="_blank"`) in the system’s default browser instead of Internet Explorer. This improves behavior in SILAboutBox and other components that use the embedded browser. +- [SIL.Windows.Forms] Fixed HtmlBrowserHandled.OnNewWindow to open external URLs (`target="_blank"`) in the system’s default browser instead of Internet Explorer. This improves behavior in SILAboutBox and other components that use the embedded browser. ### Changed From a59dd66ad1870f9a951b2517f3a610941c39a76e Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 16:41:34 -0400 Subject: [PATCH 07/25] Fix Image setter leaking previous _savedOriginalImage and _croppingImage Dispose previous field values before overwriting them so repeated SetImage calls don't accumulate leaked TempFile handles and GDI+ bitmaps. Co-Authored-By: Claude Sonnet 4.6 --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index d07144429..57ce887d4 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -145,6 +145,9 @@ public PalasoImage Image if (value == null) return; + _savedOriginalImage?.Dispose(); + _croppingImage?.Dispose(); + //other code changes the image of this palaso image, at which time the PI disposes of its copy, //so we better keep our own. From a5355d6d0aaac574346251f84c6443ded12e4089 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 16:46:32 -0400 Subject: [PATCH 08/25] Update CHANGELOG to include Image setter leak fix Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 449de81bf..a3ba1db7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,7 @@ 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 leak + premature MemoryStream disposal) +- [SIL.Windows.Forms] Fixed ImageCropper crash when switching between Crop and Choose tabs (Application.Idle leak + premature MemoryStream disposal); also fixed `Image` setter leaking previous temp file and cropping image on re-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. From 09796e3c3e1be756fca625e1fa3c6dd242359b79 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 16:56:52 -0400 Subject: [PATCH 09/25] Null fields after disposal; fix height vs width typo in Image setter - Null _savedOriginalImage and _croppingImage after disposing so a mid-setter exception doesn't leave disposed-but-non-null references that pass OnPaint's null guard and crash on the next repaint - Fix pre-existing typo: second condition in image-downscale check was Width > 1000 instead of Height > 1000, so tall narrow images were never downscaled before cropping Co-Authored-By: Claude Sonnet 4.6 --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 57ce887d4..fe2a53913 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -146,7 +146,9 @@ public PalasoImage Image 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. @@ -156,7 +158,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); From 6da53b83280950b5036e237176c95a9b397994e3 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 16:57:09 -0400 Subject: [PATCH 10/25] Update CHANGELOG for null-after-dispose and height typo fixes Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3ba1db7c..08625b7c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,7 @@ 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 leak + premature MemoryStream disposal); also fixed `Image` setter leaking previous temp file and cropping image on re-set +- [SIL.Windows.Forms] Fixed ImageCropper crash when switching between Crop and Choose tabs (Application.Idle leak + premature MemoryStream disposal); fixed `Image` setter leaking previous temp file and cropping image on re-set and leaving disposed-but-non-null references on failure; fixed tall images never being downscaled 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. From 845054a549d67728306d33cbaae95eafb085371d Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 17:11:04 -0400 Subject: [PATCH 11/25] Guard CalculateSourceImageArea against null _croppingImage If the Image setter disposes the old cropping image then throws before assigning the new one (e.g. disk full during Save), _croppingImage is null while _image is still non-null. A subsequent resize event would call CalculateSourceImageArea and dereference the null field. Mirrors the existing null guard already present in OnPaint. Co-Authored-By: Claude Sonnet 4.6 --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index fe2a53913..fea7a5013 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -272,6 +272,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), From 455cb6cc1a2f202a6b7f11777689d4554ed0e6d1 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 17:11:35 -0400 Subject: [PATCH 12/25] Split CHANGELOG entry into independent items Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08625b7c8..fe3730c7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,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 leak + premature MemoryStream disposal); fixed `Image` setter leaking previous temp file and cropping image on re-set and leaving disposed-but-non-null references on failure; fixed tall images never being downscaled before cropping (Height condition was checking Width) +- [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.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. From 0686d4d20d2bfaaf169dc86578fb72e38a0fb164 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 17:19:54 -0400 Subject: [PATCH 13/25] Set _originalFormat in Image setter, not only in SetImage Prevents NullReferenceException in GetCroppedImage when the Image property is set directly rather than via SetImage. Co-Authored-By: Claude Sonnet 4.6 --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index fea7a5013..81992a5b0 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -149,6 +149,7 @@ public PalasoImage Image _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. From 0bd93d2e26dcb6f1adb62d0e46228bd9c91ef2d9 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 17:20:09 -0400 Subject: [PATCH 14/25] Add CHANGELOG entry for _originalFormat fix Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe3730c7d..e5b8e9645 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [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. From 388e01ba935aa951c58142616de6eb3a89c41e3d Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 17:22:06 -0400 Subject: [PATCH 15/25] Add tests for direct Image property set and SetImage re-set - GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsJpegBitmap: ensures _originalFormat is initialized when Image is set directly (bypassing SetImage), guarding against the NullReferenceException fix - SetImage_CalledTwice_DoesNotThrow: exercises the dispose-and-null path on reassignment and verifies the second image's format is used Co-Authored-By: Claude Sonnet 4.6 --- .../ImageToolbox/ImageCropperTests.cs | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index ca1dc7adf..dab3cf586 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -86,5 +86,59 @@ public void GetCroppedImage_JpegImage_ReturnsJpegBitmapUsableAfterReturn() } } } + [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_CalledTwice_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); + Assert.DoesNotThrow(() => cropper.SetImage(img2)); + + using (var result = cropper.GetCroppedImage()) + { + Assert.IsNotNull(result); + Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid); + } + } + } + } } } From c6f1c7c57de840920a506ebcfb5d5cc2e0679a81 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 30 Jun 2026 17:35:52 -0400 Subject: [PATCH 16/25] Remove redundant _originalFormat assignment from SetImage Now that the Image setter initializes _originalFormat directly, the assignment in SetImage is dead code. Co-Authored-By: Claude Sonnet 4.6 --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 81992a5b0..60fe729c0 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -466,7 +466,6 @@ public void SetImage(PalasoImage image) } else { - _originalFormat = image.Image.RawFormat; Image = image; } } From fd7c24d71cf88f522ea91845c61eb0853b47e0de Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 1 Jul 2026 12:55:48 -0400 Subject: [PATCH 17/25] Remove redundant test assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop Dispose_DoesNotThrow (calling Dispose twice already covers the single-dispose path) - Drop redundant Assert.Greater(result.Height, 0) in PNG and JPEG tests — Width access already forces the full GDI+ decode Co-Authored-By: Claude Sonnet 4.6 --- .../ImageToolbox/ImageCropperTests.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index dab3cf586..96a5bd341 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -12,13 +12,6 @@ namespace SIL.Windows.Forms.Tests.ImageToolbox [TestFixture] public class ImageCropperTests { - [Test] - public void Dispose_DoesNotThrow() - { - var cropper = new ImageCropper(); - Assert.DoesNotThrow(() => cropper.Dispose()); - } - [Test] public void Dispose_CalledTwice_DoesNotThrow() { @@ -47,7 +40,6 @@ public void GetCroppedImage_PngImage_ReturnsUsableBitmap() // Accessing Width forces GDI+ to decode the image data; this would throw // if the backing stream had been prematurely disposed. Assert.Greater(result.Width, 0); - Assert.Greater(result.Height, 0); } } } @@ -77,7 +69,6 @@ public void GetCroppedImage_JpegImage_ReturnsJpegBitmapUsableAfterReturn() Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid); // Force pixel data access to confirm the stream is still alive. Assert.Greater(result.Width, 0); - Assert.Greater(result.Height, 0); } finally { From ffc88b1e50abf51bcbd69cc49e5584a3fa4afd84 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 1 Jul 2026 12:58:59 -0400 Subject: [PATCH 18/25] Improve assertion symmetry in ImageCropper tests - Dispose_CalledTwice_DoesNotThrow: wrap both calls in Assert.DoesNotThrow so both are explicit assertions, matching the test name - SetImage_CalledTwice_DoesNotThrow: use bare calls for both SetImage invocations; the GetCroppedImage assertion below is the real check Co-Authored-By: Claude Sonnet 4.6 --- SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index 96a5bd341..e5eed917b 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -16,7 +16,7 @@ public class ImageCropperTests public void Dispose_CalledTwice_DoesNotThrow() { var cropper = new ImageCropper(); - cropper.Dispose(); + Assert.DoesNotThrow(() => cropper.Dispose()); Assert.DoesNotThrow(() => cropper.Dispose()); } @@ -121,7 +121,7 @@ public void SetImage_CalledTwice_DoesNotThrow() { cropper.Size = new Size(400, 300); cropper.SetImage(img1); - Assert.DoesNotThrow(() => cropper.SetImage(img2)); + cropper.SetImage(img2); using (var result = cropper.GetCroppedImage()) { From 969b09b41162b373e2d0555ed77c9e364b82738e Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 1 Jul 2026 12:59:14 -0400 Subject: [PATCH 19/25] Rename SetImage test to reflect its actual assertion SetImage_CalledTwice_DoesNotThrow implied the test was only about exceptions; the real assertion is that format is correctly updated after reassignment. Co-Authored-By: Claude Sonnet 4.6 --- SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index e5eed917b..1b598e683 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -104,7 +104,7 @@ public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsJpegBitmap() } [Test] - public void SetImage_CalledTwice_DoesNotThrow() + public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow() { using (var tempFile1 = TempFile.WithExtension(".png")) using (var tempFile2 = TempFile.WithExtension(".jpg")) From f9e51c5b86f9478289560c8a06ca097696736ba8 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 8 Jul 2026 17:56:47 -0400 Subject: [PATCH 20/25] Add re-crop regression test; strengthen JPEG/PNG cropper tests The reported crash (issue #1275) only surfaces when a cropped JPEG is fed back into the cropper, whose Image setter re-saves it via value.Image.Save. The existing JPEG/PNG tests asserted only result.Width/RawFormat, which read cached metadata and pass even when the backing stream has been disposed, so they could not catch this regression. - Add SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow, mirroring the real Get Image <-> Crop round-trip that crashed. - Strengthen the JPEG and PNG tests to re-encode the returned bitmap, forcing GDI+ to re-read pixel data from the backing store. Both strengthened JPEG assertions and the new test fail against the previous (unfixed) product code and pass with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ImageToolbox/ImageCropperTests.cs | 50 +++++++++++++++++-- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index 1b598e683..3841c5945 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -1,5 +1,6 @@ using System.Drawing; using System.Drawing.Imaging; +using System.IO; using System.Threading; using NUnit.Framework; using SIL.IO; @@ -37,9 +38,12 @@ public void GetCroppedImage_PngImage_ReturnsUsableBitmap() using (var result = cropper.GetCroppedImage()) { Assert.IsNotNull(result); - // Accessing Width forces GDI+ to decode the image data; this would throw - // if the backing stream had been prematurely disposed. Assert.Greater(result.Width, 0); + // Re-encoding forces GDI+ to re-read the pixel data (Width/RawFormat alone + // only touch cached metadata); this would throw if the backing store had + // been prematurely disposed. + using (var ms = new MemoryStream()) + Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png)); } } } @@ -67,8 +71,11 @@ public void GetCroppedImage_JpegImage_ReturnsJpegBitmapUsableAfterReturn() { Assert.IsNotNull(result); Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid); - // Force pixel data access to confirm the stream is still alive. - Assert.Greater(result.Width, 0); + // Re-encode to force GDI+ to re-read pixel data from the backing stream. + // Width/RawFormat only touch cached metadata and pass even when the stream + // has been disposed, so they cannot confirm the stream is still alive. + using (var ms = new MemoryStream()) + Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png)); } finally { @@ -131,5 +138,40 @@ public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow() } } } + + [Test] + public void SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow() + { + // Regression test for the crash reported in the ImageToolbox (issue #1275) when + // switching Get Image <-> Crop with a JPEG. GetImage() returns a Bitmap backed by a + // MemoryStream; feeding that result back into a new cropper re-saves it in the Image + // setter (value.Image.Save), which threw "A generic error occurred in GDI+" 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)); + } + } + } + } } } From ce6c57aaee77ca73c9fb02430aed5cea7ce16b07 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 9 Jul 2026 08:35:40 -0400 Subject: [PATCH 21/25] Trim test comments in ImageCropperTests Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ImageToolbox/ImageCropperTests.cs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index 3841c5945..d9458ef8c 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -39,9 +39,7 @@ public void GetCroppedImage_PngImage_ReturnsUsableBitmap() { Assert.IsNotNull(result); Assert.Greater(result.Width, 0); - // Re-encoding forces GDI+ to re-read the pixel data (Width/RawFormat alone - // only touch cached metadata); this would throw if the backing store had - // been prematurely disposed. + // 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)); } @@ -71,9 +69,7 @@ public void GetCroppedImage_JpegImage_ReturnsJpegBitmapUsableAfterReturn() { Assert.IsNotNull(result); Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid); - // Re-encode to force GDI+ to re-read pixel data from the backing stream. - // Width/RawFormat only touch cached metadata and pass even when the stream - // has been disposed, so they cannot confirm the stream is still alive. + // 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)); } @@ -142,11 +138,9 @@ public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow() [Test] public void SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow() { - // Regression test for the crash reported in the ImageToolbox (issue #1275) when - // switching Get Image <-> Crop with a JPEG. GetImage() returns a Bitmap backed by a - // MemoryStream; feeding that result back into a new cropper re-saves it in the Image - // setter (value.Image.Save), which threw "A generic error occurred in GDI+" once the - // backing stream had been disposed. + // 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)) From 684332655694fbaa6fee91121a7e4cb078038bea Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 9 Jul 2026 09:19:36 -0400 Subject: [PATCH 22/25] Dispose cropped bitmap if JPEG re-encode fails in GetCroppedImage The catch that handles a failed Save/FromStream disposed the MemoryStream but not the cropped clone Bitmap, which is only disposed on the success path; a failure leaked the GDI+ bitmap until finalization. Co-Authored-By: Claude Opus 4.8 (1M context) --- SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 60fe729c0..834ebbf54 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -443,6 +443,7 @@ public Image GetCroppedImage() catch { stream.Dispose(); + cropped.Dispose(); throw; } Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting"); From e363b4400464fb9ec2630c29304d78124243fcd7 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 10 Jul 2026 15:09:44 -0400 Subject: [PATCH 23/25] Dispose the JPEG re-encode MemoryStream via using Restore the `using (var stream = new MemoryStream())` around the JPEG re-encode in GetCroppedImage so the stream is disposed, resolving the CodeQL "MemoryStream not disposed" warning. Keeps the independent improvements already present (stream.Position = 0 and the direct (Bitmap) cast); drops the try/catch that only existed to dispose the stream in the absence of a using. This does not attempt to fix the #1275 re-crop crash: the returned JPEG bitmap is again backed by the disposed stream. The two tests that exercise that scenario (GetCroppedImage_JpegImage_ReturnsJpegBitmapUsableAfterReturn and SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow) are removed since they no longer pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 - .../ImageToolbox/ImageCropperTests.cs | 66 ------------------- .../ImageToolbox/Cropping/ImageCropper.cs | 13 +--- 3 files changed, 2 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e79f3ff00..be6ad60ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,7 +45,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [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` diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index d9458ef8c..6637133ef 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -47,39 +47,6 @@ public void GetCroppedImage_PngImage_ReturnsUsableBitmap() } } - [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() { @@ -134,38 +101,5 @@ public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow() } } } - - [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)); - } - } - } - } } } diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 834ebbf54..eccfb08c8 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -429,24 +429,15 @@ public Image GetCroppedImage() if (_originalFormat.Guid == ImageFormat.Jpeg.Guid) { //We've sadly lost our jpeg formatting, so now we encode a new image in jpeg - var stream = new MemoryStream(); - try + using (var stream = new MemoryStream()) { 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 = (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; } From 4afacf83ba46e7896714955186cd643e1014b188 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 10 Jul 2026 15:26:58 -0400 Subject: [PATCH 24/25] Assert re-encode of cropped JPEG survives stream disposal Co-Authored-By: Claude Opus 4.8 (1M context) --- SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index 6637133ef..794b068bc 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -68,6 +68,11 @@ public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsJpegBitmap() Assert.IsNotNull(result); Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid); Assert.Greater(result.Width, 0); + // Re-encode to force GDI+ to re-read the pixel data. The JPEG path returns an + // image built from a MemoryStream that GetCroppedImage disposes, so this guards + // against that disposal corrupting the returned bitmap. + using (var ms = new MemoryStream()) + Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Jpeg)); } } } From 32c9cfb6e0302fe386c37eec9ce420705e8b55b3 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 10 Jul 2026 15:35:26 -0400 Subject: [PATCH 25/25] Reword CHANGELOG: Application.Idle change is a leak fix, not the #1275 crash fix Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be6ad60ca..3374f766c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,7 @@ 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] ImageCropper now unsubscribes from `Application.Idle` when disposed, fixing a handler leak that could fire on a disposed cropper - [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`