Update next_id when loading sync DB

sync/syncable/syncable_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 <string>
 
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/location.h"
@@ skipping 374 matching lines @@
     ExpectDictBooleanValue(true, *value, "good");
     EXPECT_TRUE(value->HasKey("kernel"));
     ExpectDictStringValue("Unspecified", *value, "modelType");
     ExpectDictBooleanValue(true, *value, "existsOnClientBecauseNameIsNonEmpty");
     ExpectDictBooleanValue(false, *value, "isRoot");
   }
 
   dir.SaveChanges();
 }
 
-// A Directory whose backing store always fails SaveChanges by returning false.
-class TestUnsaveableDirectory : public Directory {
- public:
-  class UnsaveableBackingStore : public OnDiskDirectoryBackingStore {
-   public:
-     UnsaveableBackingStore(const std::string& dir_name,
-                            const FilePath& backing_filepath)
-         : OnDiskDirectoryBackingStore(dir_name, backing_filepath) { }
-     virtual bool SaveChanges(const Directory::SaveChangesSnapshot& snapshot) {
-       return false;
-     }
-  };
-
-  TestUnsaveableDirectory(const std::string& dir_name,
-                          const FilePath& backing_filepath)
-      : Directory(&encryptor_, &handler_, NULL,
-                  new UnsaveableBackingStore(dir_name, backing_filepath)) {}
- private:
-  FakeEncryptor encryptor_;
-  TestUnrecoverableErrorHandler handler_;
-};
-
 // A test fixture for syncable::Directory.  Uses an in-memory database to keep
 // the unit tests fast.
 class SyncableDirectoryTest : public testing::Test {
  protected:
   MessageLoop message_loop_;
   static const char kName[];
 
   virtual void SetUp() {
     dir_.reset(new Directory(&encryptor_, &handler_, NULL,
                              new InMemoryDirectoryBackingStore(kName)));
@@ skipping 86 matching lines @@
                      bool is_del);
 
   // When a directory is saved then loaded from disk, it will pass through
   // DropDeletedEntries().  This will remove some entries from the directory.
   // This function is intended to simulate that process.
   //
   // WARNING: The directory will be deleted by this operation.  You should
   // not have any pointers to the directory (open transactions included)
   // when you call this.
   DirOpenResult SimulateSaveAndReloadDir();
+
+  // This function will close and re-open the directory without saving any
+  // pending changes.  This is intended to simulate the recovery from a crash
+  // scenario.  The same warnings for SimulateSaveAndReloadDir apply here.
+  DirOpenResult SimulateCrashAndReloadDir();
+
+ private:
+  // A helper function for Simulate{Save,Crash}AndReloadDir.
+  DirOpenResult ReloadDirImpl();
 };
 
 TEST_F(SyncableDirectoryTest, TakeSnapshotGetsMetahandlesToPurge) {
   const int metas_to_create = 50;
   MetahandleSet expected_purges;
   MetahandleSet all_handles;
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
     for (int i = 0; i < metas_to_create; i++) {
       MutableEntry e(&trans, CREATE, trans.root_id(), "foo");
@@ skipping 721 matching lines @@
     ChangeEntryIDAndUpdateChildren(&trans, &parent, id_factory.NewServerId());
     parent.Put(IS_UNSYNCED, false);
     parent.Put(BASE_VERSION, 1);
     parent.Put(SERVER_VERSION, 1);
   }
 
   // Final check for validity.
   EXPECT_EQ(OPENED, SimulateSaveAndReloadDir());
 }
 
