8336382: Fix error reporting in loading AWT #20169
Conversation
If there is problem with finding and calling e.g. java/awt/GraphicsEnvironment
in AWTIsHeadless, the env' Exception remains set and it 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 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 with some clear message that indicates 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, e.g.
oracle/graal#9138
oracle/graal#8475
oracle/graal#9300
quarkusio/quarkus#31596
graalvm/mandrel#292
Karm/mandrel-integration-tests#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 java.desktop@22.0.1/java.awt.Font.createFont0(Font.java:1205)
at java.desktop@22.0.1/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)
|
👋 Welcome back Karm! A progress list of the required criteria for merging this PR into |
|
@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: 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
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 |
Karm
changed the title
Webrevs
|
avu
left a comment
avu
left a comment
There was a problem hiding this comment.
Overall LGTM, please update copyright years in the changed files.
prrace
left a comment
prrace
left a comment
There was a problem hiding this comment.
There is nothing in this PR that I would accept. It should be withdrawn.
|
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 If you take a look at the flow of
Previous fixSo I made this PR, thinking that for some headless JDK distributions it is O.K. to be entirely missing @@ -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 fixThe new fix I propose treats the missing class or missing method as fatal errors. The crash makes sense, the error message is correct: 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 |
I think we're all finding it a bit hard to understand what to do. Is the problem that
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. |
|
ping @prrace |
|
@Karm There are a few things that have not yet been addressed:
I'd also merge in latest master since x86 tests have since been disabled. That should fix the GHA issue. Thanks! |
… java.homenot set and AWT cannot try to lookup fonts
|
Hello @jerboaa, The year there is 2024 for both edited files.
Use the title from this PR, please.
Done. |
Font.java and information leakageWould 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? |
I retract this. It's not that inconvenient and I don't want to complicate this PR. |
Done. |
|
@prrace I added a test that triggers the described issue. Unpatched awt_LoadLibrary.cPatched awt_LoadLibrary.c |
prrace
left a comment
prrace
left a comment
There was a problem hiding this comment.
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.
|
|
|
|
||
| #define CHECK_EXCEPTION_FATAL(env, message) \ | ||
| if ((*env)->ExceptionCheck(env)) { \ | ||
| (*env)->ExceptionDescribe(env); \ |
There was a problem hiding this comment.
Is it possible that ExceptionDescribe could leak something important?
Should we preserve ExceptionClear instead?
| if (graphicsEnvClass == NULL) { | ||
| return JNI_TRUE; | ||
| } | ||
| CHECK_EXCEPTION_FATAL(env, "FindClass java/awt/GraphicsEnvironment failed"); |
There was a problem hiding this comment.
Sorry, I didn't make myself clear.
Since there's no Java code to catch the exception, there's no other option but to call (*env)->FatalError via CHECK_EXCEPTION_FATAL.
|
@Karm I've updated the subject of the JBS issue; please update the PR subject. |
Karm
changed the title
Thank you for the review. This is indeed an awkward, yet real-world exceptional case. |
Thank you @aivanov-jdk . Done. |
|
/integrate |
|
/sponsor |
|
Going to push as commit 1858dc1.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
The new test will not compile. The ASM library has been removed from OpenJDK, except for use by some hotspot tests. Everything else should be converted to use the Classfile API. See JDK-8349099. |
|
@dholmes-ora Let me take a look presently... |
|
See https://bugs.openjdk.org/browse/JDK-8349099 @Karm I was wondering why this did not show up on GHA (it's probably not executed there). But I noticed when researching this that your last merge from mainline was in September. Even if there are no "physical" merge conflicts that git can detect, there are likely to be "logical" conflicts like this, where a library was removed that your test depended on. So for a long running PR like this, when it is ready to be checked in, you need to merge in main and verify that the fix (and the tests) still works, and nothing else breaks. If there are no "physical" conflicts you do not need to push the merge to the PR on github, since that might invalidate approvals, but you need to test it locally. As for how long "long running" is, I don't know. A week without merge is likely okay, 3 months is not. |
I thought I'd run the client tests with changes from this PR, and I usually merge master into my local branch before submitting a job. Yet I can't find the job, I might not have run the tests. |
|
Thanks for the comments. I improved my flow; e.g. #23852 (comment) |
9 participants
If there is a problem with finding and calling e.g.
java/awt/GraphicsEnvironmentinAWTIsHeadless, the env' Exception remains set and it is not cleared. Later, that manifests as: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_NewStringPlatformcalls failed, e.g.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:
Adding the cause there makes it clearer, i.e. that JAVA_HOME might be missing:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20169/head:pull/20169$ git checkout pull/20169Update a local copy of the PR:
$ git checkout pull/20169$ git pull https://git.openjdk.org/jdk.git pull/20169/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20169View PR using the GUI difftool:
$ git pr show -t 20169Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20169.diff
Using Webrev
Link to Webrev Comment