fix: keep audio device change handling from aborting or corrupting state

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

@@ -15,9 +15,11 @@
 #include <jni.h>
 #include <list>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <unordered_map>
 #include <utility>
+#include <vector>
 
 namespace jni
 {
@@ -39,39 +41,81 @@ namespace jni
 
 			JavaVM * getVM();
 
-			void addNativeRef(JNIEnv * env, const JavaLocalRef<jobject> & javaRef, const std::shared_ptr<void> & nativeRef);
+			// Keeps a native object (a listener's native peer) alive for as long as
+			// the Java object stays registered. An entry is found again by the Java
+			// object together with its owner and kind: the same Java listener can be
+			// registered with several owners (tracks), or as several kinds of
+			// listener with one owner, and each registration has its own native
+			// object.
+			void addNativeRef(JNIEnv * env, const JavaLocalRef<jobject> & javaRef, const std::shared_ptr<void> & nativeRef, const void * owner = nullptr, int kind = 0);
 
 			template<typename T>
-			std::shared_ptr<T> removeNativeRef(JNIEnv * env, const JavaLocalRef<jobject> & javaRef)
+			std::shared_ptr<T> removeNativeRef(JNIEnv * env, const JavaLocalRef<jobject> & javaRef, const void * owner = nullptr, int kind = 0)
 			{
 				auto className = JavaClassUtils::toNativeClassName(env, javaRef);
+
+				std::lock_guard<std::mutex> lock(objectMapMutex);
+
 				auto it = objectMap.find(className);
 
 				if (it == objectMap.end()) {
 					return nullptr;
 				}
 
-				auto globalRef = JavaGlobalRef<jobject>(env, javaRef.get());
 				auto & list = it->second;
 				std::shared_ptr<T> nativeRef = nullptr;
 
 				for (auto it = list.begin(); it != list.end(); ++it) {
-					if (env->IsSameObject(it->first, globalRef)) {
-						nativeRef = std::static_pointer_cast<T>(it->second);
+					if (it->owner == owner && it->kind == kind && env->IsSameObject(it->javaRef, javaRef)) {
+						nativeRef = std::static_pointer_cast<T>(it->nativeRef);
 						list.erase(it);
 						break;
 					}
 				}
 
-				env->DeleteGlobalRef(globalRef);
-
 				return nativeRef;
 			}
 
+			// Removes every entry of the owner, of all kinds, which must all hold a
+			// native object of type T.
+			template<typename T>
+			std::vector<std::shared_ptr<T>> removeNativeRefs(const void * owner)
+			{
+				std::vector<std::shared_ptr<T>> nativeRefs;
+
+				std::lock_guard<std::mutex> lock(objectMapMutex);
+
+				for (auto & entry : objectMap) {
+					auto & list = entry.second;
+
+					for (auto it = list.begin(); it != list.end();) {
+						if (it->owner == owner) {
+							nativeRefs.push_back(std::static_pointer_cast<T>(it->nativeRef));
+							it = list.erase(it);
+						}
+						else {
+							++it;
+						}
+					}
+				}
+
+				return nativeRefs;
+			}
+
 		private:
+			struct NativeRef
+			{
+				JavaGlobalRef<jobject> javaRef;
+				const void * owner;
+				int kind;
+				std::shared_ptr<void> nativeRef;
+			};
+
 			JavaVM * vm;
-			// Java object class name mapped to a Java object reference and its native object reference.
-			std::unordered_map<std::string, std::list<std::pair<JavaGlobalRef<jobject>, std::shared_ptr<void>>>> objectMap;
+			// Java object class name mapped to the registrations of Java objects of that class.
+			std::unordered_map<std::string, std::list<NativeRef>> objectMap;
+			// Listeners are added and removed on any Java thread.
+			std::mutex objectMapMutex;
 	};
 }
 

webrtc-jni/src/main/cpp/dependencies/jni-voithos/src/JavaContext.cpp

@@ -19,20 +19,13 @@ namespace jni
 		return vm;
 	}
 
