Fix failure of reading zero-byte files on Drive via StreamReader.

HttpByteRange by its nature represents ranges in inclusive format.
We should avoid using it during computation that might involve
empty ranges. In the old code, [0-*].ComputeBounds(0) was failing.

BUG=329328

Review URL: https://codereview.chromium.org/93883006

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

chrome/browser/chromeos/drive/drive_file_stream_reader.cc

@@ -35,6 +35,31 @@ void RunTaskOnUIThread(const base::Closure& task) {
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI), task);
 }
 
+// Computes the concrete |start| offset and the |length| of |range| in a file
+// of |total| size.
+//
+// This is a thin wrapper of HttpByteRange::ComputeBounds, extended to allow
+// an empty range at the end of the file, like "Range: bytes 0-" on a zero byte
+// file. This is for convenience in unifying implementation with the seek
+// operation of stream reader. HTTP doesn't allow such ranges but we want to
+// treat such seeking as valid.
+bool ComputeConcretePosition(net::HttpByteRange range, int64 total,
+                             int64* start, int64* length) {
+  // The special case when empty range in the end of the file is selected.
+  if (range.HasFirstBytePosition() && range.first_byte_position() == total) {
+    *start = range.first_byte_position();
+    *length = 0;
+    return true;
+  }
+
+  // Otherwise forward to HttpByteRange::ComputeBounds.
+  if (!range.ComputeBounds(total))
+    return false;
+  *start = range.first_byte_position();
+  *length = range.last_byte_position() - range.first_byte_position() + 1;
+  return true;
+}
+
 }  // namespace
 
 namespace internal {
@@ -339,7 +364,7 @@ int DriveFileStreamReader::Read(net::IOBuffer* buffer, int buffer_length,
 }
 
 void DriveFileStreamReader::InitializeAfterGetFileContentInitialized(
-    const net::HttpByteRange& in_byte_range,
+    const net::HttpByteRange& byte_range,
     const InitializeCompletionCallback& callback,
     FileError error,
     scoped_ptr<ResourceEntry> entry,
@@ -353,8 +378,9 @@ void DriveFileStreamReader::InitializeAfterGetFileContentInitialized(
   }
   DCHECK(entry);
 
-  net::HttpByteRange byte_range = in_byte_range;
-  if (!byte_range.ComputeBounds(entry->file_info().size())) {
+  int64 range_start = 0, range_length = 0;
+  if (!ComputeConcretePosition(byte_range, entry->file_info().size(),
+                               &range_start, &range_length)) {
     // If |byte_range| is invalid (e.g. out of bounds), return with an error.
     // At the same time, we cancel the in-flight downloading operation if
     // needed and and invalidate weak pointers so that we won't
@@ -367,17 +393,12 @@ void DriveFileStreamReader::InitializeAfterGetFileContentInitialized(
     return;
   }
 
-  // Note: both boundary of |byte_range| are inclusive.
-  int64 range_length =
-      byte_range.last_byte_position() - byte_range.first_byte_position() + 1;
-  DCHECK_GE(range_length, 0);
-
   if (local_cache_file_path.empty()) {
     // The file is not cached, and being downloaded.
     DCHECK(!ui_cancel_download_closure.is_null());
     reader_proxy_.reset(
         new internal::NetworkReaderProxy(
-            byte_range.first_byte_position(), range_length,
+            range_start, range_length,
             base::Bind(&RunTaskOnUIThread, ui_cancel_download_closure)));
     callback.Run(net::OK, entry.Pass());
     return;
@@ -389,7 +410,7 @@ void DriveFileStreamReader::InitializeAfterGetFileContentInitialized(
   util::LocalFileReader* file_reader_ptr = file_reader.get();
   file_reader_ptr->Open(
       local_cache_file_path,
-      byte_range.first_byte_position(),
+      range_start,
       base::Bind(
           &DriveFileStreamReader::InitializeAfterLocalFileOpen,
           weak_ptr_factory_.GetWeakPtr(),

chrome/browser/chromeos/drive/drive_file_stream_reader.h

@@ -175,7 +175,7 @@ class DriveFileStreamReader {
   // Part of Initialize. Called after GetFileContent's initialization
   // is done.
   void InitializeAfterGetFileContentInitialized(
-      const net::HttpByteRange& in_byte_range,
+      const net::HttpByteRange& byte_range,
       const InitializeCompletionCallback& callback,
       FileError error,
       scoped_ptr<ResourceEntry> entry,

chrome/browser/chromeos/drive/drive_file_stream_reader_unittest.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/chromeos/drive/test_util.h"
 #include "chrome/browser/drive/fake_drive_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "google_apis/drive/gdata_wapi_parser.h"
 #include "google_apis/drive/test_util.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -482,4 +483,80 @@ TEST_F(DriveFileStreamReaderTest, OutOfRangeError) {
   EXPECT_FALSE(entry);
 }
 
+TEST_F(DriveFileStreamReaderTest, ZeroByteFileRead) {
+  // Prepare an empty file
+  {
+    google_apis::GDataErrorCode error = google_apis::GDATA_OTHER_ERROR;
+    scoped_ptr<google_apis::ResourceEntry> entry;
+    fake_drive_service_->AddNewFile(
+        "text/plain",
+        "",  // empty
+        fake_drive_service_->GetRootResourceId(),
+        "EmptyFile.txt",
+        false,  // shared_with_me
+        google_apis::test_util::CreateCopyResultCallback(&error, &entry));
+    drive::test_util::RunBlockingPoolTask();
+    ASSERT_EQ(google_apis::HTTP_CREATED, error);
+    ASSERT_TRUE(entry);
+    ASSERT_EQ(0, entry->file_size());
+  }
+
+  const base::FilePath kDriveFile =
+      util::GetDriveMyDriveRootPath().AppendASCII("EmptyFile.txt");
+  // Create the reader, and initialize it.
+  // In this case, the file is not yet locally cached.
+  scoped_ptr<DriveFileStreamReader> reader(new DriveFileStreamReader(
+      GetFileSystemGetter(), worker_thread_->message_loop_proxy().get()));
+  EXPECT_FALSE(reader->IsInitialized());
+
+  int error = net::ERR_FAILED;
+  scoped_ptr<ResourceEntry> entry;
+  {
+    base::RunLoop run_loop;
+    reader->Initialize(
+        kDriveFile,
+        net::HttpByteRange(),
+        google_apis::test_util::CreateQuitCallback(
+            &run_loop,
+            google_apis::test_util::CreateCopyResultCallback(&error, &entry)));
+    run_loop.Run();
+  }
+  EXPECT_EQ(net::OK, error);
+  ASSERT_TRUE(entry);
+  ASSERT_EQ(0u, entry->file_info().size());  // It's a zero-byte file.
+  EXPECT_TRUE(reader->IsInitialized());
+
+  // Read data from the reader. Check that it successfuly reads empty data.
+  std::string first_content;
+  ASSERT_EQ(net::OK, test_util::ReadAllData(reader.get(), &first_content));
+  EXPECT_EQ(0u, first_content.size());
+
+  // Create second instance and initialize it.
+  // In this case, the file should be cached one.
+  reader.reset(new DriveFileStreamReader(
+      GetFileSystemGetter(), worker_thread_->message_loop_proxy().get()));
+  EXPECT_FALSE(reader->IsInitialized());
+
+  error = net::ERR_FAILED;
+  entry.reset();
+  {
+    base::RunLoop run_loop;
+    reader->Initialize(
+        kDriveFile,
+        net::HttpByteRange(),
+        google_apis::test_util::CreateQuitCallback(
+            &run_loop,
+            google_apis::test_util::CreateCopyResultCallback(&error, &entry)));
+    run_loop.Run();
+  }
+  EXPECT_EQ(net::OK, error);
+  ASSERT_TRUE(entry);
+  EXPECT_TRUE(reader->IsInitialized());
+
+  // Read data from the reader, again.
+  std::string second_content;
+  ASSERT_EQ(net::OK, test_util::ReadAllData(reader.get(), &second_content));
+  EXPECT_EQ(0u, second_content.size());
+}
+
 }  // namespace drive