* Add an OnCancelResolution() notifier to HostResolver::Observer, so observers can tell when a request has been cancelled.

* Use OnCancelResolution() in DNS prefetcher observer, to avoid leaking entries in the |resolution| table when requests are cancelled. (BUG=14138)

* Fix a bug where completion notification wasn't being sent when the response was cached. (BUG=14188)

BUG=14138,14188
TEST=HostResolverTest.CancellationObserver, HostResolverTest.Observer

Review URL: http://codereview.chromium.org/125171

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@18520 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 178 insertions, 27 deletions

diff --git a/net/base/host_resolver.cc b/net/base/host_resolver.cc
index 5a357eb..ca742da 100644
--- a/net/base/host_resolver.cc
+++ b/net/base/host_resolver.cc
@@ -184,7 +184,7 @@ class HostResolver::Request {
         addresses_(addresses) {}
 
   // Mark the request as cancelled.
-  void Cancel() {
+  void MarkAsCancelled() {
     job_ = NULL;
     callback_ = NULL;
     addresses_ = NULL;
@@ -289,10 +289,26 @@ class HostResolver::Job : public base::RefCountedThreadSafe<HostResolver::Job> {
 
   // Cancels the current job. Callable from origin thread.
   void Cancel() {
+    HostResolver* resolver = resolver_;
     resolver_ = NULL;
 
-    AutoLock locked(origin_loop_lock_);
-    origin_loop_ = NULL;
+    // Mark the job as cancelled, so when worker thread completes it will
+    // not try to post completion to origin loop.
+    {
+      AutoLock locked(origin_loop_lock_);
+      origin_loop_ = NULL;
+    }
+
+    // We don't have to do anything further to actually cancel the requests
+    // that were attached to this job (since they are unreachable now).
+    // But we will call HostResolver::CancelRequest(Request*) on each one
+    // in order to notify any observers.
+    for (RequestsList::const_iterator it = requests_.begin();
+         it != requests_.end(); ++it) {
+      HostResolver::Request* req = *it;
+      if (!req->was_cancelled())
+        resolver->CancelRequest(req);
+    }
   }
 
   // Called from origin thread.
@@ -390,7 +406,7 @@ HostResolver::HostResolver(int max_cache_entries, int cache_duration_ms)
 
 HostResolver::~HostResolver() {
   // Cancel the outstanding jobs. Those jobs may contain several attached
-  // requests, which will now never be completed.
+  // requests, which will also be cancelled.
   for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ++it)
     it->second->Cancel();
 
@@ -417,7 +433,12 @@ int HostResolver::Resolve(const RequestInfo& info,
         info.hostname(), base::TimeTicks::Now());
     if (cache_entry) {
       addresses->SetFrom(cache_entry->addrlist, info.port());
-      return OK;
+      int error = OK;
+
+      // Notify registered observers.
+      NotifyObserversFinishRequest(request_id, info, error);
+
+      return error;
     }
   }
 
@@ -478,7 +499,8 @@ void HostResolver::CancelRequest(Request* req) {
   DCHECK(req);
   DCHECK(req->job());
   // NULL out the fields of req, to mark it as cancelled.
-  req->Cancel();
+  req->MarkAsCancelled();
+  NotifyObserversCancelRequest(req->id(), req->info());
 }
 
 void HostResolver::AddObserver(Observer* observer) {
@@ -569,6 +591,14 @@ void HostResolver::NotifyObserversFinishRequest(int request_id,
   }
 }
 
+void HostResolver::NotifyObserversCancelRequest(int request_id,
+                                                const RequestInfo& info) {
+  for (ObserversList::iterator it = observers_.begin();
+       it != observers_.end(); ++it) {
+    (*it)->OnCancelResolution(request_id, info);
+  }
+}
+
 //-----------------------------------------------------------------------------
 
 SingleRequestHostResolver::SingleRequestHostResolver(HostResolver* resolver)
diff --git a/net/base/host_resolver.h b/net/base/host_resolver.h
index 34c803f..0b5c3fc 100644
--- a/net/base/host_resolver.h
+++ b/net/base/host_resolver.h
@@ -106,13 +106,18 @@ class HostResolver {
 
     // Called at the start of HostResolver::Resolve(). |id| is a unique number
     // given to the request, so it can be matched up with a corresponding call
-    // to OnFinishResolutionWithStatus().
+    // to OnFinishResolutionWithStatus() or OnCancelResolution().
     virtual void OnStartResolution(int id, const RequestInfo& info) = 0;
 
     // Called on completion of request |id|. Note that if the request was
-    // cancelled, OnFinishResolutionWithStatus() will not be called.
+    // cancelled, OnCancelResolution() will be called instead.
     virtual void OnFinishResolutionWithStatus(int id, bool was_resolved,
                                               const RequestInfo& info) = 0;
+
+    // Called when request |id| has been cancelled. A request is "cancelled"
+    // if either the HostResolver is destroyed while a resolution is in
+    // progress, or HostResolver::CancelRequest() is called.
+    virtual void OnCancelResolution(int id, const RequestInfo& info) = 0;
   };
 
   // Creates a HostResolver that caches up to |max_cache_entries| for
@@ -175,15 +180,19 @@ class HostResolver {
   // Callback for when |job| has completed with |error| and |addrlist|.
   void OnJobComplete(Job* job, int error, const AddressList& addrlist);
 
-  // Notify all obsevers of the start of a resolve request.
+  // Notify all observers of the start of a resolve request.
   void NotifyObserversStartRequest(int request_id,
                                    const RequestInfo& info);
 
-  // Notify all obsevers of the completion of a resolve request.
+  // Notify all observers of the completion of a resolve request.
   void NotifyObserversFinishRequest(int request_id,
                                     const RequestInfo& info,
                                     int error);
 
+  // Notify all observers of the cancellation of a resolve request.
+  void NotifyObserversCancelRequest(int request_id,
+                                    const RequestInfo& info);
+
   // Cache of host resolution results.
   HostCache cache_;
 
diff --git a/net/base/host_resolver_unittest.cc b/net/base/host_resolver_unittest.cc
index 9c07c29..ce2c495 100644
--- a/net/base/host_resolver_unittest.cc
+++ b/net/base/host_resolver_unittest.cc
@@ -20,6 +20,7 @@
 #include "net/base/completion_callback.h"
 #include "net/base/host_resolver_unittest.h"
 #include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using net::RuleBasedHostMapper;
@@ -701,7 +702,7 @@ class CapturingObserver : public net::HostResolver::Observer {
   // DnsResolutionObserver methods:
   virtual void OnStartResolution(int id,
                                  const net::HostResolver::RequestInfo& info) {
-    start_log.push_back(StartEntry(id, info));
+    start_log.push_back(StartOrCancelEntry(id, info));
   }
 
   virtual void OnFinishResolutionWithStatus(
@@ -711,12 +712,17 @@ class CapturingObserver : public net::HostResolver::Observer {
     finish_log.push_back(FinishEntry(id, was_resolved, info));
   }
 
+  virtual void OnCancelResolution(int id,
+                                  const net::HostResolver::RequestInfo& info) {
+    cancel_log.push_back(StartOrCancelEntry(id, info));
+  }
+
   // Tuple (id, info).
-  struct StartEntry {
-    StartEntry(int id, const net::HostResolver::RequestInfo& info)
+  struct StartOrCancelEntry {
+    StartOrCancelEntry(int id, const net::HostResolver::RequestInfo& info)
         : id(id), info(info) {}
 
-    bool operator==(const StartEntry& other) const {
+    bool operator==(const StartOrCancelEntry& other) const {
       return id == other.id && info == other.info;
     }
 
@@ -741,11 +747,14 @@ class CapturingObserver : public net::HostResolver::Observer {
     net::HostResolver::RequestInfo info;
   };
 
-  std::vector<StartEntry> start_log;
+  std::vector<StartOrCancelEntry> start_log;
   std::vector<FinishEntry> finish_log;
+  std::vector<StartOrCancelEntry> cancel_log;
 };
 
 // Test that registering, unregistering, and notifying of observers works.
+// Does not test the cancellation notification since all resolves are
+// synchronous.
 TEST_F(HostResolverTest, Observers) {
   net::HostResolver host_resolver;
 
@@ -757,25 +766,44 @@ TEST_F(HostResolverTest, Observers) {
 
   // Resolve "host1".
   net::HostResolver::RequestInfo info1("host1", 70);
-  host_resolver.Resolve(info1, &addrlist, NULL, NULL);
+  int rv = host_resolver.Resolve(info1, &addrlist, NULL, NULL);
+  EXPECT_EQ(net::OK, rv);
 
   EXPECT_EQ(1U, observer.start_log.size());
   EXPECT_EQ(1U, observer.finish_log.size());
-  EXPECT_TRUE(
-      observer.start_log[0] == CapturingObserver::StartEntry(0, info1));
-  EXPECT_TRUE(
-      observer.finish_log[0] == CapturingObserver::FinishEntry(0, true, info1));
+  EXPECT_EQ(0U, observer.cancel_log.size());
+  EXPECT_TRUE(observer.start_log[0] ==
+              CapturingObserver::StartOrCancelEntry(0, info1));
+  EXPECT_TRUE(observer.finish_log[0] ==
+              CapturingObserver::FinishEntry(0, true, info1));
+
+  // Resolve "host1" again -- this time it  will be served from cache, but it
+  // should still notify of completion.
+  TestCompletionCallback callback;
+  rv = host_resolver.Resolve(info1, &addrlist, &callback, NULL);
+  ASSERT_EQ(net::OK, rv);  // Should complete synchronously.
+
+  EXPECT_EQ(2U, observer.start_log.size());
+  EXPECT_EQ(2U, observer.finish_log.size());
+  EXPECT_EQ(0U, observer.cancel_log.size());
+  EXPECT_TRUE(observer.start_log[1] ==
+              CapturingObserver::StartOrCancelEntry(1, info1));
+  EXPECT_TRUE(observer.finish_log[1] ==
+              CapturingObserver::FinishEntry(1, true, info1));
 
   // Resolve "host2", setting referrer to "http://foobar.com"
   net::HostResolver::RequestInfo info2("host2", 70);
   info2.set_referrer(GURL("http://foobar.com"));
-  host_resolver.Resolve(info2, &addrlist, NULL, NULL);
+  rv = host_resolver.Resolve(info2, &addrlist, NULL, NULL);
+  EXPECT_EQ(net::OK, rv);
 
-  EXPECT_EQ(2U, observer.start_log.size());
-  EXPECT_EQ(2U, observer.finish_log.size());
-  EXPECT_TRUE(observer.start_log[1] == CapturingObserver::StartEntry(1, info2));
-  EXPECT_TRUE(observer.finish_log[1] == CapturingObserver::FinishEntry(
-      1, true, info2));
+  EXPECT_EQ(3U, observer.start_log.size());
+  EXPECT_EQ(3U, observer.finish_log.size());
+  EXPECT_EQ(0U, observer.cancel_log.size());
+  EXPECT_TRUE(observer.start_log[2] ==
+              CapturingObserver::StartOrCancelEntry(2, info2));
+  EXPECT_TRUE(observer.finish_log[2] ==
+              CapturingObserver::FinishEntry(2, true, info2));
 
   // Unregister the observer.
   host_resolver.RemoveObserver(&observer);
@@ -785,8 +813,87 @@ TEST_F(HostResolverTest, Observers) {
   host_resolver.Resolve(info3, &addrlist, NULL, NULL);
 
   // No effect this time, since observer was removed.
+  EXPECT_EQ(3U, observer.start_log.size());
+  EXPECT_EQ(3U, observer.finish_log.size());
+  EXPECT_EQ(0U, observer.cancel_log.size());
+}
+
+// Tests that observers are sent OnCancelResolution() whenever a request is
+// cancelled. There are two ways to cancel a request:
+//  (1) Delete the HostResolver while job is outstanding.
+//  (2) Call HostResolver::CancelRequest() while a request is outstanding.
+TEST_F(HostResolverTest, CancellationObserver) {
+  // Set a blocking mapper so we control when the resolver thread finishes.
+  scoped_refptr<WaitingHostMapper> mapper = new WaitingHostMapper();
+  ScopedHostMapper scoped_mapper(mapper.get());
+
+  CapturingObserver observer;
+  {
+    // Create a host resolver and attach an observer.
+    net::HostResolver host_resolver;
+    host_resolver.AddObserver(&observer);
+
+    TestCompletionCallback callback;
+
+    EXPECT_EQ(0U, observer.start_log.size());
+    EXPECT_EQ(0U, observer.finish_log.size());
+    EXPECT_EQ(0U, observer.cancel_log.size());
+
+    // Start an async resolve for (host1:70).
+    net::HostResolver::RequestInfo info1("host1", 70);
+    net::HostResolver::Request* req = NULL;
+    net::AddressList addrlist;
+    int rv = host_resolver.Resolve(info1, &addrlist, &callback, &req);
+    EXPECT_EQ(net::ERR_IO_PENDING, rv);
+    EXPECT_TRUE(NULL != req);
+
+    EXPECT_EQ(1U, observer.start_log.size());
+    EXPECT_EQ(0U, observer.finish_log.size());
+    EXPECT_EQ(0U, observer.cancel_log.size());
+
+    EXPECT_TRUE(observer.start_log[0] ==
+                CapturingObserver::StartOrCancelEntry(0, info1));
+
+    // Cancel the request (host mapper is blocked so it cant be finished yet).
+    host_resolver.CancelRequest(req);
+
+    EXPECT_EQ(1U, observer.start_log.size());
+    EXPECT_EQ(0U, observer.finish_log.size());
+    EXPECT_EQ(1U, observer.cancel_log.size());
+
+    EXPECT_TRUE(observer.cancel_log[0] ==
+                CapturingObserver::StartOrCancelEntry(0, info1));
+
+    // Start an async request for (host2:60)
+    net::HostResolver::RequestInfo info2("host2", 60);
+    rv = host_resolver.Resolve(info2, &addrlist, &callback, NULL);
+    EXPECT_EQ(net::ERR_IO_PENDING, rv);
+    EXPECT_TRUE(NULL != req);
+
+    EXPECT_EQ(2U, observer.start_log.size());
+    EXPECT_EQ(0U, observer.finish_log.size());
+    EXPECT_EQ(1U, observer.cancel_log.size());
+
+    EXPECT_TRUE(observer.start_log[1] ==
+                CapturingObserver::StartOrCancelEntry(1, info2));
+
+    // Upon exiting this scope, HostResolver is destroyed, so all requests are
+    // implicitly cancelled.
+  }
+
+  // Check that destroying the HostResolver sent a notification for
+  // cancellation of host2:60 request.
+
   EXPECT_EQ(2U, observer.start_log.size());
-  EXPECT_EQ(2U, observer.finish_log.size());
+  EXPECT_EQ(0U, observer.finish_log.size());
+  EXPECT_EQ(2U, observer.cancel_log.size());
+
+  net::HostResolver::RequestInfo info("host2", 60);
+  EXPECT_TRUE(observer.cancel_log[1] ==
+              CapturingObserver::StartOrCancelEntry(1, info));
+
+  // Unblock the host mapper to let the worker threads finish.
+  mapper->Signal();
 }
 
 }  // namespace
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 778bd8f..7026639 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -3054,6 +3054,11 @@ class ResolutionReferrerObserver : public HostResolver::Observer {
       called_finish_with_referrer_ = true;
   }
 
+  virtual void OnCancelResolution(int id,
+                                  const HostResolver::RequestInfo& info ) {
+    FAIL() << "Should not be cancelling any requests!";
+  }
+
   bool did_complete_with_expected_referrer() const {
     return called_start_with_referrer_ && called_finish_with_referrer_;
   }