Fix leaking fonts in ConsoleZoomHandler - #2847
Conversation
|
@raghucssit : FYI. |
There was a problem hiding this comment.
Pull request overview
This PR addresses SWT Font resource leaks introduced in ConsoleZoomHandler by changing how zoom fonts are tracked and disposed when consoles are zoomed and when consoles are removed, aligning with the reported regression in issue #2844.
Changes:
- Replaces storing zoom fonts in
TextConsoleattributes with a static per-console font map. - Adjusts when zoom application/disposal work is dispatched asynchronously.
- Updates the console removal test to validate disposal based on the console’s active font rather than a console attribute.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleZoomHandler.java | Switches zoom font tracking to an internal map and changes scheduling/disposal behavior for zoom fonts. |
| debug/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/ConsoleTests.java | Updates the regression test to assert font disposal using console.getFont() after zoom/remove. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Don't use TextConsole attributes for storing fonts, the attributes will be cleaned up on console dispose - Don't run too many tasks async, only those originating from non-UI thread - collect created fonts to dispose after console closes - dispose created console fonts only after console is removed and with a delay to prevent possibly still running StyledText layout operation to run into errors. Fixes eclipse-platform#2844
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleZoomHandler.java:319
applyZoom(...)JavaDoc says it must run on the UI thread and will re-dispatch itself otherwise, but the UI-thread guard was removed. This method is still callable from non-UI threads (e.g., programmatic callers/tests), and it ultimately calls SWT APIs (viaapplyHeight(...)) which can triggerSWTException: Invalid thread access. Reintroduce the conditional re-dispatch to the UI thread to match the contract and avoid crashes.
public static void applyZoom(IWorkbenchPart part, int delta) {
if (!(part instanceof IConsoleView consoleView)) {
return;
}
debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleZoomHandler.java:388
applyHeight(...)no longer re-dispatches to the UI thread, but it performs SWT/UI operations (addPropertyChangeListener,getFont,setFont,new Font). Since it can be reached fromonFontChanged(...)(property change notifications) andapplyZoom(...), this can cause invalid-thread-access errors if invoked off the UI thread. Add a UI-thread guard at the top (and keep the existing behavior of using asyncExec, not syncExec).
private static void applyHeight(TextConsole textConsole, int height) {
// make sure this console's font is (still) being watched, in case it was
// registered before the zoom handler class got loaded, or the listener
// was otherwise not yet attached
textConsole.addPropertyChangeListener(FONT_ENFORCER);
debug/org.eclipse.ui.console/src/org/eclipse/ui/internal/console/ConsoleZoomHandler.java:439
disposeZoomFonts(...)usesDisplay.getDefault().timerExec(...)without ensuring it is called on the UI thread. SWT requirestimerExecto be invoked from the display thread; calling this method directly from a non-UI thread would throwSWTException: Invalid thread accessand leak fonts. Make the method self-contained by re-dispatching whenDisplay.getCurrent()==null, and use the current display for scheduling.
private static void disposeZoomFonts(TextConsole textConsole) {
List<Font> oldFonts = fontsMap.remove(textConsole);
if (oldFonts != null && !oldFonts.isEmpty()) {
Display.getDefault().timerExec(100, () -> {
for (Font font : oldFonts) {
if (font != null && !font.isDisposed()) {
|
With my setup (starting launch group with few processes reporting to system out) I was able to consistently reproduce the leak, with the fix it doesn't appear. Merging this now, because in the current IDE it reports error on almost every start of a process with console. |
Fixes #2844