Remove HostResolver's CacheHitCallback logic.

It was landed almost a year ago, but was never used. If it turns out we
really do need it, can bring it back.

Bug: none
Change-Id: I8635219b8a3af3c46711ff7931403bf32854f351
Reviewed-on: https://chromium-review.googlesource.com/580387Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488460}

net/dns/host_resolver.cc

@@ -102,8 +102,7 @@ HostResolver::RequestInfo::RequestInfo(const RequestInfo& request_info)
       host_resolver_flags_(request_info.host_resolver_flags_),
       allow_cached_response_(request_info.allow_cached_response_),
       is_speculative_(request_info.is_speculative_),
-      is_my_ip_address_(request_info.is_my_ip_address_),
-      cache_hit_callback_(request_info.cache_hit_callback_) {}
+      is_my_ip_address_(request_info.is_my_ip_address_) {}
 
 HostResolver::RequestInfo::~RequestInfo() {}
 

net/dns/host_resolver.h

@@ -109,14 +109,6 @@ class NET_EXPORT HostResolver {
     bool is_my_ip_address() const { return is_my_ip_address_; }
     void set_is_my_ip_address(bool b) { is_my_ip_address_ = b; }
 
-    using CacheHitCallback = base::Callback<void(const RequestInfo&)>;
-    const CacheHitCallback& cache_hit_callback() const {
-      return cache_hit_callback_;
-    }
-    void set_cache_hit_callback(const CacheHitCallback& callback) {
-      cache_hit_callback_ = callback;
-    }
-
    private:
     RequestInfo();
 
@@ -138,10 +130,6 @@ class NET_EXPORT HostResolver {
     // Indicates a request for myIpAddress (to differentiate from other requests
     // for localhost, currently used by Chrome OS).
     bool is_my_ip_address_;
-
-    // A callback that will be called when another request reads the cache data
-    // returned (and possibly written) by this request.
-    CacheHitCallback cache_hit_callback_;
   };
 
   // Set Options.max_concurrent_resolves to this to select a default level

net/dns/host_resolver_impl.cc

@@ -1817,12 +1817,8 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
 
     bool did_complete = (entry.error() != ERR_NETWORK_CHANGED) &&
                         (entry.error() != ERR_HOST_RESOLVER_QUEUE_TOO_LARGE);
-    if (did_complete) {
+    if (did_complete)
       resolver_->CacheResult(key_, entry, ttl);
-      // Erase any previous cache hit callbacks, since a new DNS request went
-      // out since they were set.
-      resolver_->cache_hit_callbacks_.erase(key_);
-    }
 
     // Complete all of the requests that were attached to the job and
     // detach them.
@@ -1833,7 +1829,6 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
       // Update the net log and notify registered observers.
       LogFinishRequest(req->source_net_log(), req->info(), entry.error());
       if (did_complete) {
-        resolver_->MaybeAddCacheHitCallback(key_, req->info());
         // Record effective total time from creation to completion.
         RecordTotalTime(had_dns_config_, req->info().is_speculative(),
                         base::TimeTicks::Now() - req->request_time());
@@ -1967,9 +1962,6 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   Key key;
   int rv = ResolveHelper(info, false, nullptr, source_net_log, addresses, &key);
   if (rv != ERR_DNS_CACHE_MISS) {
-    // TODO(mmenke): Remove this code. Nothing's using it.
-    if (rv == OK)
-      MaybeAddCacheHitCallback(key, info);
     LogFinishRequest(source_net_log, info, rv);
     RecordTotalTime(HaveDnsConfig(), info.is_speculative(), base::TimeDelta());
     return rv;
@@ -2112,7 +2104,6 @@ int HostResolverImpl::ResolveHelper(const RequestInfo& info,
     source_net_log.AddEvent(NetLogEventType::HOST_RESOLVER_IMPL_CACHE_HIT,
                             addresses->CreateNetLogCallback());
     // |ServeFromCache()| will set |*stale_info| as needed.
-    RunCacheHitCallbacks(*key, info);
     return net_error;
   }
   // TODO(szym): Do not do this if nsswitch.conf instructs not to.
@@ -2507,10 +2498,8 @@ void HostResolverImpl::OnIPAddressChanged() {
   last_ipv6_probe_time_ = base::TimeTicks();
   // Abandon all ProbeJobs.
   probe_weak_ptr_factory_.InvalidateWeakPtrs();
-  if (cache_.get()) {
+  if (cache_.get())
     cache_->OnNetworkChange();
-    cache_hit_callbacks_.clear();
-  }
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
   RunLoopbackProbeJob();
 #endif
@@ -2568,10 +2557,8 @@ void HostResolverImpl::UpdateDNSConfig(bool config_changed) {
     // have to expire our internal cache :( Note that OS level DNS caches, such
     // as NSCD's cache should be dropped automatically by the OS when
     // resolv.conf changes so we don't need to do anything to clear that cache.
-    if (cache_.get()) {
+    if (cache_.get())
       cache_->OnNetworkChange();
-      cache_hit_callbacks_.clear();
-    }
 
     // Life check to bail once |this| is deleted.
     base::WeakPtr<HostResolverImpl> self = weak_ptr_factory_.GetWeakPtr();
@@ -2617,28 +2604,6 @@ void HostResolverImpl::OnDnsTaskResolve(int net_error) {
                               std::abs(net_error));
 }
 
-void HostResolverImpl::OnCacheEntryEvicted(const HostCache::Key& key,
-                                           const HostCache::Entry& entry) {
-  cache_hit_callbacks_.erase(key);
-}
-
-void HostResolverImpl::MaybeAddCacheHitCallback(const HostCache::Key& key,
-                                                const RequestInfo& info) {
-  const RequestInfo::CacheHitCallback& callback = info.cache_hit_callback();
-  if (callback.is_null())
-    return;
-  cache_hit_callbacks_[key].push_back(callback);
-}
-
-void HostResolverImpl::RunCacheHitCallbacks(const HostCache::Key& key,
-                                            const RequestInfo& info) {
-  auto it = cache_hit_callbacks_.find(key);
-  if (it == cache_hit_callbacks_.end())
-    return;
-  for (auto& callback : it->second)
-    callback.Run(info);
-}
-
 void HostResolverImpl::SetDnsClient(std::unique_ptr<DnsClient> dns_client) {
   // DnsClient and config must be updated before aborting DnsTasks, since doing
   // so may start new jobs.

net/dns/host_resolver_impl.h

@@ -309,13 +309,6 @@ class NET_EXPORT HostResolverImpl
   // and resulted in |net_error|.
   void OnDnsTaskResolve(int net_error);
 
-  void OnCacheEntryEvicted(const HostCache::Key& key,
-                           const HostCache::Entry& entry);
-  void ClearCacheHitCallbacks(const HostCache::Key& key);
-  void MaybeAddCacheHitCallback(const HostCache::Key& key,
-                                const RequestInfo& info);
-  void RunCacheHitCallbacks(const HostCache::Key& key, const RequestInfo& info);
-
   void ApplyPersistentData(std::unique_ptr<const base::Value>);
   std::unique_ptr<const base::Value> GetPersistentData();
 
@@ -378,9 +371,6 @@ class NET_EXPORT HostResolverImpl
   // tasks, but can be overridden for tests.
   scoped_refptr<base::TaskRunner> worker_task_runner_;
 
-  std::map<const HostCache::Key, std::vector<RequestInfo::CacheHitCallback>>
-      cache_hit_callbacks_;
-
   bool persist_initialized_;
   PersistCallback persist_callback_;
   base::OneShotTimer persist_timer_;

net/dns/host_resolver_impl_unittest.cc

@@ -2578,63 +2578,4 @@ TEST_F(HostResolverImplTest, ResolveLocalHostname) {
       ResolveLocalHostname("foo.localhoste", kLocalhostLookupPort, &addresses));
 }
 
-void TestCacheHitCallback(int* callback_count,
-                          HostResolver::RequestInfo* last_request_info,
-                          const HostResolver::RequestInfo& request_info) {
-  ++*callback_count;
-  *last_request_info = request_info;
-}
-
-TEST_F(HostResolverImplTest, CacheHitCallback) {
-  proc_->AddRuleForAllFamilies("just.testing", "192.168.1.42");
-  proc_->SignalMultiple(5u);
-
-  HostResolver::RequestInfo last_request_info(HostPortPair("unassigned", 80));
-
-  // Set a cache hit callback.
-  int count1 = 0;
-  HostResolver::RequestInfo info_callback1(HostPortPair("just.testing", 80));
-  info_callback1.set_cache_hit_callback(
-      base::Bind(&TestCacheHitCallback, &count1, &last_request_info));
-  Request* req = CreateRequest(info_callback1, MEDIUM);
-  EXPECT_THAT(req->Resolve(), IsError(ERR_IO_PENDING));
-  EXPECT_THAT(req->WaitForResult(), IsOk());
-  EXPECT_EQ(0, count1);
-
-  // Make sure the cache hit callback is called, and set another one.
-  // Future requests should call *both* callbacks.
-  int count2 = 0;
-  HostResolver::RequestInfo info_callback2(HostPortPair("just.testing", 80));
-  info_callback2.set_cache_hit_callback(
-      base::Bind(&TestCacheHitCallback, &count2, &last_request_info));
-  req = CreateRequest(info_callback2, MEDIUM);
-  EXPECT_THAT(req->Resolve(), IsOk());
-  EXPECT_EQ(1, count1);
-  EXPECT_EQ(0, count2);
-
-  // Make another request to make sure both callbacks are called.
-  req = CreateRequest("just.testing", 80);
-  EXPECT_THAT(req->Resolve(), IsOk());
-  EXPECT_EQ(2, count1);
-  EXPECT_EQ(1, count2);
-
-  // Make an uncached request to clear the cache hit callbacks.
-  // (They should be cleared because the uncached request will write a new
-  // result into the cache.)
-  // It should not call the callbacks itself, since it doesn't hit the cache.
-  HostResolver::RequestInfo info_uncached(HostPortPair("just.testing", 80));
-  info_uncached.set_allow_cached_response(false);
-  req = CreateRequest(info_uncached, MEDIUM);
-  EXPECT_THAT(req->Resolve(), IsError(ERR_IO_PENDING));
-  EXPECT_THAT(req->WaitForResult(), IsOk());
-  EXPECT_EQ(2, count1);
-  EXPECT_EQ(1, count2);
-
-  // Make another request to make sure both callbacks were cleared.
-  req = CreateRequest("just.testing", 80);
-  EXPECT_THAT(req->Resolve(), IsOk());
-  EXPECT_EQ(2, count1);
-  EXPECT_EQ(1, count2);
-}
-
 }  // namespace net