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

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

Index: chrome/browser/extensions/api/storage/sync_storage_backend.cc
diff --git a/chrome/browser/extensions/api/storage/settings_backend.cc b/chrome/browser/extensions/api/storage/sync_storage_backend.cc
similarity index 67%
copy from chrome/browser/extensions/api/storage/settings_backend.cc
copy to chrome/browser/extensions/api/storage/sync_storage_backend.cc
index df2bd134c0fa652bc7092e14042509ef32becf2f..838d3af164d2bb619ca474ecd9d9b079b40fa6e3 100644
--- a/chrome/browser/extensions/api/storage/settings_backend.cc
+++ b/chrome/browser/extensions/api/storage/sync_storage_backend.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/extensions/api/storage/settings_backend.h"
+#include "chrome/browser/extensions/api/storage/sync_storage_backend.h"
 
 #include "base/files/file_enumerator.h"
 #include "base/logging.h"
@@ -16,16 +16,28 @@ using content::BrowserThread;
 
 namespace extensions {
 
-SettingsBackend::SettingsBackend(
+namespace {
+
+void AddAllSyncData(const std::string& extension_id,
+                    const base::DictionaryValue& src,
+                    syncer::ModelType type,
+                    syncer::SyncDataList* dst) {
+  for (base::DictionaryValue::Iterator it(src); !it.IsAtEnd(); it.Advance()) {
+    dst->push_back(settings_sync_util::CreateData(
+        extension_id, it.key(), it.value(), type));
+  }
+}
+
+}  // namespace
+
+SyncStorageBackend::SyncStorageBackend(
     const scoped_refptr<SettingsStorageFactory>& storage_factory,
     const base::FilePath& base_path,
-    syncer::ModelType sync_type,
-    const syncer::SyncableService::StartSyncFlare& flare,
     const SettingsStorageQuotaEnforcer::Limits& quota,
-    const scoped_refptr<SettingsObserverList>& observers)
-    : storage_factory_(storage_factory),
-      base_path_(base_path),
-      quota_(quota),
+    const scoped_refptr<SettingsObserverList>& observers,
+    syncer::ModelType sync_type,
+    const syncer::SyncableService::StartSyncFlare& flare)
+    : SettingsBackend(storage_factory, base_path, quota),
       observers_(observers),
       sync_type_(sync_type),
       flare_(flare) {
@@ -34,18 +46,15 @@ SettingsBackend::SettingsBackend(
          sync_type_ == syncer::APP_SETTINGS);
 }
 
-SettingsBackend::~SettingsBackend() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-}
+SyncStorageBackend::~SyncStorageBackend() {}
 
