Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe #2762

Closed
wants to merge 2 commits into from
Closed

8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe #2762

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
758bfdd
[PATCH] Thread-safe initialization of SunFontManager.platformFontMap
turbanoff Feb 27, 2021
f1911b4
[PATCH] Thread-safe initialization of SunFontManager.platformFontMap
turbanoff Feb 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/java.desktop/share/classes/sun/font/SunFontManager.java
View file
Open in desktop
Expand Up @@ -1433,6 +1433,7 @@ public static class FamilyDescription {
public String boldFileName;
public String italicFileName;
public String boldItalicFileName;
boolean failed;
}

static HashMap<String, FamilyDescription> platformFontMap;
Expand All @@ -1445,8 +1446,10 @@ public HashMap<String, FamilyDescription> populateHardcodedFileNameMap() {
}

Font2D findFontFromPlatformMap(String lcName, int style) {
HashMap<String, FamilyDescription> platformFontMap = SunFontManager.platformFontMap;
if (platformFontMap == null) {
platformFontMap = populateHardcodedFileNameMap();
SunFontManager.platformFontMap = platformFontMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it is enough for "thread-safe initialization". The "platformFontMap" might not be null, but its content can be broken/incomplete at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2691 (comment)

In short, previously platformFontMap references were field set/gets and may be inconsistent due to concurrency. It is now moved to a local variable; also the map modifications have been moved to fields to avoid concurrency issues with hash map as far as I can see.

Copy link
Member

@mrserb mrserb Feb 28, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thread may create the local platformFontMap, then set the static SunFontManager.platformFontMap field, and then initialize the platformFontMap or mix these operations. The second thread may see non-null SunFontManager.platformFontMap which is not still initialized.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made volatile to guarantee memory visibility if thread saw non-null value.

Copy link
Member

@mrserb mrserb Mar 6, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any other questions, will file a bug and run the tests.

}

if (platformFontMap == null || platformFontMap.size() == 0) {
Expand All @@ -1460,7 +1463,7 @@ Font2D findFontFromPlatformMap(String lcName, int style) {
}

FamilyDescription fd = platformFontMap.get(firstWord);
if (fd == null) {
if (fd == null || fd.failed) {
return null;
}
/* Once we've established that its at least the first word,
Expand Down Expand Up @@ -1527,7 +1530,7 @@ Font2D findFontFromPlatformMap(String lcName, int style) {
if (FontUtilities.isLogging()) {
FontUtilities.logInfo("Hardcoded file missing looking for " + lcName);
}
platformFontMap.remove(firstWord);
fd.failed = true;
return null;
}

Expand All @@ -1554,7 +1557,7 @@ public Boolean run() {
if (FontUtilities.isLogging()) {
FontUtilities.logInfo("Hardcoded file missing looking for " + lcName);
}
platformFontMap.remove(firstWord);
fd.failed = true;
return null;
}

Expand Down