Scope FontRegistry font caching and disposal to the Display that created each font - #4269
Open
HeikoKlare wants to merge 6 commits into
Open
Scope FontRegistry font caching and disposal to the Display that created each font#4269HeikoKlare wants to merge 6 commits into
HeikoKlare wants to merge 6 commits into
Conversation
FontRegistry keeps its symbolic-name-to-FontData mapping in a plain HashMap that is read and written from arbitrary threads: - put(String, FontData[]) is public and carries no UI-thread restriction, unlike the methods that hand out Font instances, - getFontData(), getDescriptor(), getKeySet() and hasValueFor() read it and are likewise unrestricted, with getKeySet() even handing out a live view of the table, and - createFont() writes to it (via the internal put() overload) from whichever thread realizes a font, i.e. from any SWT Display's thread. Unsynchronized HashMap mutation from several threads can corrupt the table itself, not merely produce stale reads. Use a ConcurrentHashMap instead, so concurrent access is safe and getKeySet() returns a weakly-consistent view rather than one that may fail arbitrarily while being iterated. Also fold the internal put()'s read-compare-write of that table into a single atomic Map#put: the previous get()/put() pair could interleave so that two threads both concluded the mapping was unchanged, or that the mapping they replaced was one that had already been overwritten. Storing the given array when it is content-equal to the existing one is a no-op for every reader, so this does not change behavior. The table of realized FontRecords is deliberately left alone here: it is guarded by the documented UI-thread restriction of the methods returning Font instances. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
filterData() is documented to return null for an empty font list, and
does so, but createFont() dereferences its result unconditionally to
check for a zero length. Registering an empty FontData[] under a
symbolic name and then looking that name up therefore fails with
java.lang.NullPointerException:
Cannot read the array length because "validData" is null
instead of falling back to the default font the way an unresolvable
name otherwise does.
Treat null like the empty result it stands for, and add a regression
test registering an empty FontData[] and asserting the default-font
fallback. Note that filterData() never actually returns a zero-length
array - it falls back to the first entry when nothing matches - so the
existing length check alone was dead code.
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
When put() replaced an already-realized font, it obtained the current default font via defaultFontRecord() purely to compare it against the replaced font's instances for staleness. defaultFontRecord() does not just look up an already-realized default font, it also creates and caches one if none exists yet. As a result, replacing an unrelated symbolic name's font could silently allocate and cache a native default-font handle earlier than it would otherwise have been needed, merely as a side effect of an identity comparison. Creating that font is not the only consequence. createFont() registers the data it used under the symbolic name it created the font for, so replacing an unrelated font also registered font data for the default font that no client ever asked for. That is directly observable: hasValueFor(JFaceResources.DEFAULT_FONT) flipped to true, and getKeySet() started reporting the default font, purely because some other name had been re-put. Look up the already-cached default font record directly instead, tolerating that it may not exist yet (in which case the replaced font's instances are unconditionally treated as stale, same as before, since they can never equal a still-unrealized default font). Add a regression test asserting that replacing a realized font leaves the default font unregistered. Beyond the observable effect above, this also matters once the font cache stops being a single shared cache, so that comparing against a display's default font does not have the side effect of allocating that default font on an unrelated display. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
FontRegistry still handles its collections the way it did before generics: explicit Iterators, casts through Object, and one variable reused for two unrelated values. It also lets FontRecord reach into the registry to retire its own fonts, mixing up who owns that decision. Simplify all of that, and write down what cleanOnDisplayDisposal == false already promises. No behavior change, other than put() now invalidating the replaced record entirely before notifying listeners rather than partly after, so the registry is consistent by the time they run. That listeners already see the new font when notified was untested, so a test now covers it. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request updates JFace’s FontRegistry to scope realized font caching and stale-font disposal per SWT Display, preventing one display’s disposal from invalidating fonts owned by other still-live displays in multi-display scenarios (notably on Windows).
Changes:
- Introduces per-
Displayfont-record caches and per-Displaystale-font queues to align font ownership/disposal with the creatingDisplay. - Updates disposal hooking so non-main displays dispose only their own cached/stale fonts, while disposing the main display still clears the whole registry per
ResourceRegistry’s contract. - Expands/adjusts
FontRegistryTestcoverage for multi-display disposal behavior, per-display invalidation onput(), and non-UI-thread fallback expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/FontRegistryTest.java | Adds/updates multi-display and non-UI-thread fallback tests to validate per-display caching, invalidation, and disposal behavior. |
| bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java | Refactors realized-font caching/disposal to be per-Display, adds concurrency-aware structures, and adjusts fallback behavior. |
Suppressed comments (2)
bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java:653
- getExistingFontRecord() can throw a NullPointerException due to a race: it calls containsKey() and then get(); if the display entry is removed between those calls (e.g., during display disposal/clearCaches), displayToFontRecords.get(currentDisplay) becomes null and the subsequent .get(symbolicName) dereference fails. Use a single map get with a null check.
Display currentDisplay = Display.getCurrent();
if (currentDisplay != null && displayToFontRecords.containsKey(currentDisplay)) {
return displayToFontRecords.get(currentDisplay).get(symbolicName);
}
bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java:161
- FontRecord.getItalicFont() creates a new Font with Display.getCurrent() without checking for null. If getItalic() is called from a thread without a current Display (possible via non-UI-thread fallback paths), this can pass null to the Font constructor and fail. Add a null-display guard and return an existing font (or fall back to the base font) instead of trying to allocate.
FontData[] italicData = getModifiedFontData(SWT.ITALIC);
italicFont = new Font(Display.getCurrent(), italicData);
return italicFont;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ted each font FontRegistry cached fonts in a single, display-agnostic table and disposed all of them whenever any one Display it had been used from was disposed. With more than one SWT Display in play, the first Display to be disposed therefore tore down the fonts of every other Display as well, leaving live Displays with disposed fonts. Which Display that is happens to be arbitrary. The registry now keeps one font cache per Display, populated and disposed independently, so disposing a Display only disposes its own fonts. Fonts replaced via put() are likewise queued as stale per display and disposed together with the display that owns them. Note that this is about ownership and lifetime, not about which Display a font may be used on. Handing a font realized on one Display to another is not by itself a problem; having it disposed underneath that other Display is. The Display the registry was created for is kept as the main display and assumed to outlive all others, which makes its fonts the ones that are always safe to fall back to. A thread without a Display of its own can neither scope a lookup nor create a font, so it falls back to the default font realized there instead of failing outright. Disposal of the main display still disposes the whole registry, so clearCaches() keeps the "disposes all currently allocated resources" contract it inherits from ResourceRegistry. The per-display caches are concurrent, since Displays run on their own threads and put() invalidates the records of every display from whichever thread it is called on. For a single-display application this is behavior-preserving: the per-display cache then holds exactly what the global one did, and that Display is the main display. Since a record is now only reachable through the cache of the display it belongs to, and can only have got there via createFont(), which already hooked that display for disposal, FontRecord no longer needs the registry to obtain its display when realizing a bold or italic font. Display.getCurrent() returns the same value in every reachable case, and with the stale-font handling already moved out, that leaves no reference to the enclosing registry, so the class becomes static. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
HeikoKlare
force-pushed
the
fontregistry-multidisplay-step2
branch
from
August 21, 2026 17:01
24fda37 to
9d6f4d5
Compare
Contributor
Test Results 858 files ± 0 858 suites ±0 57m 31s ⏱️ + 6m 58s Results for commit 9d6f4d5. ± Comparison against base commit 5b3e6dd. This pull request removes 1 and adds 10 tests. Note that renamed tests count towards both. |
HeikoKlare
marked this pull request as ready for review
August 24, 2026 12:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
This PR is based on five preparatory PRs, which make up its first five commits. They should be reviewed and merged first — only the last commit,
Scope FontRegistry font caching and disposal to the Display that created each font, is what this PR actually covers.The problem
FontRegistrykeeps its realized fonts in a single, display-agnostic table, and hooks disposal on everyDisplayit has been used from. Whichever of those Displays is disposed first runsclearCaches()and tears down the entire table — including the fonts owned by every other, still-live Display. Those Displays are then left holding disposed fonts, and which Display triggers this is arbitrary; typically it is not the one the registry was created for.Fonts replaced via
put()have the same problem from the other side: they are queued in one global list of stale fonts and disposed together with whichever Display happens to go first, rather than with the Display that owns them.None of this is observable in a single-display application, which is nearly every Eclipse application. It becomes observable as soon as a second
Displayexists, which is possible on Windows.The solution
The registry keeps one font cache per
Display, populated and disposed independently. ADisplayrealizes and owns its own fonts, and disposing it disposes exactly those, leaving every otherDisplayuntouched. Replaced fonts are likewise queued as stale per display and disposed with the display that owns them.Note this is about ownership and lifetime, not about which
Displaya font may be used on. Handing a font realized on oneDisplayto another is not by itself a problem; having it disposed underneath that otherDisplayis.The
Displaythe registry was created for is kept as the main display and assumed to outlive all others, which makes its fonts the ones that are always safe to fall back to. Two things build on that:Displayof its own can neither scope a lookup nor create a font, so it falls back to the default font realized on the main display instead of failing outright.clearCaches()keeps the "disposes all currently allocated resources" contract it inherits fromResourceRegistry.The per-display caches are concurrent, since Displays run on their own threads and
put()invalidates the records of every display from whichever thread it is called on.For a single-display application this is behaviour-preserving: the per-display cache then holds exactly what the global one did, and that
Displayis the main display.The verification
Tests were added on both sides of the line:
multipleDisplayDispose,multipleDisplayDispose_boldFontandmultipleDisplayDispose_italicFontare existing tests whose expectation is corrected from "the same font on every Display" to "a Display that has to realize a font owns it";multipleDisplayDispose_noDisposeOtherThreadFontsis new and covers the defect above directly.put()invalidates the cached font on every Display, thatcleanOnDisplayDisposal == falsestill caches and still never disposes automatically, and that a thread without aDisplaystill falls back to the default font.🤖 Generated with Claude Code