Refactor: Eliminte locking from PrefetchObserver and DnsMaster in favor of making these classes non-threadsafe.

Conceptually, PrefetchObserver and DnsMaster live on the IO thread, and their methods can only be called from the IO thread.

In the cases where calls do need to be made from the UI thread, we post a task to be run on the IO loop and return without blocking.

The only time where we block is during shutdown, when we must wait on the IO thread to get us the startup list and referral list.

BUG=25335


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

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

1 files changed, 32 insertions, 66 deletions

diff --git a/chrome/browser/net/dns_master.cc b/chrome/browser/net/dns_master.cc
index 91e2d1f..9ac9a0a 100644
--- a/chrome/browser/net/dns_master.cc
+++ b/chrome/browser/net/dns_master.cc
@@ -10,7 +10,6 @@
 
 #include "base/compiler_specific.h"
 #include "base/histogram.h"
-#include "base/lock.h"
 #include "base/stats_counters.h"
 #include "base/string_util.h"
 #include "base/time.h"
@@ -84,8 +83,7 @@ DnsMaster::~DnsMaster() {
 }
 
 void DnsMaster::Shutdown() {
-  AutoLock auto_lock(lock_);
-
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   DCHECK(!shutdown_);
   shutdown_ = true;
 
@@ -97,51 +95,30 @@ void DnsMaster::Shutdown() {
 // Overloaded Resolve() to take a vector of names.
 void DnsMaster::ResolveList(const NameList& hostnames,
                             DnsHostInfo::ResolutionMotivation motivation) {
-  AutoLock auto_lock(lock_);
-
-  // We need to run this on the IO thread since we may access
-  // |host_resolver_| which is not thread safe.
-  if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
-    ChromeThread::PostTask(
-        ChromeThread::IO, FROM_HERE,
-        NewRunnableMethod(
-            this, &DnsMaster::ResolveList, hostnames, motivation));
-    return;
-  }
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   NameList::const_iterator it;
   for (it = hostnames.begin(); it < hostnames.end(); ++it)
-    PreLockedResolve(*it, motivation);
+    AppendToResolutionQueue(*it, motivation);
 }
 
 // Basic Resolve() takes an invidual name, and adds it
 // to the queue.
 void DnsMaster::Resolve(const std::string& hostname,
                         DnsHostInfo::ResolutionMotivation motivation) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   if (0 == hostname.length())
     return;
-  AutoLock auto_lock(lock_);
-
-  // We need to run this on the IO thread since we may access
-  // |host_resolver_| which is not thread safe.
-  if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
-    ChromeThread::PostTask(
-        ChromeThread::IO, FROM_HERE,
-        NewRunnableMethod(this, &DnsMaster::Resolve, hostname, motivation));
-    return;
-  }
-
-  PreLockedResolve(hostname, motivation);
+  AppendToResolutionQueue(hostname, motivation);
 }
 
 bool DnsMaster::AccruePrefetchBenefits(const GURL& referrer,
-                                      DnsHostInfo* navigation_info) {
+                                       DnsHostInfo* navigation_info) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   std::string hostname = navigation_info->hostname();
 
