Revert 153131 (Histograms showed it doesn't help much)

Reland of http://codereview.chromium.org/10854204/, which
I reverted because of a bug in its unit tests.

Retry failed network requests.

R=willchan@chomium.org
BUG=143425

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

TBR=mmenke@chromium.org

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

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

3 files changed, 1 insertions, 209 deletions

diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index 309f0fb..901e3e7 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -49,17 +49,6 @@ 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);
@@ -258,9 +247,7 @@ URLRequestHttpJob::URLRequestHttpJob(URLRequest* request,
           base::Bind(&URLRequestHttpJob::OnHeadersReceivedCallback,
                      base::Unretained(this)))),
       awaiting_callback_(false),
-      http_transaction_delegate_(new HttpTransactionDelegateImpl(request)),
-      has_retried_(false),
-      error_before_retry_(OK) {
+      http_transaction_delegate_(new HttpTransactionDelegateImpl(request)) {
   URLRequestThrottlerManager* manager = request->context()->throttler_manager();
   if (manager)
     throttling_entry_ = manager->RegisterRequestUrl(request->url());
@@ -773,14 +760,6 @@ 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 =
@@ -837,8 +816,6 @@ 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));
   }
@@ -890,78 +867,6 @@ 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;
@@ -1416,38 +1321,6 @@ 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 8ee24b2..22e092ec 100644
--- a/net/url_request/url_request_http_job.h
+++ b/net/url_request/url_request_http_job.h
@@ -65,12 +65,6 @@ 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(
@@ -156,8 +150,6 @@ class URLRequestHttpJob : public URLRequestJob {
   void RecordTimer();
   void ResetTimer();
 
-  void RecordRetryResult(int result) const;
-
   virtual void UpdatePacketReadTimes() OVERRIDE;
   void RecordPacketStats(FilterContext::StatisticSelector statistic) const;
 
@@ -249,15 +241,6 @@ 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 44e4571..b815a0c 100644
--- a/net/url_request/url_request_job_unittest.cc
+++ b/net/url_request/url_request_job_unittest.cc
@@ -4,7 +4,6 @@
 
 #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"
@@ -20,15 +19,7 @@ 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(),
@@ -86,58 +77,3 @@ 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);
-  }
-}