Sync: Clear IS_UNSYNCED for deleted local items

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 "sync/syncable/syncable.h"
 
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
 #include "base/stringprintf.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/test/values_test_util.h"
 #include "base/threading/platform_thread.h"
 #include "base/values.h"
 #include "sync/engine/syncproto.h"
+#include "sync/engine/syncer_util.h"

[rlarocque, 2012/05/11 22:41:26]

I'm not sure if it's OK to test syncer_util functions from this file.  Thoughts?

 #include "sync/util/test_unrecoverable_error_handler.h"
 #include "sync/syncable/directory_backing_store.h"
 #include "sync/syncable/directory_change_delegate.h"
 #include "sync/syncable/on_disk_directory_backing_store.h"
 #include "sync/test/engine/test_id_factory.h"
 #include "sync/test/engine/test_syncable_utils.h"
 #include "sync/test/fake_encryptor.h"
 #include "sync/test/null_directory_change_delegate.h"
 #include "sync/test/null_transaction_observer.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
@@ skipping 389 matching lines @@
 
   virtual void SetUp() {
     dir_.reset(new Directory(&encryptor_, &handler_, NULL));
     ASSERT_TRUE(dir_.get());
     ASSERT_EQ(OPENED, dir_->OpenInMemoryForTest(kName, &delegate_,
                                                 NullTransactionObserver()));
     ASSERT_TRUE(dir_->good());
   }
 
   virtual void TearDown() {
-    dir_->SaveChanges();
+    if (dir_.get())
+      dir_->SaveChanges();
     dir_.reset();
   }
 
   void GetAllMetaHandles(BaseTransaction* trans, MetahandleSet* result) {
     dir_->GetAllMetaHandles(trans, result);
   }
 
   bool IsInDirtyMetahandles(int64 metahandle) {
     return 1 == dir_->kernel_->dirty_metahandles->count(metahandle);
   }
@@ skipping 58 matching lines @@
     me.Put(IS_UNSYNCED, true);
   }
 
   void ValidateEntry(BaseTransaction* trans,
                      int64 id,
                      bool check_name,
                      const std::string& name,
                      int64 base_version,
                      int64 server_version,
                      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();
 };
 
 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 626 matching lines @@
 
     browser_sync::SyncEntity item_entity;
     item_entity.set_id(id_factory.NewServerId());
     item_entity.set_deleted(false);
     item_entity.set_folder(false);
     item_entity.mutable_specifics()->CopyFrom(specifics);
     ASSERT_EQ(datatype, item_entity.GetModelType());
   }
 }
 
+// A test that roughly mimics the directory interaction that occurs when a
+// bookmark folder and entry are created then synced for the first time.  It is
+// a more common variant of the 'DeletedAndUnsyncedChild' scenario tested below.

[Nicolas Zea, 2012/05/11 23:21:25]

DeleteAndUnsyncedChild -> DeleteAnUnsyncedChild? (here and below)

[rlarocque, 2012/05/15 00:41:55]

It's DeletedAndUnsyncedChild.

+TEST_F(SyncableDirectoryTest, ChangeEntryIDAndUpdateChildren_ParentAndChild) {
+  TestIdFactory id_factory;
+  Id orig_parent_id;
+  Id orig_child_id;
+
+  {
+    // Create two client-side items, a parent and child.
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
+
+    MutableEntry parent(&trans, CREATE, id_factory.root(), "parent");
+    parent.Put(IS_DIR, true);
+    parent.Put(IS_UNSYNCED, true);
+
+    MutableEntry child(&trans, CREATE, parent.Get(ID), "child");
+    child.Put(IS_UNSYNCED, true);
+
+    orig_parent_id = parent.Get(ID);
+    orig_child_id = child.Get(ID);
+  }
+
+  {
+    // Simulate what happens after committing two items.  Their IDs will be
+    // replaced with server IDs.  The child is renamed first, then the parent.
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
+
+    MutableEntry parent(&trans, GET_BY_ID, orig_parent_id);
+    MutableEntry child(&trans, GET_BY_ID, orig_child_id);
+
+    browser_sync::SyncerUtil::ChangeEntryIDAndUpdateChildren(
+        &trans, &child, id_factory.NewServerId());
+    child.Put(IS_UNSYNCED, false);
+    child.Put(BASE_VERSION, 1);
+    child.Put(SERVER_VERSION, 1);
+
+    browser_sync::SyncerUtil::ChangeEntryIDAndUpdateChildren(
+        &trans, &parent, id_factory.NewServerId());
+    parent.Put(IS_UNSYNCED, false);
+    parent.Put(BASE_VERSION, 1);
+    parent.Put(SERVER_VERSION, 1);
+  }
+
+  // Final check of validity.

[Nicolas Zea, 2012/05/11 23:21:25]

of -> for

[rlarocque, 2012/05/15 00:41:55]

Done.

+  EXPECT_EQ(OPENED, SimulateSaveAndReloadDir());
+}
+
+// A test based on the scenario where we create a bookmark folder and entry
+// locally, but with a twist.  In this case, the bookmark is deleted before we
+// are abot to sync either it or its parent folder.  This scenario used to cause

