Update UMA to track bypasses due to 4xx responses that are missing the proxy's via header and bypasses due to network errors.

BUG=376148

Review URL: https://codereview.chromium.org/298883011

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

9 files changed, 130 insertions, 28 deletions

diff --git a/google_apis/gcm/engine/connection_factory_impl.cc b/google_apis/gcm/engine/connection_factory_impl.cc
index 76d2832..bab0a95 100644
--- a/google_apis/gcm/engine/connection_factory_impl.cc
+++ b/google_apis/gcm/engine/connection_factory_impl.cc
@@ -457,7 +457,7 @@ int ConnectionFactoryImpl::ReconsiderProxyAfterError(int error) {
   }
 
   int status = network_session_->proxy_service()->ReconsiderProxyAfterError(
-      GetCurrentEndpoint(), &proxy_info_,
+      GetCurrentEndpoint(), error, &proxy_info_,
       base::Bind(&ConnectionFactoryImpl::OnProxyResolveDone,
                  weak_ptr_factory_.GetWeakPtr()),
       &pac_request_,
diff --git a/jingle/glue/proxy_resolving_client_socket.cc b/jingle/glue/proxy_resolving_client_socket.cc
index 8933cd9..7622c21 100644
--- a/jingle/glue/proxy_resolving_client_socket.cc
+++ b/jingle/glue/proxy_resolving_client_socket.cc
@@ -277,7 +277,7 @@ int ProxyResolvingClientSocket::ReconsiderProxyAfterError(int error) {
   }
 
   int rv = network_session_->proxy_service()->ReconsiderProxyAfterError(
-      proxy_url_, &proxy_info_, proxy_resolve_callback_, &pac_request_,
+      proxy_url_, error, &proxy_info_, proxy_resolve_callback_, &pac_request_,
       bound_net_log_);
   if (rv == net::OK || rv == net::ERR_IO_PENDING) {
     CloseTransportSocket();
diff --git a/net/http/http_response_headers.cc b/net/http/http_response_headers.cc
index 7ecf37a..f07fbaf 100644
--- a/net/http/http_response_headers.cc
+++ b/net/http/http_response_headers.cc
@@ -1504,6 +1504,13 @@ HttpResponseHeaders::GetDataReductionProxyBypassEventType(
     // response is to minimize information transfer, a sender in general
     // should not generate representation metadata other than Cache-Control,
     // Content-Location, Date, ETag, Expires, and Vary.
+
+    // The proxy Via header might also not be present in a 4xx response.
+    // Separate this case from other responses that are missing the header.
+    if (response_code() >= HTTP_BAD_REQUEST &&
+        response_code() < HTTP_INTERNAL_SERVER_ERROR) {
+      return ProxyService::PROXY_4XX_BYPASS;
+    }
     return ProxyService::MISSING_VIA_HEADER;
   }
   // There is no bypass event.
diff --git a/net/http/http_response_headers_unittest.cc b/net/http/http_response_headers_unittest.cc
index c9c8c46..b097a18 100644
--- a/net/http/http_response_headers_unittest.cc
+++ b/net/http/http_response_headers_unittest.cc
@@ -2245,6 +2245,17 @@ TEST(HttpResponseHeadersTest, GetDataReductionProxyBypassEventType) {
       "Chrome-Proxy: bypass=1799\n",
       net::ProxyService::SHORT_BYPASS,
     },
+    { "HTTP/1.1 502 Bad Gateway\n"
+      "Chrome-Proxy: bypass=1799\n",
+      net::ProxyService::SHORT_BYPASS,
+    },
+    { "HTTP/1.1 414 Request-URI Too Long\n",
+      net::ProxyService::PROXY_4XX_BYPASS,
+    },
+    { "HTTP/1.1 414 Request-URI Too Long\n"
+      "Via: 1.1 Chrome-Compression-Proxy\n",
+      net::ProxyService::BYPASS_EVENT_TYPE_MAX,
+    }
   };
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     std::string headers(tests[i].headers);
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 647172d..93b9ef2 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -1317,7 +1317,8 @@ int HttpStreamFactoryImpl::Job::ReconsiderProxyAfterError(int error) {
   }
 
   int rv = session_->proxy_service()->ReconsiderProxyAfterError(
-      request_info_.url, &proxy_info_, io_callback_, &pac_request_, net_log_);
+      request_info_.url, error, &proxy_info_, io_callback_, &pac_request_,
+      net_log_);
   if (rv == OK || rv == ERR_IO_PENDING) {
     // If the error was during connection setup, there is no socket to
     // disconnect.
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index b2fcc0e..b70ea2c 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -47,6 +47,7 @@
 
 #if defined(SPDY_PROXY_AUTH_ORIGIN)
 #include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
 #endif
 
 using base::TimeDelta;
@@ -1168,6 +1169,7 @@ void ProxyService::OnInitProxyResolverComplete(int result) {
 }
 
 int ProxyService::ReconsiderProxyAfterError(const GURL& url,
+                                            int net_error,
                                             ProxyInfo* result,
                                             const CompletionCallback& callback,
                                             PacRequest** pac_request,
@@ -1191,9 +1193,13 @@ int ProxyService::ReconsiderProxyAfterError(const GURL& url,
   if (result->proxy_server().isDataReductionProxy()) {
     RecordDataReductionProxyBypassInfo(
         true, result->proxy_server(), ERROR_BYPASS);
+    RecordDataReductionProxyBypassOnNetworkError(
+        true, result->proxy_server(), net_error);
   } else if (result->proxy_server().isDataReductionProxyFallback()) {
     RecordDataReductionProxyBypassInfo(
         false, result->proxy_server(), ERROR_BYPASS);
+    RecordDataReductionProxyBypassOnNetworkError(
+        false, result->proxy_server(), net_error);
   }
 #endif
 
@@ -1439,6 +1445,25 @@ void ProxyService::RecordDataReductionProxyBypassInfo(
                               bypass_type, BYPASS_EVENT_TYPE_MAX);
   }
 }
+
+void ProxyService::RecordDataReductionProxyBypassOnNetworkError(
+    bool is_primary,
+    const ProxyServer& proxy_server,
+    int net_error) {
+  // Only record UMA if the proxy isn't already on the retry list.
+  if (proxy_retry_info_.find(proxy_server.ToURI()) != proxy_retry_info_.end())
+    return;
+
+  if (is_primary) {
+    UMA_HISTOGRAM_SPARSE_SLOWLY(
+        "DataReductionProxy.BypassOnNetworkErrorPrimary",
+        std::abs(net_error));
+    return;
+  }
+  UMA_HISTOGRAM_SPARSE_SLOWLY(
+      "DataReductionProxy.BypassOnNetworkErrorFallback",
+      std::abs(net_error));
+}
 #endif  // defined(SPDY_PROXY_AUTH_ORIGIN)
 
 void ProxyService::OnProxyConfigChanged(
@@ -1581,13 +1606,14 @@ int SyncProxyServiceHelper::ResolveProxy(const GURL& url,
 }
 
 int SyncProxyServiceHelper::ReconsiderProxyAfterError(
-    const GURL& url, ProxyInfo* proxy_info, const BoundNetLog& net_log) {
+    const GURL& url, int net_error, ProxyInfo* proxy_info,
+    const BoundNetLog& net_log) {
   DCHECK(io_message_loop_ != base::MessageLoop::current());
 
   io_message_loop_->PostTask(
       FROM_HERE,
       base::Bind(&SyncProxyServiceHelper::StartAsyncReconsider, this, url,
-                 net_log));
+                 net_error, net_log));
 
   event_.Wait();
 
@@ -1609,9 +1635,10 @@ void SyncProxyServiceHelper::StartAsyncResolve(const GURL& url,
 }
 
 void SyncProxyServiceHelper::StartAsyncReconsider(const GURL& url,
+                                                  int net_error,
                                                   const BoundNetLog& net_log) {
   result_ = proxy_service_->ReconsiderProxyAfterError(
-      url, &proxy_info_, callback_, NULL, net_log);
+      url, net_error, &proxy_info_, callback_, NULL, net_log);
   if (result_ != net::ERR_IO_PENDING) {
     OnCompletion(result_);
   }
diff --git a/net/proxy/proxy_service.h b/net/proxy/proxy_service.h
index 36b4589..6f85568 100644
--- a/net/proxy/proxy_service.h
+++ b/net/proxy/proxy_service.h
@@ -132,8 +132,10 @@ class NET_EXPORT ProxyService : public NetworkChangeNotifier::IPAddressObserver,
   // This method is called after a failure to connect or resolve a host name.
   // It gives the proxy service an opportunity to reconsider the proxy to use.
   // The |results| parameter contains the results returned by an earlier call
-  // to ResolveProxy.  The semantics of this call are otherwise similar to
-  // ResolveProxy.
+  // to ResolveProxy.  The |net_error| parameter contains the network error
+  // code associated with the failure. See "net/base/net_error_list.h" for a
+  // list of possible values. The semantics of this call are otherwise
+  // similar to ResolveProxy.
   //
   // NULL can be passed for |pac_request| if the caller will not need to
   // cancel the request.
@@ -142,6 +144,7 @@ class NET_EXPORT ProxyService : public NetworkChangeNotifier::IPAddressObserver,
   //
   // Profiling information for the request is saved to |net_log| if non-NULL.
   int ReconsiderProxyAfterError(const GURL& url,
+                                int net_error,
                                 ProxyInfo* results,
                                 const CompletionCallback& callback,
                                 PacRequest** pac_request,
@@ -286,6 +289,9 @@ class NET_EXPORT ProxyService : public NetworkChangeNotifier::IPAddressObserver,
     // Bypass the proxy because responses appear not to be coming via it.
     MISSING_VIA_HEADER,
 
+    // Bypass the proxy because the proxy, not the origin, sent a 4xx response.
+    PROXY_4XX_BYPASS,
+
     // This must always be last.
     BYPASS_EVENT_TYPE_MAX
   };
@@ -296,6 +302,13 @@ class NET_EXPORT ProxyService : public NetworkChangeNotifier::IPAddressObserver,
       bool is_primary,
       const ProxyServer& proxy_server,
       DataReductionProxyBypassEventType bypass_type) const;
