IEP-1724: SWT resource leaks and disposal crashes in IDF Size Analysis Editor#1410
Conversation
📝 WalkthroughWalkthroughThese changes add explicit resource disposal logic to three SWT UI editor components in the size analysis UI package. Fonts, images, and editor instances are now properly disposed when their parent widgets are destroyed to prevent resource leaks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeChartsComposite.java (1)
318-323: Correct disposal pattern with minor redundancy.The dispose listener properly ensures the SWT
Imageis released when theLabelis disposed. The!swtImg.isDisposed()check is good practice.Minor: The
swtImg != nullcheck is redundant sinceswtImgis effectively final and was successfully created at line 315 before this listener is registered—it can never be null when the lambda executes.🔧 Optional: Remove redundant null check
imgLabel.addDisposeListener(e -> { - if (swtImg != null && !swtImg.isDisposed()) + if (!swtImg.isDisposed()) { swtImg.dispose(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeChartsComposite.java` around lines 318 - 323, The dispose listener on imgLabel uses a redundant null check for swtImg; since swtImg is created before registering the listener it cannot be null when invoked, so simplify the lambda in imgLabel.addDisposeListener(...) to only check !swtImg.isDisposed() before calling swtImg.dispose(); remove the swtImg != null branch to reduce dead code while keeping the isDisposed guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeChartsComposite.java`:
- Around line 318-323: The dispose listener on imgLabel uses a redundant null
check for swtImg; since swtImg is created before registering the listener it
cannot be null when invoked, so simplify the lambda in
imgLabel.addDisposeListener(...) to only check !swtImg.isDisposed() before
calling swtImg.dispose(); remove the swtImg != null branch to reduce dead code
while keeping the isDisposed guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7d59c0b-1ed0-403d-9e17-edacca3e6eb8
📒 Files selected for processing (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeAnalysisEditor.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeChartsComposite.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/size/IDFSizeOverviewComposite.java
alirana01
left a comment
There was a problem hiding this comment.
Hi @sigmaaa this is fine but maybe add a small dispose listener that can be reused on other places as well a bit generic and maybe just one instance of that will be fine for every swt component but that can be done in other PR
|
LGTM 👍 |
Description
During testing of the application size analysis, I noticed some SWT exceptions and discovered resource leaks that are addressed in this PR.
Fixes # (IEP-1724)
Type of change
Please delete options that are not relevant.
How has this been tested?
Open Activity Monitor and run the application size analysis. Go to Charts and click “Switch to …”. The error log should contain no SWT exceptions, and there should be no significant increase in memory usage.
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit