NetConnectivity - Collect stats for TCP/UDP network

chrome/browser/net/network_stats.cc

Index: chrome/browser/net/network_stats.cc
===================================================================
--- chrome/browser/net/network_stats.cc	(revision 135283)
+++ chrome/browser/net/network_stats.cc	(working copy)
@@ -16,11 +16,13 @@
 #include "base/tuple.h"
 #include "chrome/common/chrome_version_info.h"
 #include "content/public/browser/browser_thread.h"
+#include "googleurl/src/gurl.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/sys_addrinfo.h"
 #include "net/base/test_completion_callback.h"
+#include "net/proxy/proxy_service.h"
 #include "net/socket/tcp_client_socket.h"
 #include "net/udp/udp_client_socket.h"
 #include "net/udp/udp_server_socket.h"
@@ -94,6 +96,7 @@
     : load_size_(0),
       bytes_to_read_(0),
       bytes_to_send_(0),
+      has_proxy_server_(false),
       packets_to_send_(0),
       packets_sent_(0),
       base_packet_number_(0),
@@ -108,13 +111,18 @@
 bool NetworkStats::Start(net::HostResolver* host_resolver,
                          const net::HostPortPair& server_host_port_pair,
                          HistogramPortSelector histogram_port,
+                         bool has_proxy_server,
                          uint32 bytes_to_send,
                          uint32 packets_to_send,
                          const net::CompletionCallback& finished_callback) {
   DCHECK(bytes_to_send);   // We should have data to send.
   DCHECK_LE(packets_to_send, kMaximumPackets);
 
-  Initialize(bytes_to_send, histogram_port, packets_to_send, finished_callback);
+  Initialize(bytes_to_send,
+             histogram_port,
+             has_proxy_server,
+             packets_to_send,
+             finished_callback);
 
   net::HostResolver::RequestInfo request(server_host_port_pair);
   int rv = host_resolver->Resolve(
@@ -130,6 +138,7 @@
 void NetworkStats::Initialize(
     uint32 bytes_to_send,
     HistogramPortSelector histogram_port,
+    bool has_proxy_server,
     uint32 packets_to_send,
     const net::CompletionCallback& finished_callback) {
   DCHECK(bytes_to_send);    // We should have data to send.
@@ -140,6 +149,7 @@
   packets_to_send_ = packets_to_send;
 
   histogram_port_ = histogram_port;
+  has_proxy_server_ = has_proxy_server;
   finished_callback_ = finished_callback;
 }
 
@@ -538,7 +548,7 @@
 
   // Build "NetConnectivity.<protocol>.Status.<port>.<load_size>" histogram
   // name. Total number of histograms are 2*5*2.
-  *status_histogram_name =  base::StringPrintf(
+  *status_histogram_name = base::StringPrintf(
       "NetConnectivity.%s.Status.%d.%s",
       protocol_string,
       kPorts[port],
@@ -547,7 +557,7 @@
   // Build "NetConnectivity.<protocol>.PacketLoss.<port>.<load_size>" histogram
   // name. Total number of histograms are 5 (because we do this test for UDP
   // only).
-  *packet_loss_histogram_name =  base::StringPrintf(
+  *packet_loss_histogram_name = base::StringPrintf(
       "NetConnectivity.%s.PacketLoss.%d.%s",
       protocol_string,
       kPorts[port],
@@ -570,31 +580,37 @@
                     &status_histogram_name,
                     &packet_loss_histogram_name);
 
-  // For packet loss test, just record packet loss data.
-  if (packets_to_send_ > 1) {
-    base::Histogram* packet_loss_histogram = base::LinearHistogram::FactoryGet(
-        packet_loss_histogram_name,
-        1,
-        2 << kMaximumPackets,
-        (2 << kMaximumPackets) + 1,
-        base::Histogram::kUmaTargetedHistogramFlag);
-    packet_loss_histogram->Add(packets_received_mask_);
-    return;
-  }
+  size_t histogram_count = has_proxy_server_ ? 1 : 2;
+  for (size_t i = 0; i < histogram_count; i++) {
+    // For packet loss test, just record packet loss data.
+    if (packets_to_send_ > 1) {
+      base::Histogram* histogram = base::LinearHistogram::FactoryGet(
+          packet_loss_histogram_name,
+          1,
+          2 << kMaximumPackets,
+          (2 << kMaximumPackets) + 1,
+          base::Histogram::kUmaTargetedHistogramFlag);
+      histogram->Add(packets_received_mask_);
+      packet_loss_histogram_name.append(".NoProxy");
+      continue;
+    }
 
-  if (result == net::OK) {
-    base::Histogram* rtt_histogram = base::Histogram::FactoryTimeGet(
-        rtt_histogram_name,
-        base::TimeDelta::FromMilliseconds(10),
-        base::TimeDelta::FromSeconds(60), 50,
+    if (result == net::OK) {
+      base::Histogram* rtt_histogram = base::Histogram::FactoryTimeGet(
+          rtt_histogram_name,
+          base::TimeDelta::FromMilliseconds(10),
+          base::TimeDelta::FromSeconds(60), 50,
+          base::Histogram::kUmaTargetedHistogramFlag);
+      rtt_histogram->AddTime(duration);
+      rtt_histogram_name.append(".NoProxy");
+    }
+
+    base::Histogram* status_histogram = base::LinearHistogram::FactoryGet(
+        status_histogram_name, 1, STATUS_MAX, STATUS_MAX+1,
         base::Histogram::kUmaTargetedHistogramFlag);
-    rtt_histogram->AddTime(duration);
+    status_histogram->Add(status);
+    status_histogram_name.append(".NoProxy");
   }
-
-  base::Histogram* status_histogram = base::LinearHistogram::FactoryGet(
-      status_histogram_name, 1, STATUS_MAX, STATUS_MAX+1,
-      base::Histogram::kUmaTargetedHistogramFlag);
-  status_histogram->Add(status);
 }
 
 // UDPStatsClient methods and members.
@@ -717,6 +733,43 @@
   delete this;
 }
 
+// ProxyDetector methods and members.
+ProxyDetector::ProxyDetector(const std::string& network_stats_server,
+                             NetworkStats::HistogramPortSelector histogram_port,
+                             IOThread* io_thread)
+    : network_stats_server_(network_stats_server),
+      histogram_port_(histogram_port),
+      io_thread_(io_thread) {
+}
+
+ProxyDetector::~ProxyDetector() {

[eroman, 2012/05/04 23:08:48]

When is ProxyDetector deleted? Note that if it is deleted while the
ResolveProxy() request is still in progress, that will lead to a use-after-free
when the callback is invoked.

I suggest adding one of two safety nets. Either:
  (a) assert to make sure there is no in-progress ProxyResolve request
  (b) Explicitly cancel the ProxyResolve request during deletion, if one is
outstanding. (ResolveProxy can take a request handle, and you can use this to
cancel it).

I recommend approach (a), and just making sure the lifetime is well understood.

[ramant (doing other things), 2012/05/07 23:35:47]

Implemented option a). Thanks for the above comment.

Done.

+}
+
+void ProxyDetector::ResolveProxy() {
+  std::string url = base::StringPrintf(
+      "https://%s:%d", network_stats_server_.c_str(), kPorts[histogram_port_]);

[eroman, 2012/05/04 23:08:48]

Assembling URLs in this fashion is generally not safe, since if the hostname is
an IPv6 literal concatenating ':' has different meaning.

There are several possible solutions:
  (a) Use HostPortPair::ToString() to stringize the host + port. In fact, I
suggest making the input to ProxyDetector be a HostPairPair regardless.

  (b) Use GURL::ReplaceComponents() to smuggle in a host and port into a URL.

The simplest strategy is probably (a), so I recommend that.

Lastly, is it intentional that the scheme be "https://" rather than "http://"?
Either is fine, just want to make sure this has been thought through (since PAC
scripts will sometimes give different results for HTTPS vs HTTP).

[ramant (doing other things), 2012/05/07 23:35:47]

Implemented option a). Thanks much for the suggestion.

Used https per following comment from rsleevi:
	
Ryan Sleevi 2012/04/28 02:06:32
  Would it make more sense to check for HTTPS here?

  The reason I mention it is that HTTPS will give you an indicator as to whether
or not the proxy server supports CONNECT, which is effectively how outbound
connections can be mediated by the proxy.

Should we use http?

Done.

+  GURL gurl(url);
+  net::ProxyService* service =
+      io_thread_->globals()->system_proxy_service.get();
+
+  int rv = service->ResolveProxy(
+      gurl,
+      &proxy_info_,
+      base::Bind(&ProxyDetector::OnResolveProxyComplete,
+                 base::Unretained(this)),
+      NULL,
+      net::BoundNetLog());
+  if (rv == net::OK)
+    OnResolveProxyComplete(rv);
+}
+
+void ProxyDetector::OnResolveProxyComplete(int result) {
+  bool has_proxy_server = (proxy_info_.proxy_server().is_valid() &&

[eroman, 2012/05/04 23:08:48]

One extra check to add is into that list is:
   result == net::OK

[ramant (doing other things), 2012/05/07 23:35:47]

Done.

+                           !proxy_info_.proxy_server().is_direct());
+  StartNetworkStatsTest(

[eroman, 2012/05/04 23:08:48]

[optional] I suggest de-coupling this from ProxyDetector. Instead, have
ProxyDetector take a Callback<boo> parameter. The caller can partially bind a
Callback to StartNetworkStatsTest with all the other parmaters already fixed
(hence don't need to mirror them in ProxyDetector).

[ramant (doing other things), 2012/05/07 23:35:47]

Done.

+      network_stats_server_, histogram_port_, has_proxy_server, io_thread_);
+}
+
 // static
 void CollectNetworkStats(const std::string& network_stats_server,
                          IOThread* io_thread) {
@@ -742,8 +795,6 @@
 
   CR_DEFINE_STATIC_LOCAL(scoped_refptr<base::FieldTrial>, trial, ());
   static bool collect_stats = false;
-
-  static uint32 port;
   static NetworkStats::HistogramPortSelector histogram_port;
 
   if (!trial.get()) {
@@ -782,7 +833,6 @@
           base::RandInt(NetworkStats::PORT_53, NetworkStats::PORT_8080));
       DCHECK_GE(histogram_port, NetworkStats::PORT_53);
       DCHECK_LE(histogram_port, NetworkStats::PORT_8080);
-      port = kPorts[histogram_port];
     }
   }
 
@@ -797,10 +847,23 @@
 
   ++number_of_tests_done;
 
+  ProxyDetector* proxy_client = new ProxyDetector(
+      network_stats_server, histogram_port, io_thread);
+  proxy_client->ResolveProxy();
+}
+
+// static
+void StartNetworkStatsTest(const std::string& network_stats_server,
+                           NetworkStats::HistogramPortSelector histogram_port,
+                           bool has_proxy_server,
+                           IOThread* io_thread) {
   net::HostResolver* host_resolver = io_thread->globals()->host_resolver.get();
   DCHECK(host_resolver);
 
-  net::HostPortPair server_address(network_stats_server, port);
+  DCHECK_GE(histogram_port, NetworkStats::PORT_53);
+  DCHECK_LE(histogram_port, NetworkStats::PORT_8080);
+  net::HostPortPair server_address(network_stats_server,
+                                   kPorts[histogram_port]);
 
   int experiment_to_run = base::RandInt(1, 5);
   switch (experiment_to_run) {
@@ -808,7 +871,7 @@
       {
         UDPStatsClient* small_udp_stats = new UDPStatsClient();
         small_udp_stats->Start(
-            host_resolver, server_address, histogram_port,
+            host_resolver, server_address, histogram_port, has_proxy_server,
             kSmallTestBytesToSend, 1, net::CompletionCallback());
       }
       break;
@@ -817,7 +880,7 @@
       {
         UDPStatsClient* large_udp_stats = new UDPStatsClient();
         large_udp_stats->Start(
-            host_resolver, server_address, histogram_port,
+            host_resolver, server_address, histogram_port, has_proxy_server,
             kLargeTestBytesToSend, 1, net::CompletionCallback());
       }
       break;
@@ -826,7 +889,7 @@
       {
         TCPStatsClient* small_tcp_client = new TCPStatsClient();
         small_tcp_client->Start(
-            host_resolver, server_address, histogram_port,
+            host_resolver, server_address, histogram_port, has_proxy_server,
             kSmallTestBytesToSend, 1, net::CompletionCallback());
       }
       break;
@@ -835,7 +898,7 @@
       {
         TCPStatsClient* large_tcp_client = new TCPStatsClient();
         large_tcp_client->Start(
-            host_resolver, server_address, histogram_port,
+            host_resolver, server_address, histogram_port, has_proxy_server,
             kLargeTestBytesToSend, 1, net::CompletionCallback());
       }
       break;
@@ -844,7 +907,7 @@
       {
         UDPStatsClient* packet_loss_udp_stats = new UDPStatsClient();
         packet_loss_udp_stats->Start(
-            host_resolver, server_address, histogram_port,
+            host_resolver, server_address, histogram_port, has_proxy_server,
             kLargeTestBytesToSend, kMaximumPackets, net::CompletionCallback());
       }
       break;