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

[Ambarish Rapte]

On Tue, 18 Apr 2023 02:08:03 GMT, Alexander Matveev <almatvee 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.

Looks good overall, suggesting few minor changes.

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.

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`.

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)`

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.

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`

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

Changes requested by arapte (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1094#pullrequestreview-1418209160
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188351969
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188473239
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188478660
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188581142
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1190643308