RFR: 8304290: Some JNI calls made without checking exceptions in media

[Alexander Matveev]

On Tue, 9 May 2023 09:05:57 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> - Added missing exception checks for JNI calls.
>>  - Improved JNI error checking by checking for both exception and return value.
>>  - Minor code clean up.
>
> modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp line 66:
> 
>> 64:         mid_toString = env->GetMethodID(klass, "getStringLocation", "()Ljava/lang/String;");
>> 65:         env->DeleteLocalRef(klass);
>> 66:         if (javaEnv.clearException() || mid_toString == NULL)
> 
> The variable name can be changed. It holds id to `Locator.getStringLocation()`, so the variable name does not reflect it correctly. As the PR is touching this code, it seems ok to change.

Fixed.

> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp line 68:
> 
>> 66:         createConnectionHolder = env->GetMethodID(klass, "createConnectionHolder", "()Lcom/sun/media/jfxmedia/locator/ConnectionHolder;");
>> 67:         env->DeleteLocalRef(klass);
>> 68:         if (javaEnv.reportException() || createConnectionHolder == 0)
> 
> I would recommend to add as  `(createConnectionHolder == NULL)`, similar to the below changes.
> And may be change previous instance and declaration of `createConnectionHolder`.

Fixed.

> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp line 73:
> 
>> 71: 
>> 72:     jobject connectionHolder = env->CallObjectMethod(jLocator, createConnectionHolder);
>> 73:     if (javaEnv.reportException() || NULL == connectionHolder)
> 
> May be enclose in parenthesis, like the below changes :  `(NULL == connectionHolder)`

Fixed.

> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp line 244:
> 
>> 242:         }
>> 243: 
>> 244:         javaEnv.reportException();
> 
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp : 240
> 
> The `reportException()` method clears the exception and **prints** the exception message. So this change would result in behavior change. Is this change safe ?
> Similar change of removing `reportException()` can be done in `NeedBuffer()` method also.

Yes, it should be safe. I do not see any need to print exception, since it is unlikely will happen. Also, these methods called many times to read data and `clearException()` is more light weight. I updated `NeedBuffer()` to use `clearException()`.

> modules/javafx.media/src/main/native/jfxmedia/jni/JavaMediaWarningListener.cpp line 49:
> 
>> 47:         jclass mediaUtilsClass = pEnv->FindClass("com/sun/media/jfxmediaimpl/MediaUtils");
>> 48:         if (!javaEnv.clearException() && mediaUtilsClass != NULL) {
>> 49:             jmethodID errorMethodID = pEnv->GetStaticMethodID(mediaUtilsClass,
> 
> `errorMethodID`  ->  `nativeWarningMethodID`

Fixed.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212352448
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212354698
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212354937
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212365506
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212367015