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

chrome/browser/value_store/leveldb_value_store_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/value_store/value_store_unittest.h"
 
+#include "base/file_util.h"
+#include "base/files/file_enumerator.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/memory/ref_counted.h"
+#include "base/message_loop/message_loop.h"
+#include "base/values.h"
 #include "chrome/browser/value_store/leveldb_value_store.h"
+#include "content/public/test/test_browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/leveldatabase/src/include/leveldb/db.h"
+#include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
 
 namespace {
 
 ValueStore* Param(const base::FilePath& file_path) {
   return new LeveldbValueStore(file_path);
 }
 
 }  // namespace
 
 INSTANTIATE_TEST_CASE_P(
     LeveldbValueStore,
     ValueStoreTest,
     testing::Values(&Param));

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

is there a test we can add to settings frontend too, to make sure this work
end-to-end?

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

Wow.  That was more work than I expected.  But I agree an end-to-end test is
useful.

Unfortunately, to test end-to-end, we need to go higher than SettingsFrontend,
since that really wouldn't do much more than test the GetStorage() function
(since all the real logic is in SettingsFunction).  So, made a
StorageApiUnittest, which hopefully does the trick.

+
+class LeveldbValueStoreUnitTest : public testing::Test {
+ public:
+  LeveldbValueStoreUnitTest()
+      : ui_thread_(content::BrowserThread::UI, base::MessageLoop::current()),
+        file_thread_(content::BrowserThread::FILE,
+                     base::MessageLoop::current()) {}
+  virtual ~LeveldbValueStoreUnitTest() {}
+
+ protected:
+  virtual void SetUp() OVERRIDE {
+    testing::Test::SetUp();

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

I don't think you're expected to do this.

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

We definitely need to for tests which inherit from anything other than
testing::Test (e.g. ExtensionUnittest), and and I think that we encourage it
just because if we ever, at some point, put something into
testing::Test::SetUp(), we don't have to then adjust several hundred tests.

[not at google - send to devlin, 2014/02/19 21:25:29]

Yes for all cases other than testing::Test you would need to do this. But if
gtest decided to add important logic into SetUp then (a) it would break lots of
things and (b) it wouldn't make a whole lot of sense, since it's the entity
which calls SetUp.

[Devlin, 2014/02/19 23:12:28]

Okay, done.

+
+    CHECK(database_dir_.CreateUniqueTempDir());
+    OpenStore();
+    CHECK(!store_->Get()->HasError());
+  }
+
+  virtual void TearDown() OVERRIDE {
+    store_->Clear();
+    store_.reset();
+  }
+
+  void CloseStore() { store_.reset(); }
+
+  void OpenStore() { store_.reset(new LeveldbValueStore(database_path())); }

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

these aren't accessors so make them proper functions (i.e. no inlining)

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

Don't we usually do this on tests?  Most I see (at least, those without a header
file) have the functions defined in-class... Should we not be doing this?

Unless you're referring to the formatting.  In which case I invite you to join
me in cursing the new "git cl format".

[not at google - send to devlin, 2014/02/19 21:25:29]

Yes I was referring to the formatting :)

+
+  LeveldbValueStore* store() { return store_.get(); }
+  const base::FilePath& database_path() { return database_dir_.path(); }
+
+ private:
+  scoped_ptr<LeveldbValueStore> store_;
+  base::ScopedTempDir database_dir_;
+
+  // Need these so that the DCHECKs for running on FILE or UI threads pass.

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

comment not necessary

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

Done.

+  base::MessageLoop message_loop_;
+  content::TestBrowserThread ui_thread_;
+  content::TestBrowserThread file_thread_;

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

can you just use a single content::TestBrowserThreadBundle?

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

Done.

