Sync: Conflict resolution code doesn't take into account implicit permanent folders

The code that resolves simple conflicts detect the type of conflict
incorrectly when occurs during transition to implicit root folder, when
Parent ID changes from root folder ID to an empty string.

This shouldn't be considered a hierarchy change as far as conflict type
detection is concerned.

BUG=548734

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

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

2 files changed, 46 insertions, 1 deletions

diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc
index f914403..0af3097 100644
--- a/sync/engine/conflict_resolver.cc
+++ b/sync/engine/conflict_resolver.cc
@@ -125,7 +125,9 @@ void ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
     // See http://crbug.com/77339.
     bool name_matches = entry.GetNonUniqueName() ==
         entry.GetServerNonUniqueName();
-    bool parent_matches = entry.GetParentId() == entry.GetServerParentId();
+    // The parent is implicit type root folder or the parent ID matches.
+    bool parent_matches = entry.GetServerParentId().IsNull() ||
+                          entry.GetParentId() == entry.GetServerParentId();
     bool entry_deleted = entry.GetIsDel();
     // The position check might fail spuriously if one of the positions was
     // based on a legacy random suffix, rather than a deterministic one based on
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 90d9e14..fff926c 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -2817,6 +2817,49 @@ TEST_F(SyncerTest, DeletingEntryInFolder) {
   EXPECT_EQ(0, GetCommitCounters(BOOKMARKS).num_commits_conflict);
 }
 
+// Test conflict resolution when handling an update for an item with specified
+// Parent ID and having an implicit (unset) Parent ID in the update.
+TEST_F(SyncerTest, ConflictWithImplicitParent) {
+  // Make sure PREFERENCES root exists so that we can get its parent ID.
+  mock_server_->AddUpdateSpecifics(1, 0, "Folder", 10, 10, true, 1,
+                                   DefaultPreferencesSpecifics());
+  mock_server_->SetLastUpdateServerTag(ModelTypeToRootTag(PREFERENCES));
+  EXPECT_TRUE(SyncShareNudge());
+
+  Id pref_root_id;
+  {
+    // Preferences type root should have been created by the update above.
+    // We need it in order to get its ID.
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+
+    Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
+    ASSERT_TRUE(pref_root.good());
+    pref_root_id = pref_root.GetId();
+  }
+
+  // Fake an item which is both unsynced and unapplied with
+  // PARENT_ID set to |pref_root_id| and SERVER_PARENT_ID unset.
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry entry(&trans, CREATE, PREFERENCES, pref_root_id, "bob");
+    entry.PutServerNonUniqueName("bob");
+    entry.PutId(ids_.FromNumber(20));
+    entry.PutBaseVersion(1);
+    entry.PutServerVersion(1);
+    entry.PutIsUnsynced(true);
+    entry.PutIsUnappliedUpdate(true);
+    entry.PutSpecifics(DefaultPreferencesSpecifics());
+    entry.PutServerSpecifics(DefaultPreferencesSpecifics());
+    entry.PutIsDel(false);
+  }
+
+  EXPECT_TRUE(SyncShareNudge());
+  // Since the hierarchy isn't really changed (the type has flat hierarchy)
+  // this conflict must be discarded.
+  EXPECT_EQ(0, session_->status_controller().num_local_overwrites());
+  EXPECT_EQ(0, session_->status_controller().num_server_overwrites());
+}
+
 TEST_F(SyncerTest, DeletingEntryWithLocalEdits) {
   int64 newfolder_metahandle;