Skip to content

Unsubscribe ImageCropper's Application.Idle handler on Dispose #1532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
imnasnainaec wants to merge 2 commits into
base: master
Choose a base branch
from
+23 −0
Open

Unsubscribe ImageCropper's Application.Idle handler on Dispose #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f9e025
Unsubscribe ImageCropper's Application.Idle handler on Dispose
imnasnainaec Jul 13, 2026
47c8cd8
Merge branch 'master' into fix/image-cropper-idle-unsubscribe
imnasnainaec Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.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.
Expand Down
20 changes: 20 additions & 0 deletions SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,26 @@ namespace SIL.Windows.Forms.Tests.ImageToolbox
[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());
}
}
}

[Test]
public void SetImage_TallImage_DownscalesCroppingImage()
{
Expand Down
2 changes: 2 additions & 0 deletions SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,8 @@ protected override void Dispose(bool disposing)
components = null;
}

Application.Idle -= Application_Idle;

@hahn-kev hahn-kev Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure how to test this or if this actually does what we want because the subscribe line is Application.Idle += new EventHandler(Application_Idle);, There's not really good options to assert this test without more changes. I would probably write a test like this which ensures that the ImageCropper gets cleaned up as expected (it would not before this bug fix).

[Test]
public void Component_DoesNotLeakMemory_AfterDisposal()
{
    // Arrange & Act
    // We isolate creation/destruction to a separate method to ensure 
    // local strong references go out of scope cleanly before we check GC.
    WeakReference componentReference = CreateAndDisposeComponent();

    // Force full Garbage Collection
    GC.Collect();
    GC.WaitForPendingFinalizers();
    GC.Collect();

    // Assert
    // If it unsubscribed, it should be garbage collected. If it's still alive,
    // the static Application.Idle event is holding a leak path.
    Assert.False(componentReference.IsAlive, 
        "Memory leak detected: Component failed to unsubscribe from Application.Idle.");
}

private WeakReference CreateAndDisposeComponent()
{
    var component = new MyGuiComponent(); // Leverages raw Application.Idle internally
    
    // Simulate the cleanup flow
    component.Dispose(); 
    
    return new WeakReference(component);
}


try
{
if (_savedOriginalImage != null)
Expand Down
Loading