Add checks to URLRequestThrottlerManager to help identify the cause of crashes.

BUG=71721
TEST=None

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

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

2 files changed, 108 insertions, 17 deletions

diff --git a/net/url_request/url_request_throttler_manager.cc b/net/url_request/url_request_throttler_manager.cc
index f71cf52..526e4b5 100644
--- a/net/url_request/url_request_throttler_manager.cc
+++ b/net/url_request/url_request_throttler_manager.cc
@@ -4,7 +4,65 @@
 
 #include "net/url_request/url_request_throttler_manager.h"
 
+#include <list>
+
+#include "base/logging.h"
 #include "base/string_util.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/platform_thread.h"
+
+namespace {
+
+// AccessLog records threads that have accessed the URLRequestThrottlerManager
+// singleton object.
+// TODO(yzshen): It is used for diagnostic purpose and should be removed once we
+// figure out crbug.com/71721
+class AccessLog {
+ public:
+  static const size_t kAccessLogSize = 4;
+
+  AccessLog() {
+    for (size_t i = 0; i < kAccessLogSize; ++i) {
+      thread_ids_[i] = base::kInvalidThreadId;
+      urls_[i][0] = '\0';
+    }
+  }
+
+  AccessLog(const AccessLog& log) {
+    base::AutoLock auto_lock(log.lock_);
+    for (size_t i = 0; i < kAccessLogSize; ++i) {
+      thread_ids_[i] = log.thread_ids_[i];
+      base::strlcpy(urls_[i], log.urls_[i], kUrlBufferSize);
+    }
+  }
+
+  void Add(base::PlatformThreadId id, const GURL& url) {
+    base::AutoLock auto_lock(lock_);
+    for (size_t i = 0; i < kAccessLogSize; ++i) {
+      if (thread_ids_[i] == id) {
+        return;
+      } else if (thread_ids_[i] == base::kInvalidThreadId) {
+        DCHECK(i == 0);
+        thread_ids_[i] = id;
+        base::strlcpy(urls_[i], url.spec().c_str(), kUrlBufferSize);
+        return;
+      }
+    }
+  }
+
+ private:
+  static const size_t kUrlBufferSize = 128;
+
+  mutable base::Lock lock_;
+  base::PlatformThreadId thread_ids_[kAccessLogSize];
+  // Records the URL argument of the first RegisterRequestUrl() call on each
+  // thread.
+  char urls_[kAccessLogSize][kUrlBufferSize];
+};
+
+AccessLog access_log;
+
+}  // namespace
 
 namespace net {
 
@@ -17,6 +75,9 @@ URLRequestThrottlerManager* URLRequestThrottlerManager::GetInstance() {
 
 scoped_refptr<URLRequestThrottlerEntryInterface>
     URLRequestThrottlerManager::RegisterRequestUrl(const GURL &url) {
+  if (record_access_log_)
+    access_log.Add(base::PlatformThread::CurrentId(), url);
+
   // Normalize the url.
   std::string url_id = GetIdFromUrl(url);
 
@@ -24,11 +85,20 @@ scoped_refptr<URLRequestThrottlerEntryInterface>
   GarbageCollectEntriesIfNecessary();
 
   // Find the entry in the map or create it.
-  scoped_refptr<URLRequestThrottlerEntry>& entry = url_entries_[url_id];
-  if (entry == NULL)
-    entry = new URLRequestThrottlerEntry();
-
-  return entry;
+  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;
+  }
 }
 
 void URLRequestThrottlerManager::OverrideEntryForTests(
@@ -52,9 +122,15 @@ void URLRequestThrottlerManager::EraseEntryForTests(const GURL& url) {
   url_entries_.erase(url_id);
 }
 
+void URLRequestThrottlerManager::InitializeOptions(bool enforce_throttling) {
+  enforce_throttling_ = enforce_throttling;
+  record_access_log_ = true;
+}
+
 URLRequestThrottlerManager::URLRequestThrottlerManager()
     : requests_since_last_gc_(0),
-      enforce_throttling_(true) {
+      enforce_throttling_(true),
+      record_access_log_(false) {
 }
 
 URLRequestThrottlerManager::~URLRequestThrottlerManager() {
@@ -89,14 +165,25 @@ void URLRequestThrottlerManager::GarbageCollectEntriesIfNecessary() {
 }
 
 void URLRequestThrottlerManager::GarbageCollectEntries() {
-  UrlEntryMap::iterator i = url_entries_.begin();
-
-  while (i != url_entries_.end()) {
-    if ((i->second)->IsEntryOutdated()) {
-      url_entries_.erase(i++);
-    } else {
-      ++i;
-    }
+  volatile AccessLog access_log_copy(access_log);
+
+  // 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);
   }
 
   // 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 189f33d..456e84c 100644
--- a/net/url_request/url_request_throttler_manager.h
+++ b/net/url_request/url_request_throttler_manager.h
@@ -49,9 +49,7 @@ class URLRequestThrottlerManager {
   // It is only used by unit tests.
   void EraseEntryForTests(const GURL& url);
 
-  void set_enforce_throttling(bool enforce_throttling) {
-    enforce_throttling_ = enforce_throttling;
-  }
+  void InitializeOptions(bool enforce_throttling);
 
   bool enforce_throttling() const { return enforce_throttling_; }
 
@@ -104,6 +102,12 @@ class URLRequestThrottlerManager {
   // period.
   bool enforce_throttling_;
 
+  // Whether to record threads that have accessed the URLRequestThrottlerManager
+  // singleton object.
+  // TODO(yzshen): It is used for diagnostic purpose and should be removed once
+  // we figure out crbug.com/71721
+  bool record_access_log_;
+
   DISALLOW_COPY_AND_ASSIGN(URLRequestThrottlerManager);
 };