Record UMA when googlezip proxies are removed from the proxy config.

chrome/browser/net/pref_proxy_config_tracker_impl.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/net/pref_proxy_config_tracker_impl.h"
 
 #include "base/bind.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/prefs/proxy_config_dictionary.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "net/proxy/proxy_list.h"
 #include "net/proxy/proxy_server.h"
 
 using content::BrowserThread;
 
 namespace {
 
 // Determine if |proxy| is of the form "*.googlezip.net".
 bool IsGooglezipDataReductionProxy(const net::ProxyServer& proxy) {
   return proxy.is_valid() && !proxy.is_direct() &&
          EndsWith(proxy.host_port_pair().host(), ".googlezip.net", true);
 }
 
 // Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|.
-void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
-  if (proxy_list->IsEmpty())
-    return;
-
+// Returns true if any proxies were removed from |proxy_list|, false otherwise.
+bool RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
   bool found_googlezip_proxy = false;
   for (const net::ProxyServer& proxy : proxy_list->GetAll()) {
     if (IsGooglezipDataReductionProxy(proxy)) {
       found_googlezip_proxy = true;
       break;
     }
   }
   if (!found_googlezip_proxy)
-    return;
+    return false;
 
   net::ProxyList replacement_list;
   for (const net::ProxyServer& proxy : proxy_list->GetAll()) {
     if (!IsGooglezipDataReductionProxy(proxy))
       replacement_list.AddProxyServer(proxy);
   }
 
   if (replacement_list.IsEmpty())
     replacement_list.AddProxyServer(net::ProxyServer::Direct());
   *proxy_list = replacement_list;
+  return true;
+}
+
+void RecordGooglezipProxyRemovalResultHistogram(
+    PrefProxyConfigTrackerImpl::GooglezipProxyRemovalResult result) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Net.PrefProxyConfig.GooglezipProxyRemovalResult", result,
+      PrefProxyConfigTrackerImpl::GOOGLEZIP_PROXY_REMOVAL_RESULT_MAX);
 }
 
 // Remove any Data Reduction Proxies like *.googlezip.net from |proxy_rules|.
 // This is to prevent a Data Reduction Proxy from being activated in an
 // unsupported way, such as from a proxy pref, which could cause Chrome to use
 // the Data Reduction Proxy without adding any of the necessary authentication
 // headers or applying the Data Reduction Proxy bypass logic. See
 // http://crbug.com/476610.
-// TODO(sclittle): Add UMA to record how often this method is called, and how
-// often it actually removes a *.googlezip.net proxy. This method should be
-// removed once it stops actually finding and removing *.googlezip.net proxies
-// from the proxy rules.
+// TODO(sclittle): This method should be removed once the UMA indicates that
+// *.googlezip.net proxies are no longer present in the |proxy_rules|.

[eroman, 2015/05/14 22:20:15]

Is there a separate strategy to actually remove them from the prefs? (Since this
code here just works around bad prefs, but does not actually modify the prefs)

[sclittle, 2015/05/15 02:29:42]

The strategy to remove the bad pref actually already exists:
https://codereview.chromium.org/1104413003

The issue was that the problem was still occuring for clients with that fix. At
the moment, we suspect that this is caused by more than one bug in the code -
the pref migration issue that was addressed in
https://codereview.chromium.org/1104413003 - and something else that we haven't
uncovered yet.

This hack is just to make absolutely sure that the pref isn't the problem, just
in case there's a race with prefs or something like that which is triggering the
DRP to be used when it shouldn't be. We suspect that
https://codereview.chromium.org/1135153002 will treat the symptoms of that
second bug.

Ideally, the UMA added in this CL will indicate that this hack is helping a
negligible number of clients, and then we can remove this hack.

 void RemoveGooglezipDataReductionProxies(
     net::ProxyConfig::ProxyRules* proxy_rules) {
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->fallback_proxies);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_ftp);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_http);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_https);
-  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->single_proxies);
+  // Remove any *.googlezip.net proxies from any of the proxy lists in
+  // |proxy_rules|. Bitwise ORs are used instead of logical ORs in order to
+  // avoid short-circuit evaluation.
+  if (RemoveGooglezipDataReductionProxiesFromList(

[eroman, 2015/05/14 22:20:15]

I don't like this. How about

bool removed = false;
removed |= RemoveGoogleZip...
removed |= RemovedGoogleZip..

Or alternately having RemoveGoogleZip return an integer count of how many were
removed. Then you could do:

size_t num_removed = RemoveGoogleZip...(...) + RemoveGoogleZip(...)

if (num_removed > 0) {
   RecordGoogleZip...
}

[sclittle, 2015/05/15 02:29:42]

Done. Changed to report total number of proxies removed.

+          &proxy_rules->fallback_proxies) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->proxies_for_ftp) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->proxies_for_http) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->proxies_for_https) |
+      RemoveGooglezipDataReductionProxiesFromList(
+          &proxy_rules->single_proxies)) {
+    RecordGooglezipProxyRemovalResultHistogram(
+        PrefProxyConfigTrackerImpl::
+            GOOGLEZIP_PROXY_REMOVAL_RESULT_FOUND_AND_REMOVED);
+  } else {
+    RecordGooglezipProxyRemovalResultHistogram(

[eroman, 2015/05/14 22:20:15]

style: I don't like having two calls to RecordGooglezip...()
Maybe just set the ProxyRemovalResult, and then call RecordHistogram once.  And
in fact at that point may as well just inline it.

[sclittle, 2015/05/15 02:29:42]

Now that it's reporting total number of proxies removed, there's only one
RecordHistogram call.

+        PrefProxyConfigTrackerImpl::GOOGLEZIP_PROXY_REMOVAL_RESULT_NOT_FOUND);
+  }
 }
 
 }  // namespace
 
 //============================= ChromeProxyConfigService =======================
 
 ChromeProxyConfigService::ChromeProxyConfigService(
     net::ProxyConfigService* base_service)
     : base_service_(base_service),
       pref_config_state_(ProxyPrefs::CONFIG_UNSET),
@@ skipping 330 matching lines @@
       (config_state_ != ProxyPrefs::CONFIG_UNSET &&
        !pref_config_.Equals(new_config))) {
     config_state_ = config_state;
     if (config_state_ != ProxyPrefs::CONFIG_UNSET)
       pref_config_ = new_config;
     update_pending_ = true;
   }
   if (update_pending_)
     OnProxyConfigChanged(config_state, new_config);
 }