There is a racing between SyncSocket::Receive in audio_thread_ and SyncSocket::Close in renderer ...

content/renderer/media/audio_input_device.cc

 // 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.
 
 #include "content/renderer/media/audio_input_device.h"
 
 #include "base/bind.h"
 #include "base/message_loop.h"
 #include "base/time.h"
 #include "content/common/child_process.h"
 #include "content/common/media/audio_messages.h"
 #include "content/common/view_messages.h"
 #include "content/renderer/render_thread_impl.h"
 #include "media/audio/audio_manager_base.h"
 #include "media/audio/audio_util.h"
 
 AudioInputDevice::AudioInputDevice(size_t buffer_size,
                                    int channels,
                                    double sample_rate,
                                    CaptureCallback* callback,
                                    CaptureEventHandler* event_handler)
     : callback_(callback),
       event_handler_(event_handler),
       audio_delay_milliseconds_(0),
       volume_(1.0),
       stream_id_(0),
       session_id_(0),
-      pending_device_ready_(false) {
+      pending_device_ready_(false),
+      audio_event_(true, false) {
   filter_ = RenderThreadImpl::current()->audio_input_message_filter();
   audio_data_.reserve(channels);
 #if defined(OS_MACOSX)
   VLOG(1) << "Using AUDIO_PCM_LOW_LATENCY as input mode on Mac OS X.";
   audio_parameters_.format = AudioParameters::AUDIO_PCM_LOW_LATENCY;
 #elif defined(OS_WIN)
   VLOG(1) << "Using AUDIO_PCM_LOW_LATENCY as input mode on Windows.";
   audio_parameters_.format = AudioParameters::AUDIO_PCM_LOW_LATENCY;
 #else
   // TODO(henrika): add support for AUDIO_PCM_LOW_LATENCY on Linux as well.
@@ skipping 44 matching lines @@
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&AudioInputDevice::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)) {
     if (audio_thread_.get()) {
-      // Terminate the main thread function in the audio thread.
-      socket_->Close();
+      // Signal the |audio_event_| to terminate the main thread function.
+      audio_event_.Signal();
       // Wait for the audio thread to exit.
       audio_thread_->Join();
       // Ensures that we can call Stop() multiple times.
       audio_thread_.reset(NULL);
     }
   } else {
     LOG(ERROR) << "Failed to shut down audio input on IO thread";

[Ami GONE FROM CHROMIUM, 2011/11/29 20:38:16]

This is incorrect code - quoting WaitableEvent::TimedWait's .h comment:
  // If this method returns false, then it
  // does not necessarily mean that max_time was exceeded.

I only mention it for context on Tommi's other comment about Stop().

     return false;
   }
 
   return true;
 }
 
 bool AudioInputDevice::SetVolume(double volume) {
   NOTIMPLEMENTED();
   return false;
 }
@@ skipping 80 matching lines @@
     // Close the socket handler.
     base::SyncSocket socket(socket_handle);
     return;
   }
 
   shared_memory_.reset(new base::SharedMemory(handle, false));
   shared_memory_->Map(length);
 
   socket_.reset(new base::SyncSocket(socket_handle));
 
+  audio_event_.Reset();
   audio_thread_.reset(
       new base::DelegateSimpleThread(this, "RendererAudioInputThread"));
   audio_thread_->Start();
 
   MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&AudioInputDevice::StartOnIOThread, this));
 }
 
 void AudioInputDevice::OnVolume(double volume) {
@@ skipping 68 matching lines @@
 // this thread.
 void AudioInputDevice::Run() {
   audio_thread_->SetThreadPriority(base::kThreadPriority_RealtimeAudio);
 
   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 (sizeof(pending_data) == socket_->Receive(&pending_data,
+  while (!audio_event_.IsSignaled() &&

[Ami GONE FROM CHROMIUM, 2011/11/29 17:48:32]

I'm not super-familiar with the audio codebase, but this *looks like* a
fundamentally broken idiom.  
The return value of IsSignaled() is irrelevant *immediately* after it returns
(before the && is eval'd) because no locks are held.

I can't tell from the CL description, but it *sounds* like you think SyncSocket
can't be used "to perform rudimentary cross-process synchronization with low
latency" (from sync_socket.h) without a data race, which sounds like a bug in
SyncSocket.  Can you fix it there?

[tommi (sloooow) - chröme, 2011/11/29 20:11:32]

Hey Ami,

A couple of questions inline:

On 2011/11/29 17:48:32, Ami Fischman wrote:
Can you explain this in a bit more detail?  No locks should be needed since if
the event is signaled, the loop breaks.  The event object is itself, thread
safe.
The race is that the socket handle is being closed and set to invalid from one
thread while another is waiting on it.

Looking closer at the code though, I see that there is a check that Start() is
always called on the IO thread but there is no such check for Stop().  Is it
simply missing or can Stop() be called from a different thread than the IO
thread?  If so, then we still have a race between Start and Stop, but if Stop is
always called from the same thread as Start, then I think the event object fixes
the race.

Then there's also the problem that was pointed out if no data is sent to the
socket - which could cause a hang.

[Ami GONE FROM CHROMIUM, 2011/11/29 20:38:16]

The problem is with the other case; IsSignaled() may return false and
immediately thereafter a signal comes in (before the subsequent Receive is run).
IIUC the bug this CL is trying to fix, you're back to square one in this case.
What does "set to invalid" mean?
Merely reading on one thread and closing on another thread should work fine (in
vanilla socket(2)).  If SyncSocket doesn't support that that's a bug IMO.
Can anyone repro a tsan alert on a SyncSocket test (not using Audio*Device)?
Start() posts to the IO thread.  Stop() posts to the IO thread for shutdown and
then waits for completion before returning.  Neither check that they are "always
called on the IO thread".
This goes to the crux of my point about fixing races in SyncSocket; SyncSocket
is a synchronization abstraction which makes me strongly doubt it's possible to
work *around* bugs in it from the outside without giving up serious latency
concessions.

[no longer working on chromium, 2011/11/30 17:02:55]

Event is thread safe. In the case you mentioned above, the socket will continue
to work, but it will quite the loop next time it check if
audio_event_.IsSignaled(). This is fine since our Stop() is synchronous. 

The only concern is that event is not "quick", my understanding here is that
when the event is signal, it may take a short interval to have IsSignaled()
return true.
I think @nfullagar has covered the situation well, I am not sure if there is any
perfect solution to handle the race during the tear down, the current Close() in
syncsocket just tries to close the handle, and this is allowed to fail. I
believe the client should take care of the case when the socket should be
closed.

Furthermore, I think there might be two races here, one is that we access to
socket_ object in two threads, another is that we try to close the handle of the
socket in Stop() while receiving data in Run().

+         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.
 
     // Convert the number of pending bytes in the capture buffer
     // into milliseconds.
     audio_delay_milliseconds_ = pending_data / bytes_per_ms;
 
     FireCaptureCallback();
   }
+
+  // Close the socket.
+  socket_->Close();
 }
 
 void AudioInputDevice::FireCaptureCallback() {
   if (!callback_)
       return;
 
   const size_t number_of_frames = audio_parameters_.samples_per_packet;
 
   // Read 16-bit samples from shared memory (browser writes to it).
   int16* input_audio = static_cast<int16*>(shared_memory_data());
@@ skipping 10 matching lines @@
                                     bytes_per_sample,
                                     number_of_frames);
   }
 
   // Deliver captured data to the client in floating point format
   // and update the audio-delay measurement.
   callback_->Capture(audio_data_,
                      number_of_frames,
                      audio_delay_milliseconds_);
 }