Guard video client stop with a mutex #684
Open
+13
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
georgezy-amzn
previously approved these changes
Oct 2, 2025
|
|
||
| eglCore?.release() | ||
| eglCore = null | ||
| eglCore?.release() |
There was a problem hiding this comment.
Is there a reason we included this in the mutex scope?
There was a problem hiding this comment.
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.
| videoClientObserver.primaryMeetingPromotionObserver = null | ||
|
|
||
| videoClientStateController.stop() | ||
| videoClientStopMutex.withLock { |
There was a problem hiding this comment.
Were we able to reproduce? Was there a stack? Can we add a comment here?
There was a problem hiding this comment.
It was reproducible with new webrtc version. Added a comment here to explain the crash
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ℹ️ Description
There are race conditions in video client stop function that may lead to a crashes if multiple threads of video client stop are triggered at the same time (e.g., pointer deallocation, reference counter update). The purpose of this change is to avoid such crashes.
Issue #, if available
Type of change
🧪 How Has This Been Tested?
describe the tests that you ran to verify your changes, any relevant details for your test configuration
Smoke tested and verified crash doesn't happen when ending a meeting
Unit test coverage
Additional Manual Test
📱 Screenshots, if available
provide screenshots/video record if there's a UI change in demo app
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.