Add a notion of 'eof' to UploadDataStream, replacing the use of its size property for detecting when an upload is finished.

While this does prevent the crash described in the bug from occurring, this doesn't fully solve the problem as now the affected uploads don't complete. Fully resolving the issue will require implementing the chunked transfer encoding for requests.

Patch from Vernon Tang <vt@foilhead.net>, original review: http://codereview.chromium.org/555194

BUG=33501
TEST=Create a file with a non-zero size and select that file in an HTML-based uploader. Before starting the upload, remove read permissions from that file. Check that the upload doesn't cause the browser to crash or hang. net_unittests: HttpNetworkTransactionTest.UploadFileSmallerThanLength, UploadDataStreamTest.*

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

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

9 files changed, 176 insertions, 11 deletions

diff --git a/net/base/upload_data.cc b/net/base/upload_data.cc
index 0045409..0496873 100644
--- a/net/base/upload_data.cc
+++ b/net/base/upload_data.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -18,6 +18,9 @@ uint64 UploadData::GetContentLength() const {
 }
 
 uint64 UploadData::Element::GetContentLength() const {
+  if (override_content_length_)
+    return content_length_;
+
   if (type_ == TYPE_BYTES)
     return static_cast<uint64>(bytes_.size());
 
diff --git a/net/base/upload_data.h b/net/base/upload_data.h
index 02880d3..3aab835 100644
--- a/net/base/upload_data.h
+++ b/net/base/upload_data.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -7,8 +7,10 @@
 
 #include <vector>
 
+#include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/ref_counted.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"
 
 namespace net {
 
@@ -24,7 +26,8 @@ class UploadData : public base::RefCounted<UploadData> {
   class Element {
    public:
     Element() : type_(TYPE_BYTES), file_range_offset_(0),
-                file_range_length_(kuint64max) {
+                file_range_length_(kuint64max),
+                override_content_length_(false) {
     }
 
     Type type() const { return type_; }
@@ -55,11 +58,22 @@ class UploadData : public base::RefCounted<UploadData> {
     uint64 GetContentLength() const;
 
    private:
+    // Allows tests to override the result of GetContentLength.
+    void SetContentLength(uint64 content_length) {
+      override_content_length_ = true;
+      content_length_ = content_length;
+    }
+
     Type type_;
     std::vector<char> bytes_;
     FilePath file_path_;
     uint64 file_range_offset_;
     uint64 file_range_length_;
+    bool override_content_length_;
+    uint64 content_length_;
+
+    FRIEND_TEST(UploadDataStreamTest, FileSmallerThanLength);
+    FRIEND_TEST(HttpNetworkTransactionTest, UploadFileSmallerThanLength);
   };
 
   void AppendBytes(const char* bytes, int bytes_len) {
diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index d22929a..221ef28 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -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),
+      eof_(false) {
   FillBuf();
 }
 
@@ -28,6 +29,7 @@ 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_);
+  DCHECK(!eof_);
 
   buf_len_ -= num_bytes;
   if (buf_len_)
@@ -106,6 +108,9 @@ void UploadDataStream::FillBuf() {
       next_element_stream_.Close();
     }
   }
+
+  if (next_element_ == end && !buf_len_)
+    eof_ = true;
 }
 
 }  // namespace net
diff --git a/net/base/upload_data_stream.h b/net/base/upload_data_stream.h
index d703c3d..0dd7dd3 100644
--- a/net/base/upload_data_stream.h
+++ b/net/base/upload_data_stream.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -27,10 +27,19 @@ class UploadDataStream {
   void DidConsume(size_t num_bytes);
 
   // Returns the total size of the data stream and the current position.
+  // size() is not to be used to determine whether the stream has ended
+  // because it is possible for the stream to end before its size is reached,
+  // for example, if the file is truncated.
   uint64 size() const { return total_size_; }
   uint64 position() const { return current_position_; }
 
+  // Returns whether there is no more data to read, regardless of whether
+  // position < size.
+  bool eof() const { return eof_; }
+
  private:
+  // Fills the buffer with any remaining data and sets eof_ if there was nothing
+  // left to fill the buffer with.
   void FillBuf();
 
   const UploadData* data_;
@@ -62,6 +71,9 @@ class UploadDataStream {
   uint64 total_size_;
   uint64 current_position_;
 
+  // Whether there is no data left to read.
+  bool eof_;
+
   DISALLOW_EVIL_CONSTRUCTORS(UploadDataStream);
 };
 
diff --git a/net/base/upload_data_stream_unittest.cc b/net/base/upload_data_stream_unittest.cc
new file mode 100644
index 0000000..ae8f7ff
--- /dev/null
+++ b/net/base/upload_data_stream_unittest.cc
@@ -0,0 +1,74 @@
+// Copyright (c) 2010 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 <vector>
+
+#include "base/basictypes.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "net/base/upload_data.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace net {
+
+namespace {
+
+const char kTestData[] = "0123456789";
+const int kTestDataSize = arraysize(kTestData) - 1;
+
+}  // namespace
+
+class UploadDataStreamTest : public PlatformTest {
+ public:
+  UploadDataStreamTest() : upload_data_(new UploadData) { }
+
+  scoped_refptr<UploadData> upload_data_;
+};
+
+TEST_F(UploadDataStreamTest, EmptyUploadData) {
+  upload_data_->AppendBytes("", 0);
+  UploadDataStream stream(upload_data_);
+  EXPECT_TRUE(stream.eof());
+}
+
+TEST_F(UploadDataStreamTest, ConsumeAll) {
+  upload_data_->AppendBytes(kTestData, kTestDataSize);
+  UploadDataStream stream(upload_data_);
+  while (!stream.eof()) {
+    stream.DidConsume(stream.buf_len());
+  }
+}
+
+TEST_F(UploadDataStreamTest, FileSmallerThanLength) {
+  FilePath temp_file_path;
+  ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file_path));
+  ASSERT_EQ(kTestDataSize, file_util::WriteFile(temp_file_path,
+                                                kTestData, kTestDataSize));
+  const uint64 kFakeSize = kTestDataSize*2;
+
+  std::vector<UploadData::Element> elements;
+  UploadData::Element element;
+  element.SetToFilePath(temp_file_path);
+  element.SetContentLength(kFakeSize);
+  elements.push_back(element);
+  upload_data_->set_elements(elements);
+  EXPECT_EQ(kFakeSize, upload_data_->GetContentLength());
+
+  UploadDataStream stream(upload_data_);
+  EXPECT_FALSE(stream.eof());
+  uint64 read_counter = 0;
+  while (!stream.eof()) {
+    read_counter += stream.buf_len();
+    stream.DidConsume(stream.buf_len());
+  }
+  EXPECT_LT(read_counter, stream.size());
+
+  file_util::Delete(temp_file_path, false);
+}
+
+}  // namespace net
+
diff --git a/net/flip/flip_stream.cc b/net/flip/flip_stream.cc
index b3d0978..15555cc 100755
--- a/net/flip/flip_stream.cc
+++ b/net/flip/flip_stream.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -381,7 +381,7 @@ int FlipStream::DoSendBodyComplete(int result) {
 
   request_body_stream_->DidConsume(result);
 
-  if (request_body_stream_->position() < request_body_stream_->size())
+  if (!request_body_stream_->eof())
     io_state_ = STATE_SEND_BODY;
   else
     io_state_ = STATE_READ_HEADERS;
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 9639457..4a71ab1 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -1,10 +1,13 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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 <math.h>  // ceil
+#include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
 #include "net/base/completion_callback.h"
 #include "net/base/mock_host_resolver.h"
 #include "net/base/request_priority.h"
@@ -3966,4 +3969,57 @@ TEST_F(HttpNetworkTransactionTest, LargeContentLengthThenClose) {
   EXPECT_EQ("", out.response_data);
 }
 
+TEST_F(HttpNetworkTransactionTest, UploadFileSmallerThanLength) {
+  SessionDependencies session_deps;
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(CreateSession(&session_deps)));
+
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/upload");
+  request.upload_data = new UploadData;
+  request.load_flags = 0;
+
+  FilePath temp_file_path;
+  ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file_path));
+  const uint64 kFakeSize = 100000;  // file is actually blank
+
+  std::vector<UploadData::Element> elements;
+  UploadData::Element element;
+  element.SetToFilePath(temp_file_path);
+  element.SetContentLength(kFakeSize);
+  elements.push_back(element);
+  request.upload_data->set_elements(elements);
+  EXPECT_EQ(kFakeSize, request.upload_data->GetContentLength());
+
+  MockRead data_reads[] = {
+    MockRead("HTTP/1.0 200 OK\r\n\r\n"),
+    MockRead("hello world"),
+    MockRead(false, OK),
+  };
+  StaticSocketDataProvider data(data_reads, NULL);
+  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());
+
+  std::string response_data;
+  rv = ReadTransaction(trans.get(), &response_data);
+  EXPECT_EQ(OK, rv);
+  EXPECT_EQ("hello world", response_data);
+
+  file_util::Delete(temp_file_path, false);
+}
+
 }  // namespace net
diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index abbf112..9ec0ad6 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -210,7 +210,7 @@ int HttpStreamParser::DoSendBody(int result) {
   if (result > 0)
     request_body_->DidConsume(result);
 
-  if (request_body_->position() < request_body_->size()) {
+  if (!request_body_->eof()) {
     int buf_len = static_cast<int>(request_body_->buf_len());
     result = connection_->socket()->Write(request_body_->buf(), buf_len,
                                           &io_callback_);
diff --git a/net/net.gyp b/net/net.gyp
index b1645d5..3f63d82 100755
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -611,6 +611,7 @@
         'base/telnet_server_unittest.cc',
         'base/test_certificate_data.h',
         'base/test_completion_callback_unittest.cc',
+        'base/upload_data_stream_unittest.cc',
         'base/x509_certificate_unittest.cc',
         'disk_cache/addr_unittest.cc',
         'disk_cache/backend_unittest.cc',