Closed Bug 1276156 Opened 4 years ago Closed 4 years ago

deadlock shutting down vie_encoder in statistics

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(1 file)

This may be a major cause of intermittent test-timed-out failures.

Looking through the logs of a couple timeouts on Try, I noticed that at least two had the same signature:

At least one thread hanging shutting down a vie_encoder thread: (MainThread in this instance:)
ProcessThreadImpl::DeRegisterModule() (criticalsection.h:117)
ViEEncoder::StopThreadsAndRemoveSharedMembers [vie_encoder.cc:225]
...
WebrtcVideoConduit::Destroy [VideoConduit.cpp:506]

This waits on the per-thread 'lock_' to be available, which is held while running code for the thread (note).

At least one thread in ProcessThreadImpl::Process() and *not* in Wait(), and generally in vie statistics code:
ProcessThreadImpl::Process [process_thread_impl.cc:209]
VideoCodingModuleImpl::Process [video_coding_impl.cc:100]
...
ViECodecImpl::GetSendCodecStatistics [vie_channel_manager.cc:395] ***

and in another timeout (android):
vcm::VideoSender::Process [video_sender.cc:99]
ViEEncoder::SendStatistics [vie_encoder.cc:783]
VideoCodecStatistics::OutgoingRate [CodecStatistics.cpp:60]
ViECodecImpl::GetSendCodecStatistics [vie_channel_manager.cc:395] ***
ViEInputManagerScoped::ViEInputManagerScoped [vie_manager_base.cc:45]
RWLockGeneric::AcquireLockShared [rw_lock_generic.cc:62]

Note the *** lines
This implies a deadlock around ViEManagerScopedBase::ViEManagerScopedBase()/instance_rwlock_.AcquireLockShared()
this should only be possible if it's owned exclusively by someone I think.  I'll note all uses of     ViEManagerWriteScoped() are scopes, and unless it's sitting in them the lock should be released, and I don't see any thread in them.
Rank: 15
Blocks: 1275814
Blocks: 1248848
To avoid deadlocks between DeleteChannel() and Process() code

MozReview-Commit-ID: G650SWDH8s0
Attachment #8757492 - Flags: review?(pkerr)
This is the deadlock scenario:

[15:15]	jesup	we're in VideoCodingModuleImpl::Process() (and thus the ProcessThread lock_ is held while we run)
[15:15]	jesup	on MainThread, we delete the VideoConduit
[15:16]	jesup	which calls DeleteChannel, which grabs an exclusive lock on the ViEManager (ViEManagerWriteScoped wl ((this))
[15:17]	jesup	then ::Process tries to do ViECodecImpl::GetSendCodecStatistics() which grabs the lock in readonly mode.  Since it's held for write, that waits.  
[15:22]	jesup	and then DeleteChannel eventually calls ViEEncoder::StopThreadsAndRemoveSharedMembers [vie_encoder.cc:225] which calls ProcessThreadImpl::DeRegisterModule() (criticalsection.h:117) which needs the lock_ -- which is held by the thread waiting on the channel lock
[15:22]	jesup	Boom, deadlock

Solution is to stop the threads *before* taking the exclusive lock which can deadlock us.
Duplicate of this bug: 1248848
Duplicate of this bug: 1275814
Attachment #8757492 - Flags: review?(pkerr) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68eb3a876ea5
Stop encoder ProcessThreads before deleting the channel r=pkerr
https://hg.mozilla.org/mozilla-central/rev/68eb3a876ea5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.