RFR: 8301302: Platform preferences API [v34]

[Kevin Rushforth]

On Sat, 2 Dec 2023 00:58:28 GMT, Michael Strauß <mstrauss at 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