Keep track of network error in ProxyRetryInfo.

Use it to log error type in BypassOneNetworkError UMA. This fixes DataReductionProxy.BypassOnNetworkError UMA recording bug. BUG=395769 Review URL: https://codereview.chromium.org/473513002 Cr-Commit-Position: refs/heads/master@{#290054} git-svn-id: svn://svn.chromium.org/chrome/trunk/src@290054 0039d316-1c4b-4281-b951-d872f2087c98
280723148 · Aug 15, 2014 · d0483b1 · d0483b1
1 parent 60c65a3
commit d0483b1
Show file tree

Hide file tree

Showing 19 changed files with 128 additions and 92 deletions.
diff --git a/chrome/browser/net/chrome_network_delegate.cc b/chrome/browser/net/chrome_network_delegate.cc
@@ -443,11 +443,10 @@ void ChromeNetworkDelegate::OnResolveProxy(
 }
 
 void ChromeNetworkDelegate::OnProxyFallback(const net::ProxyServer& bad_proxy,
-                                            int net_error,
-                                            bool did_fallback) {
+                                            int net_error) {
   if (data_reduction_proxy_usage_stats_) {
     data_reduction_proxy_usage_stats_->RecordBypassEventHistograms(
-        bad_proxy, net_error, did_fallback);
+        bad_proxy, net_error);
   }
 }
 

diff --git a/chrome/browser/net/chrome_network_delegate.h b/chrome/browser/net/chrome_network_delegate.h
@@ -216,8 +216,7 @@ class ChromeNetworkDelegate : public net::NetworkDelegate {
       const net::ProxyService& proxy_service,
       net::ProxyInfo* result) OVERRIDE;
   virtual void OnProxyFallback(const net::ProxyServer& bad_proxy,
-                               int net_error,
-                               bool did_fallback) OVERRIDE;
+                               int net_error) OVERRIDE;
   virtual int OnBeforeSendHeaders(net::URLRequest* request,
                                   const net::CompletionCallback& callback,
                                   net::HttpRequestHeaders* headers) OVERRIDE;

