Recover from bad proxy settings pointing to non-proxy servers that reply anyway.

Added tests for proxy error recovery.

BUG=chromium-os:20775
TEST=DeviceManagementServiceTest.*. Steps described in the bug now work as expected.


Review URL: http://codereview.chromium.org/8054013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@103172 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 115 insertions, 14 deletions

diff --git a/chrome/browser/policy/cloud_policy_controller.h b/chrome/browser/policy/cloud_policy_controller.h
index c446704..b7226e4 100644
--- a/chrome/browser/policy/cloud_policy_controller.h
+++ b/chrome/browser/policy/cloud_policy_controller.h
@@ -37,7 +37,7 @@ class CloudPolicyController
   // Sets the refresh rate at which to re-fetch policy information.
   void SetRefreshRate(int64 refresh_rate_milliseconds);
 
-  // Triggers an immediate retry of of the current operation.
+  // Triggers an immediate retry of the current operation.
   void Retry();
 
   // Stops any pending activity and resets the controller to unenrolled state.
diff --git a/chrome/browser/policy/device_management_service.cc b/chrome/browser/policy/device_management_service.cc
index 9a51ae7..198cae7 100644
--- a/chrome/browser/policy/device_management_service.cc
+++ b/chrome/browser/policy/device_management_service.cc
@@ -17,6 +17,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/ssl_config_service_defaults.h"
 #include "net/http/http_network_layer.h"
