fix: keep native references until their Java wrapper is created

webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactories.h

@@ -15,8 +15,11 @@
 
 #include <jni.h>
 #include <memory>
+#include <type_traits>
 #include <typeindex>
+#include <typeinfo>
 #include <unordered_map>
+#include <utility>
 
 #ifdef __cpp_lib_any
 #include <any>
@@ -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 <class Ptr>
+			static JavaLocalRef<jobject> createOwning(JNIEnv * env, Ptr && owner)
+			{
+				return createOwning(env, std::forward<Ptr>(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 <class Ptr, class Bind>
+			static JavaLocalRef<jobject> createOwning(JNIEnv * env, Ptr && owner, Bind && bind)
+			{
+				static_assert(!std::is_lvalue_reference<Ptr>::value,
+					"Pass the owner with std::move, the call takes its reference");
+
+				using T = typename std::remove_cv<typename std::remove_pointer<decltype(owner.get())>::type>::type;
+
+				JavaLocalRef<jobject> object = create<T>(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<void>(owner.release());
+
+				return object;
+			}
+
 #ifdef __cpp_lib_any
 			template <class T>
 			static void add(JNIEnv * env, const char * className)

webrtc-jni/src/main/cpp/dependencies/jni-voithos/include/JavaFactory.h

@@ -31,12 +31,12 @@ namespace jni
 
 			virtual JavaLocalRef<jobject> create(JNIEnv * env, const T * nativeObject) const
 			{
-				jobject object = env->NewObject(javaClass, javaCtor);
+				JavaLocalRef<jobject> object(env, env->NewObject(javaClass, javaCtor));
 				ExceptionCheck(env);
 
-				SetHandle(env, object, nativeObject);
+				SetHandle(env, object.get(), nativeObject);
 
-				return JavaLocalRef<jobject>(env, object);
+				return object;
 			}
 
 			virtual JavaLocalRef<jobjectArray> createArray(JNIEnv * env, const jsize & length) const

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 <class T>
 	JavaLocalRef<jobjectArray> createOwningObjectArray(JNIEnv * env, std::vector<webrtc::scoped_refptr<T>> vector)
 	{
@@ -68,9 +74,15 @@ namespace jni
 		}
 
 		for (jsize i = 0; i < vectorSize; i++) {
-			JavaLocalRef<jobject> obj = JavaFactories::create(env, vector[i].release());
+			JavaLocalRef<jobject> 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<void>(item.release());
 		}
 
 		return objectArray;

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<webrtc::AudioTrackInterface> 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<webrtc::VideoTrackInterface> videoTrack = factory->CreateVideoTrack(
 			webrtc::scoped_refptr<webrtc::VideoTrackSourceInterface>(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

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

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

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<void>(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<void>(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<void>(observer.release());
 
 		return jInjector.release();
 	}

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

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());
 		}