Load the table of visited links from database file asynchronously.

components/visitedlink/browser/visitedlink_master.cc

Index: components/visitedlink/browser/visitedlink_master.cc
diff --git a/components/visitedlink/browser/visitedlink_master.cc b/components/visitedlink/browser/visitedlink_master.cc
index ee493415d2da9e4d44ed468d918d8bcad18ba6e6..f86ac051950cb7cfd9367c7cf47f637e7dbf13b0 100644
--- a/components/visitedlink/browser/visitedlink_master.cc
+++ b/components/visitedlink/browser/visitedlink_master.cc
@@ -116,6 +116,47 @@ void AsyncClose(FILE** file) {
 
 }  // namespace
 
+struct VisitedLinkMaster::LoadFromFileResult
+    : public base::RefCountedThreadSafe<LoadFromFileResult> {
+  LoadFromFileResult(base::ScopedFILE file,
+                     scoped_ptr<base::SharedMemory> shared_memory,
+                     Fingerprint* hash_table,
+                     int32 num_entries,
+                     int32 used_count,
+                     uint8 salt[LINK_SALT_LENGTH]);
+
+  base::ScopedFILE file;
+  scoped_ptr<base::SharedMemory> shared_memory;
+  Fingerprint* hash_table;
+  int32 num_entries;
+  int32 used_count;
+  uint8 salt[LINK_SALT_LENGTH];
+
+ private:
+  friend class base::RefCountedThreadSafe<LoadFromFileResult>;
+  virtual ~LoadFromFileResult();
+
+  DISALLOW_COPY_AND_ASSIGN(LoadFromFileResult);
+};
+
+VisitedLinkMaster::LoadFromFileResult::LoadFromFileResult(
+    base::ScopedFILE file,
+    scoped_ptr<base::SharedMemory> shared_memory,
+    Fingerprint* hash_table,
+    int32 num_entries,
+    int32 used_count,
+    uint8 salt[LINK_SALT_LENGTH])
+    : file(file.Pass()),
+      shared_memory(shared_memory.Pass()),
+      hash_table(hash_table),
+      num_entries(num_entries),
+      used_count(used_count) {
+  memcpy(this->salt, salt, LINK_SALT_LENGTH);
+}
+
+VisitedLinkMaster::LoadFromFileResult::~LoadFromFileResult() {
+}
+
 // TableBuilder ---------------------------------------------------------------
 
 // How rebuilding from history works
