Temporary fix to prevent the DRP from being activated improperly.

chrome/browser/net/pref_proxy_config_tracker_impl.cc

Index: chrome/browser/net/pref_proxy_config_tracker_impl.cc
diff --git a/chrome/browser/net/pref_proxy_config_tracker_impl.cc b/chrome/browser/net/pref_proxy_config_tracker_impl.cc
index b7d0d3aa20eed9e84df81d8d4ed24ac25734946e..51ab1a95aedc430bde6a972477400c8bf9e7ac8b 100644
--- a/chrome/browser/net/pref_proxy_config_tracker_impl.cc
+++ b/chrome/browser/net/pref_proxy_config_tracker_impl.cc
@@ -7,6 +7,7 @@
 #include "base/bind.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"
@@ -15,9 +16,48 @@
 #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 {
+
+// Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|.

[eroman, 2015/05/12 00:43:01]

Please add more context on why this is being done, along with a link to the bug.

[sclittle, 2015/05/12 01:13:59]

Done, added to the comment of the RemoveGooglezipDataReductionProxies function
below.

+void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
+  if (proxy_list->IsEmpty())
+    return;
+
+  net::ProxyList replacement_list;

[eroman, 2015/05/12 00:43:01]

Please do this in two phases:
  (1) Scan the list to see if it contains at least 1 proxy ending in
".googlezip.net"
  (2) If it does then build the replacement list and swap.

The reason I would rather do it this way, is so there is less changes to the
normal codeflow (i.e. .googlezip.net is no in there).

[sclittle, 2015/05/12 01:13:59]

Done.

+  for (const net::ProxyServer& proxy : proxy_list->GetAll()) {
+    if (!proxy.is_valid() || proxy.is_direct() ||
+        !EndsWith(proxy.host_port_pair().host(), ".googlezip.net", true)) {
+      replacement_list.AddProxyServer(proxy);
+    }
+  }
+
+  if (replacement_list.IsEmpty())
+    replacement_list.AddProxyServer(net::ProxyServer::Direct());
+  if (replacement_list.size() != proxy_list->size())

[eroman, 2015/05/12 00:43:01]

This doesn't look correct. What if the list were:

  ["http://foo.googlezip.net]

In that case we will rewrite it to:

  ["direct://"]

Because of the line above. In which case sizes will be unchanged (1) however
replacement is in fact needed.

See my comment above which I think will also address this (do it in two phases).

[sclittle, 2015/05/12 01:13:59]

Thanks, I just noticed this as well when testing now.

+    *proxy_list = replacement_list;
+}
+
+// Remove any Data Reduction Proxies like *.googlezip.net from |proxy_rules|.
+// TODO(sclittle): Add UMA to record how often this method is called, and how

[eroman, 2015/05/12 00:43:01]

When to do you plan on doing this? If we are going to be adding this hack to M43
it should also have appropriate measurements.

[sclittle, 2015/05/12 01:13:59]

I'm planning on adding the measurements in M44. I'm trying to minimize the code
that gets merged into M43 here. It seems about as useful for us to measure these
stats in M44, since the full fix for this will likely also be in M44.

+// 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.
+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);
+}
+
+}  // namespace
+
 //============================= ChromeProxyConfigService =======================
 
 ChromeProxyConfigService::ChromeProxyConfigService(
@@ -179,25 +219,37 @@ net::ProxyConfigService::ConfigAvailability
         bool ignore_fallback_config,
         ProxyPrefs::ConfigState* effective_config_state,
         net::ProxyConfig* effective_config) {
+  net::ProxyConfigService::ConfigAvailability rv;
   *effective_config_state = pref_state;
 
   if (PrefPrecedes(pref_state)) {
     *effective_config = pref_config;
-    return net::ProxyConfigService::CONFIG_VALID;
-  }
-
-  // If there's no system proxy config, fall back to prefs or default.
-  if (system_availability == net::ProxyConfigService::CONFIG_UNSET) {
+    rv = net::ProxyConfigService::CONFIG_VALID;
+  } else if (system_availability == net::ProxyConfigService::CONFIG_UNSET) {

[eroman, 2015/05/12 00:43:01]

OK. alternately you could have extracted it to a helper function and kept the
"return" short-circuits.

[sclittle, 2015/05/12 01:13:59]

Acknowledged. I'll leave it this way unless you'd prefer it as a separate helper
function.

+    // If there's no system proxy config, fall back to prefs or default.
     if (pref_state == ProxyPrefs::CONFIG_FALLBACK && !ignore_fallback_config)
       *effective_config = pref_config;
     else
       *effective_config = net::ProxyConfig::CreateDirect();
-    return net::ProxyConfigService::CONFIG_VALID;
+    rv = net::ProxyConfigService::CONFIG_VALID;
+  } else {
+    *effective_config_state = ProxyPrefs::CONFIG_SYSTEM;
+    *effective_config = system_config;
+    rv = system_availability;
   }
 
-  *effective_config_state = ProxyPrefs::CONFIG_SYSTEM;
-  *effective_config = system_config;
-  return system_availability;
+  // Remove any Data Reduction Proxies like *.googlezip.net from the proxy
+  // config rules, since specifying a DRP in the proxy rules is not a supported
+  // means of activating the DRP, and could cause requests to be sent to the DRP
+  // without the appropriate authentication headers and without using any of the
+  // DRP bypass logic. This prevents the Data Reduction Proxy from being
+  // improperly activated via the proxy pref.
+  // TODO(sclittle): This is a temporary hotfix for http://crbug.com/476610, and
+  // should be removed once that bug is fixed and verified.
+  if (rv == net::ProxyConfigService::CONFIG_VALID)
+    RemoveGooglezipDataReductionProxies(&effective_config->proxy_rules());
+
+  return rv;
 }
 
 // static