DevTools: do not retain HostResolver in TCPDeviceProvider

After we switched to using mojo host resolver in
https://chromium-review.googlesource.com/c/chromium/src/+/1199848/
we need to make sure HostResolver is destroyed on the same thread
that it was created on.
Ensuring this with ref-counted DeviceProvider would require
a helper class, so just re-create it each time we need to
resolve a host for simplicity.
The overhead should be small considering to other things we
do (like not caching host resolution lately etc).

Bug: 1028292, 1025369, 921608

Change-Id: I6f8d2e663f0ac872964e1725e261232493bbbfe5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1972781
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726022}

chrome/browser/devtools/device/tcp_device_provider.cc

@@ -43,17 +43,25 @@ static void RunSocketCallback(
 
 class ResolveHostAndOpenSocket final : public network::ResolveHostClientBase {
  public:
-  ResolveHostAndOpenSocket(
-      const net::HostPortPair& address,
-      const AdbClientSocket::SocketCallback& callback,
-      mojo::Remote<network::mojom::HostResolver>* host_resolver)
+  ResolveHostAndOpenSocket(const net::HostPortPair& address,
+                           const AdbClientSocket::SocketCallback& callback)
       : callback_(callback) {
+    mojo::Remote<network::mojom::HostResolver> resolver;
+    base::PostTask(FROM_HERE, {content::BrowserThread::UI},
+                   base::BindOnce(
+                       [](mojo::PendingReceiver<network::mojom::HostResolver>
+                              pending_receiver) {
+                         g_browser_process->system_network_context_manager()
+                             ->GetContext()
+                             ->CreateHostResolver(base::nullopt,
+                                                  std::move(pending_receiver));
+                       },
+                       resolver.BindNewPipeAndPassReceiver()));
     // Fine to use a transient NetworkIsolationKey here - this is for debugging,
     // so performance doesn't matter, and it doesn't need to share a DNS cache
     // with anything else.
-    (*host_resolver)
-        ->ResolveHost(address, net::NetworkIsolationKey::CreateTransient(),
-                      nullptr, receiver_.BindNewPipeAndPassRemote());
+    resolver->ResolveHost(address, net::NetworkIsolationKey::CreateTransient(),
+                          nullptr, receiver_.BindNewPipeAndPassRemote());
     receiver_.set_disconnect_handler(
         base::BindOnce(&ResolveHostAndOpenSocket::OnComplete,
                        base::Unretained(this), net::ERR_NAME_NOT_RESOLVED,
@@ -139,15 +147,7 @@ void TCPDeviceProvider::OpenSocket(const std::string& serial,
   // (debugging purposes).
   int port;
   base::StringToInt(socket_name, &port);
-  net::HostPortPair host_port(serial, port);
-
-  // OpenSocket() is run on the devtools ADB thread, while TCPDeviceProvider is
-  // created on the UI thread, so do any initialization of |host_resolver_|
-  // here.
-  if (!host_resolver_) {
-    InitializeHostResolver();
-  }
-  new ResolveHostAndOpenSocket(host_port, callback, &host_resolver_);
+  new ResolveHostAndOpenSocket(net::HostPortPair(serial, port), callback);
 }
 
 void TCPDeviceProvider::ReleaseDevice(const std::string& serial) {
@@ -162,20 +162,3 @@ void TCPDeviceProvider::set_release_callback_for_test(
 
 TCPDeviceProvider::~TCPDeviceProvider() {
 }
-
-void TCPDeviceProvider::InitializeHostResolver() {
-  host_resolver_.reset();
-  base::PostTask(
-      FROM_HERE, {content::BrowserThread::UI},
-      base::BindOnce(&TCPDeviceProvider::InitializeHostResolverOnUI, this,
-                     host_resolver_.BindNewPipeAndPassReceiver()));
-  host_resolver_.set_disconnect_handler(base::BindOnce(
-      &TCPDeviceProvider::InitializeHostResolver, base::Unretained(this)));
-}
-
-void TCPDeviceProvider::InitializeHostResolverOnUI(
-    mojo::PendingReceiver<network::mojom::HostResolver> receiver) {
-  g_browser_process->system_network_context_manager()
-      ->GetContext()
-      ->CreateHostResolver(base::nullopt, std::move(receiver));
-}

chrome/browser/devtools/device/tcp_device_provider.h

@@ -15,8 +15,6 @@
 #include "net/base/host_port_pair.h"
 #include "services/network/public/mojom/host_resolver.mojom.h"
 
-// Instantiate this class only in a test and/or when the DEBUG_DEVTOOLS
-// BUILDFLAG is set.
 class TCPDeviceProvider : public AndroidDeviceManager::DeviceProvider {
  public:
   static scoped_refptr<TCPDeviceProvider> CreateForLocalhost(uint16_t port);
@@ -42,13 +40,8 @@ class TCPDeviceProvider : public AndroidDeviceManager::DeviceProvider {
  private:
   ~TCPDeviceProvider() override;
 
-  void InitializeHostResolver();
-  void InitializeHostResolverOnUI(
-      mojo::PendingReceiver<network::mojom::HostResolver> receiver);
-
   HostPortSet targets_;
   base::Closure release_callback_;
-  mojo::Remote<network::mojom::HostResolver> host_resolver_;
 };
 
 #endif  // CHROME_BROWSER_DEVTOOLS_DEVICE_TCP_DEVICE_PROVIDER_H_

chrome/browser/devtools/devtools_sanity_browsertest.cc

@@ -1053,7 +1053,7 @@ IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
 // different from the extension's background page, are rendered in their own
 // processes and not in the devtools process or the extension's process.
 IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
-                       DISABLED_HttpIframeInDevToolsExtensionDevtools) {
+                       HttpIframeInDevToolsExtensionDevtools) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
   // Install the dynamically-generated extension.
@@ -1819,23 +1819,13 @@ class MAYBE_DevToolsReattachAfterCrashTest : public DevToolsSanityTest {
   }
 };
 
-// Crashes on Win. http://crbug.com/1025369
-#if defined(OS_WIN)
-#define MAYBE_TestReattachAfterCrashOnTimeline \
-  DISABLED_TestReattachAfterCrashOnTimeline
-#define MAYBE_TestReattachAfterCrashOnNetwork \
-  DISABLED_TestReattachAfterCrashOnNetwork
-#else
-#define MAYBE_TestReattachAfterCrashOnTimeline TestReattachAfterCrashOnTimeline
-#define MAYBE_TestReattachAfterCrashOnNetwork TestReattachAfterCrashOnNetwork
-#endif
 IN_PROC_BROWSER_TEST_F(MAYBE_DevToolsReattachAfterCrashTest,
-                       MAYBE_TestReattachAfterCrashOnTimeline) {
+                       TestReattachAfterCrashOnTimeline) {
   RunTestWithPanel("timeline");
 }
 
 IN_PROC_BROWSER_TEST_F(MAYBE_DevToolsReattachAfterCrashTest,
-                       MAYBE_TestReattachAfterCrashOnNetwork) {
+                       TestReattachAfterCrashOnNetwork) {
   RunTestWithPanel("network");
 }