Restructure URLRequestSlowDownloadJob to avoid races.

This is the believed cause of recent DownloadTest.*nownSize flakiness.

BUG=104310


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

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

chrome/browser/download/download_browsertest.cc

@@ -645,7 +645,7 @@ class DownloadTest : public InProcessBrowserTest {
     std::string file_contents;
 
     bool read = file_util::ReadFileToString(path, &file_contents);
-
+    EXPECT_TRUE(read) << "Failed reading file: " << path.value() << std::endl;
     if (!read)
       return false;  // Couldn't read the file.
 
@@ -654,10 +654,12 @@ class DownloadTest : public InProcessBrowserTest {
     size_t expected_size = static_cast<size_t>(file_size);
 
     // Check the size.
+    EXPECT_EQ(expected_size, file_contents.size());
     if (expected_size != file_contents.size())
       return false;
 
     // Check the contents.
+    EXPECT_EQ(value, file_contents);
     if (memcmp(file_contents.c_str(), value.c_str(), expected_size) != 0)
       return false;
 
@@ -837,10 +839,6 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, UnknownSize) {
                           "32.0 KB - ", "100% - "));
 }
 
-#if defined(OS_LINUX)
-// http://crbug.com/104310
-#define KnownSize FLAKY_KnownSize
-#endif
 IN_PROC_BROWSER_TEST_F(DownloadTest, KnownSize) {
   ASSERT_TRUE(RunSizeTest(browser(), SIZE_TEST_TYPE_KNOWN,
                           "71% - ", "100% - "));

content/browser/net/url_request_slow_download_job.cc

@@ -83,7 +83,7 @@ void URLRequestSlowDownloadJob::FinishPendingRequests() {
 
 URLRequestSlowDownloadJob::URLRequestSlowDownloadJob(net::URLRequest* request)
     : net::URLRequestJob(request),
-      first_download_size_remaining_(kFirstDownloadSize),
+      bytes_already_sent_(0),
       should_finish_download_(false),
       buffer_size_(0),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
@@ -96,56 +96,99 @@ void URLRequestSlowDownloadJob::StartAsync() {
   NotifyHeadersComplete();
 }
 
-bool URLRequestSlowDownloadJob::ReadRawData(net::IOBuffer* buf, int buf_size,
-                                            int *bytes_read) {
-  if (LowerCaseEqualsASCII(kFinishDownloadUrl,
-                           request_->url().spec().c_str())) {
-    *bytes_read = 0;
-    return true;
+// ReadRawData and CheckDoneStatus together implement a state
+// machine.  ReadRawData may be called arbitrarily by the network stack.
+// It responds by:
+//      * If there are bytes remaining in the first chunk, they are
+//        returned.
+//      [No bytes remaining in first chunk.   ]
+//      * If should_finish_download_ is not set, it returns IO_PENDING,
+//        and starts calling CheckDoneStatus on a regular timer.
+//      [should_finish_download_ set.]
+//      * If there are bytes remaining in the second chunk, they are filled.
+//      * Otherwise, return *bytes_read = 0 to indicate end of request.
+// CheckDoneStatus is called on a regular basis, in the specific
+// case where we have transmitted all of the first chunk and none of the
+// second.  If should_finish_download_ becomes set, it will "complete"
+// the ReadRawData call that spawned off the CheckDoneStatus() repeated call.
+//
+// FillBufferHelper is a helper function that does the actual work of figuring
+// out where in the state machine we are and how we should fill the buffer.
+// It returns an enum indicating the state of the read.
+URLRequestSlowDownloadJob::ReadStatus
+URLRequestSlowDownloadJob::FillBufferHelper(
+    net::IOBuffer* buf, int buf_size, int* bytes_written) {
+  if (bytes_already_sent_ < kFirstDownloadSize) {
+    int bytes_to_write = std::min(kFirstDownloadSize - bytes_already_sent_,
+                                  buf_size);
+    for (int i = 0; i < bytes_to_write; ++i) {
+      buf->data()[i] = '*';
+    }
+    *bytes_written = bytes_to_write;
+    bytes_already_sent_ += bytes_to_write;
+    return BUFFER_FILLED;
   }
 
-  if (first_download_size_remaining_ > 0) {
-    int send_size = std::min(first_download_size_remaining_, buf_size);
-    for (int i = 0; i < send_size; ++i) {
+  if (!should_finish_download_)
+    return REQUEST_BLOCKED;
+
+  if (bytes_already_sent_ < kFirstDownloadSize + kSecondDownloadSize) {
+    int bytes_to_write =
+        std::min(kFirstDownloadSize + kSecondDownloadSize - bytes_already_sent_,
+                 buf_size);
+    for (int i = 0; i < bytes_to_write; ++i) {
       buf->data()[i] = '*';
     }
-    *bytes_read = send_size;
-    first_download_size_remaining_ -= send_size;
-
-    DCHECK(!is_done());
-    return true;
+    *bytes_written = bytes_to_write;
+    bytes_already_sent_ += bytes_to_write;
+    return BUFFER_FILLED;
   }
 
-  if (should_finish_download_) {
+  return REQUEST_COMPLETE;
+}
+
+bool URLRequestSlowDownloadJob::ReadRawData(net::IOBuffer* buf, int buf_size,
+                                            int* bytes_read) {
+  if (LowerCaseEqualsASCII(kFinishDownloadUrl,
+                           request_->url().spec().c_str())) {
+    VLOG(10) << __FUNCTION__ << " called w/ kFinishDownloadUrl.";
     *bytes_read = 0;
     return true;
   }
 
-  // If we make it here, the first chunk has been sent and we need to wait
-  // until a request is made for kFinishDownloadUrl.
-  buffer_ = buf;
-  buffer_size_ = buf_size;
-  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
-  MessageLoop::current()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&URLRequestSlowDownloadJob::CheckDoneStatus,
-                 weak_factory_.GetWeakPtr()),
-      100);
-
-  // Return false to signal there is pending data.
-  return false;
+  VLOG(10) << __FUNCTION__ << " called at position "
+           << bytes_already_sent_ << " in the stream.";
+  ReadStatus status = FillBufferHelper(buf, buf_size, bytes_read);
+  switch (status) {
+    case BUFFER_FILLED:
+      return true;
+    case REQUEST_BLOCKED:
+      buffer_ = buf;
+      buffer_size_ = buf_size;
+      SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
+      MessageLoop::current()->PostDelayedTask(
+          FROM_HERE,
+          base::Bind(&URLRequestSlowDownloadJob::CheckDoneStatus,
+                     weak_factory_.GetWeakPtr()),
+          100);
+      return false;
+    case REQUEST_COMPLETE:
+      *bytes_read = 0;
+      return true;
+  }
+  NOTREACHED();
+  return true;
 }
 
 void URLRequestSlowDownloadJob::CheckDoneStatus() {
   if (should_finish_download_) {
+    VLOG(10) << __FUNCTION__ << " called w/ should_finish_download_ set.";
     DCHECK(NULL != buffer_);
-    // Send the rest of the data.
-    DCHECK_GT(buffer_size_, kSecondDownloadSize);
-    for (int i = 0; i < kSecondDownloadSize; ++i) {
-      buffer_->data()[i] = '*';
-    }
+    int bytes_written = 0;
+    ReadStatus status = FillBufferHelper(buffer_, buffer_size_, &bytes_written);
+    DCHECK_EQ(BUFFER_FILLED, status);
     SetStatus(net::URLRequestStatus());
-    NotifyReadComplete(kSecondDownloadSize);  // Deletes this.
+    NotifyReadComplete(bytes_written);
   } else {
     MessageLoop::current()->PostDelayedTask(
         FROM_HERE,

content/browser/net/url_request_slow_download_job.h

@@ -55,6 +55,25 @@ class URLRequestSlowDownloadJob : public net::URLRequestJob {
   explicit URLRequestSlowDownloadJob(net::URLRequest* request);
   virtual ~URLRequestSlowDownloadJob();
 
+  // Enum indicating where we are in the read after a call to
+  // FillBufferHelper.
+  enum ReadStatus {
+    // The buffer was filled with data and may be returned.
+    BUFFER_FILLED,
+
+    // No data was added to the buffer because kFinishDownloadUrl has
+    // not yet been seen and we've already returned the first chunk.
+    REQUEST_BLOCKED,
+
+    // No data was added to the buffer because we've already returned
+    // all the data.
+    REQUEST_COMPLETE
+  };
+  ReadStatus FillBufferHelper(
+      net::IOBuffer* buf,
+      int buf_size,
+      int* bytes_written);
+
   void GetResponseInfoConst(net::HttpResponseInfo* info) const;
 
   // Mark all pending requests to be finished.  We keep track of pending
@@ -69,7 +88,7 @@ class URLRequestSlowDownloadJob : public net::URLRequestJob {
 
   void set_should_finish_download() { should_finish_download_ = true; }
 
-  int first_download_size_remaining_;
+  int bytes_already_sent_;
   bool should_finish_download_;
   scoped_refptr<net::IOBuffer> buffer_;
   int buffer_size_;