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

net/base/upload_data_stream.cc

 // Copyright (c) 2006-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.
 
 #include "net/base/upload_data_stream.h"
 
 #include "base/logging.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 
 namespace net {
 
 UploadDataStream::UploadDataStream(const UploadData* data)
     : data_(data),
       buf_(new IOBuffer(kBufSize)),
       buf_len_(0),
       next_element_(data->elements().begin()),
       next_element_offset_(0),
       next_element_remaining_(0),
       total_size_(data->GetContentLength()),
-      current_position_(0) {
+      current_position_(0),
+      file_failed_to_open_(false) {
   FillBuf();
 }
 
 UploadDataStream::~UploadDataStream() {
 }
 
 void UploadDataStream::DidConsume(size_t num_bytes) {
   // TODO(vandebo): Change back to a DCHECK when issue 27870 is resolved.
   CHECK(num_bytes <= buf_len_);
 
@@ skipping 26 matching lines @@
       buf_len_ += bytes_copied;
 
       if (bytes_copied == count) {
         advance_to_next_element = true;
       } else {
         next_element_offset_ += bytes_copied;
       }
     } 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);
         // 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 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) {
             DLOG(WARNING) << "Failed to seek \"" << element.file_path().value()
                           << "\" to offset: " << offset;
           } else {
             next_element_remaining_ = element.file_range_length();
           }
         }
       }
 
       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 {
         advance_to_next_element = true;
       }
     }
 
     if (advance_to_next_element) {
       ++next_element_;
+      file_failed_to_open_ = false;
       next_element_offset_ = 0;
       next_element_stream_.Close();
     }
   }
 }
 
 }  // namespace net