Fix BufferedResourceLoader so it only makes Range requests when a subset of the file is needed.

BUG=74975
TEST=none

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

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

5 files changed, 111 insertions, 50 deletions

diff --git a/webkit/glue/media/buffered_data_source.cc b/webkit/glue/media/buffered_data_source.cc
index 03b2a0a..8aafd26 100644
--- a/webkit/glue/media/buffered_data_source.cc
+++ b/webkit/glue/media/buffered_data_source.cc
@@ -458,7 +458,6 @@ void BufferedDataSource::HttpInitialStartCallback(int error) {
   DCHECK(loader_.get());
 
   int64 instance_size = loader_->instance_size();
-  bool partial_response = loader_->partial_response();
   bool success = error == net::OK;
 
   if (!initialize_callback_.get()) {
@@ -471,7 +470,8 @@ void BufferedDataSource::HttpInitialStartCallback(int error) {
     // request or their partial response is not complete.
     total_bytes_ = instance_size;
     loaded_ = false;
-    streaming_ = (instance_size == kPositionNotSpecified) || !partial_response;
+    streaming_ = (instance_size == kPositionNotSpecified) ||
+        !loader_->range_supported();
   } else {
     // TODO(hclam): In case of failure, we can retry several times.
     loader_->Stop();
@@ -572,11 +572,8 @@ void BufferedDataSource::PartialReadStartCallback(int error) {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(loader_.get());
 
-  // This callback method is invoked after we have verified the server has
-  // range request capability, so as a safety guard verify again the response
-  // is partial.
-  if (error == net::OK && loader_->partial_response()) {
-    // Once the range request has started successfully, we can proceed with
+  if (error == net::OK) {
+    // Once the request has started successfully, we can proceed with
     // reading from it.
     ReadInternal();
     return;
@@ -631,6 +628,14 @@ void BufferedDataSource::ReadCallback(int error) {
     // If a position error code is received, read was successful. So copy
     // from intermediate read buffer to the target read buffer.
     memcpy(read_buffer_, intermediate_read_buffer_.get(), error);
+  } else if (error == 0 && total_bytes_ == kPositionNotSpecified) {
+    // We've reached the end of the file and we didn't know the total size
+    // before. Update the total size so Read()s past the end of the file will
+    // fail like they would if we had known the file size at the beginning.
+    total_bytes_ = loader_->instance_size();
+
+    if (host() && total_bytes_ != kPositionNotSpecified)
+      host()->SetTotalBytes(total_bytes_);
   }
   DoneRead_Locked(error);
 }
diff --git a/webkit/glue/media/buffered_data_source_unittest.cc b/webkit/glue/media/buffered_data_source_unittest.cc
index dd5c891..528a66f 100644
--- a/webkit/glue/media/buffered_data_source_unittest.cc
+++ b/webkit/glue/media/buffered_data_source_unittest.cc
@@ -76,7 +76,7 @@ class MockBufferedResourceLoader : public BufferedResourceLoader {
                           net::CompletionCallback* callback));
   MOCK_METHOD0(content_length, int64());
   MOCK_METHOD0(instance_size, int64());
-  MOCK_METHOD0(partial_response, bool());
+  MOCK_METHOD0(range_supported, bool());
   MOCK_METHOD0(network_activity, bool());
   MOCK_METHOD0(url, const GURL&());
   MOCK_METHOD0(GetBufferedFirstBytePosition, int64());
@@ -164,8 +164,11 @@ class BufferedDataSourceTest : public testing::Test {
 
     ON_CALL(*loader_, instance_size())
         .WillByDefault(Return(instance_size));
-    ON_CALL(*loader_, partial_response())
+
+    // range_supported() return true if we expect to get a partial response.
+    ON_CALL(*loader_, range_supported())
         .WillByDefault(Return(partial_response));
+
     ON_CALL(*loader_, url())
         .WillByDefault(ReturnRef(gurl_));
     media::PipelineStatus expected_init_status = media::PIPELINE_OK;
@@ -277,7 +280,7 @@ class BufferedDataSourceTest : public testing::Test {
     loader_ = NULL;
   }
 
-  void ReadDataSourceMiss(int64 position, int size) {
+  void ReadDataSourceMiss(int64 position, int size, int start_error) {
     EXPECT_TRUE(loader_);
 
     // 1. Reply with a cache miss for the read.
@@ -298,19 +301,26 @@ class BufferedDataSourceTest : public testing::Test {
 
     // 3. Then the new loader will be started.
     EXPECT_CALL(*new_loader, Start(NotNull(), NotNull(), NotNull()))
-        .WillOnce(DoAll(Assign(&error_, net::OK),
+        .WillOnce(DoAll(Assign(&error_, start_error),
                         Invoke(this,
                                &BufferedDataSourceTest::InvokeStartCallback)));
-    EXPECT_CALL(*new_loader, partial_response())
-        .WillRepeatedly(Return(loader_->partial_response()));
 
-    // 4. Then again a read request is made to the new loader.
-    EXPECT_CALL(*new_loader, Read(position, size, NotNull(), NotNull()))
-        .WillOnce(DoAll(Assign(&error_, size),
-                        Invoke(this,
-                               &BufferedDataSourceTest::InvokeReadCallback)));
+    if (start_error == net::OK) {
+      EXPECT_CALL(*new_loader, range_supported())
+          .WillRepeatedly(Return(loader_->range_supported()));
 
-    EXPECT_CALL(*this, ReadCallback(size));
+      // 4a. Then again a read request is made to the new loader.
+      EXPECT_CALL(*new_loader, Read(position, size, NotNull(), NotNull()))
+          .WillOnce(DoAll(Assign(&error_, size),
+                          Invoke(this,
+                                 &BufferedDataSourceTest::InvokeReadCallback)));
+
+      EXPECT_CALL(*this, ReadCallback(size));
+    } else {
+      // 4b. The read callback is called with an error because Start() on the
+      // new loader returned an error.
+      EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
+    }
 
     data_source_->Read(
         position, size, buffer_,
@@ -318,7 +328,8 @@ class BufferedDataSourceTest : public testing::Test {
     message_loop_->RunAllPending();
 
     // Make sure data is correct.
-    EXPECT_EQ(0, memcmp(buffer_, data_ + static_cast<int>(position), size));
+    if (start_error == net::OK)
+      EXPECT_EQ(0, memcmp(buffer_, data_ + static_cast<int>(position), size));
 
     loader_ = new_loader;
   }
@@ -367,8 +378,8 @@ class BufferedDataSourceTest : public testing::Test {
         .WillOnce(DoAll(Assign(&error_, net::OK),
                         Invoke(this,
                                &BufferedDataSourceTest::InvokeStartCallback)));
-    EXPECT_CALL(*new_loader, partial_response())
-        .WillRepeatedly(Return(loader_->partial_response()));
+    EXPECT_CALL(*new_loader, range_supported())
+        .WillRepeatedly(Return(loader_->range_supported()));
 
     // 4. Then again a read request is made to the new loader.
     EXPECT_CALL(*new_loader, Read(position, size, NotNull(), NotNull()))
@@ -459,8 +470,17 @@ TEST_F(BufferedDataSourceTest, ReadCacheHit) {
 
 TEST_F(BufferedDataSourceTest, ReadCacheMiss) {
   InitializeDataSource(kHttpUrl, net::OK, true, 1024, LOADING);
-  ReadDataSourceMiss(1000, 10);
-  ReadDataSourceMiss(20, 10);
+  ReadDataSourceMiss(1000, 10, net::OK);
+  ReadDataSourceMiss(20, 10, net::OK);
+  StopDataSource();
+}
+
+// Test the case where the initial response from the server indicates that
+// Range requests are supported, but a later request prove otherwise.
+TEST_F(BufferedDataSourceTest, ServerLiesAboutRangeSupport) {
+  InitializeDataSource(kHttpUrl, net::OK, true, 1024, LOADING);
+  ReadDataSourceHit(10, 10, 10);
+  ReadDataSourceMiss(1000, 10, net::ERR_INVALID_RESPONSE);
   StopDataSource();
 }
 
diff --git a/webkit/glue/media/buffered_resource_loader.cc b/webkit/glue/media/buffered_resource_loader.cc
index 0c54fad..b19fc92 100644
--- a/webkit/glue/media/buffered_resource_loader.cc
+++ b/webkit/glue/media/buffered_resource_loader.cc
@@ -53,7 +53,7 @@ BufferedResourceLoader::BufferedResourceLoader(
       defer_strategy_(kReadThenDefer),
       completed_(false),
       range_requested_(false),
-      partial_response_(false),
+      range_supported_(false),
       url_(url),
       first_byte_position_(first_byte_position),
       last_byte_position_(last_byte_position),
@@ -90,7 +90,6 @@ void BufferedResourceLoader::Start(net::CompletionCallback* start_callback,
   event_callback_.reset(event_callback);
 
   if (first_byte_position_ != kPositionNotSpecified) {
-    range_requested_ = true;
     // TODO(hclam): server may not support range request so |offset_| may not
     // equal to |first_byte_position_|.
     offset_ = first_byte_position_;
@@ -103,10 +102,14 @@ void BufferedResourceLoader::Start(net::CompletionCallback* start_callback,
   // Prepare the request.
   WebURLRequest request(url_);
   request.setTargetType(WebURLRequest::TargetIsMedia);
-  request.setHTTPHeaderField(WebString::fromUTF8("Range"),
-                             WebString::fromUTF8(GenerateHeaders(
-                                                 first_byte_position_,
-                                                 last_byte_position_)));
+
+  if (!IsWholeFileRange()) {
+    range_requested_ = true;
+    request.setHTTPHeaderField(WebString::fromUTF8("Range"),
+                               WebString::fromUTF8(GenerateHeaders(
+                                   first_byte_position_,
+                                   last_byte_position_)));
+  }
   frame->setReferrerForRequest(request, WebKit::WebURL());
 
   // This flag is for unittests as we don't want to reset |url_loader|
@@ -212,8 +215,8 @@ int64 BufferedResourceLoader::instance_size() {
   return instance_size_;
 }
 
-bool BufferedResourceLoader::partial_response() {
-  return partial_response_;
+bool BufferedResourceLoader::range_supported() {
+  return range_supported_;
 }
 
 bool BufferedResourceLoader::network_activity() {
@@ -274,6 +277,8 @@ void BufferedResourceLoader::didReceiveResponse(
   if (!start_callback_.get())
     return;
 
+  bool partial_response = false;
+
   // We make a strong assumption that when we reach here we have either
   // received a response from HTTP/HTTPS protocol or the request was
   // successful (in particular range request). So we only verify the partial
@@ -281,13 +286,17 @@ void BufferedResourceLoader::didReceiveResponse(
   if (url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme)) {
     int error = net::OK;
 
-    if (response.httpStatusCode() == kHttpPartialContent)
-      partial_response_ = true;
+    // Check to see whether the server supports byte ranges.
+    std::string accept_ranges =
+        response.httpHeaderField("Accept-Ranges").utf8();
+    range_supported_ = (accept_ranges.find("bytes") != std::string::npos);
 
-    if (range_requested_ && partial_response_) {
+    partial_response = (response.httpStatusCode() == kHttpPartialContent);
+
+    if (range_requested_) {
       // If we have verified the partial response and it is correct, we will
       // return net::OK.
-      if (!VerifyPartialResponse(response))
+      if (!partial_response || !VerifyPartialResponse(response))
         error = net::ERR_INVALID_RESPONSE;
     } else if (response.httpStatusCode() != kHttpOK) {
       // We didn't request a range but server didn't reply with "200 OK".
@@ -302,7 +311,7 @@ void BufferedResourceLoader::didReceiveResponse(
   } else {
     // For any protocol other than HTTP and HTTPS, assume range request is
     // always fulfilled.
-    partial_response_ = range_requested_;
+    partial_response = range_requested_;
   }
 
   // Expected content length can be |kPositionNotSpecified|, in that case
@@ -311,7 +320,7 @@ void BufferedResourceLoader::didReceiveResponse(
 
   // If we have not requested a range, then the size of the instance is equal
   // to the content length.
-  if (!partial_response_)
+  if (!partial_response)
     instance_size_ = content_length_;
 
   // Calls with a successful response.
@@ -372,6 +381,11 @@ void BufferedResourceLoader::didFinishLoading(
   DCHECK(!completed_);
   completed_ = true;
 
+  // If we didn't know the |instance_size_| we do now.
+  if (instance_size_ == kPositionNotSpecified) {
+    instance_size_ = offset_ + buffer_->forward_bytes();
+  }
+
   // If there is a start callback, calls it.
   if (start_callback_.get()) {
     DoneStart(net::OK);
@@ -629,4 +643,10 @@ void BufferedResourceLoader::NotifyNetworkEvent() {
     event_callback_->Run();
 }
 
+bool BufferedResourceLoader::IsWholeFileRange() const {
+  return ((first_byte_position_ == kPositionNotSpecified ||
+           first_byte_position_ == 0) &&
+          last_byte_position_ == kPositionNotSpecified);
+}
+
 }  // namespace webkit_glue
diff --git a/webkit/glue/media/buffered_resource_loader.h b/webkit/glue/media/buffered_resource_loader.h
index 9e0c870..2d43fb8 100644
--- a/webkit/glue/media/buffered_resource_loader.h
+++ b/webkit/glue/media/buffered_resource_loader.h
@@ -103,9 +103,8 @@ class BufferedResourceLoader :
   // |kPositionNotSpecified|, then the size is unknown.
   virtual int64 instance_size();
 
-  // Returns true if the response for this loader is a partial response.
-  // It means a 206 response in HTTP/HTTPS protocol.
-  virtual bool partial_response();
+  // Returns true if the server supports byte range requests.
+  virtual bool range_supported();
 
   // Returns true if network is currently active.
   virtual bool network_activity();
@@ -208,6 +207,10 @@ class BufferedResourceLoader :
 
   bool HasPendingRead() { return read_callback_.get() != NULL; }
 
+  // Helper function that returns true if a range is not provided
+  // or a 0- range is specified.
+  bool IsWholeFileRange() const;
+
   // A sliding window of buffer.
   scoped_ptr<media::SeekableBuffer> buffer_;
 
@@ -223,8 +226,8 @@ class BufferedResourceLoader :
   // True if a range request was made.
   bool range_requested_;
 
-  // True if response data received is a partial range.
-  bool partial_response_;
+  // True if Range header is supported.
+  bool range_supported_;
 
   // Does the work of loading and sends data back to this client.
   scoped_ptr<WebKit::WebURLLoader> url_loader_;
diff --git a/webkit/glue/media/buffered_resource_loader_unittest.cc b/webkit/glue/media/buffered_resource_loader_unittest.cc
index 97cd3c1..3e7dccd 100644
--- a/webkit/glue/media/buffered_resource_loader_unittest.cc
+++ b/webkit/glue/media/buffered_resource_loader_unittest.cc
@@ -103,7 +103,15 @@ class BufferedResourceLoaderTest : public testing::Test {
   }
 
   void FullResponse(int64 instance_size) {
-    EXPECT_CALL(*this, StartCallback(net::OK));
+    FullResponse(instance_size, net::OK);
+  }
+
+  void FullResponse(int64 instance_size, int status) {
+    EXPECT_CALL(*this, StartCallback(status));
+    if (status != net::OK) {
+      EXPECT_CALL(*url_loader_, cancel())
+          .WillOnce(RequestCanceled(loader_));
+    }
 
     WebURLResponse response(gurl_);
     response.setHTTPHeaderField(WebString::fromUTF8("Content-Length"),
@@ -112,9 +120,13 @@ class BufferedResourceLoaderTest : public testing::Test {
     response.setExpectedContentLength(instance_size);
     response.setHTTPStatusCode(kHttpOK);
     loader_->didReceiveResponse(url_loader_, response);
-    EXPECT_EQ(instance_size, loader_->content_length());
-    EXPECT_EQ(instance_size, loader_->instance_size());
-    EXPECT_FALSE(loader_->partial_response());
+
+    if (status == net::OK) {
+      EXPECT_EQ(instance_size, loader_->content_length());
+      EXPECT_EQ(instance_size, loader_->instance_size());
+    }
+
+    EXPECT_FALSE(loader_->range_supported());
   }
 
   void PartialResponse(int64 first_position, int64 last_position,
@@ -123,6 +135,8 @@ class BufferedResourceLoaderTest : public testing::Test {
     int64 content_length = last_position - first_position + 1;
 
     WebURLResponse response(gurl_);
+    response.setHTTPHeaderField(WebString::fromUTF8("Accept-Ranges"),
+                                WebString::fromUTF8("bytes"));
     response.setHTTPHeaderField(WebString::fromUTF8("Content-Range"),
                                 WebString::fromUTF8(base::StringPrintf("bytes "
                                             "%" PRId64 "-%" PRId64 "/%" PRId64,
@@ -134,7 +148,7 @@ class BufferedResourceLoaderTest : public testing::Test {
     loader_->didReceiveResponse(url_loader_, response);
     EXPECT_EQ(content_length, loader_->content_length());
     EXPECT_EQ(instance_size, loader_->instance_size());
-    EXPECT_TRUE(loader_->partial_response());
+    EXPECT_TRUE(loader_->range_supported());
   }
 
   void Redirect(const char* url) {
@@ -224,8 +238,7 @@ TEST_F(BufferedResourceLoaderTest, BadHttpResponse) {
 TEST_F(BufferedResourceLoaderTest, NotPartialResponse) {
   Initialize(kHttpUrl, 100, -1);
   Start();
-  FullResponse(1024);
-  StopWhenLoad();
+  FullResponse(1024, net::ERR_INVALID_RESPONSE);
 }
 
 // Tests that a 200 response is received.