Make URLRequestThrottlerManager a member of URLRequestContext.

This addresses a long-standing TODO and allows us to simplify the
logic in the class a bit as it now lives fully on the IO thread.  It
should also allow further cleanup in follow-up changes e.g. to stop
using scoped_refptr for the URLRequestThrottlerEntry instances.

BUG=119760


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

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

8 files changed, 70 insertions, 76 deletions

diff --git a/net/url_request/url_request_context.cc b/net/url_request/url_request_context.cc
index 45b6f29..1076f00 100644
--- a/net/url_request/url_request_context.cc
+++ b/net/url_request/url_request_context.cc
@@ -31,6 +31,7 @@ URLRequestContext::URLRequestContext()
       http_transaction_factory_(NULL),
       ftp_transaction_factory_(NULL),
       job_factory_(NULL),
+      throttler_manager_(NULL),
       url_requests_(new std::set<const URLRequest*>) {
 }
 
@@ -55,6 +56,7 @@ void URLRequestContext::CopyFrom(URLRequestContext* other) {
   set_http_transaction_factory(other->http_transaction_factory());
   set_ftp_transaction_factory(other->ftp_transaction_factory());
   set_job_factory(other->job_factory());
+  set_throttler_manager(other->throttler_manager());
 }
 
 void URLRequestContext::set_cookie_store(CookieStore* cookie_store) {
diff --git a/net/url_request/url_request_context.h b/net/url_request/url_request_context.h
index c46ae12..ca5c7b1 100644
--- a/net/url_request/url_request_context.h
+++ b/net/url_request/url_request_context.h
@@ -37,6 +37,7 @@ class ServerBoundCertService;
 class ProxyService;
 class URLRequest;
 class URLRequestJobFactory;
+class URLRequestThrottlerManager;
 
 // Subclass to provide application-specific context for URLRequest
 // instances. Note that URLRequestContext typically does not provide storage for
@@ -191,6 +192,14 @@ class NET_EXPORT URLRequestContext
     job_factory_ = job_factory;
   }
 
+  // May be NULL.
+  URLRequestThrottlerManager* throttler_manager() const {
+    return throttler_manager_;
+  }
+  void set_throttler_manager(URLRequestThrottlerManager* throttler_manager) {
+    throttler_manager_ = throttler_manager;
+  }
+
   // Gets the URLRequest objects that hold a reference to this
   // URLRequestContext.
   std::set<const URLRequest*>* url_requests() const {
@@ -236,6 +245,7 @@ class NET_EXPORT URLRequestContext
   HttpTransactionFactory* http_transaction_factory_;
   FtpTransactionFactory* ftp_transaction_factory_;
   const URLRequestJobFactory* job_factory_;
+  URLRequestThrottlerManager* throttler_manager_;
 
   // ---------------------------------------------------------------------------
   // Important: When adding any new members below, consider whether they need to
diff --git a/net/url_request/url_request_context_storage.cc b/net/url_request/url_request_context_storage.cc
index ed7950e..2a75db2 100644
--- a/net/url_request/url_request_context_storage.cc
+++ b/net/url_request/url_request_context_storage.cc
@@ -19,6 +19,7 @@
 #include "net/url_request/fraudulent_certificate_reporter.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_job_factory.h"
+#include "net/url_request/url_request_throttler_manager.h"
 
 namespace net {
 
@@ -115,4 +116,10 @@ void URLRequestContextStorage::set_job_factory(
   job_factory_.reset(job_factory);
 }
 
+void URLRequestContextStorage::set_throttler_manager(
+    URLRequestThrottlerManager* throttler_manager) {
+  context_->set_throttler_manager(throttler_manager);
+  throttler_manager_.reset(throttler_manager);
+}
+
 }  // namespace net
diff --git a/net/url_request/url_request_context_storage.h b/net/url_request/url_request_context_storage.h
index 1e62fb1..7652952 100644
--- a/net/url_request/url_request_context_storage.h
+++ b/net/url_request/url_request_context_storage.h
@@ -29,6 +29,7 @@ class SSLConfigService;
 class TransportSecurityState;
 class URLRequestContext;
 class URLRequestJobFactory;
+class URLRequestThrottlerManager;
 
 // URLRequestContextStorage is a helper class that provides storage for unowned
 // member variables of URLRequestContext.
@@ -64,6 +65,7 @@ class NET_EXPORT URLRequestContextStorage {
   void set_ftp_transaction_factory(
       FtpTransactionFactory* ftp_transaction_factory);
   void set_job_factory(URLRequestJobFactory* job_factory);
+  void set_throttler_manager(URLRequestThrottlerManager* throttler_manager);
 
  private:
   // We use a raw pointer to prevent reference cycles, since
@@ -89,6 +91,7 @@ class NET_EXPORT URLRequestContextStorage {
   scoped_ptr<HttpTransactionFactory> http_transaction_factory_;
   scoped_ptr<FtpTransactionFactory> ftp_transaction_factory_;
   scoped_ptr<URLRequestJobFactory> job_factory_;
+  scoped_ptr<URLRequestThrottlerManager> throttler_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestContextStorage);
 };
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index 8e6626a..4e31963 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -176,8 +176,7 @@ URLRequestHttpJob::URLRequestHttpJob(URLRequest* request)
                      base::Unretained(this)))),
       read_in_progress_(false),
       transaction_(NULL),
