Fix a memory leak in the Secure DNS UI

If a DNS probe happens before the previous probe completes,
the previous callback will never be called, leading to a
JS stack leak.  This change resolves the first callback
to free up the stack.

This leak is unlikely in practice.  If a leak does occur,
it persists until the user closes the settings page.

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

chrome/browser/net/dns_probe_test_util.cc

@@ -139,6 +139,17 @@ void FakeHostResolverNetworkContext::CreateHostResolver(
   }
 }
 
+HangingHostResolverNetworkContext::HangingHostResolverNetworkContext() =
+    default;
+HangingHostResolverNetworkContext::~HangingHostResolverNetworkContext() =
+    default;
+
+void HangingHostResolverNetworkContext::CreateHostResolver(
+    const base::Optional<net::DnsConfigOverrides>& config_overrides,
+    mojo::PendingReceiver<network::mojom::HostResolver> receiver) {
+  resolver_ = std::make_unique<HangingHostResolver>(std::move(receiver));
+}
+
 FakeDnsConfigChangeManager::FakeDnsConfigChangeManager(
     mojo::PendingReceiver<network::mojom::DnsConfigChangeManager> receiver)
     : receiver_(this, std::move(receiver)) {}

chrome/browser/net/dns_probe_test_util.h

@@ -108,6 +108,19 @@ class FakeHostResolverNetworkContext : public network::TestNetworkContext {
   std::unique_ptr<FakeHostResolver> google_config_resolver_;
 };
 
