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.

Author: HeikoKlare

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<Display> 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<Font> iterator) {
 	 * Hook a dispose listener on the SWT display.
 	 */
 	private void hookDisplayDispose(Display display) {
-		displayDisposeHooked = true;
+		displayDisposeHooked.add(display);
 		display.disposeExec(displayRunnable);
 	}
 

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<Display> 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<Display> 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();
+		}
+	}
+
 }