-  AutoLock auto_lock(lock_);
   Results::iterator it = results_.find(hostname);
   if (it == results_.end()) {
-    // Remain under lock to assure static HISTOGRAM constructor is safely run.
     // Use UMA histogram to quantify potential future gains here.
     UMA_HISTOGRAM_LONG_TIMES("DNS.UnexpectedResolutionL",
                              navigation_info->resolve_duration());
@@ -167,7 +144,6 @@ bool DnsMaster::AccruePrefetchBenefits(const GURL& referrer,
   switch (benefit) {
     case PREFETCH_NAME_FOUND:
     case PREFETCH_NAME_NONEXISTANT:
-      // Remain under lock to push data.
       cache_hits_.push_back(*navigation_info);
       if (referrer_based_prefetch) {
         std::string motivating_referrer(
@@ -180,7 +156,6 @@ bool DnsMaster::AccruePrefetchBenefits(const GURL& referrer,
       return true;
 
     case PREFETCH_CACHE_EVICTION:
-      // Remain under lock to push data.
       cache_eviction_map_[hostname] = *navigation_info;
       return false;
 
@@ -195,7 +170,8 @@ bool DnsMaster::AccruePrefetchBenefits(const GURL& referrer,
 }
 
 void DnsMaster::NonlinkNavigation(const GURL& referrer,
-                                  DnsHostInfo* navigation_info) {
+                                  const DnsHostInfo* navigation_info) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   std::string referring_host = referrer.host();
   if (referring_host.empty() || referring_host == navigation_info->hostname())
     return;
@@ -204,16 +180,7 @@ void DnsMaster::NonlinkNavigation(const GURL& referrer,
 }
 
 void DnsMaster::NavigatingTo(const std::string& host_name) {
-  AutoLock auto_lock(lock_);
-
-  // We need to run this on the IO thread since we may access
-  // |host_resolver_| which is not thread safe.
-  if (!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
-    ChromeThread::PostTask(
-        ChromeThread::IO, FROM_HERE,
-        NewRunnableMethod(this, &DnsMaster::NavigatingTo, host_name));
-    return;
-  }
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   Referrers::iterator it = referrers_.find(host_name);
   if (referrers_.end() == it)
@@ -221,7 +188,7 @@ void DnsMaster::NavigatingTo(const std::string& host_name) {
   Referrer* referrer = &(it->second);
   for (Referrer::iterator future_host = referrer->begin();
        future_host != referrer->end(); ++future_host) {
-    DnsHostInfo* queued_info = PreLockedResolve(
+    DnsHostInfo* queued_info = AppendToResolutionQueue(
         future_host->first,
         DnsHostInfo::LEARNED_REFERAL_MOTIVATED);
     if (queued_info)
@@ -285,7 +252,7 @@ struct RightToLeftStringSorter {
 };
 
 void DnsMaster::GetHtmlReferrerLists(std::string* output) {
-  AutoLock auto_lock(lock_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   if (referrers_.empty())
     return;
 
@@ -320,6 +287,7 @@ void DnsMaster::GetHtmlReferrerLists(std::string* output) {
 }
 
 void DnsMaster::GetHtmlInfo(std::string* output) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   // Local lists for calling DnsHostInfo
   DnsHostInfo::DnsInfoTable cache_hits;
   DnsHostInfo::DnsInfoTable cache_evictions;
@@ -327,11 +295,10 @@ void DnsMaster::GetHtmlInfo(std::string* output) {
   DnsHostInfo::DnsInfoTable network_hits;
   DnsHostInfo::DnsInfoTable already_cached;
 
-  // Get copies of all useful data under protection of a lock.
+  // Get copies of all useful data.
   typedef std::map<std::string, DnsHostInfo, RightToLeftStringSorter> Snapshot;
   Snapshot snapshot;
   {
-    AutoLock auto_lock(lock_);
     // DnsHostInfo supports value semantics, so we can do a shallow copy.
     for (Results::iterator it(results_.begin()); it != results_.end(); it++) {
       snapshot[it->first] = it->second;
@@ -388,10 +355,10 @@ void DnsMaster::GetHtmlInfo(std::string* output) {
       "Prefetching DNS records revealed non-existance for ", brief, output);
 }
 
-DnsHostInfo* DnsMaster::PreLockedResolve(
+DnsHostInfo* DnsMaster::AppendToResolutionQueue(
     const std::string& hostname,
     DnsHostInfo::ResolutionMotivation motivation) {
-  // DCHECK(We have the lock);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   DCHECK_NE(0u, hostname.length());
 
   if (shutdown_)
@@ -411,13 +378,11 @@ DnsHostInfo* DnsMaster::PreLockedResolve(
 
   info->SetQueuedState(motivation);
   work_queue_.Push(hostname, motivation);
-  PreLockedScheduleLookups();
+  StartSomeQueuedResolutions();
   return info;
 }
 
-void DnsMaster::PreLockedScheduleLookups() {
-  // We need to run this on the IO thread since we may access |host_resolver_|
-  // which is not thread safe.
+void DnsMaster::StartSomeQueuedResolutions() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   while (!work_queue_.IsEmpty() &&
@@ -427,7 +392,7 @@ void DnsMaster::PreLockedScheduleLookups() {
     DCHECK(info->HasHostname(hostname));
     info->SetAssignedState();
 
-    if (PreLockedCongestionControlPerformed(info)) {
+    if (CongestionControlPerformed(info)) {
       DCHECK(work_queue_.IsEmpty());
       return;
     }
@@ -443,13 +408,14 @@ void DnsMaster::PreLockedScheduleLookups() {
       // Completed synchronously (was already cached by HostResolver), or else
       // there was (equivalently) some network error that prevents us from
       // finding the name.  Status net::OK means it was "found."
-      PrelockedLookupFinished(request, hostname, status == net::OK);
+      LookupFinished(request, hostname, status == net::OK);
       delete request;
     }
   }
 }
 
-bool DnsMaster::PreLockedCongestionControlPerformed(DnsHostInfo* info) {
+bool DnsMaster::CongestionControlPerformed(DnsHostInfo* info) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   // Note: queue_duration is ONLY valid after we go to assigned state.
   if (info->queue_duration() < max_queue_delay_)
     return false;
@@ -470,17 +436,17 @@ void DnsMaster::OnLookupFinished(LookupRequest* request,
                                  const std::string& hostname, bool found) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
-  AutoLock auto_lock(lock_);  // For map access (changing info values).
-  PrelockedLookupFinished(request, hostname, found);
+  LookupFinished(request, hostname, found);
   pending_lookups_.erase(request);
   delete request;
 
-  PreLockedScheduleLookups();
+  StartSomeQueuedResolutions();
 }
 
-void DnsMaster::PrelockedLookupFinished(LookupRequest* request,
-                                        const std::string& hostname,
-                                        bool found) {
+void DnsMaster::LookupFinished(LookupRequest* request,
+                               const std::string& hostname,
+                               bool found) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   DnsHostInfo* info = &results_[hostname];
   DCHECK(info->HasHostname(hostname));
   if (info->is_marked_to_delete()) {
@@ -494,7 +460,7 @@ void DnsMaster::PrelockedLookupFinished(LookupRequest* request,
 }
 
 void DnsMaster::DiscardAllResults() {
-  AutoLock auto_lock(lock_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   // Delete anything listed so far in this session that shows in about:dns.
   cache_eviction_map_.clear();
   cache_hits_.clear();
@@ -535,8 +501,8 @@ void DnsMaster::DiscardAllResults() {
 }
 
 void DnsMaster::TrimReferrers() {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   std::vector<std::string> hosts;
-  AutoLock auto_lock(lock_);
   for (Referrers::const_iterator it = referrers_.begin();
        it != referrers_.end(); ++it)
     hosts.push_back(it->first);
@@ -546,8 +512,8 @@ void DnsMaster::TrimReferrers() {
 }
 
 void DnsMaster::SerializeReferrers(ListValue* referral_list) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   referral_list->Clear();
-  AutoLock auto_lock(lock_);
   for (Referrers::const_iterator it = referrers_.begin();
        it != referrers_.end(); ++it) {
     // Serialize the list of subresource names.
@@ -563,7 +529,7 @@ void DnsMaster::SerializeReferrers(ListValue* referral_list) {
 }
 
 void DnsMaster::DeserializeReferrers(const ListValue& referral_list) {
-  AutoLock auto_lock(lock_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   for (size_t i = 0; i < referral_list.GetSize(); ++i) {
     ListValue* motivating_host;
     if (!referral_list.GetList(i, &motivating_host))