+// Ask the directory to generate a unique ID.  Close and re-open the database
+// without saving, then ask for another unique ID.  Verify IDs are not reused.
+// This scenario simulates a crash within the first few seconds of operation.
+TEST_F(SyncableDirectoryTest, LocalIdReuseTest) {
+  Id pre_crash_id = dir_->NextId();
+  SimulateCrashAndReloadDir();
+  Id post_crash_id = dir_->NextId();
+  EXPECT_NE(pre_crash_id, post_crash_id);
+}
+
+// Ask the directory to generate a unique ID.  Save the directory.  Close and
+// re-open the database without saving, then ask for another unique ID.  Verify
+// IDs are not reused.  This scenario simulates a steady-state crash.
+TEST_F(SyncableDirectoryTest, LocalIdReuseTestWithSave) {

[Nicolas Zea, 2012/08/21 00:26:35]

is this really testing anything different than the above?

[rlarocque, 2012/08/21 00:54:35]

They're slightly different.  This bug only affected clients that crashed before
the first call to SaveChanges().  Before this fix, LocalIdReuseTest would have
failed but LocalIdReuseTestWithSave would not.

[Nicolas Zea, 2012/08/21 01:08:56]

I was thinking that if LocalIdReuseTest succeeds, then this one basically has to
succeed. But I guess that's not true the other way around, so it still has some
value.

+  Id pre_crash_id = dir_->NextId();
+  dir_->SaveChanges();
+  SimulateCrashAndReloadDir();
+  Id post_crash_id = dir_->NextId();
+  EXPECT_NE(pre_crash_id, post_crash_id);
+}
+
 // Ensure that the unsynced, is_del and server unkown entries that may have been
 // left in the database by old clients will be deleted when we open the old
 // database.
 TEST_F(SyncableDirectoryTest, OldClientLeftUnsyncedDeletedLocalItem) {
   // We must create an entry with the offending properties.  This is done with
   // some abuse of the MutableEntry's API; it doesn't expect us to modify an
   // item after it is deleted.  If this hack becomes impractical we will need to
   // find a new way to simulate this scenario.
 
   TestIdFactory id_factory;
@@ skipping 52 matching lines @@
     EXPECT_TRUE(server_knows.good());
 
     Entry not_is_del(&trans, GET_BY_ID, not_is_del_id);
     EXPECT_TRUE(not_is_del.good());
 
     Entry zombie(&trans, GET_BY_ID, zombie_id);
     EXPECT_FALSE(zombie.good());
   }
 }
 
