1276156 - deadlock shutting down vie_encoder in statistics

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Stop encoder ProcessThreads before deleting the channel

To avoid deadlocks between DeleteChannel() and Process() code

MozReview-Commit-ID: G650SWDH8s0

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e3941c8229

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Note: here are the two other hits we had before that led me to find this:
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ca6f3e3049e22cb707dac332de5f7f726fadb1
https://treeherder.mozilla.org/logviewer.html#?job_id=21535308&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=21534169&repo=try

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68eb3a876ea5
Stop encoder ProcessThreads before deleting the channel r=pkerr

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68eb3a876ea5