-	void JavaContext::addNativeRef(JNIEnv * env, const JavaLocalRef<jobject> & javaRef, const std::shared_ptr<void> & nativeRef)
+	void JavaContext::addNativeRef(JNIEnv * env, const JavaLocalRef<jobject> & javaRef, const std::shared_ptr<void> & nativeRef, const void * owner, int kind)
 	{
 		auto className = JavaClassUtils::toNativeClassName(env, javaRef);
-		auto globalRef = JavaGlobalRef<jobject>(env, javaRef.get());
-		auto pair = std::make_pair(globalRef, nativeRef);
-		auto it = objectMap.find(className);
+		NativeRef entry = { JavaGlobalRef<jobject>(env, javaRef.get()), owner, kind, nativeRef };
 
-		if (it == objectMap.end()) {
-			std::list<std::pair<JavaGlobalRef<jobject>, std::shared_ptr<void>>> list = { pair };
+		std::lock_guard<std::mutex> lock(objectMapMutex);
 
-			objectMap[className] = list;
-		}
-		else {
-			it->second.push_back(pair);
-		}
+		objectMap[className].push_back(std::move(entry));
 	}
 }

webrtc-jni/src/main/cpp/include/media/DeviceChangeListener.h

@@ -38,6 +38,9 @@ namespace jni
 			void deviceConnected(avdev::DevicePtr device) override;
 			void deviceDisconnected(avdev::DevicePtr device) override;
 
+		private:
+			void notify(const avdev::DevicePtr & device, jmethodID method);
+
 		private:
 			class JavaDeviceChangeListenerClass : public JavaClass
 			{

webrtc-jni/src/main/cpp/include/media/DeviceManager.h

@@ -19,6 +19,7 @@
 
 #include <list>
 #include <memory>
+#include <mutex>
 
 #include "media/DeviceHotplugListener.h"
 
@@ -48,6 +49,7 @@ namespace jni
 				void notifyListeners(DevicePtr device, const DeviceEvent event);
 
 				std::list<std::weak_ptr<DeviceHotplugListener>> hotplugListeners;
+				std::mutex listenerMutex;
 		};
 	}
 }

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

