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

content/renderer/media/webrtc_audio_device_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/webrtc_audio_device_impl.h"
 
 #include "base/bind.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
 #include "base/win/windows_version.h"
@@ skipping 379 matching lines @@
     uint32_t* sample_rate) const {
   DCHECK(signaling_thread_checker_.CalledOnValidThread());
   *sample_rate = renderer_.get() ? renderer_->sample_rate() : 0;
   return 0;
 }
 
 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;
 }
 
 void WebRtcAudioDeviceImpl::AddAudioCapturer(
     const scoped_refptr<WebRtcAudioCapturer>& capturer) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   DVLOG(1) << "WebRtcAudioDeviceImpl::AddAudioCapturer()";
   DCHECK(capturer.get());
   DCHECK(!capturer->device_id().empty());
@@ skipping 54 matching lines @@
   // If there is no capturer or there are more than one open capture devices,
   // return false.
   if (capturers_.size() != 1)
     return false;
 
   return capturers_.back()->GetPairedOutputParameters(
       session_id, output_sample_rate, output_frames_per_buffer);
 }
 
 }  // namespace content