Do not open file on UI thread in VisitedLinkMaster

chrome/browser/visitedlink/visitedlink_master.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/visitedlink/visitedlink_master.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <io.h>
 #include <shlobj.h>
@@ skipping 46 matching lines @@
 
 // Fills the given salt structure with some quasi-random values
 // It is not necessary to generate a cryptographically strong random string,
 // only that it be reasonably different for different users.
 void GenerateSalt(uint8 salt[LINK_SALT_LENGTH]) {
   DCHECK_EQ(LINK_SALT_LENGTH, 8) << "This code assumes the length of the salt";
   uint64 randval = base::RandUint64();
   memcpy(salt, &randval, 8);
 }
 
+// Opens file on a background thread to not block UI thread.
+void AsyncOpen(FILE** file, FilePath filename) {
+  *file = OpenFile(filename, "wb+");
+  if (!(*file)) {
+    DLOG(ERROR) << "Failed to open file " << filename.value();
+  }
+}
+
 // Returns true if the write was complete.
 static bool WriteToFile(FILE* file,
                         off_t offset,
                         const void* data,
                         size_t data_len) {
   if (fseek(file, offset, SEEK_SET) != 0)
     return false;  // Don't write to an invalid part of the file.
 
   size_t num_written = fwrite(data, 1, data_len, file);
 
   // The write may not make it to the kernel (stdlib may buffer the write)
   // until the next fseek/fclose call.  If we crash, it's easy for our used
   // item count to be out of sync with the number of hashes we write.
   // Protect against this by calling fflush.
   int ret = fflush(file);
   DCHECK_EQ(0, ret);
   return num_written == data_len;
 }
 
 // This task executes on a background thread and executes a write. This
-// prevents us from blocking the UI thread doing I/O.
-void AsyncWrite(FILE* file, int32 offset, const std::string& data) {
-  WriteToFile(file, offset, data.data(), data.size());
+// prevents us from blocking the UI thread doing I/O. Double pointer to FILE
+// is used because file may still not be opened by the time of scheduling
+// the task for execution.
+void AsyncWrite(FILE** file, int32 offset, const std::string& data) {
+  WriteToFile(*file, offset, data.data(), data.size());
+}
+
+// Truncates the file to the current position asynchronously on a background
+// thread. Double pointer to FILE is used because file may still not be opened
+// by the time of scheduling the task for execution.
+void AsyncTruncate(FILE** file) {
+  base::IgnoreResult(TruncateFile(*file));
+}
+
+// Closes the file on a background thread and releases memory used for storage
+// of FILE* value. Double pointer to FILE is used because file may still not
+// be opened by the time of scheduling the task for execution.
+void AsyncClose(FILE** file) {
+  base::IgnoreResult(fclose(*file));
+  delete file;
 }
 
 }  // namespace
 
 // TableBuilder ---------------------------------------------------------------
 
 // How rebuilding from history works
 // ---------------------------------
 //
 // We mark that we're rebuilding from history by setting the table_builder_
@@ skipping 70 matching lines @@
 }
 
 VisitedLinkMaster::~VisitedLinkMaster() {
   if (table_builder_.get()) {
     // Prevent the table builder from calling us back now that we're being
     // destroyed. Note that we DON'T delete the object, since the history
     // system is still writing into it. When that is complete, the table
     // builder will destroy itself when it finds we are gone.
     table_builder_->DisownMaster();
   }
   FreeURLTable();

[brettw, 2012/07/27 22:11:14]

Can you add a comment here that the file_ pointer will be freed as a side effect
of freeing the URL table?

 }
 
 void VisitedLinkMaster::InitMembers(Listener* listener, Profile* profile) {
   DCHECK(listener);
 
   listener_ = listener;
   file_ = NULL;
   shared_memory_ = NULL;
   shared_memory_serial_ = 0;
   used_items_ = 0;
@@ skipping 257 matching lines @@
       AddFingerprint(shuffled_fingerprints[i], false);
   }
 
   // Write the affected range to disk [deleted_hash, end_range].
   if (update_file)
     WriteHashRangeToFile(deleted_hash, end_range);
 
   return true;
 }
 
