Fix the race condition on TopSites initialization.

Change TopSites::GetMostVisitedURLs to take a callback. This allows TopSites to wait for the results list from the TopSitesDatabase.

BUG=none
TEST=TopSitesTest

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

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

3 files changed, 235 insertions, 48 deletions

diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index 533bddf..3ce1286 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -31,7 +31,8 @@ static const int64 kMaxUpdateIntervalMinutes = 60;
 TopSites::TopSites(Profile* profile) : profile_(profile),
                                        mock_history_service_(NULL),
                                        last_num_urls_changed_(0),
-                                       migration_in_progress_(false) {
+                                       migration_in_progress_(false),
+                                       waiting_for_results_(true) {
   registrar_.Add(this, NotificationType::HISTORY_URLS_DELETED,
                  Source<Profile>(profile_));
   registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
@@ -161,9 +162,23 @@ bool TopSites::SetPageThumbnailNoDB(const GURL& url,
   return true;
 }
 
-MostVisitedURLList TopSites::GetMostVisitedURLs() {
-  AutoLock lock(lock_);
-  return top_sites_;
+void TopSites::GetMostVisitedURLs(CancelableRequestConsumer* consumer,
+                                  GetTopSitesCallback* callback) {
+
+  scoped_refptr<CancelableRequest<GetTopSitesCallback> > request(
+      new CancelableRequest<GetTopSitesCallback>(callback));
+  // This ensures cancelation of requests when either the consumer or the
+  // provider is deleted. Deletion of requests is also guaranteed.
+  AddRequest(request, consumer);
+  if (waiting_for_results_) {
+    // A request came in before we have any top sites.
+    // We have to keep track of the requests ourselves.
+    pending_callbacks_.insert(request);
+    return;
+  }
+  if (request->canceled())
+    return;
+  request->ForwardResult(GetTopSitesCallback::TupleType(top_sites_));
 }
 
 bool TopSites::GetPageThumbnail(const GURL& url, RefCountedBytes** data) const {
@@ -278,6 +293,8 @@ void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) {
   // Take ownership of the most visited data.
   top_sites_.clear();
   top_sites_.swap(*most_visited);
+  waiting_for_results_ = false;
+
 
   // Save the redirect information for quickly mapping to the canonical URLs.
   canonical_urls_.clear();
@@ -402,6 +419,18 @@ base::TimeDelta TopSites::GetUpdateDelay() {
 void TopSites::OnTopSitesAvailable(
     CancelableRequestProvider::Handle handle,
     MostVisitedURLList pages) {
+  if (!pending_callbacks_.empty()) {
+    PendingCallbackSet copy(pending_callbacks_);
+    PendingCallbackSet::iterator i;
+    for (i = pending_callbacks_.begin();
+         i != pending_callbacks_.end(); ++i) {
+      scoped_refptr<CancelableRequest<GetTopSitesCallback> > request = *i;
+      if (!request->canceled())
+        request->ForwardResult(GetTopSitesCallback::TupleType(pages));
+    }
+    pending_callbacks_.clear();
+  }
+
   ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableMethod(
       this, &TopSites::UpdateMostVisited, pages));
 }
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index a1eb531..44ff8c9 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -45,7 +45,8 @@ typedef std::vector<MostVisitedURL> MostVisitedURLList;
 // a nontrivial performance win, especially when the browser is starting and
 // the UI thread is busy.
 class TopSites : public NotificationObserver,
-                 public base::RefCountedThreadSafe<TopSites> {
+                 public base::RefCountedThreadSafe<TopSites>,
+                 public CancelableRequestProvider {
  public:
   explicit TopSites(Profile* profile);
 
@@ -82,9 +83,13 @@ class TopSites : public NotificationObserver,
                         const SkBitmap& thumbnail,
                         const ThumbnailScore& score);
 
-  // Returns a list of most visited URLs or an empty list if it's not
-  // cached yet.
-  MostVisitedURLList GetMostVisitedURLs();
+  // Callback for GetMostVisitedURLs.
+  typedef Callback1<const MostVisitedURLList&>::Type GetTopSitesCallback;
+
+  // Returns a list of most visited URLs via a callback.
+  // NOTE: the callback may be called immediately if we have the data cached.
+  void GetMostVisitedURLs(CancelableRequestConsumer* consumer,
+                          GetTopSitesCallback* callback);
 
   // Get a thumbnail for a given page. Returns true iff we have the thumbnail.
   bool GetPageThumbnail(const GURL& url, RefCountedBytes** data) const;
@@ -104,6 +109,8 @@ class TopSites : public NotificationObserver,
   friend class TopSitesTest_DeleteNotifications_Test;
   friend class TopSitesTest_GetUpdateDelay_Test;
   friend class TopSitesTest_Migration_Test;
+  friend class TopSitesTest_QueueingRequestsForTopSites_Test;
+  friend class TopSitesTest_CancelingRequestsForTopSites_Test;
 
   ~TopSites();
 
@@ -240,6 +247,16 @@ class TopSites : public NotificationObserver,
   // URLs for which we are expecting thumbnails.
   std::set<GURL> migration_pending_urls_;
 
+  // The map of requests for the top sites list. Can only be
+  // non-empty at startup. After we read the top sites from the DB, we'll
+  // always have a cached list.
+  typedef std::set<scoped_refptr<CancelableRequest<GetTopSitesCallback> > >
+      PendingCallbackSet;
+  PendingCallbackSet pending_callbacks_;
+
+  // Are we waiting for the top sites from HistoryService?
+  bool waiting_for_results_;
+
   // TODO(brettw): use the blacklist.
   // std::set<GURL> blacklist_;
 
diff --git a/chrome/browser/history/top_sites_unittest.cc b/chrome/browser/history/top_sites_unittest.cc
index a383ee8..e81219e 100644
--- a/chrome/browser/history/top_sites_unittest.cc
+++ b/chrome/browser/history/top_sites_unittest.cc
@@ -24,17 +24,20 @@ static const unsigned char kBlob[] =
 
 class TopSitesTest : public testing::Test {
  public:
-  TopSitesTest() {
+  TopSitesTest() : number_of_callbacks_(0) {
   }
   ~TopSitesTest() {
   }
 
   TopSites& top_sites() { return *top_sites_; }
+  MostVisitedURLList& urls() { return urls_; }
   Profile& profile() {return *profile_;}
   FilePath& file_name() { return file_name_; }
   RefCountedBytes* google_thumbnail() { return google_thumbnail_; }
   RefCountedBytes* random_thumbnail() { return random_thumbnail_; }
   RefCountedBytes* weewar_thumbnail() { return weewar_thumbnail_; }
+  CancelableRequestConsumer* consumer() { return &consumer_; }
+  size_t number_of_callbacks() {return number_of_callbacks_; }
 
   virtual void SetUp() {
     profile_.reset(new TestingProfile);
@@ -62,6 +65,12 @@ class TopSitesTest : public testing::Test {
     EXPECT_TRUE(file_util::Delete(file_name_, false));
   }
 
+  // Callback for TopSites::GetMostVisitedURLs.
+  void OnTopSitesAvailable(const history::MostVisitedURLList& data) {
+    urls_ = data;
+    number_of_callbacks_++;
+  }
+
   // Wrappers that allow private TopSites functions to be called from the
   // individual tests without making them all be friends.
   GURL GetCanonicalURL(const GURL& url) const {
@@ -85,6 +94,8 @@ class TopSitesTest : public testing::Test {
 
  private:
   scoped_refptr<TopSites> top_sites_;
+  MostVisitedURLList urls_;
+  size_t number_of_callbacks_;
   scoped_ptr<TestingProfile> profile_;
   ScopedTempDir temp_dir_;
   FilePath file_name_;  // Database filename.
@@ -92,6 +103,7 @@ class TopSitesTest : public testing::Test {
   scoped_refptr<RefCountedBytes> random_thumbnail_;
   scoped_refptr<RefCountedBytes> weewar_thumbnail_;
   MessageLoop message_loop_;
+  CancelableRequestConsumer consumer_;
 
   DISALLOW_COPY_AND_ASSIGN(TopSitesTest);
 };
@@ -395,10 +407,13 @@ TEST_F(TopSitesTest, GetMostVisited) {
 
   top_sites().StartQueryForMostVisited();
   MessageLoop::current()->RunAllPending();
-  MostVisitedURLList results = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(2u, results.size());
-  EXPECT_EQ(news, results[0].url);
-  EXPECT_EQ(google, results[1].url);
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(2u, urls().size());
+  EXPECT_EQ(news, urls()[0].url);
+  EXPECT_EQ(google, urls()[1].url);
 }
 
 TEST_F(TopSitesTest, MockDatabase) {
@@ -422,10 +437,13 @@ TEST_F(TopSitesTest, MockDatabase) {
 
   top_sites().ReadDatabase();
 
-  MostVisitedURLList result = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(1u, result.size());
-  EXPECT_EQ(asdf_url, result[0].url);
-  EXPECT_EQ(asdf_title, result[0].title);
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(1u, urls().size());
+  EXPECT_EQ(asdf_url, urls()[0].url);
+  EXPECT_EQ(asdf_title, urls()[0].title);
 
   MostVisitedURL url2;
   url2.url = google_url;
@@ -437,12 +455,15 @@ TEST_F(TopSitesTest, MockDatabase) {
 
   top_sites().ReadDatabase();
 
-  result = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(2u, result.size());
-  EXPECT_EQ(google_url, result[0].url);
-  EXPECT_EQ(google_title, result[0].title);
-  EXPECT_EQ(asdf_url, result[1].url);
-  EXPECT_EQ(asdf_title, result[1].title);
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(2u, urls().size());
+  EXPECT_EQ(google_url, urls()[0].url);
+  EXPECT_EQ(google_title, urls()[0].title);
+  EXPECT_EQ(asdf_url, urls()[1].url);
+  EXPECT_EQ(asdf_title, urls()[1].title);
 
   MockHistoryServiceImpl hs;
   // Add one old, one new URL to the history.
@@ -455,6 +476,7 @@ TEST_F(TopSitesTest, MockDatabase) {
   MessageLoop::current()->RunAllPending();
 
   std::map<GURL, TopSites::Images> thumbnails;
+  MostVisitedURLList result;
   db->GetPageThumbnails(&result, &thumbnails);
   ASSERT_EQ(2u, result.size());
   EXPECT_EQ(google_title, result[0].title);
@@ -571,10 +593,13 @@ TEST_F(TopSitesTest, RealDatabase) {
 
   top_sites().ReadDatabase();
 
-  MostVisitedURLList result = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(1u, result.size());
-  EXPECT_EQ(asdf_url, result[0].url);
-  EXPECT_EQ(asdf_title, result[0].title);
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(1u, urls().size());
+  EXPECT_EQ(asdf_url, urls()[0].url);
+  EXPECT_EQ(asdf_title, urls()[0].title);
 
   TopSites::Images img_result;
   db->GetPageThumbnail(asdf_url, &img_result);
@@ -600,19 +625,22 @@ TEST_F(TopSitesTest, RealDatabase) {
 
   top_sites().ReadDatabase();
 
-  result = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(2u, result.size());
-  EXPECT_EQ(google1_url, result[0].url);
-  EXPECT_EQ(google_title, result[0].title);
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(2u, urls().size());
+  EXPECT_EQ(google1_url, urls()[0].url);
+  EXPECT_EQ(google_title, urls()[0].title);
   EXPECT_TRUE(top_sites().GetPageThumbnail(google1_url, &thumbnail_result));
   EXPECT_TRUE(ThumbnailsAreEqual(google_thumbnail(), thumbnail_result));
-  ASSERT_EQ(3u, result[0].redirects.size());
-  EXPECT_EQ(google1_url, result[0].redirects[0]);
-  EXPECT_EQ(google2_url, result[0].redirects[1]);
-  EXPECT_EQ(google3_url, result[0].redirects[2]);
+  ASSERT_EQ(3u, urls()[0].redirects.size());
+  EXPECT_EQ(google1_url, urls()[0].redirects[0]);
+  EXPECT_EQ(google2_url, urls()[0].redirects[1]);
+  EXPECT_EQ(google3_url, urls()[0].redirects[2]);
 
-  EXPECT_EQ(asdf_url, result[1].url);
-  EXPECT_EQ(asdf_title, result[1].title);
+  EXPECT_EQ(asdf_url, urls()[1].url);
+  EXPECT_EQ(asdf_title, urls()[1].title);
 
   MockHistoryServiceImpl hs;
   // Add one old, one new URL to the history.
@@ -625,10 +653,11 @@ TEST_F(TopSitesTest, RealDatabase) {
   MessageLoop::current()->RunAllPending();
 
   std::map<GURL, TopSites::Images> thumbnails;
-  db->GetPageThumbnails(&result, &thumbnails);
-  ASSERT_EQ(2u, result.size());
-  EXPECT_EQ(google_title, result[0].title);
-  EXPECT_EQ(news_title, result[1].title);
+  MostVisitedURLList results;
+  db->GetPageThumbnails(&results, &thumbnails);
+  ASSERT_EQ(2u, results.size());
+  EXPECT_EQ(google_title, results[0].title);
+  EXPECT_EQ(news_title, results[1].title);
 
   scoped_ptr<SkBitmap> weewar_bitmap(
       gfx::JPEGCodec::Decode(weewar_thumbnail()->front(),
@@ -696,8 +725,11 @@ TEST_F(TopSitesTest, DeleteNotifications) {
   top_sites().StartQueryForMostVisited();
   MessageLoop::current()->RunAllPending();
 
-  MostVisitedURLList result = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(2u, result.size());
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(2u, urls().size());
 
   hs.RemoveMostVisitedURL();
 
@@ -708,9 +740,12 @@ TEST_F(TopSitesTest, DeleteNotifications) {
                       (const NotificationDetails&)details);
   MessageLoop::current()->RunAllPending();
 
-  result = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(1u, result.size());
-  EXPECT_EQ(google_title, result[0].title);
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(1u, urls().size());
+  EXPECT_EQ(google_title, urls()[0].title);
 
   hs.RemoveMostVisitedURL();
   details.all_history = true;
@@ -718,8 +753,11 @@ TEST_F(TopSitesTest, DeleteNotifications) {
                       Source<Profile> (&profile()),
                       (const NotificationDetails&)details);
   MessageLoop::current()->RunAllPending();
-  result = top_sites().GetMostVisitedURLs();
-  ASSERT_EQ(0u, result.size());
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(0u, urls().size());
 }
 
 TEST_F(TopSitesTest, GetUpdateDelay) {
@@ -761,4 +799,107 @@ TEST_F(TopSitesTest, Migration) {
   EXPECT_FALSE(top_sites().migration_in_progress_);
 }
 
+TEST_F(TopSitesTest, QueueingRequestsForTopSites) {
+  ChromeThread db_loop(ChromeThread::DB, MessageLoop::current());
+  CancelableRequestConsumer c1;
+  CancelableRequestConsumer c2;
+  CancelableRequestConsumer c3;
+  top_sites().GetMostVisitedURLs(
+      &c1,
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+
+  top_sites().GetMostVisitedURLs(
+      &c2,
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+
+  top_sites().GetMostVisitedURLs(
+      &c3,
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+
+  EXPECT_EQ(0u, number_of_callbacks());
+  EXPECT_EQ(0u, urls().size());
+
+  MostVisitedURLList pages;
+  MostVisitedURL url;
+  url.url = GURL("http://1.com/");
+  url.redirects.push_back(url.url);
+  pages.push_back(url);
+  url.url = GURL("http://2.com/");
+  url.redirects.push_back(url.url);
+  pages.push_back(url);
+  top_sites().OnTopSitesAvailable(0, pages);
+  MessageLoop::current()->RunAllPending();
+
+  EXPECT_EQ(3u, number_of_callbacks());
+
+  ASSERT_EQ(2u, urls().size());
+  EXPECT_EQ("http://1.com/", urls()[0].url.spec());
+  EXPECT_EQ("http://2.com/", urls()[1].url.spec());
+
+  url.url = GURL("http://3.com/");
+  url.redirects.push_back(url.url);
+  pages.push_back(url);
+  top_sites().OnTopSitesAvailable(0, pages);
+  MessageLoop::current()->RunAllPending();
+
+  top_sites().GetMostVisitedURLs(
+      &c3,
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+
+  EXPECT_EQ(4u, number_of_callbacks());
+
+  ASSERT_EQ(3u, urls().size());
+  EXPECT_EQ("http://1.com/", urls()[0].url.spec());
+  EXPECT_EQ("http://2.com/", urls()[1].url.spec());
+  EXPECT_EQ("http://3.com/", urls()[2].url.spec());
+}
+
+TEST_F(TopSitesTest, CancelingRequestsForTopSites) {
+  CancelableRequestConsumer c1;
+  CancelableRequestConsumer c2;
+  top_sites().GetMostVisitedURLs(
+      &c1,
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+
+  top_sites().GetMostVisitedURLs(
+      &c2,
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+
+  {
+    CancelableRequestConsumer c3;
+    top_sites().GetMostVisitedURLs(
+        &c3,
+        NewCallback(static_cast<TopSitesTest*>(this),
+                    &TopSitesTest::OnTopSitesAvailable));
+    // c3 is out of scope, and the request should be cancelled.
+  }
+
+  // No requests until OnTopSitesAvailable is called.
+  EXPECT_EQ(0u, number_of_callbacks());
+  EXPECT_EQ(0u, urls().size());
+
+  MostVisitedURLList pages;
+  MostVisitedURL url;
+  url.url = GURL("http://1.com/");
+  url.redirects.push_back(url.url);
+  pages.push_back(url);
+  url.url = GURL("http://2.com/");
+  pages.push_back(url);
+
+  top_sites().OnTopSitesAvailable(0, pages);
+
+  // 1 request was canceled.
+  EXPECT_EQ(2u, number_of_callbacks());
+
+  ASSERT_EQ(2u, urls().size());
+  EXPECT_EQ("http://1.com/", urls()[0].url.spec());
+  EXPECT_EQ("http://2.com/", urls()[1].url.spec());
+}
+
 }  // namespace history