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

net/base/upload_data_stream.cc

Index: net/base/upload_data_stream.cc
diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index d22929abbab1dd452b2d7de47fcfe706c252f04e..83fceddc6b6335f4ac72e369df79bcd79fc2ed82 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -18,7 +18,8 @@ UploadDataStream::UploadDataStream(const UploadData* data)
       next_element_offset_(0),
       next_element_remaining_(0),
       total_size_(data->GetContentLength()),
-      current_position_(0) {
+      current_position_(0),
+      file_failed_to_open_(false) {
   FillBuf();
 }
 
@@ -65,7 +66,7 @@ void UploadDataStream::FillBuf() {
     } else {
       DCHECK(element.type() == UploadData::TYPE_FILE);
 
-      if (!next_element_stream_.IsOpen()) {
+      if (!file_failed_to_open_ && !next_element_stream_.IsOpen()) {
         int flags = base::PLATFORM_FILE_OPEN |
                     base::PLATFORM_FILE_READ;
         int rv = next_element_stream_.Open(element.file_path(), flags);
@@ -76,6 +77,15 @@ void UploadDataStream::FillBuf() {
                                    << "\" for reading: " << rv;
 
         next_element_remaining_ = 0;  // Default to reading nothing.
+
+        // If a file is visible to us but not readable then, at this point,

[wtc, 2010/01/12 19:51:02]

What does "visible but not readable" mean?

[agl, 2010/01/25 14:14:09]

For example, /etc/shadow is visible (you can ls -l it), but you cannot read it.
If there's a clearer way to express this please let me know.

[wtc, 2010/01/25 20:07:11]

Got it.  Thanks for the explanation.

How about "visible in directory listing but not readable"?

+        // we'll have assumed a non-zero length, but we'll have just failed to
+        // open the file. In this case we just fill in NULs.
+        if (rv != OK && element.expected_length() > 0) {
+          file_failed_to_open_ = true;
+          next_element_remaining_ = element.expected_length();
+        }
+
         if (rv == OK) {
           uint64 offset = element.file_range_offset();
           if (offset && next_element_stream_.Seek(FROM_BEGIN, offset) < 0) {
@@ -90,9 +100,15 @@ void UploadDataStream::FillBuf() {
       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) {
+      if (count > 0 && file_failed_to_open_) {
+        memset(buf_->data() + buf_len_, 0, count);

[wtc, 2010/01/12 19:51:02]

Please add a comment here to note that we're filling in NULs.
I had a little trouble finding this code when I read your
comment at lines 82-83.

Is filling in NULs the right thing to do?  Can we fail the
upload or somehow break out of the while loop to avoid the
livelock?

[agl, 2010/01/25 14:14:09]

Done.
At this point in the code, I don't think that there's a lot that we can do. Note
that, in the current patch set, the only situation where we pad with NULs is if
the file shrunk under us.

+        buf_len_ += count;
+        next_element_remaining_ -= count;
+      } else if (count > 0 &&
+                  (rv = next_element_stream_.Read(buf_->data() + buf_len_,
+                                                  count, NULL)) > 0) {
+        // TODO(darin): The file might have been truncated between when we
+        // asked for its size and now.
         buf_len_ += rv;
         next_element_remaining_ -= rv;
       } else {
@@ -102,6 +118,7 @@ void UploadDataStream::FillBuf() {
 
     if (advance_to_next_element) {
       ++next_element_;
+      file_failed_to_open_ = false;
       next_element_offset_ = 0;
       next_element_stream_.Close();
     }