Take 2: Preload the visited link db on the file thread if

the file exists. Otherwise, just load like normal on the UI thread.

This failed before because the browser may have shutdown before the
posted task ran.  When the posted task finally runs, it tried to
use the profile, but the profile was already deleted.  Make a small
change to VisitedLinkMaster so GetDatabaseFileName no longer depends
on the profile.

BUG=24163

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

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

5 files changed, 208 insertions, 91 deletions

diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index 0c57f3a..8eb5409 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -44,6 +44,7 @@
 #include "chrome/browser/plugin_service.h"
 #include "chrome/browser/process_singleton.h"
 #include "chrome/browser/profile_manager.h"
+#include "chrome/browser/profile.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/user_data_manager.h"
@@ -100,7 +101,6 @@
 #include "chrome/browser/browser_trial.h"
 #include "chrome/browser/metrics/user_metrics.h"
 #include "chrome/browser/net/url_fixer_upper.h"
-#include "chrome/browser/profile.h"
 #include "chrome/browser/rlz/rlz.h"
 #include "chrome/browser/views/user_data_dir_dialog.h"
 #include "chrome/common/env_vars.h"
@@ -683,6 +683,10 @@ int BrowserMain(const MainFunctionParams& parameters) {
   CHECK(profile) << "Cannot get default profile.";
 #endif
 
+  // Now that we have a profile, try to load the visited link master on a
+  // background thread.  We want to start this as soon as we can.
+  profile->PreloadVisitedLinkMaster();
+
   PrefService* user_prefs = profile->GetPrefs();
   DCHECK(user_prefs);
 
diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc
index 3231ede2..fefbc8d 100644
--- a/chrome/browser/profile.cc
+++ b/chrome/browser/profile.cc
@@ -125,6 +125,87 @@ bool HasACacheSubdir(const FilePath &dir) {
 
 }  // namespace
 
+// This helper class owns the VisitedLinkMaster and exposes a way to load it
+// from on the file thread.
+class VisitedLinkCreator
+    : public base::RefCountedThreadSafe<VisitedLinkCreator> {
+ public:
+  enum LoadState {
+    NOT_LOADED,
+    FILE_LOAD_FAILED,
+    LOAD_FINISHED
+  };
+
+  explicit VisitedLinkCreator(Profile* profile)
+      : listener_(new VisitedLinkEventListener()),
+        profile_(profile),
+        load_state_(NOT_LOADED) {
+    visited_link_master_.reset(new VisitedLinkMaster(listener_.get(),
+                                                     profile_));
+  }
+
+  // Preload the visited link master on the file thread.
+  void Preload() {
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+    ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE, NewRunnableMethod(
+        this, &VisitedLinkCreator::LoadBackground));
+  }
+
+  // This method can return NULL if for some reason we can't initialize the
+  // visited link master.
+  VisitedLinkMaster* GetMaster() {
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+    // Go ahead and try to load the VisitedLink table.  If the data was
+    // preloaded, Load() does nothing.
+    AutoLock l(lock_);
+    if (LOAD_FINISHED != load_state_) {
+      bool success = false;
+      if (FILE_LOAD_FAILED == load_state_) {
+        success = visited_link_master_->InitFromScratch();
+      } else if (NOT_LOADED == load_state_) {
+        // We haven't tried to load from file yet, so go ahead and do that.
+        success = visited_link_master_->Init();
+      }
+
+      if (!success) {
+        NOTREACHED() << "Failed to init visited link master.";
+        visited_link_master_.reset();
+      }
+      load_state_ = LOAD_FINISHED;
+    }
+
+    // We don't need to lock here because after Load() has been called, we
+    // never change vistied_link_master_.
+    return visited_link_master_.get();
+  }
+
+ private:
+  // Tries to load the visited link database from the UI thread.
+  void LoadBackground() {
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+    AutoLock l(lock_);
+    if (NOT_LOADED != load_state_)
+      return;
+
+    // We only want to try to load the data from a file (it's not thread safe
+    // to InitFromScratch on the file thread).
+    load_state_ = visited_link_master_->InitFromFile() ? LOAD_FINISHED
+                                                       : FILE_LOAD_FAILED;
+  }
+
+  scoped_ptr<VisitedLinkMaster::Listener> listener_;
+  Profile* profile_;
+
+  // This lock protects visited_link_master_ and load_state_.
+  Lock lock_;
+  scoped_ptr<VisitedLinkMaster> visited_link_master_;
+  // Once created_ is true, we can stop trying to create and init the
+  // VisitedLinkMaster.
+  LoadState load_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(VisitedLinkCreator);
+};
+
 // A pointer to the request context for the default profile.  See comments on
 // Profile::GetDefaultRequestContext.
 URLRequestContextGetter* Profile::default_request_context_;
