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

net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc

+// 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.
+
+#include "net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
+
+#include "base/message_loop_proxy.h"
+#include "base/sys_string_conversions.h"
+#include "base/task.h"
+#include "base/threading/worker_pool.h"
+#include "base/time.h"
+#include "net/base/net_errors.h"
+#include "net/proxy/dhcpcsvc_init_win.h"
+
+#include <windows.h>
+#include <winsock2.h>
+#include <dhcpcsdk.h>
+#pragma comment(lib, "dhcpcsvc.lib")
+
+namespace {
+
+// Maximum amount of time to wait for response from the Win32 DHCP API.
+const int kTimeoutMs = 2000;

[eroman, 2011/04/21 05:22:48]

What is the rationale for 2seconds?

[Jói, 2011/04/21 15:57:38]

This is mostly a guess.  DHCP should normally be very quick; it's (almost?)
always on the LAN, so roundtrip times are a couple of milliseconds or less and
there are something like two roundtrips for an initial query.  DHCP servers are
probably usually fairly fast.  I could make it longer but that would, in certain
unlikely edge-cases, increase the time we block on attempting lookup of the PAC
URL from DHCP before trying DNS.

If/when we switch to doing DHCP and DNS in parallel, this won't be so much of an
issue - we'll cancel the DHCP queries after some fairly short timeout if DNS
comes in first.

+
+}  // namespace
+
+namespace net {
+
+DhcpProxyScriptAdapterFetcher::DhcpProxyScriptAdapterFetcher(
+    URLRequestContext* url_request_context)
+    : state_(STATE_START),
+      result_(ERR_IO_PENDING),
+      callback_(NULL),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          script_fetcher_callback_(
+              this, &DhcpProxyScriptAdapterFetcher::OnFetcherDone)),
+      origin_loop_(base::MessageLoopProxy::CreateForCurrentThread()),
+      url_request_context_(url_request_context) {
+}
+
+DhcpProxyScriptAdapterFetcher::~DhcpProxyScriptAdapterFetcher() {
+  Cancel();
+}
+
+void DhcpProxyScriptAdapterFetcher::Fetch(
+    const std::string& adapter_name, CompletionCallback* callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(state_ == STATE_START);

[eroman, 2011/04/21 05:22:48]

nit: DCHECK_EQ()

[Jói, 2011/05/03 21:20:59]

Done.

+  result_ = ERR_IO_PENDING;
+  pac_script_ = string16();
+  state_ = STATE_WAIT_DHCP;
+  callback_ = callback;
+
+  wait_timer_.Start(base::TimeDelta::FromMilliseconds(ImplGetTimeoutMs()),
+                    this, &DhcpProxyScriptAdapterFetcher::OnTimeout);
+  bool succeeded = base::WorkerPool::PostTask(
+      FROM_HERE,
+      NewRunnableMethod(
+          this,
+          &DhcpProxyScriptAdapterFetcher::QueryDhcpOnWorkerThread,
+          adapter_name),
+      true);
+  DCHECK(succeeded);
+}
+
+void DhcpProxyScriptAdapterFetcher::Cancel() {
+  DCHECK(CalledOnValidThread());
+  callback_ = NULL;
+  if (script_fetcher_.get()) {
+    script_fetcher_.reset();
+  }
+
+  switch (state_) {
+    case STATE_WAIT_DHCP:
+      // Nothing to do here, we let the worker thread run to completion,
+      // the task it posts back when it completes will check the state.
+      break;
+    case STATE_WAIT_URL:
+      break;
+    case STATE_START:
+    case STATE_FINISH:
+    case STATE_CANCEL:
+      break;
+  }
+
+  if (state_ != STATE_FINISH) {
+    result_ = ERR_ABORTED;
+    state_ = STATE_CANCEL;
+  }
+}
+
+bool DhcpProxyScriptAdapterFetcher::DidFinish() const {
+  DCHECK(CalledOnValidThread());
+  return state_ == STATE_FINISH;
+}
+
+int DhcpProxyScriptAdapterFetcher::result() const {
+  DCHECK(CalledOnValidThread());
+  return result_;
+}
+
+string16 DhcpProxyScriptAdapterFetcher::pac_script() const {
+  DCHECK(CalledOnValidThread());
+  return pac_script_;
+}
+
+GURL DhcpProxyScriptAdapterFetcher::pac_url() const {
+  DCHECK(CalledOnValidThread());
+  return pac_url_;
+}
+
+void DhcpProxyScriptAdapterFetcher::QueryDhcpOnWorkerThread(
+    const std::string& adapter_name) {
+  std::string url = ImplGetPacURLFromDhcp(adapter_name);
+
+  bool succeeded = origin_loop_->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this,
+                        &DhcpProxyScriptAdapterFetcher::OnQueryDhcpDone,
+                        url));
+  DCHECK(succeeded);
+}
+
+void DhcpProxyScriptAdapterFetcher::OnQueryDhcpDone(
+    std::string url) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(state_ == STATE_CANCEL || state_ == STATE_WAIT_DHCP);
+  if (state_ == STATE_CANCEL)
+    return;
+
+  wait_timer_.Stop();
+
+  if (url.empty()) {
+    result_ = ERR_PAC_NOT_IN_DHCP;
+    TransitionToFinish();
+  } else {
+    state_ = STATE_WAIT_URL;
+    pac_url_ = GURL(url);

[eroman, 2011/04/21 05:22:48]

One possibility to consider is whether the URL string parsed to a valid URL.

It probably doesn't matter, since if you call script_fetcher_ with an invalid
URL it will probably fail anyway.

Shrug. If you wanted to check for this, there is an is_valid() or something on
GURL.

[Jói, 2011/05/03 21:20:59]

I now have
  pac_url_ = GURL(url);
  if (pac_url_.is_empty() || !pac_url_.is_valid()) {
where previously there was
  if (url.empty())

+    script_fetcher_.reset(ImplCreateScriptFetcher(pac_url_));
+    script_fetcher_->Fetch(&pac_script_, &script_fetcher_callback_);
+  }
+}
+
+void DhcpProxyScriptAdapterFetcher::OnTimeout() {
+  DCHECK(state_ == STATE_WAIT_DHCP);
+  result_ = ERR_TIMED_OUT;
+  TransitionToFinish();
+}
+
+void DhcpProxyScriptAdapterFetcher::OnFetcherDone(int result) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(state_ == STATE_WAIT_URL || state_ == STATE_CANCEL);
+  if (state_ == STATE_CANCEL)
+    return;
+
+  // At this point, pac_script_ has already been written to.
+  script_fetcher_.reset();
+  result_ = result;
+  TransitionToFinish();
+}
+
+void DhcpProxyScriptAdapterFetcher::TransitionToFinish() {
+  DCHECK(state_ == STATE_WAIT_DHCP || state_ == STATE_WAIT_URL);
+  state_ = STATE_FINISH;
+  callback_->Run(result_);
+  callback_ = NULL;
+}
+
+std::string DhcpProxyScriptAdapterFetcher::ImplGetPacURLFromDhcp(
+    const std::string& adapter_name) {
+  return GetPacURLFromDhcp(adapter_name);
+}
+
+ProxyScriptFetcher* DhcpProxyScriptAdapterFetcher::ImplCreateScriptFetcher(
+    const GURL& url) {
+  URLProxyScriptFetcher* fetcher =
+      new ProxyScriptFetcherImpl(url_request_context_);

[eroman, 2011/04/21 05:22:48]

This isn't really desirable to be instantiating specific implementations of
URLProxyScriptFetcher here, whereas the other consumer (InitProxyResolver) is
passed in the dependency. It would be better to pass some URLProxyScriptFetcher
factory in as a dependency, so the caller can decide how to do the fetching.

[Jói, 2011/04/21 15:57:38]

I don't see the benefit of that if we are only trying to achieve testability,
since there is only the one URL-fetching implementation of ProxyScriptFetcher,
and that seems unlikely to change.  However, this is one of those things I will
change if in our discussion we conclude that dependency injection must be used
as the method of introducing testability.

+  fetcher->SetURL(url);
+  return fetcher;
+}
+
+int DhcpProxyScriptAdapterFetcher::ImplGetTimeoutMs() const {

[eroman, 2011/04/21 05:22:48]

I don't much care for the use of virtuals. I would prefer dependency injection
pattern. See my meta comment for alternatives.

[Jói, 2011/05/03 21:20:59]

As discussed, I changed InitProxyResolver and its tests and users to use
dependency injection for the DHCP WPAD stuff, but am leaving the DHCP classes as
is.

+  return kTimeoutMs;
+}
+
+std::string DhcpProxyScriptAdapterFetcher::GetPacURLFromDhcp(
+    const std::string& adapter_name) {
+  EnsureDhcpcsvcInit();
+
+  std::wstring adapter_name_wide = base::SysMultiByteToWide(adapter_name,
+                                                            CP_ACP);
+
+  DHCPCAPI_PARAMS_ARRAY send_params = { 0, NULL };
+
+  BYTE option_data[] = { 1, 252 };
+  DHCPCAPI_PARAMS wpad_params = { 0 };
+  wpad_params.OptionId = 252;
+  wpad_params.IsVendor = FALSE;  // Surprising, but intentional.
+
+  DHCPCAPI_PARAMS_ARRAY request_params = { 0 };
+  request_params.nParams = 1;
+  request_params.Params = &wpad_params;
+
+  // The maximum message size is typically 4096 bytes on Windows per
+  // http://support.microsoft.com/kb/321592
+  DWORD result_buffer_size = 4096;
+  scoped_ptr_malloc<BYTE> result_buffer;
+  int retry_count = 0;
+  DWORD res = NO_ERROR;
+  do {
+    result_buffer.reset(reinterpret_cast<BYTE*>(malloc(result_buffer_size)));
+
+    // Note that while the DHCPCAPI_REQUEST_SYNCHRONOUS flag seems to indicate
+    // there might be an asynchronous mode, there seems to be (at least in
+    // terms of well-documented use of this API) only a synchronous mode, with
+    // an optional "async notifications later if the option changes" mode.
+    // Even IE9, which we hope to emulate as IE is the most widely deployed
+    // previous implementation of the DHCP aspect of WPAD and the only one
+    // on Windows (Konqueror is the other, on Linux), uses this API with the
+    // synchronous flag.  There seem to be several Microsoft Knowledge Base
+    // articles about calls to this function failing when other flags are used
+    // (e.g. http://support.microsoft.com/kb/885270) so we won't take any
+    // chances on non-standard, poorly documented usage.

[eroman, 2011/04/21 05:22:48]

Nice comment block!

[Jói, 2011/05/03 21:20:59]

thx

+    res = ::DhcpRequestParams(DHCPCAPI_REQUEST_SYNCHRONOUS,
+                              NULL,
+                              const_cast<LPWSTR>(adapter_name_wide.c_str()),
+                              NULL,
+                              send_params, request_params,
+                              result_buffer.get(), &result_buffer_size,
+                              NULL);
+    ++retry_count;
+  } while (res == ERROR_MORE_DATA && retry_count <= 3);
+
+  if (res != NO_ERROR) {
+    NOTREACHED();

[eroman, 2011/04/21 05:22:48]

nit: I suggest adding return "" here, and removing the "else" below.

[Jói, 2011/05/03 21:20:59]

It's an else if; there can be NO_ERROR and wpad_params.nBytesData can be 0.

+  } else if (wpad_params.nBytesData) {
+    // The result should be ASCII, not wide character.
+    DCHECK(strlen(reinterpret_cast<const char*>(wpad_params.Data)) + 1 ==

[eroman, 2011/04/21 05:22:48]

nit: DCHECK_EQ()

[Jói, 2011/05/03 21:20:59]

Done.

+           wpad_params.nBytesData);
+    return std::string(reinterpret_cast<const char *>(wpad_params.Data));
+  }
+
+  return "";
+}
+
+}  // namespace net