Modified cancel and interrupt processing to avoid race with history.

content/browser/net/url_request_slow_download_job.cc

Index: content/browser/net/url_request_slow_download_job.cc
diff --git a/content/browser/net/url_request_slow_download_job.cc b/content/browser/net/url_request_slow_download_job.cc
index ef1370ff32329b41538af42a002002efbe73ef68..87584b092f8cea6ffa930bace47644cd8d55b72d 100644
--- a/content/browser/net/url_request_slow_download_job.cc
+++ b/content/browser/net/url_request_slow_download_job.cc
@@ -10,9 +10,11 @@
 #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;
@@ -23,10 +25,21 @@ 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,
@@ -43,6 +56,8 @@ void URLRequestSlowDownloadJob::AddUrlHandler() {
                         &URLRequestSlowDownloadJob::Factory);
   filter->AddUrlHandler(GURL(kFinishDownloadUrl),
                         &URLRequestSlowDownloadJob::Factory);
+  filter->AddUrlHandler(GURL(kErrorFinishDownloadUrl),
+                        &URLRequestSlowDownloadJob::Factory);
 }
 
 /*static */
@@ -50,17 +65,21 @@ 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();
 }
@@ -70,19 +89,24 @@ URLRequestSlowDownloadJob::URLRequestSlowDownloadJob(net::URLRequest* 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;
   }
@@ -132,6 +156,9 @@ void URLRequestSlowDownloadJob::CheckDoneStatus() {
     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,
@@ -154,8 +181,7 @@ 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");