On Sat, 2 Dec 2023 00:58:28 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp line
>> 68:
>>
>>> 66: jobject PlatformSupport::collectPreferences() const {
>>> 67: jobject prefs = env->NewObject(jHashMapCls, jHashMapInit);
>>> 68: if (EXCEPTION_OCCURED(env)) return NULL;
>>
>> You should also check for `prefs == NULL` (or `!prefs` as you prefer);
>
> Is that necessary? The JNI documentation for `NewObject` states: "Returns a
> Java object, or NULL if the object cannot be constructed."
> Is there a non-exceptional way to _not_ construct an instance of a class?
I double-checked this with a few experts in this area. The [JNI
NewStringUTF](https://docs.oracle.com/en/java/javase/21/docs/specs/jni/functions.html#newstringutf)
spec states that it returns a Java string object, or `NULL` if the string
cannot be constructed, and throws an exception if the system runs out of
memory. Also, the [Java Exceptions
section](https://docs.oracle.com/en/java/javase/21/docs/specs/jni/design.html#java-exceptions)
in the JNI design overview says that you can check for an error return value
and be assured that a non-error value means that no exceptions have been thrown.
So the above means that if an exception is thrown, the ojbect will definitely
be NULL. By itself, that doesn't necessarily guarantee the converse, but I
don't know of any cases (and no one I asked did, either) where it would be
possible for the `NewString*`, `NewObject*`, or `New<PrimitiveType>Array`
functions to return null without throwing an exception.
Having said that, most of our code that checks the return value checks for
`NULL`, so I think checking for NULL would be a better pattern, but as long as
you check for one or the other, I won't insist on it.
Since this now isn't a case of missing error checking, but rather a code
consistency issue (and we have other places where we do exactly what you
propose to do), we might file a cleanup task to look into making our checks
more consistent, but that would be after we take care of the functional
problems we have around missing checks.
>> modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp line
>> 70:
>>
>>> 68: if (EXCEPTION_OCCURED(env)) return NULL;
>>> 69:
>>> 70: GtkStyle* style = gtk_style_new();
>>
>> Can `gtk_style_new` return NULL?
>
> It's not specified to do so. Internally, it will call `g_object_new`, which
> is also not specified to return null. I've found some evidence on some
> gnome.org blog that it will not ever return null, but I haven't looked
> further than that.
I suspect that any object allocation method, such as `g_object_new` could
return null if memory is exhausted, so a NULL check seems like a good idea.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1414548560
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1414551756
