Propagate more information about ValueStore errors to callers, notably an

chrome/browser/value_store/leveldb_value_store.cc

Index: chrome/browser/value_store/leveldb_value_store.cc
diff --git a/chrome/browser/value_store/leveldb_value_store.cc b/chrome/browser/value_store/leveldb_value_store.cc
index 1a2dbe6cd55bafe7f26363e2584f6ec240b4a5e2..8e7e37bd453cbbadeee06c3c14fbe9a86f0aa1a5 100644
--- a/chrome/browser/value_store/leveldb_value_store.cc
+++ b/chrome/browser/value_store/leveldb_value_store.cc
@@ -11,47 +11,24 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
+#include "chrome/browser/value_store/value_store_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/leveldatabase/src/include/leveldb/iterator.h"
 #include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
 
+namespace util = value_store_util;
 using content::BrowserThread;
 
 namespace {
 
-const char* kInvalidJson = "Invalid JSON";
+const char kInvalidJson[] = "Invalid JSON";
 
-ValueStore::ReadResult ReadFailure(const std::string& action,
-                                   const std::string& reason) {
-  CHECK_NE("", reason);
-  return ValueStore::MakeReadResult(base::StringPrintf(
-      "Failure to %s: %s", action.c_str(), reason.c_str()));
-}
-
-ValueStore::ReadResult ReadFailureForKey(const std::string& action,
-                                         const std::string& key,
-                                         const std::string& reason) {
-  CHECK_NE("", reason);
-  return ValueStore::MakeReadResult(base::StringPrintf(
-      "Failure to %s for key %s: %s",
-      action.c_str(), key.c_str(), reason.c_str()));
-}
-
-ValueStore::WriteResult WriteFailure(const std::string& action,
-                                     const std::string& reason) {
-  CHECK_NE("", reason);
-  return ValueStore::MakeWriteResult(base::StringPrintf(
-      "Failure to %s: %s", action.c_str(), reason.c_str()));
-}
-
-ValueStore::WriteResult WriteFailureForKey(const std::string& action,
-                                           const std::string& key,
-                                           const std::string& reason) {
-  CHECK_NE("", reason);
-  return ValueStore::MakeWriteResult(base::StringPrintf(
-      "Failure to %s for key %s: %s",
-      action.c_str(), key.c_str(), reason.c_str()));
-}
+// Hack for converting a base::FilePath to a std::string for use by leveldb.

[Matt Perry, 2013/09/06 20:07:30]

FilePath already has an AsUTF8Unsafe

[not at google - send to devlin, 2013/09/06 21:44:28]

Done.

+#if defined(OS_POSIX)
+#define OS_PATH(file_path) ((file_path).value())
+#elif defined(OS_WIN)
+#define OS_PATH(file_path) base::SysWideToUTF8((file_path).value())
+#endif
 
 // Scoped leveldb snapshot which releases the snapshot on destruction.
 class ScopedSnapshot {
@@ -80,9 +57,9 @@ LeveldbValueStore::LeveldbValueStore(const base::FilePath& db_path)
     : db_path_(db_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    LOG(WARNING) << error;
+  scoped_ptr<Error> open_error = EnsureDbIsOpen();
+  if (open_error)
+    LOG(WARNING) << open_error->message;
 }
 
 LeveldbValueStore::~LeveldbValueStore() {
@@ -91,14 +68,8 @@ LeveldbValueStore::~LeveldbValueStore() {
   // Delete the database from disk if it's empty (but only if we managed to
   // open it!). This is safe on destruction, assuming that we have exclusive
   // access to the database.
-  if (db_ && IsEmpty()) {
-    // Close |db_| now to release any lock on the directory.
-    db_.reset();
-    if (!base::DeleteFile(db_path_, true)) {
-      LOG(WARNING) << "Failed to delete LeveldbValueStore database " <<
-          db_path_.value();
-    }
-  }
+  if (db_ && IsEmpty())
+    DeleteDbFile();
 }
 
 size_t LeveldbValueStore::GetBytesInUse(const std::string& key) {
@@ -123,28 +94,28 @@ size_t LeveldbValueStore::GetBytesInUse() {
 ValueStore::ReadResult LeveldbValueStore::Get(const std::string& key) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    return ValueStore::MakeReadResult(error);
+  scoped_ptr<Error> open_error = EnsureDbIsOpen();
+  if (open_error)
+    return MakeReadResult(open_error.Pass());
 
   scoped_ptr<Value> setting;
-  error = ReadFromDb(leveldb::ReadOptions(), key, &setting);
-  if (!error.empty())
-    return ReadFailureForKey("get", key, error);
+  scoped_ptr<Error> error = ReadFromDb(leveldb::ReadOptions(), key, &setting);
+  if (error)
+    return MakeReadResult(error.Pass());
 
   DictionaryValue* settings = new DictionaryValue();
-  if (setting.get())
+  if (setting)
     settings->SetWithoutPathExpansion(key, setting.release());
-  return MakeReadResult(settings);
+  return MakeReadResult(make_scoped_ptr(settings));
 }
 
 ValueStore::ReadResult LeveldbValueStore::Get(
     const std::vector<std::string>& keys) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    return ValueStore::MakeReadResult(error);
+  scoped_ptr<Error> open_error = EnsureDbIsOpen();
+  if (open_error)
+    return MakeReadResult(open_error.Pass());
 
   leveldb::ReadOptions options;
   scoped_ptr<DictionaryValue> settings(new DictionaryValue());
@@ -156,22 +127,22 @@ ValueStore::ReadResult LeveldbValueStore::Get(
   for (std::vector<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
     scoped_ptr<Value> setting;
-    error = ReadFromDb(options, *it, &setting);
-    if (!error.empty())
-      return ReadFailureForKey("get multiple items", *it, error);
-    if (setting.get())
+    scoped_ptr<Error> error = ReadFromDb(options, *it, &setting);
+    if (error)
+      return MakeReadResult(error.Pass());
+    if (setting)
       settings->SetWithoutPathExpansion(*it, setting.release());
   }
 
-  return MakeReadResult(settings.release());
+  return MakeReadResult(settings.Pass());
 }
 
 ValueStore::ReadResult LeveldbValueStore::Get() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    return ValueStore::MakeReadResult(error);
+  scoped_ptr<Error> open_error = EnsureDbIsOpen();
+  if (open_error)
+    return MakeReadResult(open_error.Pass());
 
   base::JSONReader json_reader;
   leveldb::ReadOptions options = leveldb::ReadOptions();
@@ -185,68 +156,65 @@ ValueStore::ReadResult LeveldbValueStore::Get() {
   for (it->SeekToFirst(); it->Valid(); it->Next()) {
     std::string key = it->key().ToString();
     Value* value = json_reader.ReadToValue(it->value().ToString());
-    if (value == NULL) {
-      // TODO(kalman): clear the offending non-JSON value from the database.
-      return ReadFailureForKey("get all", key, kInvalidJson);
+    if (!value) {
+      return MakeReadResult(
+          Error::Create(CORRUPTION, kInvalidJson, util::NewKey(key)));
     }
     settings->SetWithoutPathExpansion(key, value);
   }
 
   if (it->status().IsNotFound()) {
     NOTREACHED() << "IsNotFound() but iterating over all keys?!";
-    return MakeReadResult(settings.release());
+    return MakeReadResult(settings.Pass());
   }
 
   if (!it->status().ok())
-    return ReadFailure("get all items", it->status().ToString());
+    return MakeReadResult(ToValueStoreError(it->status(), util::NoKey()));
 
-  return MakeReadResult(settings.release());
+  return MakeReadResult(settings.Pass());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Set(
     WriteOptions options, const std::string& key, const Value& value) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    return ValueStore::MakeWriteResult(error);
+  scoped_ptr<Error> open_error = EnsureDbIsOpen();
+  if (open_error)
+    return MakeWriteResult(open_error.Pass());
 
   leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
-  error = AddToBatch(options, key, value, &batch, changes.get());
-  if (!error.empty())
-    return WriteFailureForKey("find changes to set", key, error);
-
-  error = WriteToDb(&batch);
-  if (!error.empty())
-    return WriteFailureForKey("set", key, error);
-  return MakeWriteResult(changes.release());
+  scoped_ptr<Error> batch_error =
+      AddToBatch(options, key, value, &batch, changes.get());
+  if (batch_error)
+    return MakeWriteResult(batch_error.Pass());
+
+  scoped_ptr<Error> write_error = WriteToDb(&batch);
+  return write_error ? MakeWriteResult(write_error.Pass())
+                     : MakeWriteResult(changes.Pass());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Set(
     WriteOptions options, const DictionaryValue& settings) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    return ValueStore::MakeWriteResult(error);
+  scoped_ptr<Error> open_error = EnsureDbIsOpen();
+  if (open_error)
+    return MakeWriteResult(open_error.Pass());
 
   leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
 
   for (DictionaryValue::Iterator it(settings); !it.IsAtEnd(); it.Advance()) {
-    error = AddToBatch(options, it.key(), it.value(), &batch, changes.get());
-    if (!error.empty()) {
-      return WriteFailureForKey("find changes to set multiple items",
-                                it.key(),
-                                error);
-    }
+    scoped_ptr<Error> batch_error =
+        AddToBatch(options, it.key(), it.value(), &batch, changes.get());
+    if (batch_error)
+      return MakeWriteResult(batch_error.Pass());
   }
 
-  error = WriteToDb(&batch);
-  if (!error.empty())
-    return WriteFailure("set multiple items", error);
-  return MakeWriteResult(changes.release());
+  scoped_ptr<Error> write_error = WriteToDb(&batch);
+  return write_error ? MakeWriteResult(write_error.Pass())
+                     : MakeWriteResult(changes.Pass());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Remove(const std::string& key) {
@@ -258,9 +226,9 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
     const std::vector<std::string>& keys) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    return ValueStore::MakeWriteResult(error);
+  scoped_ptr<Error> open_error = EnsureDbIsOpen();
+  if (open_error)
+    return MakeWriteResult(open_error.Pass());
 
   leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
@@ -268,14 +236,12 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
   for (std::vector<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
     scoped_ptr<Value> old_value;
-    error = ReadFromDb(leveldb::ReadOptions(), *it, &old_value);
-    if (!error.empty()) {
-      return WriteFailureForKey("find changes to remove multiple items",
-                                *it,
-                                error);
-    }
+    scoped_ptr<Error> read_error =
+        ReadFromDb(leveldb::ReadOptions(), *it, &old_value);
+    if (read_error)
+      return MakeWriteResult(read_error.Pass());
 
-    if (old_value.get()) {
+    if (old_value) {
       changes->push_back(ValueStoreChange(*it, old_value.release(), NULL));
       batch.Delete(*it);
     }
@@ -283,144 +249,115 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
 
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
   if (!status.ok() && !status.IsNotFound())
-    return WriteFailure("remove multiple items", status.ToString());
-  return MakeWriteResult(changes.release());
+    return MakeWriteResult(ToValueStoreError(status, util::NoKey()));
+  return MakeWriteResult(changes.Pass());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Clear() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string error = EnsureDbIsOpen();
-  if (!error.empty())
-    return ValueStore::MakeWriteResult(error);
-
-  leveldb::ReadOptions read_options;
-  // All interaction with the db is done on the same thread, so snapshotting
-  // isn't strictly necessary.  This is just defensive.
-  leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
 
-  ScopedSnapshot snapshot(db_.get());
-  read_options.snapshot = snapshot.get();
-  scoped_ptr<leveldb::Iterator> it(db_->NewIterator(read_options));
-  for (it->SeekToFirst(); it->Valid(); it->Next()) {
-    const std::string key = it->key().ToString();
-    const std::string old_value_json = it->value().ToString();
-    Value* old_value = base::JSONReader().ReadToValue(old_value_json);
-    if (!old_value) {
-      // TODO: delete the bad JSON.
-      return WriteFailureForKey("find changes to clear", key, kInvalidJson);
-    }
-    changes->push_back(ValueStoreChange(key, old_value, NULL));
-    batch.Delete(key);
+  ReadResult read_result = Get();
+  if (read_result->HasError())
+    return MakeWriteResult(read_result->PassError());
+
+  base::DictionaryValue& whole_db = read_result->settings();
+  while (!whole_db.empty()) {
+    std::string next_key = DictionaryValue::Iterator(whole_db).key();
+    scoped_ptr<base::Value> next_value;
+    whole_db.RemoveWithoutPathExpansion(next_key, &next_value);
+    changes->push_back(
+        ValueStoreChange(next_key, next_value.release(), NULL));
   }
 
-  if (it->status().IsNotFound())
-    NOTREACHED() << "IsNotFound() but clearing?!";
-  else if (!it->status().ok())
-    return WriteFailure("find changes to clear", it->status().ToString());
-
-  leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
-  if (status.IsNotFound()) {
-    NOTREACHED() << "IsNotFound() but clearing?!";
-    return MakeWriteResult(changes.release());
-  }
-  if (!status.ok())
-    return WriteFailure("clear", status.ToString());
-  return MakeWriteResult(changes.release());
+  DeleteDbFile();
+  return MakeWriteResult(changes.Pass());
 }
 
-std::string LeveldbValueStore::EnsureDbIsOpen() {
+scoped_ptr<ValueStore::Error> LeveldbValueStore::EnsureDbIsOpen() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  if (db_.get())
-    return std::string();
-
-#if defined(OS_POSIX)
-  std::string os_path(db_path_.value());
-#elif defined(OS_WIN)
-  std::string os_path = base::SysWideToUTF8(db_path_.value());
-#endif
+  if (db_)
+    return util::NoError();
 
   leveldb::Options options;
   options.max_open_files = 0;  // Use minimum.
   options.create_if_missing = true;
-  leveldb::DB* db;
-  leveldb::Status status = leveldb::DB::Open(options, os_path, &db);
-  if (!status.ok()) {
-    // |os_path| may contain sensitive data, and these strings are passed
-    // through to the extension, so strip that out.
-    std::string status_string = status.ToString();
-    ReplaceSubstringsAfterOffset(&status_string, 0u, os_path, "...");
-    return base::StringPrintf("Failed to open database: %s",
-                              status_string.c_str());
-  }
 
+  leveldb::DB* db = NULL;
+  leveldb::Status status = leveldb::DB::Open(options, OS_PATH(db_path_), &db);
+  if (!status.ok())
+    return ToValueStoreError(status, util::NoKey());
+
+  CHECK(db);
   db_.reset(db);
-  return std::string();
+  return util::NoError();
 }
 
-std::string LeveldbValueStore::ReadFromDb(
+scoped_ptr<ValueStore::Error> LeveldbValueStore::ReadFromDb(
     leveldb::ReadOptions options,
     const std::string& key,
     scoped_ptr<Value>* setting) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(setting != NULL);
+  DCHECK(setting);
+
   std::string value_as_json;
   leveldb::Status s = db_->Get(options, key, &value_as_json);
 
   if (s.IsNotFound()) {
-    // Despite there being no value, it was still a success.
-    // Check this first because ok() is false on IsNotFound.
-    return std::string();
+    // Despite there being no value, it was still a success. Check this first
+    // because ok() is false on IsNotFound.
+    return util::NoError();
   }
 
   if (!s.ok())
-    return s.ToString();
+    return ToValueStoreError(s, util::NewKey(key));
 
   Value* value = base::JSONReader().ReadToValue(value_as_json);
-  if (value == NULL) {
-    // TODO(kalman): clear the offending non-JSON value from the database.
-    return kInvalidJson;
-  }
+  if (!value)
+    return Error::Create(CORRUPTION, kInvalidJson, util::NewKey(key));
 
   setting->reset(value);
-  return std::string();
+  return util::NoError();
 }
 
-std::string LeveldbValueStore::AddToBatch(
+scoped_ptr<ValueStore::Error> LeveldbValueStore::AddToBatch(
     ValueStore::WriteOptions options,
     const std::string& key,
     const base::Value& value,
     leveldb::WriteBatch* batch,
     ValueStoreChangeList* changes) {
-  scoped_ptr<Value> old_value;
-  if (!(options & NO_CHECK_OLD_VALUE)) {
-    std::string error = ReadFromDb(leveldb::ReadOptions(), key, &old_value);
-    if (!error.empty())
-      return error;
-  }
+  bool write_new_value = true;
 
-  if (!old_value.get() || !old_value->Equals(&value)) {
-    if (!(options & NO_GENERATE_CHANGES)) {
+  if (!(options & NO_GENERATE_CHANGES)) {
+    scoped_ptr<Value> old_value;
+    scoped_ptr<Error> read_error =
+        ReadFromDb(leveldb::ReadOptions(), key, &old_value);
+    if (read_error)
+      return read_error.Pass();
+    if (!old_value || !old_value->Equals(&value)) {
       changes->push_back(
           ValueStoreChange(key, old_value.release(), value.DeepCopy()));
+    } else {
+      write_new_value = false;
     }
+  }
+
+  if (write_new_value) {
     std::string value_as_json;
     base::JSONWriter::Write(&value, &value_as_json);
     batch->Put(key, value_as_json);
   }
 
-  return std::string();
+  return util::NoError();
 }
 
-std::string LeveldbValueStore::WriteToDb(leveldb::WriteBatch* batch) {
+scoped_ptr<ValueStore::Error> LeveldbValueStore::WriteToDb(
+    leveldb::WriteBatch* batch) {
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), batch);
-  if (status.IsNotFound()) {
-    NOTREACHED() << "IsNotFound() but writing?!";
-    return std::string();
-  }
-  return status.ok() ? std::string() : status.ToString();
+  return status.ok() ? util::NoError()
+                     : ToValueStoreError(status, util::NoKey());
 }
 
 bool LeveldbValueStore::IsEmpty() {
@@ -435,3 +372,25 @@ bool LeveldbValueStore::IsEmpty() {
   }
   return is_empty;
 }
+
+void LeveldbValueStore::DeleteDbFile() {
+  db_.reset();  // release any lock on the directory
+  if (!base::DeleteFile(db_path_, true /* recursive */)) {
+    LOG(WARNING) << "Failed to delete LeveldbValueStore database at " <<
+        db_path_.value();
+  }
+}
+
+scoped_ptr<ValueStore::Error> LeveldbValueStore::ToValueStoreError(
+    const leveldb::Status& status,
+    scoped_ptr<std::string> key) {
+  CHECK(!status.ok());
+  CHECK(!status.IsNotFound());  // not an error
+
+  std::string message = status.ToString();
+  // The message may contain |db_path_|, which may be considered sensitive
+  // data, and those strings are passed to the extension, so strip it out.
+  ReplaceSubstringsAfterOffset(&message, 0u, OS_PATH(db_path_), "...");
+
+  return Error::Create(CORRUPTION, message, key.Pass());
+}