chromeos: Avoid DCHECK in ProxyResolutionServiceProvider.

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/bind_helpers.h"
+#include "base/logging.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/test/thread_test_helper.h"
+#include "base/threading/thread.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";
 
+// Runs all tasks on |task_runner|.
+void RunAllTasks(scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  scoped_refptr<base::ThreadTestHelper> helper =
+      new base::ThreadTestHelper(task_runner);
+  ASSERT_TRUE(helper->Run());
+}
+
 // 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;
@@ skipping 60 matching lines @@
 
 // Test ProxyResolutionServiceProvider::Delegate implementation.
 class TestDelegate : public ProxyResolutionServiceProvider::Delegate {
  public:
   explicit TestDelegate(
       const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner,
       net::ProxyResolver* proxy_resolver)
       : proxy_resolver_(proxy_resolver),
         context_getter_(
             new net::TestURLRequestContextGetter(network_task_runner)) {
+    // NetworkProxy derives from base::NonThreadSafe, so it needs to be created
+    // on the network thread so it won't blow up when it's called from there
+    // later.
+    network_task_runner->PostTask(
+        FROM_HERE, base::Bind(&TestDelegate::CreateProxyServiceOnNetworkThread,
+                              base::Unretained(this)));
+    RunAllTasks(network_task_runner);

[Daniel Erat, 2017/04/01 00:19:19]

yuck.

[James Cook, 2017/04/03 15:30:23]

lol

+  }
+
+  ~TestDelegate() override {
+    context_getter_->GetNetworkTaskRunner()->PostTask(
+        FROM_HERE, base::Bind(&TestDelegate::DeleteProxyServiceOnNetworkThread,
+                              base::Unretained(this)));
+    RunAllTasks(context_getter_->GetNetworkTaskRunner());

[Daniel Erat, 2017/04/01 00:19:19]

yuck again.

+  }
+
+  // ProxyResolutionServiceProvider::Delegate:
+  scoped_refptr<net::URLRequestContextGetter> GetRequestContext() override {
+    return context_getter_;
+  }
+
+ private:
+  // Helper method for the constructor that initializes |proxy_service_| and
+  // injects it into |context_getter_|'s context.
+  void CreateProxyServiceOnNetworkThread() {
+    CHECK(context_getter_->GetNetworkTaskRunner()->BelongsToCurrentThread());
+
     // 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;
 
-  // ProxyResolutionServiceProvider::Delegate:
-  scoped_refptr<net::URLRequestContextGetter> GetRequestContext() override {
-    return context_getter_;
+  // Helper method for the destructor that resets |proxy_service_|.
+  void DeleteProxyServiceOnNetworkThread() {
+    CHECK(context_getter_->GetNetworkTaskRunner()->BelongsToCurrentThread());
+    proxy_service_.reset();
   }
 
- private:
   net::ProxyResolver* proxy_resolver_;  // Not owned.
   std::unique_ptr<net::ProxyService> proxy_service_;

[James Cook, 2017/04/03 15:30:23]

optional: comment that it lives on network thread?

[Daniel Erat, 2017/04/04 00:01:37]

Done.

   scoped_refptr<net::TestURLRequestContextGetter> context_getter_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDelegate);
 };
 
 }  // namespace
 
 class ProxyResolutionServiceProviderTest : public testing::Test {
  public:
-  ProxyResolutionServiceProviderTest() = default;
-  ~ProxyResolutionServiceProviderTest() override = default;
+  ProxyResolutionServiceProviderTest() : network_thread_("NetworkThread") {
+    CHECK(network_thread_.Start());
 
-  // testing::Test:
-  void SetUp() override {
-    proxy_resolver_ = base::MakeUnique<TestProxyResolver>(
-        base::ThreadTaskRunnerHandle::Get());
+    proxy_resolver_ =
+        base::MakeUnique<TestProxyResolver>(network_thread_.task_runner());
     service_provider_ = base::MakeUnique<ProxyResolutionServiceProvider>(
-        base::MakeUnique<TestDelegate>(base::ThreadTaskRunnerHandle::Get(),
+        base::MakeUnique<TestDelegate>(network_thread_.task_runner(),
                                        proxy_resolver_.get()));
     test_helper_.SetUp(kResolveNetworkProxy, service_provider_.get());
   }
-  void TearDown() override {
+
+  ~ProxyResolutionServiceProviderTest() override {
     test_helper_.TearDown();
+
+    // URLRequestContextGetter posts a task to delete itself to its task runner,
+    // so give it a chance to do that.
+    service_provider_.reset();
+    RunAllTasks(network_thread_.task_runner());
+
+    network_thread_.Stop();
   }
 
  protected:
   // Arguments extracted from a D-Bus signal.
   struct SignalInfo {
     std::string source_url;
     std::string proxy_info;
     std::string error_message;
   };
 
@@ skipping 43 matching lines @@
       // signal. OnSignalReceived() will be called upon the delivery.
       test_helper_.SetUpReturnSignal(
           kReturnSignalInterface, kReturnSignalName,
           base::Bind(&ProxyResolutionServiceProviderTest::OnSignalReceived,
                      base::Unretained(this)),
           base::Bind(&ProxyResolutionServiceProviderTest::OnConnectedToSignal,
                      base::Unretained(this)));
     }
 
     *response_out = test_helper_.CallMethod(&method_call);
+
+    // Let the main thread perform work on the network thread and then send the
+    // response.
     base::RunLoop().RunUntilIdle();
+    RunAllTasks(network_thread_.task_runner());
+    base::RunLoop().RunUntilIdle();

[Daniel Erat, 2017/04/04 00:01:37]

sigh, it turns out there's a tricky race here. i think that this is what
happens:

- the first RunUntilIdle call presumably doesn't do anything, as we're running
ResolveProxy synchronously (whoops). ResolveProxyOnNetworkThread is posted to
the network thread, as expected.
- the RunAllTasks call results in ResolveProxyOnNetworkThread running, but
looking at ThreadTestHelper's implementation, it just posts a task and waits for
it to be executed (instead of running the thread until it has no scheduled
tasks). as a result, in the async-lookup case, RunAllTasks can return before
ResolveProxyOnNetworkThread runs on the network thread.

i initially looked into making RunAllTasks actually wait for a task that runs
RunLoop::RunUntilIdle on the target thread, but that doesn't work either. i
suspect that net::ProxyService's threading story is more complicated than i
thought.

luckily, it's not to hard to make us just run the main message loop here until
we receive the d-bus signal, which seems to do the trick and which is what i
should've done in the first place instead of trying to run multiple message
loops in the correct order.

and even better, that code will go away once this class only sends d-bus
responses and no longer emits signals.

+
     *signal_out = std::move(signal_);
   }
 
+  // Thread used to perform network operations.
+  base::Thread network_thread_;
+
   // 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_;
 };
 
 // Tests that synchronously-resolved proxy information is returned via a signal.
 TEST_F(ProxyResolutionServiceProviderTest, SignalSync) {
@@ skipping 69 matching lines @@
   std::string proxy_info, error;
   EXPECT_TRUE(reader.PopString(&proxy_info));
   EXPECT_TRUE(reader.PopString(&error));
   EXPECT_EQ(proxy_resolver_->proxy_info().ToPacString(), proxy_info);
   EXPECT_EQ("", error);
 
   // No signal should've been emitted.
   EXPECT_FALSE(signal);
 }
 
 }  // namespace chromeos

[James Cook, 2017/04/03 15:30:23]

Awesome that you were able to make the tests cover the threading.