Use separate WaitableEvents in BlockingUrlProtocol for signalling abort versus read complete.

Previously two separate threads were updating last_bytes_read_ in an unsafe manner.


Review URL: https://chromiumcodereview.appspot.com/11360237

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

3 files changed, 20 insertions, 30 deletions

diff --git a/media/filters/blocking_url_protocol.cc b/media/filters/blocking_url_protocol.cc
index ceb5d71..edb7e96 100644
--- a/media/filters/blocking_url_protocol.cc
+++ b/media/filters/blocking_url_protocol.cc
@@ -15,8 +15,8 @@ BlockingUrlProtocol::BlockingUrlProtocol(
     const base::Closure& error_cb)
     : data_source_(data_source),
       error_cb_(error_cb),
-      read_event_(false, false),
-      read_has_failed_(false),
+      aborted_(true, false),  // We never want to reset |aborted_|.
+      read_complete_(false, false),
       last_read_bytes_(0),
       read_position_(0) {
 }
@@ -24,12 +24,12 @@ BlockingUrlProtocol::BlockingUrlProtocol(
 BlockingUrlProtocol::~BlockingUrlProtocol() {}
 
 void BlockingUrlProtocol::Abort() {
-  SignalReadCompleted(DataSource::kReadError);
+  aborted_.Signal();
 }
 
 int BlockingUrlProtocol::Read(int size, uint8* data) {
   // Read errors are unrecoverable.
-  if (read_has_failed_)
+  if (aborted_.IsSignaled())
     return AVERROR(EIO);
 
   // Even though FFmpeg defines AVERROR_EOF, it's not to be used with I/O
@@ -38,16 +38,21 @@ int BlockingUrlProtocol::Read(int size, uint8* data) {
   if (data_source_->GetSize(&file_size) && read_position_ >= file_size)
     return 0;
 
-  // Blocking read from data source until |last_read_bytes_| is set and event is
-  // signalled.
+  // Blocking read from data source until either:
+  //   1) |last_read_bytes_| is set and |read_complete_| is signalled
+  //   2) |aborted_| is signalled
   data_source_->Read(read_position_, size, data, base::Bind(
       &BlockingUrlProtocol::SignalReadCompleted, base::Unretained(this)));
-  read_event_.Wait();
+
+  base::WaitableEvent* events[] = { &aborted_, &read_complete_ };
+  size_t index = base::WaitableEvent::WaitMany(events, arraysize(events));
+
+  if (events[index] == &aborted_)
+    return AVERROR(EIO);
 
   if (last_read_bytes_ == DataSource::kReadError) {
-    // TODO(scherkus): We shouldn't fire |error_cb_| if it was due to Abort().
+    aborted_.Signal();
     error_cb_.Run();
-    read_has_failed_ = true;
     return AVERROR(EIO);
   }
 
@@ -81,7 +86,7 @@ bool BlockingUrlProtocol::IsStreaming() {
 
 void BlockingUrlProtocol::SignalReadCompleted(int size) {
   last_read_bytes_ = size;
-  read_event_.Signal();
+  read_complete_.Signal();
 }
 
 }  // namespace media
diff --git a/media/filters/blocking_url_protocol.h b/media/filters/blocking_url_protocol.h
index dbffbbc..8eb8ebc 100644
--- a/media/filters/blocking_url_protocol.h
+++ b/media/filters/blocking_url_protocol.h
@@ -16,9 +16,6 @@ class DataSource;
 
 // An implementation of FFmpegURLProtocol that blocks until the underlying
 // asynchronous DataSource::Read() operation completes.
-//
-// TODO(scherkus): Should be more thread-safe as |last_read_bytes_| is updated
-// from multiple threads without any protection.
 class MEDIA_EXPORT BlockingUrlProtocol : public FFmpegURLProtocol {
  public:
   // Implements FFmpegURLProtocol using the given |data_source|. |error_cb| is
@@ -32,8 +29,6 @@ class MEDIA_EXPORT BlockingUrlProtocol : public FFmpegURLProtocol {
 
   // Aborts any pending reads by returning a read error. After this method
   // returns all subsequent calls to Read() will immediately fail.
-  //
-  // TODO(scherkus): Currently this will cause |error_cb| to fire. Fix.
   void Abort();
 
   // FFmpegURLProtocol implementation.
@@ -51,13 +46,9 @@ class MEDIA_EXPORT BlockingUrlProtocol : public FFmpegURLProtocol {
   scoped_refptr<DataSource> data_source_;
   base::Closure error_cb_;
 
-  // Used to convert an asynchronous DataSource::Read() into a blocking
-  // FFmpegUrlProtocol::Read().
-  base::WaitableEvent read_event_;
-
-  // Read errors and aborts are unrecoverable. Any subsequent reads will
-  // immediately fail when this is set to true.
-  bool read_has_failed_;
+  // Used to unblock the thread during shutdown and when reads complete.
+  base::WaitableEvent aborted_;
+  base::WaitableEvent read_complete_;
 
   // Cached number of bytes last read from the data source.
   int last_read_bytes_;
diff --git a/media/filters/blocking_url_protocol_unittest.cc b/media/filters/blocking_url_protocol_unittest.cc
index 8bd30c1..be33557 100644
--- a/media/filters/blocking_url_protocol_unittest.cc
+++ b/media/filters/blocking_url_protocol_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/synchronization/waitable_event.h"
+#include "media/base/test_data_util.h"
 #include "media/filters/blocking_url_protocol.h"
 #include "media/filters/file_data_source.h"
 #include "media/ffmpeg/ffmpeg_common.h"
@@ -18,15 +19,8 @@ namespace media {
 class BlockingUrlProtocolTest : public testing::Test {
  public:
   BlockingUrlProtocolTest() {
-    FilePath file_path;
-    EXPECT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &file_path));
-    file_path = file_path.Append(FILE_PATH_LITERAL("media"))
-        .Append(FILE_PATH_LITERAL("test"))
-        .Append(FILE_PATH_LITERAL("data"))
-        .AppendASCII("bear-320x240.webm");
-
     data_source_ = new FileDataSource();
-    CHECK(data_source_->Initialize(file_path.MaybeAsASCII()));
+    CHECK(data_source_->Initialize(GetTestDataURL("bear-320x240.webm")));
 
     url_protocol_.reset(new BlockingUrlProtocol(data_source_, base::Bind(
         &BlockingUrlProtocolTest::OnDataSourceError, base::Unretained(this))));