-ValueStore* SettingsBackend::GetStorage(
-    const std::string& extension_id) const {
+ValueStore* SyncStorageBackend::GetStorage(const std::string& extension_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   base::DictionaryValue empty;
   return GetOrCreateStorageWithSyncData(extension_id, empty);
 }
 
-SyncableSettingsStorage* SettingsBackend::GetOrCreateStorageWithSyncData(
+SyncableSettingsStorage* SyncStorageBackend::GetOrCreateStorageWithSyncData(
     const std::string& extension_id,
     const base::DictionaryValue& sync_data) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -55,34 +64,38 @@ SyncableSettingsStorage* SettingsBackend::GetOrCreateStorageWithSyncData(
     return maybe_storage->second.get();
   }
 
-  ValueStore* storage = storage_factory_->Create(base_path_, extension_id);
+  ValueStore* storage = storage_factory()->Create(base_path(), extension_id);
   CHECK(storage);
 
+  // Check the ValueStore to ensure that it's not corrupted. If it is corrupted,
+  // make an effort to restore it, and, failing that, clear it forcefully.
+  // This is slow and not ideal, because it means that we always read the full
+  // synced database for corruption. Unfortunately, there's no good alternative,
+  // because we have to catch this *before* we sync. Thankfully, sync databases
+  // are small by policy.
+  ValueStore::ReadResult result = storage->Get();

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

corruption seems pretty rare, and there isn't much harm in creating a quota
enforcer over a corrupt database. so, more of this logic is common with the
local storage backend than just this (I suppose that's why it was combined
before).

What I would imagine: the factory still does the work (but curries in the base
path).

the local storage backend would just create the storage.

this class would create the storage, try to read, restore on failure, then
re-create.

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

Oooh, good point.  We can actually do more, because I forgot that QuotaEnforcer
has to read the full ValueStore at construction anyway. So we can actually have
QuotaEnforcer restore the DB, and then put almost all creation in
SettingsBackend. Thanks :)

+  if (result->HasError())
+    storage->Restore();
+
   // It's fine to create the quota enforcer underneath the sync layer, since
   // sync will only go ahead if each underlying storage operation succeeds.
-  storage = new SettingsStorageQuotaEnforcer(quota_, storage);
+  storage = new SettingsStorageQuotaEnforcer(quota(), storage);
 
   linked_ptr<SyncableSettingsStorage> syncable_storage(
       new SyncableSettingsStorage(
-          observers_,
-          extension_id,
-          storage,
-          sync_type_,
-          flare_));
+          observers_, extension_id, storage, sync_type_, flare_));
   storage_objs_[extension_id] = syncable_storage;
 
   if (sync_processor_.get()) {
-    syncer::SyncError error =
-        syncable_storage->StartSyncing(
-            sync_data,
-            CreateSettingsSyncProcessor(extension_id).Pass());
+    syncer::SyncError error = syncable_storage->StartSyncing(
+        sync_data, CreateSettingsSyncProcessor(extension_id).Pass());
     if (error.IsSet())
       syncable_storage.get()->StopSyncing();
   }
   return syncable_storage.get();
 }
 