+// An OnDirectoryBackingStore that can be set to always fail SaveChanges.
+class TestBackingStore : public OnDiskDirectoryBackingStore {
+ public:
+  TestBackingStore(const std::string& dir_name,

[Nicolas Zea, 2012/08/21 00:26:35]

virtual destructor

[rlarocque, 2012/08/21 00:54:35]

Done.

+                   const FilePath& backing_filepath);
+
+  virtual bool SaveChanges(const Directory::SaveChangesSnapshot& snapshot)
+      OVERRIDE;
+
+   void StartFailingSaveChanges() {
+     fail_save_changes_ = true;
+   }
+
+   bool fail_save_changes_;

[Nicolas Zea, 2012/08/21 00:26:35]

make private

[rlarocque, 2012/08/21 00:54:35]

Done.

+};
+
+TestBackingStore::TestBackingStore(const std::string& dir_name,
+                                   const FilePath& backing_filepath)
+  : OnDiskDirectoryBackingStore(dir_name, backing_filepath),
+    fail_save_changes_(false) {
+}
+
+bool TestBackingStore::SaveChanges(
+    const Directory::SaveChangesSnapshot& snapshot){
+  if (fail_save_changes_) {
+    return false;
+  } else {
+    return OnDiskDirectoryBackingStore::SaveChanges(snapshot);
+  }
+}
+
+// A directory whose Save() function can be set to always fail.
+class TestDirectory : public Directory {
+ public:

[Nicolas Zea, 2012/08/21 00:26:35]

virtual destructor

[rlarocque, 2012/08/21 00:54:35]

Done.

+  // A factory function used to work around some initialization order issues.
+  static TestDirectory* Create(
+      Encryptor *encryptor,
+      UnrecoverableErrorHandler *handler,
+      const std::string& dir_name,
+      const FilePath& backing_filepath);
+
+  void StartFailingSaveChanges() {
+    backing_store_->StartFailingSaveChanges();
+  }
+
+ private:
+  TestDirectory(Encryptor* encryptor,
+                UnrecoverableErrorHandler* handler,
+                TestBackingStore* backing_store);
+
+  TestBackingStore* backing_store_;
+};
+
+TestDirectory* TestDirectory::Create(
+    Encryptor *encryptor,
+    UnrecoverableErrorHandler *handler,
+    const std::string& dir_name,
+    const FilePath& backing_filepath) {
+  TestBackingStore* backing_store =
+      new TestBackingStore(dir_name, backing_filepath);
+  return new TestDirectory(encryptor, handler, backing_store);
+}
+
+TestDirectory::TestDirectory(Encryptor* encryptor,
+                             UnrecoverableErrorHandler* handler,
+                             TestBackingStore* backing_store)
+    : Directory(encryptor, handler, NULL, backing_store),
+      backing_store_(backing_store) {
+}
+
+TEST(OnDiskSyncableDirectory, FailInitialWrite) {
+  FakeEncryptor encryptor;
+  TestUnrecoverableErrorHandler handler;
+  ScopedTempDir temp_dir;
+  FilePath file_path = temp_dir.path().Append("Test.sqlite3");
+  std::string name = "user@x.com";
+  NullDirectoryChangeDelegate delegate;
+
+  scoped_ptr<TestDirectory> test_dir(
+      TestDirectory::Create(&encryptor, &handler, name, file_path));
+
+  test_dir->StartFailingSaveChanges();
+  ASSERT_EQ(FAILED_INITIAL_WRITE, test_dir->Open(name, &delegate,
+                                                 NullTransactionObserver()));
+}
+
 // A variant of SyncableDirectoryTest that uses a real sqlite database.
 class OnDiskSyncableDirectoryTest : public SyncableDirectoryTest {
  protected:
   // SetUp() is called before each test case is run.
   // The sqlite3 DB is deleted before each test is run.
   virtual void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     file_path_ = temp_dir_.path().Append(
         FILE_PATH_LITERAL("Test.sqlite3"));
     file_util::Delete(file_path_, true);
-    dir_.reset(new Directory(&encryptor_, &handler_, NULL,
-        new OnDiskDirectoryBackingStore(kName, file_path_)));
+    test_directory_ =

[Nicolas Zea, 2012/08/21 00:26:35]

reuse the ReloadDir method? dir_->good should probably be in ReloadDir anyways

[rlarocque, 2012/08/21 00:54:35]

Done.

+        TestDirectory::Create(&encryptor_, &handler_, kName, file_path_);
+    dir_.reset(test_directory_);
     ASSERT_TRUE(dir_.get());
     ASSERT_EQ(OPENED, dir_->Open(kName, &delegate_,
                                  NullTransactionObserver()));
     ASSERT_TRUE(dir_->good());
   }
 
   virtual void TearDown() {
     // This also closes file handles.
     dir_->SaveChanges();
     dir_.reset();
     file_util::Delete(file_path_, true);
   }
 
   void ReloadDir() {
-    dir_.reset(new Directory(&encryptor_, &handler_, NULL,
-        new OnDiskDirectoryBackingStore(kName, file_path_)));
+    test_directory_ =
+        TestDirectory::Create(&encryptor_, &handler_, kName, file_path_);
+    dir_.reset(test_directory_);
     ASSERT_TRUE(dir_.get());
     ASSERT_EQ(OPENED, dir_->Open(kName, &delegate_,
                                  NullTransactionObserver()));
   }
 
   void SaveAndReloadDir() {
     dir_->SaveChanges();
     ReloadDir();
   }
 
-  void SwapInUnsaveableDirectory() {
-    dir_.reset(); // Delete the old directory.
-
-    // We first assign the object to a pointer of type TestUnsaveableDirectory
-    // because the OpenUnsaveable function is not available in the parent class.
-    scoped_ptr<TestUnsaveableDirectory> dir(new TestUnsaveableDirectory(
-        kName, file_path_));
-    ASSERT_TRUE(dir.get());
-    ASSERT_EQ(OPENED, dir->Open(kName, &delegate_, NullTransactionObserver()));
-
-    // Finally, move the unsaveable directory to the dir_ variable.
-    dir_ = dir.Pass();
+  void StartFailingSaveChanges() {
+    test_directory_->StartFailingSaveChanges();
   }
 
+  TestDirectory *test_directory_;  // mirrors scoped_ptr<Directory> dir_
   ScopedTempDir temp_dir_;
   FilePath file_path_;
 };
 
 TEST_F(OnDiskSyncableDirectoryTest, TestPurgeEntriesWithTypeIn) {
   sync_pb::EntitySpecifics bookmark_specs;
   sync_pb::EntitySpecifics autofill_specs;
   sync_pb::EntitySpecifics preference_specs;
   AddDefaultFieldValue(BOOKMARKS, &bookmark_specs);
   AddDefaultFieldValue(PREFERENCES, &preference_specs);
@@ skipping 210 matching lines @@
     MutableEntry aguilera(&trans, GET_BY_HANDLE, handle1);
     ASSERT_TRUE(aguilera.good());
     EXPECT_FALSE(aguilera.GetKernelCopy().is_dirty());
     EXPECT_EQ(aguilera.Get(NON_UNIQUE_NAME), "aguilera");
     aguilera.Put(NON_UNIQUE_NAME, "overwritten");
     EXPECT_TRUE(aguilera.GetKernelCopy().is_dirty());
     EXPECT_TRUE(IsInDirtyMetahandles(handle1));
   }
   ASSERT_TRUE(dir_->SaveChanges());
 
