Move InMemoryURLIndex Caching Operations to FILE Thread

chrome/browser/history/in_memory_url_index.cc

Index: chrome/browser/history/in_memory_url_index.cc
===================================================================
--- chrome/browser/history/in_memory_url_index.cc	(revision 125451)
+++ chrome/browser/history/in_memory_url_index.cc	(working copy)
@@ -4,23 +4,42 @@
 
 #include "chrome/browser/history/in_memory_url_index.h"
 
+#include "base/file_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/url_database.h"
 #include "chrome/browser/history/url_index_private_data.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 
 using in_memory_url_index::InMemoryURLIndexCacheItem;
 
 namespace history {
 
+// Called by DoSaveToCacheFile to delete any old cache file at |path| when
+// there is no private data to save. Runs on the FILE thread.
+void DeleteCacheFile(const FilePath& path) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  file_util::Delete(path, false);
+}
+
+// Restore/SaveCacheObserver ---------------------------------------------------
+
+InMemoryURLIndex::RestoreCacheObserver::~RestoreCacheObserver() {}
+
+InMemoryURLIndex::SaveCacheObserver::~SaveCacheObserver() {}
+
+// RebuildPrivateDataFromHistoryDBTask -----------------------------------------
+
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index)
     : index_(index),
-      succeeded_(false) {}
+      succeeded_(false) {
+}
 
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     ~RebuildPrivateDataFromHistoryDBTask() {}
@@ -37,16 +56,19 @@
 
 void InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     DoneRunOnMainThread() {
-  if (succeeded_)
-    index_->DoneRebuidingPrivateDataFromHistoryDB(data_.release());
+  index_->DoneRebuidingPrivateDataFromHistoryDB(succeeded_, data_.release());
 }
 
+// InMemoryURLIndex ------------------------------------------------------------
+
 InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
                                    const FilePath& history_dir,
                                    const std::string& languages)
     : profile_(profile),
       history_dir_(history_dir),
       private_data_(new URLIndexPrivateData),
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
       shutdown_(false),
       needs_to_be_cached_(false) {
   private_data_->set_languages(languages);
@@ -64,6 +86,8 @@
 InMemoryURLIndex::InMemoryURLIndex()
     : profile_(NULL),
       private_data_(new URLIndexPrivateData),
+      restore_cache_observer_(NULL),
+      save_cache_observer_(NULL),
       shutdown_(false),
       needs_to_be_cached_(false) {
 }
@@ -75,14 +99,14 @@
 }
 
 void InMemoryURLIndex::Init() {
-  RestoreFromCacheFile();
+  PostRestoreFromCacheFileTask();
 }
 
 void InMemoryURLIndex::ShutDown() {
   registrar_.RemoveAll();
   cache_reader_consumer_.CancelAllRequests();
   shutdown_ = true;
-  SaveToCacheFile();
+  PostSaveToCacheFileTask();
   needs_to_be_cached_ = false;
 }
 
@@ -121,12 +145,11 @@
       OnURLsDeleted(
           content::Details<history::URLsDeletedDetails>(details).ptr());
       break;
-    case chrome::NOTIFICATION_HISTORY_LOADED: {
+    case chrome::NOTIFICATION_HISTORY_LOADED:
       registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_LOADED,
                         content::Source<Profile>(profile_));
       ScheduleRebuildFromHistory();
       break;
-    }
     default:
       // For simplicity, the unit tests send us all notifications, even when
       // we haven't registered for them, so don't assert here.
@@ -157,24 +180,53 @@
 
 // Restoring from Cache --------------------------------------------------------
 
-void InMemoryURLIndex::RestoreFromCacheFile() {
+void InMemoryURLIndex::PostRestoreFromCacheFileTask() {
   FilePath path;
-  if (GetCacheFilePath(&path) && !shutdown_)
-    DoRestoreFromCacheFile(path);
+  if (!GetCacheFilePath(&path) || shutdown_)
+    return;
+  scoped_refptr<RefCountedURLIndexPrivateDataPtr> restored_private_data =
+      new RefCountedURLIndexPrivateDataPtr;
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&URLIndexPrivateData::RestoreFromFileTask, path,
+          restored_private_data),
+      base::Bind(&InMemoryURLIndex::OnCacheLoadDone, AsWeakPtr(),
+          restored_private_data));
 }
 
