Add a histogram (Net.PacResultForStrippedUrl) that estimates how often PAC scripts depend on the URL path.

BUG=593759

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

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

8 files changed, 472 insertions, 9 deletions

diff --git a/net/data/proxy_resolver_v8_tracing_unittest/alert_url.js b/net/data/proxy_resolver_v8_tracing_unittest/alert_url.js
new file mode 100644
index 0000000..3cc14c9
--- /dev/null
+++ b/net/data/proxy_resolver_v8_tracing_unittest/alert_url.js
@@ -0,0 +1,8 @@
+// Copyright 2016 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.
+
+function FindProxyForURL(url, host) {
+  alert(url);
+  return "PROXY foobar:99";
+}
diff --git a/net/data/proxy_resolver_v8_tracing_unittest/dns_depending_on_url.js b/net/data/proxy_resolver_v8_tracing_unittest/dns_depending_on_url.js
new file mode 100644
index 0000000..ac66110
--- /dev/null
+++ b/net/data/proxy_resolver_v8_tracing_unittest/dns_depending_on_url.js
@@ -0,0 +1,17 @@
+// Copyright 2016 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.
+
+// This proxy script has different DNS dependencies based on whether the URL
+// contains "UseMyIpAddress". The final proxy list however is the same.
+function FindProxyForURL(url, host) {
+  if (url.indexOf("UseMyIpAddress") == -1) {
+    if (!myIpAddress())
+      return null;
+  } else {
+    if (!dnsResolve(host))
+      return null;
+  }
+
+  return "PROXY foopy:47";
+}
diff --git a/net/data/proxy_resolver_v8_tracing_unittest/error_depending_on_url.js b/net/data/proxy_resolver_v8_tracing_unittest/error_depending_on_url.js
new file mode 100644
index 0000000..42256eb
--- /dev/null
+++ b/net/data/proxy_resolver_v8_tracing_unittest/error_depending_on_url.js
@@ -0,0 +1,13 @@
+// Copyright 2016 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.
+
+// This proxy script throws an error if the URL does not contain the substring
+// "DontThrowError".
+function FindProxyForURL(url, host) {
+  if (url.indexOf("DontThrowError") == -1) {
+    ErrorUndefinedFunction();
+    return -1;
+  }
+  return "PROXY foopy:42";
+}
diff --git a/net/data/proxy_resolver_v8_tracing_unittest/return_url_as_proxy.js b/net/data/proxy_resolver_v8_tracing_unittest/return_url_as_proxy.js
new file mode 100644
index 0000000..fad3c85
--- /dev/null
+++ b/net/data/proxy_resolver_v8_tracing_unittest/return_url_as_proxy.js
@@ -0,0 +1,16 @@
+// Copyright 2016 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.
+
+// This proxy script returns a proxy list that encodes the URL that was passed
+// in.
+
+function convertUrlToHostname(url) {
+  // Turn the URL into something that resembles a hostname.
+  var result = encodeURIComponent(url);
+  return result.replace(/%/g, "x");
+}
+
+function FindProxyForURL(url, host) {
+  return "PROXY " + convertUrlToHostname(url) + ":99";
+}
diff --git a/net/proxy/proxy_resolver_v8_tracing.cc b/net/proxy/proxy_resolver_v8_tracing.cc
index dd2fe40..a9fdeeb 100644
--- a/net/proxy/proxy_resolver_v8_tracing.cc
+++ b/net/proxy/proxy_resolver_v8_tracing.cc
@@ -9,8 +9,10 @@
 #include <utility>
 #include <vector>
 
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/cancellation_flag.h"
@@ -25,6 +27,7 @@
 #include "net/proxy/proxy_info.h"
 #include "net/proxy/proxy_resolver_error_observer.h"
 #include "net/proxy/proxy_resolver_v8.h"
+#include "url/url_canon.h"
 
 // The intent of this class is explained in the design document:
 // https://docs.google.com/a/chromium.org/document/d/16Ij5OcVnR3s0MH4Z5XkhI9VTPoMJdaBn9rKreAmGOdE/edit
