[SIL.Windows.Forms] Fix ImageCropper resource leaks and cropping correctness bugs#1528
Draft
imnasnainaec wants to merge 26 commits into
Draft
[SIL.Windows.Forms] Fix ImageCropper resource leaks and cropping correctness bugs#1528imnasnainaec wants to merge 26 commits into
imnasnainaec wants to merge 26 commits into
Conversation
…posal (#1275) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents NullReferenceException in GetCroppedImage when the Image property is set directly rather than via SetImage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Now that the Image setter initializes _originalFormat directly, the assignment in SetImage is dead code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Contributor
Palaso Tests 4 files ± 0 4 suites ±0 10m 18s ⏱️ +20s For more details on these failures, see this check. Results for commit 32c9cfb. ± Comparison against base commit 4840de8. This pull request skips 1 test.♻️ This comment has been updated with latest results. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
imnasnainaec
changed the title
…crash fix Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
imnasnainaec
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #1275 (does not fully resolve it — the tab-switch crash fix is being deferred to a separate PR). This PR lands a set of related
ImageCropperresource, correctness, and robustness fixes found while investigating that crash.Fixes
Imagesetter leaked the previous temp file and cropping image on re-set. Old resources are now disposed and the fields nulled, so re-settingImageno longer leaks and no longer leaves disposed-but-non-null references.Widthtwice; the second condition now correctly checksHeight.GetCroppedImagethrewNullReferenceExceptionwhenImagewas set directly (not viaSetImage)._originalFormatis now initialized in theImagesetter, and the redundant assignment inSetImagewas removed.GetCroppedImage. The re-encodeMemoryStreamis reset (Position = 0) before reading, disposed viausing, and the cropped bitmap is disposed on the failure path. The result cast was tightened to(Bitmap).CalculateSourceImageAreaguarded against a null_croppingImage.ImageCroppernow unsubscribes fromApplication.Idleon dispose. This fixes a handler leak (the handler could otherwise fire on a disposed cropper). Note: this is a related safety fix and is not, on its own, confirmed to resolve the Crash in ImageToolbox switching between Crop and Choose #1275 crash.Tests
Added
ImageCropperTestscovering:Disposedoes not throw.Imageproperty returns a JPEG bitmap that re-encodes cleanly (regression guard for the disposed re-encode stream).SetImageupdates the format and does not throw.Devin review: https://app.devin.ai/review/sillsdev/libpalaso/pull/1528
This change is