diff --git a/gradle.properties b/gradle.properties index 89429d7..ea59365 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1 +1 @@ -version=1.0.4 +version=1.0.4-ip.1 diff --git a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaUtils.h b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaUtils.h index 28c1305..575a158 100644 --- a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaUtils.h +++ b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaUtils.h @@ -115,4 +115,33 @@ void SetHandle(JNIEnv * env, jobject obj, T * t) SetHandle(env, obj, "nativeHandle", t); } +// Replaces the native observer stored in the `handleName` field on `obj` +// with `observer`, deleting whatever was stored there before. Use this +// together with ClearNativeObserver() wherever a Java object registers a +// heap-allocated native observer against a "replace the previous one" +// register() API, so the previous observer's lifetime can never be +// forgotten and leaked. +template +void ReplaceNativeObserver(JNIEnv * env, jobject obj, const std::string & handleName, T * observer) +{ + T * old = GetHandle(env, obj, handleName); + + delete old; + + SetHandle(env, obj, handleName, observer); +} + +// Deletes and clears the native observer stored in the `handleName` field +// on `obj`, if any. Safe to call when no observer is currently registered. +template +void ClearNativeObserver(JNIEnv * env, jobject obj, const std::string & handleName) +{ + T * observer = GetHandle(env, obj, handleName); + + if (observer) { + SetHandle(env, obj, handleName, nullptr); + delete observer; + } +} + #endif \ No newline at end of file diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCDataChannel.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCDataChannel.cpp index b1300fb..a0ed49f 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCDataChannel.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCDataChannel.cpp @@ -30,7 +30,14 @@ JNIEXPORT void JNICALL Java_dev_kastle_webrtc_RTCDataChannel_registerObserver webrtc::DataChannelInterface * channel = GetHandle(env, caller); CHECK_HANDLE(channel); - channel->RegisterObserver(new jni::RTCDataChannelObserver(env, jni::JavaGlobalRef(env, jObserver))); + // Unregister and free a previously registered observer before replacing + // it, otherwise it would leak along with its JNI global reference. + channel->UnregisterObserver(); + + auto observer = new jni::RTCDataChannelObserver(env, jni::JavaGlobalRef(env, jObserver)); + ReplaceNativeObserver(env, caller, "observerHandle", observer); + + channel->RegisterObserver(observer); } JNIEXPORT void JNICALL Java_dev_kastle_webrtc_RTCDataChannel_unregisterObserver @@ -40,6 +47,8 @@ JNIEXPORT void JNICALL Java_dev_kastle_webrtc_RTCDataChannel_unregisterObserver CHECK_HANDLE(channel); channel->UnregisterObserver(); + + ClearNativeObserver(env, caller, "observerHandle"); } JNIEXPORT jstring JNICALL Java_dev_kastle_webrtc_RTCDataChannel_getLabel @@ -147,6 +156,10 @@ JNIEXPORT void JNICALL Java_dev_kastle_webrtc_RTCDataChannel_dispose webrtc::DataChannelInterface * channel = GetHandle(env, caller); CHECK_HANDLE(channel); + channel->UnregisterObserver(); + + ClearNativeObserver(env, caller, "observerHandle"); + webrtc::RefCountReleaseStatus status = channel->Release(); if (status != webrtc::RefCountReleaseStatus::kDroppedLastRef) { diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp index f388eec..70e0e94 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp @@ -458,12 +458,11 @@ JNIEXPORT void JNICALL Java_dev_kastle_webrtc_RTCPeerConnection_close SetHandle(env, caller, nullptr); - auto observer = GetHandle(env, caller, "observerHandle"); + ClearNativeObserver(env, caller, "observerHandle"); - if (observer) { - SetHandle(env, caller, "observerHandle", nullptr); - delete observer; - } + // Drop the owning reference taken when the PeerConnection was handed + // to Java in PeerConnectionFactory::createPeerConnection. + pc->Release(); } catch (...) { ThrowCxxJavaException(env); diff --git a/webrtc-jni/src/main/cpp/src/api/DataBufferFactory.cpp b/webrtc-jni/src/main/cpp/src/api/DataBufferFactory.cpp index 619b376..3f6ccc6 100644 --- a/webrtc-jni/src/main/cpp/src/api/DataBufferFactory.cpp +++ b/webrtc-jni/src/main/cpp/src/api/DataBufferFactory.cpp @@ -27,10 +27,12 @@ namespace jni JavaLocalRef DataBufferFactory::create(JNIEnv * env, const webrtc::DataBuffer * dataBuffer) const { - jobject directBuffer = env->NewDirectByteBuffer(const_cast(dataBuffer->data.data()), dataBuffer->data.size()); + // Held by a JavaLocalRef so the local reference is deleted once the buffer object + // holds its own reference. Callers on attached native threads never free it otherwise. + JavaLocalRef directBuffer(env, env->NewDirectByteBuffer(const_cast(dataBuffer->data.data()), dataBuffer->data.size())); const jboolean isBinary = static_cast(dataBuffer->binary); - jobject object = env->NewObject(javaClass, javaCtor, directBuffer, isBinary); + jobject object = env->NewObject(javaClass, javaCtor, directBuffer.get(), isBinary); ExceptionCheck(env); return JavaLocalRef(env, object); diff --git a/webrtc-jni/src/main/cpp/src/api/RTCDataChannelObserver.cpp b/webrtc-jni/src/main/cpp/src/api/RTCDataChannelObserver.cpp index c93c29f..a6d6747 100644 --- a/webrtc-jni/src/main/cpp/src/api/RTCDataChannelObserver.cpp +++ b/webrtc-jni/src/main/cpp/src/api/RTCDataChannelObserver.cpp @@ -31,6 +31,10 @@ namespace jni { JNIEnv * env = AttachCurrentThread(); + if (env == nullptr) { + return; + } + env->CallVoidMethod(observer, javaClass->onStateChange); ExceptionCheck(env); @@ -40,9 +44,16 @@ namespace jni { JNIEnv * env = AttachCurrentThread(); + if (env == nullptr) { + return; + } + + // This runs on a native WebRTC thread that stays attached to the JVM, so a local + // reference is never freed by returning to Java. Keep ownership with jBuffer, so the + // reference is deleted when it goes out of scope instead of leaking on every message. JavaLocalRef jBuffer = bufferFactory->create(env, &buffer); - env->CallVoidMethod(observer, javaClass->onMessage, jBuffer.release()); + env->CallVoidMethod(observer, javaClass->onMessage, jBuffer.get()); ExceptionCheck(env); } @@ -51,6 +62,10 @@ namespace jni { JNIEnv * env = AttachCurrentThread(); + if (env == nullptr) { + return; + } + env->CallVoidMethod(observer, javaClass->onBufferedAmountChange, static_cast(sent_data_size)); ExceptionCheck(env); diff --git a/webrtc/src/main/java/dev/kastle/webrtc/RTCDataChannel.java b/webrtc/src/main/java/dev/kastle/webrtc/RTCDataChannel.java index 3da6414..e81f56f 100644 --- a/webrtc/src/main/java/dev/kastle/webrtc/RTCDataChannel.java +++ b/webrtc/src/main/java/dev/kastle/webrtc/RTCDataChannel.java @@ -28,6 +28,16 @@ */ public class RTCDataChannel extends DisposableNativeObject { + /** + * The native observer registered via {@link #registerObserver}. The + * RTCDataChannel does not take ownership of the Java observer, but it + * owns the native observer wrapper; it is freed when replaced, when + * {@link #unregisterObserver()} is called, and when this channel is + * disposed. + */ + @SuppressWarnings("unused") + private long observerHandle; + /** * Used by the native api. */