chromeos: Simplify D-Bus proxy provider (but not tests).

chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc

 // Copyright (c) 2012 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 "chromeos/dbus/services/proxy_resolution_service_provider.h"
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/dbus/services/service_provider_test_helper.h"
 #include "dbus/message.h"
 #include "net/base/net_errors.h"
+#include "net/proxy/mock_proxy_resolver.h"
+#include "net/proxy/proxy_config_service_fixed.h"
+#include "net/proxy/proxy_info.h"
+#include "net/proxy/proxy_resolver.h"
+#include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_test_util.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 namespace {
 
 // ProxyResolutionServiceProvider will return the proxy info as a D-Bus
 // signal, to the following signal interface and the signal name.
 const char kReturnSignalInterface[] = "org.chromium.TestInterface";
 const char kReturnSignalName[] = "TestSignal";
 
+// Trivial net::ProxyResolver implementation that returns canned data either
+// synchronously or asynchronously.
+class TestProxyResolver : public net::ProxyResolver {
+ public:
+  explicit TestProxyResolver(
+      scoped_refptr<base::SingleThreadTaskRunner> network_task_runner)
+      : network_task_runner_(network_task_runner) {
+    proxy_info_.UseDirect();
+  }
+  ~TestProxyResolver() override = default;
+
+  const net::ProxyInfo& proxy_info() const { return proxy_info_; }
+  net::ProxyInfo* mutable_proxy_info() { return &proxy_info_; }
+
+  void set_error(net::Error error) { error_ = error; }
+  void set_async(bool async) { async_ = async; }
+
+  // net::ProxyResolver:
+  int GetProxyForURL(const GURL& url,
+                     net::ProxyInfo* results,
+                     const net::CompletionCallback& callback,
+                     std::unique_ptr<Request>* request,
+                     const net::NetLogWithSource& net_log) override {
+    CHECK(network_task_runner_->BelongsToCurrentThread());
+    results->Use(proxy_info_);

[James Cook, 2017/03/31 17:33:10]

Should this only happen in the non-async case? I'm unclear on the contract for
GetProxyForURL().

[Daniel Erat, 2017/03/31 17:46:37]

nope, this is how the results are returned for async queries too.
net::CompletionCallback is just base::Callback<void(int)>.

+    if (!async_)
+      return error_;
+
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, error_));
+    return net::ERR_IO_PENDING;
+  }
+
+ private:
+  scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
+
+  // Final result for GetProxyForURL() to return.
+  net::Error error_ = net::OK;
+
+  // Proxy info for GetProxyForURL() to return.
+  net::ProxyInfo proxy_info_;
+
+  // If true, GetProxyForURL() replies asynchronously rather than synchronously.
+  bool async_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(TestProxyResolver);
+};
+
+// Trivial net::ProxyResolverFactory implementation that synchronously creates
+// net::ForwardingProxyResolvers that forward to a single passed-in resolver.
+class TestProxyResolverFactory : public net::ProxyResolverFactory {
+ public:
+  // Ownership of |resolver| remains with the caller. |resolver| must outlive
+  // the forwarding resolvers returned by CreateProxyResolver().
+  explicit TestProxyResolverFactory(net::ProxyResolver* resolver)
+      : net::ProxyResolverFactory(false /* expects_pac_bytes */),
+        resolver_(resolver) {}
+  ~TestProxyResolverFactory() override = default;
+
+  // net::ProxyResolverFactory:
+  int CreateProxyResolver(
+      const scoped_refptr<net::ProxyResolverScriptData>& pac_script,
+      std::unique_ptr<net::ProxyResolver>* resolver,
+      const net::CompletionCallback& callback,
+      std::unique_ptr<Request>* request) override {
+    *resolver = base::MakeUnique<net::ForwardingProxyResolver>(resolver_);
+    return net::OK;
+  }
+
+ private:
+  net::ProxyResolver* resolver_;  // Not owned.
+
+  DISALLOW_COPY_AND_ASSIGN(TestProxyResolverFactory);
+};
+
 // Test ProxyResolutionServiceProvider::Delegate implementation.
 class TestDelegate : public ProxyResolutionServiceProvider::Delegate {
  public:
   explicit TestDelegate(
-      const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner)
-      : request_context_getter_(
+      const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner,
+      net::ProxyResolver* proxy_resolver)
+      : proxy_resolver_(proxy_resolver),
+        context_getter_(
             new net::TestURLRequestContextGetter(network_task_runner)) {
-    // Use direct connections by default.
-    proxy_info_.UseDirect();
+    // The config's autodetect property needs to be set in order for
+    // net::ProxyService to send requests to our resolver.
+    net::ProxyConfig config = net::ProxyConfig::CreateAutoDetect();
+    proxy_service_ = base::MakeUnique<net::ProxyService>(
+        base::MakeUnique<net::ProxyConfigServiceFixed>(config),
+        base::MakeUnique<TestProxyResolverFactory>(proxy_resolver_),
+        nullptr /* net_log */);
+    context_getter_->GetURLRequestContext()->set_proxy_service(
+        proxy_service_.get());
   }
   ~TestDelegate() override = default;
 
-  void set_async(bool async) { async_ = async; }
-  void set_result(net::Error result) { result_ = result; }
-
-  const net::ProxyInfo& proxy_info() const { return proxy_info_; }
-  net::ProxyInfo* mutable_proxy_info() { return &proxy_info_; }
-
   // ProxyResolutionServiceProvider::Delegate:
   scoped_refptr<net::URLRequestContextGetter> GetRequestContext() override {
-    return request_context_getter_;
-  }
-  int ResolveProxy(net::ProxyService* proxy_service,
-                   const GURL& url,
-                   net::ProxyInfo* results,
-                   const net::CompletionCallback& callback) override {
-    EXPECT_EQ(proxy_service,
-              request_context_getter_->GetURLRequestContext()->proxy_service());
-    *results = proxy_info_;
-
-    if (!async_)
-      return result_;
-
-    request_context_getter_->GetNetworkTaskRunner()->PostTask(
-        FROM_HERE, base::Bind(callback, result_));
-    return net::ERR_IO_PENDING;
+    return context_getter_;
   }
 
  private:
-  // Should ResolveProxy() run asynchronously (rather than synchronously)?
-  bool async_ = false;
-
-  // Final result for ResolveProxy() to return.
-  net::Error result_ = net::OK;
-
-  // Proxy info for ResolveProxy() to return.
-  net::ProxyInfo proxy_info_;
-
-  scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_;
+  net::ProxyResolver* proxy_resolver_;  // Not owned.
+  std::unique_ptr<net::ProxyService> proxy_service_;
+  scoped_refptr<net::TestURLRequestContextGetter> context_getter_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDelegate);
 };
 
 }  // namespace
 
 class ProxyResolutionServiceProviderTest : public testing::Test {
  public:
+  ProxyResolutionServiceProviderTest() = default;
+  ~ProxyResolutionServiceProviderTest() override = default;
+
+  // testing::Test:
   void SetUp() override {
-    // Create the proxy resolution service with the mock bus and the mock
-    // resolver injected.
-    delegate_ = new TestDelegate(base::ThreadTaskRunnerHandle::Get());
+    proxy_resolver_ = base::MakeUnique<TestProxyResolver>(
+        base::ThreadTaskRunnerHandle::Get());
     service_provider_ = base::MakeUnique<ProxyResolutionServiceProvider>(
-        std::unique_ptr<TestDelegate>(delegate_));
-
+        base::MakeUnique<TestDelegate>(base::ThreadTaskRunnerHandle::Get(),
+                                       proxy_resolver_.get()));
     test_helper_.SetUp(kResolveNetworkProxy, service_provider_.get());
   }
