Fix IllegalArgumentException when drawing unified diff code minings#2773
Fix IllegalArgumentException when drawing unified diff code minings#2773tobiasmelcher wants to merge 1 commit into
Conversation
UnifiedDiffLineHeaderCodeMining.draw() caches Font references in its
foregrounds list and in cachedFont. These fonts can be disposed under
the cache when:
1. The mining is disposed while a paint event for it is still in flight
(e.g. "hide diff" recomputes the mining list and a pending
paintControl still references the old annotations).
2. The widget font is replaced by an instance with equal FontData. The
previous invalidation used Font.equals(), which is a value compare
and does not detect a disposed handle.
Once an exception was thrown, the stale cache was never refreshed, so
every subsequent scroll repaint produced another error.
Fix:
- dispose() nulls the cached state so a stray paint after disposal
cannot enter the fast path.
- Cache invalidation also checks cachedFont.isDisposed().
- The fast-path loop validates f.font.isDisposed() before use; if stale,
the cache is dropped and the slow path rebuilds it in the same paint.
Fixes eclipse-platform#2772
|
@iloveeclipse Unfortunately, I was unable to reproduce the issue on a Linux machine. Were there any exceptions in the error log besides the java.lang.IllegalArgumentException? From the current behavior, it seems that cached fonts may have been disposed unexpectedly, but I’m not sure under which circumstances this could happen. |
There was a problem hiding this comment.
Pull request overview
This PR fixes repeated IllegalArgumentException errors thrown while painting unified diff line-header code minings when cached Font instances become disposed (e.g., after “hide diff” triggers recomputation while a paint is still in flight). It also hardens unified diff navigation runnables against empty diff lists.
Changes:
- Clear cached mining paint state on
dispose()and invalidate caches when the cached widget font is disposed. - Detect disposed cached foreground fonts during the fast-path draw loop and fall back to rebuilding caches in the same paint.
- Prevent
NextRunnable/PreviousRunnablefrom indexing into empty unified diff lists.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/UnifiedDiffCodeMiningProvider.java |
Improves cache invalidation and draw fast-path robustness when fonts are disposed; clears cached state on dispose. |
team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/PreviousRunnable.java |
Adds a guard to return early when there are no diffs to navigate. |
team/bundles/org.eclipse.compare/compare/org/eclipse/compare/unifieddiff/internal/NextRunnable.java |
Adds a guard to return early when there are no diffs to navigate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| lastRectangle = null; | ||
| backgrounds = null; | ||
| foregrounds = null; | ||
| styledFonts.clear(); | ||
| fontIsDisposed = true; | ||
| break; | ||
| } |
|
Not sure if it is related, I always use Eclipse with disabled themes. Beside this, RHEL 9.6 with X11, KDE5, nothing special. I believe all errors were same, but I will check tomorrow, they were literally flooding my error log. |
UnifiedDiffLineHeaderCodeMining.draw() caches Font references in its foregrounds list and in cachedFont. These fonts can be disposed under the cache when:
Once an exception was thrown, the stale cache was never refreshed, so every subsequent scroll repaint produced another error.
Fix:
Fixes #2772