Skip to content

Fix ImageCropper crash and resource leaks#1530

Draft
imnasnainaec wants to merge 3 commits into
masterfrom
fix/1275-bitmap-instead-of-stream
Draft

Fix ImageCropper crash and resource leaks#1530
imnasnainaec wants to merge 3 commits into
masterfrom
fix/1275-bitmap-instead-of-stream

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Jul 13, 2026 •
edited by andrew-polk
Loading

Copy link
Copy Markdown
Contributor

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.

Fixes #1275

Devin review: https://app.devin.ai/review/sillsdev/libpalaso/pull/1530

This change is Reviewable

@imnasnainaec @claude
Fix ImageCropper crash and resource leaks (BL-1275)
6554b40 
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 <noreply@anthropic.com>
@imnasnainaec imnasnainaec self-assigned this Jul 13, 2026
@github-actions

github-actions Bot commented Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Palaso Tests

     4 files  ± 0       4 suites  ±0   10m 9s ⏱️ +31s
 5 109 tests + 6   4 876 ✅ + 7  233 💤  - 1  0 ❌ ±0 
16 639 runs  +18  15 921 ✅ +21  718 💤  - 3  0 ❌ ±0 

Results for commit 384aae8. ± Comparison against base commit ba68a4a.

This pull request removes 1 and adds 7 tests. Note that renamed tests count towards both. 
SIL.Windows.Forms.Tests.ImageToolbox.ImageToolboxTests ‑ ShowToolboxWith_PreExisting_EnsureRawFormatUnchanged

SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ Dispose_CalledTwiceAfterSettingImage_DoesNotThrow
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_JpegImage_ReturnsUsableBitmap
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsUsableBitmap
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ GetCroppedImage_PngImage_ReturnsUsableBitmap
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ SetImage_Reassign_DisposesPreviousSavedOriginalAndCroppingImage
SIL.Windows.Forms.Tests.ImageToolbox.ImageCropperTests ‑ SetImage_TallImage_DownscalesCroppingImage

♻️ This comment has been updated with latest results.

@imnasnainaec @claude
Trim stale comment and remove obsolete RawFormat test (BL-1275)
e588f15 
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 <noreply@anthropic.com>
@imnasnainaec

imnasnainaec commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Claude summary of a Devin flag

In the Image setter (ImageCropper.cs:146-149), the old _savedOriginalImage and _croppingImage are disposed and nulled before value.Image.Save(...) runs. If that save throws, _image ends up pointing at a PalasoImage whose backing temp file and cropping bitmap are already gone — leaving the object in an inconsistent state.

Claude rebuttal

Reaching that state requires Save to throw mid-setter (out-of-memory, disk full, permissions) — already an unrecoverable condition for this control. The only consumer of that state, GetCroppedImage(), wraps its work in try/catch and returns null via ErrorReport.NotifyUserOfProblem on any failure, so a corrupted _savedOriginalImage just hits that existing safety net instead of crashing. Reordering to save-before-dispose would mean holding both the old and new temp file/cropping bitmap simultaneously — added complexity for an edge case that already degrades safely, and one that's strictly better than the prior behavior of leaking the old resources unconditionally on every re-set.

@imnasnainaec imnasnainaec marked this pull request as draft July 13, 2026 16:36
@imnasnainaec @claude
Expand ImageCropper test coverage and document CalculateSourceImageAr…
384aae8 
…ea 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 <noreply@anthropic.com>
github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 13, 2026
View reviewed changes
Comment thread SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs
// 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<ArgumentException>(() => { var _ = firstCroppingImage.Width; },
@imnasnainaec imnasnainaec mentioned this pull request Jul 13, 2026
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@github-advanced-security github-advanced-security[bot] github-advanced-security[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@imnasnainaec imnasnainaec

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash in ImageToolbox switching between Crop and Choose

2 participants

@imnasnainaec @github-advanced-security