[Sync] Add application logic for non-nigori control types (reland)

Relanding after revert. Original codereview at
https://chromiumcodereview.appspot.com/11271009/.

BUG=122825
TBR=rlarcoque@chromium.org, akalin@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@167466 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 196 insertions, 45 deletions

diff --git a/sync/engine/apply_control_data_updates.cc b/sync/engine/apply_control_data_updates.cc
index d0f182d..6277365 100644
--- a/sync/engine/apply_control_data_updates.cc
+++ b/sync/engine/apply_control_data_updates.cc
@@ -24,15 +24,75 @@ using syncable::SERVER_SPECIFICS;
 using syncable::SPECIFICS;
 using syncable::SYNCER;
 
-void ApplyControlDataUpdates(syncable::Directory* dir) {
+void ApplyControlDataUpdates(sessions::SyncSession* session) {
+  syncable::Directory* dir = session->context()->directory();
   syncable::WriteTransaction trans(FROM_HERE, SYNCER, dir);
 
-  if (ApplyNigoriUpdates(&trans, dir->GetCryptographer(&trans))) {
-    dir->set_initial_sync_ended_for_type(NIGORI, true);
+  std::vector<int64> handles;
+  dir->GetUnappliedUpdateMetaHandles(
+      &trans, ToFullModelTypeSet(ControlTypes()), &handles);
+
+  // First, go through and manually apply any new top level datatype nodes (so
+  // that we don't have to worry about hitting a CONFLICT_HIERARCHY with an
+  // entry because we haven't applied its parent yet).
+  // TODO(sync): if at some point we support control datatypes with actual
+  // hierarchies we'll need to revisit this logic.
+  ModelTypeSet control_types = ControlTypes();
+  for (ModelTypeSet::Iterator iter = control_types.First(); iter.Good();
+       iter.Inc()) {
+    syncable::MutableEntry entry(&trans,
+                                 syncable::GET_BY_SERVER_TAG,
+                                 ModelTypeToRootTag(iter.Get()));
+    if (!entry.good())
+      continue;
+    if (!entry.Get(syncable::IS_UNAPPLIED_UPDATE))
+      continue;
+
+    ModelType type = entry.GetServerModelType();
+    if (type == NIGORI) {
+      // Nigori node applications never fail.
+      ApplyNigoriUpdate(&trans,
+                        &entry,
+                        dir->GetCryptographer(&trans));
+    } else {
+      ApplyControlUpdate(&trans,
+                         &entry,
+                         dir->GetCryptographer(&trans));
+    }
+  }
+
+  // Go through the rest of the unapplied control updates, skipping over any
+  // top level folders.
+  for (std::vector<int64>::const_iterator iter = handles.begin();
+       iter != handles.end(); ++iter) {
+    syncable::MutableEntry entry(&trans, syncable::GET_BY_HANDLE, *iter);
+    CHECK(entry.good());
+    ModelType type = entry.GetServerModelType();
+    CHECK(ControlTypes().Has(type));
+    if (!entry.Get(syncable::UNIQUE_SERVER_TAG).empty()) {
+      // We should have already applied all top level control nodes.
+      DCHECK(!entry.Get(syncable::IS_UNAPPLIED_UPDATE));
+      continue;
+    }
+
+    ApplyControlUpdate(&trans,
+                       &entry,
+                       dir->GetCryptographer(&trans));
+  }
+
+  // Set initial sync ended bits for all control types requested.
+  for (ModelTypeSet::Iterator it =
+           session->status_controller().updates_request_types().First();
+       it.Good(); it.Inc()) {
+    if (!IsControlType(it.Get()))
+        continue;
+
+    // This gets persisted to the directory's backing store.
+    dir->set_initial_sync_ended_for_type(it.Get(), true);
   }
 }
 
-// Update the sync encryption handler with the server's nigori node.
+// Update the nigori handler with the server's nigori node.
 //
 // If we have a locally modified nigori node, we merge them manually. This
 // handles the case where two clients both set a different passphrase. The
@@ -43,24 +103,18 @@ void ApplyControlDataUpdates(syncable::Directory* dir) {
 // passphrase, the cryptographer will preserve the encryption keys based on the
 // local passphrase, while the nigori node will preserve the server encryption
 // keys.
-bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
-                        Cryptographer* cryptographer) {
-  syncable::MutableEntry nigori_node(trans, GET_BY_SERVER_TAG,
-                                     ModelTypeToRootTag(NIGORI));
-
-  // Mainly for unit tests.  We should have a Nigori node by this point.
-  if (!nigori_node.good()) {
-    return false;
-  }
-
-  if (!nigori_node.Get(IS_UNAPPLIED_UPDATE)) {
-    return true;
-  }
+void ApplyNigoriUpdate(syncable::WriteTransaction* const trans,
+                       syncable::MutableEntry* const entry,
+                       Cryptographer* cryptographer) {
+  DCHECK(entry->Get(IS_UNAPPLIED_UPDATE));
 
   // We apply the nigori update regardless of whether there's a conflict or
   // not in order to preserve any new encrypted types or encryption keys.
+  // TODO(zea): consider having this return a bool reflecting whether it was a
+  // valid update or not, and in the case of invalid updates not overwrite the
+  // local data.
   const sync_pb::NigoriSpecifics& nigori =
-      nigori_node.Get(SERVER_SPECIFICS).nigori();
+      entry->Get(SERVER_SPECIFICS).nigori();
   trans->directory()->GetNigoriHandler()->ApplyNigoriUpdate(nigori, trans);
 
   // Make sure any unsynced changes are properly encrypted as necessary.
@@ -82,19 +136,19 @@ bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
     syncable::ProcessUnsyncedChangesForEncryption(trans);
   }
 
-  if (!nigori_node.Get(IS_UNSYNCED)) {  // Update only.
-    UpdateLocalDataFromServerData(trans, &nigori_node);
+  if (!entry->Get(IS_UNSYNCED)) {  // Update only.
+    UpdateLocalDataFromServerData(trans, entry);
   } else {  // Conflict.
     const sync_pb::EntitySpecifics& server_specifics =
-        nigori_node.Get(SERVER_SPECIFICS);
+        entry->Get(SERVER_SPECIFICS);
     const sync_pb::NigoriSpecifics& server_nigori = server_specifics.nigori();
     const sync_pb::EntitySpecifics& local_specifics =
-        nigori_node.Get(SPECIFICS);
+        entry->Get(SPECIFICS);
     const sync_pb::NigoriSpecifics& local_nigori = local_specifics.nigori();
 
     // We initialize the new nigori with the server state, and will override
     // it as necessary below.
-    sync_pb::EntitySpecifics new_specifics = nigori_node.Get(SERVER_SPECIFICS);
+    sync_pb::EntitySpecifics new_specifics = entry->Get(SERVER_SPECIFICS);
     sync_pb::NigoriSpecifics* new_nigori = new_specifics.mutable_nigori();
 
     // If the cryptographer is not ready, another client set a new encryption
@@ -145,18 +199,35 @@ bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
         new_nigori,
         trans);
 
-    nigori_node.Put(SPECIFICS, new_specifics);
+    entry->Put(SPECIFICS, new_specifics);
     DVLOG(1) << "Resolving simple conflict, merging nigori nodes: "
-             << nigori_node;
+             << entry;
 
-    conflict_util::OverwriteServerChanges(&nigori_node);
+    conflict_util::OverwriteServerChanges(entry);
 
     UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                               ConflictResolver::NIGORI_MERGE,
                               ConflictResolver::CONFLICT_RESOLUTION_SIZE);
   }
+}
+
+void ApplyControlUpdate(syncable::WriteTransaction* const trans,
+                        syncable::MutableEntry* const entry,
+                        Cryptographer* cryptographer) {
+  DCHECK_NE(entry->GetServerModelType(), NIGORI);
+  DCHECK(entry->Get(IS_UNAPPLIED_UPDATE));
+  if (entry->Get(IS_UNSYNCED)) {
+      // We just let the server win all conflicts with control types.
+    DVLOG(1) << "Ignoring local changes for control update.";
+    conflict_util::IgnoreLocalChanges(entry);
+    UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
+                              ConflictResolver::OVERWRITE_LOCAL,
+                              ConflictResolver::CONFLICT_RESOLUTION_SIZE);
+  }
 
-  return true;
+  UpdateAttemptResponse response = AttemptToUpdateEntry(
+      trans, entry, cryptographer);
+  DCHECK_EQ(SUCCESS, response);
 }
 
 }  // namespace syncer
