Switch the Windows DHCP/PAC implementation to use SequencedWorkerPool.

This accomplishes two things:
a) One less usage of the deprecated base::WorkerPool; and
b) Sets an upper limit on the number of threads used to look up PAC information.

BUG=240034,251774
R=eroman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219744
Rolled back due to compilation problem, will re-land.

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

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

6 files changed, 107 insertions, 30 deletions

diff --git a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc
index 56e4747..676f6c3 100644
--- a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc
@@ -10,7 +10,7 @@
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/sys_string_conversions.h"
-#include "base/threading/worker_pool.h"
+#include "base/task_runner.h"
 #include "base/time/time.h"
 #include "net/base/net_errors.h"
 #include "net/proxy/dhcpcsvc_init_win.h"
@@ -32,8 +32,10 @@ const int kTimeoutMs = 2000;
 namespace net {
 
 DhcpProxyScriptAdapterFetcher::DhcpProxyScriptAdapterFetcher(
-    URLRequestContext* url_request_context)
-    : state_(STATE_START),
+    URLRequestContext* url_request_context,
+    scoped_refptr<base::TaskRunner> task_runner)
+    : task_runner_(task_runner),
+      state_(STATE_START),
       result_(ERR_IO_PENDING),
       url_request_context_(url_request_context) {
   DCHECK(url_request_context_);
@@ -55,7 +57,7 @@ void DhcpProxyScriptAdapterFetcher::Fetch(
   wait_timer_.Start(FROM_HERE, ImplGetTimeout(),
                     this, &DhcpProxyScriptAdapterFetcher::OnTimeout);
   scoped_refptr<DhcpQuery> dhcp_query(ImplCreateDhcpQuery());
-  base::WorkerPool::PostTaskAndReply(
+  task_runner_->PostTaskAndReply(
       FROM_HERE,
       base::Bind(
           &DhcpProxyScriptAdapterFetcher::DhcpQuery::GetPacURLForAdapter,
@@ -64,8 +66,7 @@ void DhcpProxyScriptAdapterFetcher::Fetch(
       base::Bind(
           &DhcpProxyScriptAdapterFetcher::OnDhcpQueryDone,
           AsWeakPtr(),
-          dhcp_query),
-      true);
+          dhcp_query));
 }
 
 void DhcpProxyScriptAdapterFetcher::Cancel() {
diff --git a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
index fadf234..59597d9 100644
--- a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
@@ -17,6 +17,10 @@
 #include "net/base/net_export.h"
 #include "url/gurl.h"
 
+namespace base {
+class TaskRunner;
+}
+
 namespace net {
 
 class ProxyScriptFetcher;
@@ -29,8 +33,9 @@ class NET_EXPORT_PRIVATE DhcpProxyScriptAdapterFetcher
       NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
   // |url_request_context| must outlive DhcpProxyScriptAdapterFetcher.
-  explicit DhcpProxyScriptAdapterFetcher(
-      URLRequestContext* url_request_context);
+  // |task_runner| will be used to post tasks to a thread.
+  DhcpProxyScriptAdapterFetcher(URLRequestContext* url_request_context,
+                                scoped_refptr<base::TaskRunner> task_runner);
   virtual ~DhcpProxyScriptAdapterFetcher();
 
   // Starts a fetch.  On completion (but not cancellation), |callback|
@@ -144,6 +149,9 @@ class NET_EXPORT_PRIVATE DhcpProxyScriptAdapterFetcher
   void OnFetcherDone(int result);
   void TransitionToFinish();
 
+  // TaskRunner for posting tasks to a worker thread.
+  scoped_refptr<base::TaskRunner> task_runner_;
+
   // Current state of this state machine.
   State state_;
 
diff --git a/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc b/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
index dc00685..1728512 100644
--- a/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/test/perftimer.h"
 #include "base/test/test_timeouts.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "base/timer/timer.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
@@ -33,8 +34,10 @@ const char* const kPacUrl = "http://pacserver/script.pac";
 class MockDhcpProxyScriptAdapterFetcher
     : public DhcpProxyScriptAdapterFetcher {
  public:
-  explicit MockDhcpProxyScriptAdapterFetcher(URLRequestContext* context)
-      : DhcpProxyScriptAdapterFetcher(context),
+  explicit MockDhcpProxyScriptAdapterFetcher(
+      URLRequestContext* context,
+      scoped_refptr<base::TaskRunner> task_runner)
+      : DhcpProxyScriptAdapterFetcher(context, task_runner),
         dhcp_delay_(base::TimeDelta::FromMilliseconds(1)),
         timeout_(TestTimeouts::action_timeout()),
         configured_url_(kPacUrl),
@@ -132,8 +135,16 @@ class FetcherClient {
  public:
   FetcherClient()
       : url_request_context_(new TestURLRequestContext()),
-        fetcher_(
-            new MockDhcpProxyScriptAdapterFetcher(url_request_context_.get())) {
+        worker_pool_(
+            new base::SequencedWorkerPool(4, "DhcpAdapterFetcherTest")),
+        fetcher_(new MockDhcpProxyScriptAdapterFetcher(
+            url_request_context_.get(),
+            worker_pool_->GetTaskRunnerWithShutdownBehavior(
+                base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN))) {
+  }
+
+  ~FetcherClient() {
+    worker_pool_->Shutdown();
   }
 
   void WaitForResult(int expected_error) {
@@ -151,6 +162,7 @@ class FetcherClient {
 
   TestCompletionCallback callback_;
   scoped_ptr<URLRequestContext> url_request_context_;
+  scoped_refptr<base::SequencedWorkerPool> worker_pool_;
   scoped_ptr<MockDhcpProxyScriptAdapterFetcher> fetcher_;
   base::string16 pac_text_;
 };
@@ -253,8 +265,9 @@ class MockDhcpRealFetchProxyScriptAdapterFetcher
     : public MockDhcpProxyScriptAdapterFetcher {
  public:
   explicit MockDhcpRealFetchProxyScriptAdapterFetcher(
-      URLRequestContext* context)
-      : MockDhcpProxyScriptAdapterFetcher(context),
+      URLRequestContext* context,
+      scoped_refptr<base::TaskRunner> task_runner)
+      : MockDhcpProxyScriptAdapterFetcher(context, task_runner),
         url_request_context_(context) {
   }
 
@@ -280,9 +293,12 @@ TEST(DhcpProxyScriptAdapterFetcher, MockDhcpRealFetch) {
 
   FetcherClient client;
   TestURLRequestContext url_request_context;
+  scoped_refptr<base::TaskRunner> runner =
+      client.worker_pool_->GetTaskRunnerWithShutdownBehavior(
+          base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
   client.fetcher_.reset(
       new MockDhcpRealFetchProxyScriptAdapterFetcher(
-          &url_request_context));
+          &url_request_context, runner));
   client.fetcher_->configured_url_ = configured_url.spec();
   client.RunTest();
   client.WaitForResult(OK);
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win.cc b/net/proxy/dhcp_proxy_script_fetcher_win.cc
index 04f0fb0..ac28e0f 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.cc
@@ -8,7 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/metrics/histogram.h"
 #include "base/test/perftimer.h"
-#include "base/threading/worker_pool.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "net/base/net_errors.h"
 #include "net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
 
@@ -18,6 +18,27 @@
 
 namespace {
 
+// How many threads to use at maximum to do DHCP lookups. This is
+// chosen based on the following UMA data:
+// - When OnWaitTimer fires, ~99.8% of users have 6 or fewer network
+//   adapters enabled for DHCP in total.
+// - At the same measurement point, ~99.7% of users have 3 or fewer pending
+//   DHCP adapter lookups.
+// - There is however a very long and thin tail of users who have
+//   systems reporting up to 100+ adapters (this must be some very weird
+//   OS bug (?), probably the cause of http://crbug.com/240034).
+//
+// The maximum number of threads is chosen such that even systems that
+// report a huge number of network adapters should not run out of
+// memory from this number of threads, while giving a good chance of
+// getting back results for any responsive adapters.
+//
+// The ~99.8% of systems that have 6 or fewer network adapters will
+// not grow the thread pool to its maximum size (rather, they will
+// grow it to 6 or fewer threads) so setting the limit lower would not
+// improve performance or memory usage on those systems.
+const int kMaxDhcpLookupThreads = 12;
+
 // How long to wait at maximum after we get results (a PAC file or
 // knowledge that no PAC file is configured) from whichever network
 // adapter finishes first.
@@ -42,11 +63,16 @@ DhcpProxyScriptFetcherWin::DhcpProxyScriptFetcherWin(
       destination_string_(NULL),
       url_request_context_(url_request_context) {
   DCHECK(url_request_context_);
+
+  worker_pool_ = new base::SequencedWorkerPool(kMaxDhcpLookupThreads,
+                                               "PacDhcpLookup");
 }
 
 DhcpProxyScriptFetcherWin::~DhcpProxyScriptFetcherWin() {
   // Count as user-initiated if we are not yet in STATE_DONE.
   Cancel();
+
+  worker_pool_->Shutdown();
 }
 
 int DhcpProxyScriptFetcherWin::Fetch(base::string16* utf16_text,
@@ -64,7 +90,7 @@ int DhcpProxyScriptFetcherWin::Fetch(base::string16* utf16_text,
   destination_string_ = utf16_text;
 
   last_query_ = ImplCreateAdapterQuery();
-  base::WorkerPool::PostTaskAndReply(
+  GetTaskRunner()->PostTaskAndReply(
       FROM_HERE,
       base::Bind(
           &DhcpProxyScriptFetcherWin::AdapterQuery::GetCandidateAdapterNames,
@@ -72,8 +98,7 @@ int DhcpProxyScriptFetcherWin::Fetch(base::string16* utf16_text,
       base::Bind(
           &DhcpProxyScriptFetcherWin::OnGetCandidateAdapterNamesDone,
           AsWeakPtr(),
-          last_query_),
-      true);
+          last_query_));
 
   return ERR_IO_PENDING;
 }
@@ -267,9 +292,15 @@ URLRequestContext* DhcpProxyScriptFetcherWin::url_request_context() const {
   return url_request_context_;
 }
 
+scoped_refptr<base::TaskRunner> DhcpProxyScriptFetcherWin::GetTaskRunner() {
+  return worker_pool_->GetTaskRunnerWithShutdownBehavior(
+      base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+}
+
 DhcpProxyScriptAdapterFetcher*
     DhcpProxyScriptFetcherWin::ImplCreateAdapterFetcher() {
-  return new DhcpProxyScriptAdapterFetcher(url_request_context_);
+  return new DhcpProxyScriptAdapterFetcher(url_request_context_,
+                                           GetTaskRunner());
 }
 
 DhcpProxyScriptFetcherWin::AdapterQuery*
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win.h b/net/proxy/dhcp_proxy_script_fetcher_win.h
index 79fc4b3..d6f14f9 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.h
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.h
@@ -16,6 +16,10 @@
 #include "base/timer/timer.h"
 #include "net/proxy/dhcp_proxy_script_fetcher.h"
 
+namespace base {
+class SequencedWorkerPool;
+}
+
 namespace net {
 
 class DhcpProxyScriptAdapterFetcher;
@@ -50,6 +54,8 @@ class NET_EXPORT_PRIVATE DhcpProxyScriptFetcherWin
 
   URLRequestContext* url_request_context() const;
 
+  scoped_refptr<base::TaskRunner> GetTaskRunner();
+
   // This inner class encapsulate work done on a worker pool thread.
   // The class calls GetCandidateAdapterNames, which can take a couple of
   // hundred milliseconds.
@@ -161,6 +167,9 @@ class NET_EXPORT_PRIVATE DhcpProxyScriptFetcherWin
   // Time |Fetch()| was last called, 0 if never.
   base::TimeTicks fetch_start_time_;
 
+  // Worker pool we use for all DHCP lookup tasks.
+  scoped_refptr<base::SequencedWorkerPool> worker_pool_;
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(DhcpProxyScriptFetcherWin);
 };
 
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc b/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
index 9ef59a4..cf0cee05 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
@@ -156,9 +156,10 @@ TEST(DhcpProxyScriptFetcherWin, RealFetchWithCancel) {
 class DelayingDhcpProxyScriptAdapterFetcher
     : public DhcpProxyScriptAdapterFetcher {
  public:
-  explicit DelayingDhcpProxyScriptAdapterFetcher(
-      URLRequestContext* url_request_context)
-      : DhcpProxyScriptAdapterFetcher(url_request_context) {
+  DelayingDhcpProxyScriptAdapterFetcher(
+      URLRequestContext* url_request_context,
+      scoped_refptr<base::TaskRunner> task_runner)
+      : DhcpProxyScriptAdapterFetcher(url_request_context, task_runner) {
   }
 
   class DelayingDhcpQuery : public DhcpQuery {
@@ -189,7 +190,8 @@ class DelayingDhcpProxyScriptFetcherWin
   }
 
   DhcpProxyScriptAdapterFetcher* ImplCreateAdapterFetcher() OVERRIDE {
-    return new DelayingDhcpProxyScriptAdapterFetcher(url_request_context());
+    return new DelayingDhcpProxyScriptAdapterFetcher(url_request_context(),
+                                                     GetTaskRunner());
   }
 };
 
@@ -212,8 +214,9 @@ TEST(DhcpProxyScriptFetcherWin, RealFetchWithDeferredCancel) {
 class DummyDhcpProxyScriptAdapterFetcher
     : public DhcpProxyScriptAdapterFetcher {
  public:
-  explicit DummyDhcpProxyScriptAdapterFetcher(URLRequestContext* context)
-      : DhcpProxyScriptAdapterFetcher(context),
+  DummyDhcpProxyScriptAdapterFetcher(URLRequestContext* context,
+                                     scoped_refptr<base::TaskRunner> runner)
+      : DhcpProxyScriptAdapterFetcher(context, runner),
         did_finish_(false),
         result_(OK),
         pac_script_(L"bingo"),
@@ -297,6 +300,8 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
     ResetTestState();
   }
 
+  using DhcpProxyScriptFetcherWin::GetTaskRunner;
+
   // Adds a fetcher object to the queue of fetchers used by
   // |ImplCreateAdapterFetcher()|, and its name to the list of adapters
   // returned by ImplGetCandidateAdapterNames.
@@ -312,7 +317,8 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
                                    base::string16 pac_script,
                                    base::TimeDelta fetch_delay) {
     scoped_ptr<DummyDhcpProxyScriptAdapterFetcher> adapter_fetcher(
-        new DummyDhcpProxyScriptAdapterFetcher(url_request_context()));
+        new DummyDhcpProxyScriptAdapterFetcher(url_request_context(),
+                                               GetTaskRunner()));
     adapter_fetcher->Configure(
         did_finish, result, pac_script, fetch_delay.InMilliseconds());
     PushBackAdapter(adapter_name, adapter_fetcher.release());
@@ -372,7 +378,7 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
 };
 
 class FetcherClient {
-public:
+ public:
   FetcherClient()
       : context_(new TestURLRequestContext),
         fetcher_(context_.get()),
@@ -414,6 +420,10 @@ public:
     fetcher_.ResetTestState();
   }
 
+  scoped_refptr<base::TaskRunner> GetTaskRunner() {
+    return fetcher_.GetTaskRunner();
+  }
+
   scoped_ptr<URLRequestContext> context_;
   MockDhcpProxyScriptFetcherWin fetcher_;
   bool finished_;
@@ -426,7 +436,8 @@ public:
 void TestNormalCaseURLConfiguredOneAdapter(FetcherClient* client) {
   TestURLRequestContext context;
   scoped_ptr<DummyDhcpProxyScriptAdapterFetcher> adapter_fetcher(
-      new DummyDhcpProxyScriptAdapterFetcher(&context));
+      new DummyDhcpProxyScriptAdapterFetcher(&context,
+                                             client->GetTaskRunner()));
   adapter_fetcher->Configure(true, OK, L"bingo", 1);
   client->fetcher_.PushBackAdapter("a", adapter_fetcher.release());
   client->RunTest();
@@ -586,7 +597,8 @@ TEST(DhcpProxyScriptFetcherWin, ShortCircuitLessPreferredAdapters) {
 void TestImmediateCancel(FetcherClient* client) {
   TestURLRequestContext context;
   scoped_ptr<DummyDhcpProxyScriptAdapterFetcher> adapter_fetcher(
-      new DummyDhcpProxyScriptAdapterFetcher(&context));
+      new DummyDhcpProxyScriptAdapterFetcher(&context,
+                                             client->GetTaskRunner()));
   adapter_fetcher->Configure(true, OK, L"bingo", 1);
   client->fetcher_.PushBackAdapter("a", adapter_fetcher.release());
   client->RunTest();