Report unreadable files as size zero when uploading.

Upload zero bytes if the file size shrinks.

BUG=30850
TEST=uploading an unreadable file works

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

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

6 files changed, 269 insertions, 62 deletions

diff --git a/net/base/upload_data.cc b/net/base/upload_data.cc
index 0496873..198b051 100644
--- a/net/base/upload_data.cc
+++ b/net/base/upload_data.cc
@@ -6,30 +6,42 @@
 
 #include "base/file_util.h"
 #include "base/logging.h"
+#include "net/base/net_errors.h"
 
 namespace net {
 
-uint64 UploadData::GetContentLength() const {
+uint64 UploadData::GetContentLength() {
   uint64 len = 0;
-  std::vector<Element>::const_iterator it = elements_.begin();
+  std::vector<Element>::iterator it = elements_.begin();
   for (; it != elements_.end(); ++it)
     len += (*it).GetContentLength();
   return len;
 }
 
-uint64 UploadData::Element::GetContentLength() const {
-  if (override_content_length_)
+uint64 UploadData::Element::GetContentLength() {
+  if (override_content_length_ || content_length_computed_)
     return content_length_;
 
   if (type_ == TYPE_BYTES)
     return static_cast<uint64>(bytes_.size());
 
-  DCHECK(type_ == TYPE_FILE);
+  DCHECK_EQ(TYPE_FILE, type_);
+  DCHECK(!file_stream_);
 
   // TODO(darin): This size calculation could be out of sync with the state of
   // the file when we get around to reading it.  We should probably find a way
   // to lock the file or somehow protect against this error condition.
 
+  content_length_computed_ = true;
+  content_length_ = 0;
+
+  // We need to open the file here to decide if we should report the file's
+  // size or zero.  We cache the open file, so that we can still read it when
+  // it comes time to.
+  file_stream_ = NewFileStreamForReading();
+  if (!file_stream_)
+    return 0;
+
   int64 length = 0;
   if (!file_util::GetFileSize(file_path_, &length))
     return 0;
@@ -38,7 +50,40 @@ uint64 UploadData::Element::GetContentLength() const {
     return 0;  // range is beyond eof
 
   // compensate for the offset and clip file_range_length_ to eof
-  return std::min(length - file_range_offset_, file_range_length_);
+  content_length_ =  std::min(length - file_range_offset_, file_range_length_);
+  return content_length_;
+}
+
+FileStream* UploadData::Element::NewFileStreamForReading() {
+  // In common usage GetContentLength() will call this first and store the
+  // result into |file_| and a subsequent call (from UploadDataStream) will
+  // get the cached open FileStream.
+  if (file_stream_) {
+    FileStream* file = file_stream_;
+    file_stream_ = NULL;
+    return file;
+  }
+
+  scoped_ptr<FileStream> file(new FileStream());
+  int64 rv = file->Open(file_path_,
+                      base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ);
+  if (rv != OK) {
+    // If the file can't be opened, we'll just upload an empty file.
+    DLOG(WARNING) << "Failed to open \"" << file_path_.value()
+                  << "\" for reading: " << rv;
+    return NULL;
+  }
+  if (file_range_offset_) {
+    rv = file->Seek(FROM_BEGIN, file_range_offset_);
+    if (rv < 0) {
+      DLOG(WARNING) << "Failed to seek \"" << file_path_.value()
+                    << "\" to offset: " << file_range_offset_ << " (" << rv
+                    << ")";
+      return NULL;
+    }
+  }
+
+  return file.release();
 }
 
 }  // namespace net
diff --git a/net/base/upload_data.h b/net/base/upload_data.h
index 869ab68..fe395f0 100644
--- a/net/base/upload_data.h
+++ b/net/base/upload_data.h
@@ -9,7 +9,9 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
+#include "base/logging.h"
 #include "base/ref_counted.h"
+#include "net/base/file_stream.h"
 #include "base/time.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
@@ -28,7 +30,14 @@ class UploadData : public base::RefCounted<UploadData> {
    public:
     Element() : type_(TYPE_BYTES), file_range_offset_(0),
                 file_range_length_(kuint64max),
-                override_content_length_(false) {
+                override_content_length_(false),
+                content_length_computed_(false),
+                file_stream_(NULL) {
+    }
+
+    ~Element() {
+      // In the common case |file__stream_| will be null.
+      delete file_stream_;
     }
 
     Type type() const { return type_; }
@@ -65,7 +74,14 @@ class UploadData : public base::RefCounted<UploadData> {
 
     // Returns the byte-length of the element.  For files that do not exist, 0
     // is returned.  This is done for consistency with Mozilla.
-    uint64 GetContentLength() const;
+    // Once called, this function will always return the same value.
+    uint64 GetContentLength();
+
+    // Returns a FileStream opened for reading for this element, positioned at
+    // |file_range_offset_|.  The caller gets ownership and is responsible
+    // for cleaning up the FileStream. Returns NULL if this element is not of
+    // type TYPE_FILE or if the file is not openable.
+    FileStream* NewFileStreamForReading();
 
    private:
     // Allows tests to override the result of GetContentLength.
@@ -81,7 +97,9 @@ class UploadData : public base::RefCounted<UploadData> {
     uint64 file_range_length_;
     base::Time expected_file_modification_time_;
     bool override_content_length_;
+    bool content_length_computed_;
     uint64 content_length_;
+    FileStream* file_stream_;
 
     FRIEND_TEST(UploadDataStreamTest, FileSmallerThanLength);
     FRIEND_TEST(HttpNetworkTransactionTest, UploadFileSmallerThanLength);
@@ -108,10 +126,10 @@ class UploadData : public base::RefCounted<UploadData> {
   }
 
   // Returns the total size in bytes of the data to upload.
-  uint64 GetContentLength() const;
+  uint64 GetContentLength();
 
-  const std::vector<Element>& elements() const {
-    return elements_;
+  std::vector<Element>* elements() {
+    return &elements_;
   }
 
   void set_elements(const std::vector<Element>& elements) {
diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index a3a67fa..140aa8a 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -11,8 +11,7 @@
 
 namespace net {
 
-UploadDataStream* UploadDataStream::Create(const UploadData* data,
-                                           int* error_code) {
+UploadDataStream* UploadDataStream::Create(UploadData* data, int* error_code) {
   scoped_ptr<UploadDataStream> stream(new UploadDataStream(data));
   int rv = stream->FillBuf();
   if (error_code)
@@ -23,11 +22,11 @@ UploadDataStream* UploadDataStream::Create(const UploadData* data,
   return stream.release();
 }
 
-UploadDataStream::UploadDataStream(const UploadData* data)
+UploadDataStream::UploadDataStream(UploadData* data)
     : data_(data),
       buf_(new IOBuffer(kBufSize)),
       buf_len_(0),
-      next_element_(data->elements().begin()),
+      next_element_(data->elements()->begin()),
       next_element_offset_(0),
       next_element_remaining_(0),
       total_size_(data->GetContentLength()),
@@ -52,13 +51,13 @@ void UploadDataStream::DidConsume(size_t num_bytes) {
 }
 
 int UploadDataStream::FillBuf() {
-  std::vector<UploadData::Element>::const_iterator end =
-      data_->elements().end();
+  std::vector<UploadData::Element>::iterator end =
+      data_->elements()->end();
 
   while (buf_len_ < kBufSize && next_element_ != end) {
     bool advance_to_next_element = false;
 
-    const UploadData::Element& element = *next_element_;
+    UploadData::Element& element = *next_element_;
 
     size_t size_remaining = kBufSize - buf_len_;
     if (element.type() == UploadData::TYPE_BYTES) {
@@ -78,57 +77,51 @@ int UploadDataStream::FillBuf() {
     } else {
       DCHECK(element.type() == UploadData::TYPE_FILE);
 
-      // If the underlying file has been changed, treat it as error.
-      // Note that the expected modification time from WebKit is based on
-      // time_t precision. So we have to convert both to time_t to compare.
-      if (!element.expected_file_modification_time().is_null()) {
-        file_util::FileInfo info;
-        if (file_util::GetFileInfo(element.file_path(), &info) &&
-            element.expected_file_modification_time().ToTimeT() !=
-                info.last_modified.ToTimeT()) {
-          return ERR_UPLOAD_FILE_CHANGED;
-        }
-      }
-
-      if (!next_element_stream_.IsOpen()) {
-        int flags = base::PLATFORM_FILE_OPEN |
-                    base::PLATFORM_FILE_READ;
-        int rv = next_element_stream_.Open(element.file_path(), flags);
-        // If the file does not exist, that's technically okay.. we'll just
-        // upload an empty file.  This is for consistency with Mozilla.
-        DLOG_IF(WARNING, rv != OK) << "Failed to open \""
-                                   << element.file_path().value()
-                                   << "\" for reading: " << rv;
-
-        next_element_remaining_ = 0;  // Default to reading nothing.
-        if (rv == OK) {
-          uint64 offset = element.file_range_offset();
-          if (offset && next_element_stream_.Seek(FROM_BEGIN, offset) < 0) {
-            DLOG(WARNING) << "Failed to seek \"" << element.file_path().value()
-                          << "\" to offset: " << offset;
-          } else {
-            next_element_remaining_ = element.file_range_length();
+      if (!next_element_remaining_) {
+        // If the underlying file has been changed, treat it as error.
+        // Note that the expected modification time from WebKit is based on
+        // time_t precision. So we have to convert both to time_t to compare.
+        if (!element.expected_file_modification_time().is_null()) {
+          file_util::FileInfo info;
+          if (file_util::GetFileInfo(element.file_path(), &info) &&
+              element.expected_file_modification_time().ToTimeT() !=
+                  info.last_modified.ToTimeT()) {
+            return ERR_UPLOAD_FILE_CHANGED;
           }
         }
+        next_element_remaining_ = element.GetContentLength();
+        next_element_stream_.reset(element.NewFileStreamForReading());
       }
 
       int rv = 0;
-      int count = static_cast<int>(std::min(
-          static_cast<uint64>(size_remaining), next_element_remaining_));
-      if (count > 0 &&
-          (rv = next_element_stream_.Read(buf_->data() + buf_len_,
-                                          count, NULL)) > 0) {
+      int count =
+          static_cast<int>(std::min(next_element_remaining_,
+                                    static_cast<uint64>(size_remaining)));
+      if (count > 0) {
+        if (next_element_stream_.get())
+          rv = next_element_stream_->Read(buf_->data() + buf_len_, count, NULL);
+        if (rv <= 0) {
+          // If there's less data to read than we initially observed, then
+          // pad with zero.  Otherwise the server will hang waiting for the
+          // rest of the data.
+          memset(buf_->data() + buf_len_, 0, count);
+          rv = count;
+        }
         buf_len_ += rv;
-        next_element_remaining_ -= rv;
-      } else {
+      }
+
+      if (static_cast<int>(next_element_remaining_) == rv) {
         advance_to_next_element = true;
+      } else {
+        next_element_remaining_ -= rv;
       }
     }
 
     if (advance_to_next_element) {
       ++next_element_;
       next_element_offset_ = 0;
-      next_element_stream_.Close();
+      next_element_remaining_ = 0;
+      next_element_stream_.reset();
     }
   }
 
diff --git a/net/base/upload_data_stream.h b/net/base/upload_data_stream.h
index b326dae..9fac07e 100644
--- a/net/base/upload_data_stream.h
+++ b/net/base/upload_data_stream.h
@@ -17,7 +17,7 @@ class UploadDataStream {
   // Returns a new instance of UploadDataStream if it can be created and
   // initialized successfully. If not, NULL will be returned and the error
   // code will be set if the output parameter error_code is not empty.
-  static UploadDataStream* Create(const UploadData* data, int* error_code);
+  static UploadDataStream* Create(UploadData* data, int* error_code);
 
   ~UploadDataStream();
 
@@ -44,14 +44,14 @@ class UploadDataStream {
  private:
   // Protects from public access since now we have a static creator function
   // which will do both creation and initialization and might return an error.
-  explicit UploadDataStream(const UploadData* data);
+  explicit UploadDataStream(UploadData* data);
 
   // Fills the buffer with any remaining data and sets eof_ if there was nothing
   // left to fill the buffer with.
   // Returns OK if the operation succeeds. Otherwise error code is returned.
   int FillBuf();
 
-  const UploadData* data_;
+  UploadData* data_;
 
   // This buffer is filled with data to be uploaded.  The data to be sent is
   // always at the front of the buffer.  If we cannot send all of the buffer at
@@ -62,7 +62,7 @@ class UploadDataStream {
   size_t buf_len_;
 
   // Iterator to the upload element to be written to the send buffer next.
-  std::vector<UploadData::Element>::const_iterator next_element_;
+  std::vector<UploadData::Element>::iterator next_element_;
 
   // The byte offset into next_element_'s data buffer if the next element is
   // a TYPE_BYTES element.
@@ -70,7 +70,7 @@ class UploadDataStream {
 
   // A stream to the currently open file, for next_element_ if the next element
   // is a TYPE_FILE element.
-  FileStream next_element_stream_;
+  scoped_ptr<FileStream> next_element_stream_;
 
   // The number of bytes remaining to be read from the currently open file
   // if the next element is of TYPE_FILE.
diff --git a/net/base/upload_data_stream_unittest.cc b/net/base/upload_data_stream_unittest.cc
index 701bad9..92c065e 100644
--- a/net/base/upload_data_stream_unittest.cc
+++ b/net/base/upload_data_stream_unittest.cc
@@ -78,7 +78,9 @@ TEST_F(UploadDataStreamTest, FileSmallerThanLength) {
     read_counter += stream->buf_len();
     stream->DidConsume(stream->buf_len());
   }
-  EXPECT_LT(read_counter, stream->size());
+  // UpdateDataStream will pad out the file with 0 bytes so that the HTTP
+  // transaction doesn't hang.  Therefore we expected the full size.
+  EXPECT_EQ(read_counter, stream->size());
 
   file_util::Delete(temp_file_path, false);
 }
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index b25dcfb..bef3237 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/scoped_ptr.h"
 #include "net/base/completion_callback.h"
 #include "net/base/mock_host_resolver.h"
 #include "net/base/request_priority.h"
@@ -4128,6 +4129,154 @@ TEST_F(HttpNetworkTransactionTest, UploadFileSmallerThanLength) {
   file_util::Delete(temp_file_path, false);
 }
 
+TEST_F(HttpNetworkTransactionTest, UploadUnreadableFile) {
+  // If we try to upload an unreadable file, the network stack should report
+  // the file size as zero and upload zero bytes for that file.
+  SessionDependencies session_deps;
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(CreateSession(&session_deps)));
+
+  FilePath temp_file;
+  ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file));
+  std::string temp_file_content("Unreadable file.");
+  ASSERT_TRUE(file_util::WriteFile(temp_file, temp_file_content.c_str(),
+                                   temp_file_content.length()));
+  ASSERT_TRUE(file_util::MakeFileUnreadable(temp_file));
+
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/upload");
+  request.upload_data = new UploadData;
+  request.load_flags = 0;
+
+  std::vector<UploadData::Element> elements;
+  UploadData::Element element;
+  element.SetToFilePath(temp_file);
+  elements.push_back(element);
+  request.upload_data->set_elements(elements);
+
+  MockRead data_reads[] = {
+    MockRead("HTTP/1.0 200 OK\r\n\r\n"),
+    MockRead(false, OK),
+  };
+  MockWrite data_writes[] = {
+    MockWrite("POST /upload HTTP/1.1\r\n"
+              "Host: www.google.com\r\n"
+              "Connection: keep-alive\r\n"
+              "Content-Length: 0\r\n\r\n"),
+    MockWrite(false, OK),
+  };
+  StaticSocketDataProvider data(data_reads, arraysize(data_reads), data_writes,
+                                arraysize(data_writes));
+  session_deps.socket_factory.AddSocketDataProvider(&data);
+
+  TestCompletionCallback callback;
+
+  int rv = trans->Start(&request, &callback, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  EXPECT_TRUE(response != NULL);
+  EXPECT_TRUE(response->headers != NULL);
+  EXPECT_EQ("HTTP/1.0 200 OK", response->headers->GetStatusLine());
+
+  file_util::Delete(temp_file, false);
+}
+
+TEST_F(HttpNetworkTransactionTest, UnreadableUploadFileAfterAuthRestart) {
+  SessionDependencies session_deps;
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(CreateSession(&session_deps)));
+
+  FilePath temp_file;
+  ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file));
+  std::string temp_file_contents("Unreadable file.");
+  std::string unreadable_contents(temp_file_contents.length(), '\0');
+  ASSERT_TRUE(file_util::WriteFile(temp_file, temp_file_contents.c_str(),
+                                   temp_file_contents.length()));
+
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/upload");
+  request.upload_data = new UploadData;
+  request.load_flags = 0;
+
+  std::vector<UploadData::Element> elements;
+  UploadData::Element element;
+  element.SetToFilePath(temp_file);
+  elements.push_back(element);
+  request.upload_data->set_elements(elements);
+
+  MockRead data_reads[] = {
+    MockRead("HTTP/1.1 401 Unauthorized\r\n"),
+    MockRead("WWW-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
+    MockRead("Content-Length: 0\r\n\r\n"),  // No response body.
+
+    MockRead("HTTP/1.1 200 OK\r\n"),
+    MockRead("Content-Length: 0\r\n\r\n"),
+    MockRead(false, OK),
+  };
+  MockWrite data_writes[] = {
+    MockWrite("POST /upload HTTP/1.1\r\n"
+              "Host: www.google.com\r\n"
+              "Connection: keep-alive\r\n"
+              "Content-Length: 16\r\n\r\n"),
+    MockWrite(false, temp_file_contents.c_str()),
+
+    MockWrite("POST /upload HTTP/1.1\r\n"
+              "Host: www.google.com\r\n"
+              "Connection: keep-alive\r\n"
+              "Content-Length: 16\r\n"
+              "Authorization: Basic Zm9vOmJhcg==\r\n\r\n"),
+    MockWrite(false, unreadable_contents.c_str(), temp_file_contents.length()),
+    MockWrite(false, OK),
+  };
+  StaticSocketDataProvider data(data_reads, arraysize(data_reads), data_writes,
+                                arraysize(data_writes));
+  session_deps.socket_factory.AddSocketDataProvider(&data);
+
+  TestCompletionCallback callback1;
+
+  int rv = trans->Start(&request, &callback1, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback1.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  EXPECT_TRUE(response != NULL);
+  EXPECT_TRUE(response->headers != NULL);
+  EXPECT_EQ("HTTP/1.1 401 Unauthorized", response->headers->GetStatusLine());
+
+  // The password prompt info should have been set in response->auth_challenge.
+  EXPECT_TRUE(response->auth_challenge.get() != NULL);
+  EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
+  EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
+  EXPECT_EQ(L"basic", response->auth_challenge->scheme);
+
+  // Now make the file unreadable and try again.
+  ASSERT_TRUE(file_util::MakeFileUnreadable(temp_file));
+
+  TestCompletionCallback callback2;
+
+  rv = trans->RestartWithAuth(L"foo", L"bar", &callback2);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback2.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  response = trans->GetResponseInfo();
+  EXPECT_TRUE(response != NULL);
+  EXPECT_TRUE(response->headers != NULL);
+  EXPECT_TRUE(response->auth_challenge.get() == NULL);
+  EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
+
+  file_util::Delete(temp_file, false);
+}
+
 // Tests that changes to Auth realms are treated like auth rejections.
 TEST_F(HttpNetworkTransactionTest, ChangeAuthRealms) {
   SessionDependencies session_deps;