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..d526a80513ee9796fb5bc019d19ab46a9ba71536 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 = ") {

[groby-ooo-7-16, 2012/12/04 00:16:30]

Why a prefix? Why not just make sure it's a valid MD5 and be done?

   DCHECK(profile);
   custom_dictionary_path_ =
       profile_->GetPath().Append(chrome::kCustomDictionaryFileName);
@@ -50,7 +54,8 @@ void SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   std::string contents;
-  file_util::ReadFileToString(custom_dictionary_path_, &contents);
+  LoadDictionaryContentsReliably(&contents);
+
   if (contents.empty()) {
     custom_words->clear();

[groby-ooo-7-16, 2012/12/04 00:16:30]

It's not your CL, but I suggest just calling custom_words.clear() in any case,
without the if-path. Otherwise, and entirely corrupt dictionary will not be
saved out again as an empty dictionary. (Unless that's intentional?)

[please use gerrit instead, 2012/12/04 05:46:40]

Done.

     return;
@@ -58,26 +63,23 @@ void SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
 
   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());
-
-  // 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.
+  custom_words->erase(std::remove_if(custom_words->begin(),

[groby-ooo-7-16, 2012/12/04 00:16:30]

While this works, it just occurred to me that only the first word can be empty.
We're sorted and unique :) (In case you want to simplify this code)

[please use gerrit instead, 2012/12/04 05:46:40]

Done.

+                                     custom_words->end(),
+                                     std::mem_fun_ref(&std::string::empty)),
+                      custom_words->end());
   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());
+
+  SaveDictionryContentsReliably(ss.str());
 }
 
 void SpellcheckCustomDictionary::SetCustomWordList(WordList* custom_words) {
@@ -87,9 +89,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());

[groby-ooo-7-16, 2012/12/04 00:16:30]

+1 for awesome macros!

 }
 
 bool SpellcheckCustomDictionary::AddWord(const std::string& word) {
@@ -108,9 +108,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 +129,18 @@ 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());
-  }
+  std::string contents;
+  LoadDictionaryContentsReliably(&contents);
+
+  std::stringstream ss;
+  ss << contents;
+  if (contents.length() > 0 && contents[contents.length() - 1] != '\n')
+    ss << '\n';

[groby-ooo-7-16, 2012/12/04 00:16:30]

Doesn't "LoadReliably" guarantee proper format?

Also, not happy with using a stringstream here - why not contents.append("\n");
?

[please use gerrit instead, 2012/12/04 05:46:40]

Done.

+  ss << word << '\n';
+
+  SaveDictionryContentsReliably(ss.str());
 }
 
 bool SpellcheckCustomDictionary::RemoveWord(const std::string& word) {
@@ -161,9 +159,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;
 }
@@ -185,38 +181,35 @@ void SpellcheckCustomDictionary::EraseWordFromCustomDictionary(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(IsStringUTF8(word));
 
+  std::string contents;
+  LoadDictionaryContentsReliably(&contents);
+  if (contents.empty())
+    return;
+
   WordList custom_words;
-  LoadDictionaryIntoCustomWordList(&custom_words);
+  base::SplitString(contents, '\n', &custom_words);
 
-  const char empty[] = {'\0'};
-  const char separator[] = {'\n', '\0'};
-  file_util::WriteFile(custom_dictionary_path_, empty, 0);
+  std::stringstream ss;
   for (WordList::iterator it = custom_words.begin();

[groby-ooo-7-16, 2012/12/04 00:16:30]

This would make the code easier to read:
  it = custom_words.find(word);
  if (it != custom_words.end()) custom_words.erase(it);
  // build ss

[please use gerrit instead, 2012/12/04 05:46:40]

Done.

-      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);
-    }
+       it != custom_words.end();

[groby-ooo-7-16, 2012/12/04 00:16:30]

The above remark (std::erase) brings up another point - we _always_ call
SplitString after LoadDictionaryContentsReliably. Can we just have it return the
list of strings?

And SaveDictReliably almost always takes a string constructed from a vector, too
- so I think using a vector instead of a string API makes more sense there.

[please use gerrit instead, 2012/12/04 05:46:40]

Done.

+       ++it) {
+    if (!(*it).empty() && *it != word)
+      ss << *it << '\n';
   }
+
+  SaveDictionryContentsReliably(ss.str());
 }
 
 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 +227,69 @@ void SpellcheckCustomDictionary::SetCustomWordListAndDelete(
   SetCustomWordList(custom_words);
   delete custom_words;
 }
