From cf921a4ae2f45f6b647e599728a2867bc426d11b Mon Sep 17 00:00:00 2001 From: Alex Andres Date: Sun, 1 Nov 2026 19:00:04 +0100 Subject: [PATCH] fix: keep native references until their Java wrapper is created JavaFactories::createOwning builds the wrapper from the smart pointer's get() and releases the reference only once the wrapper exists and holds its handles, so a failed allocation no longer leaks the native object. All call sites that handed over released pointers use it now, including the peer connection and the frame injectors, which bind their observer before the transfer, and createOwningObjectArray. --- .../jni-voithos/include/JavaFactories.h | 44 +++++++++++++++++++ .../jni-voithos/include/JavaFactory.h | 6 +-- .../src/main/cpp/include/api/WebRTCUtils.h | 14 +++++- .../cpp/src/JNI_PeerConnectionFactory.cpp | 30 +++++++++---- .../main/cpp/src/JNI_RTCPeerConnection.cpp | 8 ++-- .../src/main/cpp/src/JNI_RTCRtpReceiver.cpp | 2 +- .../src/main/cpp/src/JNI_RTCRtpSender.cpp | 23 +++++++--- .../main/cpp/src/JNI_RTCRtpTransceiver.cpp | 4 +- .../cpp/src/api/PeerConnectionObserver.cpp | 8 ++-- 9 files changed, 109 insertions(+), 30 deletions(-) diff --git a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactories.h b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactories.h index 7e21f15c9..f14172804 100644 --- a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactories.h +++ b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactories.h @@ -15,8 +15,11 @@ #include #include +#include #include +#include #include +#include #ifdef __cpp_lib_any #include @@ -30,6 +33,47 @@ namespace jni JavaFactories() = default; ~JavaFactories() = default; + // Creates the Java object for what `owner` points to and moves the + // reference `owner` holds into it, for a Java object that owns and + // disposes its native object. The reference is handed over only once + // the Java object exists and holds the handle: if creating it fails, + // `owner` keeps the reference and frees it as usual, and the Java + // object, which nothing can reach yet, is left to the garbage + // collector. `owner` is a smart pointer with get() and release(), such + // as webrtc::scoped_refptr or std::unique_ptr, passed with std::move. + template + static JavaLocalRef createOwning(JNIEnv * env, Ptr && owner) + { + return createOwning(env, std::forward(owner), [](jobject) {}); + } + + // Same as above, and calls `bind` with the new Java object before the + // reference is handed over, so that whatever else has to be bound to + // the Java object (e.g. the handle of an observer) is bound before the + // Java object owns anything. If `bind` throws, `owner` keeps the + // reference. + template + static JavaLocalRef createOwning(JNIEnv * env, Ptr && owner, Bind && bind) + { + static_assert(!std::is_lvalue_reference::value, + "Pass the owner with std::move, the call takes its reference"); + + using T = typename std::remove_cv::type>::type; + + JavaLocalRef object = create(env, owner.get()); + + if (object.get() == nullptr) { + throw Exception("Create Java object for [%s] failed", typeid(T).name()); + } + + bind(object.get()); + + // The Java object holds the reference now. + static_cast(owner.release()); + + return object; + } + #ifdef __cpp_lib_any template static void add(JNIEnv * env, const char * className) diff --git a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactory.h b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactory.h index c2d0747b2..e7e91e02c 100644 --- a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactory.h +++ b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactory.h @@ -31,12 +31,12 @@ namespace jni virtual JavaLocalRef create(JNIEnv * env, const T * nativeObject) const { - jobject object = env->NewObject(javaClass, javaCtor); + JavaLocalRef object(env, env->NewObject(javaClass, javaCtor)); ExceptionCheck(env); - SetHandle(env, object, nativeObject); + SetHandle(env, object.get(), nativeObject); - return JavaLocalRef(env, object); + return object; } virtual JavaLocalRef createArray(JNIEnv * env, const jsize & length) const diff --git a/webrtc-jni/src/main/cpp/include/api/WebRTCUtils.h b/webrtc-jni/src/main/cpp/include/api/WebRTCUtils.h index 9735a8bf9..8ca7aff22 100644 --- a/webrtc-jni/src/main/cpp/include/api/WebRTCUtils.h +++ b/webrtc-jni/src/main/cpp/include/api/WebRTCUtils.h @@ -56,6 +56,12 @@ namespace jni // and nothing else already guarantees the pointer outlives the call -- // callers of the returned array elements are then responsible for // disposing them. + // + // The references are handed over only once the whole array is filled, as + // JavaFactories::createOwning does for a single object: if a wrapper + // cannot be created, `vector` frees all of them, and the wrappers made so + // far, which only the dropped array refers to, are left to the garbage + // collector. template JavaLocalRef createOwningObjectArray(JNIEnv * env, std::vector> vector) { @@ -68,9 +74,15 @@ namespace jni } for (jsize i = 0; i < vectorSize; i++) { - JavaLocalRef obj = JavaFactories::create(env, vector[i].release()); + JavaLocalRef obj = JavaFactories::create(env, vector[i].get()); env->SetObjectArrayElement(objectArray, i, obj.get()); + ExceptionCheck(env); + } + + // The Java objects hold the references now. + for (auto & item : vector) { + static_cast(item.release()); } return objectArray; diff --git a/webrtc-jni/src/main/cpp/src/JNI_PeerConnectionFactory.cpp b/webrtc-jni/src/main/cpp/src/JNI_PeerConnectionFactory.cpp index 122f3c824..494c2471b 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_PeerConnectionFactory.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_PeerConnectionFactory.cpp @@ -323,7 +323,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_PeerConnectionFactory_createAud return nullptr; } - return jni::JavaFactories::create(env, audioSource.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(audioSource)).release(); } catch (...) { ThrowCxxJavaException(env); @@ -354,7 +354,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_PeerConnectionFactory_createAud webrtc::scoped_refptr audioTrack = factory->CreateAudioTrack(label, source); - return jni::JavaFactories::create(env, audioTrack.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(audioTrack)).release(); } catch (...) { ThrowCxxJavaException(env); @@ -414,7 +414,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_PeerConnectionFactory_createVid webrtc::scoped_refptr videoTrack = factory->CreateVideoTrack( webrtc::scoped_refptr(source), label); - return jni::JavaFactories::create(env, videoTrack.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(videoTrack)).release(); } catch (...) { ThrowCxxJavaException(env); @@ -468,17 +468,29 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_PeerConnectionFactory_createPee auto pc = result.MoveValue(); - if (pc != nullptr) { - auto javaPeerConnection = jni::JavaFactories::create(env, pc.release()); + if (pc == nullptr) { + delete observer; + return nullptr; + } - SetHandle(env, javaPeerConnection.get(), "observerHandle", observer); + try { + // The observer is bound before the Java object takes the connection, so + // that it ends up with both of them or with neither. + auto javaPeerConnection = jni::JavaFactories::createOwning(env, std::move(pc), [&](jobject object) { + SetHandle(env, object, "observerHandle", observer); + }); return javaPeerConnection.release(); } + catch (...) { + // The connection is still ours. It calls the observer while it closes, + // so it goes first. + pc = nullptr; + delete observer; - delete observer; - - return nullptr; + ThrowCxxJavaException(env); + return nullptr; + } } JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_PeerConnectionFactory_getRtpReceiverCapabilities diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp index cc2ca4277..a40b95b94 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp @@ -133,7 +133,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCPeerConnection_addTrackInter if (result.ok()) { auto sender = result.MoveValue(); - return jni::JavaFactories::create(env, sender.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(sender)).release(); } env->Throw(jni::JavaRuntimeException(env, "%s", jni::RTCErrorToString(result.error()).c_str())); @@ -197,7 +197,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCPeerConnection_addTransceive if (result.ok()) { auto transceiver = result.MoveValue(); - return jni::JavaFactories::create(env, transceiver.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(transceiver)).release(); } env->Throw(jni::JavaRuntimeException(env, "%s", jni::RTCErrorToString(result.error()).c_str())); @@ -238,7 +238,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCPeerConnection_addTransceive if (result.ok()) { auto transceiver = result.MoveValue(); - return jni::JavaFactories::create(env, transceiver.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(transceiver)).release(); } env->Throw(jni::JavaRuntimeException(env, "%s", jni::RTCErrorToString(result.error()).c_str())); @@ -280,7 +280,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCPeerConnection_createDataCha auto dataChannel = result.MoveValue(); - return jni::JavaFactories::create(env, dataChannel.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(dataChannel)).release(); } catch (...) { ThrowCxxJavaException(env); diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp index da95519e9..c74441258 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp @@ -60,7 +60,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpReceiver_getTransport // The Java object holds a reference until it is disposed, since the // connection frees the transport once it closes or drops it. - return jni::JavaFactories::create(env, transport.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(transport)).release(); } catch (...) { ThrowCxxJavaException(env); diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp index f38910bd4..5d9ab3fa1 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp @@ -60,7 +60,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_getTransport // The Java object holds a reference until it is disposed, since the // connection frees the transport once it closes or drops it. - return jni::JavaFactories::create(env, transport.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(transport)).release(); } catch (...) { ThrowCxxJavaException(env); @@ -149,7 +149,8 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_getDtmfSender // The Java object holds a reference until it is disposed, since the // sender frees its DTMF sender once the connection drops the sender. - SetHandle(env, jDtmfSender, dtmfSender.release()); + SetHandle(env, jDtmfSender, dtmfSender.get()); + static_cast(dtmfSender.release()); return jDtmfSender.release(); } @@ -223,9 +224,14 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_createVideoFrameIn return nullptr; } - auto jInjector = jni::JavaFactories::create(env, injector.release()); + // The observer is bound before the Java object takes the injector, so + // that it ends up with both of them or with neither. + auto jInjector = jni::JavaFactories::createOwning(env, std::move(injector), [&](jobject object) { + SetHandle(env, object, "observerHandle", observer.get()); + }); - SetHandle(env, jInjector.get(), "observerHandle", observer.release()); + // The Java object owns the observer now. + static_cast(observer.release()); return jInjector.release(); } @@ -259,9 +265,14 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_createAudioFrameIn return nullptr; } - auto jInjector = jni::JavaFactories::create(env, injector.release()); + // The observer is bound before the Java object takes the injector, so + // that it ends up with both of them or with neither. + auto jInjector = jni::JavaFactories::createOwning(env, std::move(injector), [&](jobject object) { + SetHandle(env, object, "observerHandle", observer.get()); + }); - SetHandle(env, jInjector.get(), "observerHandle", observer.release()); + // The Java object owns the observer now. + static_cast(observer.release()); return jInjector.release(); } diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpTransceiver.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpTransceiver.cpp index 18ef0b0e6..fe282dcff 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpTransceiver.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpTransceiver.cpp @@ -58,7 +58,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpTransceiver_getSender // Transfer the reference this local scoped_refptr holds into the Java // wrapper, which owns and disposes it -- otherwise the wrapper would be // left with a pointer nothing on the Java/JNI side keeps alive. - return jni::JavaFactories::create(env, sender.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(sender)).release(); } catch (...) { ThrowCxxJavaException(env); @@ -78,7 +78,7 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpTransceiver_getReceiver // Transfer the reference this local scoped_refptr holds into the Java // wrapper, which owns and disposes it -- otherwise the wrapper would be // left with a pointer nothing on the Java/JNI side keeps alive. - return jni::JavaFactories::create(env, receiver.release()).release(); + return jni::JavaFactories::createOwning(env, std::move(receiver)).release(); } catch (...) { ThrowCxxJavaException(env); diff --git a/webrtc-jni/src/main/cpp/src/api/PeerConnectionObserver.cpp b/webrtc-jni/src/main/cpp/src/api/PeerConnectionObserver.cpp index 1d093e995..0c05f3633 100644 --- a/webrtc-jni/src/main/cpp/src/api/PeerConnectionObserver.cpp +++ b/webrtc-jni/src/main/cpp/src/api/PeerConnectionObserver.cpp @@ -90,7 +90,7 @@ namespace jni // it; transfer that reference into the Java wrapper, which disposes // it, instead of releasing it right back when transceiver goes out // of scope and leaving the wrapper's pointer unowned. - auto jTransceiver = JavaFactories::create(env, transceiver.release()); + auto jTransceiver = JavaFactories::createOwning(env, std::move(transceiver)); env->CallVoidMethod(observer, javaClass->onTrack, jTransceiver.get()); } @@ -118,7 +118,7 @@ namespace jni // it; transfer that reference into the Java wrapper, which // disposes it, instead of releasing it right back when receiver // goes out of scope and leaving the wrapper's pointer unowned. - auto jReceiver = JavaFactories::create(env, receiver.release()); + auto jReceiver = JavaFactories::createOwning(env, std::move(receiver)); env->CallVoidMethod(observer, javaClass->onAddTrack, jReceiver.get(), streamArray.get()); } @@ -142,7 +142,7 @@ namespace jni // it; transfer that reference into the Java wrapper, which disposes // it, instead of releasing it right back when receiver goes out of // scope and leaving the wrapper's pointer unowned. - auto jReceiver = JavaFactories::create(env, receiver.release()); + auto jReceiver = JavaFactories::createOwning(env, std::move(receiver)); env->CallVoidMethod(observer, javaClass->onRemoveTrack, jReceiver.get()); } @@ -162,7 +162,7 @@ namespace jni } try { - auto jDataChannel = JavaFactories::create(env, channel.release()); + auto jDataChannel = JavaFactories::createOwning(env, std::move(channel)); env->CallVoidMethod(observer, javaClass->onDataChannel, jDataChannel.get()); }