Write ThumbnailStore's cache to disk on browser shutdown.

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

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

4 files changed, 79 insertions, 32 deletions

diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index 72bf157..36517c4 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -604,6 +604,12 @@ ProfileImpl::~ProfileImpl() {
   // The theme provider provides bitmaps to whoever wants them.
   theme_provider_ = NULL;
 
+  // The ThumbnailStore saves thumbnails used by the NTP.  Call Shutdown to
+  // save any new thumbnails to disk and release its reference to the
+  // HistoryService.
+  if (thumbnail_store_.get())
+    thumbnail_store_->Shutdown();
+
   // Remove pref observers.
   PrefService* prefs = GetPrefs();
   prefs->RemovePrefObserver(prefs::kSpellCheckDictionary, this);
diff --git a/chrome/browser/thumbnail_store.cc b/chrome/browser/thumbnail_store.cc
index 9c8c315..e6f0e99 100644
--- a/chrome/browser/thumbnail_store.cc
+++ b/chrome/browser/thumbnail_store.cc
@@ -30,7 +30,8 @@ ThumbnailStore::ThumbnailStore()
 }
 
 ThumbnailStore::~ThumbnailStore() {
-  CommitCacheToDB(NULL);
+  // Ensure that shutdown was called.
+  DCHECK(hs_ == NULL);
 }
 
 void ThumbnailStore::Init(const FilePath& db_name,
@@ -132,6 +133,26 @@ bool ThumbnailStore::GetPageThumbnail(
   return true;
 }
 
+void ThumbnailStore::Shutdown() {
+  // We must release our reference to the HistoryService here to prevent
+  // shutdown issues. Please refer to the comment in HistoryService::Cleanup
+  // for details.
+  hs_ = NULL;
+
+  // The source of notifications is the Profile. We may outlive the Profile so
+  // we unregister for notifications here.
+  registrar_.RemoveAll();
+
+  // Stop the timer to ensure that UpdateURLData is not called during shutdown.
+  timer_.Stop();
+
+  // Write the cache to disk.  This will schedule the disk operations to be run
+  // on the file_thread.  Note that Join() does not need to be called with the
+  // file_thread because when the disk operation is scheduled, it will hold a
+  // reference to |this| keeping this object alive.
+  CleanCacheData();
+}
+
 void ThumbnailStore::OnRedirectsForURLAvailable(
     HistoryService::Handle handle,
     GURL url,
@@ -201,13 +222,15 @@ void ThumbnailStore::CleanCacheData() {
 
   scoped_refptr<RefCountedVector<GURL> > urls_to_delete =
       new RefCountedVector<GURL>;
+  Cache* data_to_save = new Cache;  // CommitCacheToDB will delete this
 
-  // For each URL in the cache, search the RedirectMap for the originating URL.
-  // If this URL is blacklisted or not in the most visited list, delete the
-  // thumbnail data for it from the cache and from disk in the background.
+  // Iterate the cache, storing urls to be deleted and dirty cache entries to
+  // be written to disk.
   for (Cache::iterator cache_it = cache_->begin();
        cache_it != cache_->end();) {
     const GURL* url = NULL;
+    // For each URL in the cache, search the RedirectMap for the originating
+    // URL.
     for (history::RedirectMap::iterator it = redirect_urls_->begin();
          it != redirect_urls_->end(); ++it) {
       if (cache_it->first == it->first ||
@@ -218,21 +241,33 @@ void ThumbnailStore::CleanCacheData() {
       }
     }
 
+    // If this URL is blacklisted or not in the most visited list, mark it for
+    // deletion. Otherwise, if the cache entry is dirty, mark it to be written
+    // to disk.
     if (url == NULL || IsURLBlacklisted(*url) || !IsPopular(*url)) {
+      // Note that we don't check whether the cache entry is dirty or not. If
+      // it is not dirty, then the thumbnail exists on disk and must be
+      // deleted. If it is dirty, it may exist on disk so we delete it anyways.
       urls_to_delete->data.push_back(cache_it->first);
       cache_->erase(cache_it++);
     } else {
+      if (cache_it->second.dirty_) {
+        data_to_save->insert(*cache_it);
+        cache_it->second.dirty_ = false;
+      }
       ++cache_it;
     }
   }
 
   g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(this, &ThumbnailStore::CommitCacheToDB,
-                        urls_to_delete));
+                        urls_to_delete, data_to_save));
 }
 
 void ThumbnailStore::CommitCacheToDB(
-    scoped_refptr<RefCountedVector<GURL> > urls_to_delete) const {
+    scoped_refptr<RefCountedVector<GURL> > urls_to_delete,
+    Cache* data) const {
+  scoped_ptr<Cache> data_to_save(data);
   if (!db_)
     return;
 
@@ -252,26 +287,24 @@ void ThumbnailStore::CommitCacheToDB(
   }
 
   // Update cached thumbnails.
-  for (Cache::iterator it = cache_->begin(); it != cache_->end(); ++it) {
-    if (!it->second.dirty_)
-      continue;
-
-    SQLITE_UNIQUE_STATEMENT(statement, *statement_cache_,
-        "INSERT OR REPLACE INTO thumbnails "
-        "(url, boring_score, good_clipping, at_top, time_taken, data) "
-        "VALUES (?,?,?,?,?,?)");
-    statement->bind_string(0, it->first.spec());
-    statement->bind_double(1, it->second.score_.boring_score);
-    statement->bind_bool(2, it->second.score_.good_clipping);
-    statement->bind_bool(3, it->second.score_.at_top);
-    statement->bind_int64(4, it->second.score_.time_at_snapshot.
-                             ToInternalValue());
-    statement->bind_blob(5, &it->second.data_->data[0],
-                         static_cast<int>(it->second.data_->data.size()));
-    if (statement->step() != SQLITE_DONE)
-      DLOG(WARNING) << "Unable to insert thumbnail for URL";
-    else
-      it->second.dirty_ = false;
+  if (data_to_save.get()) {
+    for (Cache::iterator it = data_to_save->begin();
+         it != data_to_save->end(); ++it) {
+      SQLITE_UNIQUE_STATEMENT(statement, *statement_cache_,
+          "INSERT OR REPLACE INTO thumbnails "
+          "(url, boring_score, good_clipping, at_top, time_taken, data) "
+          "VALUES (?,?,?,?,?,?)");
+      statement->bind_string(0, it->first.spec());
+      statement->bind_double(1, it->second.score_.boring_score);
+      statement->bind_bool(2, it->second.score_.good_clipping);
+      statement->bind_bool(3, it->second.score_.at_top);
+      statement->bind_int64(4, it->second.score_.time_at_snapshot.
+                                  ToInternalValue());
+      statement->bind_blob(5, &it->second.data_->data[0],
+                           static_cast<int>(it->second.data_->data.size()));
+      if (statement->step() != SQLITE_DONE)
+        DLOG(WARNING) << "Unable to insert thumbnail for URL";
+    }
   }
 
   rv = sqlite3_exec(db_, "COMMIT", NULL, NULL, NULL);
diff --git a/chrome/browser/thumbnail_store.h b/chrome/browser/thumbnail_store.h
index d68965e..8b5fde8 100644
--- a/chrome/browser/thumbnail_store.h
+++ b/chrome/browser/thumbnail_store.h
@@ -55,6 +55,10 @@ class ThumbnailStore : public base::RefCountedThreadSafe<ThumbnailStore>,
   // Returns false if no thumbnail available.
   bool GetPageThumbnail(const GURL& url, RefCountedBytes** data);
 
+  // This is called when the browser is shutting down to write all dirty cache
+  // entries to disk.
+  void Shutdown();
+
  private:
   FRIEND_TEST(ThumbnailStoreTest, RetrieveFromCache);
   FRIEND_TEST(ThumbnailStoreTest, RetrieveFromDisk);
@@ -106,9 +110,10 @@ class ThumbnailStore : public base::RefCountedThreadSafe<ThumbnailStore>,
 
   // Remove stale data --------------------------------------------------------
 
-  // Remove entries from the in memory thumbnail cache and redirect lists
-  // cache that have been blacklisted or are not in the top kMaxCacheSize
-  // visited sites.
+  // Remove entries from the in memory thumbnail cache cache that have been
+  // blacklisted or are not in the top kMaxCacheSize visited sites.  Call
+  // CommitCacheToDB on the file_thread to remove these entries from disk and
+  // to also write new entries to disk.
   void CleanCacheData();
 
   // Disk operations ----------------------------------------------------------
@@ -130,7 +135,8 @@ class ThumbnailStore : public base::RefCountedThreadSafe<ThumbnailStore>,
   // Delete each URL in the given vector from the DB and write all dirty
   // cache entries to the DB.
   void CommitCacheToDB(
-      scoped_refptr<RefCountedVector<GURL> > urls_to_delete) const;
+      scoped_refptr<RefCountedVector<GURL> > urls_to_delete,
+      Cache* data) const;
 
   // Decide whether to store data ---------------------------------------------
 
@@ -178,10 +184,11 @@ class ThumbnailStore : public base::RefCountedThreadSafe<ThumbnailStore>,
   // Consumer for queries to the HistoryService.
   CancelableRequestConsumer consumer_;
 
+  // Registrar to get notified when the history is cleared.
   NotificationRegistrar registrar_;
 
   static const unsigned int kMaxCacheSize = 24;
-  static const int64 kInitialUpdateIntervalSecs = 20;
+  static const int64 kInitialUpdateIntervalSecs = 180;
   static const int64 kMaxUpdateIntervalSecs = 3600;
 
   DISALLOW_COPY_AND_ASSIGN(ThumbnailStore);
diff --git a/chrome/browser/thumbnail_store_unittest.cc b/chrome/browser/thumbnail_store_unittest.cc
index 483cdf1..331d5df 100644
--- a/chrome/browser/thumbnail_store_unittest.cc
+++ b/chrome/browser/thumbnail_store_unittest.cc
@@ -181,7 +181,8 @@ TEST_F(ThumbnailStoreTest, RetrieveFromDisk) {
   // Write the thumbnail to disk and retrieve it.
 
   store_->InitializeFromDB(db_name_, NULL);
-  store_->CommitCacheToDB(NULL);  // Write to the DB (dirty bit sould be set)
+  // Write to the DB (dirty bit sould be set)
+  store_->CommitCacheToDB(NULL, new ThumbnailStore::Cache(*store_->cache_));
   store_->cache_->clear();        // Clear it from the cache.
 
   // Read from the DB.