@@ -59,6 +62,24 @@ const size_t kMaxUniqueResolveDnsPerExec = 20;
 // hit this. (In fact normal scripts should not even have alerts() or errors).
 const size_t kMaxAlertsAndErrorsBytes = 2048;
 
+// Strips path information from the URL and replaces it with a
+// recognizable placeholder.
+// TODO(eroman): Remove when done gathering data for crbug.com/593759
+GURL StripUrlForBug593759(const GURL& url) {
+  GURL::Replacements replacements;
+  replacements.SetPathStr("PathIsHiddenForCrbug593759");
+  replacements.ClearQuery();
+  replacements.ClearRef();
+  return url.ReplaceComponents(replacements);
+}
+
+// TODO(eroman): Remove when done gathering data for crbug.com/593759
+void LogHistogramForBug593759(PacResultForStrippedUrl value) {
+  UMA_HISTOGRAM_ENUMERATION(
+      kHistogramPacResultForStrippedUrl, static_cast<int>(value),
+      static_cast<int>(PacResultForStrippedUrl::MAX_VALUE));
+}
+
 // The Job class is responsible for executing GetProxyForURL() and
 // creating ProxyResolverV8 instances, since both of these operations share
 // similar code.
@@ -149,6 +170,9 @@ class Job : public base::RefCountedThreadSafe<Job>,
   void ExecuteNonBlocking();
   int ExecuteProxyResolver();
 