diff --git a/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc b/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
@@ -223,8 +223,7 @@ void DataReductionProxyUsageStats::RecordBypassedBytesHistograms(
 
 void DataReductionProxyUsageStats::RecordBypassEventHistograms(
     const net::ProxyServer& bypassed_proxy,
-    int net_error,
-    bool did_fallback) const {
+    int net_error) const {
   DataReductionProxyTypeInfo data_reduction_proxy_info;
   if (bypassed_proxy.is_valid() && !bypassed_proxy.is_direct() &&
       data_reduction_proxy_params_->IsDataReductionProxy(

diff --git a/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h b/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h
@@ -64,8 +64,7 @@ class DataReductionProxyUsageStats
       const BooleanPrefMember& data_reduction_proxy_enabled);
 
   void RecordBypassEventHistograms(const net::ProxyServer& bypassed_proxy,
-                                   int net_error,
-                                   bool did_fallback) const;
+                                   int net_error) const;
 
  private:
   enum BypassedBytesType {

diff --git a/google_apis/gcm/engine/connection_factory_impl.cc b/google_apis/gcm/engine/connection_factory_impl.cc
@@ -536,7 +536,7 @@ int ConnectionFactoryImpl::ReconsiderProxyAfterError(int error) {
 
 void ConnectionFactoryImpl::ReportSuccessfulProxyConnection() {
   if (gcm_network_session_ && gcm_network_session_->proxy_service())
-    gcm_network_session_->proxy_service()->ReportSuccess(proxy_info_);
+    gcm_network_session_->proxy_service()->ReportSuccess(proxy_info_, NULL);
 }
 
 void ConnectionFactoryImpl::CloseSocket() {

diff --git a/jingle/glue/proxy_resolving_client_socket.cc b/jingle/glue/proxy_resolving_client_socket.cc
@@ -309,7 +309,7 @@ int ProxyResolvingClientSocket::ReconsiderProxyAfterError(int error) {
 }
 
 void ProxyResolvingClientSocket::ReportSuccessfulProxyConnection() {
-  network_session_->proxy_service()->ReportSuccess(proxy_info_);
+  network_session_->proxy_service()->ReportSuccess(proxy_info_, NULL);
 }
 
 void ProxyResolvingClientSocket::Disconnect() {

diff --git a/net/base/network_delegate.cc b/net/base/network_delegate.cc
@@ -33,10 +33,9 @@ void NetworkDelegate::NotifyResolveProxy(
 
 void NetworkDelegate::NotifyProxyFallback(
     const ProxyServer& bad_proxy,
-    int net_error,
-    bool did_fallback) {
+    int net_error) {
   DCHECK(CalledOnValidThread());
-  OnProxyFallback(bad_proxy, net_error, did_fallback);
+  OnProxyFallback(bad_proxy, net_error);
 }
 
 int NetworkDelegate::NotifyBeforeSendHeaders(
@@ -181,8 +180,7 @@ void NetworkDelegate::OnResolveProxy(
 }
 
 void NetworkDelegate::OnProxyFallback(const ProxyServer& bad_proxy,
-                                      int net_error,
-                                      bool did_fallback) {
+                                      int net_error) {
 }
 
 int NetworkDelegate::OnBeforeSendHeaders(URLRequest* request,

diff --git a/net/base/network_delegate.h b/net/base/network_delegate.h
@@ -68,8 +68,7 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
                           const ProxyService& proxy_service,
                           ProxyInfo* result);
   void NotifyProxyFallback(const ProxyServer& bad_proxy,
-                           int net_error,
-                           bool did_fallback);
+                           int net_error);
   int NotifyBeforeSendHeaders(URLRequest* request,
                               const CompletionCallback& callback,
                               HttpRequestHeaders* headers);
@@ -138,12 +137,12 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
                               const ProxyService& proxy_service,
                               ProxyInfo* result);
 
-  // Called when use of |bad_proxy| fails due to |net_error|. |did_fallback| is
-  // true if the proxy service was able to fallback to another proxy
-  // configuration.
+  // Called when use of |bad_proxy| fails due to |net_error|. |net_error| is
+  // the network error encountered, if any, and OK if the fallback was
+  // for a reason other than a network error (e.g. the proxy service was
+  // explicitly directed to skip a proxy).
   virtual void OnProxyFallback(const ProxyServer& bad_proxy,
-                               int net_error,
-                               bool did_fallback);
+                               int net_error);
 
   // Called right before the HTTP headers are sent. Allows the delegate to
   // read/write |headers| before they get sent out. |callback| and |headers| are

diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
@@ -1137,7 +1137,8 @@ int HttpStreamFactoryImpl::Job::DoCreateStreamComplete(int result) {
   if (result < 0)
     return result;
 
-  session_->proxy_service()->ReportSuccess(proxy_info_);
+  session_->proxy_service()->ReportSuccess(proxy_info_,
+                                           session_->network_delegate());
   next_state_ = STATE_NONE;
   return OK;
 }

diff --git a/net/proxy/proxy_info.cc b/net/proxy/proxy_info.cc
@@ -63,8 +63,8 @@ std::string ProxyInfo::ToPacString() const {
   return proxy_list_.ToPacString();
 }
 
-bool ProxyInfo::Fallback(const BoundNetLog& net_log) {
-  return proxy_list_.Fallback(&proxy_retry_info_, net_log);
+bool ProxyInfo::Fallback(int net_error, const BoundNetLog& net_log) {
+  return proxy_list_.Fallback(&proxy_retry_info_, net_error, net_log);
 }
 
 void ProxyInfo::DeprioritizeBadProxies(

diff --git a/net/proxy/proxy_info.h b/net/proxy/proxy_info.h
@@ -115,9 +115,12 @@ class NET_EXPORT ProxyInfo {
   // See description in ProxyList::ToPacString().
   std::string ToPacString() const;
 
-  // Marks the current proxy as bad. Returns true if there is another proxy
+  // Marks the current proxy as bad. |net_error| should contain the network
+  // error encountered when this proxy was tried, if any. If this fallback
+  // is not because of a network error, then |OK| should be passed in (eg. for
+  // reasons such as local policy). Returns true if there is another proxy is
   // available to try in proxy list_.
-  bool Fallback(const BoundNetLog& net_log);
+  bool Fallback(int net_error, const BoundNetLog& net_log);
 
   // De-prioritizes the proxies that we have cached as not working, by moving
   // them to the end of the proxy list.

diff --git a/net/proxy/proxy_info_unittest.cc b/net/proxy/proxy_info_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "net/base/net_errors.h"
 #include "net/proxy/proxy_info.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -32,7 +33,7 @@ TEST(ProxyInfoTest, ProxyInfoIsDirectOnly) {
   EXPECT_FALSE(info.is_direct());
   EXPECT_FALSE(info.is_direct_only());
   // After falling back to direct, we shouldn't consider it DIRECT only.
-  EXPECT_TRUE(info.Fallback(BoundNetLog()));
+  EXPECT_TRUE(info.Fallback(ERR_PROXY_CONNECTION_FAILED, BoundNetLog()));
   EXPECT_TRUE(info.is_direct());
   EXPECT_FALSE(info.is_direct_only());
 }

diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc
@@ -146,6 +146,7 @@ base::ListValue* ProxyList::ToValue() const {
 }
 
 bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info,
+                         int net_error,
                          const BoundNetLog& net_log) {
 
   // TODO(eroman): It would be good if instead of removing failed proxies
@@ -171,6 +172,7 @@ bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info,
                             TimeDelta::FromMinutes(5),
                             true,
                             ProxyServer(),
+                            net_error,
                             net_log);
 
   // Remove this proxy from our list.
@@ -182,6 +184,7 @@ void ProxyList::AddProxyToRetryList(ProxyRetryInfoMap* proxy_retry_info,
                                     base::TimeDelta retry_delay,
                                     bool try_while_bad,
                                     const ProxyServer& proxy_to_retry,
+                                    int net_error,
                                     const BoundNetLog& net_log) const {
   // Mark this proxy as bad.
   std::string proxy_key = proxy_to_retry.ToURI();
@@ -195,6 +198,7 @@ void ProxyList::AddProxyToRetryList(ProxyRetryInfoMap* proxy_retry_info,
     retry_info.current_delay = retry_delay;
     retry_info.bad_until = TimeTicks().Now() + retry_info.current_delay;
     retry_info.try_while_bad = try_while_bad;
+    retry_info.net_error = net_error;
     (*proxy_retry_info)[proxy_key] = retry_info;
   }
   net_log.AddEvent(NetLog::TYPE_PROXY_LIST_FALLBACK,
@@ -206,6 +210,7 @@ void ProxyList::UpdateRetryInfoOnFallback(
     base::TimeDelta retry_delay,
     bool reconsider,
     const ProxyServer& another_proxy_to_bypass,
+    int net_error,
     const BoundNetLog& net_log) const {
   DCHECK(retry_delay != base::TimeDelta());
 
@@ -215,14 +220,22 @@ void ProxyList::UpdateRetryInfoOnFallback(
   }
 
   if (!proxies_[0].is_direct()) {
-    AddProxyToRetryList(proxy_retry_info, retry_delay, reconsider, proxies_[0],
+    AddProxyToRetryList(proxy_retry_info,
+                        retry_delay,
+                        reconsider,
+                        proxies_[0],
+                        net_error,
                         net_log);
 
     // If an additional proxy to bypass is specified, add it to the retry map
     // as well.
     if (another_proxy_to_bypass.is_valid()) {
-      AddProxyToRetryList(proxy_retry_info, retry_delay, reconsider,
-                          another_proxy_to_bypass, net_log);
+      AddProxyToRetryList(proxy_retry_info,
+                          retry_delay,
+                          reconsider,
+                          another_proxy_to_bypass,
+                          net_error,
+                          net_log);
     }
   }
 }

diff --git a/net/proxy/proxy_list.h b/net/proxy/proxy_list.h
@@ -78,10 +78,14 @@ class NET_EXPORT_PRIVATE ProxyList {
   // Returns a serialized value for the list. The caller takes ownership of it.
   base::ListValue* ToValue() const;
 
-  // Marks the current proxy server as bad and deletes it from the list.  The
-  // list of known bad proxies is given by proxy_retry_info.  Returns true if
-  // there is another server available in the list.
+  // Marks the current proxy server as bad and deletes it from the list. The
+  // list of known bad proxies is given by |proxy_retry_info|. |net_error|
+  // should contain the network error encountered when this proxy was tried, if
+  // any. If this fallback is not because of a network error, then |OK| should
+  // be passed in (eg. for reasons such as local policy). Returns true if there
+  // is another server available in the list.
   bool Fallback(ProxyRetryInfoMap* proxy_retry_info,
+                int net_error,
                 const BoundNetLog& net_log);
 
   // Updates |proxy_retry_info| to indicate that the first proxy in the list
@@ -90,22 +94,28 @@ class NET_EXPORT_PRIVATE ProxyList {
   // retry after |retry_delay| if positive, and will use the default proxy retry
   // duration otherwise. It may reconsider the proxy beforehand if |reconsider|
   // is true. Additionally updates |proxy_retry_info| with
-  // |another_proxy_to_bypass| if non-empty.
+  // |another_proxy_to_bypass| if non-empty. |net_error| should contain the
+  // network error countered when this proxy was tried, or OK if the proxy retry
+  // info is being updated for a non-network related reason (e.g. local policy).
   void UpdateRetryInfoOnFallback(
       ProxyRetryInfoMap* proxy_retry_info,
       base::TimeDelta retry_delay,
       bool reconsider,
       const ProxyServer& another_proxy_to_bypass,
+      int net_error,
       const BoundNetLog& net_log) const;
 
  private:
   // Updates |proxy_retry_info| to indicate that the |proxy_to_retry| in
   // |proxies_| is bad for |retry_delay|, but may be reconsidered earlier if
-  // |try_while_bad| is true.
+  // |try_while_bad| is true. |net_error| should contain the network error
+  // countered when this proxy was tried, or OK if the proxy retry info is
+  // being updated for a non-network related reason (e.g. local policy).
   void AddProxyToRetryList(ProxyRetryInfoMap* proxy_retry_info,
                            base::TimeDelta retry_delay,
                            bool try_while_bad,
                            const ProxyServer& proxy_to_retry,
+                           int net_error,
                            const BoundNetLog& net_log) const;
 
   // List of proxies.

diff --git a/net/proxy/proxy_list_unittest.cc b/net/proxy/proxy_list_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "net/proxy/proxy_list.h"
 
+#include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/proxy/proxy_retry_info.h"
 #include "net/proxy/proxy_server.h"
@@ -172,13 +173,38 @@ TEST(ProxyListTest, UpdateRetryInfoOnFallback) {
     ProxyList list;
     ProxyRetryInfoMap retry_info_map;
     BoundNetLog net_log;
+    ProxyServer proxy_server(
+        ProxyServer::FromURI("foopy1:80", ProxyServer::SCHEME_HTTP));
     list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80");
     list.UpdateRetryInfoOnFallback(&retry_info_map,
                                    base::TimeDelta::FromSeconds(60),
                                    true,
-                                   ProxyServer(),
+                                   proxy_server,
+                                   ERR_PROXY_CONNECTION_FAILED,
+                                   net_log);
+    EXPECT_TRUE(retry_info_map.end() != retry_info_map.find("foopy1:80"));
+    EXPECT_EQ(ERR_PROXY_CONNECTION_FAILED,
+              retry_info_map[proxy_server.ToURI()].net_error);
+    EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy2:80"));
+    EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy3:80"));
+  }
+  // Retrying should put the first proxy on the retry list, even if there
+  // was no network error.
+  {
+    ProxyList list;
+    ProxyRetryInfoMap retry_info_map;
+    BoundNetLog net_log;
+    ProxyServer proxy_server(
+        ProxyServer::FromURI("foopy1:80", ProxyServer::SCHEME_HTTP));
+    list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80");
+    list.UpdateRetryInfoOnFallback(&retry_info_map,
+                                   base::TimeDelta::FromSeconds(60),
+                                   true,
+                                   proxy_server,
+                                   OK,
                                    net_log);
     EXPECT_TRUE(retry_info_map.end() != retry_info_map.find("foopy1:80"));
+    EXPECT_EQ(OK, retry_info_map[proxy_server.ToURI()].net_error);
     EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy2:80"));
     EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy3:80"));
   }
@@ -195,8 +221,11 @@ TEST(ProxyListTest, UpdateRetryInfoOnFallback) {
                                    base::TimeDelta::FromSeconds(60),
                                    true,
                                    proxy_server,
+                                   ERR_NAME_RESOLUTION_FAILED,
                                    net_log);
     EXPECT_TRUE(retry_info_map.end() != retry_info_map.find("foopy1:80"));
+    EXPECT_EQ(ERR_NAME_RESOLUTION_FAILED,
+              retry_info_map[proxy_server.ToURI()].net_error);
     EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy2:80"));
     EXPECT_TRUE(retry_info_map.end() != retry_info_map.find("foopy3:80"));
   }
@@ -213,6 +242,7 @@ TEST(ProxyListTest, UpdateRetryInfoOnFallback) {
                                    base::TimeDelta::FromSeconds(60),
                                    true,
                                    proxy_server,
+                                   OK,
                                    net_log);
     EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy2:80"));
     EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy3:80"));

diff --git a/net/proxy/proxy_retry_info.h b/net/proxy/proxy_retry_info.h
@@ -24,6 +24,11 @@ struct ProxyRetryInfo {
 
   // True if this proxy should be considered even if still bad.
   bool try_while_bad;
+
+  // The network error received when this proxy failed, or |OK| if the proxy
+  // was added to the retry list for a non-network related reason. (e.g. local
+  // policy).
+  int net_error;
 };
 
 // Map of proxy servers with the associated RetryInfo structures.