Improve reliability of custom spelling dictionary file.

chrome/browser/spellchecker/spellcheck_custom_dictionary.cc

Index: chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
diff --git a/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc b/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
index 4510332e59e3032ee7b35bf26f640ff6f3f2885c..aa777e0a10a4384e1e3758a830a7b32da15be4cc 100644
--- a/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
+++ b/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
@@ -7,6 +7,8 @@
 #include <functional>
 
 #include "base/file_util.h"
+#include "base/files/important_file_writer.h"
+#include "base/md5.h"
 #include "base/string_split.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
@@ -20,7 +22,9 @@ using chrome::spellcheck_common::WordList;
 SpellcheckCustomDictionary::SpellcheckCustomDictionary(Profile* profile)
     : SpellcheckDictionary(profile),
       custom_dictionary_path_(),
-      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      backup_extension_(FILE_PATH_LITERAL("backup")),
+      checksum_prefix_("checksum_v1 = ") {
   DCHECK(profile);
   custom_dictionary_path_ =
       profile_->GetPath().Append(chrome::kCustomDictionaryFileName);
@@ -49,35 +53,19 @@ void SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
     WordList* custom_words) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string contents;
-  file_util::ReadFileToString(custom_dictionary_path_, &contents);
-  if (contents.empty()) {
-    custom_words->clear();
+  LoadDictionaryFileReliably(custom_words);
+  if (custom_words->empty())
     return;
-  }
-
-  base::SplitString(contents, '\n', custom_words);
 
-  // Erase duplicates.
+  // Clean up the dictionary file contents by removing duplicates and empty
+  // words.
   std::sort(custom_words->begin(), custom_words->end());
   custom_words->erase(std::unique(custom_words->begin(), custom_words->end()),
                       custom_words->end());
+  if (custom_words->at(0).length() == 0)

[groby-ooo-7-16, 2012/12/05 00:56:19]

at(0).empty()

[please use gerrit instead, 2012/12/05 16:51:26]

Done.

+    custom_words->erase(custom_words->begin());
 
-  // Clear out empty words.
-  custom_words->erase(remove_if(custom_words->begin(), custom_words->end(),
-    mem_fun_ref(&std::string::empty)), custom_words->end());
-
-  // Write out the clean file.
-  std::stringstream ss;
-  for (WordList::iterator it = custom_words->begin();
-       it != custom_words->end();
-       ++it) {
-    ss << *it << '\n';
-  }
-  contents = ss.str();
-  file_util::WriteFile(custom_dictionary_path_,
-                       contents.c_str(),
-                       contents.length());
+  SaveDictionaryFileReliably(*custom_words);
 }
 
 void SpellcheckCustomDictionary::SetCustomWordList(WordList* custom_words) {
@@ -87,9 +75,7 @@ void SpellcheckCustomDictionary::SetCustomWordList(WordList* custom_words) {
   if (custom_words)
     std::swap(words_, *custom_words);
 
-  std::vector<Observer*>::iterator it;
-  for (it = observers_.begin(); it != observers_.end(); ++it)
-    (*it)->OnCustomDictionaryLoaded();
+  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryLoaded());
 }
 
 bool SpellcheckCustomDictionary::AddWord(const std::string& word) {
@@ -108,9 +94,7 @@ bool SpellcheckCustomDictionary::AddWord(const std::string& word) {
     i.GetCurrentValue()->Send(new SpellCheckMsg_WordAdded(word));
   }
 
-  std::vector<Observer*>::iterator it;
-  for (it = observers_.begin(); it != observers_.end(); ++it)
-    (*it)->OnCustomDictionaryWordAdded(word);
+  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryWordAdded(word));
 
   return true;
 }
@@ -131,18 +115,12 @@ bool SpellcheckCustomDictionary::CustomWordAddedLocally(
 void SpellcheckCustomDictionary::WriteWordToCustomDictionary(
     const std::string& word) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  // Stored in UTF-8.
   DCHECK(IsStringUTF8(word));
 
-  std::string word_to_add(word + "\n");
-  if (!file_util::PathExists(custom_dictionary_path_)) {
-    file_util::WriteFile(custom_dictionary_path_, word_to_add.c_str(),
-        word_to_add.length());
-  } else {
-    file_util::AppendToFile(custom_dictionary_path_, word_to_add.c_str(),
-        word_to_add.length());
-  }
+  WordList custom_words;
+  LoadDictionaryFileReliably(&custom_words);
+  custom_words.push_back(word);
+  SaveDictionaryFileReliably(custom_words);
 }
 
 bool SpellcheckCustomDictionary::RemoveWord(const std::string& word) {
@@ -161,9 +139,7 @@ bool SpellcheckCustomDictionary::RemoveWord(const std::string& word) {
     i.GetCurrentValue()->Send(new SpellCheckMsg_WordRemoved(word));
   }
 
-  std::vector<Observer*>::iterator it;
-  for (it = observers_.begin(); it != observers_.end(); ++it)
-    (*it)->OnCustomDictionaryWordRemoved(word);
+  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryWordRemoved(word));
 
   return true;
 }
