Simplify DoH probe and separate it from the UI

This change reduces code duplication by reusing DnsProbeRunner for
probing DoH servers in the UI.  It also extracts the probe logic
from the UI code, in order to reuse it in the forthcoming Android
DoH settings UI.

Change-Id: If6f6f1a8d7b6bb8f1d5810880d56dafbfdcf26f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150031
Auto-Submit: Ben Schwartz <bemasc@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#764137}

chrome/browser/net/dns_util.cc

@@ -80,4 +80,14 @@ bool IsValidDohTemplateGroup(base::StringPiece group) {
   });
 }
 
+void ApplyDohTemplate(net::DnsConfigOverrides* overrides,
+                      base::StringPiece server_template) {
+  std::string server_method;
+  // We only allow use of templates that have already passed a format
+  // validation check.
+  CHECK(net::dns_util::IsValidDohTemplate(server_template, &server_method));
+  overrides->dns_over_https_servers.emplace({net::DnsOverHttpsServerConfig(
+      std::string(server_template), server_method == "POST")});
+}
+
 }  // namespace chrome_browser_net

chrome/browser/net/dns_util.h

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/strings/string_piece.h"
+#include "net/dns/dns_config_overrides.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -22,6 +23,10 @@ std::vector<base::StringPiece> SplitDohTemplateGroup(base::StringPiece group);
 // stored preferences.
 bool IsValidDohTemplateGroup(base::StringPiece group);
 
+// Modifies |overrides| to use the DoH server specified by |server_template|.
+void ApplyDohTemplate(net::DnsConfigOverrides* overrides,
+                      base::StringPiece server_template);
+
 const char kDnsOverHttpsModeOff[] = "off";
 const char kDnsOverHttpsModeAutomatic[] = "automatic";
 const char kDnsOverHttpsModeSecure[] = "secure";

chrome/browser/net/dns_util_unittest.cc

@@ -165,4 +165,24 @@ TEST(DNSUtil, IsValidDohTemplateGroup) {
   EXPECT_FALSE(IsValidDohTemplateGroup("invalid invalid2"));
 }
 
+TEST(DNSUtil, ApplyDohTemplatePost) {
+  std::string post_template("https://valid");
+  net::DnsConfigOverrides overrides;
+  ApplyDohTemplate(&overrides, post_template);
+
+  EXPECT_THAT(overrides.dns_over_https_servers,
+              testing::Optional(ElementsAre(net::DnsOverHttpsServerConfig(
+                  {post_template, true /* use_post */}))));
+}
+
+TEST(DNSUtil, ApplyDohTemplateGet) {
+  std::string get_template("https://valid/{?dns}");
+  net::DnsConfigOverrides overrides;
+  ApplyDohTemplate(&overrides, get_template);
+
+  EXPECT_THAT(overrides.dns_over_https_servers,
+              testing::Optional(ElementsAre(net::DnsOverHttpsServerConfig(
+                  {get_template, false /* use_post */}))));
+}
+
 }  // namespace chrome_browser_net

chrome/browser/ui/webui/settings/settings_secure_dns_handler.cc

