Fix a crash in BufferedDataSource

This crash is caused by a race condition when the media
pipeline is shutting down.
Here is a sequence that leads to crash:
1. PipelineImpl::Stop() is called.
2. BufferedDataSource::Stop() is called on pipeline thread.
   BufferedDataSource::StopTask() is posted on render thread.
3. BufferedDataSource::StopTask() is executed.
4. BufferedDataSource::Read() is called on demuxer thread.
5. BufferedDataSource::ReadTask() is executed. *bang*

The cause of the crash is the involvement of three threads
and it happens that ReadTask() is executed after StopTask().
Since we cannot prevent BufferedDataSource::Read() being
called by FFmpegDemuxer since the stop signal hasn't arrived
at the demuxer yet. This change will suppress activity of
data source after the stop task executed.

I didn't reuse the |stopped_| variable to suppress activity
but instead introduce another |stop_task_executed_| signal
because |stopped_| is prevent the data source  from making
callbacks to the demuxer after stopped has received. And by
doing this we don't need to introduce an additional critical
section which is not desirable.

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@22128 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 19 insertions, 2 deletions

diff --git a/webkit/glue/media/buffered_data_source.cc b/webkit/glue/media/buffered_data_source.cc
index 738f30d..4dd9287 100644
--- a/webkit/glue/media/buffered_data_source.cc
+++ b/webkit/glue/media/buffered_data_source.cc
@@ -453,7 +453,8 @@ BufferedDataSource::BufferedDataSource(
       intermediate_read_buffer_(new uint8[kInitialReadBufferSize]),
       intermediate_read_buffer_size_(kInitialReadBufferSize),
       render_loop_(render_loop),
-      stopped_(false) {
+      stopped_(false),
+      stop_task_finished_(false) {
 }
 
 BufferedDataSource::~BufferedDataSource() {
@@ -568,6 +569,14 @@ void BufferedDataSource::ReadTask(
      int64 position, int read_size, uint8* buffer,
      media::DataSource::ReadCallback* read_callback) {
   DCHECK(MessageLoop::current() == render_loop_);
+
+  // If StopTask() was executed we should return immediately. We check this
+  // variable to prevent doing any actual work after clean up was done. We do
+  // not check |stopped_| because anything use of it has to be within |lock_|
+  // which is not desirable.
+  if (stop_task_finished_)
+    return;
+
   DCHECK(!read_callback_.get());
   DCHECK(read_callback);
 
@@ -604,6 +613,9 @@ void BufferedDataSource::StopTask() {
   read_buffer_ = 0;
   read_submitted_time_ = base::Time();
   read_attempts_ = 0;
+
+  // Signal that stop task has finished execution.
+  stop_task_finished_ = true;
 }
 
 void BufferedDataSource::SwapLoaderTask(BufferedResourceLoader* loader) {
diff --git a/webkit/glue/media/buffered_data_source.h b/webkit/glue/media/buffered_data_source.h
index ef46477..ea6736b 100644
--- a/webkit/glue/media/buffered_data_source.h
+++ b/webkit/glue/media/buffered_data_source.h
@@ -330,9 +330,14 @@ class BufferedDataSource : public media::DataSource {
   // Protects |stopped_|.
   Lock lock_;
 
-  // Stop signal to suppressing activities.
+  // Stop signal to suppressing activities. This variable is set on the pipeline
+  // thread and read from the render thread.
   bool stopped_;
 
+  // This variable is set by StopTask() and read from ReadTask(). It is used to
+  // prevent ReadTask() from doing anything after StopTask() is executed.
+  bool stop_task_finished_;
+
   // This timer is to run the WatchDogTask repeatedly. We use a timer instead
   // of doing PostDelayedTask() reduce the extra reference held by the message
   // loop. The RepeatingTimer does PostDelayedTask() internally, by using it