Remove dependency on server generated type root folders

This change prepares the client to deal with implicit permanent folders
in Sync server updates. That includes the following:
- Expect type root folders to not come from the server on the initial
  sync. The client auto-creates type root folders for all types except
  Bookmarks and Nigori when progress marker changes from empty to
  non-empty. These folders are created as local nodes (with client IDs)
  that aren't expected to sync back to the server.
- Expect empty parent IDs in updates for both new and existing items.
- Because the client code updates first, the client must expect server
  to override locally created type root folders. To enable that the
  directory update code that matches update entities to local entities
  was updated to look at server unique tags in addition to client
  unique tags. Later when the server stops generating folders we
  should be able to remove that code.
- Added some extra special cases in Directory::CheckTreeInvariants to
  deal with client side created type root folders.
- Added / modified a few tests to cover cases with implicit parent IDs
  in updates.

BUG=438313

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

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

sync/engine/directory_update_handler.cc

@@ -9,6 +9,7 @@
 #include "sync/engine/update_applicator.h"
 #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_model_neutral_write_transaction.h"
 #include "sync/syncable/syncable_write_transaction.h"
 
@@ -65,16 +66,42 @@ SyncerError DirectoryUpdateHandler::ProcessGetUpdatesResponse(
     }
   }
 
+  // Auto-create permanent folder for the type if the progress marker
+  // changes from empty to non-empty.
+  if (!IsTypeWithServerGeneratedRoot(type_) &&
+      dir_->HasEmptyDownloadProgress(type_) &&
+      IsValidProgressMarker(progress_marker)) {
+    CreateTypeRoot(&trans);
+  }
+
   UpdateSyncEntities(&trans, applicable_updates, status);
 
   if (IsValidProgressMarker(progress_marker)) {
     ExpireEntriesIfNeeded(&trans, progress_marker);
     UpdateProgressMarker(progress_marker);
   }
+
   debug_info_emitter_->EmitUpdateCountersUpdate();
   return SYNCER_OK;
 }
 
