Remove dependency on server generated type root folders

sync/syncable/directory.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/syncable/directory.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/base64.h"
@@ skipping 730 matching lines @@
 
     EntryKernelSet entries_to_journal;
     STLElementDeleter<EntryKernelSet> journal_deleter(&entries_to_journal);
 
     {
       ScopedKernelLock lock(this);
 
       bool found_progress = false;
       for (ModelTypeSet::Iterator iter = disabled_types.First(); iter.Good();
            iter.Inc()) {
-        if (!kernel_->persisted_info.HasEmptyDownloadProgress(iter.Get()))
+        if (!HasEmptyDownloadProgress(iter.Get()))
           found_progress = true;
       }
 
       // If none of the disabled types have progress markers, there's nothing to
       // purge.
       if (!found_progress)
         return true;
 
       // We iterate in two passes to avoid a bug in STLport (which is used in
       // the Android build).  There are some versions of that library where a
@@ skipping 156 matching lines @@
 }
 
 void Directory::SetDownloadProgress(
     ModelType model_type,
     const sync_pb::DataTypeProgressMarker& new_progress) {
   ScopedKernelLock lock(this);
   kernel_->persisted_info.download_progress[model_type].CopyFrom(new_progress);
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
 }
 
+bool Directory::HasEmptyDownloadProgress(ModelType type) const {
+  return kernel_->persisted_info.HasEmptyDownloadProgress(type);

[maniscalco, 2015/01/24 00:55:59]

Normally, when Directory methods access kernel_ they either take a ScopedLock
(like GetEntriesCount) or they assert the thread already has a lock (like
GetTransactionVersion).  Should this new method follow one of those approaches?

[stanisc, 2015/01/26 19:02:09]

Done.

[maniscalco, 2015/01/26 19:17:34]

Since Directory::HasEmptyDownloadProgress is called from PurgeEntriesWithTypeIn
while a ScopedKernelLock on "this" is held, I'm thinking
Directory::HasEmptyDownloadProgress shouldn't take a lock because SKLs are
non-recursive, right?  As is, I would think the code might DCHECK:

https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

Unless, maybe I'm missing something?

Thinking more about this... WDYT about adding const ScopedKernelLock& to
Directory::HasEmptyDownloadProgress ?  We do that elsewhere in Directory to
ensure the caller is holding a lock (e.g. ClearDirtyMetahandles).  That way,
it's harder to misuse.

[stanisc, 2015/01/29 00:27:37]

Directory::HasEmptyDownloadProgress isn't called from PurgeEntriesWithTypeIn
anymore. I changed that code back to calling
Directory::PersistedKernelInfo::HasEmptyDownloadProgress directly to avoid
exactly the problem you described. I did it at the same time when I added the
lock to Directory::HasEmptyDownloadProgress. In general none of functions in
PersistedKernelInfo get ScopedKernelLock& as one of parameters. I didn't want to
change the existing pattern so I decided it would be better to inline the call
in PurgeEntriesWithTypeIn instead.

[maniscalco, 2015/01/29 00:36:34]

Ah, you're right.  I misread which HasEmptyDownloadProgress was being called
(Directory's vs Directory::PersistedKernelInfo's).

+}
+
 int64 Directory::GetTransactionVersion(ModelType type) const {
   kernel_->transaction_mutex.AssertAcquired();
   return kernel_->persisted_info.transaction_version[type];
 }
 
 void Directory::IncrementTransactionVersion(ModelType type) {
   kernel_->transaction_mutex.AssertAcquired();
   kernel_->persisted_info.transaction_version[type]++;
 }
 
 void Directory::GetDataTypeContext(BaseTransaction* trans,
                                    ModelType type,
                                    sync_pb::DataTypeContext* context) const {
   ScopedKernelLock lock(this);
   context->CopyFrom(kernel_->persisted_info.datatype_context[type]);
 }
 
 void Directory::SetDataTypeContext(
     BaseWriteTransaction* trans,
     ModelType type,
     const sync_pb::DataTypeContext& context) {
   ScopedKernelLock lock(this);
   kernel_->persisted_info.datatype_context[type].CopyFrom(context);
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
 }
 