@@ -31,8 +31,6 @@ namespace settings {
 
 namespace {
 
-const char kProbeHostname[] = "google.com";
-
 std::unique_ptr<base::DictionaryValue> CreateSecureDnsSettingDict() {
   // Fetch the current host resolver configuration. It is not sufficient to read
   // the secure DNS prefs directly since the host resolver configuration takes
@@ -76,7 +74,10 @@ std::unique_ptr<base::DictionaryValue> CreateSecureDnsSettingDict() {
 
 }  // namespace
 
-SecureDnsHandler::SecureDnsHandler() = default;
+SecureDnsHandler::SecureDnsHandler()
+    : network_context_getter_(
+          base::BindRepeating(&SecureDnsHandler::GetNetworkContext,
+                              base::Unretained(this))) {}
 
 SecureDnsHandler::~SecureDnsHandler() = default;
 
@@ -179,7 +180,17 @@ base::Value SecureDnsHandler::GetSecureDnsResolverListForCountry(
 
 void SecureDnsHandler::SetNetworkContextForTesting(
     network::mojom::NetworkContext* network_context) {
-  network_context_for_testing_ = network_context;
+  network_context_getter_ = base::BindRepeating(
+      [](network::mojom::NetworkContext* network_context) {
+        return network_context;
+      },
+      network_context);
+}
+
+network::mojom::NetworkContext* SecureDnsHandler::GetNetworkContext() {
+  return content::BrowserContext::GetDefaultStoragePartition(
+             web_ui()->GetWebContents()->GetBrowserContext())
+      ->GetNetworkContext();
 }
 
 void SecureDnsHandler::HandleGetSecureDnsResolverList(
@@ -225,10 +236,10 @@ void SecureDnsHandler::HandleProbeCustomDnsTemplate(
   AllowJavascript();
 
   if (!probe_callback_id_.empty()) {
-    // Cancel the pending probe and report a non-error response to avoid
-    // leaking the callback.
-    receiver_.reset();
-    host_resolver_.reset();
+    // Cancel the pending probe by destroying the probe runner (which does not
+    // call the callback), and report a non-error response to avoid leaking the
+    // callback.
+    runner_.reset();
     ResolveJavascriptCallback(base::Value(probe_callback_id_),
                               base::Value(true));
   }
@@ -242,33 +253,12 @@ void SecureDnsHandler::HandleProbeCustomDnsTemplate(
   overrides.attempts = 1;
   overrides.randomize_ports = false;
   overrides.secure_dns_mode = net::DnsConfig::SecureDnsMode::SECURE;
-  std::string server_method;
-  // We only send probe queries to templates that have already passed a format
-  // validation check.
-  CHECK(net::dns_util::IsValidDohTemplate(server_template, &server_method));
-  overrides.dns_over_https_servers.emplace({net::DnsOverHttpsServerConfig(
-      server_template, server_method == "POST")});
-  auto* network_context =
-      network_context_for_testing_
-          ? network_context_for_testing_
-          : content::BrowserContext::GetDefaultStoragePartition(
-                web_ui()->GetWebContents()->GetBrowserContext())
-                ->GetNetworkContext();
-  network_context->CreateHostResolver(
-      overrides, host_resolver_.BindNewPipeAndPassReceiver());
-
-  network::mojom::ResolveHostParametersPtr parameters =
-      network::mojom::ResolveHostParameters::New();
-  parameters->dns_query_type = net::DnsQueryType::A;
-  parameters->source = net::HostResolverSource::DNS;
-  parameters->cache_usage =
-      network::mojom::ResolveHostParameters::CacheUsage::DISALLOWED;
-  host_resolver_->ResolveHost(net::HostPortPair(kProbeHostname, 80),
-                              net::NetworkIsolationKey::CreateTransient(),
-                              std::move(parameters),
-                              receiver_.BindNewPipeAndPassRemote());
-  receiver_.set_disconnect_handler(base::BindOnce(
-      &SecureDnsHandler::OnMojoConnectionError, base::Unretained(this)));
+  chrome_browser_net::ApplyDohTemplate(&overrides, server_template);
+  DCHECK(!runner_);
+  runner_ = std::make_unique<chrome_browser_net::DnsProbeRunner>(
+      overrides, network_context_getter_);
+  runner_->RunProbe(base::BindOnce(&SecureDnsHandler::OnProbeComplete,
+                                   base::Unretained(this)));
 }
 
 void SecureDnsHandler::HandleRecordUserDropdownInteraction(
@@ -296,24 +286,16 @@ void SecureDnsHandler::HandleRecordUserDropdownInteraction(
   }
 }
 
-// network::ResolveHostClientBase impl:
-void SecureDnsHandler::OnComplete(
-    int result,
-    const net::ResolveErrorInfo& resolve_error_info,
-    const base::Optional<net::AddressList>& resolved_addresses) {
-  receiver_.reset();
-  host_resolver_.reset();
-  UMA_HISTOGRAM_BOOLEAN("Net.DNS.UI.ProbeAttemptSuccess", (result == 0));
+void SecureDnsHandler::OnProbeComplete() {
+  bool success =
+      runner_->result() == chrome_browser_net::DnsProbeRunner::CORRECT;
+  runner_.reset();
+  UMA_HISTOGRAM_BOOLEAN("Net.DNS.UI.ProbeAttemptSuccess", success);
   ResolveJavascriptCallback(base::Value(probe_callback_id_),
-                            base::Value(result == 0));
+                            base::Value(success));
   probe_callback_id_.clear();
 }
 
-void SecureDnsHandler::OnMojoConnectionError() {
-  OnComplete(net::ERR_NAME_NOT_RESOLVED, net::ResolveErrorInfo(net::ERR_FAILED),
-             base::nullopt);
-}
-
 void SecureDnsHandler::SendSecureDnsSettingUpdatesToJavascript() {
   FireWebUIListener("secure-dns-setting-changed",
                     *CreateSecureDnsSettingDict());

chrome/browser/ui/webui/settings/settings_secure_dns_handler.h

@@ -11,20 +11,17 @@
 
 #include "base/macros.h"
 #include "base/values.h"
+#include "chrome/browser/net/dns_probe_runner.h"
 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
 #include "components/prefs/pref_change_registrar.h"
-#include "mojo/public/cpp/bindings/receiver.h"
-#include "mojo/public/cpp/bindings/remote.h"
 #include "net/dns/public/doh_provider_list.h"
 #include "services/network/public/cpp/resolve_host_client_base.h"
-#include "services/network/public/mojom/host_resolver.mojom.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 
 namespace settings {
 
 // Handler for the Secure DNS setting.
-class SecureDnsHandler : public SettingsPageUIHandler,
-                         network::ResolveHostClientBase {
+class SecureDnsHandler : public SettingsPageUIHandler {
  public:
   SecureDnsHandler();
   ~SecureDnsHandler() override;
@@ -66,18 +63,13 @@ class SecureDnsHandler : public SettingsPageUIHandler,
   void SendSecureDnsSettingUpdatesToJavascript();
 
  private:
-  // network::ResolveHostClientBase impl:
-  void OnComplete(
-      int result,
-      const net::ResolveErrorInfo& resolve_error_info,
-      const base::Optional<net::AddressList>& resolved_addresses) override;
-
-  void OnMojoConnectionError();
+  network::mojom::NetworkContext* GetNetworkContext();
+  void OnProbeComplete();
 
   std::map<std::string, net::DohProviderIdForHistogram> resolver_histogram_map_;
-  network::mojom::NetworkContext* network_context_for_testing_ = nullptr;
-  mojo::Receiver<network::mojom::ResolveHostClient> receiver_{this};
-  mojo::Remote<network::mojom::HostResolver> host_resolver_;
+  std::unique_ptr<chrome_browser_net::DnsProbeRunner> runner_;
+  chrome_browser_net::DnsProbeRunner::NetworkContextGetter
+      network_context_getter_;
   // ID of the Javascript callback for the current pending probe, or "" if
   // there is no probe currently in progress.
   std::string probe_callback_id_;