diff --git a/.changeset/screen-capture-connection-bind-leak.md b/.changeset/screen-capture-connection-bind-leak.md new file mode 100644 index 000000000..b3de0eb7e --- /dev/null +++ b/.changeset/screen-capture-connection-bind-leak.md @@ -0,0 +1,7 @@ +--- +"client-sdk-android": patch +--- + +Fix `ScreenCaptureService` staying bound when screen share setup is cancelled. `ScreenCaptureConnection` recorded a binding only once `onServiceConnected` arrived, so a coroutine cancelled before `connect()` returned left the `ServiceConnection` registered, and `BIND_AUTO_CREATE` kept the service alive for the lifetime of the context. `LocalParticipant.setScreenShareEnabled` awaits the bind internally and abandons the track it just created if cancelled, so nothing reached `stop()` on that path. A cancelled connect now releases the binding itself once no caller is left waiting on it, and `stop()` unbinds on the same wider condition. Callers stay tracked until `connect()` actually returns, so a cancellation landing after the service connected but before the caller resumed releases the binding too. This also covers the documented case where `bindService` leaves a connection registered while reporting failure or throwing. + +Two paths that could leave `connect()` suspended forever are fixed as well. `stop()` racing a connect no longer strands the caller, since requesting the bind and registering the waiter now happen under one lock, and a failed `bindService` no longer leaves the state claiming a bind is in flight for the next caller to wait on. diff --git a/livekit-android-sdk/src/main/java/io/livekit/android/room/track/screencapture/ScreenCaptureConnection.kt b/livekit-android-sdk/src/main/java/io/livekit/android/room/track/screencapture/ScreenCaptureConnection.kt index 19c5225e2..f4bbfa1a3 100644 --- a/livekit-android-sdk/src/main/java/io/livekit/android/room/track/screencapture/ScreenCaptureConnection.kt +++ b/livekit-android-sdk/src/main/java/io/livekit/android/room/track/screencapture/ScreenCaptureConnection.kt @@ -1,5 +1,5 @@ /* - * Copyright 2023-2025 LiveKit, Inc. + * Copyright 2023-2026 LiveKit, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -24,76 +24,194 @@ import android.content.Intent import android.content.ServiceConnection import android.os.IBinder import io.livekit.android.util.LKLog +import kotlinx.coroutines.CancellableContinuation +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.suspendCancellableCoroutine -import kotlin.coroutines.Continuation import kotlin.coroutines.resume +import kotlin.coroutines.resumeWithException /** * Handles connecting to a [ScreenCaptureService]. */ internal class ScreenCaptureConnection(private val context: Context) { + /** + * True while [ScreenCaptureService] is connected and [startForeground] can reach it. + */ var isBound = false private set + + /** + * True from the start of a bind attempt until its matching unbind attempt. This is a wider + * window than [isBound]: the binding is owed an unbind even if the service never connects. + */ + private var isBindRequested = false private var service: ScreenCaptureService? = null - private val queuedConnects = mutableSetOf>() + private var hasConnectedCaller = false + private val queuedConnects = mutableSetOf>() private val connection: ServiceConnection = object : ServiceConnection { override fun onServiceDisconnected(name: ComponentName) { LKLog.v { "Screen capture service is disconnected" } - isBound = false - service = null + synchronized(this@ScreenCaptureConnection) { + isBound = false + service = null + } } override fun onServiceConnected(name: ComponentName, binder: IBinder) { LKLog.v { "Screen capture service is connected" } val screenCaptureBinder = binder as ScreenCaptureService.ScreenCaptureBinder - service = screenCaptureBinder.service - handleConnect() + val connects = synchronized(this@ScreenCaptureConnection) { + if (!isBindRequested) { + return + } + service = screenCaptureBinder.service + isBound = true + queuedConnects.filter { it.isActive } + } + connects.forEach { it.resume(Unit) } } - } - suspend fun connect() { - if (isBound) { - return + override fun onBindingDied(name: ComponentName) { + LKLog.w { "Screen capture service binding died" } + failPendingConnects("ScreenCaptureService binding died.") } - val intent = Intent(context, ScreenCaptureService::class.java) - val bound = context.bindService(intent, connection, BIND_AUTO_CREATE) - if (!bound) { - throw IllegalStateException("Failed to bind ScreenCaptureService.") + override fun onNullBinding(name: ComponentName) { + LKLog.w { "Screen capture service returned a null binding" } + failPendingConnects("ScreenCaptureService returned a null binding.") } - return suspendCancellableCoroutine { cont -> - cont.invokeOnCancellation { - synchronized(this) { - queuedConnects.remove(cont) - } - } + } + + /** + * Binds to [ScreenCaptureService] and suspends until it is connected. + * + * @throws IllegalStateException if the service could not be bound, or the binding died or + * came back null before the service connected. + * @throws SecurityException if the caller cannot access the service. + * @throws CancellationException if [stop] tears the connection down while connecting. + */ + suspend fun connect() { + lateinit var continuation: CancellableContinuation + suspendCancellableCoroutine { cont -> + continuation = cont + // Binding and enqueueing happen under one lock, so a concurrent stop() either + // precedes the bind or cancels the waiter, and never strands it against a + // connection that has already been unbound. + var outcome: Result? = null synchronized(this) { if (isBound) { - cont.resume(Unit) + outcome = Result.success(Unit) } else { queuedConnects.add(cont) + val failure = runCatching { if (!isBindRequested) bind() }.exceptionOrNull() + if (failure != null) { + queuedConnects.remove(cont) + outcome = Result.failure(failure) + } } } + + val result = outcome + if (result == null) { + // Registered once the waiter is queued, so a continuation that was already + // cancelled still releases the binding it just requested. + cont.invokeOnCancellation { abandonConnect(cont) } + } else { + // Resumed outside the lock, since resuming runs the continuation inline. + cont.resumeWith(result) + } } - } - fun startForeground(notificationId: Int? = null, notification: Notification? = null) { - service?.start(notificationId, notification) + val connected = synchronized(this) { + queuedConnects.remove(continuation) + if (isBindRequested && isBound) { + hasConnectedCaller = true + true + } else { + if (queuedConnects.isEmpty() && !hasConnectedCaller) { + unbind() + } + false + } + } + if (!connected) { + throw CancellationException("ScreenCaptureService connection was stopped.") + } } - private fun handleConnect() { + /** + * Releases a binding that no longer has anyone waiting on it. Screen share setup is routinely + * driven from a cancellable scope, and the caller that requested the bind does not necessarily + * survive to call [stop]. + */ + private fun abandonConnect(cont: CancellableContinuation) { synchronized(this) { - isBound = true - queuedConnects.forEach { it.resume(Unit) } - queuedConnects.clear() + queuedConnects.remove(cont) + if (queuedConnects.isEmpty() && !hasConnectedCaller) { + unbind() + } } } - fun stop() { - if (isBound) { - context.unbindService(connection) + /** + * Releases a binding the platform has reported will never connect, and fails every caller + * waiting on it. Neither report is followed by [ServiceConnection.onServiceConnected], so a + * queued caller would otherwise stay suspended until [stop]. + */ + private fun failPendingConnects(message: String) { + val failedConnects = synchronized(this) { + if (!isBindRequested) { + return + } + unbind() + queuedConnects.toList().also { queuedConnects.clear() } } - service = null + // Resumed outside the lock, since resuming runs the continuation inline. + failedConnects.forEach { it.resumeWithException(IllegalStateException(message)) } + } + + private fun bind() { + val intent = Intent(context, ScreenCaptureService::class.java) + // A connection is registered even when bindService reports failure, so the unbind is + // owed from the moment the call is made. + isBindRequested = true + val bound = try { + context.bindService(intent, connection, BIND_AUTO_CREATE) + } catch (e: Exception) { + unbind() + throw e + } + if (!bound) { + unbind() + throw IllegalStateException("Failed to bind ScreenCaptureService.") + } + } + + private fun unbind() { + if (!isBindRequested) { + return + } + isBindRequested = false isBound = false + service = null + hasConnectedCaller = false + try { + context.unbindService(connection) + } catch (e: IllegalArgumentException) { + LKLog.v(e) { "Screen capture service was not bound" } + } + } + + fun startForeground(notificationId: Int? = null, notification: Notification? = null) { + service?.start(notificationId, notification) + } + + fun stop() { + val abandonedConnects = synchronized(this) { + unbind() + queuedConnects.toList().also { queuedConnects.clear() } + } + // Cancelled outside the lock, since cancellation runs handlers inline. + abandonedConnects.forEach { it.cancel() } } } diff --git a/livekit-android-test/src/test/java/io/livekit/android/room/track/screencapture/ScreenCaptureConnectionTest.kt b/livekit-android-test/src/test/java/io/livekit/android/room/track/screencapture/ScreenCaptureConnectionTest.kt new file mode 100644 index 000000000..36960a391 --- /dev/null +++ b/livekit-android-test/src/test/java/io/livekit/android/room/track/screencapture/ScreenCaptureConnectionTest.kt @@ -0,0 +1,191 @@ +/* + * Copyright 2025-2026 LiveKit, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.livekit.android.room.track.screencapture + +import android.app.Application +import android.content.ComponentName +import androidx.test.core.app.ApplicationProvider +import io.livekit.android.test.BaseTest +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.runCurrent +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +class ScreenCaptureConnectionTest : BaseTest() { + + private val application = ApplicationProvider.getApplicationContext() + + private fun connectService() { + shadowOf(application).boundServiceConnections.single().onServiceConnected( + ComponentName(application, ScreenCaptureService::class.java), + ScreenCaptureService().onBind(null), + ) + } + + private fun disconnectService() { + shadowOf(application).boundServiceConnections.single().onServiceDisconnected( + ComponentName(application, ScreenCaptureService::class.java), + ) + } + + @Test + fun cancellingConnectUnbindsBeforeServiceConnects() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + + val connectJob = launch { screenCaptureConnection.connect() } + assertEquals(1, shadowOf(application).boundServiceConnections.size) + + // The caller that requested the bind is not guaranteed to survive to call stop(). + connectJob.cancelAndJoin() + + assertEquals(1, shadowOf(application).unboundServiceConnections.size) + assertEquals(0, shadowOf(application).boundServiceConnections.size) + } + + @Test + fun connectRebindsAfterAnEarlierConnectWasCancelled() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + + launch { screenCaptureConnection.connect() }.cancelAndJoin() + assertEquals(0, shadowOf(application).boundServiceConnections.size) + + val connectJob = launch { screenCaptureConnection.connect() } + + // A cancelled connect must not leave the state claiming a bind is still in flight, + // which would leave this caller waiting on a connection that was already released. + assertEquals(1, shadowOf(application).boundServiceConnections.size) + + connectJob.cancelAndJoin() + } + + @Test + fun cancellingConnectAfterServiceConnectsUnbindsBeforeResumption() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + val connectJob = launch(StandardTestDispatcher(testScheduler)) { + screenCaptureConnection.connect() + } + runCurrent() + + connectService() + connectJob.cancel() + + runCurrent() + connectJob.join() + assertEquals(0, shadowOf(application).boundServiceConnections.size) + } + + @Test + fun connectKeepsServiceBoundOnceConnected() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + val connectJob = launch(StandardTestDispatcher(testScheduler)) { + screenCaptureConnection.connect() + } + runCurrent() + + connectService() + runCurrent() + connectJob.join() + + assertTrue(screenCaptureConnection.isBound) + assertEquals(1, shadowOf(application).boundServiceConnections.size) + } + + @Test + fun cancellingConnectKeepsBindingHeldByAnEarlierCaller() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + val firstConnect = launch(StandardTestDispatcher(testScheduler)) { + screenCaptureConnection.connect() + } + runCurrent() + connectService() + runCurrent() + firstConnect.join() + + // A dropped service connection leaves the binding in place, so a second caller waits on + // it rather than binding again. + disconnectService() + val secondConnect = launch(StandardTestDispatcher(testScheduler)) { + screenCaptureConnection.connect() + } + runCurrent() + secondConnect.cancelAndJoin() + + // Releasing on cancellation must not tear down a binding the first caller still holds. + assertEquals(1, shadowOf(application).boundServiceConnections.size) + } + + @Test + fun nullBindingFailsPendingConnect() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + + var failure: Throwable? = null + val connectJob = launch { + failure = runCatching { screenCaptureConnection.connect() }.exceptionOrNull() + } + shadowOf(application).boundServiceConnections.single().onNullBinding( + ComponentName(application, ScreenCaptureService::class.java), + ) + + connectJob.join() + // A null binding is never followed by onServiceConnected, so the caller must fail + // rather than stay suspended. + assertTrue(failure is IllegalStateException) + assertEquals(0, shadowOf(application).boundServiceConnections.size) + } + + @Test + fun deadBindingFailsPendingConnectAndReleasesTheBinding() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + + var failure: Throwable? = null + val connectJob = launch { + failure = runCatching { screenCaptureConnection.connect() }.exceptionOrNull() + } + shadowOf(application).boundServiceConnections.single().onBindingDied( + ComponentName(application, ScreenCaptureService::class.java), + ) + + connectJob.join() + assertTrue(failure is IllegalStateException) + + // A dead binding never reconnects, so the next caller must bind fresh instead of + // waiting on it. + val secondConnect = launch { screenCaptureConnection.connect() } + assertEquals(1, shadowOf(application).boundServiceConnections.size) + secondConnect.cancelAndJoin() + } + + @Test + fun stopEndsPendingConnect() = runTest { + val screenCaptureConnection = ScreenCaptureConnection(application) + + val connectJob = launch { screenCaptureConnection.connect() } + screenCaptureConnection.stop() + + connectJob.join() + assertEquals(0, shadowOf(application).boundServiceConnections.size) + } +}