Sync: Fixed crash in ParentChildIndex::Remove

The crash is caused by attempting to remove a non-existing node from
ParentChildIndex. There is DCHECK when running on debug, but on
on release the code would simply use an invalid iterator and crash.
At appears the situation is triggered by the "undelete" case in
WriteNode::InitUniqueByCreationImpl which reuses an
existing "server only" node with a matching client tag (server only
means an unapplied node with server only fields assigned).
Deleted nodes are excluded from the index. When the node is reused
it should be inserted in the index when it "deleted" flag is cleared
but it doesn't because at the time it doesn't yet have it client SPECIFICS
value and the login in ParentChildIndex::ShouldInclude prevents it
from being inserted. There is no more actions that would trigger
insertion in the index so the node ends up being excluded from the
index.

The fix.
1) The condition in ParentChild::ShouldInclude should be simplified.
It is unnecessary to look at presence of client SPECIFICS. "Server only"
unapplied nodes are initialized with their "deleted" bit set so
they would already be excluded from the index even without that
extra condition.
2) The client datatype of a node must be known when it is inserted
in the index in order to handle nodes with implicit (unset) parent.
It makes the code fragile because it requires the certain order of
setting the node fields - the SPECIFICS must be assigned before
any other field that triggers (re)insertion in the index. The fix in
WriteNode::InitUniqueByCreationImpl is to move the code that
assigns SPECIFICS before PutIsDel(false).
3) There is a new unit test that reproduces the crash without the
fix and passes with the fix.

A better fix to avoid this kind of problem in general would be to store
the type explicitly in each node and not try to derive it from either
client or server SPECIFICS (each of these fields might be unset
on new nodes that have never been committed or applied). Once
node type is set on construction it should never be changed.
This is tracked by crbug.com/508392.

BUG=505761

Review URL: https://codereview.chromium.org/1306033004

Cr-Commit-Position: refs/heads/master@{#346025}

1 files changed, 1 insertions, 7 deletions

diff --git a/sync/syncable/parent_child_index.cc b/sync/syncable/parent_child_index.cc
index b1b493e..bf6e03f 100644
--- a/sync/syncable/parent_child_index.cc
+++ b/sync/syncable/parent_child_index.cc
@@ -64,13 +64,7 @@ ParentChildIndex::~ParentChildIndex() {
 bool ParentChildIndex::ShouldInclude(const EntryKernel* entry) {
   // This index excludes deleted items and the root item.  The root
   // item is excluded so that it doesn't show up as a child of itself.
-  // The index also excludes items without client data e.g. the items that
-  // don't have client SPECIFICS or PARENT_ID. There is no need to include
-  // server-only entries because they shouldn't show up on the client until
-  // they're applied.
-  return !entry->ref(IS_DEL) && !entry->ref(ID).IsRoot() &&
-         (entry->GetModelType() != UNSPECIFIED ||
-          !entry->ref(PARENT_ID).IsNull());
+  return !entry->ref(IS_DEL) && !entry->ref(ID).IsRoot();
 }
 
 bool ParentChildIndex::Insert(EntryKernel* entry) {