fix: native observer and object lifecycles around closing a connection

webrtc-jni/src/main/cpp/include/api/SetSessionDescriptionObserver.h

@@ -21,17 +21,23 @@
 #include "JavaRef.h"
 
 #include "api/jsep.h"
+#include "api/rtc_error.h"
 
 #include <jni.h>
 #include <memory>
 
 namespace jni
 {
+	// Reports the outcome of setting a local or remote description to a Java
+	// SetSessionDescriptionObserver, exactly once.
 	class SetSessionDescriptionObserver : public webrtc::SetSessionDescriptionObserver
 	{
 		public:
 			explicit SetSessionDescriptionObserver(JNIEnv * env, const JavaGlobalRef<jobject> & observer);
-			~SetSessionDescriptionObserver() = default;
+			~SetSessionDescriptionObserver();
+
+			SetSessionDescriptionObserver(const SetSessionDescriptionObserver &) = delete;
+			SetSessionDescriptionObserver & operator=(const SetSessionDescriptionObserver &) = delete;
 
 			// SetSessionDescriptionObserver implementation.
 			void OnSuccess() override;
@@ -47,6 +53,8 @@ namespace jni
 					jmethodID onFailure;
 			};
 
+			void Notify(const char * error) noexcept;
+
 		private:
 			JavaGlobalRef<jobject> observer;
 

webrtc-jni/src/main/cpp/src/JNI_RTCDtlsTransport.cpp

@@ -119,4 +119,25 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_unregisterObserve
 	transport->UnregisterObserver();
 
 	ClearNativeObserver<jni::RTCDtlsTransportObserver>(env, caller, "observerHandle");
+}
+
+JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_dispose
+(JNIEnv * env, jobject caller)
+{
+	webrtc::DtlsTransportInterface * transport = GetHandle<webrtc::DtlsTransportInterface>(env, caller);
+	CHECK_HANDLE(transport);
+
+	// WebRTC must let go of an observer registered through this object before
+	// it is deleted.
+	if (GetHandle<jni::RTCDtlsTransportObserver>(env, caller, "observerHandle") != nullptr) {
+		transport->UnregisterObserver();
+
+		ClearNativeObserver<jni::RTCDtlsTransportObserver>(env, caller, "observerHandle");
+	}
+
+	// The connection, its senders and receivers, and other Java objects keep
+	// their own references, so this one need not be the last.
+	transport->Release();
+
+	SetHandle<std::nullptr_t>(env, caller, nullptr);
 }

webrtc-jni/src/main/cpp/src/JNI_RTCPeerConnection.cpp

@@ -39,15 +39,18 @@
 #include "JavaRuntimeException.h"
 #include "JavaString.h"
 #include "JavaUtils.h"
+#include "JNI_WebRTC.h"
 
 #include "api/peer_connection_interface.h"
 #include "api/transport/bitrate_settings.h"
 #include "api/rtp_receiver_interface.h"
 #include "api/rtp_sender_interface.h"
+#include "rtc_base/thread.h"
 
 #include <memory>
 #include <optional>
 #include <string>
+#include <utility>
 #include <vector>
 
 JNIEXPORT jobjectArray JNICALL Java_dev_onvoid_webrtc_RTCPeerConnection_getSenders