+class HangingHostResolverNetworkContext : public network::TestNetworkContext {
+ public:
+  HangingHostResolverNetworkContext();
+  ~HangingHostResolverNetworkContext() override;
+
+  void CreateHostResolver(
+      const base::Optional<net::DnsConfigOverrides>& config_overrides,
+      mojo::PendingReceiver<network::mojom::HostResolver> receiver) override;
+
+ private:
+  std::unique_ptr<HangingHostResolver> resolver_;
+};
+
 class FakeDnsConfigChangeManager
     : public network::mojom::DnsConfigChangeManager {
  public:

chrome/browser/resources/settings/privacy_page/privacy_page_browser_proxy.js

@@ -80,7 +80,8 @@ cr.define('settings', function() {
     validateCustomDnsEntry(entry) {}
 
     /**
-     * Returns whether a test query to the secure DNS template succeeded.
+     * Returns True if a test query to the secure DNS template succeeded
+     * or was cancelled.
      * @param {string} template
      * @return {!Promise<boolean>}
      */

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

@@ -223,8 +223,15 @@ void SecureDnsHandler::HandleValidateCustomDnsEntry(
 void SecureDnsHandler::HandleProbeCustomDnsTemplate(
     const base::ListValue* args) {
   AllowJavascript();
-  receiver_.reset();
-  host_resolver_.reset();
+
+  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();
+    ResolveJavascriptCallback(base::Value(probe_callback_id_),
+                              base::Value(true));
+  }
 
   std::string server_template;
   CHECK(args->GetString(0, &probe_callback_id_));
@@ -299,6 +306,7 @@ void SecureDnsHandler::OnComplete(
   UMA_HISTOGRAM_BOOLEAN("Net.DNS.UI.ProbeAttemptSuccess", (result == 0));
   ResolveJavascriptCallback(base::Value(probe_callback_id_),
                             base::Value(result == 0));
+  probe_callback_id_.clear();
 }
 
 void SecureDnsHandler::OnMojoConnectionError() {

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

@@ -78,6 +78,8 @@ class SecureDnsHandler : public SettingsPageUIHandler,
   network::mojom::NetworkContext* network_context_for_testing_ = nullptr;
   mojo::Receiver<network::mojom::ResolveHostClient> receiver_{this};
   mojo::Remote<network::mojom::HostResolver> host_resolver_;
+  // ID of the Javascript callback for the current pending probe, or "" if
+  // there is no probe currently in progress.
   std::string probe_callback_id_;
   PrefChangeRegistrar pref_registrar_;
 

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

@@ -606,4 +606,57 @@ IN_PROC_BROWSER_TEST_F(SecureDnsHandlerTest, TemplateProbeFailure) {
   histograms.ExpectBucketCount("Net.DNS.UI.ProbeAttemptSuccess", true, 0);
 }
 
+IN_PROC_BROWSER_TEST_F(SecureDnsHandlerTest, TemplateProbeDebounce) {
+  auto network_context_hang =
+      std::make_unique<chrome_browser_net::HangingHostResolverNetworkContext>();
+  auto network_context_fail =
+      std::make_unique<chrome_browser_net::FakeHostResolverNetworkContext>(
+          std::vector<chrome_browser_net::FakeHostResolver::SingleResult>(
+              {chrome_browser_net::FakeHostResolver::SingleResult(
+                  net::ERR_NAME_NOT_RESOLVED,
+                  net::ResolveErrorInfo(net::ERR_DNS_MALFORMED_RESPONSE),
+                  chrome_browser_net::FakeHostResolver::
+                      kNoResponse)}) /* current_config_result_list */,
+          std::vector<chrome_browser_net::FakeHostResolver::
+                          SingleResult>() /* google_config_result_list */);
+  base::HistogramTester histograms;
+  base::ListValue args_valid;
+  args_valid.AppendString(kWebUiFunctionName);
+  args_valid.AppendString("https://example.template/dns-query");
+  // Request a probe that will hang.
+  handler_->SetNetworkContextForTesting(network_context_hang.get());
+  web_ui_.HandleReceivedMessage(kProbeCustomDnsTemplate, &args_valid);
+  size_t responses = web_ui_.call_data().size();
+  base::RunLoop().RunUntilIdle();
+  // No response yet from the hanging probe.
+  EXPECT_EQ(responses, web_ui_.call_data().size());
+
+  // Request a probe that will fail.
+  handler_->SetNetworkContextForTesting(network_context_fail.get());
+  web_ui_.HandleReceivedMessage(kProbeCustomDnsTemplate, &args_valid);
+  // The hanging response should now have arrived.
+  EXPECT_EQ(responses + 1, web_ui_.call_data().size());
+  const content::TestWebUI::CallData& first_response =
+      *web_ui_.call_data().back();
+  ASSERT_EQ("cr.webUIResponse", first_response.function_name());
+  ASSERT_EQ(kWebUiFunctionName, first_response.arg1()->GetString());
+  // The cancelled probe should indicate no error.
+  ASSERT_TRUE(first_response.arg2()->GetBool());
+  EXPECT_TRUE(first_response.arg3()->GetBool());
+
+  // Wait for the second response.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(responses + 2, web_ui_.call_data().size());
+  const content::TestWebUI::CallData& second_response =
+      *web_ui_.call_data().back();
+  ASSERT_EQ("cr.webUIResponse", second_response.function_name());
+  ASSERT_EQ(kWebUiFunctionName, second_response.arg1()->GetString());
+  // The second request should have resolved with a failure indication.
+  ASSERT_TRUE(second_response.arg2()->GetBool());
+  EXPECT_FALSE(second_response.arg3()->GetBool());
+  // Only the second response should be counted in the histograms.
+  histograms.ExpectBucketCount("Net.DNS.UI.ProbeAttemptSuccess", false, 1);
+  histograms.ExpectBucketCount("Net.DNS.UI.ProbeAttemptSuccess", true, 0);
+}
+
 }  // namespace settings

chrome/test/data/webui/settings/secure_dns_interactive_test.js

@@ -398,8 +398,7 @@ suite('SettingsSecureDnsInteractive', function() {
 
     // Make the template valid, but don't change the radio button yet.
     secureDnsInput.focus();
-    secureDnsInput.value =
-        `${validEntry} ${invalidEntry} https://dns.ex.another/dns-query`;
+    secureDnsInput.value = `${validEntry} https://dns.ex.another/dns-query`;
     testBrowserProxy.setValidEntry(validEntry);
     testBrowserProxy.setProbeSuccess(true);
     secureDnsInput.blur();
@@ -433,7 +432,7 @@ suite('SettingsSecureDnsInteractive', function() {
         settings.SecureDnsMode.SECURE,
         testElement.prefs.dns_over_https.mode.value);
     assertEquals(
-        `${validEntry} ${invalidEntry} https://dns.ex.another/dns-query`,
+        `${validEntry} https://dns.ex.another/dns-query`,
         testElement.prefs.dns_over_https.templates.value);
 
     // Make sure the input field updates with a change in the underlying