+  // TODO(eroman): Remove when done gathering data for crbug.com/593759
+  void LogMetricsForBug593759(int original_error);
+
   // Implementation of ProxyResolverv8::JSBindings
   bool ResolveDns(const std::string& host,
                   ResolveDnsOperation op,
@@ -275,6 +299,9 @@ class Job : public base::RefCountedThreadSafe<Job>,
   // Whether the current execution needs to be restarted in blocking mode.
   bool should_restart_with_blocking_dns_;
 
+  // TODO(eroman): Remove when done gathering data for crbug.com/593759
+  bool dont_start_dns_ = false;
+
   // ---------------------------------------------------------------------------
   // State for pending DNS request.
   // ---------------------------------------------------------------------------
@@ -533,6 +560,9 @@ void Job::ExecuteNonBlocking() {
 
   int result = ExecuteProxyResolver();
 
+  // TODO(eroman): Remove when done gathering data for crbug.com/593759
+  LogMetricsForBug593759(result);
+
   if (should_restart_with_blocking_dns_) {
     DCHECK(!blocking_dns_);
     DCHECK(abandoned_);
@@ -572,6 +602,110 @@ int Job::ExecuteProxyResolver() {
   return result;
 }
 
+// Gathers data on how often PAC scripts execute differently depending
+// on the URL path and parameters.
+//
+// TODO(eroman): Remove when done gathering data for crbug.com/593759
+void Job::LogMetricsForBug593759(int original_error) {
+  CheckIsOnWorkerThread();
+
+  DCHECK(!blocking_dns_);
+
+  if (operation_ != GET_PROXY_FOR_URL || !url_.SchemeIsCryptographic()) {
+    // Only interested in FindProxyForURL() invocations for cryptographic URL
+    // schemes (https:// and wss://).
+    return;
+  }
+
+  if (should_restart_with_blocking_dns_) {
+    // The current instrumentation is limited to non-blocking DNS mode, for
+    // simplicity. Fallback to blocking mode is possible for unusual PAC
+    // scripts (non-deterministic, or relies on global state).
+    LogHistogramForBug593759(
+        PacResultForStrippedUrl::SKIPPED_FALLBACK_BLOCKING_DNS);
+    return;
+  }
+
+  if (abandoned_) {
+    // If the FindProxyForURL() attempt was abandoned, it was either cancelled
+    // or it encountered a missing DNS dependency. In the latter case the job
+    // will be re-started once the DNS has been resolved, so just wait until
+    // then.
+    return;
+  }
+
+  if (original_error != OK) {
+    // Only run the extra diagnostics for successful invocations of
+    // FindProxyForURL(). In other words, the instrumentation will
+    // not check whether the script succeeds when using a stripped
+    // path after having already failed on the original URL. A script error
+    // means the PAC script is already broken, so this would just skew the data.
+    return;
+  }
+
+  // Save some state variables to compare the original run against the new run
+  // using a stripped URL.
+  auto original_num_dns = num_dns_;
+  auto original_alerts_and_errors_byte_cost_ = alerts_and_errors_byte_cost_;
+  auto original_results = results_.ToPacString();
+
+  // Reset state variables used by ExecuteProxyResolver().
+  //
+  // This is a bit messy, but it keeps the diagnostics code local
+  // to LogMetricsForBug593759() without having to refactor the existing
+  // code.
+  //
+  // The intent is that after returning from this function all of the
+  // internal state is re-set to reflect the original completion of
+  // ExecuteProxyResolver(), not the second diagnostic one.
+  //
+  // Any global modifications made to the script state however are
+  // not undone, since creating a new script context just for this
+  // test would be expensive.
+  //
+  // Lastly, DNS resolution is disabled before calling
+  // ExecuteProxyResolver(), so only previously cached results can be
+  // used.
+  base::AutoReset<GURL> reset_url(&url_, StripUrlForBug593759(url_));
+  base::AutoReset<int> reset_num_dns(&num_dns_, 0);
+  base::AutoReset<std::vector<AlertOrError>> reset_alerts_and_errors(
+      &alerts_and_errors_, std::vector<AlertOrError>());
+  base::AutoReset<size_t> reset_alerts_and_errors_byte_cost(
+      &alerts_and_errors_byte_cost_, 0);
+  base::AutoReset<bool> reset_should_restart_with_blocking_dns(
+      &should_restart_with_blocking_dns_, false);
+  base::AutoReset<ProxyInfo> reset_results(&results_, ProxyInfo());
+  base::AutoReset<bool> reset_dont_start_dns(&dont_start_dns_, true);
+  base::AutoReset<bool> reset_abandoned(&abandoned_, false);
+
+  // Re-run FindProxyForURL().
+  auto result = ExecuteProxyResolver();
+
+  // Log the result of having run FindProxyForURL() with the modified
+  // URL to an UMA histogram.
+  if (should_restart_with_blocking_dns_) {
+    LogHistogramForBug593759(
+        PacResultForStrippedUrl::FAIL_FALLBACK_BLOCKING_DNS);
+  } else if (abandoned_) {
+    LogHistogramForBug593759(PacResultForStrippedUrl::FAIL_ABANDONED);
+  } else if (result != OK) {
+    LogHistogramForBug593759(PacResultForStrippedUrl::FAIL_ERROR);
+  } else if (original_results != results_.ToPacString()) {
+    LogHistogramForBug593759(
+        PacResultForStrippedUrl::FAIL_DIFFERENT_PROXY_LIST);
+  } else if (original_alerts_and_errors_byte_cost_ !=
+             alerts_and_errors_byte_cost_) {
+    // Here alerts_and_errors_byte_cost_ is being used as a cheap (albeit
+    // imprecise) fingerprint for the calls that were made to alert().
+    LogHistogramForBug593759(PacResultForStrippedUrl::SUCCESS_DIFFERENT_ALERTS);
+  } else if (original_num_dns != num_dns_) {
+    LogHistogramForBug593759(
+        PacResultForStrippedUrl::SUCCESS_DIFFERENT_NUM_DNS);
+  } else {
+    LogHistogramForBug593759(PacResultForStrippedUrl::SUCCESS);
+  }
+}
+
 bool Job::ResolveDns(const std::string& host,
                      ResolveDnsOperation op,
                      std::string* output,
@@ -646,6 +780,12 @@ bool Job::ResolveDnsNonBlocking(const std::string& host,
     return rv;
   }
 
+  // TODO(eroman): Remove when done gathering data for crbug.com/593759
+  if (dont_start_dns_) {
+    abandoned_ = true;
+    return false;
+  }
+
   if (num_dns_ <= last_num_dns_) {
     // The sequence of DNS operations is different from last time!
     ScheduleRestartWithBlockingDns();
@@ -1099,4 +1239,6 @@ ProxyResolverV8TracingFactory::Create() {
   return make_scoped_ptr(new ProxyResolverV8TracingFactoryImpl());
 }
 
+const char kHistogramPacResultForStrippedUrl[] = "Net.PacResultForStrippedUrl";
+
 }  // namespace net
diff --git a/net/proxy/proxy_resolver_v8_tracing.h b/net/proxy/proxy_resolver_v8_tracing.h
index f5f66cf..4ca3c82 100644
--- a/net/proxy/proxy_resolver_v8_tracing.h
+++ b/net/proxy/proxy_resolver_v8_tracing.h
@@ -89,6 +89,55 @@ class NET_EXPORT ProxyResolverV8TracingFactory {
   DISALLOW_COPY_AND_ASSIGN(ProxyResolverV8TracingFactory);
 };
 
+// This enum is used by an UMA histogram, so the values shouldn't be reordered
+// or renumbered.
+//
+// TODO(eroman): Remove when done gathering data for crbug.com/593759
+enum class PacResultForStrippedUrl {
+  // Did NOT measure the impact of running FindProxyForURL() with a modified
+  // URL path, because the original URL could not complete using the
+  // non-blocking DNS mode.
+  SKIPPED_FALLBACK_BLOCKING_DNS = 0,
+
+  // The result of running FindProxyForURL() with a modified URL path appears
+  // to be indistinguishable. (Although there may have been sideffects to the
+  // script state that won't manifest until later invocations).
+  SUCCESS = 1,
+
+  // Calling FindProxyForURL() with a modified URL path returned the same proxy
+  // list, but had measurable sideffects in calls to alert().
+  SUCCESS_DIFFERENT_ALERTS = 2,
+
+  // Calling FindProxyForURL() with a modified URL path returned the same proxy
+  // list, but invoked a different sequence of DNS resolutions. This would
+  // require a rather unusual script to trigger.
+  SUCCESS_DIFFERENT_NUM_DNS = 3,
+
+  // Calling FindProxyForURL() with a modified URL path resulted in a different
+  // set of DNS dependencies.
+  FAIL_ABANDONED = 4,
+
+  // Calling FindProxyForURL() with a modified URL path caused a different
+  // execution flow. Whereas with the original URL it succeeded with
+  // non-blocking DNS, this attempt requires fallback to blocking DNS (and was
+  // not attempted).
+  FAIL_FALLBACK_BLOCKING_DNS = 5,
+
+  // Calling FindProxyForURL() with a modified URL path caused a script error.
+  FAIL_ERROR = 6,
+
+  // Calling FindProxyForURL() with a modified URL path returned a different
+  // proxy list.
+  FAIL_DIFFERENT_PROXY_LIST = 7,
+
+  MAX_VALUE,
+};
+
+// TODO(eroman): Remove when done gathering data for crbug.com/593759
+//
+// This histogram name is exported only for the sake of unit-tests.
+extern NET_EXPORT_PRIVATE const char kHistogramPacResultForStrippedUrl[];
+
 }  // namespace net
 
 #endif  // NET_PROXY_PROXY_RESOLVER_V8_TRACING_H_
diff --git a/net/proxy/proxy_resolver_v8_tracing_unittest.cc b/net/proxy/proxy_resolver_v8_tracing_unittest.cc
index 888dd18..4d2b296 100644
--- a/net/proxy/proxy_resolver_v8_tracing_unittest.cc
+++ b/net/proxy/proxy_resolver_v8_tracing_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/histogram_tester.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_checker.h"
 #include "base/values.h"
@@ -37,6 +38,23 @@ class ProxyResolverV8TracingTest : public testing::Test {
     // spilling into the next test's execution.
     base::RunLoop().RunUntilIdle();
   }
+
+ protected:
+  // TODO(eroman): Remove when done gathering data for crbug.com/593759
+  void ExpectHistogramBucketCount(PacResultForStrippedUrl bucket,
+                                  size_t expected_total) {
+    histograms_.ExpectUniqueSample(kHistogramPacResultForStrippedUrl,
+                                   static_cast<int>(bucket), expected_total);
+  }
+
+  // TODO(eroman): Remove when done gathering data for crbug.com/593759
+  void ExpectHistogramTotal(size_t expected_total) {
+    histograms_.ExpectTotalCount(kHistogramPacResultForStrippedUrl,
+                                 expected_total);
+  }
+
+ private:
+  base::HistogramTester histograms_;
 };
 
 scoped_refptr<ProxyResolverScriptData> LoadScriptData(const char* filename) {
@@ -160,12 +178,15 @@ TEST_F(ProxyResolverV8TracingTest, Simple) {
   TestCompletionCallback callback;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://foo/"), &proxy_info,
                            callback.callback(), NULL,
                            mock_bindings.CreateBindings());
 
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::SUCCESS, 1);
+  ExpectHistogramTotal(1);
+
   EXPECT_EQ("foo:99", proxy_info.proxy_server().ToURI());
 
   EXPECT_EQ(0u, host_resolver.num_resolve());
@@ -175,6 +196,36 @@ TEST_F(ProxyResolverV8TracingTest, Simple) {
   EXPECT_TRUE(mock_bindings.GetErrors().empty());
 }
 
+TEST_F(ProxyResolverV8TracingTest, AlertUrl) {
+  MockCachingHostResolver host_resolver;
+  MockBindings mock_bindings(&host_resolver);
+
+  scoped_ptr<ProxyResolverV8Tracing> resolver =
+      CreateResolver(mock_bindings.CreateBindings(), "alert_url.js");
+
+  TestCompletionCallback callback;
+  ProxyInfo proxy_info;
+
+  resolver->GetProxyForURL(GURL("https://foo/path"), &proxy_info,
+                           callback.callback(), NULL,
+                           mock_bindings.CreateBindings());
+
+  EXPECT_EQ(OK, callback.WaitForResult());
+
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::SUCCESS_DIFFERENT_ALERTS,
+                             1);
+  ExpectHistogramTotal(1);
+
+  EXPECT_EQ("foobar:99", proxy_info.proxy_server().ToURI());
+
+  EXPECT_EQ(0u, host_resolver.num_resolve());
+
+  // There was 1 alerts and no errors.
+  EXPECT_EQ(1u, mock_bindings.GetAlerts().size());
+  EXPECT_EQ("https://foo/path", mock_bindings.GetAlerts()[0]);
+  EXPECT_TRUE(mock_bindings.GetErrors().empty());
+}
+
 TEST_F(ProxyResolverV8TracingTest, JavascriptError) {
   MockCachingHostResolver host_resolver;
   MockBindings mock_bindings(&host_resolver);
@@ -185,12 +236,14 @@ TEST_F(ProxyResolverV8TracingTest, JavascriptError) {
   TestCompletionCallback callback;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://throw-an-error/"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://throw-an-error/"), &proxy_info,
                            callback.callback(), NULL,
                            mock_bindings.CreateBindings());
 
   EXPECT_EQ(ERR_PAC_SCRIPT_FAILED, callback.WaitForResult());
 
+  ExpectHistogramTotal(0);
+
   EXPECT_EQ(0u, host_resolver.num_resolve());
 
   // Check the output -- there was 1 alert and 1 javascript error.
@@ -212,12 +265,16 @@ TEST_F(ProxyResolverV8TracingTest, TooManyAlerts) {
   TestCompletionCallback callback;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://foo/"), &proxy_info,
                            callback.callback(), NULL,
                            mock_bindings.CreateBindings());
 
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  ExpectHistogramBucketCount(
+      PacResultForStrippedUrl::SKIPPED_FALLBACK_BLOCKING_DNS, 1);
+  ExpectHistogramTotal(1);
+
   // Iteration1 does a DNS resolve
   // Iteration2 exceeds the alert buffer
   // Iteration3 runs in blocking mode and completes
@@ -248,12 +305,16 @@ TEST_F(ProxyResolverV8TracingTest, TooManyEmptyAlerts) {
   TestCompletionCallback callback;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://foo/"), &proxy_info,
                            callback.callback(), NULL,
                            mock_bindings.CreateBindings());
 
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  ExpectHistogramBucketCount(
+      PacResultForStrippedUrl::SKIPPED_FALLBACK_BLOCKING_DNS, 1);
+  ExpectHistogramTotal(1);
+
   EXPECT_EQ("foo:3", proxy_info.proxy_server().ToURI());
 
   EXPECT_EQ(1u, host_resolver.num_resolve());
@@ -294,12 +355,15 @@ TEST_F(ProxyResolverV8TracingTest, Dns) {
   TestCompletionCallback callback;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://foo/"), &proxy_info,
                            callback.callback(), NULL,
                            mock_bindings.CreateBindings());
 
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::SUCCESS, 1);
+  ExpectHistogramTotal(1);
+
   // The test does 13 DNS resolution, however only 7 of them are unique.
   EXPECT_EQ(7u, host_resolver.num_resolve());
 
@@ -346,12 +410,18 @@ TEST_F(ProxyResolverV8TracingTest, DnsChecksCache) {
   TestCompletionCallback callback2;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://foopy/req1"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://foopy/req1"), &proxy_info,
                            callback1.callback(), NULL,
                            mock_bindings.CreateBindings());
 
   EXPECT_EQ(OK, callback1.WaitForResult());
 
+  // This fails because executing FindProxyForURL() PAC script modifies global
+  // state each time, changing the result that is returned.
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::FAIL_DIFFERENT_PROXY_LIST,
+                             1);
+  ExpectHistogramTotal(1);
+
   // The test does 2 DNS resolutions.
   EXPECT_EQ(2u, host_resolver.num_resolve());
 
