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

content/renderer/media/audio_input_device.cc

Index: content/renderer/media/audio_input_device.cc
===================================================================
--- content/renderer/media/audio_input_device.cc	(revision 111890)
+++ content/renderer/media/audio_input_device.cc	(working copy)
@@ -25,7 +25,8 @@
       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)
@@ -90,8 +91,8 @@
   // 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.
@@ -199,6 +200,7 @@
 
   socket_.reset(new base::SyncSocket(socket_handle));
 
+  audio_event_.Reset();
   audio_thread_.reset(
       new base::DelegateSimpleThread(this, "RendererAudioInputThread"));
   audio_thread_->Start();
@@ -287,7 +289,8 @@
   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
@@ -299,6 +302,9 @@
 
     FireCaptureCallback();
   }
+
+  // Close the socket.
+  socket_->Close();
 }
 
 void AudioInputDevice::FireCaptureCallback() {