Revert 119769 - Switch AudioDevice classes from SyncSocket to CancelableSyncSocket.

This fixes the double close issue and makes Stop() instant.
Also, this addresses some issues reported by tsan (see bug reports).

BUG=103711,95509
TEST=content_unittests,media_unittests


Review URL: http://codereview.chromium.org/9121045

TBR=tommi@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9317001

git-svn-id: svn://svn.chromium.org/chrome/branches/1025/src@119921 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 97 insertions, 80 deletions

diff --git a/content/browser/renderer_host/media/audio_input_sync_writer.cc b/content/browser/renderer_host/media/audio_input_sync_writer.cc
index 9524ad0..5a8870c 100644
--- a/content/browser/renderer_host/media/audio_input_sync_writer.cc
+++ b/content/browser/renderer_host/media/audio_input_sync_writer.cc
@@ -31,10 +31,9 @@ void AudioInputSyncWriter::Close() {
 }
 
 bool AudioInputSyncWriter::Init() {
-  socket_.reset(new base::CancelableSyncSocket());
-  foreign_socket_.reset(new base::CancelableSyncSocket());
-  return base::CancelableSyncSocket::CreatePair(socket_.get(),
-                                                foreign_socket_.get());
+  socket_.reset(new base::SyncSocket());
+  foreign_socket_.reset(new base::SyncSocket());
+  return base::SyncSocket::CreatePair(socket_.get(), foreign_socket_.get());
 }
 
 #if defined(OS_WIN)
diff --git a/content/browser/renderer_host/media/audio_input_sync_writer.h b/content/browser/renderer_host/media/audio_input_sync_writer.h
index 3114c35..b3f69a8b 100644
--- a/content/browser/renderer_host/media/audio_input_sync_writer.h
+++ b/content/browser/renderer_host/media/audio_input_sync_writer.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -41,12 +41,12 @@ class AudioInputSyncWriter : public media::AudioInputController::SyncWriter {
  private:
   base::SharedMemory* shared_memory_;
 
-  // Socket for transmitting audio data.
-  scoped_ptr<base::CancelableSyncSocket> socket_;
+  // A pair of SyncSocket for transmitting audio data.
+  scoped_ptr<base::SyncSocket> socket_;
 
-  // Socket to be used by the renderer. The reference is released after
+  // SyncSocket to be used by the renderer. The reference is released after
   // PrepareForeignSocketHandle() is called and ran successfully.
-  scoped_ptr<base::CancelableSyncSocket> foreign_socket_;
+  scoped_ptr<base::SyncSocket> foreign_socket_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(AudioInputSyncWriter);
 };
diff --git a/content/browser/renderer_host/media/audio_sync_reader.cc b/content/browser/renderer_host/media/audio_sync_reader.cc
index 7b6664b..c06345a 100644
--- a/content/browser/renderer_host/media/audio_sync_reader.cc
+++ b/content/browser/renderer_host/media/audio_sync_reader.cc
@@ -92,10 +92,9 @@ void AudioSyncReader::Close() {
 }
 
 bool AudioSyncReader::Init() {
-  socket_.reset(new base::CancelableSyncSocket());
-  foreign_socket_.reset(new base::CancelableSyncSocket());
-  return base::CancelableSyncSocket::CreatePair(socket_.get(),
-                                                foreign_socket_.get());
+  socket_.reset(new base::SyncSocket());
+  foreign_socket_.reset(new base::SyncSocket());
+  return base::SyncSocket::CreatePair(socket_.get(), foreign_socket_.get());
 }
 
 #if defined(OS_WIN)
