On Mon, 15 Jul 2024 19:11:38 GMT, Phil Race <[email protected]> wrote:
>> Karm Michal Babacek has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Treats missing class as a fatal error > > 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 `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. > src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c line 65: > >> 63: >> "java/awt/GraphicsEnvironment"); >> 64: if (graphicsEnvClass == NULL) { >> 65: // Not finding the class is not necessarily an error. > > I can't see how that comment can possibly be true. This change makes no sense. See [comment](https://github.com/openjdk/jdk/pull/20169#issuecomment-2247823182), please. > src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c line 83: > >> 81: headlessFn); >> 82: // If an exception occurred, we assume headless mode. >> 83: if ((*env)->ExceptionCheck(env)) { > > I don't like this at all. See [comment](https://github.com/openjdk/jdk/pull/20169#issuecomment-2247823182), please. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20169#issuecomment-2247823182 PR Review Comment: https://git.openjdk.org/jdk/pull/20169#discussion_r1689731774 PR Review Comment: https://git.openjdk.org/jdk/pull/20169#discussion_r1689731419
