Automatically retry failed network requests.

This is a temporary experiment to see if this behavior is
worth implementing in a cleaner fashion.

BUG=143425

Review URL: https://chromiumcodereview.appspot.com/10854204

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

6 files changed, 256 insertions, 30 deletions

diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index abbdad2..36d7c80 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -236,6 +236,7 @@ class FastTransactionServer {
 bool FastTransactionServer::no_store;
 
 const MockTransaction kFastNoStoreGET_Transaction = {
+  net::OK,
   "http://www.google.com/nostore",
   "GET",
   base::Time(),
@@ -373,6 +374,7 @@ void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request,
 }
 
 const MockTransaction kRangeGET_TransactionOK = {
+  net::OK,
   "http://www.google.com/range",
   "GET",
   base::Time(),
@@ -1791,14 +1793,14 @@ static void ConditionalizedRequestUpdatesCacheHelper(
 
   // We will control the network layer's responses for |kUrl| using
   // |mock_network_response|.
-  MockTransaction mock_network_response = { 0 };
+  MockTransaction mock_network_response = { net::OK, 0 };
   mock_network_response.url = kUrl;
   AddMockTransaction(&mock_network_response);
 
   // Request |kUrl| for the first time. It should hit the network and
   // receive |kNetResponse1|, which it saves into the HTTP cache.
 
-  MockTransaction request = { 0 };
+  MockTransaction request = { net::OK, 0 };
   request.url = kUrl;
   request.method = "GET";
   request.request_headers = "";
@@ -1982,11 +1984,11 @@ TEST(HttpCache, ConditionalizedRequestUpdatesCache4) {
 
   // We will control the network layer's responses for |kUrl| using
   // |mock_network_response|.
-  MockTransaction mock_network_response = { 0 };
+  MockTransaction mock_network_response = { net::OK, 0 };
   mock_network_response.url = kUrl;
   AddMockTransaction(&mock_network_response);
 
-  MockTransaction request = { 0 };
+  MockTransaction request = { net::OK, 0 };
   request.url = kUrl;
   request.method = "GET";
   request.request_headers = kExtraRequestHeaders;
@@ -2026,11 +2028,11 @@ TEST(HttpCache, ConditionalizedRequestUpdatesCache5) {
 
   // We will control the network layer's responses for |kUrl| using
   // |mock_network_response|.
-  MockTransaction mock_network_response = { 0 };
+  MockTransaction mock_network_response = { net::OK, 0 };
   mock_network_response.url = kUrl;
   AddMockTransaction(&mock_network_response);
 
-  MockTransaction request = { 0 };
+  MockTransaction request = { net::OK, 0 };
   request.url = kUrl;
   request.method = "GET";
   request.request_headers = kExtraRequestHeaders;
@@ -4658,14 +4660,14 @@ TEST(HttpCache, UpdatesRequestResponseTimeOn304) {
   const char* kUrl = "http://foobar";
   const char* kData = "body";
 
-  MockTransaction mock_network_response = { 0 };
+  MockTransaction mock_network_response = { net::OK, 0 };
   mock_network_response.url = kUrl;
 
   AddMockTransaction(&mock_network_response);
 
   // Request |kUrl|, causing |kNetResponse1| to be written to the cache.
 
-  MockTransaction request = { 0 };
+  MockTransaction request = { net::OK, 0 };
   request.url = kUrl;
   request.method = "GET";
   request.request_headers = "";
diff --git a/net/http/http_transaction_unittest.cc b/net/http/http_transaction_unittest.cc
index f8d1958..e6cceb1 100644
--- a/net/http/http_transaction_unittest.cc
+++ b/net/http/http_transaction_unittest.cc
@@ -27,6 +27,7 @@ static MockTransactionMap mock_transactions;
 // mock transaction data
 
 const MockTransaction kSimpleGET_Transaction = {
+  net::OK,
   "http://www.google.com/",
   "GET",
   base::Time(),
@@ -42,6 +43,7 @@ const MockTransaction kSimpleGET_Transaction = {
 };
 
 const MockTransaction kSimplePOST_Transaction = {
+  net::OK,
   "http://bugdatabase.com/edit",
   "POST",
   base::Time(),
@@ -57,6 +59,7 @@ const MockTransaction kSimplePOST_Transaction = {
 };
 
 const MockTransaction kTypicalGET_Transaction = {
+  net::OK,
   "http://www.example.com/~foo/bar.html",
   "GET",
   base::Time(),
@@ -73,6 +76,7 @@ const MockTransaction kTypicalGET_Transaction = {
 };
 
 const MockTransaction kETagGET_Transaction = {
+  net::OK,
   "http://www.google.com/foopy",
   "GET",
   base::Time(),
@@ -89,6 +93,7 @@ const MockTransaction kETagGET_Transaction = {
 };
 
 const MockTransaction kRangeGET_Transaction = {
+  net::OK,
   "http://www.google.com/",
   "GET",
   base::Time(),
@@ -126,6 +131,9 @@ const MockTransaction* FindMockTransaction(const GURL& url) {
 }
 
 void AddMockTransaction(const MockTransaction* trans) {
+  // To return a result asynchronously, set the TEST_MODE_SYNC_NET_START bit
+  // of |test_mode|.
+  ASSERT_NE(net::ERR_IO_PENDING, trans->start_result);
   mock_transactions[GURL(trans->url).spec()] = trans;
 }
 
@@ -230,35 +238,38 @@ int MockNetworkTransaction::Start(const net::HttpRequestInfo* request,
   if (!t)
     return net::ERR_FAILED;
 
-  std::string resp_status = t->status;
-  std::string resp_headers = t->response_headers;
-  std::string resp_data = t->data;
-  if (t->handler)
-    (t->handler)(request, &resp_status, &resp_headers, &resp_data);
+  net::Error result = t->start_result;
+  if (result == net::OK) {
+    std::string resp_status = t->status;
+    std::string resp_headers = t->response_headers;
+    std::string resp_data = t->data;
+    if (t->handler)
+      (t->handler)(request, &resp_status, &resp_headers, &resp_data);
 
-  std::string header_data = base::StringPrintf(
-      "%s\n%s\n", resp_status.c_str(), resp_headers.c_str());
-  std::replace(header_data.begin(), header_data.end(), '\n', '\0');
+    std::string header_data = base::StringPrintf(
+        "%s\n%s\n", resp_status.c_str(), resp_headers.c_str());
+    std::replace(header_data.begin(), header_data.end(), '\n', '\0');
 
-  response_.request_time = base::Time::Now();
-  if (!t->request_time.is_null())
-    response_.request_time = t->request_time;
+    response_.request_time = base::Time::Now();
+    if (!t->request_time.is_null())
+      response_.request_time = t->request_time;
 
-  response_.was_cached = false;
+    response_.was_cached = false;
 
-  response_.response_time = base::Time::Now();
-  if (!t->response_time.is_null())
-    response_.response_time = t->response_time;
+    response_.response_time = base::Time::Now();
+    if (!t->response_time.is_null())
+      response_.response_time = t->response_time;
 
-  response_.headers = new net::HttpResponseHeaders(header_data);
-  response_.ssl_info.cert_status = t->cert_status;
-  data_ = resp_data;
-  test_mode_ = t->test_mode;
+    response_.headers = new net::HttpResponseHeaders(header_data);
+    response_.ssl_info.cert_status = t->cert_status;
+    data_ = resp_data;
+    test_mode_ = t->test_mode;
+  }
 
   if (test_mode_ & TEST_MODE_SYNC_NET_START)
-    return net::OK;
+    return result;
 
-  CallbackLater(callback, net::OK);
+  CallbackLater(callback, result);
   return net::ERR_IO_PENDING;
 }
 
diff --git a/net/http/http_transaction_unittest.h b/net/http/http_transaction_unittest.h
index 43a9fa7..15b893c 100644
--- a/net/http/http_transaction_unittest.h
+++ b/net/http/http_transaction_unittest.h
@@ -48,6 +48,11 @@ typedef void (*MockTransactionHandler)(const net::HttpRequestInfo* request,
                                        std::string* response_data);
 
 struct MockTransaction {
+  // Net error code returned as a result of starting the transaction.  If not
+  // net::OK, all response-related fields will be ignored.  Must not be
+  // ERR_IO_PENDING.
+  net::Error start_result;
+
   const char* url;
   const char* method;
   // If |request_time| is unspecified, the current time will be used.
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index f763bcf..1d60eef 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -49,6 +49,17 @@ static const char kAvailDictionaryHeader[] = "Avail-Dictionary";
 
 namespace net {
 
+namespace {
+
+// Array of all network error codes.  Needed for histograms.
+const int kAllNetErrorCodes[] = {
+#define NET_ERROR(label, value) -(value),
+#include "net/base/net_error_list.h"
+#undef NET_ERROR
+};
+
+}  // namespace
+
 class URLRequestHttpJob::HttpFilterContext : public FilterContext {
  public:
   explicit HttpFilterContext(URLRequestHttpJob* job);
@@ -244,7 +255,9 @@ URLRequestHttpJob::URLRequestHttpJob(URLRequest* request)
           base::Bind(&URLRequestHttpJob::OnHeadersReceivedCallback,
                      base::Unretained(this)))),
       awaiting_callback_(false),
-      http_transaction_delegate_(new HttpTransactionDelegateImpl(request)) {
+      http_transaction_delegate_(new HttpTransactionDelegateImpl(request)),
+      has_retried_(false),
+      error_before_retry_(OK) {
   URLRequestThrottlerManager* manager = request->context()->throttler_manager();
   if (manager)
     throttling_entry_ = manager->RegisterRequestUrl(request->url());
@@ -757,6 +770,14 @@ void URLRequestHttpJob::OnStartCompleted(int result) {
 
   const URLRequestContext* context = request_->context();
 
+  // If this was a retry, record whether or not we successfully connected to
+  // the server on the second try.
+  if (error_before_retry_ != OK) {
+    DCHECK(has_retried_);
+    RecordRetryResult(result);
+    error_before_retry_ = OK;
+  }
+
   if (result == ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN &&
       transaction_->GetResponseInfo() != NULL) {
     FraudulentCertificateReporter* reporter =
@@ -813,6 +834,8 @@ void URLRequestHttpJob::OnStartCompleted(int result) {
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     NotifyCertificateRequested(
         transaction_->GetResponseInfo()->cert_request_info);
+  } else if (ShouldRetryRequest(result)) {
+    RetryRequestOnError(result);
   } else {
     NotifyStartError(URLRequestStatus(URLRequestStatus::FAILED, result));
   }
@@ -864,6 +887,78 @@ void URLRequestHttpJob::RestartTransactionWithAuth(
   AddCookieHeaderAndStart();
 }
 
+void URLRequestHttpJob::RetryRequestOnError(int result) {
+  // Should have a transaction that returned the result, but should not have
+  // received headers yet.
+  DCHECK(transaction_.get());
+  DCHECK(!response_info_);
+  DCHECK(result != OK && result != ERR_IO_PENDING);
+
+  error_before_retry_ = result;
+  has_retried_ = true;
+  ResetTimer();
+  transaction_.reset();
+
+  SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
+  StartTransactionInternal();
+}
+
+bool URLRequestHttpJob::ShouldRetryRequest(int result) const {
+  // Request must not have been cancelled, and no response info may have been
+  // received yet.
+  DCHECK(transaction_.get());
+  DCHECK(request_);
+  DCHECK(!response_info_);
+
+  // Don't retry if the request has already been retried or it's not a GET.
+  if (has_retried_ || request_->method() != "GET") {
+    return false;
+  }
+
+
+  // If the HTTP job has already taken too long, do not retry the request.
+  base::TimeDelta request_duration = base::TimeTicks::Now() - start_time_;
+  if (request_duration >= base::TimeDelta::FromSeconds(10))
+    return false;
+
+  // TODO(mmenke):  Look at the metrics and figure out which, if any, of these
+  //                error codes it's worth retrying on.
+  switch (result) {
+    // These four error codes will also result in a retry in NetworkTransaction,
+    // in the case of a reused socket.  It's unclear how much correlation there
+    // is between receiving one of these errors on a reused socket and on a
+    // fresh socket on retry.
+    case ERR_CONNECTION_RESET:
+    case ERR_CONNECTION_CLOSED:
+    case ERR_CONNECTION_ABORTED:
+    case ERR_SOCKET_NOT_CONNECTED:
+
+    // System errors.
+    case ERR_FAILED:
+    case ERR_UNEXPECTED:
+
+    // Connection errors.
+    case ERR_CONNECTION_REFUSED:
+    case ERR_CONNECTION_FAILED:
+    case ERR_NAME_NOT_RESOLVED:
+    case ERR_INTERNET_DISCONNECTED:
+    case ERR_ADDRESS_UNREACHABLE:
+    case ERR_NETWORK_ACCESS_DENIED:
+    case ERR_ADDRESS_IN_USE:
+
+    // Content errors.
+    case ERR_EMPTY_RESPONSE:
+
+    // Cache errors.
+    case ERR_CACHE_MISS:
+    case ERR_CACHE_READ_FAILURE:
+      return true;
+
+    default:
+      return false;
+  }
+}
+
 void URLRequestHttpJob::SetUpload(UploadData* upload) {
   DCHECK(!transaction_.get()) << "cannot change once started";
   request_info_.upload_data = upload;
@@ -1317,6 +1412,38 @@ void URLRequestHttpJob::ResetTimer() {
   request_creation_time_ = base::Time::Now();
 }
 
+void URLRequestHttpJob::RecordRetryResult(int result) const {
+  if (request_info_.load_flags & LOAD_MAIN_FRAME) {
+    if (result != OK) {
+      UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+          "Net.NetworkErrorsRecovered.MainFrame",
+          -error_before_retry_,
+          base::CustomHistogram::ArrayToCustomRanges(
+              kAllNetErrorCodes, arraysize(kAllNetErrorCodes)));
+    } else {
+      UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+          "Net.NetworkErrorsUnrecovered.MainFrame",
+          -error_before_retry_,
+          base::CustomHistogram::ArrayToCustomRanges(
+              kAllNetErrorCodes, arraysize(kAllNetErrorCodes)));
+    }
+  } else {
+    if (result != OK) {
+      UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+          "Net.NetworkErrorsRecovered.Subresource",
+          -error_before_retry_,
+          base::CustomHistogram::ArrayToCustomRanges(
+              kAllNetErrorCodes, arraysize(kAllNetErrorCodes)));
+    } else {
+      UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+          "Net.NetworkErrorsUnrecovered.Subresource",
+          -error_before_retry_,
+          base::CustomHistogram::ArrayToCustomRanges(
+              kAllNetErrorCodes, arraysize(kAllNetErrorCodes)));
+    }
+  }
+}
+
 void URLRequestHttpJob::UpdatePacketReadTimes() {
   if (!packet_timing_enabled_)
     return;
diff --git a/net/url_request/url_request_http_job.h b/net/url_request/url_request_http_job.h
index 874955e..d028348 100644
--- a/net/url_request/url_request_http_job.h
+++ b/net/url_request/url_request_http_job.h
@@ -64,6 +64,12 @@ class URLRequestHttpJob : public URLRequestJob {
 
   void RestartTransactionWithAuth(const AuthCredentials& credentials);
 
+  void RetryRequestOnError(int result);
+
+  // Returns true if this request should be retried after receiving the given
+  // error as a result of a call to Start.
+  bool ShouldRetryRequest(int result) const;
+
   // Overridden from URLRequestJob:
   virtual void SetUpload(UploadData* upload) OVERRIDE;
   virtual void SetExtraRequestHeaders(
@@ -149,6 +155,8 @@ class URLRequestHttpJob : public URLRequestJob {
   void RecordTimer();
   void ResetTimer();
 
+  void RecordRetryResult(int result) const;
+
   virtual void UpdatePacketReadTimes() OVERRIDE;
   void RecordPacketStats(FilterContext::StatisticSelector statistic) const;
 
@@ -240,6 +248,15 @@ class URLRequestHttpJob : public URLRequestJob {
 
   scoped_ptr<HttpTransactionDelegateImpl> http_transaction_delegate_;
 
+  // True if the request job has automatically retried the request as a result
+  // of an error.
+  bool has_retried_;
+
+  // The network error code error that |this| was automatically retried as a
+  // result of, if any.  If this request has not been retried, must be 0.
+  // Reset once it's been used to log histograms.
+  int error_before_retry_;
+
   DISALLOW_COPY_AND_ASSIGN(URLRequestHttpJob);
 };
 
diff --git a/net/url_request/url_request_job_unittest.cc b/net/url_request/url_request_job_unittest.cc
index b815a0c..44e4571 100644
--- a/net/url_request/url_request_job_unittest.cc
+++ b/net/url_request/url_request_job_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "net/url_request/url_request_job.h"
 
+#include "base/basictypes.h"
 #include "net/http/http_transaction_unittest.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -19,7 +20,15 @@ void GZipServer(const net::HttpRequestInfo* request,
   response_data->assign(kGzipGata, sizeof(kGzipGata));
 }
 
+void NullServer(const net::HttpRequestInfo* request,
+                std::string* response_status,
+                std::string* response_headers,
+                std::string* response_data) {
+  FAIL();
+}
+
 const MockTransaction kGZip_Transaction = {
+  net::OK,
   "http://www.google.com/gzyp",
   "GET",
   base::Time(),
@@ -77,3 +86,58 @@ TEST(URLRequestJob, SyncTransactionNotifiedWhenDone) {
 
   RemoveMockTransaction(&transaction);
 }
+
+// Tests that automatic retry triggers for certain errors on GETs, and is not
+// triggered for other errors or with other methods.
+TEST(URLRequestJob, RetryRequests) {
+  const struct {
+    net::Error transaction_result;
+    const char* method;
+    int expected_transaction_count;
+  } tests[] = {
+    {net::ERR_EMPTY_RESPONSE, "GET", 2},
+    {net::ERR_CONNECTION_REFUSED, "GET", 2},
+    {net::ERR_CONNECTION_REFUSED, "POST", 1},
+    {net::ERR_CONNECTION_REFUSED, "PUT", 1},
+    {net::ERR_ACCESS_DENIED, "GET", 1},
+    {net::ERR_CONTENT_DECODING_FAILED, "GET", 1},
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    const MockTransaction dead_transaction = {
+      tests[i].transaction_result,
+      "http://www.dead.com/",
+      tests[i].method,
+      base::Time(),
+      "",
+      net::LOAD_NORMAL,
+      "",
+      "",
+      base::Time(),
+      "",
+      TEST_MODE_NORMAL,
+      &NullServer,
+      0
+    };
+
+    MockNetworkLayer network_layer;
+    TestURLRequestContext context;
+    context.set_http_transaction_factory(&network_layer);
+
+    TestDelegate d;
+    TestURLRequest req(GURL(dead_transaction.url), &d, &context);
+    MockTransaction transaction(dead_transaction);
+    transaction.test_mode = TEST_MODE_SYNC_ALL;
+    AddMockTransaction(&transaction);
+
+    req.set_method(tests[i].method);
+    req.Start();
+
+    MessageLoop::current()->Run();
+
+    EXPECT_EQ(tests[i].expected_transaction_count,
+              network_layer.transaction_count());
+
+    RemoveMockTransaction(&transaction);
+  }
+}