-void SettingsBackend::DeleteStorage(const std::string& extension_id) {
+void SyncStorageBackend::DeleteStorage(const std::string& extension_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Clear settings when the extension is uninstalled.  Leveldb implementations
@@ -100,20 +113,25 @@ void SettingsBackend::DeleteStorage(const std::string& extension_id) {
   storage_objs_.erase(extension_id);
 }
 
-std::set<std::string> SettingsBackend::GetKnownExtensionIDs() const {
+syncer::SyncableService* SyncStorageBackend::GetAsSyncableService() {
+  return this;
+}
+
+std::set<std::string> SyncStorageBackend::GetKnownExtensionIDs() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   std::set<std::string> result;
 
   // Storage areas can be in-memory as well as on disk. |storage_objs_| will
   // contain all that are in-memory.
   for (StorageObjMap::iterator it = storage_objs_.begin();
-      it != storage_objs_.end(); ++it) {
+       it != storage_objs_.end();
+       ++it) {
     result.insert(it->first);
   }
 
-  // Leveldb databases are directories inside base_path_.
+  // Leveldb databases are directories inside base_path().
   base::FileEnumerator extension_dirs(
-      base_path_, false, base::FileEnumerator::DIRECTORIES);
+      base_path(), false, base::FileEnumerator::DIRECTORIES);
   while (!extension_dirs.Next().empty()) {
     base::FilePath extension_dir = extension_dirs.GetInfo().GetName();
     DCHECK(!extension_dir.IsAbsolute());
@@ -127,24 +145,12 @@ std::set<std::string> SettingsBackend::GetKnownExtensionIDs() const {
   return result;
 }
 
-static void AddAllSyncData(
-    const std::string& extension_id,
-    const base::DictionaryValue& src,
-    syncer::ModelType type,
-    syncer::SyncDataList* dst) {
-  for (base::DictionaryValue::Iterator it(src); !it.IsAtEnd(); it.Advance()) {
-    dst->push_back(settings_sync_util::CreateData(
-        extension_id, it.key(), it.value(), type));
-  }
-}
-
-syncer::SyncDataList SettingsBackend::GetAllSyncData(
-    syncer::ModelType type) const {
+syncer::SyncDataList SyncStorageBackend::GetAllSyncData(syncer::ModelType type)
+    const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   // Ignore the type, it's just for sanity checking; assume that whatever base
   // path we're constructed with is correct for the sync type.
-  DCHECK(type == syncer::EXTENSION_SETTINGS ||
-         type == syncer::APP_SETTINGS);
+  DCHECK(type == syncer::EXTENSION_SETTINGS || type == syncer::APP_SETTINGS);
 
   // For all extensions, get all their settings.  This has the effect
   // of bringing in the entire state of extension settings in memory; sad.
@@ -152,11 +158,13 @@ syncer::SyncDataList SettingsBackend::GetAllSyncData(
   std::set<std::string> known_extension_ids(GetKnownExtensionIDs());
 
   for (std::set<std::string>::const_iterator it = known_extension_ids.begin();
-      it != known_extension_ids.end(); ++it) {
-    ValueStore::ReadResult maybe_settings = GetStorage(*it)->Get();
+       it != known_extension_ids.end();
+       ++it) {
+    ValueStore::ReadResult maybe_settings =
+        GetOrCreateStorageWithSyncData(*it, base::DictionaryValue())->Get();
     if (maybe_settings->HasError()) {
-      LOG(WARNING) << "Failed to get settings for " << *it << ": " <<
-          maybe_settings->error().message;
+      LOG(WARNING) << "Failed to get settings for " << *it << ": "
+                   << maybe_settings->error().message;
       continue;
     }
     AddAllSyncData(*it, maybe_settings->settings(), type, &all_sync_data);
@@ -165,7 +173,7 @@ syncer::SyncDataList SettingsBackend::GetAllSyncData(
   return all_sync_data;
 }
 
-syncer::SyncMergeResult SettingsBackend::MergeDataAndStartSyncing(
+syncer::SyncMergeResult SyncStorageBackend::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
@@ -182,24 +190,27 @@ syncer::SyncMergeResult SettingsBackend::MergeDataAndStartSyncing(
   // Group the initial sync data by extension id.
   std::map<std::string, linked_ptr<base::DictionaryValue> > grouped_sync_data;
   for (syncer::SyncDataList::const_iterator it = initial_sync_data.begin();
-      it != initial_sync_data.end(); ++it) {
+       it != initial_sync_data.end();
+       ++it) {
     SettingSyncData data(*it);
     linked_ptr<base::DictionaryValue> sync_data =
         grouped_sync_data[data.extension_id()];
     if (!sync_data.get()) {
-      sync_data = linked_ptr<base::DictionaryValue>(
-          new base::DictionaryValue());
+      sync_data =
+          linked_ptr<base::DictionaryValue>(new base::DictionaryValue());
       grouped_sync_data[data.extension_id()] = sync_data;
     }
-    DCHECK(!sync_data->HasKey(data.key())) <<
-        "Duplicate settings for " << data.extension_id() << "/" << data.key();
+    DCHECK(!sync_data->HasKey(data.key())) << "Duplicate settings for "
+                                           << data.extension_id() << "/"
+                                           << data.key();
     sync_data->SetWithoutPathExpansion(data.key(), data.value().DeepCopy());
   }
 
   // Start syncing all existing storage areas.  Any storage areas created in
   // the future will start being synced as part of the creation process.
   for (StorageObjMap::iterator it = storage_objs_.begin();
-      it != storage_objs_.end(); ++it) {
+       it != storage_objs_.end();
+       ++it) {
     std::map<std::string, linked_ptr<base::DictionaryValue> >::iterator
         maybe_sync_data = grouped_sync_data.find(it->first);
     syncer::SyncError error;
@@ -211,8 +222,7 @@ syncer::SyncMergeResult SettingsBackend::MergeDataAndStartSyncing(
     } else {
       base::DictionaryValue empty;
       error = it->second->StartSyncing(
-          empty,
-          CreateSettingsSyncProcessor(it->first).Pass());
+          empty, CreateSettingsSyncProcessor(it->first).Pass());
     }
     if (error.IsSet())
       it->second->StopSyncing();
@@ -222,14 +232,16 @@ syncer::SyncMergeResult SettingsBackend::MergeDataAndStartSyncing(
   // Under normal circumstances (i.e. not first-time sync) this will be all of
   // them.
   for (std::map<std::string, linked_ptr<base::DictionaryValue> >::iterator it =
-      grouped_sync_data.begin(); it != grouped_sync_data.end(); ++it) {
+           grouped_sync_data.begin();
+       it != grouped_sync_data.end();
+       ++it) {
     GetOrCreateStorageWithSyncData(it->first, *it->second);
   }
 
   return syncer::SyncMergeResult(type);
 }
 
-syncer::SyncError SettingsBackend::ProcessSyncChanges(
+syncer::SyncError SyncStorageBackend::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& sync_changes) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -238,15 +250,18 @@ syncer::SyncError SettingsBackend::ProcessSyncChanges(
   // Group changes by extension, to pass all changes in a single method call.
   std::map<std::string, SettingSyncDataList> grouped_sync_data;
   for (syncer::SyncChangeList::const_iterator it = sync_changes.begin();
-      it != sync_changes.end(); ++it) {
+       it != sync_changes.end();
+       ++it) {
     SettingSyncData data(*it);
     grouped_sync_data[data.extension_id()].push_back(data);
   }
 
   // Create any storage areas that don't exist yet but have sync data.
   base::DictionaryValue empty;
-  for (std::map<std::string, SettingSyncDataList>::iterator
-      it = grouped_sync_data.begin(); it != grouped_sync_data.end(); ++it) {
+  for (std::map<std::string, SettingSyncDataList>::iterator it =
+           grouped_sync_data.begin();
+       it != grouped_sync_data.end();
+       ++it) {
     SyncableSettingsStorage* storage =
         GetOrCreateStorageWithSyncData(it->first, empty);
     syncer::SyncError error = storage->ProcessSyncChanges(it->second);
@@ -257,14 +272,14 @@ syncer::SyncError SettingsBackend::ProcessSyncChanges(
   return syncer::SyncError();
 }
 
-void SettingsBackend::StopSyncing(syncer::ModelType type) {
+void SyncStorageBackend::StopSyncing(syncer::ModelType type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(type == syncer::EXTENSION_SETTINGS ||
-         type == syncer::APP_SETTINGS);
+  DCHECK(type == syncer::EXTENSION_SETTINGS || type == syncer::APP_SETTINGS);
   DCHECK_EQ(sync_type_, type);
 
   for (StorageObjMap::iterator it = storage_objs_.begin();
-      it != storage_objs_.end(); ++it) {
+       it != storage_objs_.end();
+       ++it) {
     // Some storage areas may have already stopped syncing if they had areas
     // and syncing was disabled, but StopSyncing is safe to call multiple times.
     it->second->StopSyncing();
@@ -274,13 +289,12 @@ void SettingsBackend::StopSyncing(syncer::ModelType type) {
   sync_error_factory_.reset();
 }
 
-scoped_ptr<SettingsSyncProcessor> SettingsBackend::CreateSettingsSyncProcessor(
-    const std::string& extension_id) const {
+scoped_ptr<SettingsSyncProcessor>
+SyncStorageBackend::CreateSettingsSyncProcessor(const std::string& extension_id)
+    const {
   CHECK(sync_processor_.get());
-  return scoped_ptr<SettingsSyncProcessor>(
-      new SettingsSyncProcessor(extension_id,
-                                sync_type_,
-                                sync_processor_.get()));
+  return scoped_ptr<SettingsSyncProcessor>(new SettingsSyncProcessor(
+      extension_id, sync_type_, sync_processor_.get()));
 }
 
 }  // namespace extensions