diff --git a/sync/engine/apply_control_data_updates.h b/sync/engine/apply_control_data_updates.h
index efe2cc8..8d19f47 100644
--- a/sync/engine/apply_control_data_updates.h
+++ b/sync/engine/apply_control_data_updates.h
@@ -9,13 +9,22 @@ namespace syncer {
 
 class Cryptographer;
 
+namespace sessions {
+class SyncSession;
+}
+
 namespace syncable {
 class Directory;
+class MutableEntry;
 class WriteTransaction;
 }
 
-void ApplyControlDataUpdates(syncable::Directory* dir);
-bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
+void ApplyControlDataUpdates(sessions::SyncSession* session);
+void ApplyNigoriUpdate(syncable::WriteTransaction* trans,
+                       syncable::MutableEntry* const entry,
+                       Cryptographer* cryptographer);
+void ApplyControlUpdate(syncable::WriteTransaction* const trans,
+                        syncable::MutableEntry* const entry,
                         Cryptographer* cryptographer);
 
 }  // namespace syncer
diff --git a/sync/engine/apply_control_data_updates_unittest.cc b/sync/engine/apply_control_data_updates_unittest.cc
index aa00865..09c5e11 100644
--- a/sync/engine/apply_control_data_updates_unittest.cc
+++ b/sync/engine/apply_control_data_updates_unittest.cc
@@ -41,12 +41,14 @@ class ApplyControlDataUpdatesTest : public SyncerCommandTest {
     workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_UI)));
     workers()->push_back(
         make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD)));
