-
Notifications
You must be signed in to change notification settings - Fork 54
Guard video client stop with a mutex #684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,8 @@ import com.xodee.client.video.VideoSubscriptionConfigurationInternal | |
import java.security.InvalidParameterException | ||
import kotlinx.coroutines.GlobalScope | ||
import kotlinx.coroutines.launch | ||
import kotlinx.coroutines.sync.Mutex | ||
import kotlinx.coroutines.sync.withLock | ||
|
||
class DefaultVideoClientController( | ||
private val context: Context, | ||
|
@@ -64,7 +66,7 @@ class DefaultVideoClientController( | |
private val cameraCaptureSource: DefaultCameraCaptureSource | ||
private var videoSourceAdapter = VideoSourceAdapter() | ||
private var isUsingInternalCaptureSource = false | ||
|
||
private val videoClientStopMutex = Mutex() | ||
init { | ||
videoClientStateController.bindLifecycleHandler(this) | ||
|
||
|
@@ -100,10 +102,16 @@ class DefaultVideoClientController( | |
// So it doesn't call it spuriously | ||
videoClientObserver.primaryMeetingPromotionObserver = null | ||
|
||
videoClientStateController.stop() | ||
videoClientStopMutex.withLock { | ||
// Race conditions in video client stop functions may lead to crashes at pointer deallocation | ||
// if multiple such threads are called at the same time. | ||
videoClientStateController.stop() | ||
|
||
eglCore?.release() | ||
eglCore = null | ||
// We put release eglCore under mutex as a race condition may decrease its reference counter | ||
// below 0 and cause crashes if multiple such calls happen together. | ||
eglCore?.release() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we included this in the mutex scope? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we only put videoClientStateController.stop() under mutex, multiple eglCore?.release() may be triggered at the same time, this is another race condition that will trigger a crash for eglCore reference counter below 0. Added a comment. |
||
eglCore = null | ||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were we able to reproduce? Was there a stack? Can we add a comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was reproducible with new webrtc version. Added a comment here to explain the crash