Add a Restore() method to ValueStore and make StorageAPI use it

chrome/browser/extensions/api/storage/storage_api.cc

Index: chrome/browser/extensions/api/storage/storage_api.cc
diff --git a/chrome/browser/extensions/api/storage/storage_api.cc b/chrome/browser/extensions/api/storage/storage_api.cc
index 1d6b361110b9b72aa7694a844be3f8073014ee09..9c402a572153443532b8badef801a13b6a3b2171 100644
--- a/chrome/browser/extensions/api/storage/storage_api.cc
+++ b/chrome/browser/extensions/api/storage/storage_api.cc
@@ -73,20 +73,30 @@ void SettingsFunction::AsyncRunWithStorage(ValueStore* storage) {
       base::Bind(&SettingsFunction::SendResponse, this, success));
 }
 
-bool SettingsFunction::UseReadResult(ValueStore::ReadResult read_result) {
-  if (read_result->HasError()) {
-    error_ = read_result->error().message;
+bool SettingsFunction::UseReadResult(ValueStore::ReadResult result,
+                                     ValueStore* storage) {
+  if (result->HasError()) {
+    if (result->error().code == ValueStore::CORRUPTION &&

[not at google - send to devlin, 2014/02/14 19:35:56]

result->IsCorruption()

[Devlin, 2014/02/18 23:55:22]

Unneeded now.

+        !tried_restoring_storage_) {
+      return RestoreStorageAndRetry(result->error(), storage);
+    }
+    error_ = result->error().message;
     return false;
   }
 
-  base::DictionaryValue* result = new base::DictionaryValue();
-  result->Swap(&read_result->settings());
-  SetResult(result);
+  base::DictionaryValue* dict = new base::DictionaryValue();

[not at google - send to devlin, 2014/02/14 19:35:56]

what was wrong with read_result vs result?

[Devlin, 2014/02/18 23:55:22]

It was declared as |result| in the .h file
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...),
so, to make the definition match the declaration, I picked |result|, since it
happened to match UseWriteResult.  I can instead switch the declaration to be
read_result, if you'd prefer. :)

+  dict->Swap(&result->settings());
+  SetResult(dict);
   return true;
 }
 
-bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result) {
+bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result,
+                                      ValueStore* storage) {
   if (result->HasError()) {
+    if (result->error().code == ValueStore::CORRUPTION &&

[not at google - send to devlin, 2014/02/14 19:35:56]

ditto

[Devlin, 2014/02/18 23:55:22]

Unneeded now.

+        !tried_restoring_storage_) {
+      return RestoreStorageAndRetry(result->error(), storage);
+    }
     error_ = result->error().message;
     return false;
   }
@@ -102,6 +112,29 @@ bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result) {
   return true;
 }
 
+bool SettingsFunction::RestoreStorageAndRetry(const ValueStore::Error& error,
+                                              ValueStore* storage) {
+  DCHECK(!tried_restoring_storage_);
+  DCHECK(error.code == ValueStore::CORRUPTION);
+  tried_restoring_storage_ = true;

[not at google - send to devlin, 2014/02/14 19:35:56]

I'd rather this state be entirely handled within this method; rename this
"OnError", "HandleError", something, then instead of these DCHECKs have like

bool SettingsFunction::HandleError(const ValueStore::Error& error, ValueStore*
storage) {
  if (is_handling_error_)
    return false;
  is_handling_error_ = true;

  switch (error.code) {
    case ValueStore::CORRUPTION:
      // all the stuff you've written here.
      return ...;

    case ValueStore::...:
    case ValueStore::...:
     ...
     return ...;
  }

  NOTREACHED();
}

[Devlin, 2014/02/18 23:55:22]

Done (mostly - there's not really a need for a switch() since we only handle
CORRUPTION).

+
+  // If the corruption is on a particular key, try to restore that key and
+  // re-run.
+  if (error.key.get()) {
+    if (storage->RestoreKey(*error.key))
+      return RunWithStorage(storage);
+  }
+
+  // If the full database is corrupted, try to restore the whole thing and
+  // re-run.
+  if (storage->Restore())
+    return RunWithStorage(storage);
+
+  // Failing that, there's nothing else we can do.
+  error_ = error.message;
+  return false;
+}
+
 // Concrete settings functions
 
 namespace {
@@ -161,32 +194,34 @@ bool StorageStorageAreaGetFunction::RunWithStorage(ValueStore* storage) {
 
   switch (input->GetType()) {
     case base::Value::TYPE_NULL:
-      return UseReadResult(storage->Get());
+      return UseReadResult(storage->Get(), storage);
 
     case base::Value::TYPE_STRING: {
       std::string as_string;
       input->GetAsString(&as_string);
-      return UseReadResult(storage->Get(as_string));
+      return UseReadResult(storage->Get(as_string), storage);
     }
 
     case base::Value::TYPE_LIST: {
       std::vector<std::string> as_string_list;
       AddAllStringValues(*static_cast<base::ListValue*>(input),
                          &as_string_list);
-      return UseReadResult(storage->Get(as_string_list));
+      return UseReadResult(storage->Get(as_string_list), storage);
     }
 
     case base::Value::TYPE_DICTIONARY: {
-      base::DictionaryValue* as_dict = static_cast<base::DictionaryValue*>(input);
+      base::DictionaryValue* as_dict =
+          static_cast<base::DictionaryValue*>(input);
       ValueStore::ReadResult result = storage->Get(GetKeys(*as_dict));
       if (result->HasError()) {
-        return UseReadResult(result.Pass());
+        return UseReadResult(result.Pass(), storage);
       }
 
       base::DictionaryValue* with_default_values = as_dict->DeepCopy();
       with_default_values->MergeDictionary(&result->settings());
       return UseReadResult(
-          ValueStore::MakeReadResult(make_scoped_ptr(with_default_values)));
+          ValueStore::MakeReadResult(make_scoped_ptr(with_default_values)),
+          storage);
     }
 
     default:
@@ -234,7 +269,7 @@ bool StorageStorageAreaGetBytesInUseFunction::RunWithStorage(
 bool StorageStorageAreaSetFunction::RunWithStorage(ValueStore* storage) {
   base::DictionaryValue* input = NULL;
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &input));
-  return UseWriteResult(storage->Set(ValueStore::DEFAULTS, *input));
+  return UseWriteResult(storage->Set(ValueStore::DEFAULTS, *input), storage);
 }
 
 void StorageStorageAreaSetFunction::GetQuotaLimitHeuristics(
@@ -250,14 +285,14 @@ bool StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) {
     case base::Value::TYPE_STRING: {
       std::string as_string;
       input->GetAsString(&as_string);
-      return UseWriteResult(storage->Remove(as_string));
+      return UseWriteResult(storage->Remove(as_string), storage);
     }
 
     case base::Value::TYPE_LIST: {
       std::vector<std::string> as_string_list;
       AddAllStringValues(*static_cast<base::ListValue*>(input),
                          &as_string_list);
-      return UseWriteResult(storage->Remove(as_string_list));
+      return UseWriteResult(storage->Remove(as_string_list), storage);
     }
 
     default:
@@ -272,7 +307,7 @@ void StorageStorageAreaRemoveFunction::GetQuotaLimitHeuristics(
 }
 
 bool StorageStorageAreaClearFunction::RunWithStorage(ValueStore* storage) {
-  return UseWriteResult(storage->Clear());
+  return UseWriteResult(storage->Clear(), storage);
 }
 
 void StorageStorageAreaClearFunction::GetQuotaLimitHeuristics(