Revert 42152 - Fix the case where the browser livelocks if we cannot open a file.

If one tries to upload a file that one doesn't have read access to,
the browser livelocks. It tries to read from the file, gets nothing
but spins forever because it knows that it hasn't finished reading.

To address this, firstly we add a check at stat() time to make sure
that we can read the file. However, this doesn't take care of the case
where the access() call was incorrect, or the permissions have changed
under us. In this case, we replace the missing file with NULs.

BUG=30850

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

TBR=vandebo@chromium.org
Review URL: http://codereview.chromium.org/1145004

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

7 files changed, 68 insertions, 195 deletions

diff --git a/net/base/file_stream.h b/net/base/file_stream.h
index dc578f3..6b7f3dc 100644
--- a/net/base/file_stream.h
+++ b/net/base/file_stream.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.  Use of this
+// Copyright (c) 2008 The Chromium Authors. All rights reserved.  Use of this
 // source code is governed by a BSD-style license that can be found in the
 // LICENSE file.
 
@@ -43,24 +43,13 @@ class FileStream {
   // Note that if there are any pending async operations, they'll be aborted.
   void Close();
 
-  // Release performs the same actions as Close, but doesn't actually close the
-  // underlying PlatformFile.
-  void Release();
-
   // Call this method to open the FileStream.  The remaining methods
   // cannot be used unless this method returns OK.  If the file cannot be
   // opened then an error code is returned.
   // open_flags is a bitfield of base::PlatformFileFlags
   int Open(const FilePath& path, int open_flags);
 
-  // Calling this method is functionally the same as constructing the object
-  // with the non-default constructor. This method can only be used if the
-  // FileSteam isn't currently open (i.e. was constructed with the default
-  // constructor).
-  int Open(base::PlatformFile file, int open_flags);
-
-  // Returns true if Open succeeded and neither Close nor Release have been
-  // called.
+  // Returns true if Open succeeded and Close has not been called.
   bool IsOpen() const;
 
   // Adjust the position from where data is read.  Upon success, the stream
diff --git a/net/base/file_stream_posix.cc b/net/base/file_stream_posix.cc
index e947a6e..a4c5b3c 100644
--- a/net/base/file_stream_posix.cc
+++ b/net/base/file_stream_posix.cc
@@ -298,8 +298,13 @@ FileStream::FileStream()
 }
 
 FileStream::FileStream(base::PlatformFile file, int flags)
