Sync: fix for the code that checks whether the initial download has completed

The code that detects whether the initial download has completed for
a type had been inadvertently broken by https://codereview.chromium.org/1142423006/

This fix changes Directory::InitialSyncEndedForType back to the
original implementation and adds special handling for implicitly created
root folders (i.e. root folders auto-created locally by the update
handling code). Since the locally created root folders don't get applied
with the rest of downloaded nodes, their base version has to be
explicitly changed from CHANGES_VERSION to some other value at
the end of update application process. That would to indicate that at
least one update has been applied for the type and be consistent with
how the server generated root folders are handled.

BUG=540814

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

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

11 files changed, 223 insertions, 43 deletions

diff --git a/sync/engine/apply_control_data_updates.cc b/sync/engine/apply_control_data_updates.cc
index 3e167b1..49825b8 100644
--- a/sync/engine/apply_control_data_updates.cc
+++ b/sync/engine/apply_control_data_updates.cc
@@ -32,13 +32,25 @@ void ApplyControlDataUpdates(syncable::Directory* dir) {
   ModelTypeSet control_types = ControlTypes();
   for (ModelTypeSet::Iterator iter = control_types.First(); iter.Good();
        iter.Inc()) {
-    syncable::MutableEntry entry(&trans, syncable::GET_TYPE_ROOT, iter.Get());
+    ModelType type = iter.Get();
+    syncable::MutableEntry entry(&trans, syncable::GET_TYPE_ROOT, type);
     if (!entry.good())
       continue;
-    if (!entry.GetIsUnappliedUpdate())
+
+    if (!entry.GetIsUnappliedUpdate()) {
+      // If this is a type with client generated root, the root node has been
+      // created locally and might never be updated by the server. In that case
+      // it has to be marked as having the initial download completed (which is
+      // done by changing the root's base version to a value other than
+      // CHANGES_VERSION). This does nothing if the root's base version is
+      // already other than CHANGES_VERSION.
+      if (IsTypeWithClientGeneratedRoot(type)) {
+        dir->MarkInitialSyncEndedForType(&trans, type);
+      }
       continue;
+    }
 
-    ModelType type = entry.GetServerModelType();
+    DCHECK_EQ(type, entry.GetServerModelType());
     if (type == NIGORI) {
       // Nigori node applications never fail.
       ApplyNigoriUpdate(&trans,
diff --git a/sync/engine/apply_control_data_updates_unittest.cc b/sync/engine/apply_control_data_updates_unittest.cc
index 7ed6bd4..c01e5a7 100644
--- a/sync/engine/apply_control_data_updates_unittest.cc
+++ b/sync/engine/apply_control_data_updates_unittest.cc
@@ -914,4 +914,67 @@ TEST_F(ApplyControlDataUpdatesTest, ControlConflict) {
           experiments().keystore_encryption().enabled());
 }
 
+// Check that applying a EXPERIMENTS update marks the datatype as downloaded.
+TEST_F(ApplyControlDataUpdatesTest, ExperimentsApplyMarksDownloadCompleted) {
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(EXPERIMENTS));
+
+  // Create root node for EXPERIMENTS datatype
+  {
+    syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    syncable::ModelNeutralMutableEntry entry(
+        &trans, syncable::CREATE_NEW_TYPE_ROOT, EXPERIMENTS);
+    ASSERT_TRUE(entry.good());
+    entry.PutServerIsDir(true);
+    entry.PutUniqueServerTag(ModelTypeToRootTag(EXPERIMENTS));
+  }
+
+  // Initial sync isn't marked as ended for EXPERIMENTS even though the
+  // root folder exists.
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(EXPERIMENTS));
+
+  std::string experiment_id = "experiment";
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_experiments()->mutable_keystore_encryption()->set_enabled(
+      true);
+  entry_factory_->CreateUnappliedNewItem(experiment_id, specifics, false);
+
+  ApplyControlDataUpdates(directory());
+
+  // After applying the updates EXPERIMENTS should be marked as having its
+  // initial sync completed.
+  EXPECT_TRUE(directory()->InitialSyncEndedForType(EXPERIMENTS));
+  // Verify that there is no side effect on another control type.
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(NIGORI));
+}
+
+// Check that applying a NIGORI update marks the datatype as downloaded.
+TEST_F(ApplyControlDataUpdatesTest, NigoriApplyMarksDownloadCompleted) {
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(NIGORI));
+
+  Cryptographer* cryptographer;
+
+  {
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+    cryptographer = directory()->GetCryptographer(&trans);
+  }
+
+  KeyParams params = {"localhost", "dummy", "foobar"};
+  cryptographer->AddKey(params);
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori();
+  cryptographer->GetKeys(nigori->mutable_encryption_keybag());
+  nigori->set_encrypt_everything(true);
+
+  entry_factory_->CreateUnappliedNewItem(ModelTypeToRootTag(NIGORI), specifics,
+                                         true);
+
+  ApplyControlDataUpdates(directory());
+
+  // After applying the updates NIGORI should be marked as having its
+  // initial sync completed.
+  EXPECT_TRUE(directory()->InitialSyncEndedForType(NIGORI));
+  // Verify that there is no side effect on another control type.
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(EXPERIMENTS));
+}
+
 }  // namespace syncer
