From cf6cf8e76759e2babf2211aebdb456c95a8de768 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Fri, 14 Aug 2026 17:41:57 +0200 Subject: [PATCH] Centralize FontRegistry font-record caching in createFont The mapping from a symbolic font name to its realized FontRecord was cached at each call site (defaultFontRecord() and getFontRecord()) after invoking createFont(), rather than by createFont() itself. As a side effect, a symbolic name that had never been explicitly registered and only ever resolved through the default-font fallback got cached as an alias pointing at the very same FontRecord instance used for the default font. Registering real data for such a name later would then remove only that alias, but still treat the (still live and cached) default record as replaced, incorrectly queuing its already-realized bold/italic fonts for disposal even though the default font remains in active use under its own name. Move the caching into createFont(), where the record is actually created, so every symbolic name is cached exactly once, at the single place responsible for creating it. This removes the accidental aliasing and the incorrect staleness it could cause. Note that with the registry's current single, display-wide cache, this inconsistency has no externally observable effect: disposing any display already tears down the entire cache and all stale fonts together in one step, so the aliased default font and its "stale" bold/italic variants are always disposed at the same time regardless. The fix still removes the incorrect internal state, and matters once disposal is no longer coupled that way, e.g. for a per-display cache where a font may otherwise be readable, writable and disposed of from separate places. With the caching gone from the end of getFontRecord(), the early return in its non-UI-thread branch no longer skips anything: it and the final return became identical. Drop it, together with the comment explaining that it avoids caching the default font under the requested name. What that comment promises still holds, a later lookup from the UI thread still creates the proper font, but that is now a consequence of createFont() doing the caching rather than of this return. Add a characterization test pinning that put() on a name only ever resolved via the default-font fallback does not disturb the default font's already-realized bold/italic instances. It does not fail without this fix given the current coupling described above, but documents the intended contract and guards against regressions once that coupling changes. Assisted-by: Claude Sonnet 5 --- .../org/eclipse/jface/resource/FontRegistry.java | 8 +++----- .../jface/tests/resources/FontRegistryTest.java | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 5 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 ec360f9dcca..873efdb2b0f 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 @@ -505,7 +505,9 @@ private FontRecord createFont(String symbolicName, FontData[] fonts) { //Do not fire the update from creation as it is not a property change put(symbolicName, validData, false); Font newFont = new Font(display, validData); - return new FontRecord(newFont, validData); + FontRecord newRecord = new FontRecord(newFont, validData); + stringToFontRecord.put(symbolicName, newRecord); + return newRecord; } private Display getDisplayAndHookForDisposal() { @@ -582,7 +584,6 @@ record = createFont(JFaceResources.DEFAULT_FONT, fontData); record = createFont(JFaceResources.DEFAULT_FONT, defaultFont.getFontData()); defaultFont.dispose(); } - stringToFontRecord.put(JFaceResources.DEFAULT_FONT, record); return record; } @@ -698,13 +699,10 @@ private FontRecord getFontRecord(String symbolicName) { if (Display.getCurrent() == null) { // log error but don't throw an exception to preserve existing functionality String msg = "Unable to create font \"" + symbolicName + "\" in a non-UI thread. Using default font instead."; //$NON-NLS-1$ //$NON-NLS-2$ Policy.logException(new SWTException(msg)); - return fontRecord; // don't add it to the cache; if later asked from UI thread, a proper font will be created } } - stringToFontRecord.put(symbolicName, fontRecord); return fontRecord; - } @Override 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 7aa7f7dfcd0..8cb68cda346 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 @@ -140,6 +140,22 @@ public void defaultFont_isStableAcrossLookupsOfOtherNames() { assertSame(defaultFont, fontRegistry.get(JFaceResources.DEFAULT_FONT)); } + @Test + public void put_onNameOnlyResolvedViaDefaultFallback_doesNotStaleDefaultFontsBoldAndItalic() { + FontRegistry fontRegistry = new FontRegistry(); + Font defaultBold = fontRegistry.getBold(JFaceResources.DEFAULT_FONT); + Font defaultItalic = fontRegistry.getItalic(JFaceResources.DEFAULT_FONT); + + // never explicitly registered, so this only ever resolves via the default-font fallback + fontRegistry.get("neverRegisteredName"); + + // registering data for that name must not disturb the still-live default font record + fontRegistry.put("neverRegisteredName", new FontData[] { new FontData("Arial", 12, SWT.NORMAL) }); + + assertSame(defaultBold, fontRegistry.getBold(JFaceResources.DEFAULT_FONT)); + assertSame(defaultItalic, fontRegistry.getItalic(JFaceResources.DEFAULT_FONT)); + } + @Test public void get_fontFromNonUIThreadFallback_doesNotOverwriteDefaultFont() throws Throwable { FontRegistry fontRegistry = new FontRegistry();