Add metrics for DHCP WPAD feature. Fix memory overwrite.

BUG=18575
TEST=net_unittests

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

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

base/metrics/histogram.cc

@@ -940,6 +940,21 @@ Histogram::ClassType CustomHistogram::histogram_type() const {
   return CUSTOM_HISTOGRAM;
 }
 
+// static
+std::vector<Histogram::Sample> CustomHistogram::ArrayToCustomRanges(
+    const Sample* values, size_t num_values) {
+  std::vector<Sample> all_values;
+  for (size_t i = 0; i < num_values; ++i) {
+    Sample value = values[i];
+    all_values.push_back(value);
+
+    // Ensure that a guard bucket is added. If we end up with duplicate
+    // values, FactoryGet will take care of removing them.
+    all_values.push_back(value + 1);
+  }
+  return all_values;
+}
+
 CustomHistogram::CustomHistogram(const std::string& name,
                                  const std::vector<Sample>& custom_ranges)
     : Histogram(name, custom_ranges[1], custom_ranges.back(),

base/metrics/histogram.h

@@ -108,7 +108,6 @@ class Lock;
 
 // Support histograming of an enumerated value.  The samples should always be
 // less than boundary_value.
-
 #define HISTOGRAM_ENUMERATION(name, sample, boundary_value) do { \
     static base::Histogram* counter(NULL); \
     if (!counter) \
@@ -118,6 +117,10 @@ class Lock;
     counter->Add(sample); \
   } while (0)
 
+// Support histograming of an enumerated value. Samples should be one of the
+// std::vector<int> list provided via |custom_ranges|. You can use the helper
+// function |base::CustomHistogram::ArrayToCustomRanges(samples, num_samples)|
+// to transform a C-style array of valid sample values to a std::vector<int>.
 #define HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges) do { \
     static base::Histogram* counter(NULL); \
     if (!counter) \
@@ -664,6 +667,14 @@ class BASE_API CustomHistogram : public Histogram {
   // Overridden from Histogram:
   virtual ClassType histogram_type() const;
 
+  // Helper method for transforming an array of valid enumeration values
+  // to the std::vector<int> expected by HISTOGRAM_CUSTOM_ENUMERATION.
+  // This function ensures that a guard bucket exists right after any
+  // valid sample value (unless the next higher sample is also a valid value),
+  // so that invalid samples never fall into the same bucket as valid samples.
+  static std::vector<Sample> ArrayToCustomRanges(const Sample* values,
+                                                 size_t num_values);
+
  protected:
   CustomHistogram(const std::string& name,
                   const std::vector<Sample>& custom_ranges);

net/base/net_errors.cc

-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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/base/net_errors.h"
 
 #include "base/basictypes.h"
+#include "base/metrics/histogram.h"
 #include "base/stringize_macros.h"
 
+namespace {
+
+// Get all valid error codes into an array as positive numbers, for use in the
+// |GetAllErrorCodesForUma| function below.
+#define NET_ERROR(label, value) -(value),
+const int kAllErrorCodes[] = {
+#include "net/base/net_error_list.h"
+};
+#undef NET_ERROR
+
+}  // namespace
+
 namespace net {
 
 const char kErrorDomain[] = "net";
@@ -26,4 +39,9 @@ const char* ErrorToString(int error) {
   }
 }
 
+std::vector<int> GetAllErrorCodesForUma() {
+  return base::CustomHistogram::ArrayToCustomRanges(
+      kAllErrorCodes, arraysize(kAllErrorCodes));
+}
+
 }  // namespace net

net/base/net_errors.h

@@ -6,6 +6,8 @@
 #define NET_BASE_NET_ERRORS_H__
 #pragma once
 