@@ -214,7 +295,9 @@ class OffTheRecordProfileImpl : public Profile,
     return reinterpret_cast<ProfileId>(this);
   }
 
-  virtual FilePath GetPath() { return profile_->GetPath(); }
+  virtual FilePath GetPath() {
+    return profile_->GetPath();
+  }
 
   virtual bool IsOffTheRecord() {
     return true;
@@ -245,6 +328,8 @@ class OffTheRecordProfileImpl : public Profile,
     return NULL;
   }
 
+  virtual void PreloadVisitedLinkMaster() {}
+
   virtual ExtensionsService* GetExtensionsService() {
     return NULL;
   }
@@ -567,7 +652,6 @@ class OffTheRecordProfileImpl : public Profile,
 
 ProfileImpl::ProfileImpl(const FilePath& path)
     : path_(path),
-      visited_link_event_listener_(new VisitedLinkEventListener()),
       extension_devtools_manager_(NULL),
       request_context_(NULL),
       media_request_context_(NULL),
@@ -814,15 +898,16 @@ webkit_database::DatabaseTracker* ProfileImpl::GetDatabaseTracker() {
 }
 
 VisitedLinkMaster* ProfileImpl::GetVisitedLinkMaster() {
-  if (!visited_link_master_.get()) {
-    scoped_ptr<VisitedLinkMaster> visited_links(
-      new VisitedLinkMaster(visited_link_event_listener_.get(), this));
-    if (!visited_links->Init())
-      return NULL;
-    visited_link_master_.swap(visited_links);
-  }
+  if (!visited_link_creator_.get())
+    visited_link_creator_ = new VisitedLinkCreator(this);
+  return visited_link_creator_->GetMaster();
+}
 
