Sync: Clear IS_UNSYNCED for deleted local items

sync/syncable/syncable_unittest.cc

Index: sync/syncable/syncable_unittest.cc
diff --git a/sync/syncable/syncable_unittest.cc b/sync/syncable/syncable_unittest.cc
index c4f87017cde2fd112ca0c69fe665c710c753ba56..d2ab5950f0c8daffff1e075d776002d6cdfc2b3f 100644
--- a/sync/syncable/syncable_unittest.cc
+++ b/sync/syncable/syncable_unittest.cc
@@ -20,6 +20,7 @@
 #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"
@@ -429,7 +430,8 @@ class SyncableDirectoryTest : public testing::Test {
   }
 
   virtual void TearDown() {
-    dir_->SaveChanges();
+    if (dir_.get())
+      dir_->SaveChanges();
     dir_.reset();
   }
 
@@ -508,6 +510,15 @@ class SyncableDirectoryTest : public testing::Test {
                      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) {
@@ -1154,6 +1165,105 @@ TEST_F(SyncableDirectoryTest, 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:
@@ -1536,6 +1646,32 @@ void SyncableDirectoryTest::ValidateEntry(BaseTransaction* trans,
   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 {