From 307da3ad2d455223d0b4742df8106b5a3f287658 Mon Sep 17 00:00:00 2001 From: Thad House Date: Tue, 29 May 2018 15:44:16 -0700 Subject: [PATCH] Simplify allocation of JNI global classes and exceptions (#1110) Helps ensure they get freed properly (We have had a few cases before where this wasn't the case). --- .../main/native/cpp/jni/CameraServerJNI.cpp | 46 ++++---- hal/src/main/native/cpp/jni/HALUtil.cpp | 105 +++++++----------- .../main/native/cpp/jni/NetworkTablesJNI.cpp | 85 ++++++-------- .../src/main/native/include/wpi/jni_util.h | 10 ++ 4 files changed, 104 insertions(+), 142 deletions(-) diff --git a/cscore/src/main/native/cpp/jni/CameraServerJNI.cpp b/cscore/src/main/native/cpp/jni/CameraServerJNI.cpp index 3b53348584..693c7421f9 100644 --- a/cscore/src/main/native/cpp/jni/CameraServerJNI.cpp +++ b/cscore/src/main/native/cpp/jni/CameraServerJNI.cpp @@ -29,6 +29,16 @@ static JException unsupportedEx; // Thread-attached environment for listener callbacks. static JNIEnv* listenerEnv = nullptr; +static const JClassInit classes[] = { + {"edu/wpi/cscore/UsbCameraInfo", &usbCameraInfoCls}, + {"edu/wpi/cscore/VideoMode", &videoModeCls}, + {"edu/wpi/cscore/VideoEvent", &videoEventCls}}; + +static const JExceptionInit exceptions[] = { + {"edu/wpi/cscore/VideoException", &videoEx}, + {"java/lang/NullPointerException", &nullPointerEx}, + {"java/lang/UnsupportedOperationException", &unsupportedEx}}; + static void ListenerOnStart() { if (!jvm) return; JNIEnv* env; @@ -59,23 +69,15 @@ JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM* vm, void* reserved) { return JNI_ERR; // Cache references to classes - usbCameraInfoCls = JClass{env, "edu/wpi/cscore/UsbCameraInfo"}; - if (!usbCameraInfoCls) return JNI_ERR; + for (auto& c : classes) { + *c.cls = JClass(env, c.name); + if (!*c.cls) return JNI_ERR; + } - videoModeCls = JClass{env, "edu/wpi/cscore/VideoMode"}; - if (!videoModeCls) return JNI_ERR; - - videoEventCls = JClass{env, "edu/wpi/cscore/VideoEvent"}; - if (!videoEventCls) return JNI_ERR; - - videoEx = JException{env, "edu/wpi/cscore/VideoException"}; - if (!videoEx) return JNI_ERR; - - nullPointerEx = JException(env, "java/lang/NullPointerException"); - if (!nullPointerEx) return JNI_ERR; - - unsupportedEx = JException(env, "java/lang/UnsupportedOperationException"); - if (!unsupportedEx) return JNI_ERR; + for (auto& c : exceptions) { + *c.cls = JException(env, c.name); + if (!*c.cls) return JNI_ERR; + } // Initial configuration of listener start/exit cs::SetListenerOnStart(ListenerOnStart); @@ -89,12 +91,12 @@ JNIEXPORT void JNICALL JNI_OnUnload(JavaVM* vm, void* reserved) { if (vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6) != JNI_OK) return; // Delete global references - usbCameraInfoCls.free(env); - videoModeCls.free(env); - videoEventCls.free(env); - videoEx.free(env); - nullPointerEx.free(env); - unsupportedEx.free(env); + for (auto& c : classes) { + c.cls->free(env); + } + for (auto& c : exceptions) { + c.cls->free(env); + } jvm = nullptr; } diff --git a/hal/src/main/native/cpp/jni/HALUtil.cpp b/hal/src/main/native/cpp/jni/HALUtil.cpp index 5e2470ca50..2c2db453d9 100644 --- a/hal/src/main/native/cpp/jni/HALUtil.cpp +++ b/hal/src/main/native/cpp/jni/HALUtil.cpp @@ -61,6 +61,29 @@ static JClass matchInfoDataCls; static JClass accumulatorResultCls; static JClass canDataCls; +static const JClassInit classes[] = { + {"edu/wpi/first/wpilibj/PWMConfigDataResult", &pwmConfigDataResultCls}, + {"edu/wpi/first/wpilibj/can/CANStatus", &canStatusCls}, + {"edu/wpi/first/wpilibj/hal/MatchInfoData", &matchInfoDataCls}, + {"edu/wpi/first/wpilibj/AccumulatorResult", &accumulatorResultCls}, + {"edu/wpi/first/wpilibj/CANData", &canDataCls}}; + +static const JExceptionInit exceptions[] = { + {"java/lang/RuntimeException", &runtimeExCls}, + {"java/lang/IllegalArgumentException", &illegalArgExCls}, + {"edu/wpi/first/wpilibj/util/BoundaryException", &boundaryExCls}, + {"edu/wpi/first/wpilibj/util/AllocationException", &allocationExCls}, + {"edu/wpi/first/wpilibj/util/HalHandleException", &halHandleExCls}, + {"edu/wpi/first/wpilibj/can/CANInvalidBufferException", + &canInvalidBufferExCls}, + {"edu/wpi/first/wpilibj/can/CANMessageNotFoundException", + &canMessageNotFoundExCls}, + {"edu/wpi/first/wpilibj/can/CANMessageNotAllowedException", + &canMessageNotAllowedExCls}, + {"edu/wpi/first/wpilibj/can/CANNotInitializedException", + &canNotInitializedExCls}, + {"edu/wpi/first/wpilibj/util/UncleanStatusException", &uncleanStatusExCls}}; + namespace frc { void ThrowAllocationException(JNIEnv* env, int32_t minRange, int32_t maxRange, @@ -273,59 +296,15 @@ JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM* vm, void* reserved) { if (vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6) != JNI_OK) return JNI_ERR; - runtimeExCls = JException(env, "java/lang/RuntimeException"); - if (!runtimeExCls) return JNI_ERR; + for (auto& c : classes) { + *c.cls = JClass(env, c.name); + if (!*c.cls) return JNI_ERR; + } - illegalArgExCls = JException(env, "java/lang/IllegalArgumentException"); - if (!illegalArgExCls) return JNI_ERR; - - boundaryExCls = - JException(env, "edu/wpi/first/wpilibj/util/BoundaryException"); - if (!boundaryExCls) return JNI_ERR; - - allocationExCls = - JException(env, "edu/wpi/first/wpilibj/util/AllocationException"); - if (!allocationExCls) return JNI_ERR; - - halHandleExCls = - JException(env, "edu/wpi/first/wpilibj/util/HalHandleException"); - if (!halHandleExCls) return JNI_ERR; - - canInvalidBufferExCls = - JException(env, "edu/wpi/first/wpilibj/can/CANInvalidBufferException"); - if (!canInvalidBufferExCls) return JNI_ERR; - - canMessageNotFoundExCls = - JException(env, "edu/wpi/first/wpilibj/can/CANMessageNotFoundException"); - if (!canMessageNotFoundExCls) return JNI_ERR; - - canMessageNotAllowedExCls = JException( - env, "edu/wpi/first/wpilibj/can/CANMessageNotAllowedException"); - if (!canMessageNotAllowedExCls) return JNI_ERR; - - canNotInitializedExCls = - JException(env, "edu/wpi/first/wpilibj/can/CANNotInitializedException"); - if (!canNotInitializedExCls) return JNI_ERR; - - uncleanStatusExCls = - JException(env, "edu/wpi/first/wpilibj/util/UncleanStatusException"); - if (!uncleanStatusExCls) return JNI_ERR; - - pwmConfigDataResultCls = - JClass(env, "edu/wpi/first/wpilibj/PWMConfigDataResult"); - if (!pwmConfigDataResultCls) return JNI_ERR; - - canStatusCls = JClass(env, "edu/wpi/first/wpilibj/can/CANStatus"); - if (!canStatusCls) return JNI_ERR; - - matchInfoDataCls = JClass(env, "edu/wpi/first/wpilibj/hal/MatchInfoData"); - if (!matchInfoDataCls) return JNI_ERR; - - accumulatorResultCls = JClass(env, "edu/wpi/first/wpilibj/AccumulatorResult"); - if (!accumulatorResultCls) return JNI_ERR; - - canDataCls = JClass(env, "edu/wpi/first/wpilibj/CANData"); - if (!canDataCls) return JNI_ERR; + for (auto& c : exceptions) { + *c.cls = JException(env, c.name); + if (!*c.cls) return JNI_ERR; + } return sim::SimOnLoad(vm, reserved); } @@ -337,21 +316,13 @@ JNIEXPORT void JNICALL JNI_OnUnload(JavaVM* vm, void* reserved) { if (vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6) != JNI_OK) return; // Delete global references - runtimeExCls.free(env); - illegalArgExCls.free(env); - boundaryExCls.free(env); - allocationExCls.free(env); - halHandleExCls.free(env); - canInvalidBufferExCls.free(env); - canMessageNotFoundExCls.free(env); - canMessageNotAllowedExCls.free(env); - canNotInitializedExCls.free(env); - uncleanStatusExCls.free(env); - pwmConfigDataResultCls.free(env); - canStatusCls.free(env); - matchInfoDataCls.free(env); - accumulatorResultCls.free(env); - canDataCls.free(env); + + for (auto& c : classes) { + c.cls->free(env); + } + for (auto& c : exceptions) { + c.cls->free(env); + } jvm = nullptr; } diff --git a/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp b/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp index 96134fe7b2..e93a7aef38 100644 --- a/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp +++ b/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp @@ -43,6 +43,24 @@ static JException interruptedEx; static JException nullPointerEx; static JException persistentEx; +static const JClassInit classes[] = { + {"java/lang/Boolean", &booleanCls}, + {"edu/wpi/first/networktables/ConnectionInfo", &connectionInfoCls}, + {"edu/wpi/first/networktables/ConnectionNotification", + &connectionNotificationCls}, + {"java/lang/Double", &doubleCls}, + {"edu/wpi/first/networktables/EntryInfo", &entryInfoCls}, + {"edu/wpi/first/networktables/EntryNotification", &entryNotificationCls}, + {"edu/wpi/first/networktables/LogMessage", &logMessageCls}, + {"edu/wpi/first/networktables/RpcAnswer", &rpcAnswerCls}, + {"edu/wpi/first/networktables/NetworkTableValue", &valueCls}}; + +static const JExceptionInit exceptions[] = { + {"java/lang/IllegalArgumentException", &illegalArgEx}, + {"java/lang/InterruptedException", &interruptedEx}, + {"java/lang/NullPointerException", &nullPointerEx}, + {"edu/wpi/first/networktables/PersistentException", &persistentEx}}; + extern "C" { JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM* vm, void* reserved) { @@ -53,47 +71,15 @@ JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM* vm, void* reserved) { return JNI_ERR; // Cache references to classes - booleanCls = JClass(env, "java/lang/Boolean"); - if (!booleanCls) return JNI_ERR; + for (auto& c : classes) { + *c.cls = JClass(env, c.name); + if (!*c.cls) return JNI_ERR; + } - connectionInfoCls = JClass(env, "edu/wpi/first/networktables/ConnectionInfo"); - if (!connectionInfoCls) return JNI_ERR; - - connectionNotificationCls = - JClass(env, "edu/wpi/first/networktables/ConnectionNotification"); - if (!connectionNotificationCls) return JNI_ERR; - - doubleCls = JClass(env, "java/lang/Double"); - if (!doubleCls) return JNI_ERR; - - entryInfoCls = JClass(env, "edu/wpi/first/networktables/EntryInfo"); - if (!entryInfoCls) return JNI_ERR; - - entryNotificationCls = - JClass(env, "edu/wpi/first/networktables/EntryNotification"); - if (!entryNotificationCls) return JNI_ERR; - - logMessageCls = JClass(env, "edu/wpi/first/networktables/LogMessage"); - if (!logMessageCls) return JNI_ERR; - - rpcAnswerCls = JClass(env, "edu/wpi/first/networktables/RpcAnswer"); - if (!rpcAnswerCls) return JNI_ERR; - - valueCls = JClass(env, "edu/wpi/first/networktables/NetworkTableValue"); - if (!valueCls) return JNI_ERR; - - illegalArgEx = JException(env, "java/lang/IllegalArgumentException"); - if (!illegalArgEx) return JNI_ERR; - - interruptedEx = JException(env, "java/lang/InterruptedException"); - if (!interruptedEx) return JNI_ERR; - - nullPointerEx = JException(env, "java/lang/NullPointerException"); - if (!nullPointerEx) return JNI_ERR; - - persistentEx = - JException(env, "edu/wpi/first/networktables/PersistentException"); - if (!persistentEx) return JNI_ERR; + for (auto& c : exceptions) { + *c.cls = JException(env, c.name); + if (!*c.cls) return JNI_ERR; + } return JNI_VERSION_1_6; } @@ -103,19 +89,12 @@ JNIEXPORT void JNICALL JNI_OnUnload(JavaVM* vm, void* reserved) { if (vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6) != JNI_OK) return; // Delete global references - booleanCls.free(env); - connectionInfoCls.free(env); - connectionNotificationCls.free(env); - doubleCls.free(env); - entryInfoCls.free(env); - entryNotificationCls.free(env); - logMessageCls.free(env); - rpcAnswerCls.free(env); - valueCls.free(env); - illegalArgEx.free(env); - interruptedEx.free(env); - nullPointerEx.free(env); - persistentEx.free(env); + for (auto& c : classes) { + c.cls->free(env); + } + for (auto& c : exceptions) { + c.cls->free(env); + } jvm = nullptr; } diff --git a/wpiutil/src/main/native/include/wpi/jni_util.h b/wpiutil/src/main/native/include/wpi/jni_util.h index e86011bb63..469dd0ea91 100644 --- a/wpiutil/src/main/native/include/wpi/jni_util.h +++ b/wpiutil/src/main/native/include/wpi/jni_util.h @@ -71,6 +71,11 @@ class JClass { jclass m_cls = nullptr; }; +struct JClassInit { + const char* name; + JClass* cls; +}; + template class JGlobal { public: @@ -618,6 +623,11 @@ class JException : public JClass { jmethodID m_constructor = nullptr; }; +struct JExceptionInit { + const char* name; + JException* cls; +}; + } // namespace java } // namespace wpi