On Tue, 27 Aug 2024 15:50:54 GMT, Jose Pereda <jper...@openjdk.org> wrote:
> 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? 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`. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1546#issuecomment-2320826055
