Add a hard CHECK on use from a single thread to URLRequestThrottlerManager. This is done via a temporary copy of ThreadChecker so it can be made to work in release builds as well.

Revert iterator use back to most optimal approach (previous changes were intended as temporary).

BUG=71721
TEST=net_unittest

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

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

3 files changed, 111 insertions, 51 deletions

diff --git a/net/url_request/url_request_throttler_entry.cc b/net/url_request/url_request_throttler_entry.cc
index e5da528..bf97feb 100644
--- a/net/url_request/url_request_throttler_entry.cc
+++ b/net/url_request/url_request_throttler_entry.cc
@@ -66,6 +66,7 @@ URLRequestThrottlerEntry::URLRequestThrottlerEntry(
 }
 
 bool URLRequestThrottlerEntry::IsEntryOutdated() const {
+  CHECK(this);  // to help track crbug.com/71721
   if (entry_lifetime_ms_ == -1)
     return false;
 
diff --git a/net/url_request/url_request_throttler_manager.cc b/net/url_request/url_request_throttler_manager.cc
index 008c235..39e1f4e 100644
--- a/net/url_request/url_request_throttler_manager.cc
+++ b/net/url_request/url_request_throttler_manager.cc
@@ -11,6 +11,31 @@
 
 namespace net {
 
+ThreadCheckerForRelease::ThreadCheckerForRelease()
+    : valid_thread_id_(base::kInvalidThreadId) {
+  EnsureThreadIdAssigned();
+}
+
+ThreadCheckerForRelease::~ThreadCheckerForRelease() {}
+
+bool ThreadCheckerForRelease::CalledOnValidThread() const {
+  EnsureThreadIdAssigned();
+  base::AutoLock auto_lock(lock_);
+  return valid_thread_id_ == base::PlatformThread::CurrentId();
+}
+
+void ThreadCheckerForRelease::DetachFromThread() {
+  base::AutoLock auto_lock(lock_);
+  valid_thread_id_ = base::kInvalidThreadId;
+}
+
+void ThreadCheckerForRelease::EnsureThreadIdAssigned() const {
+  base::AutoLock auto_lock(lock_);
+  if (valid_thread_id_ != base::kInvalidThreadId)
+    return;
+  valid_thread_id_ = base::PlatformThread::CurrentId();
+}
+
 const unsigned int URLRequestThrottlerManager::kMaximumNumberOfEntries = 1500;
 const unsigned int URLRequestThrottlerManager::kRequestsBetweenCollecting = 200;
 
@@ -20,6 +45,8 @@ URLRequestThrottlerManager* URLRequestThrottlerManager::GetInstance() {
 
 scoped_refptr<URLRequestThrottlerEntryInterface>
     URLRequestThrottlerManager::RegisterRequestUrl(const GURL &url) {
+  CHECK(being_tested_ || thread_checker_.CalledOnValidThread());
+
   // Normalize the url.
   std::string url_id = GetIdFromUrl(url);
 
@@ -27,20 +54,14 @@ scoped_refptr<URLRequestThrottlerEntryInterface>
   GarbageCollectEntriesIfNecessary();
 
   // Find the entry in the map or create it.
-  UrlEntryMap::iterator i = url_entries_.find(url_id);
-  if (i == url_entries_.end()) {
-    scoped_refptr<URLRequestThrottlerEntry> entry(
-        new URLRequestThrottlerEntry());
-    // Explicitly check whether the new operation is successful or not, in order
-    // to track down crbug.com/71721
-    CHECK(entry.get());
-
-    url_entries_.insert(std::make_pair(url_id, entry));
-    return entry;
-  } else {
-    CHECK(i->second.get());
-    return i->second;
-  }
+  scoped_refptr<URLRequestThrottlerEntry>& entry = url_entries_[url_id];
+  if (entry.get() == NULL)
+    entry = new URLRequestThrottlerEntry();
+
+  // TODO(joi): Demote CHECKs in this file to DCHECKs (or remove them) once
+  // we fully understand crbug.com/71721
+  CHECK(entry.get());
+  return entry;
 }
 
 void URLRequestThrottlerManager::OverrideEntryForTests(
@@ -66,14 +87,28 @@ void URLRequestThrottlerManager::EraseEntryForTests(const GURL& url) {
 
 void URLRequestThrottlerManager::InitializeOptions(bool enforce_throttling) {
   enforce_throttling_ = enforce_throttling;
+  being_tested_ = false;
 }
 
 URLRequestThrottlerManager::URLRequestThrottlerManager()
     : requests_since_last_gc_(0),
-      enforce_throttling_(true) {
+      enforce_throttling_(true),
+      being_tested_(true) {
+  // Construction/destruction is on main thread (because BrowserMain
+  // retrieves an instance to call InitializeOptions), but is from then on
+  // used on I/O thread.
+  thread_checker_.DetachFromThread();
+
+  url_id_replacements_.ClearPassword();
+  url_id_replacements_.ClearUsername();
+  url_id_replacements_.ClearQuery();
+  url_id_replacements_.ClearRef();
 }
 
 URLRequestThrottlerManager::~URLRequestThrottlerManager() {
+  // Destruction is on main thread (AtExit), but real use is on I/O thread.
+  thread_checker_.DetachFromThread();
+
   // Delete all entries.
   url_entries_.clear();
 }
@@ -82,16 +117,7 @@ std::string URLRequestThrottlerManager::GetIdFromUrl(const GURL& url) const {
   if (!url.is_valid())
     return url.possibly_invalid_spec();
 
-  if (url_id_replacements_ == NULL) {
-    url_id_replacements_.reset(new GURL::Replacements());
-
-    url_id_replacements_->ClearPassword();
-    url_id_replacements_->ClearUsername();
-    url_id_replacements_->ClearQuery();
-    url_id_replacements_->ClearRef();
-  }
-
-  GURL id = url.ReplaceComponents(*url_id_replacements_);
+  GURL id = url.ReplaceComponents(url_id_replacements_);
   return StringToLowerASCII(id.spec());
 }
 
@@ -99,29 +125,21 @@ void URLRequestThrottlerManager::GarbageCollectEntriesIfNecessary() {
   requests_since_last_gc_++;
   if (requests_since_last_gc_ < kRequestsBetweenCollecting)
     return;
-
   requests_since_last_gc_ = 0;
+
   GarbageCollectEntries();
 }
 
 void URLRequestThrottlerManager::GarbageCollectEntries() {
-  // The more efficient way to remove outdated entries is iterating over all the
-  // elements in the map, and removing those outdated ones during the process.
-  // However, one hypothesis about the cause of crbug.com/71721 is that some
-  // kind of iterator error happens when we change the map during iteration. As
-  // a result, we write the code this way in order to track down the bug.
-  std::list<std::string> outdated_keys;
-  for (UrlEntryMap::iterator entry_iter = url_entries_.begin();
-       entry_iter != url_entries_.end(); ++entry_iter) {
-    std::string key = entry_iter->first;
-    CHECK(entry_iter->second.get());
-
-    if ((entry_iter->second)->IsEntryOutdated())
-      outdated_keys.push_back(key);
-  }
-  for (std::list<std::string>::iterator key_iter = outdated_keys.begin();
-       key_iter != outdated_keys.end(); ++key_iter) {
-    url_entries_.erase(*key_iter);
+  UrlEntryMap::iterator i = url_entries_.begin();
+
+  while (i != url_entries_.end()) {
+    CHECK(i->second.get());
+    if ((i->second)->IsEntryOutdated()) {
+      url_entries_.erase(i++);
+    } else {
+      ++i;
+    }
   }
 
   // In case something broke we want to make sure not to grow indefinitely.
diff --git a/net/url_request/url_request_throttler_manager.h b/net/url_request/url_request_throttler_manager.h
index 58d1985..1862cd0 100644
--- a/net/url_request/url_request_throttler_manager.h
+++ b/net/url_request/url_request_throttler_manager.h
@@ -11,22 +11,52 @@
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
 #include "base/singleton.h"
+#include "base/synchronization/lock.h"  // ThreadCheckerForRelease
+#include "base/threading/platform_thread.h"  // ThreadCheckerForRelease
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request_throttler_entry.h"
 
 namespace net {
 
-// Class that registers URL request throttler entries for URLs being accessed in
-// order to supervise traffic. URL requests for HTTP contents should register
-// their URLs in this manager on each request.
+// TODO(joi): Delete this temporary copy of base::ThreadChecker (needed to
+// enable it in release builds) and go back to simply inheriting from
+// NonThreadSafe once crbug.com/71721 has been tracked down.
+class ThreadCheckerForRelease {
+ public:
+  ThreadCheckerForRelease();
+  ~ThreadCheckerForRelease();
+
+  bool CalledOnValidThread() const;
+
+  // Changes the thread that is checked for in CalledOnValidThread.  This may
+  // be useful when an object may be created on one thread and then used
+  // exclusively on another thread.
+  void DetachFromThread();
+
+ private:
+  void EnsureThreadIdAssigned() const;
+
+  mutable base::Lock lock_;
+  // This is mutable so that CalledOnValidThread can set it.
+  // It's guarded by |lock_|.
+  mutable base::PlatformThreadId valid_thread_id_;
+};
+
+// Class that registers URL request throttler entries for URLs being accessed
+// in order to supervise traffic. URL requests for HTTP contents should
+// register their URLs in this manager on each request.
+//
 // URLRequestThrottlerManager maintains a map of URL IDs to URL request
-// throttler entries. It creates URL request throttler entries when new URLs are
-// registered, and does garbage collection from time to time in order to clean
-// out outdated entries. URL ID consists of lowercased scheme, host, port and
-// path. All URLs converted to the same ID will share the same entry.
+// throttler entries. It creates URL request throttler entries when new URLs
+// are registered, and does garbage collection from time to time in order to
+// clean out outdated entries. URL ID consists of lowercased scheme, host, port
+// and path. All URLs converted to the same ID will share the same entry.
 //
 // NOTE: All usage of the singleton object of this class should be on the same
 // thread.
+//
+// TODO(joi): Switch back to NonThreadSafe (and remove checks in release builds)
+// once crbug.com/71721 has been tracked down.
 class URLRequestThrottlerManager {
  public:
   static URLRequestThrottlerManager* GetInstance();
@@ -96,12 +126,23 @@ class URLRequestThrottlerManager {
   // GarbageCollectEntries.
   unsigned int requests_since_last_gc_;
 
-  mutable scoped_ptr<GURL::Replacements> url_id_replacements_;
+  // Valid after construction.
+  GURL::Replacements url_id_replacements_;
 
   // Whether we would like to reject outgoing HTTP requests during the back-off
   // period.
   bool enforce_throttling_;
 
+  // Certain tests do not obey the net component's threading policy, so we
+  // keep track of whether we're being used by tests, and turn off certain
+  // checks.
+  //
+  // TODO(joi): See if we can fix the offending unit tests and remove this
+  // workaround.
+  bool being_tested_;
+
+  ThreadCheckerForRelease thread_checker_;
+
   DISALLOW_COPY_AND_ASSIGN(URLRequestThrottlerManager);
 };