diff --git a/sync/engine/directory_update_handler.cc b/sync/engine/directory_update_handler.cc
index 5b2bac8..8adc6dd 100644
--- a/sync/engine/directory_update_handler.cc
+++ b/sync/engine/directory_update_handler.cc
@@ -10,6 +10,7 @@
 #include "sync/sessions/directory_type_debug_info_emitter.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/model_neutral_mutable_entry.h"
+#include "sync/syncable/syncable_changes_version.h"
 #include "sync/syncable/syncable_model_neutral_write_transaction.h"
 #include "sync/syncable/syncable_write_transaction.h"
 #include "sync/util/data_type_histogram.h"
@@ -109,34 +110,34 @@ void DirectoryUpdateHandler::CreateTypeRoot(
 }
 
 void DirectoryUpdateHandler::ApplyUpdates(sessions::StatusController* status) {
-  if (!IsApplyUpdatesRequired()) {
-    return;
+  if (IsApplyUpdatesRequired()) {
+    // This will invoke handlers that belong to the model and its thread, so we
+    // switch to the appropriate thread before we start this work.
+    WorkCallback c =
+        base::Bind(&DirectoryUpdateHandler::ApplyUpdatesImpl,
+                   // We wait until the callback is executed.  We can safely use
+                   // Unretained.
+                   base::Unretained(this), base::Unretained(status));
+    worker_->DoWorkAndWaitUntilDone(c);
+
+    debug_info_emitter_->EmitUpdateCountersUpdate();
+    debug_info_emitter_->EmitStatusCountersUpdate();
   }
 
-  // This will invoke handlers that belong to the model and its thread, so we
-  // switch to the appropriate thread before we start this work.
-  WorkCallback c = base::Bind(
-      &DirectoryUpdateHandler::ApplyUpdatesImpl,
-      // We wait until the callback is executed.  We can safely use Unretained.
-      base::Unretained(this),
-      base::Unretained(status));
-  worker_->DoWorkAndWaitUntilDone(c);
-
-  debug_info_emitter_->EmitUpdateCountersUpdate();
-  debug_info_emitter_->EmitStatusCountersUpdate();
+  PostApplyUpdates();
 }
 
 void DirectoryUpdateHandler::PassiveApplyUpdates(
     sessions::StatusController* status) {
-  if (!IsApplyUpdatesRequired()) {
-    return;
-  }
+  if (IsApplyUpdatesRequired()) {
+    // Just do the work here instead of deferring to another thread.
+    ApplyUpdatesImpl(status);
 
-  // Just do the work here instead of deferring to another thread.
-  ApplyUpdatesImpl(status);
+    debug_info_emitter_->EmitUpdateCountersUpdate();
+    debug_info_emitter_->EmitStatusCountersUpdate();
+  }
 
-  debug_info_emitter_->EmitUpdateCountersUpdate();
-  debug_info_emitter_->EmitStatusCountersUpdate();
+  PostApplyUpdates();
 }
 
 SyncerError DirectoryUpdateHandler::ApplyUpdatesImpl(
@@ -218,6 +219,20 @@ SyncerError DirectoryUpdateHandler::ApplyUpdatesImpl(
   return SYNCER_OK;
 }
 
+void DirectoryUpdateHandler::PostApplyUpdates() {
+  // If this is a type with client generated root, the root node has been
+  // created locally and didn't go through ApplyUpdatesImpl.
+  // Mark it as having the initial download completed so that the type
+  // reports as properly initialized (which is done by changing the root's
+  // base version to a value other than CHANGES_VERSION).
+  // This does nothing if the root's base version is already other than
+  // CHANGES_VERSION.
+  if (IsTypeWithClientGeneratedRoot(type_)) {
+    syncable::ModelNeutralWriteTransaction trans(FROM_HERE, SYNCER, dir_);
+    dir_->MarkInitialSyncEndedForType(&trans, type_);
+  }
+}
+
 bool DirectoryUpdateHandler::IsApplyUpdatesRequired() {
   if (IsControlType(type_)) {
     return false;  // We don't process control types here.
diff --git a/sync/engine/directory_update_handler.h b/sync/engine/directory_update_handler.h
index 246d412..df94f34 100644
--- a/sync/engine/directory_update_handler.h
+++ b/sync/engine/directory_update_handler.h
@@ -68,6 +68,11 @@ class SYNC_EXPORT_PRIVATE DirectoryUpdateHandler : public UpdateHandler {
   // Sometimes there is nothing to do, so we can return without doing anything.
   bool IsApplyUpdatesRequired();
 
+  // Called at the end of ApplyUpdates and PassiveApplyUpdates and performs
+  // steps common to both (even when IsApplyUpdatesRequired has returned
+  // false).
+  void PostApplyUpdates();
+
   // Processes the given SyncEntities and stores their data in the directory.
   // Their types must match this update handler's type.
   void UpdateSyncEntities(
diff --git a/sync/engine/directory_update_handler_unittest.cc b/sync/engine/directory_update_handler_unittest.cc
index 9579e93..2a25f16 100644
--- a/sync/engine/directory_update_handler_unittest.cc
+++ b/sync/engine/directory_update_handler_unittest.cc
@@ -86,9 +86,7 @@ class DirectoryUpdateHandlerProcessUpdateTest : public ::testing::Test {
   }
 
   bool TypeRootExists(ModelType model_type) {
-    syncable::ReadTransaction trans(FROM_HERE, dir());
-    syncable::Entry e(&trans, syncable::GET_TYPE_ROOT, model_type);
-    return e.good() && !e.GetIsDel();
+    return dir()->InitialSyncEndedForType(model_type);
   }
 
  protected:
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index c630fe4..90d9e14 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -4336,6 +4336,8 @@ TEST_F(SyncerTest, ClientTagUpdateClashesWithLocalEntry) {
     tag2_metahandle = tag2.GetMetahandle();
 
     // Preferences type root should have been created by the updates above.
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
 
@@ -4378,6 +4380,8 @@ TEST_F(SyncerTest, ClientTagUpdateClashesWithLocalEntry) {
     EXPECT_EQ(tag2_metahandle, tag2.GetMetahandle());
 
     // Preferences type root should have been created by the updates above.
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
 
@@ -4460,6 +4464,8 @@ TEST_F(SyncerTest, ClientTagClashWithinBatchOfUpdates) {
     EXPECT_EQ("tag c", tag_c.GetUniqueClientTag());
 
     // Preferences type root should have been created by the updates above.
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
 
@@ -4483,6 +4489,9 @@ TEST_F(SyncerTest, EntryWithParentIdUpdatedWithEntryWithoutParentId) {
     // 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());
+
+    ASSERT_TRUE(directory()->InitialSyncEndedForType(&trans, PREFERENCES));
+
     Entry pref_root(&trans, GET_TYPE_ROOT, PREFERENCES);
     ASSERT_TRUE(pref_root.good());
     pref_root_id = pref_root.GetId();
@@ -4675,17 +4684,18 @@ TEST_F(SyncerTest, ConfigureDownloadsTwoBatchesSuccess) {
   syncable::Id node2 = ids_.NewServerId();
 
   // Construct the first GetUpdates response.
-  mock_server_->AddUpdateDirectory(node1, ids_.root(), "one", 1, 10,
-                                   foreign_cache_guid(), "-2");
+  mock_server_->AddUpdatePref(node1.GetServerId(), "", "one", 1, 10);
   mock_server_->SetChangesRemaining(1);
   mock_server_->NextUpdateBatch();
 
   // Construct the second GetUpdates response.
-  mock_server_->AddUpdateDirectory(node2, ids_.root(), "two", 1, 20,
-                                   foreign_cache_guid(), "-2");
+  mock_server_->AddUpdatePref(node2.GetServerId(), "", "two", 2, 20);
 
   SyncShareConfigure();
 
+  // The type should now be marked as having the initial sync completed.
+  EXPECT_TRUE(directory()->InitialSyncEndedForType(PREFERENCES));
+
   syncable::ReadTransaction trans(FROM_HERE, directory());
   // Both nodes should be downloaded and applied.
 
@@ -4710,17 +4720,18 @@ TEST_F(SyncerTest, ConfigureFailsDontApplyUpdates) {
   mock_server_->FailNthPostBufferToPathCall(2);
 
   // Construct the first GetUpdates response.
-  mock_server_->AddUpdateDirectory(node1, ids_.root(), "one", 1, 10,
-                                   foreign_cache_guid(), "-1");
+  mock_server_->AddUpdatePref(node1.GetServerId(), "", "one", 1, 10);
   mock_server_->SetChangesRemaining(1);
   mock_server_->NextUpdateBatch();
 
-  // Consutrct the second GetUpdates response.
-  mock_server_->AddUpdateDirectory(node2, ids_.root(), "two", 1, 20,
-                                   foreign_cache_guid(), "-2");
+  // Construct the second GetUpdates response.
+  mock_server_->AddUpdatePref(node2.GetServerId(), "", "two", 2, 20);
 
   SyncShareConfigure();
 
+  // The type shouldn't be marked as having the initial sync completed.
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(PREFERENCES));
+
   syncable::ReadTransaction trans(FROM_HERE, directory());
 
   // The first node was downloaded, but not applied.
@@ -4767,6 +4778,27 @@ TEST_F(SyncerTest, GetKeyEmpty) {
   }
 }
 
+// Trigger an update that contains a progress marker only and verify that
+// the type's permanent folder is created and the type is marked as having
+// initial sync complete.
+TEST_F(SyncerTest, ProgressMarkerOnlyUpdateCreatesRootFolder) {
+  EXPECT_FALSE(directory()->InitialSyncEndedForType(PREFERENCES));
+  sync_pb::DataTypeProgressMarker* marker =
+      mock_server_->AddUpdateProgressMarker();
+  marker->set_data_type_id(GetSpecificsFieldNumberFromModelType(PREFERENCES));
+  marker->set_token("foobar");
+
+  SyncShareNudge();
+
+  {
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+    syncable::Entry root(&trans, syncable::GET_TYPE_ROOT, PREFERENCES);
+    EXPECT_TRUE(root.good());
+  }
+
+  EXPECT_TRUE(directory()->InitialSyncEndedForType(PREFERENCES));
+}
+
 // Tests specifically related to bookmark (and therefore no client tags) sync
 // logic. Entities without client tags have custom logic in parts of the code,
 // and hence are not covered by e.g. the Undeletion tests below.
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index 3b735e9..33cb6d4 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -19,6 +19,7 @@
 #include "sync/syncable/entry.h"
 #include "sync/syncable/entry_kernel.h"
 #include "sync/syncable/in_memory_directory_backing_store.h"
+#include "sync/syncable/model_neutral_mutable_entry.h"
 #include "sync/syncable/on_disk_directory_backing_store.h"
 #include "sync/syncable/scoped_kernel_lock.h"
 #include "sync/syncable/scoped_parent_child_index_updater.h"
@@ -985,9 +986,29 @@ bool Directory::InitialSyncEndedForType(ModelType type) {
 
 bool Directory::InitialSyncEndedForType(
     BaseTransaction* trans, ModelType type) {
-  // True iff the type's root node has been created.
-  syncable::Entry entry(trans, syncable::GET_TYPE_ROOT, type);
-  return entry.good();
+  // True iff the type's root node has been created and changes
+  // for the type have been applied at least once.
+  Entry root(trans, GET_TYPE_ROOT, type);
+  return root.good() && root.GetBaseVersion() != CHANGES_VERSION;
+}
+
+void Directory::MarkInitialSyncEndedForType(BaseWriteTransaction* trans,
+                                            ModelType type) {
+  // If the root folder is downloaded for the server, the root's base version
+  // get updated automatically at the end of update cycle when the update gets
+  // applied. However if this is a type with client generated root, the root
+  // node gets created locally and never goes through the update cycle. In that
+  // case its base version has to be explictly changed from CHANGES_VERSION
+  // at the end of the initial update cycle to mark the type as downloaded.
+  // See Directory::InitialSyncEndedForType
+  DCHECK(IsTypeWithClientGeneratedRoot(type));
+  ModelNeutralMutableEntry root(trans, GET_TYPE_ROOT, type);
+
+  // Some tests don't bother creating type root. Need to check if the root
+  // exists before clearing its base version.
+  if (root.good() && root.GetBaseVersion() == CHANGES_VERSION) {
+    root.PutBaseVersion(0);
+  }
 }
 
 string Directory::store_birthday() const {
diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h
index d71fcf8..7402697 100644
--- a/sync/syncable/directory.h
+++ b/sync/syncable/directory.h
@@ -300,8 +300,16 @@ class SYNC_EXPORT Directory {
                           ModelType type,
                           const sync_pb::DataTypeContext& context);
 
+  // Returns types for which the initial sync has ended.
   ModelTypeSet InitialSyncEndedTypes();
+
+  // Returns true if the initial sync for |type| has completed.
   bool InitialSyncEndedForType(ModelType type);
+  bool InitialSyncEndedForType(BaseTransaction* trans, ModelType type);
+
+  // Marks the |type| as having its intial sync complete.
+  // This applies only to types with implicitly created root folders.
+  void MarkInitialSyncEndedForType(BaseWriteTransaction* trans, ModelType type);
 
   // (Account) Store birthday is opaque to the client, so we keep it in the
   // format it is in the proto buffer in case we switch to a binary birthday
@@ -620,9 +628,6 @@ class SYNC_EXPORT Directory {
   // detected.
   void OnCatastrophicError();
 
-  // Returns true if the initial sync for |type| has completed.
-  bool InitialSyncEndedForType(BaseTransaction* trans, ModelType type);
-
   // Stops sending events to the delegate and the transaction
   // observer.
   void Close();
diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc
index 198e534..a9d8058 100644
--- a/sync/syncable/directory_unittest.cc
+++ b/sync/syncable/directory_unittest.cc
@@ -2110,6 +2110,28 @@ TEST_F(SyncableDirectoryTest, SharingOfClientAndServerSpecifics) {
                                            item.GetBaseServerSpecifics()));
 }
 
+// Tests checking and marking a type as having its initial sync completed.
+TEST_F(SyncableDirectoryTest, InitialSyncEndedForType) {
+  // Not completed if there is no root node.
+  EXPECT_FALSE(dir()->InitialSyncEndedForType(PREFERENCES));
+
+  WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+  // Create the root node.
+  ModelNeutralMutableEntry entry(&trans, syncable::CREATE_NEW_TYPE_ROOT,
+                                 PREFERENCES);
+  DCHECK(entry.good());
+
+  entry.PutServerIsDir(true);
+  entry.PutUniqueServerTag(ModelTypeToRootTag(PREFERENCES));
+
+  // Should still be marked as incomplete.
+  EXPECT_FALSE(dir()->InitialSyncEndedForType(&trans, PREFERENCES));
+
+  // Mark as complete and verify.
+  dir()->MarkInitialSyncEndedForType(&trans, PREFERENCES);
+  EXPECT_TRUE(dir()->InitialSyncEndedForType(&trans, PREFERENCES));
+}
+
 }  // namespace syncable
 
 }  // namespace syncer
diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc
index cb07db4..42e3ec7 100644
--- a/sync/test/engine/mock_connection_manager.cc
+++ b/sync/test/engine/mock_connection_manager.cc
@@ -499,11 +499,15 @@ void MockConnectionManager::SetNewTimestamp(int ts) {
   ApplyToken();
 }
 
+sync_pb::DataTypeProgressMarker*
+MockConnectionManager::AddUpdateProgressMarker() {
+  return GetUpdateResponse()->add_new_progress_marker();
+}
+
 void MockConnectionManager::ApplyToken() {
   if (!update_queue_.empty()) {
     GetUpdateResponse()->clear_new_progress_marker();
-    sync_pb::DataTypeProgressMarker* new_marker =
-        GetUpdateResponse()->add_new_progress_marker();
+    sync_pb::DataTypeProgressMarker* new_marker = AddUpdateProgressMarker();
     new_marker->set_data_type_id(-1);  // Invalid -- clients shouldn't see.
     new_marker->set_token(next_token_);
   }
diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h
index c09cf87..8bcc5c2 100644
--- a/sync/test/engine/mock_connection_manager.h
+++ b/sync/test/engine/mock_connection_manager.h
@@ -269,6 +269,9 @@ class MockConnectionManager : public ServerConnectionManager {
   // Locate the most recent update message for purpose of alteration.
   sync_pb::SyncEntity* GetMutableLastUpdate();
 
+  // Adds a new progress marker to the last update.
+  sync_pb::DataTypeProgressMarker* AddUpdateProgressMarker();
+
  private:
   sync_pb::SyncEntity* AddUpdateFull(syncable::Id id,
                                      syncable::Id parentid,