+};
+
+// Check that we can restore a single corrupted key in the LeveldbValueStore.
+TEST_F(LeveldbValueStoreUnitTest, RestoreKeyTest) {
+  const char kNotCorruptKey[] = "not-corrupt";
+  const char kValue[] = "value";
+
+  // Insert a valid pair.
+  scoped_ptr<base::Value> value(base::Value::CreateStringValue(kValue));
+  ASSERT_FALSE(
+      store()->Set(ValueStore::DEFAULTS, kNotCorruptKey, *value)->HasError());
+
+  // Insert a corrupt pair.
+  const char kCorruptKey[] = "corrupt";
+  leveldb::WriteBatch batch;
+  batch.Put(kCorruptKey, "[{(.*+\"\'\\");
+  ASSERT_TRUE(store()->WriteToDbForTest(&batch));
+
+  // Verify corruption.
+  ValueStore::ReadResult result = store()->Get(kCorruptKey);
+  ASSERT_TRUE(result->HasError());
+  ASSERT_EQ(ValueStore::CORRUPTION, result->error().code);
+
+  // Restore and verify.
+  ASSERT_TRUE(store()->RestoreKey(kCorruptKey));
+  result = store()->Get(kCorruptKey);
+  EXPECT_FALSE(result->HasError());
+  EXPECT_TRUE(result->settings().empty());
+
+  // Verify that the valid pair is still present.
+  result = store()->Get(kNotCorruptKey);
+  EXPECT_FALSE(result->HasError());
+  EXPECT_TRUE(result->settings().HasKey(kNotCorruptKey));
+  std::string value_string;
+  EXPECT_TRUE(result->settings().GetString(kNotCorruptKey, &value_string));
+  EXPECT_EQ(kValue, value_string);
+}
+
+// Test that the Restore() method does not just delete the entire database
+// (unless absolutely necessary), and instead only removes corrupted keys.
+TEST_F(LeveldbValueStoreUnitTest, RestoreDoesMinimumNecessary) {
+  const char* kNotCorruptKeys[] = {"a", "n", "z"};
+  const size_t kNotCorruptKeysSize = 3u;
+  const char kCorruptKey1[] = "f";
+  const char kCorruptKey2[] = "s";
+  const char kValue[] = "value";
+  const char kCorruptValue[] = "[{(.*+\"\'\\";
+
+  // Insert a collection of non-corrupted pairs.
+  scoped_ptr<base::Value> value(base::Value::CreateStringValue(kValue));
+  for (size_t i = 0; i < kNotCorruptKeysSize; ++i) {
+    ASSERT_FALSE(store()
+                     ->Set(ValueStore::DEFAULTS, kNotCorruptKeys[i], *value)
+                     ->HasError());
+  }
+
+  // Insert a few corrupted pairs.
+  leveldb::WriteBatch batch;
+  batch.Put(kCorruptKey1, kCorruptValue);
+  batch.Put(kCorruptKey2, kCorruptValue);
+  ASSERT_TRUE(store()->WriteToDbForTest(&batch));
+
+  // Verify that we broke it, and then fix it.
+  ValueStore::ReadResult result = store()->Get();
+  ASSERT_TRUE(result->HasError());
+  ASSERT_EQ(ValueStore::CORRUPTION, result->error().code);
+
+  ASSERT_TRUE(store()->Restore());
+
+  // We should still have all valid pairs present in the database.
+  std::string value_string;
+  for (size_t i = 0; i < kNotCorruptKeysSize; ++i) {
+    result = store()->Get(kNotCorruptKeys[i]);
+    EXPECT_FALSE(result->HasError());
+    EXPECT_TRUE(result->settings().HasKey(kNotCorruptKeys[i]));
+    EXPECT_TRUE(
+        result->settings().GetString(kNotCorruptKeys[i], &value_string));
+    EXPECT_EQ(kValue, value_string);
+  }
+}
+
+// Test that the LeveldbValueStore can recover in the case of a CATastrophic
+// failure and we have total corruption. In this case, the database is plagued
+// by LolCats.
+// Full corruption has been known to happen occasionally in strange edge cases,
+// such as after users use Windows Restore. We can't prevent it, but we need to
+// be able to handle it smoothly.
+TEST_F(LeveldbValueStoreUnitTest, RestoreFullDatabase) {
+  const std::string kLolCats("I can haz leveldb filez?");
+  const char* kNotCorruptKeys[] = {"a", "n", "z"};
+  const size_t kNotCorruptKeysSize = 3u;
+  const char kValue[] = "value";
+
+  // Generate a database.
+  scoped_ptr<base::Value> value(base::Value::CreateStringValue(kValue));
+  for (size_t i = 0; i < kNotCorruptKeysSize; ++i) {
+    ASSERT_FALSE(store()
+                     ->Set(ValueStore::DEFAULTS, kNotCorruptKeys[i], *value)
+                     ->HasError());
+  }
+
+  // Close it (so we remove the lock), and replace all files with LolCats.
+  CloseStore();
+  base::FileEnumerator enumerator(
+      database_path(), true /* recursive */, base::FileEnumerator::FILES);
+  for (base::FilePath file = enumerator.Next(); !file.empty();
+       file = enumerator.Next()) {
+    // WriteFile() failure is a result of -1.
+    ASSERT_NE(file_util::WriteFile(file, kLolCats.c_str(), kLolCats.length()),
+              -1);
+  }
+  OpenStore();
+
+  // We should definitely have an error.
+  ValueStore::ReadResult result = store()->Get();
+  ASSERT_TRUE(result->HasError());
+  ASSERT_EQ(ValueStore::CORRUPTION, result->error().code);
+
+  ASSERT_TRUE(store()->Restore());
+  result = store()->Get();
+  EXPECT_FALSE(result->HasError());
+  // We couldn't recover anything, but we should be in a sane state again.
+  EXPECT_EQ(0u, result->settings().size());
+}