RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]

[Phil Race]

On Fri, 29 Jan 2021 16:23:20 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> Phil Race has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8260616: Removing remaining JNF dependencies in the java.desktop modul
>
> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 601:
> 
>> 599:         jchar unichars[len];
>> 600:         (*env)->GetStringRegion(env, str, 0, len, unichars);
>> 601:         CHECK_EXCEPTION();
> 
> Are `JNF_CHECK_AND_RETHROW_EXCEPTION` and `CHECK_EXCEPTION` equivalent?
> 
> `JNF_CHECK_AND_RETHROW_EXCEPTION` seems to throw exception, but `CHECK_EXCEPTION` does not?

JNF_CHECK_AND_RETHROW_EXCEPTION is this


// JNF_CHECK_AND_RETHROW_EXCEPTION - rethrows exceptions from Java
//
// Takes an exception thrown from Java, and transforms it into an
// NSException. The NSException should bubble up to the upper-most
// JNF_COCOA_ENTER/JNF_COCOA_EXIT pair, and then be re-thrown as
// a Java exception when returning from JNI. This check should be
// done after raw JNI operations which could cause a Java exception
// to be be thrown. The JNF{Get/Set/Call}  macros below do this
// check automatically.
#define JNF_CHECK_AND_RETHROW_EXCEPTION(env)                                                    \
{                                                                                                                                       \
    jthrowable _exception = (*env)->ExceptionOccurred(env);                     \
    if (_exception) [JNFException raise:env throwable:_exception];      \
}

So what it actually does is clear the exception, raise an NSException and when
the code reaches JNF_COCOA_EXIT - which then has to be there - it clears the NSException
and re-throws the Java exception.

So the name of the macro is misleading since this does NOT itself rethrow the exception,
it relies on other code to do that.

CHECK_EXCEPTION will amount to the same - it throws an NSException if there is a Java exception pending.
And it will clear the NSException before exiting back to Java.
The difference is that it doesn't clear the Java Exception and later rethrow it since there is no need
There is one exception to this in  CHECK_EXCEPTION - if this is the main (ie appkit) thread the java exception is cleared since there's no one to return it to ...

> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 608:
> 
>> 606:     {
>> 607:         // Get string to draw and the length
>> 608:         const jchar *unichars = JNFGetStringUTF16UniChars(env, str);
> 
> According to `JNFString.h`, the `JNFGetStringUTF16UniChars()` API:
> 
> /*
>  * Gets UTF16 unichars from a Java string, and checks for errors and raises a JNFException if
>  * the unichars cannot be obtained from Java.
>  */
> 
> raises a (JNFException) if it can't get the chars, but now we simply return if `GetStringChars()` can not return the chars.
> 
> Seems like we handle this case differently in the new code now - is this change desired and correct?

You are correct, but I decided that here it is fine.
If GetStringChars fails it will return NULL (it does not raise any excptions itself).
I added a check for NULL if NULL we then return from the JNI method to Java.
In the old case it also would return but would additionally propagate that raised exception to Java which JNF
decided to throw into the mix when there's already a standard way of testing failure.
And since we already know str != NULL we are in the realms of something went badly wrong inside the VM for
this to fail. So I decided this was all fine.

> src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 83:
> 
>> 81:                   stringWithFileSystemRepresentation:chs length:len];
>> 82:     return result;
>> 83: }
> 
> Why are we doing:
> 
> `java_string -> chars -> NSString -> chars -> [NSFileManager stringWithFileSystemRepresentation]`
> 
> ?
> 
> Why not just get the raw characters form JNI and feed them directly into `[NSFileManager  stringWithFileSystemRepresentation]`, ie:
> 
> `java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]`
> 
> and skip the `JavaStringToNSString` step altogether?

I tried to explain the odd-ness of this in the comments preceding the function definition.
Near as I could figure out that intermediate call is needed to do the decomposition since the NSFileManager won't do that.
But this is not exactly my area of expertise and there may be a better way. Who is an expert on the intricacies of the macOS file system naming ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2305