-void InMemoryURLIndex::DoRestoreFromCacheFile(const FilePath& path) {
-  if (private_data_->RestoreFromFile(path))
-    return;
+void InMemoryURLIndex::OnCacheLoadDone(
+    scoped_refptr<RefCountedURLIndexPrivateDataPtr> private_data_ptr) {
+  URLIndexPrivateData* private_data = private_data_ptr->release();
+  if (private_data) {
+    OnCacheRestored(private_data, true);
+  } else if (profile_) {
+    // When unable to restore from the cache file delete the cache file, if
+    // it exists, and then rebuild from the history database if it's available,
+    // otherwise wait until the history database loaded and then rebuild.
+    FilePath path;
+    if (!GetCacheFilePath(&path) || shutdown_)
+      return;
+    content::BrowserThread::PostBlockingPoolTask(
+        FROM_HERE, base::Bind(DeleteCacheFile, path));
+    HistoryService* service = profile_->GetHistoryServiceWithoutCreating();
+    if (service && service->backend_loaded())
+      ScheduleRebuildFromHistory();
+    else
+      registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
+                     content::Source<Profile>(profile_));
+  }
+}
 
-  // When unable to restore from the cache file we must rebuild from the
-  // history database.
-  HistoryService* service = profile_->GetHistoryServiceWithoutCreating();
-  if (service && service->backend_loaded())
-    ScheduleRebuildFromHistory();
-  // We must wait to rebuild until the history backend has been loaded.
-  registrar_.Add(this, chrome::NOTIFICATION_HISTORY_LOADED,
-                 content::Source<Profile>(profile_));
+void InMemoryURLIndex::OnCacheRestored(URLIndexPrivateData* private_data,
+                                       bool succeeded) {
+  // TODO(mrossetti): Take care of remembering any visit transactions while
+  // we were away rebuilding the index and then apply them here.
+  if (succeeded)
+    private_data_.reset(private_data);
+  else
+    private_data_->Clear();

[brettw, 2012/03/12 04:37:40]

You leak the data here.

[mrossetti, 2012/03/12 23:21:13]

Fixed.

On 2012/03/12 04:37:40, brettw wrote:

+  if (restore_cache_observer_)
+    restore_cache_observer_->OnCacheRestoreFinished(succeeded);
 }
 
 // Restoring from the History DB -----------------------------------------------
@@ -188,13 +240,20 @@
 }
 
 void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB(
+    bool succeeded,
     URLIndexPrivateData* data) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   scoped_ptr<URLIndexPrivateData> private_data(data);
-  private_data_.swap(private_data);
-  // Cache the newly rebuilt index.
-  FilePath cache_file_path;
-  if (GetCacheFilePath(&cache_file_path))
-    private_data_->SaveToFile(cache_file_path);
+  if (succeeded) {
+    private_data_.swap(private_data);
+    PostSaveToCacheFileTask();  // Cache the newly rebuilt index.
+  } else {
+    private_data_->Clear();  // Dump the old private data.
+    // There is no need to do anything with the cache file as it was deleted
+    // when the rebuild from the history operation was kicked off.
+  }
+  if (restore_cache_observer_)
+    restore_cache_observer_->OnCacheRestoreFinished(succeeded);
 }
 
 void InMemoryURLIndex::RebuildFromHistory(HistoryDatabase* history_db) {
@@ -203,14 +262,36 @@
 
 // Saving to Cache -------------------------------------------------------------
 
-void InMemoryURLIndex::SaveToCacheFile() {
+void InMemoryURLIndex::PostSaveToCacheFileTask() {
   FilePath path;
-  if (GetCacheFilePath(&path))
-    DoSaveToCacheFile(path);
+  if (!GetCacheFilePath(&path))
+    return;
+  // If there is anything in our private data then make a copy of it and tell
+  // it to save itself to a file.
+  URLIndexPrivateData* private_data = private_data_.get();
+  if (private_data && !private_data->Empty()) {
+    // Note that ownership of the copy of our private data is passed to the
+    // completion closure below.
+    scoped_ptr<URLIndexPrivateData> private_data_copy(
+        new URLIndexPrivateData(*private_data));
+    scoped_refptr<RefCountedBool> succeeded(new RefCountedBool(false));
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&URLIndexPrivateData::WritePrivateDataToCacheFileTask,
+                   base::Passed(&private_data_copy), path, succeeded),
+        base::Bind(&InMemoryURLIndex::OnCacheSaveDone, AsWeakPtr(), succeeded));
+  } else {
+    // If there is no data in our index then delete any existing cache file.
+    content::BrowserThread::PostBlockingPoolTask(
+        FROM_HERE,
+        base::Bind(DeleteCacheFile, path));
+  }
 }
 
-void InMemoryURLIndex::DoSaveToCacheFile(const FilePath& path) {
-  private_data_->SaveToFile(path);
+void InMemoryURLIndex::OnCacheSaveDone(
+    scoped_refptr<RefCountedBool> succeeded) {
+  if (save_cache_observer_)
+    save_cache_observer_->OnCacheSaveFinished(succeeded->value());
 }
 
 }  // namespace history