Added debug checks to CancelableRequestConsumer to ensure proper thread usage.

Added assertions to ensure that any given consumer is only used with a single provider (since we rely on the provider to enforce thread safety, using multiple providers is forbidden).

Also updated various tests/code that were improperly using the same
consumer with multiple providers.

BUG=none
TEST=run unit tests, see no crashes

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

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

5 files changed, 25 insertions, 14 deletions

diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index b625e65..3d5a5e0 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -169,7 +169,7 @@ void TopSites::Init(const FilePath& db_name) {
   backend_ = new TopSitesBackend;
   backend_->Init(db_name);
   backend_->GetMostVisitedThumbnails(
-      &cancelable_consumer_,
+      &top_sites_consumer_,
       NewCallback(this, &TopSites::OnGotMostVisitedThumbnails));
 
   // History may have already finished loading by the time we're created.
@@ -288,7 +288,7 @@ void TopSites::MigrateFromHistory() {
       new LoadThumbnailsFromHistoryTask(
           this,
           num_results_to_request_from_history()),
-      &cancelable_consumer_);
+      &history_consumer_);
   MigratePinnedURLs();
 }
 
@@ -319,7 +319,7 @@ void TopSites::FinishHistoryMigration(const ThumbnailMigration& data) {
   // that notifies us when done. When done we'll know everything was written and
   // we can tell history to finish its part of migration.
   backend_->DoEmptyRequest(
-      &cancelable_consumer_,
+      &top_sites_consumer_,
       NewCallback(this, &TopSites::OnHistoryMigrationWrittenToDisk));
 }
 
@@ -463,7 +463,8 @@ void TopSites::Shutdown() {
   // Cancel all requests so that the service doesn't callback to us after we've
   // invoked Shutdown (this could happen if we have a pending request and
   // Shutdown is invoked).
-  cancelable_consumer_.CancelAllRequests();
+  history_consumer_.CancelAllRequests();
+  top_sites_consumer_.CancelAllRequests();
   backend_->Shutdown();
 }
 
@@ -520,7 +521,7 @@ CancelableRequestProvider::Handle TopSites::StartQueryForMostVisited() {
     return hs->QueryMostVisitedURLs(
         num_results_to_request_from_history(),
         kDaysOfHistory,
-        &cancelable_consumer_,
+        &history_consumer_,
         NewCallback(this, &TopSites::OnTopSitesAvailableFromHistory));
   }
   return 0;
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index 11433a2..a6bb9a2 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -326,7 +326,10 @@ class TopSites
   // Lock used to access |thread_safe_cache_|.
   mutable base::Lock lock_;
 
-  CancelableRequestConsumer cancelable_consumer_;
+  // Need a separate consumer for each CancelableRequestProvider we interact
+  // with (HistoryService and TopSitesBackend).
+  CancelableRequestConsumer history_consumer_;
+  CancelableRequestConsumer top_sites_consumer_;
 
   // Timer that asks history for the top sites. This is used to make sure our
   // data stays in sync with history.
diff --git a/chrome/test/live_sync/live_typed_urls_sync_test.cc b/chrome/test/live_sync/live_typed_urls_sync_test.cc
index 9c8e789..cc465fd 100644
--- a/chrome/test/live_sync/live_typed_urls_sync_test.cc
+++ b/chrome/test/live_sync/live_typed_urls_sync_test.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/profiles/profile.h"
+#include "content/browser/cancelable_request.h"
 
 namespace {
 
@@ -66,10 +67,11 @@ LiveTypedUrlsSyncTest::GetTypedUrlsFromClient(int index) {
 
 std::vector<history::URLRow>
 LiveTypedUrlsSyncTest::GetTypedUrlsFromHistoryService(HistoryService *service) {
+  CancelableRequestConsumer cancelable_consumer;
   std::vector<history::URLRow> rows;
   base::WaitableEvent wait_event(true, false);
   service->ScheduleDBTask(new GetTypedUrlsTask(&rows, &wait_event),
-                          &cancelable_consumer_);
+                          &cancelable_consumer);
   wait_event.Wait();
   return rows;
 }
@@ -121,11 +123,12 @@ void LiveTypedUrlsSyncTest::DeleteUrlFromHistory(int index, const GURL& url) {
 }
 
 void LiveTypedUrlsSyncTest::WaitForHistoryDBThread(int index) {
+  CancelableRequestConsumer cancelable_consumer;
   HistoryService* service =
       GetProfile(index)->GetHistoryServiceWithoutCreating();
   base::WaitableEvent wait_event(true, false);
   service->ScheduleDBTask(new FlushHistoryDBQueueTask(&wait_event),
-                          &cancelable_consumer_);
+                          &cancelable_consumer);
   wait_event.Wait();
 }
 
diff --git a/chrome/test/live_sync/live_typed_urls_sync_test.h b/chrome/test/live_sync/live_typed_urls_sync_test.h
index f6f8d61..ab8d3ea 100644
--- a/chrome/test/live_sync/live_typed_urls_sync_test.h
+++ b/chrome/test/live_sync/live_typed_urls_sync_test.h
@@ -10,7 +10,6 @@
 
 #include "chrome/browser/history/history_types.h"
 #include "chrome/test/live_sync/live_sync_test.h"
-#include "content/browser/cancelable_request.h"
 
 namespace base {
 class Time;
@@ -65,11 +64,6 @@ class LiveTypedUrlsSyncTest : public LiveSyncTest {
   std::vector<history::URLRow> GetTypedUrlsFromHistoryService(
       HistoryService* service);
 
-
-  // Helper object to make sure we don't leave tasks running on the history
-  // thread.
-  CancelableRequestConsumerT<int, 0> cancelable_consumer_;
-
   base::Time timestamp_;
   DISALLOW_COPY_AND_ASSIGN(LiveTypedUrlsSyncTest);
 };
diff --git a/content/browser/cancelable_request.h b/content/browser/cancelable_request.h
index 769439f..df27cbc 100644
--- a/content/browser/cancelable_request.h
+++ b/content/browser/cancelable_request.h
@@ -344,6 +344,9 @@ size_t CancelableRequestConsumerTSimple<T>::PendingRequestCount() const {
 
 template<class T>
 void CancelableRequestConsumerTSimple<T>::CancelAllRequests() {
+  // TODO(atwilson): This code is not thread safe as it is called from the
+  // consumer thread (via the destructor) and accesses pending_requests_
+  // without acquiring the provider lock (http://crbug.com/85970).
   PendingRequestList copied_requests(pending_requests_);
   for (typename PendingRequestList::iterator i = copied_requests.begin();
        i != copied_requests.end(); ++i)
@@ -387,6 +390,13 @@ template<class T>
 void CancelableRequestConsumerTSimple<T>::OnRequestAdded(
     CancelableRequestProvider* provider,
     CancelableRequestProvider::Handle handle) {
+  // Make sure we're not mixing/matching providers (since the provider is
+  // responsible for providing thread safety until http://crbug.com/85970 is
+  // fixed, we can't use the same consumer with multiple providers).
+#ifndef NDEBUG
+  if (!pending_requests_.empty())
+    DCHECK(pending_requests_.begin()->first.provider == provider);
+#endif
   DCHECK(pending_requests_.find(PendingRequest(provider, handle)) ==
          pending_requests_.end());
   pending_requests_[PendingRequest(provider, handle)] = get_initial_t();