Restructure URLRequestSlowDownloadJob to avoid races.

content/browser/net/url_request_slow_download_job.cc

 // Copyright (c) 2011 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 "content/browser/net/url_request_slow_download_job.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
@@ skipping 65 matching lines @@
   typedef std::set<URLRequestSlowDownloadJob*> JobList;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   for (JobList::iterator it = pending_requests_.Get().begin(); it !=
        pending_requests_.Get().end(); ++it) {
     (*it)->set_should_finish_download();
   }
 }
 
 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)) {
 }
 
 void URLRequestSlowDownloadJob::StartAsync() {
   if (LowerCaseEqualsASCII(kFinishDownloadUrl, request_->url().spec().c_str()))
     URLRequestSlowDownloadJob::FinishPendingRequests();
 
   NotifyHeadersComplete();
 }
 
+// ReadRawData and CheckDoneStatus implement together implement a state

[eroman, 2011/11/18 01:54:46]

nit: redundant "implement".

[Randy Smith (Not in Mondays), 2011/11/18 15:30:18]

Done.

+// 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 (!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_written = bytes_to_write;
+    bytes_already_sent_ += bytes_to_write;
+    return BUFFER_FILLED;
+  }
+
+  return REQUEST_COMPLETE;
+}
+
 bool URLRequestSlowDownloadJob::ReadRawData(net::IOBuffer* buf, int buf_size,
-                                            int *bytes_read) {
+                                            int* bytes_read) {
   if (LowerCaseEqualsASCII(kFinishDownloadUrl,
                            request_->url().spec().c_str())) {
+    VLOG(10) << __FUNCTION__ << " called w/ kFinishDownloadUrl.";
     *bytes_read = 0;
     return true;
   }
 
-  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) {
-      buf->data()[i] = '*';
-    }
-    *bytes_read = send_size;
-    first_download_size_remaining_ -= send_size;
-
-    DCHECK(!is_done());
-    return true;
+  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;
   }
-
-  if (should_finish_download_) {
-    *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;
+  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,
         base::Bind(&URLRequestSlowDownloadJob::CheckDoneStatus,
                    weak_factory_.GetWeakPtr()),
         100);
   }
 }
 
 // Public virtual version.
@@ skipping 33 matching lines @@
   // ParseRawHeaders expects \0 to end each header line.
   ReplaceSubstringsAfterOffset(&raw_headers, 0, "\n", std::string("\0", 1));
   info->headers = new net::HttpResponseHeaders(raw_headers);
 }
 
 bool URLRequestSlowDownloadJob::GetMimeType(std::string* mime_type) const {
   net::HttpResponseInfo info;
   GetResponseInfoConst(&info);
   return info.headers && info.headers->GetMimeType(mime_type);
 }