@@ -181,7 +222,9 @@ VisitedLinkMaster::VisitedLinkMaster(content::BrowserContext* browser_context,
     : browser_context_(browser_context),
       delegate_(delegate),
       listener_(new VisitedLinkEventListener(this, browser_context)),
-      persist_to_disk_(persist_to_disk) {
+      persist_to_disk_(persist_to_disk),
+      table_is_loading_from_file_(false),
+      weak_ptr_factory_(this) {
   InitMembers();
 }
 
@@ -193,7 +236,9 @@ VisitedLinkMaster::VisitedLinkMaster(Listener* listener,
                                      int32 default_table_size)
     : browser_context_(NULL),
       delegate_(delegate),
-      persist_to_disk_(persist_to_disk) {
+      persist_to_disk_(persist_to_disk),
+      table_is_loading_from_file_(false),
+      weak_ptr_factory_(this) {
   listener_.reset(listener);
   DCHECK(listener_.get());
   InitMembers();
@@ -214,6 +259,18 @@ VisitedLinkMaster::~VisitedLinkMaster() {
   FreeURLTable();
   // FreeURLTable() will schedule closing of the file and deletion of |file_|.
   // So nothing should be done here.
+
+  if (table_is_loading_from_file_ &&
+      (!added_since_rebuild_or_load_.empty() ||
+       !deleted_since_rebuild_or_load_.empty())) {
+    // Delete the database file if exist because we don't have enough time to
+    // load the table from the database file and now we have inconsistent
+    // state. On the next start table will be rebuilt.
+    base::FilePath filename;
+    GetDatabaseFileName(&filename);
+    PostIOTask(FROM_HERE,
+               base::Bind(IgnoreResult(&base::DeleteFile), filename, false));
+  }
 }
 
 void VisitedLinkMaster::InitMembers() {
@@ -224,18 +281,9 @@ void VisitedLinkMaster::InitMembers() {
   table_size_override_ = 0;
   suppress_rebuild_ = false;
   sequence_token_ = BrowserThread::GetBlockingPool()->GetSequenceToken();
-
-#ifndef NDEBUG
-  posted_asynchronous_operation_ = false;
-#endif
 }
 
 bool VisitedLinkMaster::Init() {
-  // We probably shouldn't be loading this from the UI thread,
-  // but it does need to happen early on in startup.
-  //   http://code.google.com/p/chromium/issues/detail?id=24163
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
-
   if (persist_to_disk_) {
     if (InitFromFile())
       return true;
@@ -258,16 +306,16 @@ VisitedLinkMaster::Hash VisitedLinkMaster::TryToAddURL(const GURL& url) {
   Fingerprint fingerprint = ComputeURLFingerprint(url.spec().data(),
                                                   url.spec().size(),
                                                   salt_);
-  if (table_builder_.get()) {
+  if (table_builder_.get() || table_is_loading_from_file_) {
     // If we have a pending delete for this fingerprint, cancel it.
     std::set<Fingerprint>::iterator found =
-        deleted_since_rebuild_.find(fingerprint);
-    if (found != deleted_since_rebuild_.end())
-        deleted_since_rebuild_.erase(found);
+        deleted_since_rebuild_or_load_.find(fingerprint);
+    if (found != deleted_since_rebuild_or_load_.end())
+        deleted_since_rebuild_or_load_.erase(found);
 
-    // A rebuild is in progress, save this addition in the temporary list so
-    // it can be added once rebuild is complete.
-    added_since_rebuild_.insert(fingerprint);
+    // A rebuild or load is in progress, save this addition in the temporary
+    // list so it can be added once rebuild is complete.
+    added_since_rebuild_or_load_.insert(fingerprint);
   }
 
   // If the table is "full", we don't add URLs and just drop them on the floor.
@@ -289,7 +337,9 @@ void VisitedLinkMaster::PostIOTask(const tracked_objects::Location& from_here,
 
 void VisitedLinkMaster::AddURL(const GURL& url) {
   Hash index = TryToAddURL(url);
-  if (!table_builder_.get() && index != null_hash_) {
+  if (!table_builder_.get() &&
+      !table_is_loading_from_file_ &&
+      index != null_hash_) {
     // Not rebuilding, so we want to keep the file on disk up-to-date.
     if (persist_to_disk_) {
       WriteUsedItemCountToFile();
@@ -303,19 +353,24 @@ void VisitedLinkMaster::AddURLs(const std::vector<GURL>& url) {
   for (std::vector<GURL>::const_iterator i = url.begin();
        i != url.end(); ++i) {
     Hash index = TryToAddURL(*i);
-    if (!table_builder_.get() && index != null_hash_)
+    if (!table_builder_.get() &&
+        !table_is_loading_from_file_ &&
+        index != null_hash_)
       ResizeTableIfNecessary();
   }
 
   // Keeps the file on disk up-to-date.
-  if (!table_builder_.get() && persist_to_disk_)
+  if (!table_builder_.get() &&
+      !table_is_loading_from_file_ &&
+      persist_to_disk_)
     WriteFullTable();
 }
 
 void VisitedLinkMaster::DeleteAllURLs() {
   // Any pending modifications are invalid.
-  added_since_rebuild_.clear();
-  deleted_since_rebuild_.clear();
+  added_since_rebuild_or_load_.clear();
+  deleted_since_rebuild_or_load_.clear();
+  table_is_loading_from_file_ = false;
 
   // Clear the hash table.
   used_items_ = 0;
@@ -339,7 +394,7 @@ void VisitedLinkMaster::DeleteURLs(URLIterator* urls) {
 
   listener_->Reset();
 
-  if (table_builder_.get()) {
+  if (table_builder_.get() || table_is_loading_from_file_) {
     // A rebuild is in progress, save this deletion in the temporary list so
     // it can be added once rebuild is complete.
     while (urls->HasNextURL()) {
@@ -349,14 +404,15 @@ void VisitedLinkMaster::DeleteURLs(URLIterator* urls) {
 
       Fingerprint fingerprint =
           ComputeURLFingerprint(url.spec().data(), url.spec().size(), salt_);
-      deleted_since_rebuild_.insert(fingerprint);
+      deleted_since_rebuild_or_load_.insert(fingerprint);
 
       // If the URL was just added and now we're deleting it, it may be in the
-      // list of things added since the last rebuild. Delete it from that list.
+      // list of things added since the last rebuild or load. Delete it from
+      // that list.
       std::set<Fingerprint>::iterator found =
-          added_since_rebuild_.find(fingerprint);
-      if (found != added_since_rebuild_.end())
-        added_since_rebuild_.erase(found);
+          added_since_rebuild_or_load_.find(fingerprint);
+      if (found != added_since_rebuild_or_load_.end())
+        added_since_rebuild_or_load_.erase(found);
 
       // Delete the URLs from the in-memory table, but don't bother writing
       // to disk since it will be replaced soon.
@@ -534,47 +590,165 @@ void VisitedLinkMaster::WriteFullTable() {
 }
 
 bool VisitedLinkMaster::InitFromFile() {
-  DCHECK(file_ == NULL);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  DCHECK(file_ == nullptr);
   DCHECK(persist_to_disk_);
 
   base::FilePath filename;
-  GetDatabaseFileName(&filename);
+  if (!GetDatabaseFileName(&filename))
+    return false;
+
+  // Create the temporary table.
+  // The salt must be generated before the table so that it can be copied to
+  // the shared memory.
+  GenerateSalt(salt_);
+  if (!CreateURLTable(DefaultTableSize()))
+    return false;
+
+#ifndef NDEBUG
+  DebugValidate();
+#endif
+
+  table_is_loading_from_file_ = true;
+
+  TableLoadCompleteCallback callback = base::Bind(
+      &VisitedLinkMaster::OnTableLoadComplete, weak_ptr_factory_.GetWeakPtr());
+
+  PostIOTask(FROM_HERE,
+             base::Bind(&VisitedLinkMaster::LoadFromFile, filename, callback));
+
+  return true;
+}
+
+// static
+void VisitedLinkMaster::LoadFromFile(
+    const base::FilePath& filename,
+    const TableLoadCompleteCallback& callback) {
+  scoped_refptr<LoadFromFileResult> load_from_file_result;
+  bool success = LoadApartFromFile(filename, &load_from_file_result);
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      base::Bind(callback, success, load_from_file_result));
+}
+
+// static
+bool VisitedLinkMaster::LoadApartFromFile(
+    const base::FilePath& filename,
+    scoped_refptr<LoadFromFileResult>* load_from_file_result) {
+  DCHECK(load_from_file_result);
+
   base::ScopedFILE file_closer(base::OpenFile(filename, "rb+"));
   if (!file_closer.get())
     return false;
 
   int32 num_entries, used_count;
-  if (!ReadFileHeader(file_closer.get(), &num_entries, &used_count, salt_))
+  uint8 salt[LINK_SALT_LENGTH];
+  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))
+  scoped_ptr<base::SharedMemory> shared_memory;
+  VisitedLinkCommon::Fingerprint* hash_table;
+  if (!CreateApartURLTable(num_entries, salt, &shared_memory, &hash_table))
     return false;
-  if (!ReadFromFile(file_closer.get(), kFileHeaderSize,
-                    hash_table_, num_entries * sizeof(Fingerprint))) {
-    FreeURLTable();
+
+  if (!ReadFromFile(file_closer.get(), kFileHeaderSize, hash_table,
+                    num_entries * sizeof(Fingerprint))) {
     return false;
   }
-  used_items_ = used_count;
+
+  *load_from_file_result = new LoadFromFileResult(file_closer.Pass(),
+                                                  shared_memory.Pass(),
+                                                  hash_table,
+                                                  num_entries,
+                                                  used_count,
+                                                  salt);
+  return true;
+}
+
+void VisitedLinkMaster::OnTableLoadComplete(
+    bool success,
+    scoped_refptr<LoadFromFileResult> load_from_file_result) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(persist_to_disk_);
+  DCHECK(!table_builder_.get());
+
+  // When the apart table was loading from the database file the current table
+  // was cleared.

[brettw, 2015/11/02 05:09:23]

"was" -> "could have been" (As-is, I think the comment describes the "return"
case rather than the entire block fo code here, which I'm trying to make clear.)

+  if (!table_is_loading_from_file_)
+    return;
+
+  table_is_loading_from_file_ = false;
+
+  if (!success) {
+    // If the table isn't loaded the table will be rebuilt.
+    if (!suppress_rebuild_) {
+      RebuildTableFromDelegate();
+    } else {
+      // When we disallow rebuilds (normally just unit tests), just use the
+      // current empty table.
+      WriteFullTable();
+    }
+    return;
+  }
+
+  DCHECK(load_from_file_result.get());
+
+  // Delete the previous table.
+  DCHECK(shared_memory_);
+  delete shared_memory_;
+  shared_memory_ = nullptr;
+
+  // Assign the open file.
+  DCHECK(!file_);
+  DCHECK(load_from_file_result->file.get());
+  file_ = static_cast<FILE**>(malloc(sizeof(*file_)));
+  *file_ = load_from_file_result->file.release();
+
+  // Assign the loaded table.
+  DCHECK(load_from_file_result->shared_memory.get());
+  DCHECK(load_from_file_result->hash_table);
+  memcpy(salt_, load_from_file_result->salt, LINK_SALT_LENGTH);
+  shared_memory_ = load_from_file_result->shared_memory.release();
+  hash_table_ = load_from_file_result->hash_table;
+  table_length_ = load_from_file_result->num_entries;
+  used_items_ = load_from_file_result->used_count;
 
 #ifndef NDEBUG
   DebugValidate();
 #endif
 
-  file_ = static_cast<FILE**>(malloc(sizeof(*file_)));
-  *file_ = file_closer.release();
-  return true;
+  if (!added_since_rebuild_or_load_.empty() ||
+      !deleted_since_rebuild_or_load_.empty()) {
+    int new_size =
+        NewTableSizeForCount(used_items_ + added_since_rebuild_or_load_.size());
+    if (new_size > table_length_)
+      ResizeTable(new_size);
+
+    // Also add anything that was added while we were asynchronously
+    // loading the table.
+    for (const auto& fingerprint : added_since_rebuild_or_load_)

[brettw, 2015/11/02 05:09:23]

From here to the end of the condition is the same as OnTableRebuildComplete. I
think this is probably complex enough to warrant a helper function to avoid the
duplication.

+      AddFingerprint(fingerprint, false);
+    added_since_rebuild_or_load_.clear();
+
+    // Now handle deletions.
+    DeleteFingerprintsFromCurrentTable(deleted_since_rebuild_or_load_);
+    deleted_since_rebuild_or_load_.clear();
+
+    if (persist_to_disk_)
+      WriteFullTable();
+  }
+
+  // Send an update notification to all child processes.

[brettw, 2015/11/02 05:09:23]

This will duplicate the message if the table is resized above. I'm not actually
sure if this is benign or not. If it doesn't hurt, this is a rare edge case and
I don't think it's worth optimizing. But I'm concerned about handle duplicating
behavior on all the platforms when sharing the same memory to a process multiple
times. For example, the renderer will close the old one and add the new one, but
I'm not sure the two handles to the same memory will always be different on all
platforms.

I think this will all go away if we move this NewTable call above the "if
(!added_since..." condition above, and then also delete the checking for whether
the new size is required at the top of that condition. The additions will
organically cause a table resize if necessary and we can have fewer special
cases here. I think the only case where manually checking for the resize might
help is if the additions cause two resizes, which should be so rare as to be
almost impossible.

+  listener_->NewTable(shared_memory_);

[brettw, 2015/11/02 05:09:23]

I *think* here we also need a Reset() after the NewTable. You may have this in
your head more freshly than me, so please correct if I'm mistaken...

Now that we're loading the links asynchronously, the new table means the visited
link state of all renderers is out-of-date because we loaded all of the hashes
from disk without sending notifications. I think this only happens in Reset().
The NewTable() assumes the contents are the same but the table was expanded, so
isn't sufficient, but obviously the NewTable call has to happen first for the
new link state to be updated. Otherwise the link state on the initial tabs will
be incorrect.

If we move NewTable up above, the Reset() needs to stay at the bottom.

 }
 
 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))
+  if (!CreateURLTable(DefaultTableSize()))
     return false;
 
 #ifndef NDEBUG
@@ -596,12 +770,11 @@ bool VisitedLinkMaster::InitFromScratch(bool suppress_rebuild) {
   return RebuildTableFromDelegate();
 }
 
+// static
 bool VisitedLinkMaster::ReadFileHeader(FILE* file,
                                        int32* num_entries,
                                        int32* used_count,
                                        uint8 salt[LINK_SALT_LENGTH]) {
-  DCHECK(persist_to_disk_);
-
   // Get file size.
   // Note that there is no need to seek back to the original location in the
   // file since ReadFromFile() [which is the next call accessing the file]
@@ -665,39 +838,57 @@ bool VisitedLinkMaster::GetDatabaseFileName(base::FilePath* filename) {
 
 // Initializes the shared memory structure. The salt should already be filled
 // in so that it can be written to the shared memory
-bool VisitedLinkMaster::CreateURLTable(int32 num_entries, bool init_to_empty) {
+bool VisitedLinkMaster::CreateURLTable(int32 num_entries) {
+  scoped_ptr<base::SharedMemory> shared_memory;
+  VisitedLinkCommon::Fingerprint* hash_table;
+  if (CreateApartURLTable(num_entries, salt_, &shared_memory, &hash_table)) {
+    shared_memory_ = shared_memory.release();
+    hash_table_ = hash_table;
+    table_length_ = num_entries;
+    used_items_ = 0;
+    return true;
+  }
+
+  return false;
+}
+
+// static
+bool VisitedLinkMaster::CreateApartURLTable(
+    int32 num_entries,
+    const uint8 salt[LINK_SALT_LENGTH],
+    scoped_ptr<base::SharedMemory>* shared_memory,
+    VisitedLinkCommon::Fingerprint** hash_table) {
+  DCHECK(salt);
+  DCHECK(shared_memory);
+  DCHECK(hash_table);
+
   // The table is the size of the table followed by the entries.
   uint32 alloc_size = num_entries * sizeof(Fingerprint) + sizeof(SharedHeader);
 
   // Create the shared memory object.
-  shared_memory_ = new base::SharedMemory();
-  if (!shared_memory_)
+  scoped_ptr<base::SharedMemory> sh_mem(new base::SharedMemory());
+  if (!sh_mem)
     return false;
 
   base::SharedMemoryCreateOptions options;
   options.size = alloc_size;
   options.share_read_only = true;
 
-  if (!shared_memory_->Create(options) || !shared_memory_->Map(alloc_size)) {
-    delete shared_memory_;
-    shared_memory_ = NULL;
+  if (!sh_mem->Create(options) || !sh_mem->Map(alloc_size))
     return false;
-  }
 
-  if (init_to_empty) {
-    memset(shared_memory_->memory(), 0, alloc_size);
-    used_items_ = 0;
-  }
-  table_length_ = num_entries;
+  memset(sh_mem->memory(), 0, alloc_size);
 
   // Save the header for other processes to read.
-  SharedHeader* header = static_cast<SharedHeader*>(shared_memory_->memory());
-  header->length = table_length_;
-  memcpy(header->salt, salt_, LINK_SALT_LENGTH);
+  SharedHeader* header = static_cast<SharedHeader*>(sh_mem->memory());
+  header->length = num_entries;
+  memcpy(header->salt, salt, LINK_SALT_LENGTH);
 
   // Our table pointer is just the data immediately following the size.
-  hash_table_ = reinterpret_cast<Fingerprint*>(
-      static_cast<char*>(shared_memory_->memory()) + sizeof(SharedHeader));
+  *hash_table = reinterpret_cast<Fingerprint*>(
+      static_cast<char*>(sh_mem->memory()) + sizeof(SharedHeader));
+
+  *shared_memory = sh_mem.Pass();
 
   return true;
 }
@@ -706,7 +897,7 @@ bool VisitedLinkMaster::BeginReplaceURLTable(int32 num_entries) {
   base::SharedMemory *old_shared_memory = shared_memory_;
   Fingerprint* old_hash_table = hash_table_;
   int32 old_table_length = table_length_;
-  if (!CreateURLTable(num_entries, true)) {
+  if (!CreateURLTable(num_entries)) {
     // Try to put back the old state.
     shared_memory_ = old_shared_memory;
     hash_table_ = old_hash_table;
@@ -796,6 +987,13 @@ void VisitedLinkMaster::ResizeTable(int32 new_size) {
     WriteFullTable();
 }
 
+uint32 VisitedLinkMaster::DefaultTableSize() const {
+  if (table_size_override_)
+    return table_size_override_;
+
+  return kDefaultTableSize;
+}
+
 uint32 VisitedLinkMaster::NewTableSizeForCount(int32 item_count) const {
   // These table sizes are selected to be the maximum prime number less than
   // a "convenient" multiple of 1K.
@@ -850,8 +1048,9 @@ void VisitedLinkMaster::OnTableRebuildComplete(
     // replacement succeeds.
     base::SharedMemory* old_shared_memory = shared_memory_;
 
-    int new_table_size = NewTableSizeForCount(
-        static_cast<int>(fingerprints.size() + added_since_rebuild_.size()));
+    int new_table_size =
+        NewTableSizeForCount(static_cast<int>(fingerprints.size() +
+                             added_since_rebuild_or_load_.size()));
     if (BeginReplaceURLTable(new_table_size)) {
       // Free the old table.
       delete old_shared_memory;
@@ -862,14 +1061,15 @@ void VisitedLinkMaster::OnTableRebuildComplete(
 
       // Also add anything that was added while we were asynchronously
       // generating the new table.
-      for (std::set<Fingerprint>::iterator i = added_since_rebuild_.begin();
-           i != added_since_rebuild_.end(); ++i)
+      for (std::set<Fingerprint>::iterator i =
+               added_since_rebuild_or_load_.begin();
+           i != added_since_rebuild_or_load_.end(); ++i)
         AddFingerprint(*i, false);
-      added_since_rebuild_.clear();
+      added_since_rebuild_or_load_.clear();
 
       // Now handle deletions.
-      DeleteFingerprintsFromCurrentTable(deleted_since_rebuild_);
-      deleted_since_rebuild_.clear();
+      DeleteFingerprintsFromCurrentTable(deleted_since_rebuild_or_load_);
+      deleted_since_rebuild_or_load_.clear();
 
       // Send an update notification to all child processes.
       listener_->NewTable(shared_memory_);
@@ -892,9 +1092,7 @@ void VisitedLinkMaster::WriteToFile(FILE** file,
                                     void* data,
                                     int32 data_size) {
   DCHECK(persist_to_disk_);
-#ifndef NDEBUG
-  posted_asynchronous_operation_ = true;
-#endif
+  DCHECK(!table_is_loading_from_file_);
   PostIOTask(FROM_HERE,
       base::Bind(&AsyncWrite, file, offset,
                  std::string(static_cast<const char*>(data), data_size)));
@@ -929,17 +1127,11 @@ void VisitedLinkMaster::WriteHashRangeToFile(Hash first_hash, Hash last_hash) {
   }
 }
 
+// static
 bool VisitedLinkMaster::ReadFromFile(FILE* file,
                                      off_t offset,
                                      void* data,
                                      size_t data_size) {
-  DCHECK(persist_to_disk_);
-#ifndef NDEBUG
-  // Since this function is synchronous, we require that no asynchronous
-  // operations could possibly be pending.
-  DCHECK(!posted_asynchronous_operation_);
-#endif
-
   if (fseek(file, offset, SEEK_SET) != 0)
     return false;