+
+void SpellcheckCustomDictionary::LoadDictionaryContentsReliably(
+    std::string* contents) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  file_util::ReadFileToString(custom_dictionary_path_, contents);
+  std::string checksum = GetChecksum(contents);
+  if (checksum.empty())

[groby-ooo-7-16, 2012/12/04 00:16:30]

Hm. This looks like it will fail for any dictionaries that have been created
before we added the checksum mechanism. Can we add a test case for that? (It'd
be great to have some for the other behaviors, too)

[please use gerrit instead, 2012/12/04 05:46:40]

Empty checksum most likely indicates that this is a legacy file, created before
we adding the checksum mechanism. Added a test case.

+    return;

[groby-ooo-7-16, 2012/12/04 00:16:30]

If you go with the one-file-to-rule-them all approach, I'd suggest making the
first or the last line the checksum. And only checksumming all the other lines,
obviously :)

[please use gerrit instead, 2012/12/04 05:46:40]

Done.

+
+  base::MD5Digest digest;
+  base::MD5Sum(contents->c_str(), contents->length(), &digest);
+  if (checksum == base::MD5DigestToBase16(digest))
+    return;
+
+  FilePath backup_path =
+      custom_dictionary_path_.AddExtension(backup_extension_);
+  if (!file_util::PathExists(backup_path))
+    return;
+
+  std::string backup;

[groby-ooo-7-16, 2012/12/04 00:16:30]

Backup is just the last copy saved before this one, right? Why do we even care
about it in the load path? An MD5 checksum is a pretty strong indicator that
things are OK... 

Either way, I'm not sure creating a backup in the Load path is a good plan -
each load operation creates a new copy. Quite a bit of cost for something that's
basically a startup operation.

If I may make a suggestion, I'd only load the backup copy if the primary copy
failed a checksum check. (And create a backup copy right before you save out a
new version, in SaveReliably)

[please use gerrit instead, 2012/12/04 05:46:40]

Done.

+  file_util::ReadFileToString(backup_path, &backup);
+  std::string backup_checksum = GetChecksum(&backup);
+  base::MD5Digest backup_digest;
+  base::MD5Sum(backup.c_str(), backup.length(), &backup_digest);
+  if (backup_checksum != base::MD5DigestToBase16(backup_digest))
+    return;
+
+  *contents = backup;
+  file_util::CopyFile(backup_path, custom_dictionary_path_);
+}
+
+void SpellcheckCustomDictionary::SaveDictionryContentsReliably(
+    const std::string& contents) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  FilePath backup_path =
+      custom_dictionary_path_.AddExtension(backup_extension_);
+  if (file_util::PathExists(custom_dictionary_path_))
+    file_util::CopyFile(custom_dictionary_path_, backup_path);
+
+  base::MD5Digest digest;
+  base::MD5Sum(contents.c_str(), contents.length(), &digest);
+  std::stringstream ss;
+  ss << checksum_prefix_ << base::MD5DigestToBase16(digest) << '\n' << contents;
+  base::ImportantFileWriter::WriteFileAtomically(custom_dictionary_path_,

[groby-ooo-7-16, 2012/12/04 00:16:30]

I might be wrong, but doesn't this just write the _checksum_ into the custom
dictionary path?

[please use gerrit instead, 2012/12/04 05:46:40]

It writes the prefix, the checksum, a newline, and the contents into the custom
dictionary file, which is located at the custom dictionary path.

+                                                 ss.str());
+}
+
+std::string SpellcheckCustomDictionary::GetChecksum(std::string* contents) {
+  std::string checksum;

[groby-ooo-7-16, 2012/12/04 00:16:30]

You can check if something is a checksum much easier if you go 16-digit hex
value:

1) Check the length == 16
2) Call digest.find_first_not_of("0123456789abcdef"). If it's std::npos, it's
not a checksum.

The difficult question is, what do we do if it's not a checksum - it can either
be an old file without checksum, or a new file with a broken checksum.

On second thought, it's worth keeping the prefix, if we include non-word
characters in there. This way, if a string doesn't match the prefix & doesn't
have non-word chars, we can make a reasonable guess it's a word, and we
encountered an old-style dictionary.

And because I worry about migrations in the future, I'd suggest a version number
in the prefix so in the future, we can cleanly migrate

I.e. checksum_v1 = <MD5> seems reasonable.

[please use gerrit instead, 2012/12/04 05:46:40]

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;
+}