Adds support for the DHCP portion of the WPAD (proxy auto-discovery) protocol.

net/proxy/dhcp_proxy_script_adapter_fetcher_win.h

+// 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.
+
+#ifndef NET_PROXY_DHCP_SCRIPT_ADAPTER_FETCHER_WIN_H_
+#define NET_PROXY_DHCP_SCRIPT_ADAPTER_FETCHER_WIN_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/string16.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/timer.h"
+#include "net/base/completion_callback.h"
+#include "googleurl/src/gurl.h"
+
+namespace base {
+class MessageLoopProxy;
+}
+
+namespace net {
+
+class ProxyScriptFetcher;
+class URLRequestContext;
+
+// For a given adapter, this class takes care of first doing a DHCP lookup
+// to get the PAC URL, then if there is one, trying to fetch it.
+//
+// Non thread-safe for clients, but does some internal multi-threading.
+class DhcpProxyScriptAdapterFetcher
+    : public base::SupportsWeakPtr<DhcpProxyScriptAdapterFetcher>,
+      public base::NonThreadSafe {
+ public:
+  // |url_request_context| must outlive DhcpProxyScriptAdapterFetcher.
+  explicit DhcpProxyScriptAdapterFetcher(
+      URLRequestContext* url_request_context);
+  virtual ~DhcpProxyScriptAdapterFetcher();
+
+  // Starts a fetch.  On completion (but not cancellation), |callback|
+  // will be invoked with the network error indicating success or failure
+  // of fetching a DHCP-configured PAC file on this adapter.
+  //
+  // You may only call Fetch() once on a given instance of
+  // DhcpProxyScriptAdapterFetcher.

[eroman, 2011/05/17 04:39:39]

could also mention that on completion result is obtained from GetPacScript() /
GetPacURL()

[Jói, 2011/05/17 15:38:35]

Done.

+  virtual void Fetch(const std::string& adapter_name,
+                     CompletionCallback* callback);
+
+  // Cancels the fetch on this adapter.
+  virtual void Cancel();
+
+  // Returns true if in the FINISH state (not CANCEL).
+  virtual bool DidFinish() const;
+
+  // Returns the network error indicating the result of the fetch. Will
+  // return IO_PENDING until the fetch is complete or cancelled.
+  virtual int GetResult() const;

[eroman, 2011/05/17 04:39:39]

Is this not the same as the integer passed to the completion callback?

[Jói, 2011/05/17 15:38:35]

It is. The implementation of going through the fetchers to find which ones are
done, have an error etc. is a lot simpler by leaving the state accessible there.
I've updated the comment as follows:

  // Returns the network error indicating the result of the fetch. Will
  // return IO_PENDING until the fetch is complete or cancelled. This is
  // the same network error passed to the |callback| provided to |Fetch()|.

+
+  // Returns the contents of the PAC file retrieved.  Only valid if
+  // |IsComplete()| is true.  Returns the empty string if |GetResult()|
+  // returns anything other than OK.
+  virtual string16 GetPacScript() const;
+
+  // Returns the PAC URL retrieved from DHCP.  Only guaranteed to be
+  // valid if |IsComplete()| is true.  Returns an empty URL if no URL was
+  // configured in DHCP.  May return a valid URL even if |result()| does
+  // not return OK (this would indicate that we found a URL configured in
+  // DHCP but failed to download it).
+  virtual GURL GetPacURL() const;
+
+  // Returns the PAC URL configured in DHCP for the given |adapter_name|, or
+  // the empty string if none is configured.
+  //
+  // This function executes synchronously due to limitations of the Windows
+  // DHCP client API.
+  static std::string GetPacURLFromDhcp(const std::string& adapter_name);
+
+ protected:
+  // This inner class is used to encapsulate the worker thread, which has
+  // 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.
+  class WorkerThread : public base::RefCountedThreadSafe<WorkerThread> {

[eroman, 2011/05/17 04:39:39]

(optional) WorkerThread sounds like it should be a base::Thread object. Another
naming idea would be "Job".

[Jói, 2011/05/17 15:38:35]

I'll leave it as is since it encapsulates our use of the worker thread pool. 
Agreed the naming is not totally ideal but I'm not sure "Job" captures it better
(that sounds like something you might post to a thread e.g. like a Task).

+   public:
+    // Creates and initializes (but does not start) the worker thread.
+    explicit WorkerThread(

[eroman, 2011/05/17 04:39:39]

Please also add a virtual destructor to prevent accidental leaks in derived
classes.

[Jói, 2011/05/17 15:38:35]

Doh, I have to stop forgetting this.  Thanks for catching it.

+        const base::WeakPtr<DhcpProxyScriptAdapterFetcher>& owner);
+
+    // Starts the worker thread, fetching information for |adapter_name| using
+    // |get_pac_from_url_func|.
+    void Start(const std::string& adapter_name);
+
+    // This is the method that runs on the worker thread.
+    void ThreadFunc(const std::string& adapter_name);

[eroman, 2011/05/17 04:39:39]

can these two methods be private?

[Jói, 2011/05/17 15:38:35]

Done.

+
+    // Callback for the above; this executes back on the main thread,
+    // not the worker thread.
+    void OnThreadDone(const std::string& url);
+
+   protected:
+    // Unit testing seam.

[eroman, 2011/05/17 04:39:39]

what is a "seam"?

[Jói, 2011/05/17 15:38:35]

Sorry, I thought it was common terminology; it's used extensively in a book I
like, "Working Effectively with Legacy Code" that is all about adding tests to
existing code.  I've changed the comment to say "Virtual method introduced to
allow unit testing."

+    virtual std::string ImplGetPacURLFromDhcp(const std::string& adapter_name);
+
+   private:
+    // All work except QueryDHcpOnWorkerThread and destruction should

[eroman, 2011/05/17 04:39:39]

ThreadFunc?

[Jói, 2011/05/17 15:38:35]

Done.

+    // 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<DhcpProxyScriptAdapterFetcher> owner_;

[eroman, 2011/05/17 04:39:39]

I like your general approach of keeping the parent class non-threadsafe.

However I am not sure if it is safe to use WeakPtr<> across threads. I *think*
it is OK since it looks like the only operation which might ever run off-thread
in this case is ~WeakPtr. However since the header has a caveat saying it is not
threadsafe, I would like to ask for confirmation.

[Jói, 2011/05/17 15:38:35]

Thanks.  The weak_ptr.h header file has a comment on the WeakReference class
that says:

  // While Flag is bound to a specific thread, it may be deleted from another
  // via base::WeakPtr::~WeakPtr().

However, looking at the implementation, it looks like you need to call
SupportsWeakPtr<>::DetachFromThread() for this to be true. I've added this to
DhcpProxyScriptAdapterFetcher's destructor.

+
+    // 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 this object is in state

[eroman, 2011/05/17 04:39:39]

nit: not sure I understand this comment. CANCEL isn't a concept in WorkerThread,
all it knows if whether owner_ has already been destroyed.

[Jói, 2011/05/17 15:38:35]

Copy/paste error.  Updated the comment to:

    // 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.

+    // CANCEL and no further processing should be done.
+    scoped_refptr<base::MessageLoopProxy> origin_loop_;
+
+    DISALLOW_COPY_AND_ASSIGN(WorkerThread);
+  };
+
+  // Event/state transition handlers
+  void OnQueryDhcpDone(const std::string& url);
+  void OnTimeout();
+  void OnFetcherDone(int result);
+  void TransitionToFinish();
+
+  // Virtual methods introduced to allow unit testing.
+  virtual ProxyScriptFetcher* ImplCreateScriptFetcher();
+  virtual WorkerThread* ImplCreateWorkerThread(
+      const base::WeakPtr<DhcpProxyScriptAdapterFetcher>& owner);
+  virtual base::TimeDelta ImplGetTimeout() const;
+
+  // This is the state machine for fetching from a given adapter.
+  //
+  // The state machine goes from START->WAIT_DHCP when it starts
+  // a worker thread to fetch the PAC URL from DHCP.
+  //
+  // In state WAIT_DHCP, if the DHCP query finishes and has no URL, it
+  // moves to state FINISH.  If there is a URL, it starts a
+  // ProxyScriptFetcher to fetch it and moves to state WAIT_URL.
+  //
+  // It goes from WAIT_URL->FINISH when the ProxyScriptFetcher completes.
+  //
+  // In state FINISH, completion is indicated to the outer class, with
+  // the results of the fetch if a PAC script was successfully fetched.
+  //
+  // In state WAIT_DHCP, our timeout occurring can push us to FINISH.
+  //
+  // In any state except FINISH, a call to Cancel() will move to state
+  // CANCEL and cause all outstanding work to be cancelled or its
+  // results ignored when available.
+  enum State {
+    STATE_START,
+    STATE_WAIT_DHCP,
+    STATE_WAIT_URL,
+    STATE_FINISH,
+    STATE_CANCEL,
+  };
+
+  // Current state of this state machine.
+  State state_;
+
+  // A network error indicating result of operation.
+  int result_;
+
+  // Empty string or the PAC script downloaded.
+  string16 pac_script_;
+
+  // Empty URL or the PAC URL configured in DHCP.
+  GURL pac_url_;
+
+  // Callback to let our client know we're done. Invalid in states
+  // START, FINISH and CANCEL.
+  CompletionCallback* callback_;
+
+  // Container for our worker thread. NULL if not currently running.
+  scoped_refptr<WorkerThread> worker_thread_;
+
+  // Fetcher to retrieve PAC files once URL is known.
+  scoped_ptr<ProxyScriptFetcher> script_fetcher_;
+
+  // Callback from the script fetcher.
+  CompletionCallbackImpl<DhcpProxyScriptAdapterFetcher>
+      script_fetcher_callback_;
+
+  // Implements a timeout on the call to the Win32 DHCP API.
+  base::OneShotTimer<DhcpProxyScriptAdapterFetcher> wait_timer_;
+
+  scoped_refptr<URLRequestContext> url_request_context_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(DhcpProxyScriptAdapterFetcher);
+};
+
+}  // namespace net
+
+#endif  // NET_PROXY_DHCP_SCRIPT_ADAPTER_FETCHER_WIN_H_