Domain Reliability: Don't send proxy address, other fixes

Make sure the monitor does not collect the proxy's address if one was used.
(In the process, refactor RequestInfo to contain the whole HttpResponseInfo,
tighten up OnRequestLegComplete, and poke at unittests a bit.)

Also, count network errors as well as HTTP errors as failures (!).

BUG=356791

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

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

components/domain_reliability/config_unittest.cc

@@ -80,8 +80,6 @@ int GetIndex(DomainReliabilityConfig* config, const char* url_string) {
   return config->GetResourceIndexForUrl(GURL(url_string));
 }
 
-}  // namespace
-
 class DomainReliabilityConfigTest : public testing::Test { };
 
 TEST_F(DomainReliabilityConfigTest, IsValid) {
@@ -209,4 +207,5 @@ TEST_F(DomainReliabilityConfigTest, FromJSON) {
       config->collectors[0]->upload_url);
 }
 
+}  // namespace
 }  // namespace domain_reliability

components/domain_reliability/context.cc

@@ -141,9 +141,9 @@ void DomainReliabilityContext::OnBeacon(const GURL& url,
     return;
   DCHECK_GT(states_.size(), index);
 
+  bool success = (beacon.status == "ok");
+
   ResourceState* state = states_[index];
-  bool success = beacon.http_response_code >= 200 &&
-                 beacon.http_response_code < 400;
   if (success)
     ++state->successful_requests;
   else

components/domain_reliability/context_unittest.cc

@@ -33,8 +33,6 @@ DomainReliabilityBeacon MakeBeacon(MockableTime* time) {
   return beacon;
 }
 
-}  // namespace
-
 class DomainReliabilityContextTest : public testing::Test {
  protected:
   DomainReliabilityContextTest()
@@ -211,4 +209,5 @@ TEST_F(DomainReliabilityContextTest, ReportUpload) {
   CallUploadCallback(true);
 }
 
+}  // namespace
 }  // namespace domain_reliability

components/domain_reliability/dispatcher_unittest.cc

@@ -8,11 +8,12 @@
 #include "components/domain_reliability/test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace domain_reliability {