-    (*mutable_routing_info())[BOOKMARKS] = GROUP_UI;
-    (*mutable_routing_info())[PASSWORDS] = GROUP_PASSWORD;
     (*mutable_routing_info())[NIGORI] = GROUP_PASSIVE;
+    (*mutable_routing_info())[EXPERIMENTS] = GROUP_PASSIVE;
     SyncerCommandTest::SetUp();
     entry_factory_.reset(new TestEntryFactory(directory()));
 
+    session()->mutable_status_controller()->set_updates_request_types(
+        ControlTypes());
+
     syncable::ReadTransaction trans(FROM_HERE, directory());
   }
 
@@ -90,7 +92,7 @@ TEST_F(ApplyControlDataUpdatesTest, NigoriUpdate) {
       ModelTypeToRootTag(NIGORI), specifics, true);
   EXPECT_FALSE(cryptographer->has_pending_keys());
 
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
@@ -170,7 +172,7 @@ TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) {
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
 
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
 
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
@@ -198,7 +200,7 @@ TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) {
     entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
   }
 
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
 
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
@@ -285,7 +287,7 @@ TEST_F(ApplyControlDataUpdatesTest, CannotEncryptUnsyncedChanges) {
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
 
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
@@ -363,7 +365,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -442,7 +444,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -516,7 +518,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -596,7 +598,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -679,7 +681,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -760,7 +762,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -841,7 +843,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(directory());
+  ApplyControlDataUpdates(session());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -868,4 +870,72 @@ TEST_F(ApplyControlDataUpdatesTest,
   }
 }
 
