From 0f9e02594f42d90c7f642fadb39fc16fe4ede56a Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 13 Jul 2026 14:25:50 -0400 Subject: [PATCH 1/3] Unsubscribe ImageCropper's Application.Idle handler on Dispose ImageCropper subscribed to the static Application.Idle event in its constructor but never unsubscribed in Dispose, so the handler could still fire after the control was disposed. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 1 + .../ImageToolbox/ImageCropperTests.cs | 35 +++++++++++++++++++ .../ImageToolbox/Cropping/ImageCropper.cs | 2 ++ 3 files changed, 38 insertions(+) create mode 100644 SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d2b8374c..dcf0ab0c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,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 caused by its `Application.Idle` handler never being unsubscribed, so it could fire on a disposed instance - [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..93ef64969 --- /dev/null +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -0,0 +1,35 @@ +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_CalledTwiceAfterSettingImage_DoesNotThrow() + { + 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()); + } + } + } + } +} diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 4ded520a6..92551ea0d 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -482,6 +482,8 @@ protected override void Dispose(bool disposing) components = null; } + Application.Idle -= Application_Idle; + try { if (_savedOriginalImage != null) From fed32e01aee6ee165d0c7921edeb17d216f40519 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Thu, 16 Jul 2026 09:27:43 -0400 Subject: [PATCH 2/3] Add memory-leak regression test for ImageCropper Idle unsubscribe The existing smoke test passed even without the fix. This test creates and disposes an ImageCropper, then asserts the instance is garbage collected, which only happens once Dispose unsubscribes from the static Application.Idle event. Verified to fail on the un-fixed code and pass with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ImageToolbox/ImageCropperTests.cs | 53 ++++++++++++++++--- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index 238d7dba2..7cd081087 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.Reflection; +using System.Runtime.CompilerServices; using System.Threading; using NUnit.Framework; using SIL.IO; @@ -23,8 +25,7 @@ public void Dispose_CalledTwiceAfterSettingImage_DoesNotThrow() using (var palasoImage = PalasoImage.FromFile(tempFile.Path)) { - var cropper = new ImageCropper(); - cropper.Size = new Size(400, 300); + var cropper = new ImageCropper { Size = new Size(400, 300) }; cropper.SetImage(palasoImage); Assert.DoesNotThrow(() => cropper.Dispose()); @@ -33,6 +34,44 @@ public void Dispose_CalledTwiceAfterSettingImage_DoesNotThrow() } } + [Test] + public void Dispose_AllowsGarbageCollection() + { + // The ImageCropper subscribes to the static Application.Idle event in its + // constructor, so it needs to unsubscribe on Dispose. + var reference = CreateAndDisposeImageCropper(); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + Assert.That(reference.IsAlive, Is.False, + "ImageCropper was not garbage collected after disposal. " + + "It may be subscribed to a static event."); + } + + // Kept in a separate, non-inlined method so the local ImageCropper reference is + // guaranteed out of scope before the caller forces garbage collection. + [MethodImpl(MethodImplOptions.NoInlining)] + private static WeakReference CreateAndDisposeImageCropper() + { + 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 { Size = new Size(400, 300) }; + cropper.SetImage(palasoImage); + + var reference = new WeakReference(cropper); + cropper.Dispose(); + return reference; + } + } + } + [Test] public void SetImage_TallImage_DownscalesCroppingImage() { @@ -42,12 +81,11 @@ public void SetImage_TallImage_DownscalesCroppingImage() bmp.Save(tempFile.Path, ImageFormat.Png); using (var palasoImage = PalasoImage.FromFile(tempFile.Path)) - using (var cropper = new ImageCropper()) + using (var cropper = new ImageCropper { Size = new Size(400, 300) }) { - cropper.Size = new Size(400, 300); cropper.SetImage(palasoImage); - // Not owned by this test -- the cropper still holds and will dispose this itself. + // Don't dispose this: the cropper owns _croppingImage and disposes it itself. var croppingImage = GetCroppingImage(cropper); Assert.Less(croppingImage.Height, 1200, @@ -65,12 +103,11 @@ public void SetImage_WideImage_DownscalesCroppingImage() bmp.Save(tempFile.Path, ImageFormat.Png); using (var palasoImage = PalasoImage.FromFile(tempFile.Path)) - using (var cropper = new ImageCropper()) + using (var cropper = new ImageCropper { Size = new Size(400, 300) }) { - cropper.Size = new Size(400, 300); cropper.SetImage(palasoImage); - // Not owned by this test -- the cropper still holds and will dispose this itself. + // Don't dispose this: the cropper owns _croppingImage and disposes it itself. var croppingImage = GetCroppingImage(cropper); Assert.Less(croppingImage.Width, 1200, From 020c3b72624235b39cea017f2c7aa498e124d009 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Fri, 17 Jul 2026 08:18:35 -0400 Subject: [PATCH 3/3] Add flaky warning --- SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs index 7cd081087..6ae60ac88 100644 --- a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -34,7 +34,10 @@ public void Dispose_CalledTwiceAfterSettingImage_DoesNotThrow() } } + // Garbage collection is non-deterministic, so this test may be flaky. + // If it turns out to be a problem, drop this test and its supporting method. [Test] + public void Dispose_AllowsGarbageCollection() { // The ImageCropper subscribes to the static Application.Idle event in its