fix: WebRTC race conditions, thread safety, and PiP lifecycle

- CallActivity.onStop: move finishAndRemoveTask inside coroutine so
  hangup signaling completes before Activity destruction; add
  hangupInitiated flag to prevent double-hangup in onDestroy
- CallController: add per-peer renegotiation debouncing via
  pendingRenegotiation map to prevent queuing multiple createOffer calls
  when video is toggled rapidly
- CallController: guard ensureForegroundService in onPeerConnected
  callback with state check to prevent restarting the service after
  cleanup
- RemoteVideoMonitor: synchronize onRemoteVideoTrack, onPeerRemoved,
  and dispose with trackLock to prevent non-atomic map mutations and
  leaked video sinks from concurrent WebRTC callback threads
- CallMediaManager: add @Synchronized to createVideoResources to
  prevent check-then-act race between IO and main threads

https://claude.ai/code/session_017HrFJNxD6zrGwiZ3s69xTh
This commit is contained in:
Claude
2026-04-11 00:48:27 +00:00
parent deecce6e77
commit c82ba766f8
4 changed files with 76 additions and 47 deletions
@@ -103,6 +103,8 @@ class CallController(
private val cleanedUp = AtomicBoolean(false) private val cleanedUp = AtomicBoolean(false)
private val videoSenders = ConcurrentHashMap<HexKey, org.webrtc.RtpSender>() private val videoSenders = ConcurrentHashMap<HexKey, org.webrtc.RtpSender>()
private val pendingRenegotiation = ConcurrentHashMap<HexKey, Boolean>()
private val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager private val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager
private var networkCallbackRegistered = false private var networkCallbackRegistered = false
private val networkCallback = private val networkCallback =
@@ -409,10 +411,19 @@ class CallController(
} }
private fun performRenegotiation(peerPubKey: HexKey) { private fun performRenegotiation(peerPubKey: HexKey) {
val webRtcSession = webRtcSession(peerPubKey) ?: return if (pendingRenegotiation.putIfAbsent(peerPubKey, true) != null) return
val webRtcSession =
webRtcSession(peerPubKey) ?: run {
pendingRenegotiation.remove(peerPubKey)
return
}
val state = callManager.state.value val state = callManager.state.value
if (state !is CallState.Connected && state !is CallState.Connecting) return if (state !is CallState.Connected && state !is CallState.Connecting) {
pendingRenegotiation.remove(peerPubKey)
return
}
webRtcSession.createOffer { sdp -> webRtcSession.createOffer { sdp ->
pendingRenegotiation.remove(peerPubKey)
scope.launch { callManager.sendRenegotiation(sdp.description, peerPubKey) } scope.launch { callManager.sendRenegotiation(sdp.description, peerPubKey) }
} }
} }
@@ -540,8 +551,10 @@ class CallController(
Log.d(TAG) { "Peer ${peerPubKey.take(8)} connected!" } Log.d(TAG) { "Peer ${peerPubKey.take(8)} connected!" }
scope.launch { scope.launch {
callManager.onPeerConnected() callManager.onPeerConnected()
if (callManager.state.value is CallState.Connected) {
ensureForegroundService() ensureForegroundService()
} }
}
}, },
onRemoteVideoTrack = { track -> videoMonitor.onRemoteVideoTrack(peerPubKey, track) }, onRemoteVideoTrack = { track -> videoMonitor.onRemoteVideoTrack(peerPubKey, track) },
onDisconnected = { scope.launch { onPeerDisconnected(peerPubKey) } }, onDisconnected = { scope.launch { onPeerDisconnected(peerPubKey) } },
@@ -627,6 +640,7 @@ class CallController(
_isAudioMuted.value = false _isAudioMuted.value = false
videoPausedByProximity = false videoPausedByProximity = false
videoSenders.clear() videoSenders.clear()
pendingRenegotiation.clear()
cleanedUp.set(false) cleanedUp.set(false)
} }
@@ -117,6 +117,7 @@ class CallMediaManager(
} }
} }
@Synchronized
fun createVideoResources() { fun createVideoResources() {
if (localVideoSource != null) return if (localVideoSource != null) return
val factory = peerConnectionFactory ?: return val factory = peerConnectionFactory ?: return
@@ -85,21 +85,28 @@ class RemoteVideoMonitor(
private val perPeerLastFrameTimeMs = ConcurrentHashMap<HexKey, AtomicLong>() private val perPeerLastFrameTimeMs = ConcurrentHashMap<HexKey, AtomicLong>()
private var groupVideoMonitorJob: Job? = null private var groupVideoMonitorJob: Job? = null
/** Protects compound read-modify-write on [_remoteVideoTracks] and
* [_remoteVideoTrack] which can be called from WebRTC callback threads. */
private val trackLock = Any()
fun onRemoteVideoTrack( fun onRemoteVideoTrack(
peerPubKey: HexKey, peerPubKey: HexKey,
track: VideoTrack, track: VideoTrack,
) { ) {
Log.d(TAG) { "Remote video track from ${peerPubKey.take(8)}" } Log.d(TAG) { "Remote video track from ${peerPubKey.take(8)}" }
synchronized(trackLock) {
_remoteVideoTracks.value = _remoteVideoTracks.value + (peerPubKey to track) _remoteVideoTracks.value = _remoteVideoTracks.value + (peerPubKey to track)
if (_remoteVideoTrack.value == null) { if (_remoteVideoTrack.value == null) {
_remoteVideoTrack.value = track _remoteVideoTrack.value = track
startPrimaryMonitor(track) startPrimaryMonitor(track)
} }
}
startPeerMonitor(peerPubKey, track) startPeerMonitor(peerPubKey, track)
} }
fun onPeerRemoved(peerPubKey: HexKey) { fun onPeerRemoved(peerPubKey: HexKey) {
stopPeerMonitor(peerPubKey) stopPeerMonitor(peerPubKey)
synchronized(trackLock) {
val currentTracks = _remoteVideoTracks.value val currentTracks = _remoteVideoTracks.value
if (peerPubKey in currentTracks) { if (peerPubKey in currentTracks) {
_remoteVideoTracks.value = currentTracks - peerPubKey _remoteVideoTracks.value = currentTracks - peerPubKey
@@ -113,8 +120,10 @@ class RemoteVideoMonitor(
} }
} }
} }
}
fun dispose() { fun dispose() {
synchronized(trackLock) {
stopPrimaryMonitor() stopPrimaryMonitor()
stopGroupMonitor() stopGroupMonitor()
for (peerPubKey in perPeerFrameSinks.keys.toList()) { for (peerPubKey in perPeerFrameSinks.keys.toList()) {
@@ -126,6 +135,7 @@ class RemoteVideoMonitor(
_remoteVideoAspectRatio.value = null _remoteVideoAspectRatio.value = null
_activePeerVideos.value = emptySet() _activePeerVideos.value = emptySet()
} }
}
private fun startPrimaryMonitor(track: VideoTrack) { private fun startPrimaryMonitor(track: VideoTrack) {
stopPrimaryMonitor() stopPrimaryMonitor()
@@ -48,10 +48,8 @@ import com.vitorpamplona.amethyst.ui.screen.ManageWebOkHttp
import com.vitorpamplona.amethyst.ui.theme.AmethystTheme import com.vitorpamplona.amethyst.ui.theme.AmethystTheme
import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.NonCancellable
import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.launch import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
class CallActivity : AppCompatActivity() { class CallActivity : AppCompatActivity() {
val isInPipMode = mutableStateOf(false) val isInPipMode = mutableStateOf(false)
@@ -69,6 +67,7 @@ class CallActivity : AppCompatActivity() {
} }
private var pendingAcceptIsVideo = false private var pendingAcceptIsVideo = false
private var hangupInitiated = false
private val pipActionReceiver = private val pipActionReceiver =
object : BroadcastReceiver() { object : BroadcastReceiver() {
@@ -201,14 +200,18 @@ class CallActivity : AppCompatActivity() {
// We must NOT hang up when the user simply presses Home from the full-screen // We must NOT hang up when the user simply presses Home from the full-screen
// call UI (that enters PiP via onUserLeaveHint instead). // call UI (that enters PiP via onUserLeaveHint instead).
if (wasInPipMode && !isInPictureInPictureMode) { if (wasInPipMode && !isInPictureInPictureMode) {
val state = CallSessionBridge.callManager?.state?.value hangupInitiated = true
val manager = CallSessionBridge.callManager
val state = manager?.state?.value
if (state is CallState.Connected || state is CallState.Connecting || state is CallState.Offering) { if (state is CallState.Connected || state is CallState.Connecting || state is CallState.Offering) {
lifecycleScope.launch { CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate).launch {
withContext(NonCancellable) { CallSessionBridge.callManager?.hangup() } manager.hangup()
}
}
finishAndRemoveTask() finishAndRemoveTask()
} }
} else {
finishAndRemoveTask()
}
}
} }
override fun onDestroy() { override fun onDestroy() {
@@ -216,8 +219,8 @@ class CallActivity : AppCompatActivity() {
// Safety net: if the Activity is destroyed while a call is still // Safety net: if the Activity is destroyed while a call is still
// ringing/offering, ensure the call is hung up so audio stops. // ringing/offering, ensure the call is hung up so audio stops.
// Use a standalone CoroutineScope because lifecycleScope is cancelled // Skip if onStop already initiated the hangup to avoid double signaling.
// during super.onDestroy() and may drop suspend work. if (!hangupInitiated) {
val manager = CallSessionBridge.callManager val manager = CallSessionBridge.callManager
when (manager?.state?.value) { when (manager?.state?.value) {
is CallState.IncomingCall -> { is CallState.IncomingCall -> {
@@ -237,6 +240,7 @@ class CallActivity : AppCompatActivity() {
else -> {} else -> {}
} }
}
super.onDestroy() super.onDestroy()
} }