-      throttling_entry_(URLRequestThrottlerManager::GetInstance()->
-          RegisterRequestUrl(request->url())),
+      throttling_entry_(NULL),
       sdch_dictionary_advertised_(false),
       sdch_test_activated_(false),
       sdch_test_control_(false),
@@ -195,6 +194,10 @@ URLRequestHttpJob::URLRequestHttpJob(URLRequest* request)
           base::Bind(&URLRequestHttpJob::OnHeadersReceivedCallback,
                      base::Unretained(this)))),
       awaiting_callback_(false) {
+  URLRequestThrottlerManager* manager = request->context()->throttler_manager();
+  if (manager)
+    throttling_entry_ = manager->RegisterRequestUrl(request->url());
+
   ResetTimer();
 }
 
@@ -207,7 +210,7 @@ void URLRequestHttpJob::NotifyHeadersComplete() {
   // also need this info.
   is_cached_content_ = response_info_->was_cached;
 
-  if (!is_cached_content_) {
+  if (!is_cached_content_ && throttling_entry_) {
     URLRequestThrottlerHeaderAdapter response_adapter(GetResponseHeaders());
     throttling_entry_->UpdateWithResponse(request_info_.url.host(),
                                           &response_adapter);
@@ -325,7 +328,11 @@ void URLRequestHttpJob::StartTransactionInternal() {
       // change (to throttle only requests originating from
       // extensions) gets into M19. Right after the M19 branch point,
       // I will sort this out in a more architecturally-sound way.
-      if (!URLRequestThrottlerManager::GetInstance()->enforce_throttling() ||
+      URLRequestThrottlerManager* manager =
+          request_->context()->throttler_manager();
+      DCHECK(!manager || throttling_entry_);
+      if (!manager ||
+          !manager->enforce_throttling() ||
           request_->first_party_for_cookies().scheme() != "chrome-extension" ||
           !throttling_entry_->ShouldRejectRequest(request_info_.load_flags)) {
         rv = transaction_->Start(
diff --git a/net/url_request/url_request_http_job.h b/net/url_request/url_request_http_job.h
index f79fc1c..836ddb6 100644
--- a/net/url_request/url_request_http_job.h
+++ b/net/url_request/url_request_http_job.h
@@ -118,7 +118,8 @@ class URLRequestHttpJob : public URLRequestJob {
 
   scoped_ptr<HttpTransaction> transaction_;
 
-  // This is used to supervise traffic and enforce exponential back-off.
+  // This is used to supervise traffic and enforce exponential
+  // back-off.  May be NULL.
   scoped_refptr<URLRequestThrottlerEntryInterface> throttling_entry_;
 
   // Indicated if an SDCH dictionary was advertised, and hence an SDCH
diff --git a/net/url_request/url_request_throttler_manager.cc b/net/url_request/url_request_throttler_manager.cc
index 048cc32..c2fa430 100644
--- a/net/url_request/url_request_throttler_manager.cc
+++ b/net/url_request/url_request_throttler_manager.cc
@@ -16,24 +16,45 @@ namespace net {
 const unsigned int URLRequestThrottlerManager::kMaximumNumberOfEntries = 1500;
 const unsigned int URLRequestThrottlerManager::kRequestsBetweenCollecting = 200;
 
-URLRequestThrottlerManager* URLRequestThrottlerManager::GetInstance() {
-  return Singleton<URLRequestThrottlerManager>::get();
+URLRequestThrottlerManager::URLRequestThrottlerManager()
+    : requests_since_last_gc_(0),
+      enforce_throttling_(true),
+      enable_thread_checks_(false),
+      logged_for_localhost_disabled_(false),
+      registered_from_thread_(base::kInvalidThreadId) {
+  url_id_replacements_.ClearPassword();
+  url_id_replacements_.ClearUsername();
+  url_id_replacements_.ClearQuery();
+  url_id_replacements_.ClearRef();
+
+  NetworkChangeNotifier::AddIPAddressObserver(this);
+  NetworkChangeNotifier::AddOnlineStateObserver(this);
+}
+
+URLRequestThrottlerManager::~URLRequestThrottlerManager() {
+  NetworkChangeNotifier::RemoveIPAddressObserver(this);
+  NetworkChangeNotifier::RemoveOnlineStateObserver(this);
+
+  // Since, for now, the manager object might conceivably go away before
+  // the entries, detach the entries' back-pointer to the manager.
+  //
+  // TODO(joi): Revisit whether to make entries non-refcounted.
+  UrlEntryMap::iterator i = url_entries_.begin();
+  while (i != url_entries_.end()) {
+    if (i->second != NULL) {
+      i->second->DetachManager();
+    }
+    ++i;
+  }
+
+  // Delete all entries.
+  url_entries_.clear();
 }
 
 scoped_refptr<URLRequestThrottlerEntryInterface>
     URLRequestThrottlerManager::RegisterRequestUrl(const GURL &url) {
   DCHECK(!enable_thread_checks_ || CalledOnValidThread());
 
-  if (registered_from_thread_ == base::kInvalidThreadId) {
-    // We can't currently do this in the constructor as it is run on the
-    // UI thread and notifications go to the thread from which they are
-    // registered.
-    // TODO(joi): Clean this up once this is no longer a Singleton.
-    NetworkChangeNotifier::AddIPAddressObserver(this);
-    NetworkChangeNotifier::AddOnlineStateObserver(this);
-    registered_from_thread_ = base::PlatformThread::CurrentId();
-  }
-
   // Normalize the url.
   std::string url_id = GetIdFromUrl(url);
 
@@ -149,53 +170,6 @@ void URLRequestThrottlerManager::OnOnlineStateChanged(bool online) {
   OnNetworkChange();
 }
 
-URLRequestThrottlerManager::URLRequestThrottlerManager()
-    : requests_since_last_gc_(0),
-      enforce_throttling_(true),
-      enable_thread_checks_(false),
-      logged_for_localhost_disabled_(false),
-      registered_from_thread_(base::kInvalidThreadId) {
-  // Construction/destruction is on main thread (because BrowserMain
-  // retrieves an instance to call InitializeOptions), but is from then on
-  // used on I/O thread.
-  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.
-  // The AtExit manager does not run until the I/O thread has finished
-  // processing.
-  DetachFromThread();
-
-  // We must currently skip this in the production case, where the destructor
-  // does not run on the thread we registered from.
-  // TODO(joi): Fix once we are no longer a Singleton.
-  if (base::PlatformThread::CurrentId() == registered_from_thread_) {
-    NetworkChangeNotifier::RemoveIPAddressObserver(this);
-    NetworkChangeNotifier::RemoveOnlineStateObserver(this);
-  }
-
-  // Since, for now, the manager object might conceivably go away before
-  // the entries, detach the entries' back-pointer to the manager.
-  //
-  // TODO(joi): Revisit whether to make entries non-refcounted.
-  UrlEntryMap::iterator i = url_entries_.begin();
-  while (i != url_entries_.end()) {
-    if (i->second != NULL) {
-      i->second->DetachManager();
-    }
-    ++i;
-  }
-
-  // Delete all entries.
-  url_entries_.clear();
-}
-
 std::string URLRequestThrottlerManager::GetIdFromUrl(const GURL& url) const {
   if (!url.is_valid())
     return url.possibly_invalid_spec();
diff --git a/net/url_request/url_request_throttler_manager.h b/net/url_request/url_request_throttler_manager.h
index 778c695..d4643a0 100644
--- a/net/url_request/url_request_throttler_manager.h
+++ b/net/url_request/url_request_throttler_manager.h
@@ -12,7 +12,6 @@
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/singleton.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/threading/platform_thread.h"
 #include "googleurl/src/gurl.h"
@@ -34,16 +33,13 @@ class NetLog;
 // 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 this singleton object must be on the same thread,
-// although to allow it to be used as a singleton, construction and destruction
-// can occur on a separate thread.
 class NET_EXPORT URLRequestThrottlerManager
     : NON_EXPORTED_BASE(public base::NonThreadSafe),
       public NetworkChangeNotifier::IPAddressObserver,
       public NetworkChangeNotifier::OnlineStateObserver {
  public:
-  static URLRequestThrottlerManager* GetInstance();
+  URLRequestThrottlerManager();
+  virtual ~URLRequestThrottlerManager();
 
   // Must be called for every request, returns the URL request throttler entry
   // associated with the URL. The caller must inform this entry of some events.
@@ -88,10 +84,6 @@ class NET_EXPORT URLRequestThrottlerManager
   // OnlineStateObserver interface.
   virtual void OnOnlineStateChanged(bool online) OVERRIDE;
 
- protected:
-  URLRequestThrottlerManager();
-  virtual ~URLRequestThrottlerManager();
-
   // Method that allows us to transform a URL into an ID that can be used in our
   // map. Resulting IDs will be lowercase and consist of the scheme, host, port
   // and path (without query string, fragment, etc.).
@@ -120,8 +112,6 @@ class NET_EXPORT URLRequestThrottlerManager
   int GetNumberOfEntriesForTests() const { return url_entries_.size(); }
 
  private:
-  friend struct DefaultSingletonTraits<URLRequestThrottlerManager>;
-
   // From each URL we generate an ID composed of the scheme, host, port and path
   // that allows us to uniquely map an entry to it.
   typedef std::map<std::string, scoped_refptr<URLRequestThrottlerEntry> >