Make Data Saver proxy bypass logic apply to redirects.

This change makes the DataReductionProxyInterceptor intercept redirects
as well as responses. This will cause clients on captive portal networks
that cause redirect loops when trying to use Data Saver bypass
immediately instead of waiting for 21 redirects.

BUG=460346

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

Cr-Commit-Position: refs/heads/master@{#319120}

components/data_reduction_proxy.gypi

@@ -181,6 +181,8 @@
         'data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.cc',
         'data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h',
       ],
+      # TODO(jschuh): crbug.com/167187 fix size_t to int truncations.
+      'msvs_disabled_warnings': [4267, ],
     },
     {
       'target_name': 'data_reduction_proxy_version_header',

components/data_reduction_proxy/core/browser/BUILD.gn

@@ -64,6 +64,8 @@ source_set("test_support") {
     "data_reduction_proxy_test_utils.h",
   ]
 
+  configs += [ "//build/config/compiler:no_size_t_to_int_warning" ]
+
   public_deps = [
     ":browser",
   ]

components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.cc

@@ -34,9 +34,23 @@ net::URLRequestJob* DataReductionProxyInterceptor::MaybeInterceptRequest(
   return nullptr;
 }
 
+net::URLRequestJob* DataReductionProxyInterceptor::MaybeInterceptRedirect(
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate,
+    const GURL& location) const {
+  return MaybeInterceptResponseOrRedirect(request, network_delegate);
+}
+
 net::URLRequestJob* DataReductionProxyInterceptor::MaybeInterceptResponse(
     net::URLRequest* request,
     net::NetworkDelegate* network_delegate) const {
+  return MaybeInterceptResponseOrRedirect(request, network_delegate);
+}
+
+net::URLRequestJob*
+DataReductionProxyInterceptor::MaybeInterceptResponseOrRedirect(
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate) const {
   if (request->response_info().was_cached)
     return nullptr;
   DataReductionProxyBypassType bypass_type = BYPASS_EVENT_TYPE_MAX;

components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h

@@ -15,7 +15,7 @@ class DataReductionProxyEventStore;
 class DataReductionProxyUsageStats;
 
 // Used to intercept responses that contain explicit and implicit signals
-// to bypass the data reduction proxy. If the proxy should be bypassed,
+// to bypass the Data Reduction Proxy. If the proxy should be bypassed,
 // the interceptor returns a new URLRequestHTTPJob that fetches the resource
 // without use of the proxy.
 class DataReductionProxyInterceptor : public net::URLRequestInterceptor {
@@ -34,15 +34,28 @@ class DataReductionProxyInterceptor : public net::URLRequestInterceptor {
       net::URLRequest* request,
       net::NetworkDelegate* network_delegate) const override;
 
-  // Returns a new URLRequestHTTPJob if the response indicates that the data
-  // reduction proxy should be bypassed according to the rules in |protocol_|.
-  // Returns NULL otherwise. If a job is returned, the interceptor's
-  // URLRequestInterceptingJobFactory will restart the request.
+  // Returns a new URLRequestHTTPJob if the redirect indicates that the Data
+  // Reduction Proxy should be bypassed. See |MaybeInterceptResponseOrRedirect|
+  // for more details.
+  net::URLRequestJob* MaybeInterceptRedirect(
+      net::URLRequest* request, net::NetworkDelegate* network_delegate,
+      const GURL& location) const override;
+
+  // Returns a new URLRequestHTTPJob if the response indicates that the Data
+  // Reduction Proxy should be bypassed. See |MaybeInterceptResponseOrRedirect|
+  // for more details.
   net::URLRequestJob* MaybeInterceptResponse(
       net::URLRequest* request,
       net::NetworkDelegate* network_delegate) const override;
 
  private:
+  // Returns a new URLRequestHTTPJob if the response or redirect indicates that
+  // the data reduction proxy should be bypassed according to the rules in
+  // |bypass_protocol_|. Returns NULL otherwise. If a job is returned, the
+  // interceptor's URLRequestInterceptingJobFactory will restart the request.
+  net::URLRequestJob* MaybeInterceptResponseOrRedirect(
+      net::URLRequest* request, net::NetworkDelegate* network_delegate) const;
+
   // Must outlive |this| if non-NULL.
   DataReductionProxyUsageStats* usage_stats_;
 

components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc

@@ -9,12 +9,17 @@
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_prefs.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h"
+#include "net/socket/socket_test_util.h"
+#include "net/url_request/url_request_context_storage.h"
+#include "net/url_request/url_request_intercepting_job_factory.h"
+#include "net/url_request/url_request_job_factory_impl.h"
 #include "net/url_request/url_request_test_util.h"
 
 namespace data_reduction_proxy {
@@ -56,6 +61,7 @@ DataReductionProxyTestContext::Builder::Builder()
       params_definitions_(0),
       client_(Client::UNKNOWN),
       request_context_(nullptr),
+      mock_socket_factory_(nullptr),
       use_mock_config_(false),
       use_test_configurator_(false),
       use_mock_service_(false),
@@ -82,6 +88,13 @@ DataReductionProxyTestContext::Builder::WithURLRequestContext(
   return *this;
 }
 
+DataReductionProxyTestContext::Builder&
+DataReductionProxyTestContext::Builder::WithMockClientSocketFactory(
+    net::MockClientSocketFactory* mock_socket_factory) {
+  mock_socket_factory_ = mock_socket_factory;
+  return *this;
+}
+
 DataReductionProxyTestContext::Builder&
 DataReductionProxyTestContext::Builder::WithClient(Client client) {
   client_ = client;
@@ -127,7 +140,13 @@ DataReductionProxyTestContext::Builder::Build() {
     request_context_getter = new net::TrivialURLRequestContextGetter(
         request_context_, task_runner);
   } else {
-    request_context_getter = new net::TestURLRequestContextGetter(task_runner);
+    scoped_ptr<net::TestURLRequestContext> test_request_context(
+        new net::TestURLRequestContext(true));
+    if (mock_socket_factory_)
+      test_request_context->set_client_socket_factory(mock_socket_factory_);
+    test_request_context->Init();
+    request_context_getter = new net::TestURLRequestContextGetter(
+        task_runner, test_request_context.Pass());
   }
 
   scoped_ptr<DataReductionProxyEventStore> event_store(
@@ -176,8 +195,8 @@ DataReductionProxyTestContext::Builder::Build() {
   scoped_ptr<DataReductionProxyTestContext> test_context(
       new DataReductionProxyTestContext(
           loop.Pass(), task_runner, pref_service.Pass(), net_log.Pass(),
-          request_context_getter, io_data.Pass(), settings.Pass(),
-          test_context_flags));
+          request_context_getter, mock_socket_factory_, io_data.Pass(),
+          settings.Pass(), test_context_flags));
 
   if (!skip_settings_initialization_)
     test_context->InitSettingsWithoutCheck();
@@ -191,6 +210,7 @@ DataReductionProxyTestContext::DataReductionProxyTestContext(
     scoped_ptr<TestingPrefServiceSimple> simple_pref_service,
     scoped_ptr<net::CapturingNetLog> net_log,
     scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+    net::MockClientSocketFactory* mock_socket_factory,
     scoped_ptr<TestDataReductionProxyIOData> io_data,
     scoped_ptr<DataReductionProxySettings> settings,
     unsigned int test_context_flags)
@@ -200,6 +220,7 @@ DataReductionProxyTestContext::DataReductionProxyTestContext(
       simple_pref_service_(simple_pref_service.Pass()),
       net_log_(net_log.Pass()),
       request_context_getter_(request_context_getter),
+      mock_socket_factory_(mock_socket_factory),
       io_data_(io_data.Pass()),
       settings_(settings.Pass()) {
 }
@@ -249,6 +270,51 @@ DataReductionProxyTestContext::CreateDataReductionProxyServiceInternal() {
   }
 }
 
+void DataReductionProxyTestContext::AttachToURLRequestContext(
+      net::URLRequestContextStorage* request_context_storage) const {
+  DCHECK(request_context_storage);
+
+  // |request_context_storage| takes ownership of the network delegate.
+  request_context_storage->set_network_delegate(
+      io_data()->CreateNetworkDelegate(
+          scoped_ptr<net::NetworkDelegate>(new net::TestNetworkDelegate()),
+          true).release());
+
+  // |request_context_storage| takes ownership of the job factory.
+  request_context_storage->set_job_factory(
+      new net::URLRequestInterceptingJobFactory(
+          scoped_ptr<net::URLRequestJobFactory>(
+              new net::URLRequestJobFactoryImpl()),
+          io_data()->CreateInterceptor().Pass()));
+}
+
+void DataReductionProxyTestContext::
+    EnableDataReductionProxyWithSecureProxyCheckSuccess() {
+  DCHECK(mock_socket_factory_);
+  // This won't actually update the proxy config when using a test configurator.
+  DCHECK(!(test_context_flags_ &
+           DataReductionProxyTestContext::USE_TEST_CONFIGURATOR));
+  // |settings_| needs to have been initialized, since a
+  // |DataReductionProxyService| is needed in order to issue the secure proxy
+  // check.
+  DCHECK(data_reduction_proxy_service());
+
+  // Enable the Data Reduction Proxy, simulating a successful secure proxy
+  // check.
+  net::MockRead mock_reads[] = {
+      net::MockRead("HTTP/1.1 200 OK\r\n\r\n"),
+      net::MockRead("OK"),
+      net::MockRead(net::SYNCHRONOUS, net::OK),
+  };
+  net::StaticSocketDataProvider socket_data_provider(
+      mock_reads, arraysize(mock_reads), nullptr, 0);
+  mock_socket_factory_->AddSocketDataProvider(&socket_data_provider);
+
+  // Set the pref to cause the secure proxy check to be issued.
+  pref_service()->SetBoolean(prefs::kDataReductionProxyEnabled, true);
+  RunUntilIdle();
+}
+
 TestDataReductionProxyConfigurator*
 DataReductionProxyTestContext::test_configurator() const {
   DCHECK(test_context_flags_ &

components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h

@@ -21,9 +21,11 @@ class SingleThreadTaskRunner;
 }
 
 namespace net {
+class MockClientSocketFactory;
 class NetLog;
 class URLRequestContext;
 class URLRequestContextGetter;
+class URLRequestContextStorage;
 }
 
 namespace data_reduction_proxy {
@@ -94,6 +96,15 @@ class DataReductionProxyTestContext {
     // owned by the caller.
     Builder& WithURLRequestContext(net::URLRequestContext* request_context);
 
+    // Specifies a |net::MockClientSocketFactory| to use. The
+    // |mock_socket_factory| is owned by the caller. If a non-NULL
+    // |request_context_| is also specified, then the caller is responsible for
+    // attaching |mock_socket_factory| to |request_context_|. Otherwise,
+    // |mock_socket_factory| will be attached to the dummy
+    // |net::URLRequestContext| generated during Build().
+    Builder& WithMockClientSocketFactory(
+        net::MockClientSocketFactory* mock_socket_factory);
+
     // Specifies the use of |MockDataReductionProxyConfig| instead of
     // |TestDataReductionProxyConfig|.
     Builder& WithMockConfig();
@@ -117,6 +128,7 @@ class DataReductionProxyTestContext {
     unsigned int params_definitions_;
     Client client_;
     net::URLRequestContext* request_context_;
+    net::MockClientSocketFactory* mock_socket_factory_;
 
     bool use_mock_config_;
     bool use_test_configurator_;
@@ -139,6 +151,19 @@ class DataReductionProxyTestContext {
   // SkipSettingsInitialization.
   scoped_ptr<DataReductionProxyService> CreateDataReductionProxyService();
 
+  // This creates a |DataReductionProxyNetworkDelegate| and
+  // |DataReductionProxyInterceptor|, using them in the |net::URLRequestContext|
+  // for |request_context_storage|. |request_context_storage| takes ownership of
+  // the created objects.
+  void AttachToURLRequestContext(
+      net::URLRequestContextStorage* request_context_storage) const;
+
+  // Enable the Data Reduction Proxy, simulating a successful secure proxy
+  // check. This can only be called if not built with WithTestConfigurator,
+  // |settings_| has been initialized, and |this| was built with a
+  // |net::MockClientSocketFactory| specified.
+  void EnableDataReductionProxyWithSecureProxyCheckSuccess();
+
   // Returns the underlying |TestDataReductionProxyConfigurator|. This can only
   // be called if built with WithTestConfigurator.
   TestDataReductionProxyConfigurator* test_configurator() const;
@@ -210,6 +235,7 @@ class DataReductionProxyTestContext {
       scoped_ptr<TestingPrefServiceSimple> simple_pref_service,
       scoped_ptr<net::CapturingNetLog> net_log,
       scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+      net::MockClientSocketFactory* mock_socket_factory,
       scoped_ptr<TestDataReductionProxyIOData> io_data,
       scoped_ptr<DataReductionProxySettings> settings,
       unsigned int test_context_flags);
@@ -227,6 +253,9 @@ class DataReductionProxyTestContext {
   scoped_ptr<TestingPrefServiceSimple> simple_pref_service_;
   scoped_ptr<net::CapturingNetLog> net_log_;
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+  // Non-owned pointer. Will be NULL if |this| was built without specifying a
+  // |net::MockClientSocketFactory|.
+  net::MockClientSocketFactory* mock_socket_factory_;
 
   scoped_ptr<TestDataReductionProxyIOData> io_data_;
   scoped_ptr<DataReductionProxySettings> settings_;

components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc

@@ -33,6 +33,7 @@
 #include "net/socket/socket_test_util.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "net/url_request/url_request_context_storage.h"
 #include "net/url_request/url_request_intercepting_job_factory.h"
 #include "net/url_request/url_request_interceptor.h"
 #include "net/url_request/url_request_job_factory_impl.h"
@@ -554,11 +555,11 @@ TEST_F(DataReductionProxyUsageStatsTest, RequestCompletionErrorCodes) {
 class DataReductionProxyUsageStatsEndToEndTest : public testing::Test {
  public:
   DataReductionProxyUsageStatsEndToEndTest()
-      : context_(true) {}
+      : context_(true), context_storage_(&context_) {}
 
   ~DataReductionProxyUsageStatsEndToEndTest() override {
-    test_context_->io_data()->ShutdownOnUIThread();
-    test_context_->RunUntilIdle();
+    drp_test_context_->io_data()->ShutdownOnUIThread();
+    drp_test_context_->RunUntilIdle();
   }
 
   void SetUp() override {
@@ -566,33 +567,18 @@ class DataReductionProxyUsageStatsEndToEndTest : public testing::Test {
     // test bypassed bytes due to proxy fallbacks. This way, a test just needs
     // to cause one proxy fallback in order for the data reduction proxy to be
     // fully bypassed.
-    test_context_ =
+    drp_test_context_ =
         DataReductionProxyTestContext::Builder()
             .WithParamsFlags(DataReductionProxyParams::kAllowed)
-            .WithParamsDefinitions(TestDataReductionProxyParams::HAS_ORIGIN)
+            .WithParamsDefinitions(
+                 TestDataReductionProxyParams::HAS_EVERYTHING &
+                 ~TestDataReductionProxyParams::HAS_DEV_ORIGIN &
+                 ~TestDataReductionProxyParams::HAS_DEV_FALLBACK_ORIGIN)
             .WithURLRequestContext(&context_)
-            .SkipSettingsInitialization()
+            .WithMockClientSocketFactory(&mock_socket_factory_)
             .Build();
-    test_context_->pref_service()->SetBoolean(prefs::kDataReductionProxyEnabled,
-                                              true);
-    test_context_->InitSettings();
-
-    network_delegate_ = test_context_->io_data()->CreateNetworkDelegate(
-        scoped_ptr<net::NetworkDelegate>(new net::TestNetworkDelegate()), true);
-    context_.set_network_delegate(network_delegate_.get());
-
+    drp_test_context_->AttachToURLRequestContext(&context_storage_);
     context_.set_client_socket_factory(&mock_socket_factory_);
-
-    job_factory_.reset(new net::URLRequestInterceptingJobFactory(
-        scoped_ptr<net::URLRequestJobFactory>(
-            new net::URLRequestJobFactoryImpl()),
-        test_context_->io_data()->CreateInterceptor().Pass()));
-    context_.set_job_factory(job_factory_.get());
-
-    test_context_->configurator()->Enable(false, true,
-                                          config()->Origin().ToURI(),
-                                          std::string(), std::string());
-    test_context_->RunUntilIdle();
   }
 
   // Create and execute a fake request using the data reduction proxy stack.
@@ -640,7 +626,7 @@ class DataReductionProxyUsageStatsEndToEndTest : public testing::Test {
     request->set_method("GET");
     request->SetLoadFlags(net::LOAD_NORMAL);
     request->Start();
-    test_context_->RunUntilIdle();
+    drp_test_context_->RunUntilIdle();
   }
 
   void set_proxy_service(net::ProxyService* proxy_service) {
@@ -652,11 +638,11 @@ class DataReductionProxyUsageStatsEndToEndTest : public testing::Test {
   }
 
   const DataReductionProxySettings* settings() const {
-    return test_context_->settings();
+    return drp_test_context_->settings();
   }
 
   DataReductionProxyConfig* config() const {
-    return test_context_->config();
+    return drp_test_context_->config();
   }
 
   void ClearBadProxies() {
@@ -665,6 +651,7 @@ class DataReductionProxyUsageStatsEndToEndTest : public testing::Test {
 
   void InitializeContext() {
     context_.Init();
+    drp_test_context_->EnableDataReductionProxyWithSecureProxyCheckSuccess();
   }
 
   void ExpectOtherBypassedBytesHistogramsEmpty(
@@ -723,10 +710,9 @@ class DataReductionProxyUsageStatsEndToEndTest : public testing::Test {
  private:
   net::TestDelegate delegate_;
   net::MockClientSocketFactory mock_socket_factory_;
-  scoped_ptr<DataReductionProxyNetworkDelegate> network_delegate_;
-  scoped_ptr<net::URLRequestJobFactory> job_factory_;
   net::TestURLRequestContext context_;
-  scoped_ptr<DataReductionProxyTestContext> test_context_;
+  net::URLRequestContextStorage context_storage_;
+  scoped_ptr<DataReductionProxyTestContext> drp_test_context_;
 };
 
 TEST_F(DataReductionProxyUsageStatsEndToEndTest, BypassedBytesNoRetry) {