Fix a race condition in MediaStreamAudioTrackHost.

content/renderer/pepper/pepper_media_stream_audio_track_host.cc

Index: content/renderer/pepper/pepper_media_stream_audio_track_host.cc
diff --git a/content/renderer/pepper/pepper_media_stream_audio_track_host.cc b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
index d972c2c061e2882eb1257d92fc9981e18a296ba5..0e2908841b5ac2fff6b26a6fc91534b8596f3f78 100644
--- a/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
+++ b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
@@ -65,6 +65,7 @@ PepperMediaStreamAudioTrackHost::AudioSink::AudioSink(
     PepperMediaStreamAudioTrackHost* host)
     : host_(host),
       buffer_data_size_(0),
+      init_buffers_count_(0),
       main_message_loop_proxy_(base::MessageLoopProxy::current()),
       weak_factory_(this),
       number_of_buffers_(kDefaultNumberOfBuffers),
@@ -103,19 +104,30 @@ void PepperMediaStreamAudioTrackHost::AudioSink::SetFormatOnMainThread(
 
 void PepperMediaStreamAudioTrackHost::AudioSink::InitBuffers() {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
+  {
+    base::AutoLock lock(lock_);
+    // Clear |buffers_|, so the audio thread will drop all incoming audio data.
+    buffers_.clear();
+    init_buffers_count_++;
+  }
   // The size is slightly bigger than necessary, because 8 extra bytes are
   // added into the struct. Also see |MediaStreamBuffer|.
   base::CheckedNumeric<int32_t> buffer_size = bytes_per_second_;
   buffer_size *= kMaxDuration;
   buffer_size /= base::Time::kMillisecondsPerSecond;
   buffer_size += sizeof(ppapi::MediaStreamBuffer::Audio);
+
+  // We don't need hold |lock_| during |host->InitBuffers()| call, because

[dmichael (off chromium), 2014/07/24 16:42:56]

need +to+ hold

[Peng, 2014/07/25 15:25:31]

Done.

+  // we just cleared |buffers_| , so the audio thread will drop all incoming
+  // audio data, and not use buffers in |host_|.
   bool result = host_->InitBuffers(number_of_buffers_,
                                    buffer_size.ValueOrDie(),
                                    kRead);
   // TODO(penghuang): Send PP_ERROR_NOMEMORY to plugin.
   CHECK(result);
+
+  // Fill the |buffers_|, so the audio thread can continue receiving audio data.
   base::AutoLock lock(lock_);
-  buffers_.clear();
   for (int32_t i = 0; i < number_of_buffers_; ++i) {
     int32_t index = host_->buffer_manager()->DequeueBuffer();
     DCHECK_GE(index, 0);
@@ -124,9 +136,15 @@ void PepperMediaStreamAudioTrackHost::AudioSink::InitBuffers() {
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::
-    SendEnqueueBufferMessageOnMainThread(int32_t index) {
+    SendEnqueueBufferMessageOnMainThread(int32_t index,
+                                         int32_t init_buffers_count) {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
-  host_->SendEnqueueBufferMessageToPlugin(index);
+  // If |InitBuffers()| is called after this task being posted from the audio
+  // thread, the buffer should become invalid already. We should ignore it.
+  // And because only the main thread modifies the |init_buffers_count_|,
+  // so we don't need lock |lock_| here (main thread).

[Anand Mistry (off Chromium), 2014/07/25 00:42:54]

Although I'll agree this is a benign read, I think this will trip up an analysis
tool like asan. Is there a way in Chrome to tag a benign read?

[Peng, 2014/07/25 15:25:31]

I think rwlock is better for this case, but I can not find it in chromium. I
don't know how to tag it in code.

[dmichael (off chromium), 2014/07/28 19:30:43]

I don't think TSAN will flag it; I think it's fine.

+  if (init_buffers_count == init_buffers_count_)
+    host_->SendEnqueueBufferMessageToPlugin(index);
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::OnData(const int16* audio_data,
@@ -138,16 +156,11 @@ void PepperMediaStreamAudioTrackHost::AudioSink::OnData(const int16* audio_data,
   DCHECK_EQ(sample_rate, audio_params_.sample_rate());
   DCHECK_EQ(number_of_channels, audio_params_.channels());
   DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer());
-  int32_t index = -1;
-  {
-    base::AutoLock lock(lock_);
-    if (!buffers_.empty()) {
-      index = buffers_.front();
-      buffers_.pop_front();
-    }
-  }
 
-  if (index != -1) {
+  base::AutoLock lock(lock_);
+  if (!buffers_.empty()) {
+    int index = buffers_.front();
+    buffers_.pop_front();
     // TODO(penghuang): support re-sampling, etc.
     ppapi::MediaStreamBuffer::Audio* buffer =
         &(host_->buffer_manager()->GetBufferPointer(index)->audio);
@@ -164,7 +177,8 @@ void PepperMediaStreamAudioTrackHost::AudioSink::OnData(const int16* audio_data,
         FROM_HERE,
         base::Bind(&AudioSink::SendEnqueueBufferMessageOnMainThread,
                    weak_factory_.GetWeakPtr(),
-                   index));
+                   index,
+                   init_buffers_count_));
   }
   timestamp_ += buffer_duration_;
 }