Fixed ScreenCaptureService staying bound when screen share setup is cancelled - #983
Conversation
🦋 Changeset detectedLatest commit: f275e98 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
9a1ad60 to
7528bc6
Compare
…ancelled ScreenCaptureConnection recorded a binding only once onServiceConnected arrived. A coroutine cancelled before connect() returned left the ServiceConnection registered, so 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 when 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. Both cover the documented case where bindService leaves a connection registered while reporting failure or throwing. Two paths could also leave connect() suspended forever. A stop() racing a connect could unbind and snapshot an empty waiter set just before the caller enqueued itself, so requesting the bind and registering the waiter now happen under one lock. A failed bindService left the state claiming a bind was in flight, which the next caller would wait on with nothing pending.
7528bc6 to
40e1881
Compare
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
lgtm in general, one question
| private var hasConnectedCaller = false | ||
| private val queuedConnects = mutableSetOf<CancellableContinuation<Unit>>() | ||
| private val connection: ServiceConnection = object : ServiceConnection { | ||
| override fun onServiceDisconnected(name: ComponentName) { |
There was a problem hiding this comment.
Is this the only way that binding can fail ? I wonder if there are other callbacks that we should handle as well
There was a problem hiding this comment.
The reachable failures are covered: bindService returning false or throwing, both handled in bind(). I went through the rest of the ServiceConnection surface, and for this service the other callbacks cannot fire today:
- onNullBinding fires when the service's onBind returns null. The intent targets the exact ScreenCaptureService class and its onBind unconditionally returns the binder. An app that disables the service via manifest merge lands in the bindService-returns-false path.
- onBindingDied fires when the binding is permanently dead, i.e. the package hosting the service was updated or force-stopped. The service is declared in the SDK manifest without android:process, so it runs in the client's own process and dies with it.
- onServiceDisconnected is handled, and queued waiters intentionally stay queued there: the binding survives a service crash, BIND_AUTO_CREATE recreates the service, and onServiceConnected resumes them.
That said, if onNullBinding or onBindingDied ever became reachable (onBind made conditional, or the service moved out of process), neither is followed by onServiceConnected, so a queued caller would suspend forever, the same bug class this PR fixes. f275e98 handles both as extra caution: release the binding and fail the waiters with IllegalStateException, with a test for each.
Neither onBindingDied nor onNullBinding is followed by onServiceConnected, so a queued caller would stay suspended until stop(). Both now release the binding and fail the waiters with IllegalStateException.
|
Thank you for looking at this so swiftly! |
davidliu
left a comment
davidliu
left a comment
There was a problem hiding this comment.
LGTM, I think this will need a follow up in LocalParticipant as well, not only to fix up the things mentioned, but cancelling there would also still leak the service binding I think (as far as ScreenCaptureConnection knows, it handled everything fine and it now on the owner to clean up the bind).
3 participants
ScreenCaptureConnectionrecords a binding only whenonServiceConnectedarrives, but the binding exists from thebindServicecall onwards. Cancel the coroutine in that window and theServiceConnectionstays registered, soBIND_AUTO_CREATEkeepsScreenCaptureServicealive for the lifetime of the context.stop()cannot be the whole answer here.LocalParticipant.setScreenShareEnabledawaits the bind insidesetTrackEnabled, and the track it creates is not published or stored yet, so a cancellation there abandons the track without anyfinallyto stop it. Nothing reachesstop()at all. A cancelled connect now releases the binding itself once no caller is left waiting on it, andstop()unbinds on that same wider condition. Callers stay tracked untilconnect()actually returns, so this covers a cancellation landing afteronServiceConnectedbut before the resumed caller runs, which is otherwise the same leak with the binding already connected.Two smaller problems live in the same window:
bindServiceregisters a connection even when it returns false or throws, which the platform docs call out, so the existingthrow IllegalStateExceptionpath leaked one as well.stop()while a connect is pending never resumed the queued continuations, sostartForegroundServicesuspended forever.Two more ways a caller could suspend forever came out of tightening the state handling, and are fixed here too. A
stop()racing a connect could unbind and snapshot an empty waiter set just before the caller enqueued itself, so requesting the bind and registering the waiter now happen under one lock. A failedbindServiceleft the state claiming a bind was in flight, which the next caller would then wait on with nothing pending.Six tests cover the cancellation, rebind, post-connect cancellation, and pending-stop paths; five of them fail on main and pass here. The sixth pins the happy path, so releasing on cancellation cannot tear down a binding a connected caller still holds.
./gradlew test, spotless, and detekt pass.Worth flagging separately: cancellation after
createScreencastTrackcan also orphan the new track becausesetTrackEnabledonly cleans it up whenpublishVideoTrackreturns false; cancellation fromstartForegroundServiceorpublishVideoTrackexits before that branch. Cancellation during the service bind happens before capture starts, so it strands the initializedSurfaceTextureHelper,VideoSource, capturer, and rtcTrack, but not aMediaProjection. Cancellation while publication is suspended happens afterstartCaptureand can leave the activeMediaProjectionrunning too. This is outside theScreenCaptureConnectionfix. Fully addressing it requires cancellation-safe ownership transfer inLocalParticipantand correctSurfaceTextureHelperownership inLocalScreencastVideoTrack, and is not attempted here, but happy to take it on if you want it in the same change.