RFR: 8336382: Fixes error reporting in loading AWT and fonts [v3]
Karm Michal Babacek
duke at openjdk.org
Wed Jul 24 12:48:34 UTC 2024
On Mon, 15 Jul 2024 19:11:38 GMT, Phil Race <prr at openjdk.org> 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
More information about the client-libs-dev
mailing list