@@ -186,37 +162,29 @@ void SpellcheckCustomDictionary::EraseWordFromCustomDictionary(
   DCHECK(IsStringUTF8(word));
 
   WordList custom_words;
-  LoadDictionaryIntoCustomWordList(&custom_words);
-
-  const char empty[] = {'\0'};
-  const char separator[] = {'\n', '\0'};
-  file_util::WriteFile(custom_dictionary_path_, empty, 0);
-  for (WordList::iterator it = custom_words.begin();
-      it != custom_words.end();
-      ++it) {
-    std::string word_to_add = *it;
-    if (word.compare(word_to_add) != 0) {
-      file_util::AppendToFile(custom_dictionary_path_, word_to_add.c_str(),
-          word_to_add.length());
-      file_util::AppendToFile(custom_dictionary_path_, separator, 1);
-    }
-  }
+  LoadDictionaryFileReliably(&custom_words);
+  if (custom_words.empty())
+    return;
+
+  WordList::iterator it = std::find(custom_words.begin(),
+                                    custom_words.end(),
+                                    word);
+  if (it != custom_words.end())
+    custom_words.erase(it);
+
+  SaveDictionaryFileReliably(custom_words);
 }
 
 void SpellcheckCustomDictionary::AddObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  observers_.push_back(observer);
+  observers_.AddObserver(observer);
 }
 
 void SpellcheckCustomDictionary::RemoveObserver(Observer* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  std::vector<Observer*>::iterator it = std::find(observers_.begin(),
-                                                  observers_.end(),
-                                                  observer);
-  if (it != observers_.end())
-    observers_.erase(it);
+  observers_.RemoveObserver(observer);
 }
 
 WordList* SpellcheckCustomDictionary::LoadDictionary() {
@@ -234,3 +202,78 @@ void SpellcheckCustomDictionary::SetCustomWordListAndDelete(
   SetCustomWordList(custom_words);
   delete custom_words;
 }
+
+void SpellcheckCustomDictionary::LoadDictionaryFileReliably(
+    WordList* custom_words) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  // Load the contents and verify the checksum.
+  std::string contents;
+  file_util::ReadFileToString(custom_dictionary_path_, &contents);
+  std::string checksum = GetChecksum(&contents);
+  base::SplitString(contents, '\n', custom_words);
+  if (!custom_words->empty() && (*(custom_words->end() - 1)).empty())

[groby-ooo-7-16, 2012/12/05 00:56:19]

Why? Just to remove trailing newlines? In that case, won't our sort/uniq process
clean that out anyways?

[please use gerrit instead, 2012/12/05 16:51:26]

Removed.

+    custom_words->erase(custom_words->end() - 1);
+  if (checksum.empty() || checksum == base::MD5String(contents))
+    return;
+
+  // Checksum is not valid. See if there's a backup.
+  FilePath backup_path =
+      custom_dictionary_path_.AddExtension(backup_extension_);
+  if (!file_util::PathExists(backup_path))
+    return;
+
+  // Load the backup and verify its checksum.
+  std::string backup;

[groby-ooo-7-16, 2012/12/05 00:56:19]

Extract that into a function - 210-217 and 228-236 are almost identical.

[please use gerrit instead, 2012/12/05 16:51:26]

Done.

+  file_util::ReadFileToString(backup_path, &backup);
+  std::string backup_checksum = GetChecksum(&backup);
+  if (backup_checksum != base::MD5String(backup))
+    return;
+
+  // Backup checksum is valid. Use that instead.
+  base::SplitString(backup, '\n', custom_words);
+  if (!custom_words->empty() && (*(custom_words->end() - 1)).empty())
+    custom_words->erase(custom_words->end() - 1);
+  file_util::CopyFile(backup_path, custom_dictionary_path_);
+}
+
+void SpellcheckCustomDictionary::SaveDictionaryFileReliably(
+    const WordList& custom_words) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  std::stringstream content;
+  for (WordList::const_iterator it = custom_words.begin();
+       it != custom_words.end();
+       ++it) {
+    content << *it << '\n';
+  }
+  std::string content_str = content.str();
+  std::stringstream file;
+  file << checksum_prefix_ << base::MD5String(content_str) << '\n'
+       << content_str;
+
+  FilePath backup_path =
+      custom_dictionary_path_.AddExtension(backup_extension_);
+  if (file_util::PathExists(custom_dictionary_path_))

[groby-ooo-7-16, 2012/12/05 00:56:19]

Do you need to check? Can't we just call CopyFile, no matter if it exists or
not?

[please use gerrit instead, 2012/12/05 16:51:26]

Done.

+    file_util::CopyFile(custom_dictionary_path_, backup_path);
+  base::ImportantFileWriter::WriteFileAtomically(custom_dictionary_path_,
+                                                 file.str());
+}
+
+std::string SpellcheckCustomDictionary::GetChecksum(std::string* contents) {
+  std::string checksum;

[groby-ooo-7-16, 2012/12/05 00:56:19]

nit: You can save a bit of string rebuilding here and when you save the file if
the checksum is always the _last_ entry of the file. (Use rfind to find the
prefix, if you choose to do that)

[please use gerrit instead, 2012/12/05 16:51:26]

Done.

+  if (contents->substr(0, checksum_prefix_.length()) == checksum_prefix_) {
+    size_t after_checksum = contents->find('\n', checksum_prefix_.length());
+    if (after_checksum == std::string::npos) {
+      checksum = contents->substr(
+          checksum_prefix_.length(),
+          contents->length() - checksum_prefix_.length());
+      contents->clear();
+    } else {
+      checksum = contents->substr(checksum_prefix_.length(),
+                                  after_checksum - checksum_prefix_.length());
+      *contents = contents->substr(after_checksum + 1);
+    }
+  }
+  return checksum;
+}