r137476 - in trunk/src/sync: engine syncable

[rlar...@chromium.org]

Author: rlar...@chromium.org
Date: Wed May 16 12:09:47 2012
New Revision: 137476

Log:
Sync: Clear IS_UNSYNCED for deleted local items

Prior to this change, if we were to delete a newly-created item before
informing the sync server of its existence, it would end up in an
unusual state. We do not commit deleted items unless they were known to
sync server. However, these items were still considered to be
'unsynced' (ie. waiting to be committed to the server) and therefore
impossible to entirely remove from the sync directory. They remain
forever both IS_DEL and IS_UNSYNCED.

This had a few side-effects. The most serious is that this behaviour
could cause directory corruption. If we created a folder and a bookmark
within it, then deleted that bookmark before it had a chance to sync,
the bookmark would remain in a zombie state. When we finally got around
to committing its parent folder, the folder's ID will change. The
zombie bookmark's PARENT_ID will not be updated, leaving us with a
dangling reference that will be detected as directory corruption the
next time sync is loaded. See crbug.com/125381 for details.

Another effect is that locally deleted, never synced UNIQUE_CLIENT_TAG
could have strange effects later on. If a foreign client were to create
an item with the same UNIQUE_CLIENT_TAG and sync it, this client would
conflict-resolve the update against the zombie entry. The zombie entry
would win, the newly created item would be deleted, and the deletion
synced back to the server. This may not be an entirely bad thing, but
it is a behaviour that's changed following this patch. From now on, the
deleted item will be overwritten (if it still exists, which it probably
won't).

Finally, we now have a mechanism for permanently deleting these items.
With this patch applied, these items will no longer have the unsynced
bit set, so they will be deleted on restart by DropDeletedEntries().

This patch changes PutIsDel() to clear the IS_UNSYNCED bit of entries
which were never committed to the sync server. The remainder of the
changes either add or update existing tests.

BUG=125381
TEST=SyncerTest.ClientTagUncommittedTagMatchesUpdate,
SyncableDirectoryTest.ChangeEntryIDAndUpdateChildren_DeletedAndUnsyncedChildren


Review URL: https://chromiumcodereview.appspot.com/10389103

Modified:
trunk/src/sync/engine/get_commit_ids_command.cc
trunk/src/sync/engine/process_commit_response_command.cc
trunk/src/sync/engine/process_updates_command.cc
trunk/src/sync/engine/syncer_unittest.cc
trunk/src/sync/engine/syncer_util.cc
trunk/src/sync/engine/syncer_util.h
trunk/src/sync/syncable/dir_open_result.h
trunk/src/sync/syncable/in_memory_directory_backing_store.cc
trunk/src/sync/syncable/syncable.cc
trunk/src/sync/syncable/syncable.h
trunk/src/sync/syncable/syncable_unittest.cc

Modified: trunk/src/sync/engine/get_commit_ids_command.cc
==============================================================================
--- trunk/src/sync/engine/get_commit_ids_command.cc (original)
+++ trunk/src/sync/engine/get_commit_ids_command.cc Wed May 16 12:09:47 2012
@@ -127,14 +127,6 @@
if (throttled_types.Has(type))
return false;

- // Drop deleted uncommitted entries.
- if (entry.Get(syncable::IS_DEL) && !entry.Get(syncable::ID).ServerKnows()) {
- // TODO(zea): These will remain unsynced indefinitely. This is harmless,
- // but we should clean them up somewhere.
- DVLOG(1) << "Ignoring deleted and uncommitted item." << entry;
- return false;
- }
-
// Extra validity checks.
syncable::Id id = entry.Get(syncable::ID);
if (id == entry.Get(syncable::PARENT_ID)) {

Modified: trunk/src/sync/engine/process_commit_response_command.cc
==============================================================================
--- trunk/src/sync/engine/process_commit_response_command.cc (original)
+++ trunk/src/sync/engine/process_commit_response_command.cc Wed May 16 12:09:47 2012
@@ -336,8 +336,7 @@
<< " during commit " << same_id;
return false;
}
- SyncerUtil::ChangeEntryIDAndUpdateChildren(
- trans, local_entry, entry_response.id());
+ ChangeEntryIDAndUpdateChildren(trans, local_entry, entry_response.id());
DVLOG(1) << "Changing ID to " << entry_response.id();
}
return true;

