openjdk
8339068: [Linux] NPE: Cannot read field "firstFont" because "<local4>" is null
This PR adds a couple of null checks to LogicalFont and FTFactory, that make use of FontConfigManager::getFontConfigFont, which under certain cases, can return null.
I've tested this PR on both Linux (Ubuntu 22.04.4) and Android, just using -Dprism.useFontConfig=false.
On Ubuntu, I've manually added some font files to the JDK path $JAVA_HOME/lib/fonts (which didn't exist), and the test passes now, though this message is still printed out:
Error: JavaFX detected no fonts! Please refer to release notes for proper font configuration
which is confusing to me, because fonts where found in the JDK path after all, and even in the case that there were no fonts found, "the release notes" is an ambiguous reference for the user.
Also, instead of adding fonts to the JDK, I tested adding a logicalfonts.properties file with -Dprism.fontdir and without fontConfig, but this case was already working before the patch for this PR, and still works after it.
Note that if there are no fonts in the JDK path, and prism.fontdir is not provided, without fontConfig, after this PR, there will still be an exception:
Caused by: java.lang.NullPointerException: Cannot invoke "com.sun.javafx.font.FontResource.getDefaultAAMode()" because the return value of "com.sun.javafx.font.LogicalFont.getSlot0Resource()" is null
at javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getDefaultAAMode(LogicalFont.java:456)
at javafx.graphics@24-internal/com.sun.javafx.font.LogicalFont.getStrike(LogicalFont.java:461)
at javafx.graphics@24-internal/com.sun.javafx.font.PrismFont.getStrike(PrismFont.java:79)
at javafx.graphics@24-internal/com.sun.javafx.text.PrismTextLayout.setContent(PrismTextLayout.java:139)
at javafx.graphics@24-internal/javafx.scene.text.Text.getTextLayout(Text.java:303)
at javafx.graphics@24-internal/javafx.scene.text.Text.needsFullTextLayout(Text.java:258)
at javafx.graphics@24-internal/javafx.scene.text.Text$6.invalidated(Text.java:576)
...
because LogicalFont::getSlot0Resource will return null. Adding null checks after this point will require many changes (LogicalFont::getSlotResource(0) is used in many places). Shouldn't we fail gracefully after LogicalFont::getSlot0Resource if slot0FontResource is finally null?
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8339068: [Linux] NPE: Cannot read field "firstFont" because "<local4>" is null (Bug - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1546/head:pull/1546
$ git checkout pull/1546
Update a local copy of the PR:
$ git checkout pull/1546
$ git pull https://git.openjdk.org/jfx.git pull/1546/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1546
View PR using the GUI difftool:
$ git pr show -t 1546
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1546.diff
Webrev
:wave: Welcome back jpereda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@jperedadnr This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8339068: [Linux] NPE: Cannot read field "firstFont" because "<local4>" is null
Reviewed-by: prr
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 36 new commits pushed to the master branch:
- f681302926af291d64982f92f6caf1ead5dd266b: 8341532: [testbug] Mark QPathTest as unstable on Linux
- c0757a2ea5977c062b350335c09dd59671f88df5: 8273743: KeyCharacterCombination for "+" does not work on US QWERTY keyboard layout
- 6e4d41b61b7efaafdfd9ea3f93cab9b7acb48de9: 8341164: Update boot JDK to 23
- d5432c3e14b06445bc45e34e4aa63ec415c03595: 8183521: Unable to type characters with tilde with swiss german keyboard layout
- 4d3c3661d3d64bf06c3772e3b35b7e2b09fc3f99: 8340829: Generated API docs should clearly identify EA builds
- 01e9e7eadb21aabc801d4764ed5bd5e3de8d451b: 8332895: Support interpolation for backgrounds and borders
- 5428f267887ea4098f6c2a87335de7ed9bf5c656: 8340982: [win] Dead key followed by Space generates two characters instead of one
- 7870a226a21826e3979e314c0218d351b3cfb82f: 8297072: Provide gradle option to test a previously built SDK
- ecab6b6b0eab288eff1e13173a79a2fa4f4aca80: 8340980: Cannot build on Windows ARM
- 0dd0c794c3b08f816e7618026d5c90deaf952046: 8340954: Add SECURITY.md file
- ... and 26 more: https://git.openjdk.org/jfx/compare/b88ac0495650bd033ba11e3131e9bffc517872eb...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
Shouldn't we fail gracefully after LogicalFont::getSlot0Resource if slot0FontResource is finally null?
It calls PrismFontFactory::getDefaultFontResource -- this function looks practically everywhere to find some kind of font. I think if it fails, JavaFX should just not start up at all (unless using FX without fonts is a "supported" configuration). Having no font at all will no doubt lead to all kinds of problems later on, so whether supported or not, we'd need checks everywhere then to ensure code can handle having no fonts.
If you think that there still is a font, then perhaps PrismFontFactory::getDefaultFontResource should return that.
The changes seem to deal with the case where Font Config is unavailable on Linux. I think those changes are fine. However, if it turns out there isn't any font at all (via an alternative source) I think it would be best to just fail.
I don't understand why this change is needed.
Perhaps I'm misunderstanding, but if usefontconfig is false, that means "I have supplied embedded fonts instead". In which case the API calls where the null checks are added should not be returning null.
In other words, setting this to false, and not supplying fonts is a misconfiguration. Arguably we should disable this property and always look for fontconfig. But one would only know how to misconfigure it by reading the sources to discover this property.
On Linux, if you use -Dprism.useFontConfig=false, but then don't set valid embedded fonts, you get the reported NPE, instead of a different failure, with a more explicit message.
But since this property is undocumented, and you indeed need to read the sources to find out about it, you might also discover that setting a fontDir with some fonts and a property file with some properties to use those fonts, will solve the issue and there won't be a NPE anymore.
So I agree that setting this to false, and not supplying fonts is a misconfiguration, but should probably be handled better, with a more explicit message or fast failure.
On Android, however, the situation is different: fontpath_linux.c(with the native implementation of FontConfigManager::getFontConfig) is not used, but FTFactory::getFallbacks calls it nonetheless.
Using -Dprism.useFontConfig=false prevents the runtime issue (UnsatisfiedLinkError), but then since FontConfigManager.getFontConfigFont returns null (note there is an AndroidFontFinder class that should manage fonts on Android), then the NPE can't be avoided, unless with add this null check.
I agree that a null check hides the underlying issue, so prabably we should use isLinux instead. PrismFontFactory has this gate before calling FontConfigManager.
So far, the documentation in https://wiki.openjdk.org/display/OpenJFX/Font+Setup is still somehow valid in this situation (no fontConfig and providing embedded fonts).
So I can revert the change in LogicalFont, and modify the message in FontConfigManager::initFontConfigLogFonts:
System.err.println("Error: JavaFX detected no fonts! " +
"Please refer to release notes for proper font configuration");
to something more meaningful, like?
System.err.println("Error: JavaFX detected no fonts!\n " +
"For proper font configuration, please check https://wiki.openjdk.org/display/OpenJFX/Font+Setup");
The NPE will be still present though.
Thoughts?
@jperedadnr This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@prrace could you have another look at this PR, after my change and comments?
Sorry, I think I was out the week you updated it and then it disappeared off my radar. Seems like a reasonable compromise
Going to push as commit 23e25954f2cbd8dda9afea7f257d22156233894e.
Since your change was applied there have been 36 commits pushed to the master branch:
- f681302926af291d64982f92f6caf1ead5dd266b: 8341532: [testbug] Mark QPathTest as unstable on Linux
- c0757a2ea5977c062b350335c09dd59671f88df5: 8273743: KeyCharacterCombination for "+" does not work on US QWERTY keyboard layout
- 6e4d41b61b7efaafdfd9ea3f93cab9b7acb48de9: 8341164: Update boot JDK to 23
- d5432c3e14b06445bc45e34e4aa63ec415c03595: 8183521: Unable to type characters with tilde with swiss german keyboard layout
- 4d3c3661d3d64bf06c3772e3b35b7e2b09fc3f99: 8340829: Generated API docs should clearly identify EA builds
- 01e9e7eadb21aabc801d4764ed5bd5e3de8d451b: 8332895: Support interpolation for backgrounds and borders
- 5428f267887ea4098f6c2a87335de7ed9bf5c656: 8340982: [win] Dead key followed by Space generates two characters instead of one
- 7870a226a21826e3979e314c0218d351b3cfb82f: 8297072: Provide gradle option to test a previously built SDK
- ecab6b6b0eab288eff1e13173a79a2fa4f4aca80: 8340980: Cannot build on Windows ARM
- 0dd0c794c3b08f816e7618026d5c90deaf952046: 8340954: Add SECURITY.md file
- ... and 26 more: https://git.openjdk.org/jfx/compare/b88ac0495650bd033ba11e3131e9bffc517872eb...master
Your commit was automatically rebased without conflicts.
@jperedadnr Pushed as commit 23e25954f2cbd8dda9afea7f257d22156233894e.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.