Fix ImageCropper crash when switching Crop/Choose tabs#1521
Draft
imnasnainaec wants to merge 23 commits into
Draft
Fix ImageCropper crash when switching Crop/Choose tabs#1521imnasnainaec wants to merge 23 commits into
imnasnainaec wants to merge 23 commits into
Conversation
…posal (#1275) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
imnasnainaec
changed the title
Contributor
Palaso Tests 4 files ± 0 4 suites ±0 10m 33s ⏱️ +35s Results for commit 6843326. ± Comparison against base commit 4840de8. This pull request skips 1 test.♻️ This comment has been updated with latest results. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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>
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
2 participants
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.
Summary
Fixes several resource-management bugs in
ImageCropper, most of which could cause crashes in the Image Toolbox: anApplication.Idlesubscription leak that fired on a disposed object when switching tabs; a prematurely disposedMemoryStreamthat broke JPEG bitmaps afterGetCroppedImagereturned; and multiple gaps in theImagesetter where old resources were leaked or left in a state that could crash a subsequent repaint or resize. Also fixes two other bugs found while reviewing the same code.Details
Application.Idlesubscription leak inImageCropper.Dispose()— handler was never unsubscribed, firing on a disposed object when the user switched tabs; unsubscription added before thetry/catchso it cannot be skippedGetCroppedImage()returning a JPEG-format bitmap backed by a prematurely disposedMemoryStream— GDI+ bitmaps hold a lazy reference to the source stream;usingremoved so the stream outlives the returnedBitmapas Bitmapwith a direct cast so an unexpected non-Bitmap fromImage.FromStreamthrows immediately rather than silently propagatingnullImagesetter leaking previous_savedOriginalImage(TempFile) and_croppingImageon re-set; fields are nulled after disposal so a mid-setter failure does not leave disposed-but-non-null references that passOnPaint's null guard and crash on repaintCalculateSourceImageAreanot guarding against null_croppingImage, which could crash a resize event if the setter threw after disposing the old imageWidth > 1000twice instead ofWidth || Height, so tall narrow images were never downscaled before croppingImagesetter not initializing_originalFormat, which caused aNullReferenceExceptioninGetCroppedImagewhen the property was set directly rather than viaSetImageTest plan
Fixes #1275
Devin review: https://app.devin.ai/review/sillsdev/libpalaso/pull/1521
This change is