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

net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.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/perftimer.h"
+#include "base/timer.h"
+#include "net/base/net_errors.h"
+#include "net/proxy/mock_proxy_script_fetcher.h"
+#include "net/proxy/proxy_script_fetcher_impl.h"
+#include "net/test/test_server.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+const char* const kPacUrl = "http://pacserver/script.pac";
+
+// In net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc there are a few
+// tests that exercise DhcpProxyScriptAdapterFetcher end-to-end along with
+// WindowsDhcpProxyScriptFetcher, i.e. it tests the end-to-end usage of Win32
+// APIs and the network.  In this file we test only by stubbing out
+// functionality.
+
+// Version of DhcpProxyScriptAdapterFetcher that mocks out dependencies
+// to allow unit testing.
+class MockDhcpProxyScriptAdapterFetcher : public DhcpProxyScriptAdapterFetcher {
+ public:
+  explicit MockDhcpProxyScriptAdapterFetcher(URLRequestContext* context)
+      : DhcpProxyScriptAdapterFetcher(context),
+        test_finished_(false),
+        dhcp_delay_ms_(1),
+        timeout_ms_(100),
+        configured_url_(kPacUrl),
+        fetcher_delay_ms_(1),
+        fetcher_result_(OK),
+        pac_script_("bingo") {
+  }
+
+  void Cancel() {
+    DhcpProxyScriptAdapterFetcher::Cancel();
+    fetcher_ = NULL;
+  }
+
+  std::string ImplGetPacURLFromDhcp(const std::string& adapter_name) OVERRIDE {
+    PerfTimer timer;
+    while (timer.Elapsed() < TimeDelta::FromMilliseconds(dhcp_delay_ms_) &&
+           !test_finished_) {
+      base::PlatformThread::Sleep(5);

[eroman, 2011/05/13 05:03:32]

I don't much like this sleep loop, or how it tests a (non-volatile) bool. Can't
the compiler decide to not re-read that variable?

[Jói, 2011/05/13 20:19:09]

Marked it volatile just in case.

+    }
+    return configured_url_;
+  }
+
+  virtual ProxyScriptFetcher* ImplCreateScriptFetcher() OVERRIDE {
+    // We don't maintain ownership of the fetcher, it is transferred to
+    // the caller.
+    fetcher_ = new MockProxyScriptFetcher();
+    if (fetcher_delay_ms_ != -1) {
+      fetcher_timer_.Start(
+          base::TimeDelta::FromMilliseconds(fetcher_delay_ms_),

[eroman, 2011/05/13 05:03:32]

So the assumption being made here is that the caller will call Start() before we
re-run the message loop?

Since if they don't we may reach NotifyFetchCompletion() before even having
received a request in it!

I think it will be simpler to simply define your own ProxyScriptFetcher that
does this delay internally.

[Jói, 2011/05/13 20:19:09]

Added this comment to OnFetcherTimer, and extended the DCHECK so that it would
hit in the case you described (it does not occur right now):

    // Note that there is an assumption by this mock implementation that
    // DhcpProxyScriptAdapterFetcher::Fetch will call ImplCreateScriptFetcher
    // and call Fetch on the fetcher before the message loop is re-entered.
    // This holds true today, but if you hit this DCHECK the problem can
    // possibly be resolved by having a separate subclass of
    // MockProxyScriptFetcher that adds the delay internally (instead of
    // the simple approach currently used in ImplCreateScriptFetcher above).
    DCHECK(fetcher_ && fetcher_->has_pending_request());

+          this, &MockDhcpProxyScriptAdapterFetcher::OnFetcherTimer);
+    }
+    return fetcher_;
+  }
+
+  // Use a shorter timeout so tests can finish more quickly.
+  virtual int ImplGetTimeoutMs() const OVERRIDE {
+    return timeout_ms_;
+  }
+
+  void OnFetcherTimer() {
+    DCHECK(fetcher_);
+    fetcher_->NotifyFetchCompletion(fetcher_result_, pac_script_);
+    fetcher_ = NULL;
+  }
+
+  bool IsWaitingForFetcher() const {
+    return state_ == STATE_WAIT_URL;
+  }
+
+  bool WasCancelled() const {
+    return state_ == STATE_CANCEL;
+  }
+
+  bool test_finished_;  // Set to true to end delay loop.
+  int dhcp_delay_ms_;
+  int timeout_ms_;
+  std::string configured_url_;
+  int fetcher_delay_ms_;
+  int fetcher_result_;
+  std::string pac_script_;
+  MockProxyScriptFetcher* fetcher_;
+  base::OneShotTimer<MockDhcpProxyScriptAdapterFetcher> fetcher_timer_;
+};
+
+class FetcherClient {
+ public:
+  FetcherClient()
+      : finished_(false),
+        url_request_context_(new TestURLRequestContext()),
+        fetcher_(
+            new MockDhcpProxyScriptAdapterFetcher(url_request_context_.get())),
+        ALLOW_THIS_IN_INITIALIZER_LIST(
+            completion_callback_(this, &FetcherClient::OnCompletion)) {
+  }
+
+  void RunTest() {
+    fetcher_->Fetch("adapter name", &completion_callback_);
+  }
+
+  void RunMessageLoopUntilComplete() {
+    while (!finished_) {

[eroman, 2011/05/13 05:03:32]

This is the pattern we use for net unit tests:

TestCompletionCallback callback;
fetcher_->Fetch("adapter name", &callback);
EXPECT_EQ(ERR_OK, callback.WaitUntilDone());

the TestCompletionCallback::WaitUntilDone() will automatically spin the message
loop until the callback is called.

Can you use that model here?

[Jói, 2011/05/13 20:19:09]

Done.

+      MessageLoop::current()->RunAllPending();
+    }
+    MessageLoop::current()->RunAllPending();
+  }
+
+  void OnCompletion(int result) {
+    finished_ = true;
+  }
+
+  void FinishTestAllowCleanup() {
+    fetcher_->test_finished_ = true;
+    base::PlatformThread::Sleep(15);
+    MessageLoop::current()->RunAllPending();
+  }
+
+  bool finished_;
+  scoped_refptr<URLRequestContext> url_request_context_;
+  scoped_refptr<MockDhcpProxyScriptAdapterFetcher> fetcher_;
+  string16 pac_text_;
+  CompletionCallbackImpl<FetcherClient> completion_callback_;
+};
+
+TEST(DhcpProxyScriptAdapterFetcher, NormalCaseURLNotInDhcp) {
+  FetcherClient client;
+  client.fetcher_->configured_url_ = "";
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(ERR_PAC_NOT_IN_DHCP, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->GetPacScript());
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, NormalCaseURLInDhcp) {
+  FetcherClient client;
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(OK, client.fetcher_->result());
+  EXPECT_EQ(string16(L"bingo"), client.fetcher_->GetPacScript());
+  EXPECT_EQ(GURL(kPacUrl), client.fetcher_->GetPacURL());
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, TimeoutDuringDhcp) {
+  // Does a Fetch() with a long enough delay on accessing DHCP that the
+  // fetcher should time out.  This is to test a case manual testing found,
+  // where under certain circumstances (e.g. adapter enabled for DHCP and
+  // needs to retrieve its configuration from DHCP, but no DHCP server
+  // present on the network) accessing DHCP can take on the order of tens
+  // of seconds.
+  FetcherClient client;
+  client.fetcher_->dhcp_delay_ms_ = 20 * 1000;
+  client.fetcher_->timeout_ms_ = 25;
+
+  PerfTimer timer;
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+
+  // The timeout should occur within about 25 ms, way before the 20s set as
+  // the API delay above.
+  ASSERT_GT(base::TimeDelta::FromMilliseconds(35), timer.Elapsed());
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(ERR_TIMED_OUT, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->GetPacScript());
+  EXPECT_EQ(GURL(), client.fetcher_->GetPacURL());
+  client.FinishTestAllowCleanup();
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, CancelWhileDhcp) {
+  FetcherClient client;
+  client.fetcher_->dhcp_delay_ms_ = 10;
+  client.RunTest();
+  client.fetcher_->Cancel();
+  MessageLoop::current()->RunAllPending();
+  ASSERT_FALSE(client.fetcher_->DidFinish());
+  ASSERT_TRUE(client.fetcher_->WasCancelled());
+  EXPECT_EQ(ERR_ABORTED, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->GetPacScript());
+  EXPECT_EQ(GURL(), client.fetcher_->GetPacURL());
+  client.FinishTestAllowCleanup();
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, CancelWhileFetcher) {
+  FetcherClient client;
+  // This causes the mock fetcher not to pretend the
+  // fetcher finishes after a timeout.
+  client.fetcher_->fetcher_delay_ms_ = -1;
+  client.RunTest();
+  int max_loops = 4;
+  while (!client.fetcher_->IsWaitingForFetcher() && max_loops--) {
+    base::PlatformThread::Sleep(10);
+    MessageLoop::current()->RunAllPending();
+  }
+  client.fetcher_->Cancel();
+  MessageLoop::current()->RunAllPending();
+  ASSERT_FALSE(client.fetcher_->DidFinish());
+  ASSERT_TRUE(client.fetcher_->WasCancelled());
+  EXPECT_EQ(ERR_ABORTED, client.fetcher_->result());
+  EXPECT_EQ(string16(L""), client.fetcher_->GetPacScript());
+  // GetPacURL() still returns the URL fetched in this case.
+  EXPECT_EQ(GURL(kPacUrl), client.fetcher_->GetPacURL());
+  client.FinishTestAllowCleanup();
+}
+
+TEST(DhcpProxyScriptAdapterFetcher, CancelAtCompletion) {
+  FetcherClient client;
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  client.fetcher_->Cancel();
+  // Canceling after you're done should have no effect, so these
+  // are identical expectations to the NormalCaseURLInDhcp test.
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(OK, client.fetcher_->result());
+  EXPECT_EQ(string16(L"bingo"), client.fetcher_->GetPacScript());
+  EXPECT_EQ(GURL(kPacUrl), client.fetcher_->GetPacURL());
+  client.FinishTestAllowCleanup();
+}
+
+// Does a real fetch on a mock DHCP configuration.
+class MockDhcpRealFetchProxyScriptAdapterFetcher
+    : public MockDhcpProxyScriptAdapterFetcher {
+ public:
+  explicit MockDhcpRealFetchProxyScriptAdapterFetcher(
+      URLRequestContext* context)
+      : MockDhcpProxyScriptAdapterFetcher(context),
+        url_request_context_(context) {
+  }
+
+  // Returns a real proxy script fetcher.
+  ProxyScriptFetcher* ImplCreateScriptFetcher() OVERRIDE {
+    ProxyScriptFetcher* fetcher =
+        new ProxyScriptFetcherImpl(url_request_context_);
+    return fetcher;
+  }
+
+  URLRequestContext* url_request_context_;
+};
+
+TEST(DhcpProxyScriptAdapterFetcher, MockDhcpRealFetch) {
+  TestServer test_server(
+      TestServer::TYPE_HTTP,
+      FilePath(FILE_PATH_LITERAL("net/data/proxy_script_fetcher_unittest")));
+  ASSERT_TRUE(test_server.Start());
+
+  GURL configured_url = test_server.GetURL("files/downloadable.pac");
+
+  FetcherClient client;
+  scoped_refptr<URLRequestContext> url_request_context(
+      new TestURLRequestContext());
+  client.fetcher_ =
+      new MockDhcpRealFetchProxyScriptAdapterFetcher(url_request_context.get());
+  client.fetcher_->configured_url_ = configured_url.spec();
+  client.RunTest();
+  client.RunMessageLoopUntilComplete();
+  ASSERT_TRUE(client.fetcher_->DidFinish());
+  EXPECT_EQ(OK, client.fetcher_->result());
+  EXPECT_EQ(string16(L"-downloadable.pac-\n"), client.fetcher_->GetPacScript());
+  EXPECT_EQ(configured_url,
+            client.fetcher_->GetPacURL());
+}
+
+}  // namespace
+}  // namespace net

[eroman, 2011/05/13 05:03:32]

nit: add a newline between these two braces to match the opening style.

[Jói, 2011/05/13 20:19:09]

Done.