diff --git a/content/browser/renderer_host/media/audio_sync_reader.h b/content/browser/renderer_host/media/audio_sync_reader.h
index 60a2879..4aff8aa9 100644
--- a/content/browser/renderer_host/media/audio_sync_reader.h
+++ b/content/browser/renderer_host/media/audio_sync_reader.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -45,12 +45,12 @@ class AudioSyncReader : public media::AudioOutputController::SyncReader {
   base::SharedMemory* shared_memory_;
   base::Time previous_call_time_;
 
-  // Socket for transmitting audio data.
-  scoped_ptr<base::CancelableSyncSocket> socket_;
+  // A pair of SyncSocket for transmitting audio data.
+  scoped_ptr<base::SyncSocket> socket_;
 
-  // Socket to be used by the renderer. The reference is released after
+  // SyncSocket to be used by the renderer. The reference is released after
   // PrepareForeignSocketHandle() is called and ran successfully.
-  scoped_ptr<base::CancelableSyncSocket> foreign_socket_;
+  scoped_ptr<base::SyncSocket> foreign_socket_;
 
   // Protect socket_ access by lock to prevent race condition when audio
   // controller thread closes the reader and hardware audio thread is reading
diff --git a/content/renderer/media/audio_device.cc b/content/renderer/media/audio_device.cc
index 19349c9..b6ed566 100644
--- a/content/renderer/media/audio_device.cc
+++ b/content/renderer/media/audio_device.cc
@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "content/common/child_process.h"
 #include "content/common/media/audio_messages.h"
@@ -245,7 +244,7 @@ void AudioDevice::OnLowLatencyCreated(
 
   shared_memory_handle_ = handle;
   memory_length_ = length;
-  audio_socket_.reset(new base::CancelableSyncSocket(socket_handle));
+  audio_socket_ = new AudioSocket(socket_handle);
 
   audio_thread_.reset(
       new base::DelegateSimpleThread(this, "renderer_audio_thread"));
@@ -272,25 +271,20 @@ void AudioDevice::Run() {
 
   base::SharedMemory shared_memory(shared_memory_handle_, false);
   shared_memory.Map(media::TotalSharedMemorySizeInBytes(memory_length_));
-  base::CancelableSyncSocket* audio_socket = audio_socket_.get();
+  scoped_refptr<AudioSocket> audio_socket(audio_socket_);
 
+  int pending_data;
   const int samples_per_ms = static_cast<int>(sample_rate_) / 1000;
   const int bytes_per_ms = channels_ * (bits_per_sample_ / 8) * samples_per_ms;
 
-  while (true) {
-    uint32 pending_data = 0;
-    size_t bytes_read = audio_socket->Receive(&pending_data,
-                                              sizeof(pending_data));
-    if (bytes_read != sizeof(pending_data)) {
-      DCHECK_EQ(bytes_read, 0U);
-      break;
-    }
-
-    if (pending_data ==
-        static_cast<uint32>(media::AudioOutputController::kPauseMark)) {
+  while (sizeof(pending_data) ==
+      audio_socket->socket()->Receive(&pending_data, sizeof(pending_data))) {
+    if (pending_data == media::AudioOutputController::kPauseMark) {
       memset(shared_memory.memory(), 0, memory_length_);
       media::SetActualDataSizeInBytes(&shared_memory, memory_length_, 0);
       continue;
+    } else if (pending_data < 0) {
+      break;
     }
 
     // Convert the number of pending bytes in the render buffer
@@ -304,6 +298,7 @@ void AudioDevice::Run() {
                                     memory_length_,
                                     num_frames * channels_ * sizeof(int16));
   }
+  audio_socket->Close();
 }
 
 size_t AudioDevice::FireRenderCallback(int16* data) {
@@ -333,11 +328,9 @@ void AudioDevice::ShutDownAudioThread() {
   if (audio_thread_.get()) {
     // Close the socket to terminate the main thread function in the
     // audio thread.
-    audio_socket_->Shutdown();  // Stops blocking Receive calls.
-    // TODO(tommi): We must not do this from the IO thread.  Fix.
-    base::ThreadRestrictions::ScopedAllowIO allow_wait;
+    audio_socket_->Close();
+    audio_socket_ = NULL;
     audio_thread_->Join();
     audio_thread_.reset(NULL);
-    audio_socket_.reset();
   }
 }
diff --git a/content/renderer/media/audio_device.h b/content/renderer/media/audio_device.h
index de6ba3b..9585ff0 100644
--- a/content/renderer/media/audio_device.h
+++ b/content/renderer/media/audio_device.h
@@ -142,6 +142,33 @@ class CONTENT_EXPORT AudioDevice
   virtual void OnVolume(double volume) OVERRIDE;
 
  private:
+  // Encapsulate socket into separate class to avoid double-close.
+  // Class is ref-counted to avoid potential race condition if audio device
+  // is deleted simultaneously with audio thread stopping.
+  class AudioSocket : public base::RefCountedThreadSafe<AudioSocket> {
+   public:
+    explicit AudioSocket(base::SyncSocket::Handle socket_handle)
+        : socket_(socket_handle) {
+    }
+    base::SyncSocket* socket() {
+      return &socket_;
+    }
+    void Close() {
+      // Close() should be thread-safe, obtain the lock.
+      base::AutoLock auto_lock(lock_);
+      socket_.Close();
+    }
+
+   private:
+    // Magic required by ref_counted.h to avoid any code deleting the object
+    // accidentally while there are references to it.
+    friend class base::RefCountedThreadSafe<AudioSocket>;
+    ~AudioSocket() { }
+
+    base::Lock lock_;
+    base::SyncSocket socket_;
+  };
+
   // Magic required by ref_counted.h to avoid any code deleting the object
   // accidentally while there are references to it.
   friend class base::RefCountedThreadSafe<AudioDevice>;
@@ -221,7 +248,7 @@ class CONTENT_EXPORT AudioDevice
   // These variables must only be set on the IO thread while the audio_thread_
   // is not running.
   base::SharedMemoryHandle shared_memory_handle_;
-  scoped_ptr<base::CancelableSyncSocket> audio_socket_;
+  scoped_refptr<AudioSocket> audio_socket_;
   int memory_length_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioDevice);
diff --git a/content/renderer/media/audio_input_device.cc b/content/renderer/media/audio_input_device.cc
index 4a346e9..71a15f4 100644
--- a/content/renderer/media/audio_input_device.cc
+++ b/content/renderer/media/audio_input_device.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -6,7 +6,6 @@
 
 #include "base/bind.h"
 #include "base/message_loop.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "content/common/child_process.h"
 #include "content/common/media/audio_messages.h"
@@ -28,6 +27,7 @@ AudioInputDevice::AudioInputDevice(size_t buffer_size,
       session_id_(0),
       pending_device_ready_(false),
       shared_memory_handle_(base::SharedMemory::NULLHandle()),
+      socket_handle_(base::SyncSocket::kInvalidHandle),
       memory_length_(0) {
   filter_ = RenderThreadImpl::current()->audio_input_message_filter();
   audio_data_.reserve(channels);
@@ -77,8 +77,13 @@ void AudioInputDevice::SetDevice(int session_id) {
 void AudioInputDevice::Stop() {
   DCHECK(MessageLoop::current() != ChildProcess::current()->io_message_loop());
   VLOG(1) << "Stop()";
+  // 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(&AudioInputDevice::ShutDownOnIOThread, this,
@@ -87,7 +92,20 @@ void AudioInputDevice::Stop() {
   // 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.
-  completion.Wait();
+  if (!completion.TimedWait(kMaxTimeOut)) {
+    LOG(ERROR) << "Failed to shut down audio input on IO thread";
+  }
+
+  if (audio_thread_.get()) {
+    // Terminate the main thread function in the audio thread.
+    {
+      base::SyncSocket socket(socket_handle_);
+    }
+    // Wait for the audio thread to exit.
+    audio_thread_->Join();
+    // Ensures that we can call Stop() multiple times.
+    audio_thread_.reset(NULL);
+  }
 }
 
 bool AudioInputDevice::SetVolume(double volume) {
@@ -143,8 +161,6 @@ void AudioInputDevice::ShutDownOnIOThread(base::WaitableEvent* completion) {
   filter_->RemoveDelegate(stream_id_);
   Send(new AudioInputHostMsg_CloseStream(stream_id_));
 
-  ShutDownAudioThread();
-
   stream_id_ = 0;
   session_id_ = 0;
   pending_device_ready_ = false;
@@ -184,7 +200,8 @@ void AudioInputDevice::OnLowLatencyCreated(
 
   shared_memory_handle_ = handle;
   memory_length_ = length;
-  audio_socket_.reset(new base::CancelableSyncSocket(socket_handle));
+
+  socket_handle_ = socket_handle;
 
   audio_thread_.reset(
       new base::DelegateSimpleThread(this, "RendererAudioInputThread"));
@@ -203,15 +220,21 @@ void AudioInputDevice::OnStateChanged(AudioStreamState state) {
   DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
   switch (state) {
     case kAudioStreamPaused:
-      // TODO(xians): Should we just call ShutDownOnIOThread here instead?
-
       // Do nothing if the stream has been closed.
       if (!stream_id_)
         return;
 
       filter_->RemoveDelegate(stream_id_);
 
-      ShutDownAudioThread();
+      // Joining the audio thread will be quite soon, since the stream has
+      // been closed before.
+      if (audio_thread_.get()) {
+        {
+          base::SyncSocket socket(socket_handle_);
+        }
+        audio_thread_->Join();
+        audio_thread_.reset(NULL);
+      }
 
       if (event_handler_)
         event_handler_->OnDeviceStopped();
@@ -245,8 +268,8 @@ void AudioInputDevice::OnDeviceReady(const std::string& device_id) {
     filter_->RemoveDelegate(stream_id_);
     stream_id_ = 0;
   } else {
-    Send(new AudioInputHostMsg_CreateStream(stream_id_, audio_parameters_,
-                                            true, device_id));
+    Send(new AudioInputHostMsg_CreateStream(
+        stream_id_, audio_parameters_, true, device_id));
   }
 
   pending_device_ready_ = false;
@@ -267,20 +290,17 @@ void AudioInputDevice::Run() {
   base::SharedMemory shared_memory(shared_memory_handle_, false);
   shared_memory.Map(memory_length_);
 
-  base::CancelableSyncSocket* socket = audio_socket_.get();
+  base::SyncSocket socket(socket_handle_);
 
+  int pending_data;
   const int samples_per_ms =
       static_cast<int>(audio_parameters_.sample_rate) / 1000;
   const int bytes_per_ms = audio_parameters_.channels *
       (audio_parameters_.bits_per_sample / 8) * samples_per_ms;
 
-  while (true) {
-    uint32 pending_data = 0;
-    size_t received = socket->Receive(&pending_data, sizeof(pending_data));
-    if (received != sizeof(pending_data)) {
-      DCHECK(received == 0U);
-      break;
-    }
+  while ((sizeof(pending_data) == socket.Receive(&pending_data,
+                                                sizeof(pending_data))) &&
+         (pending_data >= 0)) {
     // TODO(henrika): investigate the provided |pending_data| value
     // and ensure that it is actually an accurate delay estimation.
 
@@ -294,7 +314,7 @@ void AudioInputDevice::Run() {
 
 void AudioInputDevice::FireCaptureCallback(int16* input_audio) {
   if (!callback_)
-    return;
+      return;
 
   const size_t number_of_frames = audio_parameters_.samples_per_packet;
 
@@ -318,22 +338,3 @@ void AudioInputDevice::FireCaptureCallback(int16* input_audio) {
                      number_of_frames,
                      audio_delay_milliseconds_);
 }
-
-void AudioInputDevice::ShutDownAudioThread() {
-  DCHECK_EQ(MessageLoop::current(), ChildProcess::current()->io_message_loop());
-
-  // TODO(tommi): This is a copy/paste of the same method in AudioDevice.
-  // We could extract the worker thread+socket bits out to a separate
-  // class to avoid having to fix the same issues twice in these classes.
-
-  if (audio_thread_.get()) {
-    // Close the socket to terminate the main thread function in the
-    // audio thread.
-    audio_socket_->Shutdown();  // Stops blocking Receive calls.
-    // TODO(tommi): We must not do this from the IO thread.  Fix.
-    base::ThreadRestrictions::ScopedAllowIO allow_wait;
-    audio_thread_->Join();
-    audio_thread_.reset(NULL);
-    audio_socket_.reset();
-  }
-}
diff --git a/content/renderer/media/audio_input_device.h b/content/renderer/media/audio_input_device.h
index 96dd2ff..ec74525 100644
--- a/content/renderer/media/audio_input_device.h
+++ b/content/renderer/media/audio_input_device.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -164,8 +164,6 @@ class CONTENT_EXPORT AudioInputDevice
   void StartOnIOThread();
   void ShutDownOnIOThread(base::WaitableEvent* completion);
   void SetVolumeOnIOThread(double volume);
-  // Closes socket and joins with the audio thread.
-  void ShutDownAudioThread();
 
   void Send(IPC::Message* message);
 
@@ -211,7 +209,7 @@ class CONTENT_EXPORT AudioInputDevice
   bool pending_device_ready_;
 
   base::SharedMemoryHandle shared_memory_handle_;
-  scoped_ptr<base::CancelableSyncSocket> audio_socket_;
+  base::SyncSocket::Handle socket_handle_;
   int memory_length_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(AudioInputDevice);