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 124934)
+++ chrome/browser/history/in_memory_url_index.cc	(working copy)
@@ -4,23 +4,31 @@
 
 #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 {
 
+InMemoryURLIndex::RestoreCacheObserver::~RestoreCacheObserver() {}
+
+InMemoryURLIndex::SaveCacheObserver::~SaveCacheObserver() {}
+
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     RebuildPrivateDataFromHistoryDBTask(InMemoryURLIndex* index)
     : index_(index),
-      succeeded_(false) {}
+      succeeded_(false) {
+}
 
 InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     ~RebuildPrivateDataFromHistoryDBTask() {}
@@ -37,8 +45,7 @@
 
 void InMemoryURLIndex::RebuildPrivateDataFromHistoryDBTask::
     DoneRunOnMainThread() {
-  if (succeeded_)
-    index_->DoneRebuidingPrivateDataFromHistoryDB(data_.release());
+    index_->DoneRebuidingPrivateDataFromHistoryDB(succeeded_, data_.release());
 }
 
 InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
@@ -47,6 +54,8 @@
     : 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 +73,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 +86,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 +132,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 +167,50 @@
 
 // Restoring from Cache --------------------------------------------------------
 
-void InMemoryURLIndex::RestoreFromCacheFile() {
+void InMemoryURLIndex::PostRestoreFromCacheFileTask() {
   FilePath path;
-  if (GetCacheFilePath(&path) && !shutdown_)
-    DoRestoreFromCacheFile(path);
+  if (!GetCacheFilePath(&path) || shutdown_)
+    return;
+  URLIndexPrivateData* restored_private_data = NULL;
+  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(URLIndexPrivateData* private_data) {
+  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(InMemoryURLIndex::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();
+  if (restore_cache_observer_)
+    restore_cache_observer_->OnCacheRestoreFinished(succeeded);
 }
 
 // Restoring from the History DB -----------------------------------------------
@@ -188,13 +224,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 +246,46 @@
 
 // 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)));
+    bool* succeeded = new bool;
+    *succeeded = false;
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&URLIndexPrivateData::WritePrivateDataToCacheFileTask,
+                   private_data_copy.get(), path, succeeded),
+        base::Bind(&InMemoryURLIndex::OnCacheSaveDone, AsWeakPtr(), succeeded,
+                   private_data_copy.release()));

[brettw, 2012/03/05 22:13:05]

You do get() and release() on the same var. I don't think the spec defines what
order the args get evaluated as, so this may do the wrong thing.

Also, this will leak if the browser exits while you're executing, since you
won't delete the data, which will turn the memory bots red. Actually, it's not
clear why you need to pass the data back to the UI thread in the first place
since all you do is delete it. You should be able to pass a scoped_ptr as the
arg (see base/bind_helpers.h and search for scoped_ptr for an example).

[mrossetti, 2012/03/06 03:49:30]

Thanks! That's exactly the kind of guidance I needed!

I'm now using a scoped_refptr for passing the copy of the private data around.
This eliminates the need to pass the copy to the completion routine for
deletion. I created a RefCountedBool for passing around the success status.
Please let me know if this is appropriate or too heavy for such a simple status
flag.

On 2012/03/05 22:13:05, brettw wrote:

+  } else {
+    // If there is no data in our index then delete any existing cache file.
+    content::BrowserThread::PostBlockingPoolTask(
+        FROM_HERE,
+        base::Bind(InMemoryURLIndex::DeleteCacheFile, path));
+  }
 }
 
-void InMemoryURLIndex::DoSaveToCacheFile(const FilePath& path) {
-  private_data_->SaveToFile(path);
+void InMemoryURLIndex::OnCacheSaveDone(bool* succeeded,

[brettw, 2012/03/05 22:13:05]

It looks like you leak this bool, and this will have the same problems as the
"copy" assuming you did. Maybe what you need here is to create a refcounted
threadsafe object containing both of these items (bool and data) and pass
scoped_refptrs as the arguments. This will keep it alive for the duration of
your calls and automatically clean everything up when you're done or if the
browser shuts down in the middle.

[mrossetti, 2012/03/06 03:49:30]

See above. I'm now using a ref-counted bool container class.

On 2012/03/05 22:13:05, brettw wrote:

+                                       URLIndexPrivateData* copy) {
+  DCHECK(copy);
+  delete copy;
+  if (save_cache_observer_)
+    save_cache_observer_->OnCacheSaveFinished(*succeeded);
 }
 
+// static
+void InMemoryURLIndex::DeleteCacheFile(const FilePath path) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  file_util::Delete(path, false);
+}
+
 }  // namespace history