+void DirectoryUpdateHandler::CreateTypeRoot(
+    syncable::ModelNeutralWriteTransaction* trans) {
+  syncable::ModelNeutralMutableEntry entry(
+      trans, syncable::CREATE_NEW_TYPE_ROOT, type_);
+  if (!entry.good()) {
+    // This will fail only if matching entry already exists, for example
+    // if the type gets disabled and its progress marker gets cleared,
+    // then the type gets re-enabled again.
+    DVLOG(1) << "Type root folder " << ModelTypeToRootTag(type_)
+             << " already exists.";
+    return;
+  }
+
+  entry.PutServerIsDir(true);
+  entry.PutUniqueServerTag(ModelTypeToRootTag(type_));
+}
+
 void DirectoryUpdateHandler::ApplyUpdates(sessions::StatusController* status) {
   if (!IsApplyUpdatesRequired()) {
     return;
@@ -205,6 +232,9 @@ void DirectoryUpdateHandler::UpdateSyncEntities(
 
 bool DirectoryUpdateHandler::IsValidProgressMarker(
     const sync_pb::DataTypeProgressMarker& progress_marker) const {
+  if (progress_marker.token().empty()) {
+    return false;
+  }
   int field_number = progress_marker.data_type_id();
   ModelType model_type = GetModelTypeFromSpecificsFieldNumber(field_number);
   if (!IsRealDataType(model_type) || type_ != model_type) {

sync/engine/directory_update_handler.h

@@ -91,6 +91,9 @@ class SYNC_EXPORT_PRIVATE DirectoryUpdateHandler : public UpdateHandler {
   // Skips all checks and goes straight to applying the updates.
   SyncerError ApplyUpdatesImpl(sessions::StatusController* status);
 
+  // Creates root node for the handled model type.
+  void CreateTypeRoot(syncable::ModelNeutralWriteTransaction* trans);
+
   syncable::Directory* dir_;
   ModelType type_;
   scoped_refptr<ModelSafeWorker> worker_;

sync/engine/directory_update_handler_unittest.cc

@@ -84,6 +84,12 @@ class DirectoryUpdateHandlerProcessUpdateTest : public ::testing::Test {
     return e.good() && !e.GetIsDel();
   }
 
+  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();
+  }
+
  protected:
   // Used in the construction of DirectoryTypeDebugInfoEmitters.
   ObserverList<TypeDebugInfoObserver> type_observers_;
@@ -267,27 +273,17 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, GarbageCollectionByVersion) {
   context.set_context("context");
   context.set_version(1);
 
-  scoped_ptr<sync_pb::SyncEntity> type_root =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("root")),
-                   Id::GetRoot().GetServerId(), SYNCED_NOTIFICATIONS);
-  type_root->set_server_defined_unique_tag(
-      ModelTypeToRootTag(SYNCED_NOTIFICATIONS));
-  type_root->set_folder(true);
-
   scoped_ptr<sync_pb::SyncEntity> e1 =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")),
-                   type_root->id_string(),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")), "",
                    SYNCED_NOTIFICATIONS);
 
   scoped_ptr<sync_pb::SyncEntity> e2 =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e2")),
-                   type_root->id_string(),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e2")), "",
                    SYNCED_NOTIFICATIONS);
   e2->set_version(kDefaultVersion + 100);
 
   // Add to the applicable updates list.
   SyncEntityList updates;
-  updates.push_back(type_root.get());
   updates.push_back(e1.get());
   updates.push_back(e2.get());
 
@@ -298,7 +294,7 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, GarbageCollectionByVersion) {
   handler.ApplyUpdates(&status);
 
   // Verify none is deleted because they are unapplied during GC.
-  EXPECT_TRUE(EntryExists(type_root->id_string()));
+  EXPECT_TRUE(TypeRootExists(SYNCED_NOTIFICATIONS));
   EXPECT_TRUE(EntryExists(e1->id_string()));
   EXPECT_TRUE(EntryExists(e2->id_string()));
 
@@ -308,7 +304,6 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, GarbageCollectionByVersion) {
             handler.ProcessGetUpdatesResponse(
                 progress, context, SyncEntityList(), &status));
   handler.ApplyUpdates(&status);
-  EXPECT_TRUE(EntryExists(type_root->id_string()));
   EXPECT_FALSE(EntryExists(e1->id_string()));
   EXPECT_TRUE(EntryExists(e2->id_string()));
 }
@@ -330,19 +325,11 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ContextVersion) {
   old_context.set_context("data");
   old_context.set_data_type_id(field_number);
 
-  scoped_ptr<sync_pb::SyncEntity> type_root =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("root")),
-                   Id::GetRoot().GetServerId(), SYNCED_NOTIFICATIONS);
-  type_root->set_server_defined_unique_tag(
-      ModelTypeToRootTag(SYNCED_NOTIFICATIONS));
-  type_root->set_folder(true);
   scoped_ptr<sync_pb::SyncEntity> e1 =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")),
-                   type_root->id_string(),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")), "",
                    SYNCED_NOTIFICATIONS);
 
   SyncEntityList updates;
-  updates.push_back(type_root.get());
   updates.push_back(e1.get());
 
   // The first response should be processed fine.
@@ -351,7 +338,9 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ContextVersion) {
                 progress, old_context, updates, &status));
   handler.ApplyUpdates(&status);
 
-  EXPECT_TRUE(EntryExists(type_root->id_string()));
+  // The PREFERENCES root should be auto-created.
+  EXPECT_TRUE(TypeRootExists(SYNCED_NOTIFICATIONS));
+
   EXPECT_TRUE(EntryExists(e1->id_string()));
 
   {
@@ -368,8 +357,7 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ContextVersion) {
   new_context.set_data_type_id(field_number);
 
   scoped_ptr<sync_pb::SyncEntity> e2 =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e2")),
-                   type_root->id_string(),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e2")), "",
                    SYNCED_NOTIFICATIONS);
   updates.clear();
   updates.push_back(e2.get());