-  // Now do some operations using a directory for which SaveChanges will
-  // always fail.
-  SwapInUnsaveableDirectory();
+  // Now do some operations when SaveChanges() will fail.
+  StartFailingSaveChanges();
   ASSERT_TRUE(dir_->good());
 
   int64 handle2 = 0;
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
 
     MutableEntry aguilera(&trans, GET_BY_HANDLE, handle1);
     ASSERT_TRUE(aguilera.good());
     EXPECT_FALSE(aguilera.GetKernelCopy().is_dirty());
     EXPECT_EQ(aguilera.Get(NON_UNIQUE_NAME), "overwritten");
@@ skipping 49 matching lines @@
     sync_pb::EntitySpecifics bookmark_specs;
     AddDefaultFieldValue(BOOKMARKS, &bookmark_specs);
     e1.Put(SPECIFICS, bookmark_specs);
     e1.Put(SERVER_SPECIFICS, bookmark_specs);
     e1.Put(ID, TestIdFactory::FromNumber(101));
     EXPECT_TRUE(e1.GetKernelCopy().is_dirty());
     EXPECT_TRUE(IsInDirtyMetahandles(handle1));
   }
   ASSERT_TRUE(dir_->SaveChanges());
 
-  // Now do some operations using a directory for which SaveChanges will
-  // always fail.
-  SwapInUnsaveableDirectory();
+  // Now do some operations while SaveChanges() is set to fail.
+  StartFailingSaveChanges();
   ASSERT_TRUE(dir_->good());
 
   ModelTypeSet set(BOOKMARKS);
   dir_->PurgeEntriesWithTypeIn(set);
   EXPECT_TRUE(IsInMetahandlesToPurge(handle1));
   ASSERT_FALSE(dir_->SaveChanges());
   EXPECT_TRUE(IsInMetahandlesToPurge(handle1));
 }
 
 }  // namespace
@@ skipping 11 matching lines @@
     ASSERT_TRUE(name == e.Get(NON_UNIQUE_NAME));
   ASSERT_TRUE(base_version == e.Get(BASE_VERSION));
   ASSERT_TRUE(server_version == e.Get(SERVER_VERSION));
   ASSERT_TRUE(is_del == e.Get(IS_DEL));
 }
 
 DirOpenResult SyncableDirectoryTest::SimulateSaveAndReloadDir() {
   if (!dir_->SaveChanges())
     return FAILED_IN_UNITTEST;
 
+  return ReloadDirImpl();
+}
+
+DirOpenResult SyncableDirectoryTest::SimulateCrashAndReloadDir() {
+  return ReloadDirImpl();
+}
+
+DirOpenResult SyncableDirectoryTest::ReloadDirImpl() {
   // Do some tricky things to preserve the backing store.
   DirectoryBackingStore* saved_store = dir_->store_.release();
 
   // Close the current directory.
   dir_->Close();
   dir_.reset();
 
   dir_.reset(new Directory(&encryptor_, &handler_, NULL, saved_store));
   DirOpenResult result = dir_->OpenImpl(kName, &delegate_,
                                         NullTransactionObserver());
@@ skipping 206 matching lines @@
   EXPECT_TRUE(CreateWithDefaultTag(factory_.NewServerId(), true));
   EXPECT_FALSE(CreateWithDefaultTag(factory_.NewServerId(), true));
   EXPECT_FALSE(CreateWithDefaultTag(factory_.NewServerId(), false));
   EXPECT_FALSE(CreateWithDefaultTag(factory_.NewLocalId(), false));
   EXPECT_FALSE(CreateWithDefaultTag(factory_.NewLocalId(), true));
 }
 
 }  // namespace
 }  // namespace syncable
 }  // namespace syncer