[Nicolas Zea, 2012/05/11 23:21:25]

abot -> able

[rlarocque, 2012/05/15 00:41:55]

Done.

+// directory corruption, see crbug.com/125381.
+TEST_F(SyncableDirectoryTest,
+       ChangeEntryIDAndUpdateChildren_DeletedAndUnsyncedChild) {
+  TestIdFactory id_factory;
+  Id orig_parent_id;
+  Id orig_child_id;
+
+  {
+    // Create two client-side items, a parent and child.
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
+
+    MutableEntry parent(&trans, CREATE, id_factory.root(), "parent");
+    parent.Put(IS_DIR, true);
+    parent.Put(IS_UNSYNCED, true);
+
+    MutableEntry child(&trans, CREATE, parent.Get(ID), "child");
+    child.Put(IS_UNSYNCED, true);
+
+    orig_parent_id = parent.Get(ID);
+    orig_child_id = child.Get(ID);
+  }
+
+  {
+    // Delete the child.
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
+
+    MutableEntry child(&trans, GET_BY_ID, orig_child_id);
+    child.Put(IS_DEL, true);
+  }
+
+  {
+    // Simulate what happens after committing the parent.  Its ID will be
+    // replaced with server a ID.
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get());
+
+    MutableEntry parent(&trans, GET_BY_ID, orig_parent_id);
+
+    browser_sync::SyncerUtil::ChangeEntryIDAndUpdateChildren(
+        &trans, &parent, id_factory.NewServerId());
+    parent.Put(IS_UNSYNCED, false);
+    parent.Put(BASE_VERSION, 1);
+    parent.Put(SERVER_VERSION, 1);
+  }
+
+  // Final check of validity.

[Nicolas Zea, 2012/05/11 23:21:25]

of -> for

[rlarocque, 2012/05/15 00:41:55]

Done.

[rlarocque, 2012/05/15 00:41:55]

Done.

+  EXPECT_EQ(OPENED, SimulateSaveAndReloadDir());
+}
+
 // 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);
@@ skipping 362 matching lines @@
                                           bool is_del) {
   Entry e(trans, GET_BY_ID, TestIdFactory::FromNumber(id));
   ASSERT_TRUE(e.good());
   if (check_name)
     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;
+
+  // Do some tricky things to preserve the backing store.
+  DirectoryBackingStore* saved_store = dir_->store_;
+  dir_->store_ = NULL;
+
+  // Close the current directory.
+  dir_->Close();
+  dir_.reset();
+
+  dir_.reset(new Directory(&encryptor_, &handler_, NULL));
+  if (!dir_.get())
+    return FAILED_IN_UNITTEST;
+  DirOpenResult result = dir_->OpenImpl(saved_store, kName, &delegate_,
+                                        NullTransactionObserver());
+
+  // If something went wrong, we to clear this member.  If we don't, TearDown()

[Nicolas Zea, 2012/05/11 23:21:25]

we to -> we need to

[rlarocque, 2012/05/15 00:41:55]

Done.

+  // will be guaranteed to crash when it calls SaveChanges().
+  if (result != OPENED)
+    dir_.reset();
+
+  return result;
+}
+
 namespace {
 
 class SyncableDirectoryManagement : public testing::Test {
  public:
   virtual void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   }
 
   virtual void TearDown() {
   }
@@ skipping 183 matching lines @@
 TEST_F(SyncableClientTagTest, TestClientTagIndexDuplicateServer) {
   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