Safe and reliable fix for 2 race conditions.

content/renderer/media/audio_device.cc

 // Copyright (c) 2012 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/audio_device.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
 #include "base/time.h"
@@ skipping 90 matching lines @@
   params.bits_per_sample = bits_per_sample_;
   params.samples_per_packet = buffer_size_;
 
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&AudioDevice::InitializeOnIOThread, this, params));
 }
 
 void AudioDevice::Stop() {
   DCHECK(MessageLoop::current() != ChildProcess::current()->io_message_loop());
-  // Max waiting time for Stop() to complete. If this time limit is passed,
-  // we will stop waiting and return false. It ensures that Stop() can't block
-  // the calling thread forever.
-  const base::TimeDelta kMaxTimeOut = base::TimeDelta::FromMilliseconds(1000);
-
   base::WaitableEvent completion(false, false);
 
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&AudioDevice::ShutDownOnIOThread, this, &completion));
 
   // We wait here for the IO task to be completed to remove race conflicts
   // with OnLowLatencyCreated() and to ensure that Stop() acts as a synchronous
   // function call.
-  if (!completion.TimedWait(kMaxTimeOut)) {
-    LOG(ERROR) << "Failed to shut down audio output on IO thread";
-  }
+  completion.Wait();

[Chris Rogers, 2012/01/06 18:26:28]

Is there a case where the time-out is actually needed?  Shijing put that in
there, so we need his opinion about it.  As an alternative to this, we could
move the "completion" variable into a member variable instead of stack-based to
avoid it going away when this method finished.

[tommi (sloooow) - chröme, 2012/01/12 13:13:07]

What about moving the call to ShutDownAudioThread to be the last step inside
ShutDownOnIOThread?  That way we don't need the completion event at all.

This is assuming that the caller of Stop doesn't expect everything to be fully
stopped when Stop() returns.

   ShutDownAudioThread();
 }
 
 void AudioDevice::Play() {
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&AudioDevice::PlayOnIOThread, this));
 }
 
 void AudioDevice::Pause(bool flush) {
@@ skipping 50 matching lines @@
     // the stream is first starting.
     play_on_start_ = false;
   }
 }
 
 void AudioDevice::ShutDownOnIOThread(base::WaitableEvent* completion) {
   DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
   is_started_ = false;
 
   // Make sure we don't call shutdown more than once.
   if (!stream_id_) {

[tommi (sloooow) - chröme, 2012/01/12 13:13:07]

nit: this method can be simplified by removing this early return and reversing
the if:

if (stream_id_) {
  filter_->RemoveDelegate(stream_id_);
  Send(new AudioHostMsg_CloseStream(stream_id_);
  stream_id_ = 0;
}

if (completion)
  completion->Signal();

     if (completion)
       completion->Signal();
     return;
   }
 
   filter_->RemoveDelegate(stream_id_);
   Send(new AudioHostMsg_CloseStream(stream_id_));
   stream_id_ = 0;
 
   if (completion)
@@ skipping 129 matching lines @@
                                   data,
                                   buffer_size_);
   }
   return num_frames;
 }
 
 void AudioDevice::ShutDownAudioThread() {
   // Synchronize with OnLowLatencyCreated().
   base::AutoLock auto_lock(lock_);
   if (audio_thread_.get()) {
     // Close the socket to terminate the main thread function in the

[tommi (sloooow) - chröme, 2012/01/12 13:13:07]

add a check that makes sure we're not currently on the audio thread.

     // audio thread.
     audio_socket_->Close();
     audio_thread_->Join();
     audio_thread_.reset(NULL);
   }
 }