@@ -838,15 +841,37 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCPeerConnection_close
 	CHECK_HANDLE(pc);
 
 	try {
-		pc->Close();
+		jni::JavaLocalRef<jobject> factory(env, env->GetObjectField(caller,
+			GetFieldID(env, caller, "factory", "L" PKG "PeerConnectionFactory;")));
+		webrtc::Thread * signalingThread = factory.get() != nullptr
+			? GetHandle<webrtc::Thread>(env, factory.get(), "signalingThreadHandle")
+			: nullptr;
+
+		webrtc::PeerConnectionObserver * observer = GetHandle<webrtc::PeerConnectionObserver>(env, caller, "observerHandle");
 
 		SetHandle<std::nullptr_t>(env, caller, nullptr);
+		SetHandle<std::nullptr_t>(env, caller, "observerHandle", nullptr);
+
+		auto close = [pc, observer]() {
+			pc->Close();
+
+			// The connection lets go of its observer once closed.
+			delete observer;
 
-		ClearNativeObserver<webrtc::PeerConnectionObserver>(env, caller, "observerHandle");
+			// Drop the owning reference taken when the PeerConnection was handed
+			// to Java in PeerConnectionFactory::createPeerConnection.
+			pc->Release();
+		};
 
-		// Drop the owning reference taken when the PeerConnection was handed
-		// to Java in PeerConnectionFactory::createPeerConnection.
-		pc->Release();
+		if (signalingThread != nullptr && signalingThread->IsCurrent()) {
+			// Called from a callback of this or another connection of the same
+			// factory. WebRTC goes on with the connection once the callback has
+			// returned, so it must not be closed and freed under its feet.
+			signalingThread->PostTask(std::move(close));
+		}
+		else {
+			close();
+		}
 	}
 	catch (...) {
 		ThrowCxxJavaException(env);

webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp

@@ -53,7 +53,13 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpReceiver_getTransport
 
 		auto transport = receiver->dtls_transport();
 
-		return jni::JavaFactories::create(env, transport.get()).release();
+		if (!transport) {
+			return nullptr;
+		}
+
+		// 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();
 	}
 	catch (...) {
 		ThrowCxxJavaException(env);

webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp

@@ -53,7 +53,13 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_getTransport
 
 		auto transport = sender->dtls_transport();
 
-		return jni::JavaFactories::create(env, transport.get()).release();
+		if (!transport) {
+			return nullptr;
+		}
+
+		// 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();
 	}
 	catch (...) {
 		ThrowCxxJavaException(env);

webrtc-jni/src/main/cpp/src/api/SetSessionDescriptionObserver.cpp

@@ -20,6 +20,8 @@
 #include "JNI_WebRTC.h"
 #include "JavaUtils.h"
 
+#include <utility>
+
 namespace jni
 {
 	SetSessionDescriptionObserver::SetSessionDescriptionObserver(JNIEnv * env, const JavaGlobalRef<jobject> & observer) :
@@ -28,36 +30,54 @@ namespace jni
 	{
 	}
 
+	SetSessionDescriptionObserver::~SetSessionDescriptionObserver()
+	{
+		// WebRTC posts the result to the signaling thread and drops it, together
+		// with this observer, when the peer connection goes away before it runs.
+		Notify("[INVALID_STATE] Setting the session description was discarded before completion");
+	}
+
 	void SetSessionDescriptionObserver::OnSuccess()
 	{
-		JNIEnv * env = AttachCurrentThread();
+		Notify(nullptr);
+	}
 
-		if (env == nullptr) {
-			return;
+	void SetSessionDescriptionObserver::OnFailure(webrtc::RTCError error)
+	{
+		try {
+			Notify(RTCErrorToString(error).c_str());
+		}
+		catch (...) {
+			Notify("[INTERNAL_ERROR] Could not report the session description result");
 		}
-
-		env->CallVoidMethod(observer, javaClass->onSuccess);
-
-		ReportPendingException(env);
 	}
 
-	void SetSessionDescriptionObserver::OnFailure(webrtc::RTCError error)
+	void SetSessionDescriptionObserver::Notify(const char * error) noexcept
 	{
+		JavaGlobalRef<jobject> callback(std::move(observer));
+
+		if (!callback.get()) {
+			return;
+		}
+
 		JNIEnv * env = AttachCurrentThread();
 
 		if (env == nullptr) {
 			return;
 		}
 
-		try {
-			JavaLocalRef<jstring> errorMessage = JavaString::toJava(env, RTCErrorToString(error));
-
-			env->CallVoidMethod(observer, javaClass->onFailure, errorMessage.get());
+		if (error == nullptr) {
+			env->CallVoidMethod(callback.get(), javaClass->onSuccess);
 		}
-		catch (...) {
-			ThrowCxxJavaException(env);
+		else {
+			JavaLocalRef<jstring> message(env, env->NewStringUTF(error));
+
+			if (!env->ExceptionCheck()) {
+				env->CallVoidMethod(callback.get(), javaClass->onFailure, message.get());
+			}
 		}
 
+		// A Java exception must not escape into WebRTC's signaling thread.
 		ReportPendingException(env);
 	}
 

webrtc/src/main/java/dev/onvoid/webrtc/RTCDtlsTransport.java

@@ -18,7 +18,7 @@
 
 import java.util.List;
 
-import dev.onvoid.webrtc.internal.NativeObject;
+import dev.onvoid.webrtc.internal.DisposableNativeObject;
 
 /**
  * Allows an application to access information about the Datagram Transport
@@ -31,16 +31,22 @@
  * transport layer for the RTP or RTCP component of a specific {@link
  * RTCRtpTransceiver}, or a group of RTCRtpTransceivers if such a group has been
  * negotiated via [BUNDLE].
+ * <p>
+ * Each instance holds a reference to the native transport, which keeps it
+ * readable after the connection has closed or dropped it; the transport then
+ * reports {@link RTCDtlsTransportState#CLOSED}. Release it with {@link
+ * #dispose()} once it is no longer needed.
  *
  * @author Alex Andres
  */
-public class RTCDtlsTransport extends NativeObject {
+public class RTCDtlsTransport extends DisposableNativeObject {
 
 	/**
 	 * The native observer registered via {@link #registerObserver}. The
 	 * RTCDtlsTransport does not take ownership of the Java observer, but it
-	 * owns the native observer wrapper; it is freed when replaced and when
-	 * {@link #unregisterObserver()} is called.
+	 * owns the native observer wrapper; it is freed when replaced, when
+	 * {@link #unregisterObserver()} is called, and when this transport is
+	 * disposed.
 	 */
 	@SuppressWarnings("unused")
 	private long observerHandle;
@@ -84,6 +90,20 @@ public class RTCDtlsTransport extends NativeObject {
 	 */
 	public native void unregisterObserver();
 
+	/**
+	 * Unregisters the observer registered through this instance, if any, and
+	 * releases the native reference held by this instance.
+	 * <p>
+	 * An RTCDtlsTransport is not exclusively owned by this instance: the
+	 * connection, its senders and receivers, and other RTCDtlsTransport
+	 * instances obtained via separate calls to {@link
+	 * RTCRtpSender#getTransport()} or {@link RTCRtpReceiver#getTransport()}
+	 * keep their own references. Disposing this instance does not affect the
+	 * transport itself or any other instance referring to it.
+	 */
+	@Override
+	public native void dispose();
+
 	/**
 	 * Two RTCDtlsTransport instances are equal if they are bound to the same
 	 * native transport, e.g. when obtained from two separate calls to {@link

webrtc/src/main/java/dev/onvoid/webrtc/RTCPeerConnection.java

@@ -507,6 +507,11 @@ public native void getStats(RTCRtpSender sender,
 	/**
 	 * Closes the peer connection, terminates all media and releases any used
 	 * resources.
+	 * <p>
+	 * When called from an observer callback of a connection created by the
+	 * same factory, the connection is closed once that callback has returned,
+	 * since WebRTC goes on with it afterwards. Until then, the {@link
+	 * PeerConnectionObserver} may still hear of the operation in progress.
 	 */
 	public native void close();
 

webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpReceiver.java

@@ -86,9 +86,12 @@ private RTCRtpReceiver() {
 	 * the form of RTP packets. When bundling is used, multiple RTCRtpReceivers
 	 * will share one transport and will all receive RTP and RTCP over the same
 	 * transport.
+	 * <p>
+	 * Each call returns a new instance holding its own reference to the
+	 * transport, to be released with {@link RTCDtlsTransport#dispose()}.
 	 *
 	 * @return The transport over which media for the receiver's track is
-	 * received.
+	 * received, or null if the receiver has none yet.
 	 */
 	public native RTCDtlsTransport getTransport();
 

webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpSender.java

@@ -89,8 +89,12 @@ private RTCRtpSender() {
 	 * form of RTP packets. When bundling is used, multiple RTCRtpSenders will
 	 * share one transport and will all send RTP and RTCP over the same
 	 * transport.
+	 * <p>
+	 * Each call returns a new instance holding its own reference to the
+	 * transport, to be released with {@link RTCDtlsTransport#dispose()}.
 	 *
-	 * @return The transport over which media from the MediaStreamTrack is sent.
+	 * @return The transport over which media from the MediaStreamTrack is sent,
+	 * or null if the sender has none yet.
 	 */
 	public native RTCDtlsTransport getTransport();