Do GetAdaptersAddresses on a worker thread.

BUG=84047
TEST=net_unittests

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

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

4 files changed, 253 insertions, 66 deletions

diff --git a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
index 79b3d1e..f650773 100644
--- a/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
+++ b/net/proxy/dhcp_proxy_script_adapter_fetcher_win.h
@@ -111,6 +111,9 @@ class NET_TEST DhcpProxyScriptAdapterFetcher
   // only a weak reference back to the main object, so that the main object
   // can be destroyed before the thread ends.  This also keeps the main
   // object completely thread safe and allows it to be non-refcounted.
+  //
+  // TODO(joi): Replace with PostTaskAndReply once http://crbug.com/86301
+  // has been implemented.
   class NET_TEST WorkerThread
       : public base::RefCountedThreadSafe<WorkerThread> {
    public:
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win.cc b/net/proxy/dhcp_proxy_script_fetcher_win.cc
index dbe4df2..2c7fd23 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.cc
@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram.h"
 #include "base/perftimer.h"
+#include "base/threading/worker_pool.h"
 #include "net/base/net_errors.h"
 #include "net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
 
@@ -45,6 +46,11 @@ DhcpProxyScriptFetcherWin::DhcpProxyScriptFetcherWin(
 DhcpProxyScriptFetcherWin::~DhcpProxyScriptFetcherWin() {
   // Count as user-initiated if we are not yet in STATE_DONE.
   Cancel();
+
+  // The WeakPtr we passed to the worker thread may be destroyed on the
+  // worker thread.  This detaches any outstanding WeakPtr state from
+  // the current thread.
+  base::SupportsWeakPtr<DhcpProxyScriptFetcherWin>::DetachFromThread();
 }
 
 int DhcpProxyScriptFetcherWin::Fetch(string16* utf16_text,
@@ -57,27 +63,12 @@ int DhcpProxyScriptFetcherWin::Fetch(string16* utf16_text,
 
   fetch_start_time_ = base::TimeTicks::Now();
 
-  std::set<std::string> adapter_names;
-  if (!ImplGetCandidateAdapterNames(&adapter_names)) {
-    return ERR_UNEXPECTED;
-  }
-  if (adapter_names.empty()) {
-    return ERR_PAC_NOT_IN_DHCP;
-  }
-
-  state_ = STATE_NO_RESULTS;
-
+  state_ = STATE_WAIT_ADAPTERS;
   client_callback_ = callback;
   destination_string_ = utf16_text;
 
-  for (std::set<std::string>::iterator it = adapter_names.begin();
-       it != adapter_names.end();
-       ++it) {
-    DhcpProxyScriptAdapterFetcher* fetcher(ImplCreateAdapterFetcher());
-    fetcher->Fetch(*it, &fetcher_callback_);
-    fetchers_.push_back(fetcher);
-  }
-  num_pending_fetchers_ = fetchers_.size();
+  worker_thread_ = ImplCreateWorkerThread(AsWeakPtr());
+  worker_thread_->Start();
 
   return ERR_IO_PENDING;
 }
@@ -113,6 +104,31 @@ void DhcpProxyScriptFetcherWin::CancelImpl() {
   }
 }
 
+void DhcpProxyScriptFetcherWin::OnGetCandidateAdapterNamesDone(
+    const std::set<std::string>& adapter_names) {
+  DCHECK(CalledOnValidThread());
+
+  // We may have been cancelled.
+  if (state_ != STATE_WAIT_ADAPTERS)
+    return;
+
+  state_ = STATE_NO_RESULTS;
+
+  if (adapter_names.empty()) {
+    TransitionToDone();
+    return;
+  }
+
+  for (std::set<std::string>::const_iterator it = adapter_names.begin();
+       it != adapter_names.end();
+       ++it) {
+    DhcpProxyScriptAdapterFetcher* fetcher(ImplCreateAdapterFetcher());
+    fetcher->Fetch(*it, &fetcher_callback_);
+    fetchers_.push_back(fetcher);
+  }
+  num_pending_fetchers_ = fetchers_.size();
+}
+
 std::string DhcpProxyScriptFetcherWin::GetFetcherName() const {
   DCHECK(CalledOnValidThread());
   return "win";
@@ -175,33 +191,33 @@ void DhcpProxyScriptFetcherWin::OnWaitTimer() {
 void DhcpProxyScriptFetcherWin::TransitionToDone() {
   DCHECK(state_ == STATE_NO_RESULTS || state_ == STATE_SOME_RESULTS);
 
-  // Should have returned immediately at Fetch() if no adapters to check.
-  DCHECK(!fetchers_.empty());
-
-  // Scan twice for the result; once through the whole list for success,
-  // then if no success, return result for most preferred network adapter,
-  // preferring "real" network errors to the ERR_PAC_NOT_IN_DHCP error.
-  // Default to ERR_ABORTED if no fetcher completed.
-  int result = ERR_ABORTED;
-  for (FetcherVector::iterator it = fetchers_.begin();
-       it != fetchers_.end();
-       ++it) {
-    if ((*it)->DidFinish() && (*it)->GetResult() == OK) {
-      result = OK;
-      *destination_string_ = (*it)->GetPacScript();
-      pac_url_ = (*it)->GetPacURL();
-      break;
-    }
-  }
-  if (result != OK) {
-    destination_string_->clear();
+  int result = ERR_PAC_NOT_IN_DHCP;  // Default if no fetchers.
+  if (!fetchers_.empty()) {
+    // Scan twice for the result; once through the whole list for success,
+    // then if no success, return result for most preferred network adapter,
+    // preferring "real" network errors to the ERR_PAC_NOT_IN_DHCP error.
+    // Default to ERR_ABORTED if no fetcher completed.
+    result = ERR_ABORTED;
     for (FetcherVector::iterator it = fetchers_.begin();
          it != fetchers_.end();
          ++it) {
-      if ((*it)->DidFinish()) {
-        result = (*it)->GetResult();
-        if (result != ERR_PAC_NOT_IN_DHCP) {
-          break;
+      if ((*it)->DidFinish() && (*it)->GetResult() == OK) {
+        result = OK;
+        *destination_string_ = (*it)->GetPacScript();
+        pac_url_ = (*it)->GetPacURL();
+        break;
+      }
+    }
+    if (result != OK) {
+      destination_string_->clear();
+      for (FetcherVector::iterator it = fetchers_.begin();
+           it != fetchers_.end();
+           ++it) {
+        if ((*it)->DidFinish()) {
+          result = (*it)->GetResult();
+          if (result != ERR_PAC_NOT_IN_DHCP) {
+            break;
+          }
         }
       }
     }
@@ -238,9 +254,10 @@ DhcpProxyScriptAdapterFetcher*
   return new DhcpProxyScriptAdapterFetcher(url_request_context_);
 }
 
-bool DhcpProxyScriptFetcherWin::ImplGetCandidateAdapterNames(
-    std::set<std::string>* adapter_names) {
-  return GetCandidateAdapterNames(adapter_names);
+DhcpProxyScriptFetcherWin::WorkerThread*
+    DhcpProxyScriptFetcherWin::ImplCreateWorkerThread(
+        const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner) {
+  return new WorkerThread(owner);
 }
 
 int DhcpProxyScriptFetcherWin::ImplGetMaxWaitMs() {
@@ -313,4 +330,53 @@ bool DhcpProxyScriptFetcherWin::GetCandidateAdapterNames(
   return true;
 }
 
+DhcpProxyScriptFetcherWin::WorkerThread::WorkerThread(
+    const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner) {
+  Init(owner);
+}
+
+DhcpProxyScriptFetcherWin::WorkerThread::~WorkerThread() {
+}
+
+void DhcpProxyScriptFetcherWin::WorkerThread::Start() {
+  bool succeeded = base::WorkerPool::PostTask(
+      FROM_HERE,
+      NewRunnableMethod(
+          this,
+          &DhcpProxyScriptFetcherWin::WorkerThread::ThreadFunc),
+      true);
+  DCHECK(succeeded);
+}
+
+void DhcpProxyScriptFetcherWin::WorkerThread::ThreadFunc() {
+  ImplGetCandidateAdapterNames(&adapter_names_);
+
+  bool succeeded = origin_loop_->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(
+          this,
+          &DhcpProxyScriptFetcherWin::WorkerThread::OnThreadDone));
+  DCHECK(succeeded);
+}
+
+void DhcpProxyScriptFetcherWin::WorkerThread::OnThreadDone() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (owner_)
+    owner_->OnGetCandidateAdapterNamesDone(adapter_names_);
+}
+
+DhcpProxyScriptFetcherWin::WorkerThread::WorkerThread() {
+}
+
+void DhcpProxyScriptFetcherWin::WorkerThread::Init(
+    const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner) {
+  owner_ = owner;
+  origin_loop_ = base::MessageLoopProxy::CreateForCurrentThread();
+}
+
+bool DhcpProxyScriptFetcherWin::WorkerThread::ImplGetCandidateAdapterNames(
+    std::set<std::string>* adapter_names) {
+  return DhcpProxyScriptFetcherWin::GetCandidateAdapterNames(adapter_names);
+}
+
 }  // namespace net
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win.h b/net/proxy/dhcp_proxy_script_fetcher_win.h
index a220f56..ae9548a 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.h
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.h
@@ -11,6 +11,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
+#include "base/message_loop_proxy.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time.h"
 #include "base/timer.h"
@@ -24,6 +25,7 @@ class URLRequestContext;
 // Windows-specific implementation.
 class NET_TEST DhcpProxyScriptFetcherWin
     : public DhcpProxyScriptFetcher,
+      public base::SupportsWeakPtr<DhcpProxyScriptFetcherWin>,
       NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
   // Creates a DhcpProxyScriptFetcherWin that issues requests through
@@ -48,15 +50,71 @@ class NET_TEST DhcpProxyScriptFetcherWin
 
   URLRequestContext* url_request_context() const;
 
+  // This inner class is used to encapsulate a worker thread that calls
+  // GetCandidateAdapterNames as it can take a couple of hundred
+  // milliseconds.
+  //
+  // TODO(joi): Replace with PostTaskAndReply once http://crbug.com/86301
+  // has been implemented.
+  class NET_TEST WorkerThread
+      : public base::RefCountedThreadSafe<WorkerThread> {
+   public:
+    // Creates and initializes (but does not start) the worker thread.
+    explicit WorkerThread(
+        const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner);
+    virtual ~WorkerThread();
+
+    // Starts the worker thread.
+    void Start();
+
+   protected:
+    WorkerThread();  // To override in unit tests only.
+    void Init(const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner);
+
+    // Virtual method introduced to allow unit testing.
+    virtual bool ImplGetCandidateAdapterNames(
+        std::set<std::string>* adapter_names);
+
+    // Callback for ThreadFunc; this executes back on the main thread,
+    // not the worker thread. May be overridden by unit tests.
+    virtual void OnThreadDone();
+
+   private:
+    // This is the method that runs on the worker thread.
+    void ThreadFunc();
+
+    // All work except ThreadFunc and (sometimes) destruction should occur
+    // on the thread that constructs the object.
+    base::ThreadChecker thread_checker_;
+
+    // May only be accessed on the thread that constructs the object.
+    base::WeakPtr<DhcpProxyScriptFetcherWin> owner_;
+
+    // Used by worker thread to post a message back to the original
+    // thread.  Fine to use a proxy since in the case where the original
+    // thread has gone away, that would mean the |owner_| object is gone
+    // anyway, so there is nobody to receive the result.
+    scoped_refptr<base::MessageLoopProxy> origin_loop_;
+
+    // This is constructed on the originating thread, then used on the
+    // worker thread, then used again on the originating thread only when
+    // the task has completed on the worker thread. No locking required.
+    std::set<std::string> adapter_names_;
+
+    DISALLOW_COPY_AND_ASSIGN(WorkerThread);
+  };
+
   // Virtual methods introduced to allow unit testing.
   virtual DhcpProxyScriptAdapterFetcher* ImplCreateAdapterFetcher();
-  virtual bool ImplGetCandidateAdapterNames(
-      std::set<std::string>* adapter_names);
+  virtual WorkerThread* ImplCreateWorkerThread(
+      const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner);
   virtual int ImplGetMaxWaitMs();
 
  private:
   // Event/state transition handlers
   void CancelImpl();
+  void OnGetCandidateAdapterNamesDone(
+      const std::set<std::string>& adapter_names);
   void OnFetcherDone(int result);
   void OnWaitTimer();
   void TransitionToDone();
@@ -73,7 +131,9 @@ class NET_TEST DhcpProxyScriptFetcherWin
   //    it has completed for the fastest adapter, return the PAC file
   //    available for the most preferred network adapter, if any.
   //
-  // The state machine goes from START->NO_RESULTS when it creates
+  // The state machine goes from START->WAIT_ADAPTERS when it starts a
+  // worker thread to get the list of adapters with DHCP enabled.
+  // It then goes from WAIT_ADAPTERS->NO_RESULTS when it creates
   // and starts an DhcpProxyScriptAdapterFetcher for each adapter.  It goes
   // from NO_RESULTS->SOME_RESULTS when it gets the first result; at this
   // point a wait timer is started.  It goes from SOME_RESULTS->DONE in
@@ -88,6 +148,7 @@ class NET_TEST DhcpProxyScriptFetcherWin
   // allowed to be outstanding at any given time.
   enum State {
     STATE_START,
+    STATE_WAIT_ADAPTERS,
     STATE_NO_RESULTS,
     STATE_SOME_RESULTS,
     STATE_DONE,
@@ -122,6 +183,8 @@ class NET_TEST DhcpProxyScriptFetcherWin
 
   scoped_refptr<URLRequestContext> url_request_context_;
 
+  scoped_refptr<WorkerThread> worker_thread_;
+
   // Time |Fetch()| was last called, 0 if never.
   base::TimeTicks fetch_start_time_;
 
diff --git a/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc b/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
index e22915b..09ffb66 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc
@@ -263,8 +263,37 @@ class DummyDhcpProxyScriptAdapterFetcher
 
 class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
  public:
+  class MockWorkerThread : public WorkerThread {
+   public:
+    MockWorkerThread() : worker_finished_event_(true, false) {
+    }
+
+    virtual ~MockWorkerThread() {
+    }
+
+    void Init(const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner) {
+      WorkerThread::Init(owner);
+    }
+
+    virtual bool ImplGetCandidateAdapterNames(
+        std::set<std::string>* adapter_names) OVERRIDE {
+      adapter_names->insert(
+          mock_adapter_names_.begin(), mock_adapter_names_.end());
+      return true;
+    }
+
+    virtual void OnThreadDone() OVERRIDE {
+      WorkerThread::OnThreadDone();
+      worker_finished_event_.Signal();
+    }
+
+    std::vector<std::string> mock_adapter_names_;
+    base::WaitableEvent worker_finished_event_;
+  };
+
   MockDhcpProxyScriptFetcherWin()
-      : DhcpProxyScriptFetcherWin(new TestURLRequestContext()) {
+      : DhcpProxyScriptFetcherWin(new TestURLRequestContext()),
+        num_fetchers_created_(0) {
     ResetTestState();
   }
 
@@ -273,7 +302,7 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
   // returned by ImplGetCandidateAdapterNames.
   void PushBackAdapter(const std::string& adapter_name,
                        DhcpProxyScriptAdapterFetcher* fetcher) {
-    adapter_names_.push_back(adapter_name);
+    worker_thread_->mock_adapter_names_.push_back(adapter_name);
     adapter_fetchers_.push_back(fetcher);
   }
 
@@ -289,13 +318,15 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
   }
 
   DhcpProxyScriptAdapterFetcher* ImplCreateAdapterFetcher() OVERRIDE {
+    ++num_fetchers_created_;
     return adapter_fetchers_[next_adapter_fetcher_index_++];
   }
 
-  bool ImplGetCandidateAdapterNames(
-      std::set<std::string>* adapter_names) OVERRIDE {
-    adapter_names->insert(adapter_names_.begin(), adapter_names_.end());
-    return true;
+  virtual WorkerThread* ImplCreateWorkerThread(
+      const base::WeakPtr<DhcpProxyScriptFetcherWin>& owner) OVERRIDE {
+    DCHECK(worker_thread_);
+    worker_thread_->Init(owner);
+    return worker_thread_.get();
   }
 
   int ImplGetMaxWaitMs() OVERRIDE {
@@ -304,9 +335,9 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
 
   void ResetTestState() {
     next_adapter_fetcher_index_ = 0;
+    num_fetchers_created_ = 0;
     adapter_fetchers_.clear();
-    // String pointers contained herein will have been freed during test.
-    adapter_names_.clear();
+    worker_thread_ = new MockWorkerThread();
     max_wait_ms_ = TestTimeouts::tiny_timeout_ms();
   }
 
@@ -320,9 +351,10 @@ class MockDhcpProxyScriptFetcherWin : public DhcpProxyScriptFetcherWin {
   // class via ImplCreateAdapterFetcher.
   std::vector<DhcpProxyScriptAdapterFetcher*> adapter_fetchers_;
 
-  std::vector<std::string> adapter_names_;
+  scoped_refptr<MockWorkerThread> worker_thread_;
 
   int max_wait_ms_;
+  int num_fetchers_created_;
 };
 
 class FetcherClient {
@@ -339,11 +371,6 @@ public:
     ASSERT_EQ(ERR_IO_PENDING, result);
   }
 
-  void RunImmediateReturnTest() {
-    int result = fetcher_.Fetch(&pac_text_, &completion_callback_);
-    ASSERT_EQ(ERR_PAC_NOT_IN_DHCP, result);
-  }
-
   void RunMessageLoopUntilComplete() {
     while (!finished_) {
       MessageLoop::current()->RunAllPending();
@@ -351,6 +378,14 @@ public:
     MessageLoop::current()->RunAllPending();
   }
 
+  void RunMessageLoopUntilWorkerDone() {
+    DCHECK(fetcher_.worker_thread_.get());
+    while (!fetcher_.worker_thread_->worker_finished_event_.TimedWait(
+        base::TimeDelta::FromMilliseconds(10))) {
+      MessageLoop::current()->RunAllPending();
+    }
+  }
+
   void OnCompletion(int result) {
     finished_ = true;
     result_ = result;
@@ -477,10 +512,11 @@ TEST(DhcpProxyScriptFetcherWin, FailureCaseNoURLConfigured) {
 }
 
 void TestFailureCaseNoDhcpAdapters(FetcherClient* client) {
-  client->RunImmediateReturnTest();
-  // In case there are any pending messages that get us in a bad state
-  // (there shouldn't be).
-  MessageLoop::current()->RunAllPending();
+  client->RunTest();
+  client->RunMessageLoopUntilComplete();
+  ASSERT_EQ(ERR_PAC_NOT_IN_DHCP, client->result_);
+  ASSERT_EQ(L"", client->pac_text_);
+  ASSERT_EQ(0, client->fetcher_.num_fetchers_created_);
 }
 
 TEST(DhcpProxyScriptFetcherWin, FailureCaseNoDhcpAdapters) {
@@ -522,6 +558,24 @@ TEST(DhcpProxyScriptFetcherWin, ShortCircuitLessPreferredAdapters) {
   TestShortCircuitLessPreferredAdapters(&client);
 }
 
+void TestImmediateCancel(FetcherClient* client) {
+  scoped_ptr<DummyDhcpProxyScriptAdapterFetcher> adapter_fetcher(
+      new DummyDhcpProxyScriptAdapterFetcher());
+  adapter_fetcher->Configure(true, OK, L"bingo", 1);
+  client->fetcher_.PushBackAdapter("a", adapter_fetcher.release());
+  client->RunTest();
+  client->fetcher_.Cancel();
+  client->RunMessageLoopUntilWorkerDone();
+  ASSERT_EQ(0, client->fetcher_.num_fetchers_created_);
+}
+
+// Regression test to check that when we cancel immediately, no
+// adapter fetchers get created.
+TEST(DhcpProxyScriptFetcherWin, ImmediateCancel) {
+  FetcherClient client;
+  TestImmediateCancel(&client);
+}
+
 TEST(DhcpProxyScriptFetcherWin, ReuseFetcher) {
   FetcherClient client;
 
@@ -542,6 +596,7 @@ TEST(DhcpProxyScriptFetcherWin, ReuseFetcher) {
   test_functions.push_back(TestFailureCaseNoURLConfigured);
   test_functions.push_back(TestFailureCaseNoDhcpAdapters);
   test_functions.push_back(TestShortCircuitLessPreferredAdapters);
+  test_functions.push_back(TestImmediateCancel);
 
   std::random_shuffle(test_functions.begin(),
                       test_functions.end(),