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

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

Index: chrome/browser/extensions/api/storage/policy_value_store.cc
diff --git a/chrome/browser/extensions/api/storage/policy_value_store.cc b/chrome/browser/extensions/api/storage/policy_value_store.cc
index f5da68205785a23105b728cac0fa2feef3c39eba..7109381db7474139c796c154de7d0f7689b32d4b 100644
--- a/chrome/browser/extensions/api/storage/policy_value_store.cc
+++ b/chrome/browser/extensions/api/storage/policy_value_store.cc
@@ -57,6 +57,13 @@ void PolicyValueStore::SetCurrentPolicy(const policy::PolicyMap& policy) {
   // must be removed.
   base::DictionaryValue previous_policy;
   ValueStore::ReadResult read_result = delegate_->Get();
+
+  // If the database is corrupted, try to restore it.
+  if (read_result->IsCorrupted()) {
+    if (delegate_->Restore())
+      read_result = delegate_->Get();

[not at google - send to devlin, 2014/02/19 18:40:37]

I still don't think this is quite right though, because now it means that down
below where the changes are calculated the extension will get told that every
key was added.

However maybe it's better to have that behaviour vs not saying anything at all,
just in case.

Anyway it's more subtle than "if the database is corrupted, try to restore it".

[Devlin, 2014/02/19 19:13:55]

Yes, the extension will be notified of all keys being added, but I don't think
that it's actually new behavior.  Previously (and still, if it's an error other
than CORRUPTION), we just don't swap in the previous policy, so we'd still be
notified of every new key.  Isn't this behavior basically the same (we found an
error, we couldn't get the old stuff, so we say everything's added)?

Regardless, I'll add a comment to make it explicitly clear.

+  }
+
   if (read_result->HasError()) {
     LOG(WARNING) << "Failed to read managed settings for extension "
         << extension_id_ << ": " << read_result->error().message;
@@ -161,4 +168,10 @@ ValueStore::WriteResult PolicyValueStore::Clear() {
   return MakeWriteResult(ReadOnlyError(util::NoKey()));
 }
 
+bool PolicyValueStore::Restore() { return delegate_->Restore(); }

[not at google - send to devlin, 2014/02/19 18:40:37]

GRR GIT CL FORMAT IS WRONG.

anyhow, it seems like this should be

bool PolicyVaueStore::Restore() {
  if delegate_->Restore()) {
    delegate_->Set(/* the current policy state */);
    return true;
  }
  return false;
}

? or am I missing something?

[not at google - send to devlin, 2014/02/19 18:48:26]

I suppose if Restore actually does manage to restore it then you wouldn't need
to read in from store.

But if Restore ends up clearing then you'd need to do the equivalent of the code
I just wrote.

But if Restore fails at everything you don't want to touch it at all.

So maybe Restore needs to be an enum :)

[Devlin, 2014/02/19 19:13:55]

Ideally, yes, Restore() would give more information about what actually
happened.  But we wouldn't be able to use an enum (or at least, not just an
enum).  Restore(), while more intelligent than immediately blowing away the
database, still doesn't actually "restore" data - it just selectively removes
corrupted pieces.  But the smallest chunk ever removed is a full key.  So the
return value would need to indicate:
- Did we succeed?
- Did we remove everything?
  - If no, which keys did we remove?

Then, whatever called restore could make an effort to reconcile the damage.

Unfortunately, I think this might be too much to cram into this already-large
change (but if you think it's worth adding it now, I'm happy to).  But it
shouldn't be disastrous in either case where we could get storage back (here and
sync).  In both cases, we'll reconcile the storage once we restart Chrome.  Not
ideal, but hopefully better.

+
+bool PolicyValueStore::RestoreKey(const std::string& key) {
+  return delegate_->RestoreKey(key);
+}
+
 }  // namespace extensions