+// TODO: change these to not rely on the folders
 ModelTypeSet Directory::InitialSyncEndedTypes() {
   syncable::ReadTransaction trans(FROM_HERE, this);
   ModelTypeSet protocol_types = ProtocolTypes();
   ModelTypeSet initial_sync_ended_types;
   for (ModelTypeSet::Iterator i = protocol_types.First(); i.Good(); i.Inc()) {
     if (InitialSyncEndedForType(&trans, i.Get())) {
       initial_sync_ended_types.Put(i.Get());
     }
   }
   return initial_sync_ended_types;
@@ skipping 204 matching lines @@
 
     if (id.IsRoot()) {
       if (!SyncAssert(e.GetIsDir(), FROM_HERE,
                       "Entry should be a directory",
                       trans))
         return false;
       if (!SyncAssert(parentid.IsRoot(), FROM_HERE,
                       "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;
     }
 
     if (!e.GetIsDel()) {
       if (!SyncAssert(id != parentid, FROM_HERE,
                       "Id should be different from parent id.",
                       trans))
          return false;
       if (!SyncAssert(!e.GetNonUniqueName().empty(), FROM_HERE,
                       "Non unique name should not be empty.",
                       trans))
         return false;
 
       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,
                           "Parent entry is not valid.", trans))
             return false;
           if (handles.end() == handles.find(parent.GetMetahandle()))
             break;  // Skip further checking if parent was unmodified.
           if (!SyncAssert(parent.GetIsDir(), FROM_HERE,
                           "Parent should be a directory", trans))
             return false;
           if (!SyncAssert(!parent.GetIsDel(), FROM_HERE,
                           "Parent should not have been marked for deletion.",
                           trans))
             return false;
           if (!SyncAssert(handles.end() != handles.find(parent.GetMetahandle()),
                           FROM_HERE, "Parent should be in the index.", trans))
             return false;
           parentid = parent.GetParentId();
           if (!SyncAssert(--safety_count > 0, FROM_HERE,
                           "Count should be greater than zero.", trans))
             return false;
         }
       }
     }
     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.",
+                          "The entry should have been deleted.",
                           trans))
             return false;
         }
         // It came from the server, so it must have a server ID.
         if (!SyncAssert(id.ServerKnows(), FROM_HERE,
                         "The id should be from a server.",
                         trans))
           return false;
       } else {
         if (e.GetIsDir()) {
           // TODO(chron): Implement this mode if clients ever need it.
           // For now, you can't combine a client tag and a directory.
           if (!SyncAssert(!using_unique_client_tag, FROM_HERE,
                           "Directory cannot have a client tag.",
                           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
         // is an unsynced create or a sucessful delete in the local copy.
         // Either way, that's a mismatch.
         if (!SyncAssert(0 == server_version, FROM_HERE,
                         "Server version should be zero.",
                         trans))
           return false;
         // Items that aren't using the unique client tag should have a zero
@@ skipping 191 matching lines @@
 void Directory::AppendChildHandles(const ScopedKernelLock& lock,
                                    const Id& parent_id,
                                    Directory::Metahandles* result) {
   const OrderedChildSet* children =
       kernel_->parent_child_index.GetChildren(parent_id);
   if (!children)
     return;
 
   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));
   }
 }
 
 void Directory::UnmarkDirtyEntry(WriteTransaction* trans, Entry* entry) {
   CHECK(trans);
   entry->kernel_->clear_dirty(&kernel_->dirty_metahandles);
 }
 
 void Directory::GetAttachmentIdsToUpload(BaseTransaction* trans,
@@ skipping 45 matching lines @@
   // TODO(maniscalco): Eliminate redundant metadata storage (bug 415203).
   std::set_difference(not_on_server_id_set.begin(),
                       not_on_server_id_set.end(),
                       on_server_id_set.begin(),
                       on_server_id_set.end(),
                       std::inserter(*id_set, id_set->end()));
 }
 
 }  // namespace syncable
 }  // namespace syncer