Modified: trunk/src/sync/engine/process_updates_command.cc
==============================================================================
--- trunk/src/sync/engine/process_updates_command.cc (original)
+++ trunk/src/sync/engine/process_updates_command.cc Wed May 16 12:09:47 2012
@@ -120,8 +120,7 @@
// change the ID now, after we're sure that the update can succeed.
if (local_id != server_id) {
DCHECK(!update.deleted());
- SyncerUtil::ChangeEntryIDAndUpdateChildren(trans, &target_entry,
- server_id);
+ ChangeEntryIDAndUpdateChildren(trans, &target_entry, server_id);
// When IDs change, versions become irrelevant. Forcing BASE_VERSION
// to zero would ensure that this update gets applied, but would indicate
// creation or undeletion if it were committed that way. Instead, prefer

Modified: trunk/src/sync/engine/syncer_unittest.cc
==============================================================================
--- trunk/src/sync/engine/syncer_unittest.cc (original)
+++ trunk/src/sync/engine/syncer_unittest.cc Wed May 16 12:09:47 2012
@@ -1450,28 +1450,28 @@
MutableEntry parent(&wtrans, syncable::CREATE, wtrans.root_id(),
"Pete");
ASSERT_TRUE(parent.good());
+ parent.Put(syncable::ID, ids_.FromNumber(103));
parent.Put(syncable::IS_UNSYNCED, true);
parent.Put(syncable::IS_DIR, true);
parent.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
parent.Put(syncable::IS_DEL, true);
- parent.Put(syncable::ID, ids_.FromNumber(103));
parent.Put(syncable::BASE_VERSION, 1);
parent.Put(syncable::MTIME, now_minus_2h);
MutableEntry child(&wtrans, syncable::CREATE, ids_.FromNumber(103),
"Pete");
ASSERT_TRUE(child.good());
+ child.Put(syncable::ID, ids_.FromNumber(104));
child.Put(syncable::IS_UNSYNCED, true);
child.Put(syncable::IS_DIR, true);
child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
child.Put(syncable::IS_DEL, true);
- child.Put(syncable::ID, ids_.FromNumber(104));
child.Put(syncable::BASE_VERSION, 1);
child.Put(syncable::MTIME, now_minus_2h);
MutableEntry grandchild(&wtrans, syncable::CREATE, ids_.FromNumber(104),
"Pete");
ASSERT_TRUE(grandchild.good());
- grandchild.Put(syncable::IS_UNSYNCED, true);
grandchild.Put(syncable::ID, ids_.FromNumber(105));
+ grandchild.Put(syncable::IS_UNSYNCED, true);
grandchild.Put(syncable::IS_DEL, true);
grandchild.Put(syncable::IS_DIR, false);
grandchild.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
@@ -3709,6 +3709,7 @@

TEST_F(SyncerTest, ClientTagConflictWithDeletedLocalEntry) {
{
+ // Create a deleted local entry with a unique client tag.
WriteTransaction trans(FROM_HERE, UNITTEST, directory());
MutableEntry perm_folder(&trans, CREATE, ids_.root(), "clientname");
ASSERT_TRUE(perm_folder.good());
@@ -3716,24 +3717,28 @@
perm_folder.Put(UNIQUE_CLIENT_TAG, "clientperm");
perm_folder.Put(SPECIFICS, DefaultBookmarkSpecifics());
perm_folder.Put(IS_UNSYNCED, true);
+
+ // Note: IS_DEL && !ServerKnows() will clear the UNSYNCED bit.
+ // (We never attempt to commit server-unknown deleted items, so this
+ // helps us clean up those entries).
perm_folder.Put(IS_DEL, true);
}

+ // Prepare an update with the same unique client tag.
mock_server_->AddUpdateDirectory(1, 0, "permitem_renamed", 10, 100);
mock_server_->SetLastUpdateClientTag("clientperm");
- mock_server_->set_conflict_all_commits(true);

SyncShareNudge();
- // This should cause client tag overwrite.
+ // The local entry will be overwritten.
{
ReadTransaction trans(FROM_HERE, directory());

Entry perm_folder(&trans, GET_BY_CLIENT_TAG, "clientperm");
ASSERT_TRUE(perm_folder.good());
ASSERT_TRUE(perm_folder.Get(ID).ServerKnows());
- EXPECT_TRUE(perm_folder.Get(IS_DEL));
+ EXPECT_FALSE(perm_folder.Get(IS_DEL));
EXPECT_FALSE(perm_folder.Get(IS_UNAPPLIED_UPDATE));
- EXPECT_TRUE(perm_folder.Get(IS_UNSYNCED));
+ EXPECT_FALSE(perm_folder.Get(IS_UNSYNCED));
EXPECT_EQ(perm_folder.Get(BASE_VERSION), 10);
EXPECT_EQ(perm_folder.Get(UNIQUE_CLIENT_TAG), "clientperm");
}

Modified: trunk/src/sync/engine/syncer_util.cc
==============================================================================
--- trunk/src/sync/engine/syncer_util.cc (original)
+++ trunk/src/sync/engine/syncer_util.cc Wed May 16 12:09:47 2012
@@ -81,59 +81,6 @@
}

// static
-void SyncerUtil::ChangeEntryIDAndUpdateChildren(
- syncable::WriteTransaction* trans,
- syncable::MutableEntry* entry,
- const syncable::Id& new_id,
- syncable::Directory::ChildHandles* children) {
- syncable::Id old_id = entry->Get(ID);
- if (!entry->Put(ID, new_id)) {
- Entry old_entry(trans, GET_BY_ID, new_id);
- CHECK(old_entry.good());
- LOG(FATAL) << "Attempt to change ID to " << new_id
- << " conflicts with existing entry.\n\n"
- << *entry << "\n\n" << old_entry;
- }
- if (entry->Get(IS_DIR)) {
- // Get all child entries of the old id.
- trans->directory()->GetChildHandlesById(trans, old_id, children);
- Directory::ChildHandles::iterator i = children->begin();
- while (i != children->end()) {
- MutableEntry child_entry(trans, GET_BY_HANDLE, *i++);
- CHECK(child_entry.good());
- // Use the unchecked setter here to avoid touching the child's NEXT_ID
- // and PREV_ID fields (which Put(PARENT_ID) would normally do to
- // maintain linked-list invariants). In this case, NEXT_ID and PREV_ID
- // among the children will be valid after the loop, since we update all
- // the children at once.
- child_entry.PutParentIdPropertyOnly(new_id);
- }
- }
- // Update Id references on the previous and next nodes in the sibling
- // order. Do this by reinserting into the linked list; the first
- // step in PutPredecessor is to Unlink from the existing order, which
- // will overwrite the stale Id value from the adjacent nodes.
- if (entry->Get(PREV_ID) == entry->Get(NEXT_ID) &&
- entry->Get(PREV_ID) == old_id) {
- // We just need a shallow update to |entry|'s fields since it is already
- // self looped.
- entry->Put(NEXT_ID, new_id);
- entry->Put(PREV_ID, new_id);
- } else {
- entry->PutPredecessor(entry->Get(PREV_ID));
- }
-}
-
-// static
-void SyncerUtil::ChangeEntryIDAndUpdateChildren(
- syncable::WriteTransaction* trans,
- syncable::MutableEntry* entry,
- const syncable::Id& new_id) {
- syncable::Directory::ChildHandles children;
- ChangeEntryIDAndUpdateChildren(trans, entry, new_id, &children);
-}
-
-// static
syncable::Id SyncerUtil::FindLocalIdToUpdate(
syncable::BaseTransaction* trans,
const SyncEntity& update) {

Modified: trunk/src/sync/engine/syncer_util.h
==============================================================================
--- trunk/src/sync/engine/syncer_util.h (original)
+++ trunk/src/sync/engine/syncer_util.h Wed May 16 12:09:47 2012
@@ -25,20 +25,10 @@

class SyncerUtil {
public:
- static void ChangeEntryIDAndUpdateChildren(
- syncable::WriteTransaction* trans,
- syncable::MutableEntry* entry,
- const syncable::Id& new_id,
- syncable::Directory::ChildHandles* children);
-
// Returns the number of unsynced entries.
static int GetUnsyncedEntries(syncable::BaseTransaction* trans,
std::vector<int64> *handles);

- static void ChangeEntryIDAndUpdateChildren(syncable::WriteTransaction* trans,
- syncable::MutableEntry* entry,
- const syncable::Id& new_id);
-
// If the server sent down a client-tagged entry, or an entry whose
// commit response was lost, it is necessary to update a local entry
// with an ID that doesn't match the ID of the update. Here, we

Modified: trunk/src/sync/syncable/dir_open_result.h
==============================================================================
--- trunk/src/sync/syncable/dir_open_result.h (original)
+++ trunk/src/sync/syncable/dir_open_result.h Wed May 16 12:09:47 2012
@@ -15,6 +15,7 @@
FAILED_DISK_FULL, // The disk is full.
FAILED_DATABASE_CORRUPT, // Something is wrong with the DB
FAILED_LOGICAL_CORRUPTION, // Invalid database contents
+ FAILED_IN_UNITTEST, // For tests.
};
} // namespace syncable


Modified: trunk/src/sync/syncable/in_memory_directory_backing_store.cc
==============================================================================
--- trunk/src/sync/syncable/in_memory_directory_backing_store.cc (original)
+++ trunk/src/sync/syncable/in_memory_directory_backing_store.cc Wed May 16 12:09:47 2012
@@ -21,6 +21,8 @@
if (!InitializeTables())
return FAILED_OPEN_DATABASE;

+ if (!DropDeletedEntries())
+ return FAILED_DATABASE_CORRUPT;
if (!LoadEntries(entry_bucket))
return FAILED_DATABASE_CORRUPT;
if (!LoadInfo(kernel_load_info))

Modified: trunk/src/sync/syncable/syncable.cc
==============================================================================
--- trunk/src/sync/syncable/syncable.cc (original)
+++ trunk/src/sync/syncable/syncable.cc Wed May 16 12:09:47 2012
@@ -1736,6 +1736,18 @@
if (!UnlinkFromOrder()) {
return false;
}
+
+ // If the server never knew about this item and it's deleted then we don't
+ // need to keep it around. Unsetting IS_UNSYNCED will:
+ // - Ensure that the item is never committed to the server.
+ // - Allow any items with the same UNIQUE_CLIENT_TAG created on other
+ // clients to override this entry.
+ // - Let us delete this entry permanently through
+ // DirectoryBackingStore::DropDeletedEntries() when we next restart sync.
+ // This will save memory and avoid crbug.com/125381.
+ if (!Get(ID).ServerKnows()) {
+ Put(IS_UNSYNCED, false);
+ }
}

