Revert "Create ResolveContext"

This reverts commit 50f1686c.

Reason for revert: Failure on Linux ChromiumOS MSan Tests

Bug:1047641

Original change's description:
> Create ResolveContext
> 
> This object stores per-context state for use in host resolution. In
> subsequent CLs, it will be passed through the stack instead of how raw
> URLRequestContext* is passed today.
> 
> Will soon be responsible for some data currently stored per-session in
> DnsSession (in this CL, implementing the DoH probe results, but not yet
> using the moved results). Because that data will be both per-context
> and per-session, but because we really only care about it in the most
> recent "active" session, ResolveContext keeps track of the "current"
> session and compares session on manipulation or retrieval of the
> per-session data.
> 
> Bug: 1022059
> Change-Id: Iad75fd590c929357415cce1aee2984355a7df251
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003256
> Commit-Queue: Eric Orth <ericorth@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#737109}

TBR=mmenke@chromium.org,ericorth@chromium.org

Change-Id: I2b48afc66abd59eff8a3bec653013c6e3a6f9738
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1022059
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031040Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737274}

net/dns/BUILD.gn

@@ -75,8 +75,6 @@ source_set("dns") {
       "notify_watcher_mac.h",
       "record_parsed.cc",
       "record_rdata.cc",
-      "resolve_context.cc",
-      "resolve_context.h",
       "serial_worker.cc",
       "serial_worker.h",
       "system_dns_config_change_notifier.cc",
@@ -389,7 +387,6 @@ source_set("tests") {
     "mapped_host_resolver_unittest.cc",
     "record_parsed_unittest.cc",
     "record_rdata_unittest.cc",
-    "resolve_context_unittest.cc",
     "serial_worker_unittest.cc",
     "system_dns_config_change_notifier_unittest.cc",
   ]

net/dns/dns_session.cc

@@ -241,15 +241,6 @@ unsigned DnsSession::NumAvailableDohServers() {
   return count;
 }
 
-base::Time DnsSession::GetLastDohFailure(size_t server_index) {
-  return GetServerStats(server_index, true /* is_doh_server */)->last_failure;
-}
-
-int DnsSession::GetLastDohFailureCount(size_t server_index) {
-  return GetServerStats(server_index, true /* is_doh_server */)
-      ->last_failure_count;
-}
-
 DnsSession::ServerStats* DnsSession::GetServerStats(unsigned server_index,
                                                     bool is_doh_server) {
   DCHECK_GE(server_index, 0u);

net/dns/dns_session.h

@@ -102,10 +102,6 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
   // Returns the number of DoH servers with successful probe states.
   unsigned NumAvailableDohServers();
 
-  // TODO(b/1022059): Remove once all server stats are moved to ResolveContext.
-  base::Time GetLastDohFailure(size_t server_index);
-  int GetLastDohFailureCount(size_t server_index);
-
   // Record that server failed to respond (due to SRV_FAIL or timeout). If
   // |is_doh_server| and the number of failures has surpassed a threshold,
   // sets the DoH probe state to unavailable.
@@ -177,9 +173,6 @@ class NET_EXPORT_PRIVATE DnsSession : public base::RefCounted<DnsSession> {
   base::TimeDelta initial_timeout_;
   base::TimeDelta max_timeout_;
 
-  // TODO(crbug.com/1022059): Move all handling of ServerStats (both for DoH and
-  // non-DoH) to ResolveContext.
-
   // Track runtime statistics of each insecure DNS server.
   std::vector<std::unique_ptr<ServerStats>> server_stats_;
 

net/dns/resolve_context.cc

-// Copyright 2020 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 "net/dns/resolve_context.h"
-
-#include "base/logging.h"
-#include "base/time/time.h"
-#include "net/base/network_change_notifier.h"
-#include "net/dns/dns_session.h"
-
-namespace net {
-
-ResolveContext::ResolveContext(URLRequestContext* url_request_context)
-    : url_request_context_(url_request_context) {
-  DCHECK(url_request_context_);
-}
-
-ResolveContext::~ResolveContext() = default;
-
-void ResolveContext::SetCurrentSession(DnsSession* session) {
-  DCHECK(!IsCurrentSession(session));
-
-  current_session_ = session;
-
-  doh_server_availability_.clear();
-  if (session) {
-    doh_server_availability_.insert(
-        doh_server_availability_.begin(),
-        session->config().dns_over_https_servers.size(), false);
-  }
-}
-
-base::Optional<size_t> ResolveContext::DohServerIndexToUse(
-    size_t starting_doh_server_index,
-    DnsConfig::SecureDnsMode secure_dns_mode,
-    const DnsSession* session) {
-  if (!IsCurrentSession(session))
-    return base::nullopt;
-
-  DCHECK_LT(starting_doh_server_index,
-            session->config().dns_over_https_servers.size());
-  size_t index = starting_doh_server_index;
-  base::Time oldest_server_failure;
-  base::Optional<size_t> oldest_available_server_failure_index;
-
-  do {
-    // For a server to be considered "available", the server must have a
-    // successful probe status if we are in AUTOMATIC mode.
-    if (secure_dns_mode == DnsConfig::SecureDnsMode::SECURE ||
-        doh_server_availability_[index]) {
-      // If number of failures on this server doesn't exceed |config_.attempts|,
-      // return its index. |config_.attempts| will generally be more restrictive
-      // than |kAutomaticModeFailureLimit|, although this is not guaranteed.
-      if (current_session_->GetLastDohFailureCount(index) <
-          session->config().attempts) {
-        return index;
-      }
-      // Track oldest failed available server.
-      base::Time cur_server_failure =
-          current_session_->GetLastDohFailure(index);
-      if (!oldest_available_server_failure_index ||
-          cur_server_failure < oldest_server_failure) {
-        oldest_server_failure = cur_server_failure;
-        oldest_available_server_failure_index = index;
-      }
-    }
-    index = (index + 1) % session->config().dns_over_https_servers.size();
-  } while (index != starting_doh_server_index);
-
-  // If we are here it means that there are either no available DoH servers or
-  // that all available DoH servers have at least |config_.attempts| consecutive
-  // failures. In the latter case, we'll return the available DoH server that
-  // failed least recently. In the former case we return nullopt.
-  return oldest_available_server_failure_index;
-}
-
-size_t ResolveContext::NumAvailableDohServers(const DnsSession* session) const {
-  if (!IsCurrentSession(session))
-    return 0;
-
-  size_t count = 0;
-  for (const auto& probe_result : doh_server_availability_) {
-    if (probe_result)
-      count++;
-  }
-  return count;
-}
-
-bool ResolveContext::GetDohServerAvailability(size_t doh_server_index,
-                                              const DnsSession* session) const {
-  if (!IsCurrentSession(session))
-    return false;
-
-  DCHECK_LT(doh_server_index, doh_server_availability_.size());
-
-  return doh_server_availability_[doh_server_index];
-}
-
-void ResolveContext::SetProbeSuccess(size_t doh_server_index,
-                                     bool success,
-                                     const DnsSession* session) {
-  if (!IsCurrentSession(session))
-    return;
-
-  DCHECK_LT(doh_server_index, session->config().dns_over_https_servers.size());
-
-  bool doh_available_before = NumAvailableDohServers(session) > 0;
-  doh_server_availability_[doh_server_index] = success;
-
-  // TODO(crbug.com/1022059): Consider figuring out some way to only for the
-  // first context enabling DoH or the last context disabling DoH.
-  if (doh_available_before != NumAvailableDohServers(session) > 0)
-    NetworkChangeNotifier::TriggerNonSystemDnsChange();
-}
-
-bool ResolveContext::IsCurrentSession(const DnsSession* session) const {
-  DCHECK(session);
-  return session == current_session_;
-}
-
-}  // namespace net

net/dns/resolve_context.h

-// Copyright 2020 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.
-
-#ifndef NET_DNS_RESOLVE_CONTEXT_H_
-#define NET_DNS_RESOLVE_CONTEXT_H_
-
-#include <vector>
-
-#include "base/optional.h"
-#include "net/base/net_export.h"
-#include "net/dns/dns_config.h"
-
-namespace net {
-
-class DnsSession;
-class URLRequestContext;
-
-// Per-URLRequestContext data used by HostResolver. Expected to be owned by the
-// ContextHostResolver, and all usage/references are expected to be cleaned up
-// or cancelled before the URLRequestContext goes out of service.
-class NET_EXPORT_PRIVATE ResolveContext {
- public:
-  explicit ResolveContext(URLRequestContext* url_request_context);
-
-  ResolveContext(const ResolveContext&) = delete;
-  ResolveContext& operator=(const ResolveContext&) = delete;
-
-  ~ResolveContext();
-
-  // Sets the "current" session for this ResolveContext and clears all session-
-  // specific properties.
-  void SetCurrentSession(DnsSession* current_session);
-
-  // Find the index of a DoH server to use for this attempt. Starts from
-  // |starting_doh_server_index| and finds the first eligible server (wrapping
-  // around as necessary) below failure limits, or of no eligible servers are
-  // below failure limits, the one with the oldest last failure. If in AUTOMATIC
-  // mode, a server is only eligible after a successful DoH probe. Returns
-  // nullopt if there are no eligible DoH servers or |session| is not the
-  // current session.
-  base::Optional<size_t> DohServerIndexToUse(
-      size_t starting_doh_server_index,
-      DnsConfig::SecureDnsMode secure_dns_mode,
-      const DnsSession* session);
-
-  // Returns the number of DoH servers with successful probe states. Always 0 if
-  // |session| is not the current session.
-  size_t NumAvailableDohServers(const DnsSession* session) const;
-
-  // Returns whether |doh_server_index| is marked available. Always |false| if
-  // |session| is not the current session.
-  bool GetDohServerAvailability(size_t doh_server_index,
-                                const DnsSession* session) const;
-
-  // Record the latest DoH probe state. Noop if |session| is not the current
-  // session.
-  void SetProbeSuccess(size_t doh_server_index,
-                       bool success,
-                       const DnsSession* session);
-
-  URLRequestContext* url_request_context() { return url_request_context_; }
-
- private:
-  bool IsCurrentSession(const DnsSession* session) const;
-
-  URLRequestContext* url_request_context_;
-
-  // Per-session data is only stored and valid for the latest session. Before
-  // accessing, should check that |current_session_| is valid and matches a
-  // passed in DnsSession.
-  //
-  // TODO(crbug.com/1022059): Make const DnsSession once server stats have been
-  // moved and no longer need to be read from DnsSession for availability logic.
-  DnsSession* current_session_;
-  std::vector<bool> doh_server_availability_;
-};
-
-}  // namespace net
-
-#endif  // NET_DNS_RESOLVE_CONTEXT_H_

net/dns/resolve_context_unittest.cc

-// Copyright 2020 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 "net/dns/resolve_context.h"
-
-#include <stdint.h>
-#include <memory>
-#include <string>
-#include <utility>
-
-#include "base/bind.h"
-#include "base/memory/ref_counted.h"
-#include "base/run_loop.h"
-#include "base/strings/stringprintf.h"
-#include "net/base/ip_address.h"
-#include "net/base/ip_endpoint.h"
-#include "net/base/mock_network_change_notifier.h"
-#include "net/base/network_change_notifier.h"
-#include "net/dns/dns_config.h"
-#include "net/dns/dns_session.h"
-#include "net/dns/dns_socket_pool.h"
-#include "net/dns/public/dns_protocol.h"
-#include "net/socket/socket_test_util.h"
-#include "net/test/test_with_task_environment.h"
-#include "net/url_request/url_request_context.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace net {
-
-namespace {
-
-class ResolveContextTest : public TestWithTaskEnvironment {
- public:
-  ResolveContextTest() = default;
-
-  scoped_refptr<DnsSession> CreateDnsSession(const DnsConfig& config) {
-    auto null_random_callback =
-        base::BindRepeating([](int, int) -> int { IMMEDIATE_CRASH(); });
-    std::unique_ptr<DnsSocketPool> dns_socket_pool =
-        DnsSocketPool::CreateNull(socket_factory_.get(), null_random_callback);
-
-    return base::MakeRefCounted<DnsSession>(config, std::move(dns_socket_pool),
-                                            null_random_callback,
-                                            nullptr /* netlog */);
-  }
-
- private:
-  std::unique_ptr<MockClientSocketFactory> socket_factory_ =
-      std::make_unique<MockClientSocketFactory>();
-};
-
-DnsConfig CreateDnsConfig(int num_servers, int num_doh_servers) {
-  DnsConfig config;
-  for (int i = 0; i < num_servers; ++i) {
-    IPEndPoint dns_endpoint(IPAddress(192, 168, 1, static_cast<uint8_t>(i)),
-                            dns_protocol::kDefaultPort);
-    config.nameservers.push_back(dns_endpoint);
-  }
-  for (int i = 0; i < num_doh_servers; ++i) {
-    std::string server_template(
-        base::StringPrintf("https://mock.http/doh_test_%d{?dns}", i));
-    config.dns_over_https_servers.push_back(DnsConfig::DnsOverHttpsServerConfig(
-        server_template, true /* is_post */));
-  }
-
-  return config;
-}
-
-TEST_F(ResolveContextTest, NoCurrentSession) {
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-
-  context.SetProbeSuccess(1u, true, session.get());
-
-  EXPECT_EQ(base::nullopt,
-            context.DohServerIndexToUse(0, DnsConfig::SecureDnsMode::AUTOMATIC,
-                                        session.get()));
-  EXPECT_EQ(0u, context.NumAvailableDohServers(session.get()));
-  EXPECT_FALSE(context.GetDohServerAvailability(1, session.get()));
-}
-
-TEST_F(ResolveContextTest, DifferentSession) {
-  DnsConfig config1 =
-      CreateDnsConfig(1 /* num_servers */, 3 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session1 = CreateDnsSession(config1);
-
-  DnsConfig config2 =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session2 = CreateDnsSession(config2);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session2.get());
-
-  // Use current session to set a probe result.
-  context.SetProbeSuccess(1u, true, session2.get());
-
-  EXPECT_EQ(base::nullopt,
-            context.DohServerIndexToUse(0, DnsConfig::SecureDnsMode::AUTOMATIC,
-                                        session1.get()));
-  EXPECT_EQ(0u, context.NumAvailableDohServers(session1.get()));
-  EXPECT_FALSE(context.GetDohServerAvailability(1u, session1.get()));
-
-  // Different session for SetProbeResult should have no effect.
-  ASSERT_TRUE(context.GetDohServerAvailability(1u, session2.get()));
-  context.SetProbeSuccess(1u, false, session1.get());
-  EXPECT_TRUE(context.GetDohServerAvailability(1u, session2.get()));
-}
-
-TEST_F(ResolveContextTest, DohServerAvailability_InitialAvailability) {
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session.get());
-
-  EXPECT_EQ(context.NumAvailableDohServers(session.get()), 0u);
-  EXPECT_EQ(base::nullopt,
-            context.DohServerIndexToUse(0u, DnsConfig::SecureDnsMode::AUTOMATIC,
-                                        session.get()));
-}
-
-TEST_F(ResolveContextTest, DohServerAvailability_ProbeSuccess) {
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session.get());
-
-  ASSERT_EQ(context.NumAvailableDohServers(session.get()), 0u);
-
-  context.SetProbeSuccess(1, true /* success */, session.get());
-  EXPECT_EQ(context.NumAvailableDohServers(session.get()), 1u);
-  EXPECT_THAT(context.DohServerIndexToUse(
-                  0u, DnsConfig::SecureDnsMode::AUTOMATIC, session.get()),
-              testing::Optional(1u));
-}
-
-TEST_F(ResolveContextTest, DohServerAvailability_ProbeFailure) {
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session.get());
-
-  context.SetProbeSuccess(1, true /* success */, session.get());
-  ASSERT_EQ(context.NumAvailableDohServers(session.get()), 1u);
-
-  context.SetProbeSuccess(1, false /* success */, session.get());
-  EXPECT_EQ(context.NumAvailableDohServers(session.get()), 0u);
-  EXPECT_EQ(base::nullopt,
-            context.DohServerIndexToUse(0u, DnsConfig::SecureDnsMode::AUTOMATIC,
-                                        session.get()));
-}
-
-TEST_F(ResolveContextTest, DohServerIndexToUse) {
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session.get());
-
-  context.SetProbeSuccess(0u, true /* success */, session.get());
-  EXPECT_THAT(context.DohServerIndexToUse(
-                  0u, DnsConfig::SecureDnsMode::AUTOMATIC, session.get()),
-              testing::Optional(0u));
-  EXPECT_THAT(context.DohServerIndexToUse(
-                  1u, DnsConfig::SecureDnsMode::AUTOMATIC, session.get()),
-              testing::Optional(0u));
-}
-
-TEST_F(ResolveContextTest, DohServerIndexToUse_NoneEligible) {
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session.get());
-
-  EXPECT_EQ(base::nullopt,
-            context.DohServerIndexToUse(0u, DnsConfig::SecureDnsMode::AUTOMATIC,
-                                        session.get()));
-  EXPECT_EQ(base::nullopt,
-            context.DohServerIndexToUse(1u, DnsConfig::SecureDnsMode::AUTOMATIC,
-                                        session.get()));
-}
-
-TEST_F(ResolveContextTest, DohServerIndexToUse_SecureMode) {
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session.get());
-
-  EXPECT_THAT(context.DohServerIndexToUse(0u, DnsConfig::SecureDnsMode::SECURE,
-                                          session.get()),
-              testing::Optional(0u));
-  EXPECT_THAT(context.DohServerIndexToUse(1u, DnsConfig::SecureDnsMode::SECURE,
-                                          session.get()),
-              testing::Optional(1u));
-}
-
-class TestDnsObserver : public NetworkChangeNotifier::DNSObserver {
- public:
-  void OnDNSChanged() override { ++dns_changed_calls_; }
-
-  int dns_changed_calls() const { return dns_changed_calls_; }
-
- private:
-  int dns_changed_calls_ = 0;
-};
-
-TEST_F(ResolveContextTest, DohServerAvailabilityNotification) {
-  test::ScopedMockNetworkChangeNotifier mock_network_change_notifier;
-  TestDnsObserver config_observer;
-  NetworkChangeNotifier::AddDNSObserver(&config_observer);
-
-  DnsConfig config =
-      CreateDnsConfig(2 /* num_servers */, 2 /* num_doh_servers */);
-  scoped_refptr<DnsSession> session = CreateDnsSession(config);
-
-  URLRequestContext request_context;
-  ResolveContext context(&request_context);
-  context.SetCurrentSession(session.get());
-
-  base::RunLoop().RunUntilIdle();  // Notifications are async.
-  EXPECT_EQ(0, config_observer.dns_changed_calls());
-
-  // Expect notification on first available DoH server.
-  context.SetProbeSuccess(0, true /* success */, session.get());
-  base::RunLoop().RunUntilIdle();  // Notifications are async.
-  EXPECT_EQ(1, config_observer.dns_changed_calls());
-
-  // No notifications as additional servers are available or unavailable.
-  context.SetProbeSuccess(1, true /* success */, session.get());
-  context.SetProbeSuccess(0, false /* success */, session.get());
-  base::RunLoop().RunUntilIdle();  // Notifications are async.
-  EXPECT_EQ(1, config_observer.dns_changed_calls());
-
-  // Expect notification on last server unavailable.
-  context.SetProbeSuccess(1, false /* success */, session.get());
-  base::RunLoop().RunUntilIdle();  // Notifications are async.
-  EXPECT_EQ(2, config_observer.dns_changed_calls());
-
-  NetworkChangeNotifier::RemoveDNSObserver(&config_observer);
-}
-
-}  // namespace
-}  // namespace net