-
   void TearDown() override {
     test_helper_.TearDown();
-    service_provider_.reset();
   }
 
  protected:
   // Arguments extracted from a D-Bus signal.
   struct SignalInfo {
     std::string source_url;
     std::string proxy_info;
     std::string error_message;
   };
 
@@ skipping 50 matching lines @@
     }
 
     *response_out = test_helper_.CallMethod(&method_call);
     base::RunLoop().RunUntilIdle();
     *signal_out = std::move(signal_);
   }
 
   // Information about the last D-Bus signal received by OnSignalReceived().
   std::unique_ptr<SignalInfo> signal_;
 
+  std::unique_ptr<TestProxyResolver> proxy_resolver_;
+  std::unique_ptr<ProxyResolutionServiceProvider> service_provider_;
   ServiceProviderTestHelper test_helper_;
-  std::unique_ptr<ProxyResolutionServiceProvider> service_provider_;
-  TestDelegate* delegate_;  // Owned by |service_provider_|.
 };
 
 // Tests that synchronously-resolved proxy information is returned via a signal.
 TEST_F(ProxyResolutionServiceProviderTest, SignalSync) {
   const char kSourceURL[] = "http://www.gmail.com/";
 
   std::unique_ptr<dbus::Response> response;
   std::unique_ptr<SignalInfo> signal;
   CallMethod(kSourceURL, true /* request_signal */, &response, &signal);
 
   // An empty response should be returned.
   ASSERT_TRUE(response);
   EXPECT_FALSE(dbus::MessageReader(response.get()).HasMoreData());
 
   // Confirm that the signal is received successfully.
   ASSERT_TRUE(signal);
   EXPECT_EQ(kSourceURL, signal->source_url);
-  EXPECT_EQ(delegate_->proxy_info().ToPacString(), signal->proxy_info);
+  EXPECT_EQ(proxy_resolver_->proxy_info().ToPacString(), signal->proxy_info);
   EXPECT_EQ("", signal->error_message);
 }
 
 // Tests that asynchronously-resolved proxy information is returned via a
 // signal.
 TEST_F(ProxyResolutionServiceProviderTest, SignalAsync) {
   const char kSourceURL[] = "http://www.gmail.com/";
-  delegate_->set_async(true);
-  delegate_->mutable_proxy_info()->UseNamedProxy("http://localhost:8080");
+  proxy_resolver_->set_async(true);
+  proxy_resolver_->mutable_proxy_info()->UseNamedProxy("http://localhost:8080");
 
   std::unique_ptr<dbus::Response> response;
   std::unique_ptr<SignalInfo> signal;
   CallMethod(kSourceURL, true /* request_signal */, &response, &signal);
 
   // An empty response should be returned.
   ASSERT_TRUE(response);
   EXPECT_FALSE(dbus::MessageReader(response.get()).HasMoreData());
 
   // Confirm that the signal is received successfully.
   ASSERT_TRUE(signal);
   EXPECT_EQ(kSourceURL, signal->source_url);
-  EXPECT_EQ(delegate_->proxy_info().ToPacString(), signal->proxy_info);
+  EXPECT_EQ(proxy_resolver_->proxy_info().ToPacString(), signal->proxy_info);
   EXPECT_EQ("", signal->error_message);
 }
 
 // Tests that an error received during proxy resolution is returned via a
 // signal.
 TEST_F(ProxyResolutionServiceProviderTest, SignalError) {
   const char kSourceURL[] = "http://www.gmail.com/";
   const net::Error kError = net::ERR_FAILED;
-  delegate_->set_result(kError);
+  proxy_resolver_->set_error(kError);
 
   std::unique_ptr<dbus::Response> response;
   std::unique_ptr<SignalInfo> signal;
   CallMethod(kSourceURL, true /* request_signal */, &response, &signal);
 
   // An empty response should be returned.
   ASSERT_TRUE(response);
   EXPECT_FALSE(dbus::MessageReader(response.get()).HasMoreData());
 
   // The signal should contain an error.
   ASSERT_TRUE(signal);
   EXPECT_EQ(kSourceURL, signal->source_url);
-  EXPECT_EQ(delegate_->proxy_info().ToPacString(), signal->proxy_info);
+  EXPECT_EQ(proxy_resolver_->proxy_info().ToPacString(), signal->proxy_info);
   EXPECT_EQ(net::ErrorToString(kError), signal->error_message);
 }
 
 TEST_F(ProxyResolutionServiceProviderTest, ResponseSync) {
   const char kSourceURL[] = "http://www.gmail.com/";
   std::unique_ptr<dbus::Response> response;
   std::unique_ptr<SignalInfo> signal;
   CallMethod(kSourceURL, false /* request_signal */, &response, &signal);
 
   // The response should contain the proxy info and an empty error.
+  // FIXME(derat): net::ProxyService::DidFinishResolvingProxy() falls back to
+  // direct connections and maps all resolution errors to net::OK unless
+  // |pac_mandatory| is set on the net::ProxyConfig, but I'm horribly confused
+  // about how to get a config with that setting into net::ProxyService().

[Daniel Erat, 2017/03/31 02:39:44]

i'm not sure how to fix this, or whether i should just delete the test. :-/

[eroman, 2017/04/04 21:06:10]

If your question is how to mock it in the test, then add:
   config.set_pac_mandatory(true);
After the line "net::ProxyConfig config = net::ProxyConfig::CreateAutoDetect();"
in initialization.

If your question is when it happens in practice, the "mandatory pac" is IIRC
only reachable when configuration is set via extension.

[Daniel Erat, 2017/04/05 02:34:13]

oh, hmm. my question was how to mock it in this test. i'm pretty sure that's
exactly what i tried (along with also setting a pac url when that didn't work,
since there appeared to be expectations in ProxyConfig that the url would also
be set).

however, pac_mandatory is always unset in the config that ProxyService ends up
using when i do this. i wasn't successful in tracing exactly what was going on,
but my working theory was that ProxyService ended up not using the original
config from the ProxyConfigService in this case due to the pac config not being
successfully fetched from the pac url.

i can make another change demonstrating the problem if that behavior seems
unexpected to you.

   ASSERT_TRUE(response);
   dbus::MessageReader reader(response.get());
   std::string proxy_info, error;
   EXPECT_TRUE(reader.PopString(&proxy_info));
   EXPECT_TRUE(reader.PopString(&error));
-  EXPECT_EQ(delegate_->proxy_info().ToPacString(), proxy_info);
+  EXPECT_EQ(proxy_resolver_->proxy_info().ToPacString(), proxy_info);
   EXPECT_EQ("", error);
 
   // No signal should've been emitted.
   EXPECT_FALSE(signal);
 }
 
 TEST_F(ProxyResolutionServiceProviderTest, ResponseAsync) {
   const char kSourceURL[] = "http://www.gmail.com/";
-  delegate_->set_async(true);
-  delegate_->mutable_proxy_info()->UseNamedProxy("http://localhost:8080");
+  proxy_resolver_->set_async(true);
+  proxy_resolver_->mutable_proxy_info()->UseNamedProxy("http://localhost:8080");
   std::unique_ptr<dbus::Response> response;
   std::unique_ptr<SignalInfo> signal;
   CallMethod(kSourceURL, false /* request_signal */, &response, &signal);
 
   // The response should contain the proxy info and an empty error.
   ASSERT_TRUE(response);
   dbus::MessageReader reader(response.get());
   std::string proxy_info, error;
   EXPECT_TRUE(reader.PopString(&proxy_info));
   EXPECT_TRUE(reader.PopString(&error));
-  EXPECT_EQ(delegate_->proxy_info().ToPacString(), proxy_info);
+  EXPECT_EQ(proxy_resolver_->proxy_info().ToPacString(), proxy_info);
   EXPECT_EQ("", error);
 
   // No signal should've been emitted.
   EXPECT_FALSE(signal);
 }
 
 }  // namespace chromeos