Sync: Clear IS_UNSYNCED for deleted local items

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


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137476

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

zea: Please review.

http://codereview.chromium.org/10389103/diff/1/sync/engine/syncer_unittest.cc
File sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/10389103/diff/1/sync/engine/syncer_unittest.cc...
sync/engine/syncer_unittest.cc:1453: parent.Put(syncable::ID,
ids_.FromNumber(103));
Because of my change, the order in which we set ID or IS_DEL actually matters. 
I'm mostly convinced that this is only a problem in unit tests.

http://codereview.chromium.org/10389103/diff/1/sync/engine/syncer_unittest.cc...
sync/engine/syncer_unittest.cc:3732: // The local entry will be overwritten.
This illustrates the change in behavior mentioned in the commit message.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:23: #include "sync/engine/syncer_util.h"
I'm not sure if it's OK to test syncer_util functions from this file.  Thoughts?

[Nicolas Zea, 2012-05-11 23:21:24]

Couple nits. Also, this actually looks like it's going back to the expectations
from before http://codereview.chromium.org/8922015, so I wonder why you had to
change the syncer unit tests.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable.cc
File sync/syncable/syncable.cc (right):

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable.cc#newc...
sync/syncable/syncable.cc:1740: // will attempt to undelete it.  It's never
going to make it to the server
"no one will attempt to undelete it" seems a bit misleading. We won't undelete
it via conflict resolution, but we will if another client creates an item with
the same unique client tag. Perhaps mention that?

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable.cc#newc...
sync/syncable/syncable.cc:1741: // either.  Mark it as unsynced so we can drop
it later in
Mark it as unsynced -> Unmark it as unsynced

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1170: // a more common variant of the
'DeletedAndUnsyncedChild' scenario tested below.
DeleteAndUnsyncedChild -> DeleteAnUnsyncedChild? (here and below)

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1212: // Final check of validity.
of -> for

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1218: // are abot to sync either it or its
parent folder.  This scenario used to cause
abot -> able

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1263: // Final check of validity.
of -> for

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1667: // If something went wrong, we to clear
this member.  If we don't, TearDown()
we to -> we need to

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

Updated the diff to address your comments.  PTAL.

I think I know why the tests only started failing with my commit.  The old
mechanism for unsetting the IS_UNSYNCED bit was in the iteration in
GetCommitIdsCommand.  The list of items iterated did not include entries
filtered out in FilterUnreadyEntries.  For the scenario in
SyncerTest.ClientTagUncommittedTagMatchesUpdate, the downloaded update put the
entry into a conflict state, so it was never iterated over when building the
commit.  The item remained untouched until conflict resolution.  

The way I've implemented it, the UNSYNCED bit is cleared earlier so the update
gets applied right away.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable.cc
File sync/syncable/syncable.cc (right):

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable.cc#newc...
sync/syncable/syncable.cc:1740: // will attempt to undelete it.  It's never
going to make it to the server
On 2012/05/11 23:21:25, nzea wrote:
> "no one will attempt to undelete it" seems a bit misleading. We won't undelete
> it via conflict resolution, but we will if another client creates an item with
> the same unique client tag. Perhaps mention that?

Will do.  I was thinking mainly of bookmarks when I wrote that comment.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1170: // a more common variant of the
'DeletedAndUnsyncedChild' scenario tested below.
On 2012/05/11 23:21:25, nzea wrote:
> DeleteAndUnsyncedChild -> DeleteAnUnsyncedChild? (here and below)

It's DeletedAndUnsyncedChild.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1212: // Final check of validity.
On 2012/05/11 23:21:25, nzea wrote:
> of -> for

Done.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1218: // are abot to sync either it or its
parent folder.  This scenario used to cause
On 2012/05/11 23:21:25, nzea wrote:
> abot -> able

Done.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1263: // Final check of validity.
On 2012/05/11 23:21:25, nzea wrote:
> of -> for

Done.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1263: // Final check of validity.
On 2012/05/11 23:21:25, nzea wrote:
> of -> for

Done.

http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
sync/syncable/syncable_unittest.cc:1667: // If something went wrong, we to clear
this member.  If we don't, TearDown()
On 2012/05/11 23:21:25, nzea wrote:
> we to -> we need to

Done.

[Nicolas Zea, 2012-05-15 18:07:02]

On 2012/05/15 00:41:55, rlarocque wrote:
> Updated the diff to address your comments.  PTAL.
> 
> I think I know why the tests only started failing with my commit.  The old
> mechanism for unsetting the IS_UNSYNCED bit was in the iteration in
> GetCommitIdsCommand.  The list of items iterated did not include entries
> filtered out in FilterUnreadyEntries.  For the scenario in
> SyncerTest.ClientTagUncommittedTagMatchesUpdate, the downloaded update put the
> entry into a conflict state, so it was never iterated over when building the
> commit.  The item remained untouched until conflict resolution.  
> 
> The way I've implemented it, the UNSYNCED bit is cleared earlier so the update
> gets applied right away.

Note that FilterUnreadyEntries is a recent addition, only added in
November/December. Previously GetCommitId's iterated through all unsynced items
items, including conflicting items.

I'm just hesitant that we don't fully know why this assumption is changing, but
if you're confident in the change, LGTM

