Revert 219744 "Switch the Windows DHCP/PAC implementation to use..."

> 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
> 
> Review URL: https://codereview.chromium.org/23068017

TBR=joi@chromium.org

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

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

6 files changed, 29 insertions, 105 deletions

diff --git a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc
index 676f6c3..56e4747 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/task_runner.h"
+#include "base/threading/worker_pool.h"
 #include "base/time/time.h"
 #include "net/base/net_errors.h"
 #include "net/proxy/dhcpcsvc_init_win.h"
@@ -32,10 +32,8 @@ const int kTimeoutMs = 2000;
 namespace net {
 
 DhcpProxyScriptAdapterFetcher::DhcpProxyScriptAdapterFetcher(
-    URLRequestContext* url_request_context,
-    scoped_refptr<base::TaskRunner> task_runner)
-    : task_runner_(task_runner),
-      state_(STATE_START),
+    URLRequestContext* url_request_context)
+    : state_(STATE_START),
       result_(ERR_IO_PENDING),
       url_request_context_(url_request_context) {
   DCHECK(url_request_context_);
@@ -57,7 +55,7 @@ void DhcpProxyScriptAdapterFetcher::Fetch(
   wait_timer_.Start(FROM_HERE, ImplGetTimeout(),
                     this, &DhcpProxyScriptAdapterFetcher::OnTimeout);
   scoped_refptr<DhcpQuery> dhcp_query(ImplCreateDhcpQuery());
-  task_runner_->PostTaskAndReply(
+  base::WorkerPool::PostTaskAndReply(
       FROM_HERE,
       base::Bind(
           &DhcpProxyScriptAdapterFetcher::DhcpQuery::GetPacURLForAdapter,
@@ -66,7 +64,8 @@ void DhcpProxyScriptAdapterFetcher::Fetch(
       base::Bind(
           &DhcpProxyScriptAdapterFetcher::OnDhcpQueryDone,
           AsWeakPtr(),
-          dhcp_query));
+          dhcp_query),
+      true);
 }
 
 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 59597d9..fadf234 100644
--- a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
@@ -17,10 +17,6 @@
 #include "net/base/net_export.h"
 #include "url/gurl.h"
 
-namespace base {
-class TaskRunner;
-}
-
 namespace net {
 
 class ProxyScriptFetcher;
@@ -33,9 +29,8 @@ class NET_EXPORT_PRIVATE DhcpProxyScriptAdapterFetcher
       NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
   // |url_request_context| must outlive DhcpProxyScriptAdapterFetcher.
-  // |task_runner| will be used to post tasks to a thread.
-  DhcpProxyScriptAdapterFetcher(URLRequestContext* url_request_context,
-                                scoped_refptr<base::TaskRunner> task_runner);
+  explicit DhcpProxyScriptAdapterFetcher(
+      URLRequestContext* url_request_context);
   virtual ~DhcpProxyScriptAdapterFetcher();
 
   // Starts a fetch.  On completion (but not cancellation), |callback|
@@ -149,9 +144,6 @@ 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 ab87b44..dc00685 100644
--- a/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
@@ -33,10 +33,8 @@ const char* const kPacUrl = "http://pacserver/script.pac";
 class MockDhcpProxyScriptAdapterFetcher
     : public DhcpProxyScriptAdapterFetcher {
  public:
-  explicit MockDhcpProxyScriptAdapterFetcher(
-      URLRequestContext* context,
-      scoped_refptr<base::TaskRunner> task_runner)
-      : DhcpProxyScriptAdapterFetcher(context, task_runner),
+  explicit MockDhcpProxyScriptAdapterFetcher(URLRequestContext* context)
+      : DhcpProxyScriptAdapterFetcher(context),
         dhcp_delay_(base::TimeDelta::FromMilliseconds(1)),
         timeout_(TestTimeouts::action_timeout()),
         configured_url_(kPacUrl),
@@ -134,16 +132,8 @@ class FetcherClient {
  public:
   FetcherClient()
       : url_request_context_(new TestURLRequestContext()),
-        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();
+        fetcher_(
+            new MockDhcpProxyScriptAdapterFetcher(url_request_context_.get())) {
   }
 
   void WaitForResult(int expected_error) {
@@ -161,7 +151,6 @@ class FetcherClient {
 
   TestCompletionCallback callback_;
   scoped_ptr<URLRequestContext> url_request_context_;
-  scoped_refptr<base::SequencedWorkerPool> worker_pool_;
   scoped_ptr<MockDhcpProxyScriptAdapterFetcher> fetcher_;
   base::string16 pac_text_;
 };
@@ -264,9 +253,8 @@ class MockDhcpRealFetchProxyScriptAdapterFetcher
     : public MockDhcpProxyScriptAdapterFetcher {
  public:
   explicit MockDhcpRealFetchProxyScriptAdapterFetcher(
-      URLRequestContext* context,
-      scoped_refptr<base::TaskRunner> task_runner)
-      : MockDhcpProxyScriptAdapterFetcher(context, task_runner),
+      URLRequestContext* context)
+      : MockDhcpProxyScriptAdapterFetcher(context),
         url_request_context_(context) {
   }
 
@@ -292,12 +280,9 @@ 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, runner));
+          &url_request_context));
   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 2f63a3e..04f0fb0 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.cc
@@ -18,27 +18,6 @@
 
 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.
@@ -63,16 +42,11 @@ 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,
@@ -90,7 +64,7 @@ int DhcpProxyScriptFetcherWin::Fetch(base::string16* utf16_text,
   destination_string_ = utf16_text;
 
   last_query_ = ImplCreateAdapterQuery();
-  GetTaskRunner()->PostTaskAndReply(
+  base::WorkerPool::PostTaskAndReply(
       FROM_HERE,
       base::Bind(
           &DhcpProxyScriptFetcherWin::AdapterQuery::GetCandidateAdapterNames,
@@ -98,7 +72,8 @@ int DhcpProxyScriptFetcherWin::Fetch(base::string16* utf16_text,
       base::Bind(
           &DhcpProxyScriptFetcherWin::OnGetCandidateAdapterNamesDone,
           AsWeakPtr(),
-          last_query_));
+          last_query_),
+      true);
 
   return ERR_IO_PENDING;
 }
@@ -292,15 +267,9 @@ 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_,
-                                           GetTaskRunner());
+  return new DhcpProxyScriptAdapterFetcher(url_request_context_);
 }
 
 DhcpProxyScriptFetcherWin::AdapterQuery*
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win.h b/net/proxy/dhcp_proxy_script_fetcher_win.h
index d6f14f9..79fc4b3 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.h
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.h
@@ -16,10 +16,6 @@
 #include "base/timer/timer.h"
 #include "net/proxy/dhcp_proxy_script_fetcher.h"
 
-namespace base {
-class SequencedWorkerPool;
-}
-
 namespace net {
 
 class DhcpProxyScriptAdapterFetcher;
@@ -54,8 +50,6 @@ 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.
@@ -167,9 +161,6 @@ 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 cf0cee05..9ef59a4 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
@@ -156,10 +156,9 @@ TEST(DhcpProxyScriptFetcherWin, RealFetchWithCancel) {
 class DelayingDhcpProxyScriptAdapterFetcher
     : public DhcpProxyScriptAdapterFetcher {
  public:
-  DelayingDhcpProxyScriptAdapterFetcher(
-      URLRequestContext* url_request_context,
-      scoped_refptr<base::TaskRunner> task_runner)
-      : DhcpProxyScriptAdapterFetcher(url_request_context, task_runner) {
+  explicit DelayingDhcpProxyScriptAdapterFetcher(
+      URLRequestContext* url_request_context)
+      : DhcpProxyScriptAdapterFetcher(url_request_context) {
   }
 
   class DelayingDhcpQuery : public DhcpQuery {
@@ -190,8 +189,7 @@ class DelayingDhcpProxyScriptFetcherWin
   }
 
   DhcpProxyScriptAdapterFetcher* ImplCreateAdapterFetcher() OVERRIDE {
-    return new DelayingDhcpProxyScriptAdapterFetcher(url_request_context(),
-                                                     GetTaskRunner());
+    return new DelayingDhcpProxyScriptAdapterFetcher(url_request_context());
   }
 };
 
@@ -214,9 +212,8 @@ TEST(DhcpProxyScriptFetcherWin, RealFetchWithDeferredCancel) {
 class DummyDhcpProxyScriptAdapterFetcher
     : public DhcpProxyScriptAdapterFetcher {
  public:
-  DummyDhcpProxyScriptAdapterFetcher(URLRequestContext* context,
-                                     scoped_refptr<base::TaskRunner> runner)
-      : DhcpProxyScriptAdapterFetcher(context, runner),
+  explicit DummyDhcpProxyScriptAdapterFetcher(URLRequestContext* context)
+      : DhcpProxyScriptAdapterFetcher(context),
         did_finish_(false),
         result_(OK),
         pac_script_(L"bingo"),
@@ -300,8 +297,6 @@ 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.
@@ -317,8 +312,7 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
                                    base::string16 pac_script,
                                    base::TimeDelta fetch_delay) {
     scoped_ptr<DummyDhcpProxyScriptAdapterFetcher> adapter_fetcher(
-        new DummyDhcpProxyScriptAdapterFetcher(url_request_context(),
-                                               GetTaskRunner()));
+        new DummyDhcpProxyScriptAdapterFetcher(url_request_context()));
     adapter_fetcher->Configure(
         did_finish, result, pac_script, fetch_delay.InMilliseconds());
     PushBackAdapter(adapter_name, adapter_fetcher.release());
@@ -378,7 +372,7 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
 };
 
 class FetcherClient {
- public:
+public:
   FetcherClient()
       : context_(new TestURLRequestContext),
         fetcher_(context_.get()),
@@ -420,10 +414,6 @@ class FetcherClient {
     fetcher_.ResetTestState();
   }
 
-  scoped_refptr<base::TaskRunner> GetTaskRunner() {
-    return fetcher_.GetTaskRunner();
-  }
-
   scoped_ptr<URLRequestContext> context_;
   MockDhcpProxyScriptFetcherWin fetcher_;
   bool finished_;
@@ -436,8 +426,7 @@ class FetcherClient {
 void TestNormalCaseURLConfiguredOneAdapter(FetcherClient* client) {
   TestURLRequestContext context;
   scoped_ptr<DummyDhcpProxyScriptAdapterFetcher> adapter_fetcher(
-      new DummyDhcpProxyScriptAdapterFetcher(&context,
-                                             client->GetTaskRunner()));
+      new DummyDhcpProxyScriptAdapterFetcher(&context));
   adapter_fetcher->Configure(true, OK, L"bingo", 1);
   client->fetcher_.PushBackAdapter("a", adapter_fetcher.release());
   client->RunTest();
@@ -597,8 +586,7 @@ TEST(DhcpProxyScriptFetcherWin, ShortCircuitLessPreferredAdapters) {
 void TestImmediateCancel(FetcherClient* client) {
   TestURLRequestContext context;
   scoped_ptr<DummyDhcpProxyScriptAdapterFetcher> adapter_fetcher(
-      new DummyDhcpProxyScriptAdapterFetcher(&context,
-                                             client->GetTaskRunner()));
+      new DummyDhcpProxyScriptAdapterFetcher(&context));
   adapter_fetcher->Configure(true, OK, L"bingo", 1);
   client->fetcher_.PushBackAdapter("a", adapter_fetcher.release());
   client->RunTest();