+
+  // Records a net error code that resulted in bypassing the data reduction
+  // proxy (|is_primary| is true) or the data reduction proxy fallback.
+  void RecordDataReductionProxyBypassOnNetworkError(
+      bool is_primary,
+      const ProxyServer& proxy_server,
+      int net_error);
 #endif
 
  private:
@@ -458,6 +471,7 @@ class NET_EXPORT SyncProxyServiceHelper
                    ProxyInfo* proxy_info,
                    const BoundNetLog& net_log);
   int ReconsiderProxyAfterError(const GURL& url,
+                                int net_error,
                                 ProxyInfo* proxy_info,
                                 const BoundNetLog& net_log);
 
@@ -467,7 +481,9 @@ class NET_EXPORT SyncProxyServiceHelper
   virtual ~SyncProxyServiceHelper();
 
   void StartAsyncResolve(const GURL& url, const BoundNetLog& net_log);
-  void StartAsyncReconsider(const GURL& url, const BoundNetLog& net_log);
+  void StartAsyncReconsider(const GURL& url,
+                            int net_error,
+                            const BoundNetLog& net_log);
 
   void OnCompletion(int result);
 
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index bad5afd..cc245b6 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -315,7 +315,8 @@ TEST_F(ProxyServiceTest, PAC_FailoverWithoutDirect) {
   // DIRECT.
   TestCompletionCallback callback2;
   rv = service.ReconsiderProxyAfterError(
-      url, &info, callback2.callback(), NULL, BoundNetLog());
+      url, net::ERR_PROXY_CONNECTION_FAILED,
+      &info, callback2.callback(), NULL, BoundNetLog());
   // ReconsiderProxyAfterError returns error indicating nothing left.
   EXPECT_EQ(ERR_FAILED, rv);
   EXPECT_TRUE(info.is_empty());
