From d851168a462373d9673dde7f9c6ea4f8ccc0d539 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Apr 2026 23:55:16 +0000 Subject: [PATCH] fix: WebRTC call resource leaks, thread safety, and error handling - Send hangup/reject to peer when WebRTC init or PeerConnection creation fails, so remote phone stops ringing instead of timing out after 60s - Throw on null PeerConnection from factory to fail fast instead of silently no-oping all subsequent WebRTC operations - Start foreground service during IncomingCall to protect ringtone playback from being killed on Android 14+ - Make cleanup() idempotent with AtomicBoolean guard to prevent double disposal when Ended state and ViewModel.onCleared race - Replace mutableMapOf with ConcurrentHashMap for videoSenders accessed from UI and WebRTC callback threads - Add @Volatile to peerConnection, videoPausedByProximity, and foregroundServiceStarted for cross-thread visibility - Capture peerConnection into local variable in dispose() to prevent TOCTOU race between close() and null assignment - Replace leaked MainScope() in CallNotificationReceiver with structured CoroutineScope that is cancelled after work completes - Remove self-wraps in group answer/reject to avoid wasting bandwidth sending encrypted messages to ourselves - Move startTimeout inside stateMutex in initiateCall for consistency https://claude.ai/code/session_017HrFJNxD6zrGwiZ3s69xTh --- .../amethyst/service/call/CallController.kt | 16 +++++++++++++--- .../service/call/CallNotificationReceiver.kt | 7 ++++++- .../amethyst/service/call/WebRtcCallSession.kt | 15 +++++++++------ .../amethyst/commons/call/CallManager.kt | 13 +++++++------ 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt index ae2e05d4c..d0d6b08b7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallController.kt @@ -53,6 +53,8 @@ import org.webrtc.EglBase import org.webrtc.IceCandidate import org.webrtc.VideoTrack import java.util.UUID +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicBoolean private const val TAG = "CallController" private const val VIDEO_MAX_BITRATE_BPS_DEFAULT = 1_500_000 @@ -95,9 +97,11 @@ class CallController( val audioRoute: StateFlow = audioManager.audioRoute val isBluetoothAvailable: StateFlow = audioManager.isBluetoothAvailable - private var videoPausedByProximity = false - private var foregroundServiceStarted = false - private val videoSenders = mutableMapOf() + @Volatile private var videoPausedByProximity = false + + @Volatile private var foregroundServiceStarted = false + private val cleanedUp = AtomicBoolean(false) + private val videoSenders = ConcurrentHashMap() private val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager private var networkCallbackRegistered = false @@ -126,6 +130,7 @@ class CallController( callManager.state.collect { state -> when (state) { is CallState.IncomingCall -> { + ensureForegroundService() withContext(Dispatchers.IO) { audioManager.startRinging() } scope.launch { NotificationUtils.showIncomingCallNotification(state.callerPubKey, context) @@ -209,6 +214,7 @@ class CallController( } catch (e: Exception) { Log.e(TAG, "Failed to initialize WebRTC", e) _errorMessage.value = "Failed to start call: ${e.message}" + callManager.hangup() return@launch } @@ -247,6 +253,7 @@ class CallController( } catch (e: Exception) { Log.e(TAG, "Failed to initialize WebRTC", e) _errorMessage.value = "Failed to accept call: ${e.message}" + callManager.rejectCall() return@launch } @@ -256,6 +263,7 @@ class CallController( } catch (e: Exception) { Log.e(TAG, "Failed to create PeerConnection", e) _errorMessage.value = "Failed to accept call: ${e.message}" + callManager.rejectCall() return@launch } @@ -582,6 +590,7 @@ class CallController( // ---- Cleanup ---- fun cleanup() { + if (!cleanedUp.compareAndSet(false, true)) return unregisterNetworkCallback() try { audioManager.release() @@ -610,6 +619,7 @@ class CallController( _isAudioMuted.value = false videoPausedByProximity = false videoSenders.clear() + cleanedUp.set(false) } // ---- Foreground service ---- diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallNotificationReceiver.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallNotificationReceiver.kt index 1add3bce0..a9b0321c0 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallNotificationReceiver.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/CallNotificationReceiver.kt @@ -24,6 +24,9 @@ import android.content.BroadcastReceiver import android.content.Context import android.content.Intent import com.vitorpamplona.amethyst.service.notifications.NotificationUtils +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.launch /** @@ -40,7 +43,8 @@ class CallNotificationReceiver : BroadcastReceiver() { ) { val callManager = CallSessionBridge.callManager ?: return val pendingResult = goAsync() - kotlinx.coroutines.MainScope().launch { + val scope = CoroutineScope(SupervisorJob() + kotlinx.coroutines.Dispatchers.Main.immediate) + scope.launch { try { when (intent.action) { ACTION_REJECT_CALL -> { @@ -54,6 +58,7 @@ class CallNotificationReceiver : BroadcastReceiver() { } } finally { pendingResult.finish() + scope.cancel() } } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/WebRtcCallSession.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/WebRtcCallSession.kt index cd164e2c0..b262114f5 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/WebRtcCallSession.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/call/WebRtcCallSession.kt @@ -55,8 +55,9 @@ class WebRtcCallSession( private val onRenegotiationNeeded: () -> Unit = {}, private val onIceRestartOffer: (SessionDescription) -> Unit = {}, ) { - private var peerConnection: PeerConnection? = null - private var iceRestartAttempted = false + @Volatile private var peerConnection: PeerConnection? = null + + @Volatile private var iceRestartAttempted = false fun createPeerConnection() { val rtcConfig = @@ -65,7 +66,7 @@ class WebRtcCallSession( continualGatheringPolicy = PeerConnection.ContinualGatheringPolicy.GATHER_CONTINUALLY } - peerConnection = + val pc = peerConnectionFactory.createPeerConnection( rtcConfig, object : PeerConnection.Observer { @@ -155,7 +156,8 @@ class WebRtcCallSession( } } }, - ) + ) ?: throw IllegalStateException("PeerConnectionFactory.createPeerConnection returned null") + peerConnection = pc } /** @@ -345,9 +347,10 @@ class WebRtcCallSession( } fun dispose() { - peerConnection?.close() - peerConnection?.dispose() + val pc = peerConnection ?: return peerConnection = null + pc.close() + pc.dispose() } private fun loggingSdpObserver(label: String) = diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/call/CallManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/call/CallManager.kt index 92db33a3b..5b6ab8758 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/call/CallManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/call/CallManager.kt @@ -201,9 +201,9 @@ class CallManager( val result = factory.createCallOffer(sdpOffer, calleePubKey, callId, callType, signer) stateMutex.withLock { _state.value = CallState.Offering(callId, setOf(calleePubKey), callType) + startTimeout(callId) } publishEvent(result.wrap) - startTimeout(callId) Log.d("CallManager") { "initiateCall: offer published, timeout started" } } @@ -281,9 +281,10 @@ class CallManager( discoveredCalleePeers.clear() } - val allRecipients = current.groupMembers + signer.pubKey - Log.d("CallManager") { "acceptCall: publishing answer to ${allRecipients.size} recipients" } - val result = factory.createGroupCallAnswer(sdpAnswer, allRecipients, current.callId, signer) + val allMembers = current.groupMembers + signer.pubKey + val otherMembers = allMembers - signer.pubKey + Log.d("CallManager") { "acceptCall: publishing answer to ${otherMembers.size} recipients" } + val result = factory.createGroupCallAnswer(sdpAnswer, otherMembers, current.callId, signer) result.wraps.forEach { publishEvent(it) } Log.d("CallManager") { "acceptCall: answer published, now in Connecting state" } @@ -306,8 +307,8 @@ class CallManager( transitionToEnded(current.callId, current.peerPubKeys(), EndReason.REJECTED) } - val allRecipients = current.groupMembers + signer.pubKey - val result = factory.createGroupReject(allRecipients, current.callId, signer = signer) + val otherMembers = current.groupMembers - signer.pubKey + val result = factory.createGroupReject(otherMembers, current.callId, signer = signer) result.wraps.forEach { publishEvent(it) } }