-    : file_(base::kInvalidPlatformFileValue) {
-  Open(file, flags);
+    : file_(file),
+      open_flags_(flags) {
+  // If the file handle is opened with base::PLATFORM_FILE_ASYNC, we need to
+  // make sure we will perform asynchronous File IO to it.
+  if (flags & base::PLATFORM_FILE_ASYNC) {
+    async_context_.reset(new AsyncContext());
+  }
 }
 
 FileStream::~FileStream() {
@@ -318,12 +323,6 @@ void FileStream::Close() {
   }
 }
 
-void FileStream::Release() {
-  // Abort any existing asynchronous operations.
-  async_context_.reset();
-  file_ = base::kInvalidPlatformFileValue;
-}
-
 int FileStream::Open(const FilePath& path, int open_flags) {
   if (IsOpen()) {
     DLOG(FATAL) << "File is already open!";
@@ -345,21 +344,6 @@ int FileStream::Open(const FilePath& path, int open_flags) {
   return OK;
 }
 
-int FileStream::Open(base::PlatformFile file, int open_flags) {
-  if (IsOpen()) {
-    DLOG(FATAL) << "File is already open!";
-    return ERR_UNEXPECTED;
-  }
-
-  open_flags_ = open_flags;
-  file_ = file;
-
-  if (open_flags & base::PLATFORM_FILE_ASYNC)
-    async_context_.reset(new AsyncContext());
-
-  return OK;
-}
-
 bool FileStream::IsOpen() const {
   return file_ != base::kInvalidPlatformFileValue;
 }
@@ -461,7 +445,7 @@ int64 FileStream::Truncate(int64 bytes) {
   if (!IsOpen())
     return ERR_UNEXPECTED;
 
-  // We better be open for writing.
+  // We better be open for reading.
   DCHECK(open_flags_ & base::PLATFORM_FILE_WRITE);
 
   // Seek to the position to truncate from.
diff --git a/net/base/file_stream_win.cc b/net/base/file_stream_win.cc
index 3119a26..cec6a9d 100644
--- a/net/base/file_stream_win.cc
+++ b/net/base/file_stream_win.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.  Use of this
+// Copyright (c) 2008 The Chromium Authors. All rights reserved.  Use of this
 // source code is governed by a BSD-style license that can be found in the
 // LICENSE file.
 
@@ -120,10 +120,16 @@ FileStream::FileStream()
       open_flags_(0) {
 }
 
-FileStream::FileStream(base::PlatformFile file, int open_flags)
-    : file_(INVALID_HANDLE_VALUE),
-      open_flags_(0) {
-  Open(file, open_flags);
+FileStream::FileStream(base::PlatformFile file, int flags)
+    : file_(file),
+      open_flags_(flags) {
+  // If the file handle is opened with base::PLATFORM_FILE_ASYNC, we need to
+  // make sure we will perform asynchronous File IO to it.
+  if (flags & base::PLATFORM_FILE_ASYNC) {
+    async_context_.reset(new AsyncContext(this));
+    MessageLoopForIO::current()->RegisterIOHandler(file_,
+                                                   async_context_.get());
+  }
 }
 
 FileStream::~FileStream() {
@@ -141,13 +147,6 @@ void FileStream::Close() {
   }
 }
 
-void FileStream::Release() {
-  if (file_ != INVALID_HANDLE_VALUE)
-    CancelIo(file_);
-  async_context_.reset();
-  file_ = INVALID_HANDLE_VALUE;
-}
-
 int FileStream::Open(const FilePath& path, int open_flags) {
   if (IsOpen()) {
     DLOG(FATAL) << "File is already open!";
@@ -171,26 +170,6 @@ int FileStream::Open(const FilePath& path, int open_flags) {
   return OK;
 }
 
-int FileStream::Open(base::PlatformFile file, int open_flags) {
-  if (IsOpen()) {
-    DLOG(FATAL) << "File is already open!";
-    return ERR_UNEXPECTED;
-  }
-
-  open_flags_ = open_flags;
-  file_ = file;
-
-  // If the file handle is opened with base::PLATFORM_FILE_ASYNC, we need to
-  // make sure we will perform asynchronous File IO to it.
-  if (open_flags_ & base::PLATFORM_FILE_ASYNC) {
-    async_context_.reset(new AsyncContext(this));
-    MessageLoopForIO::current()->RegisterIOHandler(file_,
-                                                   async_context_.get());
-  }
-
-  return OK;
-}
-
 bool FileStream::IsOpen() const {
   return file_ != INVALID_HANDLE_VALUE;
 }
diff --git a/net/base/upload_data.cc b/net/base/upload_data.cc
index a23b25e..0496873 100644
--- a/net/base/upload_data.cc
+++ b/net/base/upload_data.cc
@@ -5,7 +5,6 @@
 #include "net/base/upload_data.h"
 
 #include "base/file_util.h"
-#include "base/platform_file.h"
 #include "base/logging.h"
 
 namespace net {
@@ -18,67 +17,28 @@ uint64 UploadData::GetContentLength() const {
   return len;
 }
 
-void UploadData::CloseFiles() {
-  std::vector<Element>::iterator it = elements_.begin();
-  for (; it != elements_.end(); ++it) {
-    if (it->type() == TYPE_FILE)
-      it->Close();
-  }
-}
-
-base::PlatformFile UploadData::Element::platform_file() const {
-  DCHECK(type_ == TYPE_FILE) << "platform_file on non-file Element";
-
-  return file_;
-}
-
-void UploadData::Element::Close() {
-  DCHECK(type_ == TYPE_FILE) << "Close on non-file Element";
-
-  if (file_ != base::kInvalidPlatformFileValue)
-    base::ClosePlatformFile(file_);
-  file_ = base::kInvalidPlatformFileValue;
-}
-
-void UploadData::Element::SetToFilePathRange(const FilePath& path,
-                                             uint64 offset,
-                                             uint64 length) {
-  type_ = TYPE_FILE;
-  file_range_offset_ = 0;
-  file_range_length_ = 0;
+uint64 UploadData::Element::GetContentLength() const {
+  if (override_content_length_)
+    return content_length_;
 
-  Close();
+  if (type_ == TYPE_BYTES)
+    return static_cast<uint64>(bytes_.size());
 
-  if (offset + length < offset) {
-    LOG(ERROR) << "Upload file offset and length overflow 64-bits. Ignoring.";
-    return;
-  }
+  DCHECK(type_ == TYPE_FILE);
 
-  base::PlatformFile file = base::CreatePlatformFile(
-      path, base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, NULL);
-  if (file == base::kInvalidPlatformFileValue) {
-    // This case occurs when the user selects a file that isn't readable.
-    file_path_= path;
-    return;
-  }
+  // 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.
 
-  uint64 file_size;
-  if (!base::GetPlatformFileSize(file, &file_size)) {
-    base::ClosePlatformFile(file);
-    return;
-  }
+  int64 length = 0;
+  if (!file_util::GetFileSize(file_path_, &length))
+    return 0;
 
-  if (offset > file_size) {
-    base::ClosePlatformFile(file);
-    return;
-  }
-  if (offset + length > file_size)
-    length = file_size - offset;
+  if (file_range_offset_ >= static_cast<uint64>(length))
+    return 0;  // range is beyond eof
 
-  file_ = file;
-  file_path_ = path;
-  file_range_offset_ = offset;
-  file_range_length_ = length;
+  // compensate for the offset and clip file_range_length_ to eof
+  return std::min(length - file_range_offset_, file_range_length_);
 }
 
 }  // namespace net
diff --git a/net/base/upload_data.h b/net/base/upload_data.h
index d01b435..3aab835 100644
--- a/net/base/upload_data.h
+++ b/net/base/upload_data.h
@@ -9,7 +9,6 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
-#include "base/platform_file.h"
 #include "base/ref_counted.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
@@ -27,8 +26,7 @@ class UploadData : public base::RefCounted<UploadData> {
   class Element {
    public:
     Element() : type_(TYPE_BYTES), file_range_offset_(0),
-                file_range_length_(0),
-                file_(base::kInvalidPlatformFileValue),
+                file_range_length_(kuint64max),
                 override_content_length_(false) {
     }
 
@@ -47,42 +45,19 @@ class UploadData : public base::RefCounted<UploadData> {
       SetToFilePathRange(path, 0, kuint64max);
     }
 
-    void SetToFilePathRange(const FilePath& path, uint64 offset, uint64 length);
+    void SetToFilePathRange(const FilePath& path,
+                            uint64 offset, uint64 length) {
+      type_ = TYPE_FILE;
+      file_path_ = path;
+      file_range_offset_ = offset;
+      file_range_length_ = length;
+    }
 
     // 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 {
-      if (override_content_length_)
-        return content_length_;
-
-      if (type_ == TYPE_BYTES) {
-        return bytes_.size();
-      } else {
-        return file_range_length_;
-      }
-    }
-
-    // For a TYPE_FILE, return a handle to the file. The caller does not take
-    // ownership and should not close the file handle.
-    base::PlatformFile platform_file() const;
-
-    // For a TYPE_FILE, this closes the file handle. It's a fatal error to call
-    // platform_file() after this.
-    void Close();
+    uint64 GetContentLength() const;
 
    private:
-    // type_ == TYPE_BYTES:
-    //   bytes_ is valid
-    // type_ == TYPE_FILE:
-    //   file_path_ should always be valid.
-    //
-    //   platform_file() may be invalid, in which case file_range_* are 0 and
-    //   file_ is invalid. This occurs when we cannot open the requested file.
-    //
-    //   Else, then file_range_* are within range of the length of the file
-    //   that we found when opening the file. Also, the sum of offset and
-    //   length will not overflow a uint64. file_ will be handle to the file.
-
     // Allows tests to override the result of GetContentLength.
     void SetContentLength(uint64 content_length) {
       override_content_length_ = true;
@@ -94,7 +69,6 @@ class UploadData : public base::RefCounted<UploadData> {
     FilePath file_path_;
     uint64 file_range_offset_;
     uint64 file_range_length_;
-    base::PlatformFile file_;
     bool override_content_length_;
     uint64 content_length_;
 
@@ -135,9 +109,6 @@ class UploadData : public base::RefCounted<UploadData> {
     elements_.swap(*elements);
   }
 
-  // CloseFiles closes the file handles of all Elements of type TYPE_FILE.
-  void CloseFiles();
-
   // Identifies a particular upload instance, which is used by the cache to
   // formulate a cache key.  This value should be unique across browser
   // sessions.  A value of 0 is used to indicate an unspecified identifier.
diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index 8c964f4..21f4ce8 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -10,7 +10,7 @@
 
 namespace net {
 
-UploadDataStream::UploadDataStream(UploadData* data)
+UploadDataStream::UploadDataStream(const UploadData* data)
     : data_(data),
       buf_(new IOBuffer(kBufSize)),
       buf_len_(0),
@@ -66,19 +66,18 @@ void UploadDataStream::FillBuf() {
     } else {
       DCHECK(element.type() == UploadData::TYPE_FILE);
 
-      if (element.file_range_length() == 0) {
-        // If we failed to open the file, then the length is set to zero. The
-        // length used when calculating the POST size was also zero. This
-        // matches the behaviour of Mozilla.
-        next_element_remaining_ = 0;
-      } else {
-        if (!next_element_stream_.IsOpen()) {
-          // We ignore the return value of Open becuase we've already checked
-          // !IsOpen, above.
-          int flags = base::PLATFORM_FILE_READ |
-                      base::PLATFORM_FILE_WRITE;
-          next_element_stream_.Open(element.platform_file(), flags);
-
+      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()
@@ -90,18 +89,11 @@ void UploadDataStream::FillBuf() {
       }
 
       int rv = 0;
-      if (next_element_remaining_ > 0) {
-        int count =
-            static_cast<int>(std::min(next_element_remaining_,
-                                      static_cast<uint64>(size_remaining)));
-        rv = next_element_stream_.Read(buf_->data() + buf_len_, count, NULL);
-        if (rv < 1) {
-          // If the file was truncated between the time that we opened it and
-          // now, or if we got an error on reading, then we pad with NULs.
-          memset(buf_->data() + buf_len_, 0, count);
-          rv = count;
-        }
-
+      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) {
         buf_len_ += rv;
         next_element_remaining_ -= rv;
       } else {
@@ -112,14 +104,12 @@ void UploadDataStream::FillBuf() {
     if (advance_to_next_element) {
       ++next_element_;
       next_element_offset_ = 0;
-      next_element_stream_.Release();
+      next_element_stream_.Close();
     }
   }
 
-  if (next_element_ == end && !buf_len_) {
+  if (next_element_ == end && !buf_len_)
     eof_ = true;
-    data_->CloseFiles();
-  }
 }
 
 }  // namespace net
diff --git a/net/base/upload_data_stream.h b/net/base/upload_data_stream.h
index 9df5db0..0dd7dd3 100644
--- a/net/base/upload_data_stream.h
+++ b/net/base/upload_data_stream.h
@@ -14,7 +14,7 @@ class IOBuffer;
 
 class UploadDataStream {
  public:
-  explicit UploadDataStream(UploadData* data);
+  explicit UploadDataStream(const UploadData* data);
   ~UploadDataStream();
 
   // Returns the stream's buffer and buffer length.
@@ -42,7 +42,7 @@ class UploadDataStream {
   // left to fill the buffer with.
   void FillBuf();
 
-  UploadData* data_;
+  const 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