{
@@ -2376,4 +2388,47 @@
return NULL;
}

+void ChangeEntryIDAndUpdateChildren(
+ syncable::WriteTransaction* trans,
+ syncable::MutableEntry* entry,
+ const syncable::Id& new_id) {
+ syncable::Id old_id = entry->Get(ID);
+ if (!entry->Put(ID, new_id)) {
+ Entry old_entry(trans, GET_BY_ID, new_id);
+ CHECK(old_entry.good());
+ LOG(FATAL) << "Attempt to change ID to " << new_id
+ << " conflicts with existing entry.\n\n"
+ << *entry << "\n\n" << old_entry;
+ }
+ if (entry->Get(IS_DIR)) {
+ // Get all child entries of the old id.
+ syncable::Directory::ChildHandles children;
+ trans->directory()->GetChildHandlesById(trans, old_id, &children);
+ Directory::ChildHandles::iterator i = children.begin();
+ while (i != children.end()) {
+ MutableEntry child_entry(trans, GET_BY_HANDLE, *i++);
+ CHECK(child_entry.good());
+ // Use the unchecked setter here to avoid touching the child's NEXT_ID
+ // and PREV_ID fields (which Put(PARENT_ID) would normally do to
+ // maintain linked-list invariants). In this case, NEXT_ID and PREV_ID
+ // among the children will be valid after the loop, since we update all
+ // the children at once.
+ child_entry.PutParentIdPropertyOnly(new_id);
+ }
+ }
+ // Update Id references on the previous and next nodes in the sibling
+ // order. Do this by reinserting into the linked list; the first
+ // step in PutPredecessor is to Unlink from the existing order, which
+ // will overwrite the stale Id value from the adjacent nodes.
+ if (entry->Get(PREV_ID) == entry->Get(NEXT_ID) &&
+ entry->Get(PREV_ID) == old_id) {
+ // We just need a shallow update to |entry|'s fields since it is already
+ // self looped.
+ entry->Put(NEXT_ID, new_id);
+ entry->Put(PREV_ID, new_id);
+ } else {
+ entry->PutPredecessor(entry->Get(PREV_ID));
+ }
+}
+
} // namespace syncable

