On Mon, 24 Apr 2023 05:53:34 GMT, Michael Strauß <mstra...@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/jni/Logger.cpp line 130: > >> 128: // Get global reference >> 129: m_cls = (jclass)pEnv->NewWeakGlobalRef(local_cls); >> 130: pEnv->DeleteLocalRef(local_cls); > > What is this code attempting to do? After `local_cls` is deleted, `m_cls` > refers to a potentially freed object and is thus not safe to use. In order to > do anything meaningful with `m_cls`, a strong reference needs to be acquired > with `NewLocalRef(m_cls)`. I think it should be fine to use `NewWeakGlobalRef`. `NewLocalRef` will make reference which is valid only for duration of `CLogger::init` and we using `m_cls` in `CLogger::logMsg` as well. I think you mean `NewGlobalRef`, but in this case there is no way to call `DeleteGlobalRef`, since CLogger instance is static global variable which gets deleted only when native media library gets unloaded and by this time JVM will be shutting down. Java Logger class referenced by m_cls should not be freed, since it contains only static methods and fields, until our native code unloads. So, using weak reference should be fine in this case. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1184395328
