Temporary fix to prevent the DRP from being activated improperly.

net/proxy/proxy_service.cc

 // Copyright (c) 2012 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 "net/proxy/proxy_service.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 336 matching lines @@
   ~UnsetProxyConfigService() override {}
 
   void AddObserver(Observer* observer) override {}
   void RemoveObserver(Observer* observer) override {}
   ConfigAvailability GetLatestProxyConfig(ProxyConfig* config) override {
     return CONFIG_UNSET;
   }
 };
 #endif
 
+// Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|.

[bengr, 2015/05/10 16:49:34]

Consider limiting all of this logic to android and ios, with #if
defined(OS_ANDROID) and #if defined(OS_IOS). We don't see this problem on
desktop, and desktop use of proxies is much more extensive than mobile.

[Not at Google. Contact bengr, 2015/05/11 17:24:09]

Can we verify that this does not affect desktop? If it does, we can call this 
behind a check for pref::kDataReductionProxyWasEnabledBefore.

[sclittle, 2015/05/11 20:43:36]

But either way, this code would only remove data reduction proxies that were
added by something other than OnProxyResolved in DRPNetworkDelegate, so using
these proxies could thus cause users to see 502 responses from that proxy.

We should prevent a data reduction proxy from being used in an unsupported way,
regardless of platform. We actually do see this issue on desktop, but much less
often than on mobile.

+void RemoveGooglezipDataReductionProxies(ProxyList* proxy_list) {

[bengr, 2015/05/10 16:49:34]

Add a TODO to add UMA to track how often this is called and how often it
actually removes a googlezip.net proxy, and to remove this code when the latter
is zero.

[sclittle, 2015/05/11 20:43:36]

Done.

+  if (proxy_list->IsEmpty())

[bengr, 2015/05/10 16:49:34]

#include "net/proxy/proxy_list.h"

[sclittle, 2015/05/11 20:43:36]

Done.

+    return;
+
+  ProxyList replacement_list;
+  for (const 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);

[bengr, 2015/05/10 16:49:33]

add an else clause that sets found_googlezip_proxy to true

[sclittle, 2015/05/11 20:43:36]

Instead of using a bool variable, I just check if the size of the replacement
list is different.

+    }
+  }
+
+  if (replacement_list.IsEmpty())
+    replacement_list.AddProxyServer(ProxyServer::Direct());

[bengr, 2015/05/10 16:49:34]

Is this the contents of a list when there's no configured proxy?

[Not at Google. Contact bengr, 2015/05/11 17:24:09]

Instead of setting ProxyServer::Direct(), you should clear prefs::kProxy pref
when there are no proxies. 

If this pref is not set, Chrome will use ProxyPrefs::MODE_SYSTEM  as the
default.

[sclittle, 2015/05/11 20:43:36]

Re bengr: It's possible that the DRP was configured as a mandatory proxy, if all
the proxies in the list were DRPs. This check is just to prevent the user from
seeing ERR_NO_SUPPORTED_PROXIES errors all the time.

Re kundaji: I'm trying to avoid assuming anything about the pref at this point,
otherwise I'd expect this other method to solve the problem:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

This method doesn't just apply to the proxy pref, it applies to any proxy
configuration set in the proxy service, e.g. proxy pref, PAC fetch,
"--proxy-server" command line flag, etc.

Also, this is called after that pref is read, so adding in Direct() would still
have to happen here.

+  *proxy_list = replacement_list;

[bengr, 2015/05/10 16:49:34]

replace only if found_googlezip_proxy is true.

[sclittle, 2015/05/11 20:43:36]

Done, but instead of using a found_googlezip_proxy variable I just check if the
replacement list is a different size.

+}
+
+// Remove any Data Reduction Proxies like *.googlezip.net from |proxy_rules|.
+void RemoveGooglezipDataReductionProxiesFromRules(
+    ProxyConfig::ProxyRules* proxy_rules) {
+  RemoveGooglezipDataReductionProxies(&proxy_rules->fallback_proxies);

[bengr, 2015/05/10 16:49:34]

#include "net/proxy/proxy_config.h"

[sclittle, 2015/05/11 20:43:36]

Done.

+  RemoveGooglezipDataReductionProxies(&proxy_rules->proxies_for_ftp);
+  RemoveGooglezipDataReductionProxies(&proxy_rules->proxies_for_http);

[bengr, 2015/05/10 16:49:34]

We only ever set HTTP. I can understand checking fallback and single proxies,
but why check ftp and https?

[sclittle, 2015/05/11 20:43:36]

For thoroughness; there's no harm in checking the others. The DRP is definitely
not supported for FTP, and the alternative DRP configuration has an HTTPS tunnel
proxy.

+  RemoveGooglezipDataReductionProxies(&proxy_rules->proxies_for_https);
+  RemoveGooglezipDataReductionProxies(&proxy_rules->single_proxies);
+}
+
 }  // namespace
 
 // ProxyService::InitProxyResolver --------------------------------------------
 
 // This glues together two asynchronous steps:
 //   (1) ProxyScriptDecider -- try to fetch/validate a sequence of PAC scripts
 //       to figure out what we should configure against.
 //   (2) Feed the fetched PAC script into the ProxyResolver.
 //
 // InitProxyResolver is a single-use class which encapsulates cancellation as
@@ skipping 786 matching lines @@
     PacRequest* req = it->get();
     if (req->is_started()) {
       req->CancelResolveJob();
 
       req->net_log()->BeginEvent(
           NetLog::TYPE_PROXY_SERVICE_WAITING_FOR_INIT_PAC);
     }
   }
 }
 
+namespace {}  // namespace
+
 void ProxyService::SetReady() {
   DCHECK(!init_proxy_resolver_.get());
   current_state_ = STATE_READY;
 
+  // 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.
+  // TODO(sclittle): This is a temporary hotfix for http://crbug.com/476610, and
+  // should be removed once that bug is fixed and verified.
+  RemoveGooglezipDataReductionProxiesFromRules(&config_.proxy_rules());

[bengr, 2015/05/10 16:49:34]

Please explain when you expect this to be called. For example, if I change the
system proxy, will this be called again? If I toggle the DRP setting off/on,
will this be called?

[sclittle, 2015/05/11 20:43:36]

Added comment. This will be called whenever SetReady is called, which is when
the ProxyService decides on a ProxyConfig. It has nothing to do with toggling
the DRP on or off.

+
   // Make a copy in case |this| is deleted during the synchronous completion
   // of one of the requests. If |this| is deleted then all of the PacRequest
   // instances will be Cancel()-ed.
   PendingRequests pending_copy = pending_requests_;
 
   for (PendingRequests::iterator it = pending_copy.begin();
        it != pending_copy.end();
        ++it) {
     PacRequest* req = it->get();
     if (!req->is_started() && !req->was_cancelled()) {
@@ skipping 454 matching lines @@
   State previous_state = ResetProxyConfig(false);
   if (previous_state != STATE_NONE)
     ApplyProxyConfigIfAvailable();
 }
 
 void ProxyService::OnDNSChanged() {
   OnIPAddressChanged();
 }
 
 }  // namespace net