Modified cancel and interrupt processing to avoid race with history.

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/compiler_specific.h"
 #include "base/message_loop.h"
 #include "base/stringprintf.h"
 #include "base/string_util.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_status.h"
 
 const int kFirstDownloadSize = 1024 * 35;
 const int kSecondDownloadSize = 1024 * 10;
 
 const char URLRequestSlowDownloadJob::kUnknownSizeUrl[] =
   "http://url.handled.by.slow.download/download-unknown-size";
 const char URLRequestSlowDownloadJob::kKnownSizeUrl[] =
   "http://url.handled.by.slow.download/download-known-size";
 const char URLRequestSlowDownloadJob::kFinishDownloadUrl[] =
   "http://url.handled.by.slow.download/download-finish";
+const char URLRequestSlowDownloadJob::kErrorFinishDownloadUrl[] =
+  "http://url.handled.by.slow.download/download-error";
 
 std::vector<URLRequestSlowDownloadJob*>
     URLRequestSlowDownloadJob::kPendingRequests;
 
+// Return whether this is the finish or error URL

[brettw, 2011/07/06 16:44:13]

Nit: Need period.

[Randy Smith (Not in Mondays), 2011/07/07 22:02:23]

Done.

+static bool IsCompletionUrl(GURL url) {
+  return (LowerCaseEqualsASCII(
+      URLRequestSlowDownloadJob::kFinishDownloadUrl, url.spec().c_str()) ||

[brettw, 2011/07/06 16:44:13]

This statement is almost impossible to read. I'd probably split the || apart
into two statements and do an early return if the first one passes.

[Randy Smith (Not in Mondays), 2011/07/07 22:02:23]

Done (though it's easier to read with the change requested by Eric).

+          LowerCaseEqualsASCII(

[eroman, 2011/07/06 22:19:01]

Why use lowercase URL comparison throughout rather than straight up string
comparison?

Note that since url.spec() is already canonicalized, the scheme and host are
necessarily going to be lowercased. (The case of the path will be preserved
though).

Unless the test usage has good reason to pass different cased paths, I suggest
not messing with it and simplify this to "==" comparison.

[Randy Smith (Not in Mondays), 2011/07/07 22:02:23]

There's no good reason except for compatibility with what was already there;
I'll change it.

+              URLRequestSlowDownloadJob::kErrorFinishDownloadUrl,
+              url.spec().c_str()));
+}
+
 void URLRequestSlowDownloadJob::Start() {
   MessageLoop::current()->PostTask(
       FROM_HERE,
       method_factory_.NewRunnableMethod(
           &URLRequestSlowDownloadJob::StartAsync));
 }
 
 // static
 void URLRequestSlowDownloadJob::AddUrlHandler() {
   net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
   filter->AddUrlHandler(GURL(kUnknownSizeUrl),
                         &URLRequestSlowDownloadJob::Factory);
   filter->AddUrlHandler(GURL(kKnownSizeUrl),
                         &URLRequestSlowDownloadJob::Factory);
   filter->AddUrlHandler(GURL(kFinishDownloadUrl),
                         &URLRequestSlowDownloadJob::Factory);
+  filter->AddUrlHandler(GURL(kErrorFinishDownloadUrl),
+                        &URLRequestSlowDownloadJob::Factory);
 }
 
 /*static */
 net::URLRequestJob* URLRequestSlowDownloadJob::Factory(
     net::URLRequest* request,
     const std::string& scheme) {
   URLRequestSlowDownloadJob* job = new URLRequestSlowDownloadJob(request);
-  if (request->url().spec() != kFinishDownloadUrl)
+  if (!IsCompletionUrl(request->url()))
     URLRequestSlowDownloadJob::kPendingRequests.push_back(job);
   return job;
 }
 
 /* static */
-void URLRequestSlowDownloadJob::FinishPendingRequests() {
+void URLRequestSlowDownloadJob::FinishPendingRequests(bool error) {
   typedef std::vector<URLRequestSlowDownloadJob*> JobList;
   for (JobList::iterator it = kPendingRequests.begin(); it !=
        kPendingRequests.end(); ++it) {

[eroman, 2011/07/06 22:19:01]

The following comment predates this change.... but why is this variable named
like a constant? This is really just a global variable.

[Randy Smith (Not in Mondays), 2011/07/07 22:02:23]

Done.

-    (*it)->set_should_finish_download();
+    if (error) {
+      (*it)->set_should_error_download();
+    } else {
+      (*it)->set_should_finish_download();
+    }
   }
   kPendingRequests.clear();
 }
 
 URLRequestSlowDownloadJob::URLRequestSlowDownloadJob(net::URLRequest* request)
     : net::URLRequestJob(request),
       first_download_size_remaining_(kFirstDownloadSize),
       should_finish_download_(false),
       should_send_second_chunk_(false),
+      should_error_download_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {}
 
 void URLRequestSlowDownloadJob::StartAsync() {
   if (LowerCaseEqualsASCII(kFinishDownloadUrl, request_->url().spec().c_str()))

[eroman, 2011/07/06 22:19:01]

Same comment as before: I think we should use exact string comparison throughout
here.

[Randy Smith (Not in Mondays), 2011/07/07 22:02:23]

Done.

-    URLRequestSlowDownloadJob::FinishPendingRequests();
+    URLRequestSlowDownloadJob::FinishPendingRequests(false);
+
+  if (LowerCaseEqualsASCII(kErrorFinishDownloadUrl,

[eroman, 2011/07/06 22:19:01]

ditto

[Randy Smith (Not in Mondays), 2011/07/07 22:02:23]

Done.

+                           request_->url().spec().c_str())) {
+    URLRequestSlowDownloadJob::FinishPendingRequests(true);
+  }
 
   NotifyHeadersComplete();
 }
 
 bool URLRequestSlowDownloadJob::ReadRawData(net::IOBuffer* buf, int buf_size,
                                             int *bytes_read) {
-  if (LowerCaseEqualsASCII(kFinishDownloadUrl,
-                           request_->url().spec().c_str())) {
+  if (IsCompletionUrl(request_->url())) {
     *bytes_read = 0;
     return true;
   }
 
   if (should_send_second_chunk_) {
     DCHECK(buf_size > kSecondDownloadSize);
     for (int i = 0; i < kSecondDownloadSize; ++i) {
       buf->data()[i] = '*';
     }
     *bytes_read = kSecondDownloadSize;
@@ skipping 29 matching lines @@
 
   // Return false to signal there is pending data.
   return false;
 }
 
 void URLRequestSlowDownloadJob::CheckDoneStatus() {
   if (should_finish_download_) {
     should_send_second_chunk_ = true;
     SetStatus(net::URLRequestStatus());
     NotifyReadComplete(kSecondDownloadSize);
+  } else if (should_error_download_) {
+    NotifyDone(
+        net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED));
   } else {
     MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         method_factory_.NewRunnableMethod(
             &URLRequestSlowDownloadJob::CheckDoneStatus),
         100);
   }
 }
 
 // Public virtual version.
 void URLRequestSlowDownloadJob::GetResponseInfo(net::HttpResponseInfo* info) {
   // Forward to private const version.
   GetResponseInfoConst(info);
 }
 
 URLRequestSlowDownloadJob::~URLRequestSlowDownloadJob() {}
 
 // Private const version.
 void URLRequestSlowDownloadJob::GetResponseInfoConst(
     net::HttpResponseInfo* info) const {
   // Send back mock headers.
   std::string raw_headers;
-  if (LowerCaseEqualsASCII(kFinishDownloadUrl,
-                           request_->url().spec().c_str())) {
+  if (IsCompletionUrl(request_->url())) {
     raw_headers.append(
       "HTTP/1.1 200 OK\n"
       "Content-type: text/plain\n");
   } else {
     raw_headers.append(
       "HTTP/1.1 200 OK\n"
       "Content-type: application/octet-stream\n"
       "Cache-Control: max-age=0\n");
 
     if (LowerCaseEqualsASCII(kKnownSizeUrl, request_->url().spec().c_str())) {
       raw_headers.append(base::StringPrintf(
           "Content-Length: %d\n",
           kFirstDownloadSize + kSecondDownloadSize));
     }
   }
 
   // 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);
 }