@@ -68,6 +68,10 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_media_MediaDevices_getDefaultAu
 	try {
 	    auto device = context->getAudioDeviceManager()->getDefaultAudioPlaybackDevice();
 
+		if (device == nullptr) {
+			return nullptr;
+		}
+
 		return jni::AudioDevice::toJavaAudioDevice(env, device).release();
 	}
 	catch (...) {
@@ -85,6 +89,10 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_media_MediaDevices_getDefaultAu
 	try {
 	    auto device = context->getAudioDeviceManager()->getDefaultAudioCaptureDevice();
 
+		if (device == nullptr) {
+			return nullptr;
+		}
+
 		return jni::AudioDevice::toJavaAudioDevice(env, device).release();
 	}
 	catch (...) {

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

@@ -33,6 +33,14 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_MediaStreamTrack_dispose
 	webrtc::MediaStreamTrackInterface * track = GetHandle<webrtc::MediaStreamTrackInterface>(env, caller);
 	CHECK_HANDLE(track);
 
+	// Listeners still registered are taken off the track before it goes. Their
+	// entries are keyed by this track's address, which a later track can be
+	// given, and an observer left registered would be freed with its entry
+	// while the track (if something else still holds it) can still call it.
+	for (const auto & observer : javaContext->removeNativeRefs<jni::MediaStreamTrackObserver>(track)) {
+		track->UnregisterObserver(observer.get());
+	}
+
 	webrtc::RefCountReleaseStatus status = track->Release();
 
 	// Our reference is gone the moment Release() returns, regardless of
@@ -104,7 +112,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_MediaStreamTrack_addEndedEve
 
 		track->RegisterObserver(observer);
 
-		javaContext->addNativeRef(env, jni::JavaLocalRef<jobject>(env, jListener), observerPtr);
+		javaContext->addNativeRef(env, jni::JavaLocalRef<jobject>(env, jListener), observerPtr, track, static_cast<int>(jni::MediaStreamTrackEvent::ended));
 	}
 	catch (...) {
 		ThrowCxxJavaException(env);
@@ -118,7 +126,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_MediaStreamTrack_removeEnded
 	CHECK_HANDLE(track);
 
 	try {
-		auto observerPtr = javaContext->removeNativeRef<jni::MediaStreamTrackObserver>(env, jni::JavaLocalRef<jobject>(env, jListener));
+		auto observerPtr = javaContext->removeNativeRef<jni::MediaStreamTrackObserver>(env, jni::JavaLocalRef<jobject>(env, jListener), track, static_cast<int>(jni::MediaStreamTrackEvent::ended));
 
 		if (observerPtr) {
 			track->UnregisterObserver(observerPtr.get());
@@ -141,7 +149,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_MediaStreamTrack_addMuteEven
 
 		track->RegisterObserver(observer);
 
-		javaContext->addNativeRef(env, jni::JavaLocalRef<jobject>(env, jListener), observerPtr);
+		javaContext->addNativeRef(env, jni::JavaLocalRef<jobject>(env, jListener), observerPtr, track, static_cast<int>(jni::MediaStreamTrackEvent::mute));
 	}
 	catch (...) {
 		ThrowCxxJavaException(env);
@@ -155,7 +163,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_media_MediaStreamTrack_removeMuteE
 	CHECK_HANDLE(track);
 
 	try {
-		auto observerPtr = javaContext->removeNativeRef<jni::MediaStreamTrackObserver>(env, jni::JavaLocalRef<jobject>(env, jListener));
+		auto observerPtr = javaContext->removeNativeRef<jni::MediaStreamTrackObserver>(env, jni::JavaLocalRef<jobject>(env, jListener), track, static_cast<int>(jni::MediaStreamTrackEvent::mute));
 
 		if (observerPtr) {
 			track->UnregisterObserver(observerPtr.get());

webrtc-jni/src/main/cpp/src/media/DeviceChangeListener.cpp

@@ -22,6 +22,10 @@
 #include "JavaUtils.h"
 #include "JNI_WebRTC.h"
 
+#include "rtc_base/logging.h"
+
+#include <exception>
+
 namespace jni
 {
 	DeviceChangeListener::DeviceChangeListener(JNIEnv * env, const JavaGlobalRef<jobject> & listener) :
@@ -32,47 +36,52 @@ namespace jni
 
 	void DeviceChangeListener::deviceConnected(avdev::DevicePtr device)
 	{
-		JNIEnv * env = AttachCurrentThread();
-
-		if (env == nullptr) {
-			return;
-		}
-
-		JavaLocalRef<jobject> jdevice = nullptr;
-
-		if (dynamic_cast<jni::avdev::AudioDevice *>(device.get())) {
-			jdevice = AudioDevice::toJavaAudioDevice(env, device);
-		}
-		else if (dynamic_cast<jni::avdev::VideoDevice *>(device.get())) {
-			const auto dev = dynamic_cast<jni::avdev::VideoDevice*>(device.get());
-			jdevice = VideoDevice::toJavaVideoDevice(env, *dev);
-		}
-
-		if (jdevice) {
-			env->CallVoidMethod(listener, javaClass->deviceConnected, jdevice.get());
-		}
+		notify(device, javaClass->deviceConnected);
 	}
 
 	void DeviceChangeListener::deviceDisconnected(avdev::DevicePtr device)
+	{
+		notify(device, javaClass->deviceDisconnected);
+	}
+
+	void DeviceChangeListener::notify(const avdev::DevicePtr & device, jmethodID method)
 	{
 		JNIEnv * env = AttachCurrentThread();
 
 		if (env == nullptr) {
 			return;
 		}
 
-		JavaLocalRef<jobject> jdevice = nullptr;
+		// This runs on a thread the operating system owns (a CoreAudio queue, the MMDevice
+		// notification thread, the PulseAudio main loop, ...), which has no handler for a C++
+		// exception and never returns to Java. So nothing may be thrown from here, and what the
+		// Java listener throws is reported and cleared: left pending, it would be there for the
+		// next JNI call on this thread, which may be the next listener or the next event.
+		try {
+			JavaLocalRef<jobject> jdevice = nullptr;
 
-		if (dynamic_cast<jni::avdev::AudioDevice *>(device.get())) {
-			jdevice = AudioDevice::toJavaAudioDevice(env, device);
+			if (dynamic_cast<jni::avdev::AudioDevice *>(device.get())) {
+				jdevice = AudioDevice::toJavaAudioDevice(env, device);
+			}
+			else if (dynamic_cast<jni::avdev::VideoDevice *>(device.get())) {
+				const auto dev = dynamic_cast<jni::avdev::VideoDevice *>(device.get());
+				jdevice = VideoDevice::toJavaVideoDevice(env, *dev);
+			}
+
+			if (jdevice) {
+				env->CallVoidMethod(listener, method, jdevice.get());
+			}
+		}
+		catch (const std::exception & e) {
+			RTC_LOG(LS_ERROR) << "Device change notification failed: " << e.what();
 		}
-		else if (dynamic_cast<jni::avdev::VideoDevice *>(device.get())) {
-			const auto dev = dynamic_cast<jni::avdev::VideoDevice *>(device.get());
-			jdevice = VideoDevice::toJavaVideoDevice(env, *dev);
+		catch (...) {
+			RTC_LOG(LS_ERROR) << "Device change notification failed";
 		}
 
-		if (jdevice) {
-			env->CallVoidMethod(listener, javaClass->deviceDisconnected, jdevice.get());
+		if (env->ExceptionCheck()) {
+			env->ExceptionDescribe();
+			env->ExceptionClear();
 		}
 	}
 

webrtc-jni/src/main/cpp/src/media/DeviceManager.cpp

@@ -16,17 +16,23 @@
 
 #include "media/DeviceManager.h"
 
+#include <vector>
+
 namespace jni
 {
 	namespace avdev
 	{
 		void DeviceManager::attachHotplugListener(PDeviceHotplugListener listener)
 		{
+			std::lock_guard<std::mutex> lock(listenerMutex);
+
 			hotplugListeners.push_back(listener);
 		}
 
 		void DeviceManager::detachHotplugListener(PDeviceHotplugListener listener)
 		{
+			std::lock_guard<std::mutex> lock(listenerMutex);
+
 			hotplugListeners.remove_if([listener](std::weak_ptr<DeviceHotplugListener> p) {
 				return !(p.owner_before(listener) || listener.owner_before(p));
 			});
@@ -44,21 +50,33 @@ namespace jni
 
 		void DeviceManager::notifyListeners(DevicePtr device, const DeviceEvent event)
 		{
-			for (auto i = hotplugListeners.begin(); i != hotplugListeners.end();) {
-				if ((*i).expired()) {
-					i = hotplugListeners.erase(i);
-				}
-				else {
+			// Listeners are attached and detached on Java threads while the operating system
+			// reports device changes on its own. The listeners are called on a copy, outside the
+			// lock, so that one may detach itself (or another) from within its callback.
+			std::vector<PDeviceHotplugListener> listeners;
+
+			{
+				std::lock_guard<std::mutex> lock(listenerMutex);
+
+				for (auto i = hotplugListeners.begin(); i != hotplugListeners.end();) {
 					PDeviceHotplugListener listener = (*i).lock();
 
-					if (event == DeviceEvent::Connected) {
-						listener->deviceConnected(device);
+					if (listener) {
+						listeners.push_back(listener);
+						++i;
 					}
-					else if (event == DeviceEvent::Disconnected) {
-						listener->deviceDisconnected(device);
+					else {
+						i = hotplugListeners.erase(i);
 					}
+				}
+			}
 
-					++i;
+			for (const PDeviceHotplugListener & listener : listeners) {
+				if (event == DeviceEvent::Connected) {
+					listener->deviceConnected(device);
+				}
+				else if (event == DeviceEvent::Disconnected) {
+					listener->deviceDisconnected(device);
 				}
 			}
 		}