RFR: 8336382: Fixes error reporting in loading AWT and fonts [v9]
Karm Michal Babacek
duke at openjdk.org
Thu Oct 24 08:13:18 UTC 2024
On Wed, 23 Oct 2024 19:54:17 GMT, Sergey Bylokhov <serb at 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.
Thank you @mrserb for your time. Let me amend the PR and run tests again.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20169#discussion_r1814491540
More information about the client-libs-dev
mailing list