+#include <vector>
+
 #include "base/basictypes.h"
 
 namespace net {
@@ -39,6 +41,15 @@ inline bool IsCertificateError(int error) {
 // Map system error code to Error.
 Error MapSystemError(int os_error);
 
+// Returns a list of all the possible net error codes (not counting OK). This
+// is intended for use with UMA histograms that are reporting the result of
+// an action that is represented as a net error code.
+//
+// Note that the error codes are all positive (since histograms expect positive
+// sample values). Also note that a guard bucket is created after any valid
+// error code that is not followed immediately by a valid error code.
+std::vector<int> GetAllErrorCodesForUma();
+
 }  // namespace net
 
 #endif  // NET_BASE_NET_ERRORS_H__

net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc

@@ -5,6 +5,7 @@
 #include "net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
 
 #include "base/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
 #include "base/sys_string_conversions.h"
 #include "base/task.h"
 #include "base/threading/worker_pool.h"
@@ -201,8 +202,12 @@ void DhcpProxyScriptAdapterFetcher::OnFetcherDone(int result) {
 void DhcpProxyScriptAdapterFetcher::TransitionToFinish() {
   DCHECK(state_ == STATE_WAIT_DHCP || state_ == STATE_WAIT_URL);
   state_ = STATE_FINISH;
-  callback_->Run(result_);
+  CompletionCallback* callback = callback_;
   callback_ = NULL;
+
+  // Be careful not to touch any member state after this, as the client
+  // may delete us during this callback.
+  callback->Run(result_);
 }
 
 ProxyScriptFetcher* DhcpProxyScriptAdapterFetcher::ImplCreateScriptFetcher() {
@@ -269,7 +274,8 @@ std::string DhcpProxyScriptAdapterFetcher::GetPacURLFromDhcp(
   } while (res == ERROR_MORE_DATA && retry_count <= 3);
 
   if (res != NO_ERROR) {
-    NOTREACHED();
+    LOG(INFO) << "Error fetching PAC URL from DHCP: " << res;
+    UMA_HISTOGRAM_COUNTS("Net.DhcpWpadUnhandledDhcpError", 1);
   } else if (wpad_params.nBytesData) {
     // The result should be ASCII, not wide character.
     DCHECK_EQ(strlen(reinterpret_cast<const char*>(wpad_params.Data)) + 1,

net/proxy/dhcp_proxy_script_fetcher_win.cc

@@ -4,6 +4,8 @@
 
 #include "net/proxy/dhcp_proxy_script_fetcher_win.h"
 
+#include "base/metrics/histogram.h"
+#include "base/perftimer.h"
 #include "net/base/net_errors.h"
 #include "net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
 
@@ -18,6 +20,14 @@ namespace {
 // adapter finishes first.
 const int kMaxWaitAfterFirstResultMs = 400;
 
+const int kGetAdaptersAddressesErrors[] = {
+  ERROR_ADDRESS_NOT_ASSOCIATED,
+  ERROR_BUFFER_OVERFLOW,
+  ERROR_INVALID_PARAMETER,
+  ERROR_NOT_ENOUGH_MEMORY,
+  ERROR_NO_DATA,
+};
+
 }  // namespace
 
 namespace net {
@@ -33,6 +43,7 @@ DhcpProxyScriptFetcherWin::DhcpProxyScriptFetcherWin(
 }
 
 DhcpProxyScriptFetcherWin::~DhcpProxyScriptFetcherWin() {
+  // Count as user-initiated if we are not yet in STATE_DONE.
   Cancel();
 }
 
@@ -44,6 +55,8 @@ int DhcpProxyScriptFetcherWin::Fetch(string16* utf16_text,
     return ERR_UNEXPECTED;
   }
 
+  fetch_start_time_ = base::TimeTicks::Now();
+
   std::set<std::string> adapter_names;
   if (!ImplGetCandidateAdapterNames(&adapter_names)) {
     return ERR_UNEXPECTED;
@@ -72,6 +85,19 @@ int DhcpProxyScriptFetcherWin::Fetch(string16* utf16_text,
 void DhcpProxyScriptFetcherWin::Cancel() {
   DCHECK(CalledOnValidThread());
 
+  if (state_ != STATE_DONE) {
+    // We only count this stat if the cancel was explicitly initiated by
+    // our client, and if we weren't already in STATE_DONE.
+    UMA_HISTOGRAM_TIMES("Net.DhcpWpadCancelTime",
+                        base::TimeTicks::Now() - fetch_start_time_);
+  }
+
+  CancelImpl();
+}
+
+void DhcpProxyScriptFetcherWin::CancelImpl() {
+  DCHECK(CalledOnValidThread());
+
   if (state_ != STATE_DONE) {
     wait_timer_.Stop();
     state_ = STATE_DONE;
@@ -134,6 +160,14 @@ void DhcpProxyScriptFetcherWin::OnFetcherDone(int result) {
 
 void DhcpProxyScriptFetcherWin::OnWaitTimer() {
   DCHECK_EQ(state_, STATE_SOME_RESULTS);
+
+  // These are intended to help us understand whether our timeout may
+  // be too aggressive or not aggressive enough.
+  UMA_HISTOGRAM_COUNTS_100("Net.DhcpWpadNumAdaptersAtWaitTimer",
+                           fetchers_.size());
+  UMA_HISTOGRAM_COUNTS_100("Net.DhcpWpadNumPendingAdaptersAtWaitTimer",
+                           num_pending_fetchers_);
+
   TransitionToDone();
 }
 
@@ -172,10 +206,18 @@ void DhcpProxyScriptFetcherWin::TransitionToDone() {
     }
   }
 
-  Cancel();
+  CancelImpl();
   DCHECK_EQ(state_, STATE_DONE);
   DCHECK(fetchers_.empty());
 
+  UMA_HISTOGRAM_TIMES("Net.DhcpWpadCompletionTime",
+                      base::TimeTicks::Now() - fetch_start_time_);
+
+  if (result != OK) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+        "Net.DhcpWpadFetchError", std::abs(result), GetAllErrorCodesForUma());
+  }
+
   client_callback_->Run(result);
 }
 
@@ -204,6 +246,8 @@ bool DhcpProxyScriptFetcherWin::GetCandidateAdapterNames(
   scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> adapters;
   ULONG error = ERROR_SUCCESS;
   int num_tries = 0;
+
+  PerfTimer time_api_access;
   do {
     adapters.reset(
         reinterpret_cast<IP_ADAPTER_ADDRESSES*>(malloc(adapters_size)));
@@ -219,6 +263,20 @@ bool DhcpProxyScriptFetcherWin::GetCandidateAdapterNames(
     ++num_tries;
   } while (error == ERROR_BUFFER_OVERFLOW && num_tries <= 3);
 
+  // This is primarily to validate our belief that the GetAdaptersAddresses API
+  // function is fast enough to call synchronously from the network thread.
+  UMA_HISTOGRAM_TIMES("Net.DhcpWpadGetAdaptersAddressesTime",
+                      time_api_access.Elapsed());
+
+  if (error != ERROR_SUCCESS) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+        "Net.DhcpWpadGetAdaptersAddressesError",
+        error,
+        base::CustomHistogram::ArrayToCustomRanges(
+            kGetAdaptersAddressesErrors,
+            arraysize(kGetAdaptersAddressesErrors)));
+  }
+
   if (error == ERROR_NO_DATA) {
     // There are no adapters that we care about.
     return true;

net/proxy/dhcp_proxy_script_fetcher_win.h

@@ -12,6 +12,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/threading/non_thread_safe.h"
+#include "base/time.h"
 #include "base/timer.h"
 #include "net/proxy/dhcp_proxy_script_fetcher.h"
 
@@ -44,6 +45,7 @@ class NET_TEST DhcpProxyScriptFetcherWin
 
  protected:
   // Event/state transition handlers
+  void CancelImpl();
   void OnFetcherDone(int result);
   void OnWaitTimer();
   void TransitionToDone();
@@ -115,6 +117,12 @@ class NET_TEST DhcpProxyScriptFetcherWin
 
   scoped_refptr<URLRequestContext> url_request_context_;
 
+ private:
+  // TODO(joi): Move other members to private as appropriate.
+
+  // Time |Fetch()| was last called, 0 if never.
+  base::TimeTicks fetch_start_time_;
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(DhcpProxyScriptFetcherWin);
 };