+#include "net/http/http_response_headers.h"
 #include "net/proxy/proxy_service.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -27,6 +28,26 @@ namespace policy {
 
 namespace {
 
+bool IsProxyError(const net::URLRequestStatus status) {
+  switch (status.error()) {
+    case net::ERR_PROXY_CONNECTION_FAILED:
+    case net::ERR_TUNNEL_CONNECTION_FAILED:
+    case net::ERR_PROXY_AUTH_UNSUPPORTED:
+    case net::ERR_HTTPS_PROXY_TUNNEL_RESPONSE:
+    case net::ERR_MANDATORY_PROXY_CONFIGURATION_FAILED:
+    case net::ERR_PROXY_CERTIFICATE_INVALID:
+    case net::ERR_SOCKS_CONNECTION_FAILED:
+    case net::ERR_SOCKS_CONNECTION_HOST_UNREACHABLE:
+      return true;
+  }
+  return false;
+}
+
+bool IsProtobufMimeType(const URLFetcher* source) {
+  return source->response_headers()->HasHeaderValue(
+      "content-type", "application/x-protobuffer");
+}
+
 // Custom request context implementation that allows to override the user agent,
 // amongst others. Wraps a baseline request context from which we reuse the
 // networking components.
@@ -214,19 +235,25 @@ void DeviceManagementService::OnURLFetchComplete(
     // Retry the job if it failed due to a broken proxy, by bypassing the
     // proxy on the next try. Don't retry if this URLFetcher already bypassed
     // the proxy.
-    int error = status.error();
-    if (!status.is_success() &&
-        ((source->load_flags() & net::LOAD_BYPASS_PROXY) == 0) &&
-        (error == net::ERR_PROXY_CONNECTION_FAILED ||
-         error == net::ERR_TUNNEL_CONNECTION_FAILED ||
-         error == net::ERR_PROXY_AUTH_UNSUPPORTED ||
-         error == net::ERR_HTTPS_PROXY_TUNNEL_RESPONSE ||
-         error == net::ERR_MANDATORY_PROXY_CONFIGURATION_FAILED ||
-         error == net::ERR_PROXY_CERTIFICATE_INVALID ||
-         error == net::ERR_SOCKS_CONNECTION_FAILED ||
-         error == net::ERR_SOCKS_CONNECTION_HOST_UNREACHABLE)) {
-      LOG(WARNING) << "Proxy failed while contacting dmserver. Retrying "
-                   << "without using the proxy.";
+    bool retry = false;
+    if ((source->load_flags() & net::LOAD_BYPASS_PROXY) == 0) {
+      if (!status.is_success() && IsProxyError(status)) {
+        LOG(WARNING) << "Proxy failed while contacting dmserver.";
+        retry = true;
+      } else if (status.is_success() &&
+                 source->was_fetched_via_proxy() &&
+                 !IsProtobufMimeType(source)) {
+        // The proxy server can be misconfigured but pointing to an existing
+        // server that replies to requests. Try to recover if a successful
+        // request that went through a proxy returns an unexpected mime type.
+        LOG(WARNING) << "Got bad mime-type in response from dmserver that was "
+                     << "fetched via a proxy.";
+        retry = true;
+      }
+    }
+
+    if (retry) {
+      LOG(WARNING) << "Retrying dmserver request without using a proxy.";
       StartJob(job, true);
     } else {
       job->HandleResponse(status, response_code, cookies, data);
diff --git a/chrome/browser/policy/device_management_service_unittest.cc b/chrome/browser/policy/device_management_service_unittest.cc
index 2d73098..a09f46f 100644
--- a/chrome/browser/policy/device_management_service_unittest.cc
+++ b/chrome/browser/policy/device_management_service_unittest.cc
@@ -15,6 +15,9 @@
 #include "content/browser/browser_thread.h"
 #include "content/test/test_url_fetcher_factory.h"
 #include "net/base/escape.h"
+#include "net/base/load_flags.h"
+#include "net/base/net_errors.h"
+#include "net/http/http_response_headers.h"
 #include "net/url_request/url_request_status.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -521,4 +524,75 @@ TEST_F(DeviceManagementServiceTest, CancelDuringCallback) {
   EXPECT_FALSE(backend_.get());
 }
 
+TEST_F(DeviceManagementServiceTest, RetryOnProxyError) {
+  // Make a request.
+  DeviceRegisterResponseDelegateMock mock;
+  EXPECT_CALL(mock, HandleRegisterResponse(_)).Times(0);
+  EXPECT_CALL(mock, OnError(_)).Times(0);
+
+  em::DeviceRegisterRequest request;
+  backend_->ProcessRegisterRequest(kGaiaAuthToken, kOAuthToken,
+                                   kDeviceId, request, &mock);
+  TestURLFetcher* fetcher = factory_.GetFetcherByID(0);
+  ASSERT_TRUE(fetcher);
+  EXPECT_TRUE((fetcher->load_flags() & net::LOAD_BYPASS_PROXY) == 0);
+  const GURL original_url(fetcher->original_url());
+  const std::string upload_data(fetcher->upload_data());
+
+  // Generate a callback with a proxy failure.
+  net::URLRequestStatus status(net::URLRequestStatus::FAILED,
+                               net::ERR_PROXY_CONNECTION_FAILED);
+  fetcher->delegate()->OnURLFetchComplete(fetcher,
+                                          GURL(kServiceUrl),
+                                          status,
+                                          0,
+                                          net::ResponseCookies(),
+                                          "");
+
+  // Verify that a new URLFetcher was started that bypasses the proxy.
+  fetcher = factory_.GetFetcherByID(0);
+  ASSERT_TRUE(fetcher);
+  EXPECT_TRUE(fetcher->load_flags() & net::LOAD_BYPASS_PROXY);
+  EXPECT_EQ(original_url, fetcher->original_url());
+  EXPECT_EQ(upload_data, fetcher->upload_data());
+}
+
+TEST_F(DeviceManagementServiceTest, RetryOnBadResponseFromProxy) {
+  // Make a request.
+  DeviceRegisterResponseDelegateMock mock;
+  EXPECT_CALL(mock, HandleRegisterResponse(_)).Times(0);
+  EXPECT_CALL(mock, OnError(_)).Times(0);
+
+  em::DeviceRegisterRequest request;
+  backend_->ProcessRegisterRequest(kGaiaAuthToken, kOAuthToken,
+                                   kDeviceId, request, &mock);
+  TestURLFetcher* fetcher = factory_.GetFetcherByID(0);
+  ASSERT_TRUE(fetcher);
+  EXPECT_TRUE((fetcher->load_flags() & net::LOAD_BYPASS_PROXY) == 0);
+  const GURL original_url(fetcher->original_url());
+  const std::string upload_data(fetcher->upload_data());
+  fetcher->set_was_fetched_via_proxy(true);
+  scoped_refptr<net::HttpResponseHeaders> headers;
+  headers = new net::HttpResponseHeaders(
+      "HTTP/1.1 200 OK\0Content-type: bad/type\0\0");
+  fetcher->set_response_headers(headers);
+
+  // Generate a callback with a valid http response, that was generated by
+  // a bad/wrong proxy.
+  net::URLRequestStatus status;
+  fetcher->delegate()->OnURLFetchComplete(fetcher,
+                                          GURL(kServiceUrl),
+                                          status,
+                                          200,
+                                          net::ResponseCookies(),
+                                          "");
+
+  // Verify that a new URLFetcher was started that bypasses the proxy.
+  fetcher = factory_.GetFetcherByID(0);
+  ASSERT_TRUE(fetcher);
+  EXPECT_TRUE((fetcher->load_flags() & net::LOAD_BYPASS_PROXY) != 0);
+  EXPECT_EQ(original_url, fetcher->original_url());
+  EXPECT_EQ(upload_data, fetcher->upload_data());
+}
+
 }  // namespace policy