Do not open file on UI thread in VisitedLinkMaster

chrome/browser/visitedlink/visitedlink_master.cc

Index: chrome/browser/visitedlink/visitedlink_master.cc
===================================================================
--- chrome/browser/visitedlink/visitedlink_master.cc	(revision 148849)
+++ chrome/browser/visitedlink/visitedlink_master.cc	(working copy)
@@ -64,6 +64,14 @@
   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)) {

[brettw, 2012/07/31 21:22:46]

Can you just write DLOG_IF(ERROR, !(*file)) << "...";

+    DLOG(ERROR) << "Failed to open file " << filename.value();
+  }
+}
+
 // Returns true if the write was complete.
 static bool WriteToFile(FILE* file,
                         off_t offset,
@@ -84,11 +92,28 @@
 }
 
 // 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));
+  free(file);
+}
+
 }  // namespace
 
 // TableBuilder ---------------------------------------------------------------
@@ -178,6 +203,8 @@
     table_builder_->DisownMaster();
   }
   FreeURLTable();
+  // FreeURLTable() will schedule closing of the file and deletion of |file_|.
+  // So nothing should be done here.
 }
 
 void VisitedLinkMaster::InitMembers(Listener* listener, Profile* profile) {
@@ -455,7 +482,7 @@
   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.
@@ -469,13 +496,10 @@
   // that the file size is different when we load it back in, and then we will
   // regenerate the table.
   if (!file_) {
+    file_ = static_cast<FILE**>(calloc(1, sizeof(*file_)));
     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.
@@ -492,8 +516,7 @@
               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() {
@@ -523,7 +546,8 @@
   DebugValidate();
 #endif
 
-  file_ = file_closer.release();
+  file_ = static_cast<FILE**>(malloc(sizeof(*file_)));
+  *file_ = file_closer.release();
   return true;
 }
 
@@ -545,7 +569,8 @@
   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
@@ -682,7 +707,9 @@
   }
   if (!file_)
     return;
-  PostIOTask(FROM_HERE, base::Bind(base::IgnoreResult(&fclose), file_));
+  PostIOTask(FROM_HERE, base::Bind(&AsyncClose, file_));
+  // AsyncClose() will close the file and free the memory pointed by |file_|.
+  file_ = NULL;
 }
 
 bool VisitedLinkMaster::ResizeTableIfNecessary() {
@@ -837,10 +864,6 @@
         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();
@@ -860,7 +883,7 @@
   }
 }
 
-void VisitedLinkMaster::WriteToFile(FILE* file,
+void VisitedLinkMaster::WriteToFile(FILE** file,
                                     off_t offset,
                                     void* data,
                                     int32 data_size) {