@@ -364,10 +434,19 @@ TEST_F(ProxyResolverV8TracingTest, DnsChecksCache) {
 
   EXPECT_EQ(OK, callback2.WaitForResult());
 
+  // The histograms are unchanged because the second invocation is for an
+  // http:// URL.
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::FAIL_DIFFERENT_PROXY_LIST,
+                             1);
+  ExpectHistogramTotal(1);
+
   EXPECT_EQ(4u, host_resolver.num_resolve());
 
   // This time no restarts were required, so g_iteration incremented by 1.
-  EXPECT_EQ("166.155.144.11:4", proxy_info.proxy_server().ToURI());
+  // TODO(eroman): Additionally the counter was incremented once by the
+  // diagnostics code that ran FindProxyForURL() with a stripped URL
+  // (should really be :4 and not :5).
+  EXPECT_EQ("166.155.144.11:5", proxy_info.proxy_server().ToURI());
 
   // There were no alerts or errors.
   EXPECT_TRUE(mock_bindings.GetAlerts().empty());
@@ -391,11 +470,15 @@ TEST_F(ProxyResolverV8TracingTest, FallBackToSynchronous1) {
   TestCompletionCallback callback;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://foo/"), &proxy_info,
                            callback.callback(), NULL,
                            mock_bindings.CreateBindings());
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  ExpectHistogramBucketCount(
+      PacResultForStrippedUrl::SKIPPED_FALLBACK_BLOCKING_DNS, 1);
+  ExpectHistogramTotal(1);
+
   // The script itself only does 2 DNS resolves per execution, however it
   // constructs the hostname using a global counter which changes on each
   // invocation.