> 
> http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable.cc
> File sync/syncable/syncable.cc (right):
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable.cc#newc...
> sync/syncable/syncable.cc:1740: // will attempt to undelete it.  It's never
> going to make it to the server
> On 2012/05/11 23:21:25, nzea wrote:
> > "no one will attempt to undelete it" seems a bit misleading. We won't
undelete
> > it via conflict resolution, but we will if another client creates an item
with
> > the same unique client tag. Perhaps mention that?
> 
> Will do.  I was thinking mainly of bookmarks when I wrote that comment.
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
> File sync/syncable/syncable_unittest.cc (right):
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
> sync/syncable/syncable_unittest.cc:1170: // a more common variant of the
> 'DeletedAndUnsyncedChild' scenario tested below.
> On 2012/05/11 23:21:25, nzea wrote:
> > DeleteAndUnsyncedChild -> DeleteAnUnsyncedChild? (here and below)
> 
> It's DeletedAndUnsyncedChild.
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
> sync/syncable/syncable_unittest.cc:1212: // Final check of validity.
> On 2012/05/11 23:21:25, nzea wrote:
> > of -> for
> 
> Done.
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
> sync/syncable/syncable_unittest.cc:1218: // are abot to sync either it or its
> parent folder.  This scenario used to cause
> On 2012/05/11 23:21:25, nzea wrote:
> > abot -> able
> 
> Done.
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
> sync/syncable/syncable_unittest.cc:1263: // Final check of validity.
> On 2012/05/11 23:21:25, nzea wrote:
> > of -> for
> 
> Done.
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
> sync/syncable/syncable_unittest.cc:1263: // Final check of validity.
> On 2012/05/11 23:21:25, nzea wrote:
> > of -> for
> 
> Done.
> 
>
http://codereview.chromium.org/10389103/diff/1/sync/syncable/syncable_unittes...
> sync/syncable/syncable_unittest.cc:1667: // If something went wrong, we to
clear
> this member.  If we don't, TearDown()
> On 2012/05/11 23:21:25, nzea wrote:
> > we to -> we need to
> 
> Done.

[rlarocque, 2012-05-15 18:44:44]

On 2012/05/15 18:07:02, nzea wrote:
> On 2012/05/15 00:41:55, rlarocque wrote:
> > Updated the diff to address your comments.  PTAL.
> > 
> > I think I know why the tests only started failing with my commit.  The old
> > mechanism for unsetting the IS_UNSYNCED bit was in the iteration in
> > GetCommitIdsCommand.  The list of items iterated did not include entries
> > filtered out in FilterUnreadyEntries.  For the scenario in
> > SyncerTest.ClientTagUncommittedTagMatchesUpdate, the downloaded update put
the
> > entry into a conflict state, so it was never iterated over when building the
> > commit.  The item remained untouched until conflict resolution.  
> > 
> > The way I've implemented it, the UNSYNCED bit is cleared earlier so the
update
> > gets applied right away.
> 
> Note that FilterUnreadyEntries is a recent addition, only added in
> November/December. Previously GetCommitId's iterated through all unsynced
items
> items, including conflicting items.
> 
> I'm just hesitant that we don't fully know why this assumption is changing,
but
> if you're confident in the change, LGTM
> 

I hadn't thought about it before, but I think the explanation still makes sense
even if the UNSYNCED bit does get cleared in GetCommitIds.

After the download, we entry would be UNSYNCED, UNAPPLIED and locally deleted. 
GetCommitIds would unset the UNSYNCED bit before we commit.  It shouldn't try to
commit the entry because it's in conflict (because the commit step defines
conflict based on BASE_VERSION and SERVER_VERSION, the UNSYNCED bit is
irrelevant here).  Even if it did try to commit, the commit would bounce with a
CONFLICT response because the unit tests told the mock server to "conflict all
commits".  The item will eventually make its way to conflict resolution, where
the local version wins.

[rlarocque, 2012-05-15 23:12:38]

I had to move some code around to fix check_deps.  That's a big enough change
that it probably merits another quick review.

[Nicolas Zea, 2012-05-15 23:25:04]

https://chromiumcodereview.appspot.com/10389103/diff/8001/sync/engine/syncer_...
File sync/engine/syncer_util.cc (right):

https://chromiumcodereview.appspot.com/10389103/diff/8001/sync/engine/syncer_...
sync/engine/syncer_util.cc:83: // static
remove comment

https://chromiumcodereview.appspot.com/10389103/diff/8001/sync/syncable/synca...
File sync/syncable/syncable.cc (right):

https://chromiumcodereview.appspot.com/10389103/diff/8001/sync/syncable/synca...
sync/syncable/syncable.cc:2391: static void ChangeEntryIDAndUpdateChildren(
is there any point to keeping this function separate from the one below?

[rlarocque, 2012-05-16 00:32:36]

Uploaded new version that removes the wrapper function.  Let's see if the
trybots like it.  (I see no reason why they wouldn't.)

http://codereview.chromium.org/10389103/diff/8001/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/10389103/diff/8001/sync/engine/syncer_util.cc#...
sync/engine/syncer_util.cc:83: // static
On 2012/05/15 23:25:04, nzea wrote:
> remove comment

This comment belongs to SyncerUtil::FindLocalIdToUpate.

http://codereview.chromium.org/10389103/diff/8001/sync/syncable/syncable.cc
File sync/syncable/syncable.cc (right):

http://codereview.chromium.org/10389103/diff/8001/sync/syncable/syncable.cc#n...
sync/syncable/syncable.cc:2391: static void ChangeEntryIDAndUpdateChildren(
On 2012/05/15 23:25:04, nzea wrote:
> is there any point to keeping this function separate from the one below?

Probably not.  I kept it separate because that's the way it was in the previous
file.  I'll try merging them and see if anything breaks.

[Nicolas Zea, 2012-05-16 00:36:38]

LGTM

[commit-bot: I haz the power, 2012-05-16 17:49:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10389103/11002

[commit-bot: I haz the power, 2012-05-16 19:09:52]

Change committed as 137476