summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorhubbe <hubbe@chromium.org>2016-09-21 07:17:46 (GMT)
committerCommit bot <commit-bot@chromium.org>2016-09-21 07:20:32 (GMT)
commit3b2a5629da155db59d3449a6d640c107e91aa14d (patch)
tree6d3d73e63c5967301cba261b5482d871a5b50882
parent623498505e8ba7742b7aa5ba56b4f3933695b66c (diff)
downloadchromium-3b2a5629da155db59d3449a6d640c107e91aa14d.tar.gz
chromium-3b2a5629da155db59d3449a6d640c107e91aa14d.tar.xz
Fix a timing bug in multibuffer.
When "cancel on defer is on", the multibuffer reader can sometimes get delete before it is allowed to deliver a pending read callback. This basically hangs the multibuffer data source as it waits for the read callback forever. BUG=648395, 647867, 646434 Review-Url: https://codereview.chromium.org/2357773003 Cr-Commit-Position: refs/heads/master@{#419995}
-rw-r--r--media/blink/multibuffer_data_source.cc14
-rw-r--r--media/blink/multibuffer_data_source.h3
-rw-r--r--media/blink/multibuffer_data_source_unittest.cc36
3 files changed, 46 insertions, 7 deletions
diff --git a/media/blink/multibuffer_data_source.cc b/media/blink/multibuffer_data_source.cc
index a343fee..1390c69 100644
--- a/media/blink/multibuffer_data_source.cc
+++ b/media/blink/multibuffer_data_source.cc
@@ -416,7 +416,7 @@ void MultibufferDataSource::ReadTask() {
} else {
reader_->Wait(1, base::Bind(&MultibufferDataSource::ReadTask,
weak_factory_.GetWeakPtr()));
- UpdateLoadingState(false);
+ UpdateLoadingState_Locked(false);
}
}
@@ -507,7 +507,7 @@ void MultibufferDataSource::StartCallback() {
// Even if data is cached, say that we're loading at this point for
// compatibility.
- UpdateLoadingState(true);
+ UpdateLoadingState_Locked(true);
}
void MultibufferDataSource::ProgressCallback(int64_t begin, int64_t end) {
@@ -517,25 +517,29 @@ void MultibufferDataSource::ProgressCallback(int64_t begin, int64_t end) {
if (assume_fully_buffered())
return;
+ base::AutoLock auto_lock(lock_);
+
if (end > begin) {
// TODO(scherkus): we shouldn't have to lock to signal host(), see
// http://crbug.com/113712 for details.
- base::AutoLock auto_lock(lock_);
if (stop_signal_received_)
return;
host_->AddBufferedByteRange(begin, end);
}
- UpdateLoadingState(false);
+ UpdateLoadingState_Locked(false);
}
-void MultibufferDataSource::UpdateLoadingState(bool force_loading) {
+void MultibufferDataSource::UpdateLoadingState_Locked(bool force_loading) {
DVLOG(1) << __func__;
+ lock_.AssertAcquired();
if (assume_fully_buffered())
return;
// Update loading state.
bool is_loading = !!reader_ && reader_->IsLoading();
+ if (read_op_)
+ is_loading = true;
if (force_loading || is_loading != loading_) {
loading_ = is_loading || force_loading;
diff --git a/media/blink/multibuffer_data_source.h b/media/blink/multibuffer_data_source.h
index 73537cb..fe89ca8 100644
--- a/media/blink/multibuffer_data_source.h
+++ b/media/blink/multibuffer_data_source.h
@@ -165,7 +165,8 @@ class MEDIA_BLINK_EXPORT MultibufferDataSource : public DataSource {
// call downloading_cb_ if needed.
// If |force_loading| is true, we call downloading_cb_ and tell it that
// we are currently loading, regardless of what reader_->IsLoading() says.
- void UpdateLoadingState(bool force_loading);
+ // Caller must hold |lock_|.
+ void UpdateLoadingState_Locked(bool force_loading);
// Update |reader_|'s preload and buffer settings.
void UpdateBufferSizes();
diff --git a/media/blink/multibuffer_data_source_unittest.cc b/media/blink/multibuffer_data_source_unittest.cc
index 489047c..91cfe3d 100644
--- a/media/blink/multibuffer_data_source_unittest.cc
+++ b/media/blink/multibuffer_data_source_unittest.cc
@@ -317,7 +317,7 @@ class MultibufferDataSourceTest : public testing::Test {
base::RunLoop().RunUntilIdle();
}
- void ReceiveData(int size) {
+ void ReceiveDataLow(int size) {
EXPECT_TRUE(url_loader());
if (!url_loader())
return;
@@ -325,6 +325,10 @@ class MultibufferDataSourceTest : public testing::Test {
memset(data.get(), 0xA5, size); // Arbitrary non-zero value.
data_provider()->didReceiveData(url_loader(), data.get(), size, size, size);
+ }
+
+ void ReceiveData(int size) {
+ ReceiveDataLow(size);
base::RunLoop().RunUntilIdle();
}
@@ -1259,6 +1263,36 @@ TEST_F(MultibufferDataSourceTest,
EXPECT_FALSE(active_loader_allownull());
}
+// This test tries to trigger an edge case where the read callback
+// never happens because the reader is deleted before that happens.
+TEST_F(MultibufferDataSourceTest,
+ ExternalResource_Response206_CancelAfterDefer2) {
+ set_preload(MultibufferDataSource::METADATA);
+ InitializeWith206Response();
+
+ EXPECT_EQ(MultibufferDataSource::METADATA, preload());
+ EXPECT_FALSE(is_local_source());
+
+ EXPECT_TRUE(data_source_->range_supported());
+ CheckReadThenDefer();
+
+ ReadAt(kDataSize);
+
+ data_source_->OnBufferingHaveEnough(false);
+ ASSERT_TRUE(active_loader());
+
+ EXPECT_CALL(*this, ReadCallback(kDataSize));
+ EXPECT_CALL(host_, AddBufferedByteRange(kDataSize, kDataSize + 2000));
+
+ ReceiveDataLow(2000);
+ EXPECT_CALL(host_, AddBufferedByteRange(0, kDataSize * 2 + 2000));
+ ReceiveDataLow(kDataSize);
+
+ base::RunLoop().RunUntilIdle();
+
+ EXPECT_FALSE(active_loader_allownull());
+}
+
TEST_F(MultibufferDataSourceTest,
ExternalResource_Response206_CancelAfterPlay) {
set_preload(MultibufferDataSource::METADATA);