-bool VisitedLinkMaster::WriteFullTable() {
+void VisitedLinkMaster::WriteFullTable() {
   // This function can get called when the file is open, for example, when we
   // resize the table. We must handle this case and not try to reopen the file,
   // since there may be write operations pending on the file I/O thread.
   //
   // Note that once we start writing, we do not delete on error. This means
   // there can be a partial file, but the short file will be detected next time
   // we start, and will be replaced.
   //
   // This might possibly get corrupted if we crash in the middle of writing.
   // We should pick up the most common types of these failures when we notice
   // that the file size is different when we load it back in, and then we will
   // regenerate the table.
   if (!file_) {
+    file_ = new FilePtr();
     FilePath filename;
     GetDatabaseFileName(&filename);
-    file_ = OpenFile(filename, "wb+");
-    if (!file_) {
-      DLOG(ERROR) << "Failed to open file " << filename.value();
-      return false;
-    }
+    PostIOTask(FROM_HERE, base::Bind(&AsyncOpen, file_, filename));
   }
 
   // Write the new header.
   int32 header[4];
   header[0] = kFileSignature;
   header[1] = kFileCurrentVersion;
   header[2] = table_length_;
   header[3] = used_items_;
   WriteToFile(file_, 0, header, sizeof(header));
   WriteToFile(file_, sizeof(header), salt_, LINK_SALT_LENGTH);
 
   // Write the hash data.
   WriteToFile(file_, kFileHeaderSize,
               hash_table_, table_length_ * sizeof(Fingerprint));
 
   // The hash table may have shrunk, so make sure this is the end.
-  PostIOTask(FROM_HERE, base::Bind(base::IgnoreResult(&TruncateFile), file_));
-  return true;
+  PostIOTask(FROM_HERE, base::Bind(&AsyncTruncate, file_));
 }
 
 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();
+  file_ = new FilePtr(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();
+    WriteFullTable();
+    return true;
   }
 
   // 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();
 }
 
@@ skipping 116 matching lines @@
   return true;
 }
 
 void VisitedLinkMaster::FreeURLTable() {
   if (shared_memory_) {
     delete shared_memory_;
     shared_memory_ = NULL;
   }
   if (!file_)
     return;
-  PostIOTask(FROM_HERE, base::Bind(base::IgnoreResult(&fclose), file_));
+  PostIOTask(FROM_HERE, base::Bind(&AsyncClose, file_));

[brettw, 2012/07/27 22:11:14]

After this call can you null out the file_ pointer and comment that deleting of
it will be taken care of by AsyncClose?

 }
 
 bool VisitedLinkMaster::ResizeTableIfNecessary() {
   DCHECK(table_length_ > 0) << "Must have a table";
 
   // Load limits for good performance/space. We are pretty conservative about
   // keeping the table not very full. This is because we use linear probing
   // which increases the likelihood of clumps of entries which will reduce
   // performance.
   const float max_table_load = 0.5f;  // Grow when we're > this full.
@@ skipping 134 matching lines @@
       for (size_t i = 0; i < fingerprints.size(); i++)
         AddFingerprint(fingerprints[i], false);
 
       // 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)
         AddFingerprint(*i, false);
       added_since_rebuild_.clear();
 
-      // We shouldn't be writing the table from the main thread!
-      //   http://code.google.com/p/chromium/issues/detail?id=24163
-      base::ThreadRestrictions::ScopedAllowIO allow_io;
-
       // Now handle deletions.
       DeleteFingerprintsFromCurrentTable(deleted_since_rebuild_);
       deleted_since_rebuild_.clear();
 
       // Send an update notification to all child processes.
       listener_->NewTable(shared_memory_);
 
       WriteFullTable();
     }
   }
   table_builder_ = NULL;  // Will release our reference to the builder.
 
   // Notify the unit test that the rebuild is complete (will be NULL in prod.)
   if (!rebuild_complete_task_.is_null()) {
     rebuild_complete_task_.Run();
     rebuild_complete_task_.Reset();
   }
 }
 
-void VisitedLinkMaster::WriteToFile(FILE* file,
+void VisitedLinkMaster::WriteToFile(FilePtr* file,
                                     off_t offset,
                                     void* data,
                                     int32 data_size) {
 #ifndef NDEBUG
   posted_asynchronous_operation_ = true;
 #endif
   PostIOTask(FROM_HERE,
       base::Bind(&AsyncWrite, file, offset,
                  std::string(static_cast<const char*>(data), data_size)));
 }
@@ skipping 77 matching lines @@
 }
 
 void VisitedLinkMaster::TableBuilder::OnCompleteMainThread() {
   if (master_)
     master_->OnTableRebuildComplete(success_, fingerprints_);
 
   // WILL (generally) DELETE THIS! This balances the AddRef in
   // VisitedLinkMaster::RebuildTableFromHistory.
   Release();
 }