+// Check that we can apply a simple control datatype node successfully.
+TEST_F(ApplyControlDataUpdatesTest, ControlApply) {
+  EXPECT_FALSE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
+
+  std::string experiment_id = "experiment";
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_experiments()->mutable_keystore_encryption()->
+      set_enabled(true);
+  int64 experiment_handle = entry_factory_->CreateUnappliedNewItem(
+      experiment_id, specifics, false);
+  ApplyControlDataUpdates(session());
+
+  EXPECT_TRUE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
+  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
+  EXPECT_TRUE(
+      entry_factory_->GetLocalSpecificsForItem(experiment_handle).
+          experiments().keystore_encryption().enabled());
+}
+
+// Verify that we apply top level folders before their children.
+TEST_F(ApplyControlDataUpdatesTest, ControlApplyParentBeforeChild) {
+  EXPECT_FALSE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
+
+  std::string parent_id = "parent";
+  std::string experiment_id = "experiment";
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_experiments()->mutable_keystore_encryption()->
+      set_enabled(true);
+  int64 experiment_handle = entry_factory_->CreateUnappliedNewItemWithParent(
+      experiment_id, specifics, parent_id);
+  int64 parent_handle = entry_factory_->CreateUnappliedNewItem(
+      parent_id, specifics, true);
+  ApplyControlDataUpdates(session());
+
+  EXPECT_TRUE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
+  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(parent_handle));
+  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
+  EXPECT_TRUE(
+      entry_factory_->GetLocalSpecificsForItem(experiment_handle).
+          experiments().keystore_encryption().enabled());
+}
+
+// Verify that we handle control datatype conflicts by preserving the server
+// data.
+TEST_F(ApplyControlDataUpdatesTest, ControlConflict) {
+  EXPECT_FALSE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
+
+  std::string experiment_id = "experiment";
+  sync_pb::EntitySpecifics local_specifics, server_specifics;
+  server_specifics.mutable_experiments()->mutable_keystore_encryption()->
+      set_enabled(true);
+  local_specifics.mutable_experiments()->mutable_keystore_encryption()->
+      set_enabled(false);
+  int64 experiment_handle = entry_factory_->CreateSyncedItem(
+      experiment_id, EXPERIMENTS, false);
+  entry_factory_->SetServerSpecificsForItem(experiment_handle,
+                                            server_specifics);
+  entry_factory_->SetLocalSpecificsForItem(experiment_handle,
+                                           local_specifics);
+  ApplyControlDataUpdates(session());
+
+  EXPECT_TRUE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
+  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
+  EXPECT_TRUE(
+      entry_factory_->GetLocalSpecificsForItem(experiment_handle).
+          experiments().keystore_encryption().enabled());
+}
+
 }  // namespace syncer
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 22010ee..90d193a 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -144,7 +144,7 @@ bool Syncer::SyncShare(sessions::SyncSession* session,
       }
       case APPLY_UPDATES: {
         // These include encryption updates that should be applied early.
-        ApplyControlDataUpdates(session->context()->directory());
+        ApplyControlDataUpdates(session);
 
         ApplyUpdatesAndResolveConflictsCommand apply_updates;
         apply_updates.Execute(session);
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index cdc1b7a..b88bc7c 100644
--- a/sync/engine/syncer_util.cc
+++ b/sync/engine/syncer_util.cc
@@ -209,7 +209,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
     // We can't decrypt this node yet.
     DVLOG(1) << "Received an undecryptable "
              << ModelTypeToString(entry->GetServerModelType())
-             << " update, returning encryption_conflict.";
+             << " update, returning conflict_encryption.";
     return CONFLICT_ENCRYPTION;
   } else if (specifics.has_password() &&
              entry->Get(UNIQUE_SERVER_TAG).empty()) {
@@ -217,7 +217,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
     const sync_pb::PasswordSpecifics& password = specifics.password();
     if (!cryptographer->CanDecrypt(password.encrypted())) {
       DVLOG(1) << "Received an undecryptable password update, returning "
-               << "encryption_conflict.";
+               << "conflict_encryption.";
       return CONFLICT_ENCRYPTION;
     }
   }
@@ -232,6 +232,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
     // different ways we deal with it once here to reduce the amount of code and
     // potential errors.
     if (!parent.good() || parent.Get(IS_DEL) || !parent.Get(IS_DIR)) {
+      DVLOG(1) <<  "Entry has bad parent, returning conflict_hierarchy.";
       return CONFLICT_HIERARCHY;
     }
     if (entry->Get(PARENT_ID) != new_parent) {