Fix the way locks are acquired in WebRtcAudioDeviceImpl::SetAudioRenderer.

content/renderer/media/webrtc_audio_device_impl.cc

Index: content/renderer/media/webrtc_audio_device_impl.cc
diff --git a/content/renderer/media/webrtc_audio_device_impl.cc b/content/renderer/media/webrtc_audio_device_impl.cc
index 5b87c186e68d7964acbfe5db0ad513ed59cff2f6..ef925ae54e2e569da9ae7b5971b3cf635ca4a175 100644
--- a/content/renderer/media/webrtc_audio_device_impl.cc
+++ b/content/renderer/media/webrtc_audio_device_impl.cc
@@ -397,13 +397,30 @@ bool WebRtcAudioDeviceImpl::SetAudioRenderer(WebRtcAudioRenderer* renderer) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   DCHECK(renderer);
 
-  base::AutoLock auto_lock(lock_);
-  if (renderer_.get())
-    return false;
+  // Here we acquire |lock_| in order to protect the internal state.
+  {
+    base::AutoLock auto_lock(lock_);
+    if (renderer_.get())
+      return false;
+  }
 
+  // We release |lock_| here because invoking |renderer|->Initialize while
+  // holding |lock_| would result in locks taken in the sequence
+  // (|this->lock_|,  |renderer->lock_|) while another thread (i.e, the
+  // AudioOutputDevice thread) might concurrently invoke a renderer method,
+  // which can itself invoke a method from |this|, resulting in locks taken in
+  // the sequence (|renderer->lock_|, |this->lock_|) in that thread.
+  // This order discrepancy can cause a deadlock (see Issue 433993).
+  // However, we do not need to hold |this->lock_| in order to invoke
+  // |renderer|->Initialize, since it does not involve any unprotected access to
+  // the internal state of |this|.
   if (!renderer->Initialize(this))
     return false;
 
+  // We acquire |lock_| again and assert our precondition, since we are
+  // accessing the internal state again.
+  base::AutoLock auto_lock(lock_);
+  DCHECK(!renderer_.get());

[tommi (sloooow) - chröme, 2015/03/31 08:29:13]

Would also be good to have a note here about how it's guaranteed that only one
thread will be calling SetAudioRenderer() at a time. (i.e. how we guarantee to
never hit this check).  Actually, can you change this to be a regular CHECK(),
not just DCHECK()?

   renderer_ = renderer;
   return true;
 }