@@ -410,21 +398,12 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest,
   context.set_context("context");
   context.set_version(1);
 
-  scoped_ptr<sync_pb::SyncEntity> type_root =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("root")),
-                   Id::GetRoot().GetServerId(), ARTICLES);
-  type_root->set_server_defined_unique_tag(ModelTypeToRootTag(ARTICLES));
-  type_root->set_folder(true);
-
-  scoped_ptr<sync_pb::SyncEntity> e1 =
-      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")),
-                   type_root->id_string(),
-                   ARTICLES);
+  scoped_ptr<sync_pb::SyncEntity> e1 = CreateUpdate(
+      SyncableIdToProto(Id::CreateFromServerId("e1")), "", ARTICLES);
   sync_pb::AttachmentIdProto* attachment_id = e1->add_attachment_id();
   *attachment_id = CreateAttachmentIdProto();
 
   SyncEntityList updates;
-  updates.push_back(type_root.get());
   updates.push_back(e1.get());
 
   // Process and apply updates.
@@ -433,7 +412,7 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest,
       handler.ProcessGetUpdatesResponse(progress, context, updates, &status));
   handler.ApplyUpdates(&status);
 
-  ASSERT_TRUE(EntryExists(type_root->id_string()));
+  ASSERT_TRUE(TypeRootExists(ARTICLES));
   ASSERT_TRUE(EntryExists(e1->id_string()));
   {
     syncable::ReadTransaction trans(FROM_HERE, dir());

sync/engine/syncer_util.cc

@@ -184,7 +184,23 @@ syncable::Id FindLocalIdToUpdate(
 
       return local_entry.GetId();
     }
+  } else if (update.has_server_defined_unique_tag() &&
+             !update.server_defined_unique_tag().empty()) {
+    // The client creates type root folders with a local ID on demand when a
+    // progress marker for the given type is initially set.
+    // The server might also attempt to send a type root folder for the same
+    // type (during the transition period until support for root folders is
+    // removed for new client versions).
+    // TODO(stanisc): crbug.com/438313: remove this once the transition to
+    // implicit root folders on the server is done.
+    syncable::Entry local_entry(trans, syncable::GET_BY_SERVER_TAG,
+                                update.server_defined_unique_tag());
+    if (local_entry.good() && !local_entry.GetId().ServerKnows()) {
+      DCHECK(local_entry.GetId() != update_id);
+      return local_entry.GetId();
+    }
   }
+
   // Fallback: target an entry having the server ID, creating one if needed.
   return update_id;
 }
@@ -198,6 +214,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
     return SUCCESS;  // No work to do.
   syncable::Id id = entry->GetId();
   const sync_pb::EntitySpecifics& specifics = entry->GetServerSpecifics();
+  ModelType type = GetModelTypeFromSpecifics(specifics);
 
   // Only apply updates that we can decrypt. If we can't decrypt the update, it
   // is likely because the passphrase has not arrived yet. Because the
