From 11c2271a49068d01dec78af560708607edfb70e4 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Thu, 5 Dec 2024 11:16:21 +0100 Subject: [PATCH 1/2] Properly dispose font in FontRegistry upon display disposal When retrieving a font from a FontRegistry instance that is not cached in the registry yet, it will be created on the current display. In order to dispose the fonts of a display upon the display's disposal, the FontRegistry registers a dispose hook at the display instance. The current implementation only does this for the first display that is instantiated. For every further display, which may be created on Windows systems, no hook is added. Thus, if an additional display is disposed, its fonts remain in the FontRegistry, referring to a disposed display. This change improved the FontRegistry implementation to properly register a dispose hook to every display for which it contains fonts. It also adds an according regression test. --- .../eclipse/jface/resource/FontRegistry.java | 9 +-- .../tests/resources/FontRegistryTest.java | 56 +++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java index ef4cfa0f0a1..af6ece8b40e 100644 --- a/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java +++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java @@ -25,6 +25,7 @@ import java.util.MissingResourceException; import java.util.ResourceBundle; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import org.eclipse.core.runtime.Assert; import org.eclipse.jface.util.Policy; @@ -207,7 +208,7 @@ void addAllocatedFontsToStale(Font defaultFont) { */ protected Runnable displayRunnable = this::clearCaches; - private boolean displayDisposeHooked; + private final Set displayDisposeHooked = ConcurrentHashMap.newKeySet(); private final boolean cleanOnDisplayDisposal; @@ -492,7 +493,7 @@ private FontRecord createFont(String symbolicName, FontData[] fonts) { if (display == null) { return null; } - if (cleanOnDisplayDisposal && !displayDisposeHooked) { + if (cleanOnDisplayDisposal && !displayDisposeHooked.contains(display)) { hookDisplayDispose(display); } @@ -718,7 +719,7 @@ protected void clearCaches() { stringToFontRecord.clear(); staleFonts.clear(); - displayDisposeHooked = false; + displayDisposeHooked.remove(Display.getCurrent()); } /** @@ -736,7 +737,7 @@ private void disposeFonts(Iterator iterator) { * Hook a dispose listener on the SWT display. */ private void hookDisplayDispose(Display display) { - displayDisposeHooked = true; + displayDisposeHooked.add(display); display.disposeExec(displayRunnable); } diff --git a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java index b578d9f6666..f7fae9ee6f0 100644 --- a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java +++ b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java @@ -14,8 +14,20 @@ package org.eclipse.jface.tests.resources; import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.atomic.AtomicReference; + +import org.eclipse.core.runtime.Platform.OS; +import org.eclipse.jface.resource.FontRegistry; import org.eclipse.jface.resource.JFaceResources; +import org.eclipse.swt.graphics.Device; import org.eclipse.swt.graphics.Font; import org.eclipse.swt.graphics.FontData; import org.eclipse.swt.widgets.Display; @@ -41,4 +53,48 @@ public void testBug544026() { assertArrayEquals(fontData, JFaceResources.getDefaultFont().getFontData()); } + @Test + public void multipleDisplayDispose() { + assumeTrue("multiple Display instance only allowed on Windows", OS.isWindows()); + + FontRegistry fontRegistry = new FontRegistry(); + Display secondDisplay = initializeDisplayInSeparateThread(); + Font fontOnSecondDisplay = secondDisplay.syncCall(fontRegistry::defaultFont); + + Font fontOnThisDisplayBeforeSecondDisplayDispose = fontRegistry.defaultFont(); + Device displayOfFontOnSecondDisplay = fontOnSecondDisplay.getDevice(); + // font registry returns same font for every display + assertEquals(secondDisplay, displayOfFontOnSecondDisplay); + assertEquals(fontOnThisDisplayBeforeSecondDisplayDispose, fontOnSecondDisplay); + + // after disposing font's display, registry should reinitialize the font + secondDisplay.syncExec(secondDisplay::dispose); + assertTrue(fontOnSecondDisplay.isDisposed()); + Font fontOnThisDisplayAfterSecondDisplayDispose = fontRegistry.defaultFont(); + assertNotEquals(fontOnThisDisplayAfterSecondDisplayDispose, fontOnSecondDisplay); + } + + private static Display initializeDisplayInSeparateThread() { + AtomicReference displayReference = new AtomicReference<>(); + new Thread(() -> { + Display display = new Display(); + displayReference.set(display); + while (!display.isDisposed()) { + if (!display.readAndDispatch()) { + display.sleep(); + } + } + }, "async display creation").start(); + waitForDisplayInstantiation(displayReference); + return displayReference.get(); + } + + private static void waitForDisplayInstantiation(AtomicReference displayReference) { + Instant maximumEndTime = Instant.now().plus(Duration.ofSeconds(10)); + while (displayReference.get() == null) { + assertFalse("display was not instantiated in time", Instant.now().isAfter(maximumEndTime)); + Thread.yield(); + } + } + } From 140773f10de7c8fba9b3d6b388d930a5efe15d62 Mon Sep 17 00:00:00 2001 From: Eclipse Platform Bot Date: Fri, 6 Dec 2024 11:54:16 +0000 Subject: [PATCH 2/2] Version bump(s) for 4.35 stream --- bundles/org.eclipse.jface/META-INF/MANIFEST.MF | 2 +- tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bundles/org.eclipse.jface/META-INF/MANIFEST.MF b/bundles/org.eclipse.jface/META-INF/MANIFEST.MF index de20b5090a8..47bd323cd19 100644 --- a/bundles/org.eclipse.jface/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.jface/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.jface;singleton:=true -Bundle-Version: 3.35.100.qualifier +Bundle-Version: 3.35.200.qualifier Bundle-Vendor: %providerName Bundle-Localization: plugin Export-Package: org.eclipse.jface, diff --git a/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF index 48ffc6df3f9..77076b9fd83 100644 --- a/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF +++ b/tests/org.eclipse.jface.tests/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %Bundle-Name Bundle-SymbolicName: org.eclipse.jface.tests -Bundle-Version: 1.4.700.qualifier +Bundle-Version: 1.4.800.qualifier Automatic-Module-Name: org.eclipse.jface.tests Bundle-RequiredExecutionEnvironment: JavaSE-17 Require-Bundle: org.junit;bundle-version="4.12.0",