Allow non-address results in HostCache::Entry

Changed addresses_ to an Optional and added additional optional fields
for text results (eg for TXT records) and hostname results (eg for SRV
or PTR records). Deliberately avoided assumptions that only one result
field could be non-nullopt at at time (but no constructor currently
allows creating it) because there's nothing wrong with some query
having results of multiple types. Eg, we may in the future choose to
pull cannonname out of AddressList and set it as a hostname result.

A subsequent CL will alter HostResolverImpl to allow querying, caching,
and returning non-address types, and it will also use HostCache::Entry
internally (and only internally) as a results container. For now, in
this CL, most of the code dealing with HostCache results will just
assume Entry::addresses() always has a value.

Bug: 846423
Change-Id: I62e177452bf89e5931f75a74448f9de70b7f2357
Reviewed-on: https://chromium-review.googlesource.com/c/1331143
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609373}

net/base/host_port_pair.h

@@ -40,6 +40,8 @@ class NET_EXPORT HostPortPair {
     return std::tie(port_, host_) < std::tie(other.port_, other.host_);
   }
 
+  bool operator==(const HostPortPair& other) const { return Equals(other); }
+
   // Equality test of contents. (Probably another violation of style guide).
   bool Equals(const HostPortPair& other) const {
     return host_ == other.host_ && port_ == other.port_;

net/dns/host_cache.h

@@ -12,14 +12,19 @@
 #include <memory>
 #include <string>
 #include <tuple>
+#include <utility>
+#include <vector>
 
 #include "base/gtest_prod_util.h"
+#include "base/logging.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "net/base/address_family.h"
 #include "net/base/address_list.h"
 #include "net/base/expiring_cache.h"
+#include "net/base/host_port_pair.h"
 #include "net/base/net_export.h"
 #include "net/dns/dns_util.h"
 #include "net/dns/host_resolver_source.h"
@@ -89,17 +94,33 @@ class NET_EXPORT HostCache {
       SOURCE_HOSTS,
     };
 
-    Entry(int error,
-          const AddressList& addresses,
-          Source source,
-          base::TimeDelta ttl);
+    template <typename T>
+    Entry(int error, T&& results, Source source, base::TimeDelta ttl)
+        : error_(error), source_(source), ttl_(ttl) {
+      DCHECK(ttl >= base::TimeDelta());
+      SetResult(std::forward<T>(results));
+    }
+
     // Use when |ttl| is unknown.
-    Entry(int error, const AddressList& addresses, Source source);
+    template <typename T>
+    Entry(int error, T&& results, Source source)
+        : error_(error),
+          source_(source),
+          ttl_(base::TimeDelta::FromSeconds(-1)) {
+      SetResult(std::forward<T>(results));
+    }
+
     Entry(Entry&& entry);
     ~Entry();
 
     int error() const { return error_; }
-    const AddressList& addresses() const { return addresses_; }
+    const base::Optional<AddressList>& addresses() const { return addresses_; }
+    const base::Optional<std::vector<std::string>>& text_records() const {
+      return text_records_;
+    }
+    const base::Optional<std::vector<HostPortPair>>& hostnames() const {
+      return hostnames_;
+    }
     Source source() const { return source_; }
     bool has_ttl() const { return ttl_ >= base::TimeDelta(); }
     base::TimeDelta ttl() const { return ttl_; }
@@ -118,11 +139,21 @@ class NET_EXPORT HostCache {
           int network_changes);
 
     Entry(int error,
-          const AddressList& addresses,
+          const base::Optional<AddressList>& addresses,
+          base::Optional<std::vector<std::string>>&& text_results,
+          base::Optional<std::vector<HostPortPair>>&& hostnames,
           Source source,
           base::TimeTicks expires,
           int network_changes);
 
+    void SetResult(AddressList addresses) { addresses_ = std::move(addresses); }
+    void SetResult(std::vector<std::string> text_records) {
+      text_records_ = std::move(text_records);
+    }
+    void SetResult(std::vector<HostPortPair> hostnames) {
+      hostnames_ = std::move(hostnames);
+    }
+
     int total_hits() const { return total_hits_; }
     int stale_hits() const { return stale_hits_; }
 
@@ -134,8 +165,10 @@ class NET_EXPORT HostCache {
 
     // The resolve results for this entry.
     int error_;
-    AddressList addresses_;
-    // Where addresses_ were obtained (e.g. DNS lookup, hosts file, etc).
+    base::Optional<AddressList> addresses_;
+    base::Optional<std::vector<std::string>> text_records_;
+    base::Optional<std::vector<HostPortPair>> hostnames_;
+    // Where results were obtained (e.g. DNS lookup, hosts file, etc).
     Source source_;
     // TTL obtained from the nameserver. Negative if unknown.
     base::TimeDelta ttl_;
@@ -243,13 +276,13 @@ class NET_EXPORT HostCache {
 
   Entry* LookupInternal(const Key& key);
 
-  void RecordSet(SetOutcome outcome,
-                 base::TimeTicks now,
-                 const Entry* old_entry,
-                 const Entry& new_entry,
-                 AddressListDeltaType delta);
-  void RecordUpdateStale(AddressListDeltaType delta,
-                         const EntryStaleness& stale);
+  void LogRecordSet(SetOutcome outcome,
+                    base::TimeTicks now,
+                    const Entry* old_entry,
+                    const Entry& new_entry,
+                    AddressListDeltaType delta);
+  void LogRecordUpdateStale(AddressListDeltaType delta,
+                            const EntryStaleness& stale);
   void RecordLookup(LookupOutcome outcome,
                     base::TimeTicks now,
                     const Entry* entry);

net/dns/host_cache_unittest.cc

@@ -694,8 +694,11 @@ TEST(HostCacheTest, SerializeAndDeserialize) {
   const HostCache::Entry* result1 =
       restored_cache.LookupStale(key1, now, &stale);
   EXPECT_TRUE(result1);
-  EXPECT_EQ(1u, result1->addresses().size());
-  EXPECT_EQ(address_ipv4, result1->addresses().front().address());
+  ASSERT_TRUE(result1->addresses());
+  EXPECT_FALSE(result1->text_records());
+  EXPECT_FALSE(result1->hostnames());
+  EXPECT_EQ(1u, result1->addresses().value().size());
+  EXPECT_EQ(address_ipv4, result1->addresses().value().front().address());
   EXPECT_EQ(1, stale.network_changes);
   // Time to TimeTicks conversion is fuzzy, so just check that expected and
   // actual expiration times are close.
@@ -707,9 +710,10 @@ TEST(HostCacheTest, SerializeAndDeserialize) {
   const HostCache::Entry* result2 =
       restored_cache.LookupStale(key2, now, &stale);
   EXPECT_TRUE(result2);
-  EXPECT_EQ(2u, result2->addresses().size());
-  EXPECT_EQ(address_ipv6, result2->addresses().front().address());
-  EXPECT_EQ(address_ipv4, result2->addresses().back().address());
+  ASSERT_TRUE(result2->addresses());
+  EXPECT_EQ(2u, result2->addresses().value().size());
+  EXPECT_EQ(address_ipv6, result2->addresses().value().front().address());
+  EXPECT_EQ(address_ipv4, result2->addresses().value().back().address());
   EXPECT_EQ(1, stale.network_changes);
   EXPECT_GT(base::TimeDelta::FromMilliseconds(100),
             (base::TimeDelta::FromSeconds(-3) - stale.expired_by).magnitude());
@@ -717,18 +721,77 @@ TEST(HostCacheTest, SerializeAndDeserialize) {
   // The "foobar3.com" entry is the new one, not the restored one.
   const HostCache::Entry* result3 = restored_cache.Lookup(key3, now);
   EXPECT_TRUE(result3);
-  EXPECT_EQ(1u, result3->addresses().size());
-  EXPECT_EQ(address_ipv4, result3->addresses().front().address());
+  ASSERT_TRUE(result3->addresses());
+  EXPECT_EQ(1u, result3->addresses().value().size());
+  EXPECT_EQ(address_ipv4, result3->addresses().value().front().address());
 
   // The "foobar4.com" entry is still present and usable.
   const HostCache::Entry* result4 = restored_cache.Lookup(key4, now);
   EXPECT_TRUE(result4);
-  EXPECT_EQ(1u, result4->addresses().size());
-  EXPECT_EQ(address_ipv4, result4->addresses().front().address());
+  ASSERT_TRUE(result4->addresses());
+  EXPECT_EQ(1u, result4->addresses().value().size());
+  EXPECT_EQ(address_ipv4, result4->addresses().value().front().address());
 
   EXPECT_EQ(3u, restored_cache.last_restore_size());
 }
 
+TEST(HostCacheTest, SerializeAndDeserialize_Text) {
+  base::TimeTicks now;
+
+  base::TimeDelta ttl = base::TimeDelta::FromSeconds(99);
+  std::vector<std::string> text_records({"foo", "bar"});
+  HostCache::Key key("example.com", DnsQueryType::A, 0,
+                     HostResolverSource::DNS);
+  HostCache::Entry entry(OK, text_records, HostCache::Entry::SOURCE_DNS, ttl);
+  EXPECT_TRUE(entry.text_records());
+
+  HostCache cache(kMaxCacheEntries);
+  cache.Set(key, entry, now, ttl);
+  EXPECT_EQ(1u, cache.size());
+
+  base::ListValue serialized_cache;
+  cache.GetAsListValue(&serialized_cache, false /* include_staleness */);
+  HostCache restored_cache(kMaxCacheEntries);
+  restored_cache.RestoreFromListValue(serialized_cache);
+
+  ASSERT_EQ(1u, cache.size());
+  const HostCache::Entry* result = cache.Lookup(key, now);
+  ASSERT_TRUE(result);
+  EXPECT_FALSE(result->addresses());
+  ASSERT_TRUE(result->text_records());
+  EXPECT_FALSE(result->hostnames());
+  EXPECT_EQ(text_records, result->text_records().value());
+}
+
+TEST(HostCacheTest, SerializeAndDeserialize_Hostname) {
+  base::TimeTicks now;
+
+  base::TimeDelta ttl = base::TimeDelta::FromSeconds(99);
+  std::vector<HostPortPair> hostnames(
+      {HostPortPair("example.com", 95), HostPortPair("chromium.org", 122)});
+  HostCache::Key key("example.com", DnsQueryType::A, 0,
+                     HostResolverSource::DNS);
+  HostCache::Entry entry(OK, hostnames, HostCache::Entry::SOURCE_DNS, ttl);
+  EXPECT_TRUE(entry.hostnames());
+
+  HostCache cache(kMaxCacheEntries);
+  cache.Set(key, entry, now, ttl);
+  EXPECT_EQ(1u, cache.size());
+
+  base::ListValue serialized_cache;
+  cache.GetAsListValue(&serialized_cache, false /* include_staleness */);
+  HostCache restored_cache(kMaxCacheEntries);
+  restored_cache.RestoreFromListValue(serialized_cache);
+
+  ASSERT_EQ(1u, cache.size());
+  const HostCache::Entry* result = cache.Lookup(key, now);
+  ASSERT_TRUE(result);
+  EXPECT_FALSE(result->addresses());
+  EXPECT_FALSE(result->text_records());
+  ASSERT_TRUE(result->hostnames());
+  EXPECT_EQ(hostnames, result->hostnames().value());
+}
+
 TEST(HostCacheTest, PersistenceDelegate) {
   const base::TimeDelta kTTL = base::TimeDelta::FromSeconds(10);
   HostCache cache(kMaxCacheEntries);

net/dns/host_resolver_impl.cc

@@ -1933,7 +1933,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
       }
       if (entry.error() == OK && !req->parameters().is_speculative) {
         req->set_address_results(EnsurePortOnAddressList(
-            entry.addresses(), req->request_host().port()));
+            entry.addresses().value(), req->request_host().port()));
       }
       req->OnJobCompleted(this, entry.error());
 
@@ -2519,7 +2519,8 @@ bool HostResolverImpl::ServeFromCache(const Key& key,
   if (*net_error == OK) {
     if (cache_entry->has_ttl())
       RecordTTL(cache_entry->ttl());
-    *addresses = EnsurePortOnAddressList(cache_entry->addresses(), host_port);
+    *addresses =
+        EnsurePortOnAddressList(cache_entry->addresses().value(), host_port);
   }
   return true;
 }

net/dns/mock_host_resolver.cc

@@ -455,7 +455,8 @@ int MockHostResolverBase::ResolveFromIPLiteralOrCache(
     if (entry) {
       rv = entry->error();
       if (rv == OK)
-        *addresses = AddressList::CopyWithPort(entry->addresses(), host.port());
+        *addresses =
+            AddressList::CopyWithPort(entry->addresses().value(), host.port());
     }
   }
   return rv;

services/proxy_resolver/host_resolver_mojo.cc

@@ -153,7 +153,8 @@ int HostResolverMojo::ResolveFromCacheInternal(const RequestInfo& info,
   if (!entry)
     return net::ERR_DNS_CACHE_MISS;
 
-  *addresses = net::AddressList::CopyWithPort(entry->addresses(), info.port());
+  *addresses =
+      net::AddressList::CopyWithPort(entry->addresses().value(), info.port());
   return entry->error();
 }