@@ -227,23 +244,30 @@ UpdateAttemptResponse AttemptToUpdateEntry(
 
   if (!entry->GetServerIsDel()) {
     syncable::Id new_parent = entry->GetServerParentId();
-    Entry parent(trans, GET_BY_ID,  new_parent);
-    // A note on non-directory parents:
-    // We catch most unfixable tree invariant errors at update receipt time,
-    // however we deal with this case here because we may receive the child
-    // first then the illegal parent. Instead of dealing with it twice in
-    // different ways we deal with it once here to reduce the amount of code and
-    // potential errors.
-    if (!parent.good() || parent.GetIsDel() || !parent.GetIsDir()) {
-      DVLOG(1) <<  "Entry has bad parent, returning conflict_hierarchy.";
-      return CONFLICT_HIERARCHY;
-    }
-    if (entry->GetParentId() != new_parent) {
-      if (!entry->GetIsDel() && !IsLegalNewParent(trans, id, new_parent)) {
-        DVLOG(1) << "Not updating item " << id
-                 << ", illegal new parent (would cause loop).";
+    if (!new_parent.IsNull()) {
+      // Perform this step only if the parent is specified.
+      // The unset parent means that the implicit type root would be used.
+      Entry parent(trans, GET_BY_ID, new_parent);
+      // A note on non-directory parents:
+      // We catch most unfixable tree invariant errors at update receipt time,
+      // however we deal with this case here because we may receive the child
+      // first then the illegal parent. Instead of dealing with it twice in
+      // different ways we deal with it once here to reduce the amount of code
+      // and potential errors.
+      if (!parent.good() || parent.GetIsDel() || !parent.GetIsDir()) {
+        DVLOG(1) << "Entry has bad parent, returning conflict_hierarchy.";
         return CONFLICT_HIERARCHY;
       }
+      if (entry->GetParentId() != new_parent) {
+        if (!entry->GetIsDel() && !IsLegalNewParent(trans, id, new_parent)) {
+          DVLOG(1) << "Not updating item " << id
+                   << ", illegal new parent (would cause loop).";
+          return CONFLICT_HIERARCHY;
+        }
+      }
+    } else {
+      // new_parent is unset.
+      DCHECK(!IsTypeWithServerGeneratedRoot(type));
     }
   } else if (entry->GetIsDir()) {
     Directory::Metahandles handles;

sync/internal_api/public/base/model_type.h

@@ -324,6 +324,10 @@ SYNC_EXPORT bool IsProxyType(ModelType model_type);
 // TODO(haitaol): Make entries of act-once data types immutable.
 SYNC_EXPORT bool IsActOnceDataType(ModelType model_type);
 
+// Returns true if |model_type| requires its root folder to be explicitly
+// created on the server during initial sync.
+SYNC_EXPORT bool IsTypeWithServerGeneratedRoot(ModelType model_type);
+
 // Returns set of model types that should be backed up before first sync.
 SYNC_EXPORT ModelTypeSet BackupTypes();
 

sync/protocol/sync.proto

@@ -707,7 +707,7 @@ message DataTypeContext {
 
 message ClientToServerMessage {
   required string share = 1;
-  optional int32 protocol_version = 2 [default = 34];
+  optional int32 protocol_version = 2 [default = 42];
   enum Contents {
     COMMIT = 1;
     GET_UPDATES = 2;

sync/syncable/directory.cc

@@ -925,6 +925,11 @@ void Directory::SetDownloadProgress(
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
 }
 
+bool Directory::HasEmptyDownloadProgress(ModelType type) const {
+  ScopedKernelLock lock(this);
+  return kernel_->persisted_info.HasEmptyDownloadProgress(type);
+}
+
 int64 Directory::GetTransactionVersion(ModelType type) const {
   kernel_->transaction_mutex.AssertAcquired();
   return kernel_->persisted_info.transaction_version[type];
@@ -951,6 +956,7 @@ void Directory::SetDataTypeContext(
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
 }
 
+// TODO(stanisc): crbug.com/438313: change these to not rely on the folders.
 ModelTypeSet Directory::InitialSyncEndedTypes() {
   syncable::ReadTransaction trans(FROM_HERE, this);
   ModelTypeSet protocol_types = ProtocolTypes();
@@ -1175,8 +1181,7 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
                       "Entry should be root",
                       trans))
          return false;
-      if (!SyncAssert(!e.GetIsUnsynced(), FROM_HERE,
-                      "Entry should be sycned",
+      if (!SyncAssert(!e.GetIsUnsynced(), FROM_HERE, "Entry should be synced",
                       trans))
          return false;
       continue;
@@ -1194,7 +1199,6 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
 
       if (!parentid.IsNull()) {
         int safety_count = handles.size() + 1;
-        // TODO(stanisc): handle items with Null parentid
         while (!parentid.IsRoot()) {
           Entry parent(trans, GET_BY_ID, parentid);
           if (!SyncAssert(parent.good(), FROM_HERE,
@@ -1222,14 +1226,16 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
     int64 base_version = e.GetBaseVersion();
     int64 server_version = e.GetServerVersion();
     bool using_unique_client_tag = !e.GetUniqueClientTag().empty();
+    bool is_type_root_folder =
+        parentid.IsRoot() &&
+        e.GetUniqueServerTag() == ModelTypeToRootTag(e.GetModelType());
     if (CHANGES_VERSION == base_version || 0 == base_version) {
       if (e.GetIsUnappliedUpdate()) {
         // Must be a new item, or a de-duplicated unique client tag
         // that was created both locally and remotely.
-        if (!using_unique_client_tag) {
+        if (!(using_unique_client_tag || is_type_root_folder)) {
           if (!SyncAssert(e.GetIsDel(), FROM_HERE,
-                          "The entry should not have been deleted.",
-                          trans))
+                          "The entry should have been deleted.", trans))
             return false;
         }
         // It came from the server, so it must have a server ID.
@@ -1246,12 +1252,26 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
                           trans))
             return false;
         }
-        // Should be an uncomitted item, or a successfully deleted one.
-        if (!e.GetIsDel()) {
-          if (!SyncAssert(e.GetIsUnsynced(), FROM_HERE,
-                          "The item should be unsynced.",
-                          trans))
+        if (is_type_root_folder) {
+          // This must be a locally created type root folder
+          if (!SyncAssert(
+                  !e.GetIsUnsynced(), FROM_HERE,
+                  "Locally created type root folders should not be unsynced.",
+                  trans))
+            return false;
+
+          if (!SyncAssert(
+                  !e.GetIsDel(), FROM_HERE,
+                  "Locally created type root folders should not be deleted.",
+                  trans))
             return false;
+        } else {
+          // Should be an uncomitted item, or a successfully deleted one.
+          if (!e.GetIsDel()) {
+            if (!SyncAssert(e.GetIsUnsynced(), FROM_HERE,
+                            "The item should be unsynced.", trans))
+              return false;
+          }
         }
         // If the next check failed, it would imply that an item exists
         // on the server, isn't waiting for application locally, but either
@@ -1463,7 +1483,6 @@ void Directory::AppendChildHandles(const ScopedKernelLock& lock,
 
   for (OrderedChildSet::const_iterator i = children->begin();
        i != children->end(); ++i) {
-    DCHECK_EQ(parent_id, (*i)->ref(PARENT_ID));
     result->push_back((*i)->ref(META_HANDLE));
   }
 }

sync/syncable/directory.h

@@ -199,10 +199,13 @@ class SYNC_EXPORT Directory {
   void GetDownloadProgressAsString(
       ModelType type,
       std::string* value_out) const;
-  size_t GetEntriesCount() const;
   void SetDownloadProgress(
       ModelType type,
       const sync_pb::DataTypeProgressMarker& value);
+  bool HasEmptyDownloadProgress(ModelType type) const;
+
+  // Gets the total number of entries in the directory.
+  size_t GetEntriesCount() const;
 
   // Gets/Increments transaction version of a model type. Must be called when
   // holding kernel mutex.

sync/syncable/directory_unittest.cc

@@ -1044,6 +1044,56 @@ TEST_F(SyncableDirectoryTest, ChangeEntryIDAndUpdateChildren_ParentAndChild) {
   EXPECT_EQ(OPENED, SimulateSaveAndReloadDir());
 }
 
+// A test that roughly mimics the directory interaction that occurs when a
+// type root folder is created locally and then re-created (updated) from the
+// server.
+TEST_F(SyncableDirectoryTest, ChangeEntryIDAndUpdateChildren_ImplicitParent) {
+  TestIdFactory id_factory;
+  Id orig_parent_id;
+  Id child_id;
+
+  {
+    // Create two client-side items, a parent and child.
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+
+    MutableEntry parent(&trans, CREATE, PREFERENCES, id_factory.root(),
+                        "parent");
+    parent.PutIsDir(true);
+    parent.PutIsUnsynced(true);
+
+    // The child has unset parent ID. The parent is inferred from the type.
+    MutableEntry child(&trans, CREATE, PREFERENCES, "child");
+    child.PutIsUnsynced(true);
+
+    orig_parent_id = parent.GetId();
+    child_id = child.GetId();
+  }
+
+  {
+    // Simulate what happens after committing two items.  Their IDs will be
+    // replaced with server IDs.  The child is renamed first, then the parent.
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+
+    MutableEntry parent(&trans, GET_BY_ID, orig_parent_id);
+
+    ChangeEntryIDAndUpdateChildren(&trans, &parent, id_factory.NewServerId());
+    parent.PutIsUnsynced(false);
+    parent.PutBaseVersion(1);
+    parent.PutServerVersion(1);
+  }
+
+  // Final check for validity.
+  EXPECT_EQ(OPENED, SimulateSaveAndReloadDir());
+
+  // Verify that child's PARENT_ID hasn't been updated.
+  {
+    ReadTransaction trans(FROM_HERE, dir().get());
+    Entry child(&trans, GET_BY_ID, child_id);
+    EXPECT_TRUE(child.good());
+    EXPECT_TRUE(child.GetParentId().IsNull());
+  }
+}
+
 // A test based on the scenario where we create a bookmark folder and entry
 // locally, but with a twist.  In this case, the bookmark is deleted before we
 // are able to sync either it or its parent folder.  This scenario used to cause

sync/syncable/entry_kernel.cc

@@ -57,7 +57,7 @@ bool EntryKernel::ShouldMaintainPosition() const {
 }
 
 bool EntryKernel::ShouldMaintainHierarchy() const {
-  // We maintain hierarchy for bookmarks, device info, and top-level folders,
+  // We maintain hierarchy for bookmarks and top-level folders,
   // but no other types.  Note that the Nigori node consists of a single
   // top-level folder, so it's included in this set.
   return (GetModelTypeFromSpecifics(ref(SPECIFICS)) == syncer::BOOKMARKS)

sync/syncable/entry_kernel.h

@@ -247,7 +247,6 @@ struct SYNC_EXPORT_PRIVATE EntryKernel {
         ProtoTimeToTime(TimeToProtoTime(value));
   }
   inline void put(IdField field, const Id& value) {
-    DCHECK(!value.IsNull());
     id_fields[field - ID_FIELDS_BEGIN] = value;
   }
   inline void put(BaseVersion field, int64 value) {

sync/syncable/model_neutral_mutable_entry.cc

@@ -44,6 +44,42 @@ ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,
   kernel_ = kernel.release();
 }
 
+ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,
+                                                   CreateNewTypeRoot,
+                                                   ModelType type)
+    : Entry(trans), base_write_transaction_(trans) {
+  DCHECK(!IsTypeWithServerGeneratedRoot(type));
+  Entry same_type_root(trans, GET_TYPE_ROOT, type);
+  kernel_ = NULL;
+  if (same_type_root.good()) {
+    return;  // already have a type root for the given type
+  }
+
+  scoped_ptr<EntryKernel> kernel(new EntryKernel());
+
+  sync_pb::EntitySpecifics specifics;
+  AddDefaultFieldValue(type, &specifics);
+  kernel->put(SPECIFICS, specifics);
+
+  kernel->put(ID,
+              syncable::Id::CreateFromClientString(ModelTypeToString(type)));
+  kernel->put(META_HANDLE, trans->directory()->NextMetahandle());
+  kernel->put(PARENT_ID, syncable::Id::GetRoot());
+  kernel->put(BASE_VERSION, CHANGES_VERSION);
+  kernel->put(NON_UNIQUE_NAME, ModelTypeToString(type));
+  kernel->put(IS_DIR, true);
+
+  kernel->mark_dirty(&trans->directory()->kernel_->dirty_metahandles);
+
+  if (!trans->directory()->InsertEntry(trans, kernel.get())) {
+    return;  // Failed inserting.
+  }
+
+  trans->TrackChangesTo(kernel.get());
+
+  kernel_ = kernel.release();
+}
+
 ModelNeutralMutableEntry::ModelNeutralMutableEntry(
     BaseWriteTransaction* trans, GetById, const Id& id)
     : Entry(trans, GET_BY_ID, id), base_write_transaction_(trans) {

sync/syncable/model_neutral_mutable_entry.h

@@ -20,6 +20,8 @@ enum CreateNewUpdateItem {
   CREATE_NEW_UPDATE_ITEM
 };
 
+enum CreateNewTypeRoot { CREATE_NEW_TYPE_ROOT };
+
 // This Entry includes all the operations one can safely perform on the sync
 // thread.  In particular, it does not expose setters to make changes that need
 // to be communicated to the model (and the model's thread).  It is not possible
@@ -30,6 +32,9 @@ class SYNC_EXPORT_PRIVATE ModelNeutralMutableEntry : public Entry {
   ModelNeutralMutableEntry(BaseWriteTransaction* trans,
                            CreateNewUpdateItem,
                            const Id& id);
+  ModelNeutralMutableEntry(BaseWriteTransaction* trans,
+                           CreateNewTypeRoot,
+                           ModelType type);
   ModelNeutralMutableEntry(BaseWriteTransaction* trans, GetByHandle, int64);
   ModelNeutralMutableEntry(BaseWriteTransaction* trans, GetById, const Id&);
   ModelNeutralMutableEntry(

sync/syncable/model_type.cc

@@ -1156,4 +1156,8 @@ bool IsActOnceDataType(ModelType model_type) {
   return model_type == HISTORY_DELETE_DIRECTIVES;
 }
 
+bool IsTypeWithServerGeneratedRoot(ModelType model_type) {
+  return model_type == BOOKMARKS || model_type == NIGORI;
+}
+
 }  // namespace syncer

sync/syncable/syncable_id.cc

@@ -35,19 +35,23 @@ string Id::GetServerId() const {
 
 Id Id::CreateFromServerId(const string& server_id) {
   Id id;
-  if (server_id == "0")
-    id.s_ = "r";
-  else
-    id.s_ = string("s") + server_id;
+  if (!server_id.empty()) {
+    if (server_id == "0")
+      id.s_ = "r";
+    else
+      id.s_ = string("s") + server_id;
+  }
   return id;
 }
 
 Id Id::CreateFromClientString(const string& local_id) {
   Id id;
-  if (local_id == "0")
-    id.s_ = "r";
-  else
-    id.s_ = string("c") + local_id;
+  if (!local_id.empty()) {
+    if (local_id == "0")
+      id.s_ = "r";
+    else
+      id.s_ = string("c") + local_id;
+  }
   return id;
 }
 

sync/syncable/syncable_util.cc

@@ -28,6 +28,8 @@ int GetUnsyncedEntries(BaseTransaction* trans,
 
 bool IsLegalNewParent(BaseTransaction* trans, const Id& entry_id,
                       const Id& new_parent_id) {
+  DCHECK(!entry_id.IsNull());
+  DCHECK(!new_parent_id.IsNull());
   if (entry_id.IsRoot())
     return false;
   // we have to ensure that the entry is not an ancestor of the new parent.
@@ -66,11 +68,14 @@ void ChangeEntryIDAndUpdateChildren(
     while (i != children.end()) {
       ModelNeutralMutableEntry child_entry(trans, GET_BY_HANDLE, *i++);
       CHECK(child_entry.good());
-      // Use the unchecked setter here to avoid touching the child's
-      // UNIQUE_POSITION field.  In this case, UNIQUE_POSITION among the
-      // children will be valid after the loop, since we update all the children
-      // at once.
-      child_entry.PutParentIdPropertyOnly(new_id);
+      // Change the parent ID of the entry unless it was unset (implicit)
+      if (!child_entry.GetParentId().IsNull()) {
+        // Use the unchecked setter here to avoid touching the child's
+        // UNIQUE_POSITION field.  In this case, UNIQUE_POSITION among the
+        // children will be valid after the loop, since we update all the
+        // children at once.
+        child_entry.PutParentIdPropertyOnly(new_id);
+      }
     }
   }
 }