summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--chrome/browser/net/dns_global.cc26
-rw-r--r--net/base/host_resolver.cc12
-rw-r--r--net/base/host_resolver.h13
-rw-r--r--net/base/host_resolver_unittest.cc107
-rw-r--r--net/http/http_network_transaction_unittest.cc5
5 files changed, 142 insertions, 21 deletions
diff --git a/chrome/browser/net/dns_global.cc b/chrome/browser/net/dns_global.cc
index cf7145c..9e4c8f6 100644
--- a/chrome/browser/net/dns_global.cc
+++ b/chrome/browser/net/dns_global.cc
@@ -149,6 +149,9 @@ class PrefetchObserver : public net::HostResolver::Observer {
int request_id,
bool was_resolved,
const net::HostResolver::RequestInfo& request_info);
+ virtual void OnCancelResolution(
+ int request_id,
+ const net::HostResolver::RequestInfo& request_info);
static void DnsGetFirstResolutionsHtml(std::string* output);
static void SaveStartupListAsPref(PrefService* local_state);
@@ -202,12 +205,9 @@ void PrefetchObserver::OnStartResolution(
NavigatingTo(request_info.hostname());
- // TODO(eroman): If the resolve request is cancelled, then
- // OnFinishResolutionWithStatus will not be called, and |resolutions| will
- // grow unbounded!
- // http://crbug.com/14138
-
AutoLock auto_lock(*lock);
+ // This entry will be deleted either by OnFinishResolutionWithStatus(), or
+ // by OnCancelResolution().
(*resolutions)[request_id] = navigation_info;
}
@@ -240,6 +240,22 @@ void PrefetchObserver::OnFinishResolutionWithStatus(
StartupListAppend(navigation_info);
}
+void PrefetchObserver::OnCancelResolution(
+ int request_id,
+ const net::HostResolver::RequestInfo& request_info) {
+ if (request_info.is_speculative())
+ return; // One of our own requests.
+
+ // Remove the entry from |resolutions| that was added by OnStartResolution().
+ AutoLock auto_lock(*lock);
+ ObservedResolutionMap::iterator it = resolutions->find(request_id);
+ if (resolutions->end() == it) {
+ DCHECK(false);
+ return;
+ }
+ resolutions->erase(it);
+}
+
// static
void PrefetchObserver::StartupListAppend(const DnsHostInfo& navigation_info) {
if (!on_the_record_switch || NULL == dns_master)
diff --git a/net/base/host_resolver.cc b/net/base/host_resolver.cc
index dd1ffcc..14ad212 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;
@@ -291,8 +291,12 @@ class HostResolver::Job : public base::RefCountedThreadSafe<HostResolver::Job> {
void Cancel() {
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;
+ }
}
// Called from origin thread.
@@ -483,7 +487,7 @@ 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();
}
void HostResolver::AddObserver(Observer* observer) {
diff --git a/net/base/host_resolver.h b/net/base/host_resolver.h
index 34c803f..6e2add7 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,11 +180,11 @@ 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);
diff --git a/net/base/host_resolver_unittest.cc b/net/base/host_resolver_unittest.cc
index 195b2fb..62281f0 100644
--- a/net/base/host_resolver_unittest.cc
+++ b/net/base/host_resolver_unittest.cc
@@ -702,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(
@@ -712,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;
}
@@ -742,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;
@@ -763,8 +771,9 @@ TEST_F(HostResolverTest, Observers) {
EXPECT_EQ(1U, observer.start_log.size());
EXPECT_EQ(1U, observer.finish_log.size());
+ EXPECT_EQ(0U, observer.cancel_log.size());
EXPECT_TRUE(observer.start_log[0] ==
- CapturingObserver::StartEntry(0, info1));
+ CapturingObserver::StartOrCancelEntry(0, info1));
EXPECT_TRUE(observer.finish_log[0] ==
CapturingObserver::FinishEntry(0, true, info1));
@@ -776,8 +785,9 @@ TEST_F(HostResolverTest, Observers) {
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::StartEntry(1, info1));
+ CapturingObserver::StartOrCancelEntry(1, info1));
EXPECT_TRUE(observer.finish_log[1] ==
CapturingObserver::FinishEntry(1, true, info1));
@@ -789,7 +799,9 @@ TEST_F(HostResolverTest, Observers) {
EXPECT_EQ(3U, observer.start_log.size());
EXPECT_EQ(3U, observer.finish_log.size());
- EXPECT_TRUE(observer.start_log[2] == CapturingObserver::StartEntry(2, info2));
+ 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));
@@ -803,6 +815,85 @@ TEST_F(HostResolverTest, Observers) {
// 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, DISABLED_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(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_;
}
generated by cgit v1.1 at 2025-01-12 09:20:23 +0000