Modified: trunk/src/sync/syncable/syncable.h
==============================================================================
--- trunk/src/sync/syncable/syncable.h (original)
+++ trunk/src/sync/syncable/syncable.h Wed May 16 12:09:47 2012
@@ -1345,6 +1345,10 @@
// This function sets only the flags needed to get this entry to sync.
bool MarkForSyncing(syncable::MutableEntry* e);

+void ChangeEntryIDAndUpdateChildren(syncable::WriteTransaction* trans,
+ syncable::MutableEntry* entry,
+ const syncable::Id& new_id);
+
} // namespace syncable

std::ostream& operator <<(std::ostream&, const syncable::Blob&);

Modified: trunk/src/sync/syncable/syncable_unittest.cc
==============================================================================
--- trunk/src/sync/syncable/syncable_unittest.cc (original)
+++ trunk/src/sync/syncable/syncable_unittest.cc Wed May 16 12:09:47 2012
@@ -429,7 +429,8 @@
}

virtual void TearDown() {
- dir_->SaveChanges();
+ if (dir_.get())
+ dir_->SaveChanges();
dir_.reset();
}

@@ -508,6 +509,15 @@
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 +1164,102 @@
}
}

+// 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.
+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);
+
+ ChangeEntryIDAndUpdateChildren(&trans, &child, id_factory.NewServerId());
+ child.Put(IS_UNSYNCED, false);
+ child.Put(BASE_VERSION, 1);
+ child.Put(SERVER_VERSION, 1);
+
+ 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());
+}
+
+// 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 able to sync either it or its parent folder. This scenario used to cause
+// 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);
+
+ 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());
+}
+
// A variant of SyncableDirectoryTest that uses a real sqlite database.
class OnDiskSyncableDirectoryTest : public SyncableDirectoryTest {
protected:
@@ -1536,6 +1642,32 @@
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 need to clear this member. If we don't,
+ // TearDown() will be guaranteed to crash when it calls SaveChanges().
+ if (result != OPENED)
+ dir_.reset();
+
+ return result;
+}
+
namespace {

class SyncableDirectoryManagement : public testing::Test {