-  return visited_link_master_.get();
+void ProfileImpl::PreloadVisitedLinkMaster() {
+  if (!visited_link_creator_.get()) {
+    visited_link_creator_ = new VisitedLinkCreator(this);
+    visited_link_creator_->Preload();
+  }
 }
 
 ExtensionsService* ProfileImpl::GetExtensionsService() {
diff --git a/chrome/browser/profile.h b/chrome/browser/profile.h
index 1d15753..95f76b0 100644
--- a/chrome/browser/profile.h
+++ b/chrome/browser/profile.h
@@ -64,8 +64,8 @@ class ThemeProvider;
 class ThumbnailStore;
 class URLRequestContextGetter;
 class UserScriptMaster;
+class VisitedLinkCreator;
 class VisitedLinkMaster;
-class VisitedLinkEventListener;
 class WebDataService;
 class WebKitContext;
 class WebResourceService;
@@ -148,6 +148,10 @@ class Profile {
   // that this method is called.
   virtual VisitedLinkMaster* GetVisitedLinkMaster() = 0;
 
+  // Loads the visited link master on the file thread.  It's safe to call
+  // GetVisitedLinkMaster without calling this in advance.
+  virtual void PreloadVisitedLinkMaster() = 0;
+
   // Retrieves a pointer to the ExtensionsService associated with this
   // profile. The ExtensionsService is created at startup.
   virtual ExtensionsService* GetExtensionsService() = 0;
@@ -406,6 +410,7 @@ class ProfileImpl : public Profile,
   virtual Profile* GetOriginalProfile();
   virtual webkit_database::DatabaseTracker* GetDatabaseTracker();
   virtual VisitedLinkMaster* GetVisitedLinkMaster();
+  virtual void PreloadVisitedLinkMaster();
   virtual UserScriptMaster* GetUserScriptMaster();
   virtual SSLHostState* GetSSLHostState();
   virtual net::TransportSecurityState* GetTransportSecurityState();
@@ -491,8 +496,7 @@ class ProfileImpl : public Profile,
 
   FilePath path_;
   FilePath base_cache_path_;
-  scoped_ptr<VisitedLinkEventListener> visited_link_event_listener_;
-  scoped_ptr<VisitedLinkMaster> visited_link_master_;
+  scoped_refptr<VisitedLinkCreator> visited_link_creator_;
   scoped_refptr<ExtensionsService> extensions_service_;
   scoped_refptr<UserScriptMaster> user_script_master_;
   scoped_refptr<ExtensionDevToolsManager> extension_devtools_manager_;
diff --git a/chrome/browser/visitedlink_master.cc b/chrome/browser/visitedlink_master.cc
index c2aa3fe..58769dc 100644
--- a/chrome/browser/visitedlink_master.cc
+++ b/chrome/browser/visitedlink_master.cc
@@ -24,7 +24,6 @@
 #include "base/rand_util.h"
 #include "base/stack_container.h"
 #include "base/string_util.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/profile.h"
@@ -144,6 +143,24 @@ class AsyncCloseHandle : public Task {
   DISALLOW_COPY_AND_ASSIGN(AsyncCloseHandle);
 };
 
+// A helper class for clearing |profile_pointer_| then re-setting it when the
+// object goes out of scope.  This helps to make sure we don't accidentally
+// use the pointer when we're not supposed to.
+class ScopedNullProfile {
+ public:
+  ScopedNullProfile(Profile** profile)
+      : profile_(*profile),
+        profile_pointer_(profile) {
+    *profile_pointer_ = NULL;
+  }
+  ~ScopedNullProfile() {
+    *profile_pointer_ = profile_;
+  }
+ private:
+  Profile* profile_;
+  Profile** profile_pointer_;
+};
+
 }  // namespace
 
 // TableBuilder ---------------------------------------------------------------
@@ -248,6 +265,9 @@ void VisitedLinkMaster::InitMembers(Listener* listener, Profile* profile) {
   suppress_rebuild_ = false;
   profile_ = profile;
 
+  if (profile_)
+    profile_dir_ = profile_->GetPath();
+
 #ifndef NDEBUG
   posted_asynchronous_operation_ = false;
 #endif
@@ -255,10 +275,74 @@ void VisitedLinkMaster::InitMembers(Listener* listener, Profile* profile) {
 
 bool VisitedLinkMaster::Init() {
   if (!InitFromFile())
-    return InitFromScratch(suppress_rebuild_);
+    return InitFromScratch();
+  return true;
+}
+
+bool VisitedLinkMaster::InitFromFile() {
+  // We allow running this method from background threads, so be careful not to
+  // access profile_ (which may already be deleted) from this method.
+  ScopedNullProfile scoped(&profile_);
+  DCHECK(file_ == NULL);
+  DCHECK(profile_ == NULL);
+
+  FilePath filename;
+  GetDatabaseFileName(&filename);
+  ScopedFILE file_closer(OpenFile(filename, "rb+"));
+  if (!file_closer.get())
+    return false;
+
+  int32 num_entries, used_count;
+  if (!ReadFileHeader(file_closer.get(), &num_entries, &used_count, salt_))
+    return false;  // Header isn't valid.
+
+  // Allocate and read the table.
+  if (!CreateURLTable(num_entries, false))
+    return false;
+  if (!ReadFromFile(file_closer.get(), kFileHeaderSize,
+                    hash_table_, num_entries * sizeof(Fingerprint))) {
+    FreeURLTable();
+    return false;
+  }
+  used_items_ = used_count;
+
+#ifndef NDEBUG
+  DebugValidate();
+#endif
+
+  file_ = file_closer.release();
   return true;
 }
 
+bool VisitedLinkMaster::InitFromScratch() {
+  int32 table_size = kDefaultTableSize;
+  if (table_size_override_)
+    table_size = table_size_override_;
+
+  // The salt must be generated before the table so that it can be copied to
+  // the shared memory.
+  GenerateSalt(salt_);
+  if (!CreateURLTable(table_size, true))
+    return false;
+
+#ifndef NDEBUG
+  DebugValidate();
+#endif
+
+  if (suppress_rebuild_) {
+    // When we disallow rebuilds (normally just unit tests), just use the
+    // current empty table.
+    return WriteFullTable();
+  }
+
+  // This will build the table from history. On the first run, history will
+  // be empty, so this will be correct. This will also write the new table
+  // to disk. We don't want to save explicitly here, since the rebuild may
+  // not complete, leaving us with an empty but valid visited link database.
+  // In the future, we won't know we need to try rebuilding again.
+  return RebuildTableFromHistory();
+}
+
 VisitedLinkMaster::Hash VisitedLinkMaster::TryToAddURL(const GURL& url) {
   // Extra check that we are not off the record. This should not happen.
   if (profile_ && profile_->IsOffTheRecord()) {
@@ -537,66 +621,6 @@ bool VisitedLinkMaster::WriteFullTable() {
   return true;
 }
 
-bool VisitedLinkMaster::InitFromFile() {
-  DCHECK(file_ == NULL);
-
-  FilePath filename;
-  GetDatabaseFileName(&filename);
-  ScopedFILE file_closer(OpenFile(filename, "rb+"));
-  if (!file_closer.get())
-    return false;
-
-  int32 num_entries, used_count;
-  if (!ReadFileHeader(file_closer.get(), &num_entries, &used_count, salt_))
-    return false;  // Header isn't valid.
-
-  // Allocate and read the table.
-  if (!CreateURLTable(num_entries, false))
-    return false;
-  if (!ReadFromFile(file_closer.get(), kFileHeaderSize,
-                    hash_table_, num_entries * sizeof(Fingerprint))) {
-    FreeURLTable();
-    return false;
-  }
-  used_items_ = used_count;
-
-#ifndef NDEBUG
-  DebugValidate();
-#endif
-
-  file_ = file_closer.release();
-  return true;
-}
-
-bool VisitedLinkMaster::InitFromScratch(bool suppress_rebuild) {
-  int32 table_size = kDefaultTableSize;
-  if (table_size_override_)
-    table_size = table_size_override_;
-
-  // The salt must be generated before the table so that it can be copied to
-  // the shared memory.
-  GenerateSalt(salt_);
-  if (!CreateURLTable(table_size, true))
-    return false;
-
-#ifndef NDEBUG
-  DebugValidate();
-#endif
-
-  if (suppress_rebuild) {
-    // When we disallow rebuilds (normally just unit tests), just use the
-    // current empty table.
-    return WriteFullTable();
-  }
-
-  // This will build the table from history. On the first run, history will
-  // be empty, so this will be correct. This will also write the new table
-  // to disk. We don't want to save explicitly here, since the rebuild may
-  // not complete, leaving us with an empty but valid visited link database.
-  // In the future, we won't know we need to try rebuilding again.
-  return RebuildTableFromHistory();
-}
-
 bool VisitedLinkMaster::ReadFileHeader(FILE* file,
                                        int32* num_entries,
                                        int32* used_count,
@@ -654,11 +678,10 @@ bool VisitedLinkMaster::GetDatabaseFileName(FilePath* filename) {
     return true;
   }
 
-  if (!profile_ || profile_->GetPath().empty())
+  if (profile_dir_.empty())
     return false;
 
-  FilePath profile_dir = profile_->GetPath();
-  *filename = profile_dir.Append(FILE_PATH_LITERAL("Visited Links"));
+  *filename = profile_dir_.Append(FILE_PATH_LITERAL("Visited Links"));
   return true;
 }
 
diff --git a/chrome/browser/visitedlink_master.h b/chrome/browser/visitedlink_master.h
index fc35ff9..14ba9d8 100644
--- a/chrome/browser/visitedlink_master.h
+++ b/chrome/browser/visitedlink_master.h
@@ -77,9 +77,18 @@ class VisitedLinkMaster : public VisitedLinkCommon {
 
   // Must be called immediately after object creation. Nothing else will work
   // until this is called. Returns true on success, false means that this
-  // object won't work.
+  // object won't work. You can also use InitFromFile() and InitFromScratch()
+  // if you need more control over loading the visited link information.
   bool Init();
 
+  // Try to load the table from the database file. If the file doesn't exist or
+  // is corrupt, this will return failure. This method may be called from a
+  // background thread, but it isn't thread safe.
+  bool InitFromFile();
+
+  // Creates a new empty table, call if InitFromFile() fails.
+  bool InitFromScratch();
+
   base::SharedMemory* shared_memory() { return shared_memory_; }
 
   // Adds a URL to the table.
@@ -167,10 +176,6 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // the table file open and the handle to it in file_
   bool WriteFullTable();
 
-  // Try to load the table from the database file. If the file doesn't exist or
-  // is corrupt, this will return failure.
-  bool InitFromFile();
-
   // Reads the header of the link coloring database from disk. Assumes the
   // file pointer is at the beginning of the file and that there are no pending
   // asynchronous I/O operations.
@@ -222,13 +227,6 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // fingerprint was deleted, false if it was not in the table to delete.
   bool DeleteFingerprint(Fingerprint fingerprint, bool update_file);
 
-  // Creates a new empty table, call if InitFromFile() fails. Normally, when
-  // |suppress_rebuild| is false, the table will be rebuilt from history,
-  // keeping us in sync. When |suppress_rebuild| is true, the new table will be
-  // empty and we will not consult history. This is used when clearing the
-  // database and for unit tests.
-  bool InitFromScratch(bool suppress_rebuild);
-
   // Allocates the Fingerprint structure and length. When init_to_empty is set,
   // the table will be filled with 0s and used_items_ will be set to 0 as well.
   // If the flag is not set, these things are untouched and it is the
@@ -369,6 +367,9 @@ class VisitedLinkMaster : public VisitedLinkCommon {
   // will be false in production.
   bool suppress_rebuild_;
 
+  // Keep a copy of the profile_dir in case we outlive the profile.
+  FilePath profile_dir_;
+
   DISALLOW_EVIL_CONSTRUCTORS(VisitedLinkMaster);
 };