openjdk
8336382: Fixes error reporting in loading AWT and fonts
If there is a problem with finding and calling e.g. java/awt/GraphicsEnvironment in AWTIsHeadless, the env' Exception remains set and it is not cleared. Later, that manifests as:
Fatal error reported via JNI: Could not allocate library name
Which is misleading. The code path is perhaps rare in a normal JDK usage, but it has been complicating our users' bug reports in the GraalVM/native-image ecosystem for quite some time.
Instead of failing later indicating that the user has incorrectly configured JNI, it bails out very soon with a message that seems as if a jstring could not have been allocated. It sends users on wild goose chases where it appears JNU_NewStringPlatform calls failed, e.g.
- https://github.com/oracle/graal/issues/9138
- https://github.com/oracle/graal/issues/8475
- https://github.com/oracle/graal/issues/9300
- https://github.com/quarkusio/quarkus/issues/31596
- https://github.com/graalvm/mandrel/issues/292
- https://github.com/Karm/mandrel-integration-tests/issues/262
This commit fixes the error reporting in the AWTIsHeadless.
Furthermore, when AOT compiled, there is little sense for having a JAVA_HOME, yet some parts of AWT code look for it to search fonts. In such case, an empty directory structure is enough to accommodate it, e.g.
/tmp/JAVA_HOME/ /tmp/JAVA_HOME/conf /tmp/JAVA_HOME/conf/fonts /tmp/JAVA_HOME/lib
The exception is somewhat cryptic for users again, merely stating:
Exception in thread "main" java.io.IOException: Problem reading font data.
at [email protected]/java.awt.Font.createFont0(Font.java:1205)
at [email protected]/java.awt.Font.createFont(Font.java:1076)
at imageio.Main.loadFonts(Main.java:139
Adding the cause there makes it clearer, i.e. that JAVA_HOME might be missing:
Exception in thread "main" java.io.IOException: Problem reading font data.
at java.desktop@23-internal/java.awt.Font.createFont0(Font.java:1206)
at java.desktop@23-internal/java.awt.Font.createFont(Font.java:1076)
at imageio.Main.loadFonts(Main.java:139)
at imageio.Main.paintRectangles(Main.java:97)
at imageio.Main.main(Main.java:195)
at java.base@23-internal/java.lang.invoke.LambdaForm$DMH/sa346b79c.invokeStaticInit(LambdaForm$DMH)
Caused by: java.lang.Error: java.home property not set
at java.desktop@23-internal/sun.awt.FontConfiguration.findFontConfigFile(FontConfiguration.java:180)
at java.desktop@23-internal/sun.awt.FontConfiguration.<init>(FontConfiguration.java:97)
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-8336382: Fixes error reporting in loading AWT and fonts (Bug - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20169/head:pull/20169
$ git checkout pull/20169
Update a local copy of the PR:
$ git checkout pull/20169
$ git pull https://git.openjdk.org/jdk.git pull/20169/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20169
View PR using the GUI difftool:
$ git pr show -t 20169
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20169.diff
Webrev
:wave: Welcome back Karm! 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}
@Karm 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:
8336382: Fix error reporting in loading AWT
Reviewed-by: prr, aivanov, serb
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 1784 new commits pushed to the master branch:
- 22069ff42b7e5c3058415ef9b6e0b50b9d2c16ef: 8348975: Broken links in the JDK 24 JavaDoc API documentation, build 33
- f81772a49ebfa197bac7bf05cf7d468d819f742a: 8348647: CDS dumping commits 3GB when large pages are used
- fb0f2d25d218e64a86995478fd3ea10d619834d5: 8300708: Some nsk jvmti tests fail with virtual thread wrapper due to jvmti missing some virtual thread support
- bb528d5ad6f28e9c1bc4e283d40c70489465bd00: 8348567: [ASAN] Memory access partially overflows by NativeCallStack
- e0c2cb48b3c51f21596d76fe6b94bbe408196e22: 8348663: [AIX] clang pollutes the burned-in library search paths of the generated executables
- 14136f8b1106137317393bc2ab0a2db0d212f8d8: 8347564: ZGC: Crash in DependencyContext::clean_unloading_dependents
- 04c24f18d554a57c537f3a77c5f6d0d03e7fc3d1: 8347779: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with Unable to deduce type of thread from address
- d985b31cbb5646c526e1a68a7547f26f56d37607: 8342096: Popup menus that request focus are not shown on Linux with Wayland
- cbe9ec530fc248be74766ff6ff32761cd415a6f0: 8348905: Add support to specify the JDK for compiling Jtreg tests
- 6b581d22e13599b16b38aff1ca5a795c6a910d30: 8347997: assert(false) failed: EA: missing memory path
- ... and 1774 more: https://git.openjdk.org/jdk/compare/73ebb848fdb66861e912ea747c039ddd1f7a5f48...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.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@avu, @prrace, @aivanov-jdk, @mrserb) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@Karm The following label will be automatically applied to this pull request:
-
client
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
Webrevs
- 09: Full - Incremental (91d554f4)
- 08: Full - Incremental (bba9a3ca)
- 07: Full - Incremental (c206f936)
- 06: Full - Incremental (aa47484d)
- 05: Full - Incremental (2b3de582)
- 04: Full - Incremental (0cd0ab42)
- 03: Full - Incremental (8b203e7c)
- 02: Full - Incremental (4e33725a)
- 01: Full - Incremental (49c6ebdd)
- 00: Full (9d0c671e)
Hello @prrace
TBH, I got somewhat bewildered by your comments on how this PR makes no sense. Then I realized I might have been erroneously treating the AWTIsHeadless function's contents as correct, merely building on the previous error.
If you take a look at the flow of awt_LoadLibrary.c, my problem with it is that errors originating in AWTIsHeadless are reported way later in AWT_OnLoad and that is wrong.
awt_LoadLibrary.c:
Previous fix
So I made this PR, thinking that for some headless JDK distributions it is O.K. to be entirely missing java/awt/GraphicsEnvironment class or isHeadless method.
@@ -62,15 +62,24 @@ JNIEXPORT jboolean JNICALL AWTIsHeadless() {
graphicsEnvClass = (*env)->FindClass(env,
"java/awt/GraphicsEnvironment");
if (graphicsEnvClass == NULL) {
+ // Not finding the class is not necessarily an error.
+ if ((*env)->ExceptionCheck(env)) {
+ (*env)->ExceptionClear(env);
+ }
return JNI_TRUE;
}
headlessFn = (*env)->GetStaticMethodID(env,
graphicsEnvClass, "isHeadless", "()Z");
if (headlessFn == NULL) {
+ // If we can't find the method, we assume headless mode.
+ if ((*env)->ExceptionCheck(env)) {
+ (*env)->ExceptionClear(env);
+ }
return JNI_TRUE;
}
isHeadless = (*env)->CallStaticBooleanMethod(env, graphicsEnvClass,
headlessFn);
+ // If an exception occurred, we assume headless mode.
if ((*env)->ExceptionCheck(env)) {
(*env)->ExceptionClear(env);
return JNI_TRUE;
New fix
The new fix I propose treats the missing class or missing method as fatal errors. The crash makes sense, the error message is correct:
$ ./target/imageio -Djava.awt.headless=true
Fatal error reported via JNI: java/awt/GraphicsEnvironment class not found
Printing instructions (ip=0x000000000048f8d9):
...
It's a little more invasive though:
diff --git a/src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c b/src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c
index 0fc44bfca71..7ef6dab8682 100644
--- a/src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c
+++ b/src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c
@@ -43,6 +43,12 @@
#define VERBOSE_AWT_DEBUG
#endif
+#define CHECK_EXCEPTION_FATAL(env, message) \
+ if ((*env)->ExceptionCheck(env)) { \
+ (*env)->ExceptionClear(env); \
+ (*env)->FatalError(env, message); \
+ }
+
static void *awtHandle = NULL;
typedef jint JNICALL JNI_OnLoad_type(JavaVM *vm, void *reserved);
@@ -61,25 +67,13 @@ JNIEXPORT jboolean JNICALL AWTIsHeadless() {
env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2);
graphicsEnvClass = (*env)->FindClass(env,
"java/awt/GraphicsEnvironment");
- if (graphicsEnvClass == NULL) {
- // Not finding the class is not necessarily an error.
- if ((*env)->ExceptionCheck(env)) {
- (*env)->ExceptionClear(env);
- }
- return JNI_TRUE;
- }
+ CHECK_EXCEPTION_FATAL(env, "java/awt/GraphicsEnvironment class not found");
headlessFn = (*env)->GetStaticMethodID(env,
graphicsEnvClass, "isHeadless", "()Z");
- if (headlessFn == NULL) {
- // If we can't find the method, we assume headless mode.
- if ((*env)->ExceptionCheck(env)) {
- (*env)->ExceptionClear(env);
- }
- return JNI_TRUE;
- }
+ CHECK_EXCEPTION_FATAL(env, "isHeadless method not found");
isHeadless = (*env)->CallStaticBooleanMethod(env, graphicsEnvClass,
headlessFn);
- // If an exception occurred, we assume headless mode.
+ // If an exception occurred, we assume headless mode and carry on.
if ((*env)->ExceptionCheck(env)) {
(*env)->ExceptionClear(env);
return JNI_TRUE;
@@ -88,12 +82,6 @@ JNIEXPORT jboolean JNICALL AWTIsHeadless() {
return isHeadless;
}
-#define CHECK_EXCEPTION_FATAL(env, message) \
- if ((*env)->ExceptionCheck(env)) { \
- (*env)->ExceptionClear(env); \
- (*env)->FatalError(env, message); \
- }
-
/*
* Pathnames to the various awt toolkits
*/
Thanks for your time. I hope the PR makes more sense now or at least that I managed to get my point across.
If you think there is a better way to handle it, tell me and I'll do it. I don't dwell on a particular implementation, I merely want the misleading error gone.
Thx K.
There is nothing in this PR that I would accept. It should be withdrawn.
I think we're all finding it a bit hard to understand what to do.
Is the problem that
- This is not a bug?
- This is not a bug that should be fixed?
- The fix is wrong?
- There is a much better way to fix it?
- This is an enhancement, but it is not a worthwhile one?
Please, help us here.
P.S. There surely is an actual problem here: the error message that is currently output misrepresents the actual cause of the failure.
@Karm There are a few things that have not yet been addressed:
- @avu mentioned here that copyright years of the file need updating
- There is a title mismatch between this PR and the JBS entry. Which one should it be if any? (and I'll help you fix that one).
I'd also merge in latest master since x86 tests have since been disabled. That should fix the GHA issue.
Thanks!
Hello @jerboaa,
- @avu mentioned here that copyright years of the file need updating
The year there is 2024 for both edited files.
- There is a title mismatch between this PR and the JBS entry. Which one should it be if any? (and I'll help you fix that one).
Use the title from this PR, please.
I'd also merge in latest master since x86 tests have since been disabled. That should fix the GHA issue.
Done.
Font.java and information leakage
@prrace @jerboaa
Would this solution address the concern? Instead of passing on the whole throwable, we just inspect it and if it is this particular one, we pass on the information to the user?
+++ b/src/java.desktop/share/classes/java/awt/Font.java
@@ -971,6 +971,9 @@ public static Font[] createFonts(InputStream fontStream)
}
return createFont0(fontFormat, fontStream, true, tracker);
} catch (InterruptedException e) {
+ if (e.getCause().getMessage().contains("java.home property not set")) {
+ throw new IOException("Problem reading font data. java.home property not set.");
+ }
throw new IOException("Problem reading font data.");
Font.java and information leakage
@prrace @jerboaa
Would this solution address the concern? Instead of passing on the whole throwable, we just inspect it and if it is this particular one, we pass on the information to the user?
+++ b/src/java.desktop/share/classes/java/awt/Font.java @@ -971,6 +971,9 @@ public static Font[] createFonts(InputStream fontStream) } return createFont0(fontFormat, fontStream, true, tracker); } catch (InterruptedException e) { + if (e.getCause().getMessage().contains("java.home property not set")) { + throw new IOException("Problem reading font data. java.home property not set."); + } throw new IOException("Problem reading font data.");
I retract this. It's not that inconvenient and I don't want to complicate this PR.
- There is a title mismatch between this PR and the JBS entry. Which one should it be if any? (and I'll help you fix that one).
Use the title from this PR, please.
Done.
@prrace I added a test that triggers the described issue.
Unpatched awt_LoadLibrary.c
$ TIME=`date +%s`;mkdir -p test.${TIME}/jdk/JTwork test.${TIME}/jdk/JTreport; jtreg -a -ignore:quiet -w:test.${TIME}/jdk/JTwork -r:test.${TIME}/jdk/JTr
eport -jdk:/home/karm/workspaceRH/jdk/build/linux-x86_64-server-release/images/graal-builder-jdk/ /home/karm/workspaceRH/jdk/test/jdk/java/awt/Headless/;
Test results: passed: 14; failed: 1
...
Output and diagnostic info for process 578718 was saved into 'pid-578718-output.log'
----------System.err:(38/2940)----------
stdout: [Transforming java.awt.GraphicsEnvironment.
isHeadless removed from java.awt.GraphicsEnvironment.
FATAL ERROR in native method: Could not allocate library name
at jdk.internal.loader.NativeLibraries.load(java.base@24-internal/Native Method)
at jdk.internal.loader.NativeLibraries$NativeLibraryImpl.open(java.base@24-internal/NativeLibraries.java:331)
...
test result: Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: 'FATAL ERROR in native method: isHeadless method not found' missing from stdout/stderr
Patched awt_LoadLibrary.c
$ TIME=`date +%s`;mkdir -p test.${TIME}/jdk/JTwork test.${TIME}/jdk/JTreport; jtreg -a -ignore:quiet -w:test.${TIME}/jdk/JTwork -r:test.${TIME}/jdk/JTreport -jdk:/home/karm/workspaceRH/jdk/build/linux-x86_64-server-release/images/graal-builder-jdk/ /home/karm/workspaceRH/jdk/test/jdk/java/awt/Headless/;
Test results: passed: 15
@Karm 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!
CI
==> make
GNU "make" has been installed as "gmake".
If you need to use it as "make", you can add a "gnubin" directory
to your PATH from your bashrc like:
PATH="/opt/homebrew/opt/make/libexec/gnubin:$PATH"
xcode-select: error: invalid developer directory '/Applications/Xcode_14.3.1.app/Contents/Developer'
Error: Process completed with exit code 1.
IMHO an env issue.
I think this looks like a prudent solution. I am not part of the client team though, so don't count this as a formal review.
:warning: @Karm the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:
$ git checkout awt-load-messages-fonts
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push
So consider it a truly exceptional case.
FindClass("java/awt/GraphicsEnvironment") can fail with OutOfMemoryError, so even in this fix it is not an exceptional case. We still assume that standard classes should always be present in the bundle.
@Karm I've updated the subject of the JBS issue; please update the PR subject.
So this is much better than what was there in the beginning. FindClass failing - for a key API class - is probably a fatal error one way or another. Although in all (?) other cases, we try to return to Java and let the VM throw a Java-level error. Note that OnLoad is not the only code path that calls AWTIsHeadless, and there will be Java code to return to in the other case. However I think that the OnLoad will always be reached first, so maybe it is OK to do the current fix. Either way, things aren't going to work.
Generally the calls to check and clear exceptions when looking up code are there to keep 'tools' happy. If these classes aren't found something is badly wrong. meaning it should NEVER EVER happen. Failing to find java/lang/String for example hardly seems like something it is worth trying to recover from .. In your case it seems like it does some times happen for GraphicsEnvironment which still worries me. I don't want to make this fix a precedent for assuming that it is normal for classes to be missing. So consider it a truly exceptional case.
Thank you for the review. This is indeed an awkward, yet real-world exceptional case.
@Karm I've updated the subject of the JBS issue; please update the PR subject.
Thank you @aivanov-jdk . Done.
@Karm Your change (at version 91d554f4919a9923a304ec1d3b23b96cb5cf144a) is now ready to be sponsored by a Committer.