chromeos: Avoid DCHECK in ProxyResolutionServiceProvider.

chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc

Index: chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
diff --git a/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc b/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
index bbd935bfe7d8d0b6103493f7d63a6ba2249b635a..336d0171928831e32df91be58cf796ee0f755056 100644
--- a/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
+++ b/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc
@@ -5,10 +5,14 @@
 #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"
@@ -30,6 +34,13 @@ namespace {
 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 {
@@ -110,6 +121,33 @@ class TestDelegate : public ProxyResolutionServiceProvider::Delegate {
       : 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();
@@ -120,14 +158,13 @@ class TestDelegate : public ProxyResolutionServiceProvider::Delegate {
     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_;
@@ -139,20 +176,26 @@ class TestDelegate : public ProxyResolutionServiceProvider::Delegate {
 
 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:
@@ -216,10 +259,19 @@ class ProxyResolutionServiceProviderTest : public testing::Test {
     }
 
     *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_;