On Wed, 23 Oct 2024 19:54:17 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c line 70: >> >>> 68: graphicsEnvClass = (*env)->FindClass(env, >>> 69: >>> "java/awt/GraphicsEnvironment"); >>> 70: CHECK_EXCEPTION_FATAL(env, "java/awt/GraphicsEnvironment class >>> not found"); >> >> Taking out any discussion about graal/etc as well as absent of any classes >> and methods, I agree that the current code is questionable. >> >> But the solution to always print "java/awt/GraphicsEnvironment class not >> found" does not seems to be correct as well. >> >> One of the problem with this code is an actual conflict between two patches. >> Both are related to warning caused by pending java exceptions, but >> implemented differently: >> - https://bugs.openjdk.org/browse/JDK-8031001: treated the java exceptions >> as a fatal errors and fail fast >> - https://bugs.openjdk.org/browse/JDK-8130507: ignores the java exceptions >> and tried to fallback into the headless mode in assumption that it may be >> fine for the application >> >> Both of that patches only changes the code paths which were reported by >> "some tools"/checkjni - this is the reason why the next code was not >> reported and was not patched(which is wrong): >> >> >> graphicsEnvClass = (*env)->FindClass(env, >> "java/awt/GraphicsEnvironment"); >> if (graphicsEnvClass == NULL) { >> return JNI_TRUE; >> } >> headlessFn = (*env)->GetStaticMethodID(env, >> graphicsEnvClass, >> "isHeadless", "()Z"); >> if (headlessFn == NULL) { >> return JNI_TRUE; >> } >> >> If we would like to follow JDK-8031001 approach we should use fatal error >> here and fail fast, for example if OOM is occurred. >> If we would like to follow JDK-8130507 we should clear an exceptions and try >> to use headless mode. >> >> I guess ignoring an exceptions is not that good and the fix for JDK-8130507 >> should be rethinking. > > You can change the CHECK_EXCEPTION_FATAL macro to something like > > #define CHECK_EXCEPTION_FATAL(env, message) \ > if ((*env)->ExceptionCheck(env)) { \ > (*env)->ExceptionDescribe(env); > (*env)->FatalError(env, message); \ > } > > In this case the root cause of the bug will always be printed. > And then update the fatal message to some generic text. > > Note that the FatalError is used in this code since we always should load the > library(libawt_xawt or libawt_headless) or fail fast, otherwise we most > probably will get an error later. @mrserb Perhaps, `ExceptionDescribe` isn't part of the `CHECK_EXCEPTION_FATAL` macro to avoid leaking sensitive information. Therefore, `ExceptionClear` which has been used previously is safer. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20169#discussion_r1922864473
