Fix a race condition in MediaStreamAudioTrackHost.

content/renderer/pepper/pepper_media_stream_audio_track_host.cc

 // Copyright 2014 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/pepper/pepper_media_stream_audio_track_host.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ skipping 47 matching lines @@
 }
 
 }  // namespace
 
 namespace content {
 
 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),
       bytes_per_second_(0) {}
 
 PepperMediaStreamAudioTrackHost::AudioSink::~AudioSink() {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::EnqueueBuffer(int32_t index) {
@@ skipping 18 matching lines @@
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::SetFormatOnMainThread(
     int bytes_per_second) {
   bytes_per_second_ = bytes_per_second;
   InitBuffers();
 }
 
 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);
     buffers_.push_back(index);
   }
 }
 
 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,
                                                         int sample_rate,
                                                         int number_of_channels,
                                                         int number_of_frames) {
   DCHECK(audio_thread_checker_.CalledOnValidThread());
   DCHECK(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);
     buffer->header.size = host_->buffer_manager()->buffer_size();
     buffer->header.type = ppapi::MediaStreamBuffer::TYPE_AUDIO;
     buffer->timestamp = timestamp_.InMillisecondsF();
     buffer->sample_rate = static_cast<PP_AudioBuffer_SampleRate>(sample_rate);
     buffer->number_of_channels = number_of_channels;
     buffer->number_of_samples = number_of_channels * number_of_frames;
     buffer->data_size = buffer_data_size_;
     memcpy(buffer->data, audio_data, buffer_data_size_);
 
     main_message_loop_proxy_->PostTask(
         FROM_HERE,
         base::Bind(&AudioSink::SendEnqueueBufferMessageOnMainThread,
                    weak_factory_.GetWeakPtr(),
-                   index));
+                   index,
+                   init_buffers_count_));
   }
   timestamp_ += buffer_duration_;
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::OnSetFormat(
     const AudioParameters& params) {
   DCHECK(params.IsValid());
   DCHECK_LE(params.GetBufferDuration().InMilliseconds(), kMaxDuration);
   DCHECK_EQ(params.bits_per_sample(), 16);
   DCHECK_NE(GetPPSampleRate(params.sample_rate()),
@@ skipping 78 matching lines @@
 }
 
 void PepperMediaStreamAudioTrackHost::DidConnectPendingHostToResource() {
   if (!connected_) {
     MediaStreamAudioSink::AddToAudioTrack(&audio_sink_, track_);
     connected_ = true;
   }
 }
 
 }  // namespace content