@@ -430,11 +513,15 @@ TEST_F(ProxyResolverV8TracingTest, FallBackToSynchronous2) {
   TestCompletionCallback callback;
   ProxyInfo proxy_info;
 
-  resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info,
+  resolver->GetProxyForURL(GURL("https://foo/"), &proxy_info,
                            callback.callback(), NULL,
                            mock_bindings.CreateBindings());
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  ExpectHistogramBucketCount(
+      PacResultForStrippedUrl::SKIPPED_FALLBACK_BLOCKING_DNS, 1);
+  ExpectHistogramTotal(1);
+
   EXPECT_EQ(3u, host_resolver.num_resolve());
 
   EXPECT_EQ("166.155.144.44:100", proxy_info.proxy_server().ToURI());
@@ -466,6 +553,9 @@ TEST_F(ProxyResolverV8TracingTest, InfiniteDNSSequence) {
                            mock_bindings.CreateBindings());
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  // Was not called because this is an http:// URL.
+  ExpectHistogramTotal(0);
+
   EXPECT_EQ(20u, host_resolver.num_resolve());
 
   EXPECT_EQ(
@@ -506,6 +596,9 @@ TEST_F(ProxyResolverV8TracingTest, InfiniteDNSSequence2) {
                            mock_bindings.CreateBindings());
   EXPECT_EQ(OK, callback.WaitForResult());
 
+  // Was not called because this is an http:// URL.
+  ExpectHistogramTotal(0);
+
   EXPECT_EQ(20u, host_resolver.num_resolve());
 
   EXPECT_EQ("null21:34", proxy_info.proxy_server().ToURI());
@@ -518,6 +611,94 @@ TEST_F(ProxyResolverV8TracingTest, InfiniteDNSSequence2) {
   EXPECT_EQ("iteration: 21", mock_bindings.GetAlerts()[0]);
 }
 
+TEST_F(ProxyResolverV8TracingTest, DifferentResultBasedOnUrl) {
+  MockCachingHostResolver host_resolver;
+  MockBindings mock_bindings(&host_resolver);
+
+  scoped_ptr<ProxyResolverV8Tracing> resolver =
+      CreateResolver(mock_bindings.CreateBindings(), "return_url_as_proxy.js");
+
+  TestCompletionCallback callback;
+  ProxyInfo proxy_info;
+
+  resolver->GetProxyForURL(GURL("https://foo/path1"), &proxy_info,
+                           callback.callback(), NULL,
+                           mock_bindings.CreateBindings());
+
+  EXPECT_EQ(OK, callback.WaitForResult());
+
+  ExpectHistogramTotal(1);
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::FAIL_DIFFERENT_PROXY_LIST,
+                             1);
+
+  EXPECT_EQ("httpsx3Ax2Fx2Ffoox2Fpath1:99", proxy_info.proxy_server().ToURI());
+
+  EXPECT_EQ(0u, host_resolver.num_resolve());
+
+  // There were no alerts or errors.
+  EXPECT_TRUE(mock_bindings.GetAlerts().empty());
+  EXPECT_TRUE(mock_bindings.GetErrors().empty());
+}
+
+TEST_F(ProxyResolverV8TracingTest, ErrorDependingOnUrl) {
+  MockCachingHostResolver host_resolver;
+  MockBindings mock_bindings(&host_resolver);
+
+  scoped_ptr<ProxyResolverV8Tracing> resolver = CreateResolver(
+      mock_bindings.CreateBindings(), "error_depending_on_url.js");
+
+  TestCompletionCallback callback;
+  ProxyInfo proxy_info;
+
+  resolver->GetProxyForURL(GURL("https://foo/DontThrowError"), &proxy_info,
+                           callback.callback(), NULL,
+                           mock_bindings.CreateBindings());
+
+  EXPECT_EQ(OK, callback.WaitForResult());
+
+  ExpectHistogramTotal(1);
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::FAIL_ERROR, 1);
+
+  EXPECT_EQ("foopy:42", proxy_info.proxy_server().ToURI());
+
+  EXPECT_EQ(0u, host_resolver.num_resolve());
+
+  // There were no alerts or errors.
+  EXPECT_TRUE(mock_bindings.GetAlerts().empty());
+  EXPECT_TRUE(mock_bindings.GetErrors().empty());
+}
+
+TEST_F(ProxyResolverV8TracingTest, DnsDependingOnUrl) {
+  MockCachingHostResolver host_resolver;
+  MockBindings mock_bindings(&host_resolver);
+
+  host_resolver.rules()->AddRule("host", "166.155.144.55");
+
+  // Catch-all that will be used for myIpAddress().
+  host_resolver.rules()->AddRule("*", "133.122.100.200");
+
+  scoped_ptr<ProxyResolverV8Tracing> resolver =
+      CreateResolver(mock_bindings.CreateBindings(), "dns_depending_on_url.js");
+
+  TestCompletionCallback callback;
+  ProxyInfo proxy_info;
+
+  resolver->GetProxyForURL(GURL("https://foo/UseMyIpAddress"), &proxy_info,
+                           callback.callback(), NULL,
+                           mock_bindings.CreateBindings());
+
+  EXPECT_EQ(OK, callback.WaitForResult());
+
+  ExpectHistogramBucketCount(PacResultForStrippedUrl::FAIL_ABANDONED, 1);
+  ExpectHistogramTotal(1);
+
+  EXPECT_EQ("foopy:47", proxy_info.proxy_server().ToURI());
+
+  // No errors.
+  EXPECT_TRUE(mock_bindings.GetErrors().empty());
+  ASSERT_EQ(0u, mock_bindings.GetAlerts().size());
+}
+
 void DnsDuringInitHelper(bool synchronous_host_resolver) {
   MockCachingHostResolver host_resolver;
   MockBindings mock_bindings(&host_resolver);
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index b42f34c..547bc2e 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -26273,6 +26273,28 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Net.PacResultForStrippedUrl" enum="PacResultForStrippedUrl">
+  <owner>eroman@chromium.org</owner>
+  <summary>
+    Proxy Auto Config (PAC) allows specifying an arbitrary javascript program to
+    pick network proxies on a per-request basis (based on the URL). This works
+    by calling the script's FindProxyForURL() function with the target URL as
+    its first argument.
+
+    Traditionally, the URL parameter passed into the script contains the exact
+    URL being loaded, except maybe for having stripped the fragment and embedded
+    identity portions of the URL.
+
+    This histogram records what happens when the URL passed into
+    FindProxyForURL() additionally has had its path component stripped. Does it
+    return the same proxy list as when calling FindProxyForURL() with the
+    original (unmodified) URL?
+
+    This comparison is done only when the URL scheme implies a secure channel
+    (i.e. https:// and wss://), in order to gather data for crbug.com/593759.
+  </summary>
+</histogram>
+
 <histogram name="Net.Ping_ResponseStartedTime" units="ms">
   <obsolete>
     Deprecated 4/16/2014. No longer tracked.
@@ -76492,6 +76514,21 @@ To add a new entry, add it with any value and run test to compute valid value.
   <int value="4" label="Index"/>
 </enum>
 
+<enum name="PacResultForStrippedUrl" type="int">
+  <summary>
+    Result for PAC script experiment as defined in
+    net/proxy/proxy_resolver_v8_tracing.h
+  </summary>
+  <int value="0" label="SKIPPED_FALLBACK_BLOCKING_DNS"/>
+  <int value="1" label="SUCCESS"/>
+  <int value="2" label="SUCCESS_DIFFERENT_ALERTS"/>
+  <int value="3" label="SUCCESS_DIFFERENT_NUM_DNS"/>
+  <int value="4" label="FAIL_ABANDONED"/>
+  <int value="5" label="FAIL_FALLBACK_BLOCKING_DNS"/>
+  <int value="6" label="FAIL_ERROR"/>
+  <int value="7" label="FAIL_DIFFERENT_PROXY_LIST"/>
+</enum>
+
 <enum name="PageLoadEvent" type="int">
   <obsolete>
     Deprecated in favor of {Committed|Provisional|InternalError}LoadEvent