+namespace {
+
 using base::TimeDelta;
 using base::TimeTicks;
 
-namespace domain_reliability {
-
 class DomainReliabilityDispatcherTest : public testing::Test {
  public:
   DomainReliabilityDispatcherTest() : dispatcher_(&time_) {}
@@ -58,4 +59,5 @@ TEST_F(DomainReliabilityDispatcherTest, TaskRunsAtDeadline) {
   EXPECT_TRUE(callback.called());
 }
 
+}  // namespace
 }  // namespace domain_reliability

components/domain_reliability/monitor.cc

@@ -12,6 +12,7 @@
 #include "components/domain_reliability/baked_in_configs.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -122,7 +123,7 @@ void DomainReliabilityMonitor::OnCompleted(net::URLRequest* request,
   if (!started)
     return;
   RequestInfo request_info(*request);
-  if (request_info.DefinitelyReachedNetwork()) {
+  if (request_info.AccessedNetwork()) {
     OnRequestLegComplete(request_info);
     // A request was just using the network, so now is a good time to run any
     // pending and eligible uploads.
@@ -141,21 +142,17 @@ DomainReliabilityMonitor::RequestInfo::RequestInfo(
     const net::URLRequest& request)
     : url(request.url()),
       status(request.status()),
-      response_code(-1),
-      socket_address(request.GetSocketAddress()),
-      was_cached(request.was_cached()),
+      response_info(request.response_info()),
       load_flags(request.load_flags()),
       is_upload(DomainReliabilityUploader::URLRequestIsUpload(request)) {
   request.GetLoadTimingInfo(&load_timing_info);
-  // Can't get response code of a canceled request -- there's no transaction.
-  if (status.status() != net::URLRequestStatus::CANCELED)
-    response_code = request.GetResponseCode();
 }
 
 DomainReliabilityMonitor::RequestInfo::~RequestInfo() {}
 
-bool DomainReliabilityMonitor::RequestInfo::DefinitelyReachedNetwork() const {
-  return status.status() != net::URLRequestStatus::CANCELED && !was_cached;
+bool DomainReliabilityMonitor::RequestInfo::AccessedNetwork() const {
+  return status.status() != net::URLRequestStatus::CANCELED &&
+     response_info.network_accessed;
 }
 
 DomainReliabilityContext* DomainReliabilityMonitor::AddContext(
@@ -184,41 +181,42 @@ DomainReliabilityContext* DomainReliabilityMonitor::AddContext(
 
 void DomainReliabilityMonitor::OnRequestLegComplete(
     const RequestInfo& request) {
-  if (!request.DefinitelyReachedNetwork())
-    return;
-
-  // Don't monitor requests that are not sending cookies, since sending a beacon
-  // for such requests may allow the server to correlate that request with the
-  // user (by correlating a particular config).
-  if (request.load_flags & net::LOAD_DO_NOT_SEND_COOKIES)
-    return;
-
-  // Don't monitor requests that were, themselves, Domain Reliability uploads,
-  // to avoid infinite chains of uploads.
-  if (request.is_upload)
-    return;
-
-  ContextMap::iterator it = contexts_.find(request.url.host());
-  if (it == contexts_.end())
-    return;
-  DomainReliabilityContext* context = it->second;
-
+  int response_code;
+  if (request.response_info.headers)
+    response_code = request.response_info.headers->response_code();
+  else
+    response_code = -1;
+  ContextMap::iterator context_it;
   std::string beacon_status;
-  bool got_status = GetDomainReliabilityBeaconStatus(
-      request.status.error(),
-      request.response_code,
-      &beacon_status);
-  if (!got_status)
+
+  // Ignore requests where:
+  // 1. There is no context for the request host.
+  // 2. The request did not access the network.
+  // 3. The request is not supposed to send cookies (to avoid associating the
+  //    request with any potentially unique data in the config).
+  // 4. The request was itself a Domain Reliability upload (to avoid loops).
+  // 5. There is no defined beacon status for the error or HTTP response code
+  //    (to avoid leaking network-local errors).
+  if ((context_it = contexts_.find(request.url.host())) == contexts_.end() ||
+      !request.AccessedNetwork() ||
+      (request.load_flags & net::LOAD_DO_NOT_SEND_COOKIES) ||
+      request.is_upload ||
+      !GetDomainReliabilityBeaconStatus(
+          request.status.error(), response_code, &beacon_status)) {
     return;
+  }
 
   DomainReliabilityBeacon beacon;
   beacon.status = beacon_status;
   beacon.chrome_error = request.status.error();
-  beacon.server_ip = request.socket_address.host();
-  beacon.http_response_code = request.response_code;
+  if (!request.response_info.was_fetched_via_proxy)
+    beacon.server_ip = request.response_info.socket_address.host();
+  else
+    beacon.server_ip.clear();
+  beacon.http_response_code = response_code;
   beacon.start_time = request.load_timing_info.request_start;
   beacon.elapsed = time_->NowTicks() - beacon.start_time;
-  context->OnBeacon(request.url, beacon);
+  context_it->second->OnBeacon(request.url, beacon);
 }
 
 }  // namespace domain_reliability

components/domain_reliability/monitor.h

@@ -17,8 +17,8 @@
 #include "components/domain_reliability/scheduler.h"
 #include "components/domain_reliability/uploader.h"
 #include "components/domain_reliability/util.h"
-#include "net/base/host_port_pair.h"
 #include "net/base/load_timing_info.h"
+#include "net/http/http_response_info.h"
 #include "net/url_request/url_request_status.h"
 
 namespace net {
@@ -69,15 +69,13 @@ class DOMAIN_RELIABILITY_EXPORT DomainReliabilityMonitor {
     explicit RequestInfo(const net::URLRequest& request);
     ~RequestInfo();
 
-    bool DefinitelyReachedNetwork() const;
+    bool AccessedNetwork() const;
 
     GURL url;
     net::URLRequestStatus status;
-    int response_code;
-    net::HostPortPair socket_address;
-    net::LoadTimingInfo load_timing_info;
-    bool was_cached;
+    net::HttpResponseInfo response_info;
     int load_flags;
+    net::LoadTimingInfo load_timing_info;
     bool is_upload;
   };
 

components/domain_reliability/monitor_unittest.cc

@@ -16,7 +16,10 @@
 #include "components/domain_reliability/config.h"
 #include "components/domain_reliability/test_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 #include "net/url_request/url_request_test_util.h"
@@ -24,23 +27,21 @@
 
 namespace domain_reliability {
 
+namespace {
+
 typedef std::vector<DomainReliabilityBeacon> BeaconVector;
 
-class DomainReliabilityMonitorTest : public testing::Test {
- protected:
-  typedef DomainReliabilityMonitor::RequestInfo RequestInfo;
+static const size_t kAlwaysReportIndex = 0u;
+static const size_t kNeverReportIndex = 1u;
 
-  DomainReliabilityMonitorTest()
-      : bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-        url_request_context_getter_(new net::TestURLRequestContextGetter(
-            base::MessageLoopProxy::current())),
-        time_(new MockTime()),
-        monitor_(url_request_context_getter_->GetURLRequestContext(),
-                 "test-reporter",
-                 scoped_ptr<MockableTime>(time_)),
-        context_(monitor_.AddContextForTesting(CreateConfig())) {}
+scoped_refptr<net::HttpResponseHeaders> MakeHttpResponseHeaders(
+    const std::string& headers) {
+  return scoped_refptr<net::HttpResponseHeaders>(
+      new net::HttpResponseHeaders(net::HttpUtil::AssembleRawHeaders(
+          headers.c_str(), headers.length())));
+}
 
-  static scoped_ptr<const DomainReliabilityConfig> CreateConfig() {
+static scoped_ptr<const DomainReliabilityConfig> MakeConfig() {
   DomainReliabilityConfig* config = new DomainReliabilityConfig();
 
   DomainReliabilityConfig::Resource* resource;
@@ -75,27 +76,58 @@ class DomainReliabilityMonitorTest : public testing::Test {
   EXPECT_TRUE(config->IsValid());
 
   return scoped_ptr<const DomainReliabilityConfig>(config);
-  }
+}
+
+}  // namespace
+
+class DomainReliabilityMonitorTest : public testing::Test {
+ protected:
+  typedef DomainReliabilityMonitor::RequestInfo RequestInfo;
 
-  RequestInfo MakeRequestInfo() {
+  DomainReliabilityMonitorTest()
+      : bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+        url_request_context_getter_(new net::TestURLRequestContextGetter(
+            base::MessageLoopProxy::current())),
+        time_(new MockTime()),
+        monitor_(url_request_context_getter_->GetURLRequestContext(),
+                 "test-reporter",
+                 scoped_ptr<MockableTime>(time_)),
+        context_(monitor_.AddContextForTesting(MakeConfig())) {}
+
+  static RequestInfo MakeRequestInfo() {
     RequestInfo request;
     request.status = net::URLRequestStatus();
-    request.response_code = 200;
-    request.was_cached = false;
+    request.status.set_status(net::URLRequestStatus::SUCCESS);
+    request.status.set_error(net::OK);
+    request.response_info.socket_address =
+        net::HostPortPair::FromString("12.34.56.78:80");
+    request.response_info.headers = MakeHttpResponseHeaders(
+        "HTTP/1.1 200 OK\n\n");
+    request.response_info.network_accessed = true;
+    request.response_info.was_fetched_via_proxy = false;
     request.load_flags = 0;
     request.is_upload = false;
     return request;
   }
 
-  bool CheckNoBeacons(size_t index) {
+  void OnRequestLegComplete(const RequestInfo& info) {
+    monitor_.OnRequestLegComplete(info);
+  }
+
+  size_t CountPendingBeacons(size_t index) {
     BeaconVector beacons;
-    unsigned successful, failed;
-    context_->GetQueuedDataForTesting(index, &beacons, &successful, &failed);
-    return beacons.empty() && successful == 0 && failed == 0;
+    context_->GetQueuedDataForTesting(index, &beacons, NULL, NULL);
+    return beacons.size();
   }
 
-  void OnRequestLegComplete(const RequestInfo& info) {
-    monitor_.OnRequestLegComplete(info);
+  bool CheckRequestCounts(size_t index,
+                          uint32 expected_successful,
+                          uint32 expected_failed) {
+    uint32 successful, failed;
+    context_->GetQueuedDataForTesting(index, NULL, &successful, &failed);
+    EXPECT_EQ(expected_successful, successful);
+    EXPECT_EQ(expected_failed, failed);
+    return expected_successful == successful && expected_failed == failed;
   }
 
   content::TestBrowserThreadBundle bundle_;
@@ -106,49 +138,138 @@ class DomainReliabilityMonitorTest : public testing::Test {
   DomainReliabilityMonitor::RequestInfo request_;
 };
 
+namespace {
+
 TEST_F(DomainReliabilityMonitorTest, Create) {
-  EXPECT_TRUE(CheckNoBeacons(0));
-  EXPECT_TRUE(CheckNoBeacons(1));
+  EXPECT_EQ(0u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountPendingBeacons(kNeverReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 0u));
 }
 
-TEST_F(DomainReliabilityMonitorTest, NoContextRequest) {
+TEST_F(DomainReliabilityMonitorTest, NoContext) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://no-context/");
   OnRequestLegComplete(request);
 
-  EXPECT_TRUE(CheckNoBeacons(0));
-  EXPECT_TRUE(CheckNoBeacons(1));
+  EXPECT_EQ(0u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+  EXPECT_EQ(0u, CountPendingBeacons(kNeverReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 0u));
+}
+
+TEST_F(DomainReliabilityMonitorTest, NotReported) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/never_report");
+  OnRequestLegComplete(request);
+
+  EXPECT_EQ(0u, CountPendingBeacons(kNeverReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 1u, 0u));
 }
 
-TEST_F(DomainReliabilityMonitorTest, ContextRequest) {
+TEST_F(DomainReliabilityMonitorTest, NetworkFailure) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://example/always_report");
+  request.status.set_status(net::URLRequestStatus::FAILED);
+  request.status.set_error(net::ERR_CONNECTION_RESET);
+  request.response_info.headers = NULL;
   OnRequestLegComplete(request);
 
-  BeaconVector beacons;
-  context_->GetQueuedDataForTesting(0, &beacons, NULL, NULL);
-  EXPECT_EQ(1u, beacons.size());
-  EXPECT_TRUE(CheckNoBeacons(1));
+  EXPECT_EQ(1u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 1u));
+}
+
+TEST_F(DomainReliabilityMonitorTest, ServerFailure) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.response_info.headers =
+      MakeHttpResponseHeaders("HTTP/1.1 500 :(\n\n");
+  OnRequestLegComplete(request);
+
+  EXPECT_EQ(1u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 1u));
+}
+
+TEST_F(DomainReliabilityMonitorTest, NotReportedFailure) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/never_report");
+  request.status.set_status(net::URLRequestStatus::FAILED);
+  request.status.set_error(net::ERR_CONNECTION_RESET);
+  OnRequestLegComplete(request);
+
+  EXPECT_EQ(0u, CountPendingBeacons(kNeverReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kNeverReportIndex, 0u, 1u));
+}
+
+TEST_F(DomainReliabilityMonitorTest, Request) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  OnRequestLegComplete(request);
+
+  EXPECT_EQ(1u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 1u, 0u));
+}
+
+// Make sure the monitor does not log requests that did not access the network.
+TEST_F(DomainReliabilityMonitorTest, DidNotAccessNetwork) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.response_info.network_accessed = false;
+  OnRequestLegComplete(request);
+
+  EXPECT_EQ(0u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
 }
 
-TEST_F(DomainReliabilityMonitorTest, ContextRequestWithDoNotSendCookies) {
+// Make sure the monitor does not log requests that don't send cookies.
+TEST_F(DomainReliabilityMonitorTest, DoNotSendCookies) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://example/always_report");
   request.load_flags = net::LOAD_DO_NOT_SEND_COOKIES;
   OnRequestLegComplete(request);
 
-  EXPECT_TRUE(CheckNoBeacons(0));
-  EXPECT_TRUE(CheckNoBeacons(1));
+  EXPECT_EQ(0u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
 }
 
-TEST_F(DomainReliabilityMonitorTest, ContextRequestThatIsUpload) {
+// Make sure the monitor does not log upload requests.
+TEST_F(DomainReliabilityMonitorTest, IsUpload) {
   RequestInfo request = MakeRequestInfo();
   request.url = GURL("http://example/always_report");
   request.is_upload = true;
   OnRequestLegComplete(request);
 
-  EXPECT_TRUE(CheckNoBeacons(0));
-  EXPECT_TRUE(CheckNoBeacons(1));
+  EXPECT_EQ(0u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+}
+
+// Make sure the monitor does not log a network-local error.
+TEST_F(DomainReliabilityMonitorTest, LocalError) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.status.set_status(net::URLRequestStatus::FAILED);
+  request.status.set_error(net::ERR_PROXY_CONNECTION_FAILED);
+  OnRequestLegComplete(request);
+
+  EXPECT_EQ(0u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 0u, 0u));
+}
+
+// Make sure the monitor does not log the proxy's IP if one was used.
+TEST_F(DomainReliabilityMonitorTest, WasFetchedViaProxy) {
+  RequestInfo request = MakeRequestInfo();
+  request.url = GURL("http://example/always_report");
+  request.response_info.socket_address =
+      net::HostPortPair::FromString("127.0.0.1:3128");
+  request.response_info.was_fetched_via_proxy = true;
+  OnRequestLegComplete(request);
+
+  EXPECT_EQ(1u, CountPendingBeacons(kAlwaysReportIndex));
+  EXPECT_TRUE(CheckRequestCounts(kAlwaysReportIndex, 1u, 0u));
+
+  BeaconVector beacons;
+  context_->GetQueuedDataForTesting(kAlwaysReportIndex, &beacons, NULL, NULL);
+  EXPECT_TRUE(beacons[0].server_ip.empty());
 }
 
 TEST_F(DomainReliabilityMonitorTest, AddBakedInConfigs) {
@@ -167,4 +288,6 @@ TEST_F(DomainReliabilityMonitorTest, AddBakedInConfigs) {
   EXPECT_EQ(num_baked_in_configs + 1, monitor_.contexts_size_for_testing());
 }
 
+}  // namespace
+
 }  // namespace domain_reliability

components/domain_reliability/scheduler_unittest.cc

@@ -11,11 +11,12 @@
 #include "components/domain_reliability/util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace domain_reliability {
+namespace {
+
 using base::TimeDelta;
 using base::TimeTicks;
 
-namespace domain_reliability {
-
 class DomainReliabilitySchedulerTest : public testing::Test {
  public:
   DomainReliabilitySchedulerTest()
@@ -247,4 +248,5 @@ TEST_F(DomainReliabilitySchedulerTest, BeaconWhileUploading) {
   ASSERT_TRUE(CheckNoPendingUpload());
 }
 
+}  // namespace
 }  // namespace domain_reliability

components/domain_reliability/uploader_unittest.cc

@@ -18,6 +18,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace domain_reliability {
+namespace {
 
 class DomainReliabilityUploaderTest : public testing::Test {
  protected:
@@ -105,4 +106,5 @@ TEST_F(DomainReliabilityUploaderTest, FailedUpload) {
   EXPECT_FALSE(upload_successful_[0]);
 }
 
+}  // namespace
 }  // namespace domain_reliability

components/domain_reliability/util_unittest.cc

@@ -8,11 +8,12 @@
 #include "components/domain_reliability/test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace domain_reliability {
+namespace {
+
 using base::TimeDelta;
 using base::TimeTicks;
 
-namespace domain_reliability {
-
 class DomainReliabilityMockTimeTest : public testing::Test {
  protected:
   MockTime time_;
@@ -120,4 +121,5 @@ TEST_F(DomainReliabilityMockTimeTest, TimerReentrantStart) {
   EXPECT_FALSE(timer->IsRunning());
 }
 
+}  // namespace
 }  // namespace domain_reliability