@@ -410,7 +411,8 @@ TEST_F(ProxyServiceTest, PAC_FailoverAfterDirect) {
 
   // Fallback 1.
   TestCompletionCallback callback2;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback2.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback2.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_FALSE(info.is_direct());
@@ -418,14 +420,16 @@ TEST_F(ProxyServiceTest, PAC_FailoverAfterDirect) {
 
   // Fallback 2.
   TestCompletionCallback callback3;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback3.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback3.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_TRUE(info.is_direct());
 
   // Fallback 3.
   TestCompletionCallback callback4;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback4.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback4.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_FALSE(info.is_direct());
@@ -433,7 +437,8 @@ TEST_F(ProxyServiceTest, PAC_FailoverAfterDirect) {
 
   // Fallback 4 -- Nothing to fall back to!
   TestCompletionCallback callback5;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback5.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback5.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(ERR_FAILED, rv);
   EXPECT_TRUE(info.is_empty());
@@ -728,7 +733,8 @@ TEST_F(ProxyServiceTest, ProxyFallback) {
 
   // Fake an error on the proxy.
   TestCompletionCallback callback2;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback2.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback2.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
 
@@ -770,7 +776,8 @@ TEST_F(ProxyServiceTest, ProxyFallback) {
 
   // We fake another error. It should now try the third one.
   TestCompletionCallback callback4;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback4.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback4.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("foopy2:9090", info.proxy_server().ToURI());
@@ -779,7 +786,8 @@ TEST_F(ProxyServiceTest, ProxyFallback) {
   // proxy servers we thought were valid; next we try the proxy server
   // that was in our bad proxies map (foopy1:8080).
   TestCompletionCallback callback5;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback5.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback5.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("foopy1:8080", info.proxy_server().ToURI());
@@ -787,7 +795,8 @@ TEST_F(ProxyServiceTest, ProxyFallback) {
   // Fake another error, the last proxy is gone, the list should now be empty,
   // so there is nothing left to try.
   TestCompletionCallback callback6;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback6.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback6.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(ERR_FAILED, rv);
   EXPECT_FALSE(info.is_direct());
@@ -861,7 +870,8 @@ TEST_F(ProxyServiceTest, ProxyFallbackToDirect) {
 
   // Fake an error on the proxy.
   TestCompletionCallback callback2;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback2.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback2.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
 
@@ -870,7 +880,8 @@ TEST_F(ProxyServiceTest, ProxyFallbackToDirect) {
 
   // Fake an error on this proxy as well.
   TestCompletionCallback callback3;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback3.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback3.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
 
@@ -883,7 +894,8 @@ TEST_F(ProxyServiceTest, ProxyFallbackToDirect) {
 
   // Now we tell the proxy service that even DIRECT failed.
   TestCompletionCallback callback4;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback4.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback4.callback(), NULL,
                                          BoundNetLog());
   // There was nothing left to try after DIRECT, so we are out of
   // choices.
@@ -931,7 +943,8 @@ TEST_F(ProxyServiceTest, ProxyFallback_NewSettings) {
       ProxyConfig::CreateFromCustomPacURL(GURL("http://foopy-new/proxy.pac")));
 
   TestCompletionCallback callback2;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback2.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback2.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
@@ -952,7 +965,8 @@ TEST_F(ProxyServiceTest, ProxyFallback_NewSettings) {
 
   // We fake another error. It should now ignore the first one.
   TestCompletionCallback callback3;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback3.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback3.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("foopy2:9090", info.proxy_server().ToURI());
@@ -964,7 +978,8 @@ TEST_F(ProxyServiceTest, ProxyFallback_NewSettings) {
 
   // We fake another error. It should go back to the first proxy.
   TestCompletionCallback callback4;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback4.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback4.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
@@ -1023,7 +1038,8 @@ TEST_F(ProxyServiceTest, ProxyFallback_BadConfig) {
 
   // Fake a proxy error.
   TestCompletionCallback callback2;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback2.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback2.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
 
@@ -1055,7 +1071,8 @@ TEST_F(ProxyServiceTest, ProxyFallback_BadConfig) {
   // "just work" the next time we call it.
   ProxyInfo info3;
   TestCompletionCallback callback4;
-  rv = service.ReconsiderProxyAfterError(url, &info3, callback4.callback(),
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info3, callback4.callback(),
                                          NULL, BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
@@ -1116,7 +1133,8 @@ TEST_F(ProxyServiceTest, ProxyFallback_BadConfigMandatory) {
 
   // Fake a proxy error.
   TestCompletionCallback callback2;
-  rv = service.ReconsiderProxyAfterError(url, &info, callback2.callback(), NULL,
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info, callback2.callback(), NULL,
                                          BoundNetLog());
   EXPECT_EQ(OK, rv);
 
@@ -1149,7 +1167,8 @@ TEST_F(ProxyServiceTest, ProxyFallback_BadConfigMandatory) {
   // "just work" the next time we call it.
   ProxyInfo info3;
   TestCompletionCallback callback4;
-  rv = service.ReconsiderProxyAfterError(url, &info3, callback4.callback(),
+  rv = service.ReconsiderProxyAfterError(url, net::ERR_PROXY_CONNECTION_FAILED,
+                                         &info3, callback4.callback(),
                                          NULL, BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 5494219..96b8529 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -2853,6 +2853,26 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   </summary>
 </histogram>
 
+<histogram name="DataReductionProxy.BypassOnNetworkErrorFallback"
+    enum="NetErrorCodes">
+  <owner>bengr@chromium.org</owner>
+  <summary>
+    Positive net error code that caused the fallback data reduction proxy to be
+    bypassed and put on the proxy retry list. Called after a failure to connect
+    or resolve a host name.
+  </summary>
+</histogram>
+
+<histogram name="DataReductionProxy.BypassOnNetworkErrorPrimary"
+    enum="NetErrorCodes">
+  <owner>bengr@chromium.org</owner>
+  <summary>
+    Positive net error code that caused the primary data reduction proxy to be
+    bypassed and put on the proxy retry list. Called after a failure to connect
+    or resolve a host name.
+  </summary>
+</histogram>
+
 <histogram name="DataReductionProxy.ProbeURL"
     enum="DataReductionProxyProbeURLFetchResult">
   <owner>bengr@chromium.org</owner>
@@ -32988,6 +33008,7 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <int value="2" label="Bypass due to internal server error"/>
   <int value="3" label="Bypass due to other error"/>
   <int value="4" label="Bypass due to missing via header"/>
+  <int value="5" label="Bypass due to 4xx response"/>
 </enum>
 
 <enum name="DataReductionProxyProbeURLFetchResult" type="int">