Fix GetCommitIds local deletion case resulting in orphaning of directory entries

sync/engine/get_commit_ids.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/engine/get_commit_ids.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <set>
@@ skipping 101 matching lines @@
 bool HasAttachmentNotOnServer(const syncable::Entry& entry) {
   const sync_pb::AttachmentMetadata& metadata = entry.GetAttachmentMetadata();
   for (int i = 0; i < metadata.record_size(); ++i) {
     if (!metadata.record(i).is_on_server()) {
       return true;
     }
   }
   return false;
 }
 
-// An entry is not considered ready for commit if any are true:
-// 1. It's in conflict.
-// 2. It requires encryption (either the type is encrypted but a passphrase
+// An entry may not commit if any are true:
+// 1. It requires encryption (either the type is encrypted but a passphrase
 //    is missing from the cryptographer, or the entry itself wasn't properly
 //    encrypted).
-// 3. It's type is currently throttled.
-// 4. It's a delete but has not been committed.
-bool IsEntryReadyForCommit(ModelTypeSet requested_types,
-                           ModelTypeSet encrypted_types,
-                           bool passphrase_missing,
-                           const syncable::Entry& entry) {
+// 2. It's type is currently throttled.
+// 3. It's a delete but has not been committed.
+bool MayEntryCommit(ModelTypeSet requested_types,
+                    ModelTypeSet encrypted_types,
+                    bool passphrase_missing,
+                    const syncable::Entry& entry) {
   DCHECK(entry.GetIsUnsynced());
-  if (IsEntryInConflict(entry))
-    return false;
 
   const ModelType type = entry.GetModelType();
   // We special case the nigori node because even though it is considered an
   // "encrypted type", not all nigori node changes require valid encryption
   // (ex: sync_tabs).
   if ((type != NIGORI) && encrypted_types.Has(type) &&
       (passphrase_missing ||
        syncable::EntryNeedsEncryption(encrypted_types, entry))) {
     // This entry requires encryption but is not properly encrypted (possibly
     // due to the cryptographer not being initialized or the user hasn't
@@ skipping 33 matching lines @@
     // This entry is not ready to be sent to the server because it has one or
     // more attachments that have not yet been uploaded to the server.  The idea
     // here is avoid propagating an entry with dangling attachment references.
     return false;
   }
 
   DVLOG(2) << "Entry is ready for commit: " << entry;
   return true;
 }
 
+bool SupportsHierarchy(const syncable::Entry& item) {
+  // Types with explicit server supported hierarchy only.
+  return IsTypeWithServerGeneratedRoot(item.GetModelType());
+}
+
+// Excludes ancestors of deleted conflicted items from
+// |ready_unsynced_set|.
+void ExcludeDeletedAncestors(
+    syncable::BaseTransaction* trans,
+    const std::vector<int64_t>& deleted_conflicted_items,
+    std::set<int64_t>* ready_unsynced_set) {
+  for (std::vector<int64_t>::const_iterator iter =

[Nicolas Zea, 2015/12/31 02:12:02]

nit: use auto? (here and elsewhere)

[stanisc, 2016/01/04 22:00:48]

Done.

+           deleted_conflicted_items.begin();
+       iter != deleted_conflicted_items.end(); ++iter) {
+    syncable::Entry item(trans, syncable::GET_BY_HANDLE, *iter);
+    syncable::Id parent_id = item.GetParentId();
+    DCHECK(!parent_id.IsNull());
+
+    while (!parent_id.IsRoot()) {
+      syncable::Entry parent(trans, syncable::GET_BY_ID, parent_id);
+      CHECK(parent.good()) << "Bad user-only parent in item path.";
+      int64_t handle = parent.GetMetahandle();
+
+      if (!parent.GetIsDel())
+        break;
+
+      std::set<int64_t>::iterator ready_iter = ready_unsynced_set->find(handle);
+      if (ready_iter == ready_unsynced_set->end())
+        break;
+
+      // Remove this entry from |ready_unsynced_set|.
+      ready_unsynced_set->erase(ready_iter);
+      parent_id = parent.GetParentId();
+    }
+  }
+}
+
+// Excludes children of items from |conflicted_items| list.

[Nicolas Zea, 2015/12/31 02:12:02]

Mention excluded_items output as well?

[stanisc, 2016/01/04 22:00:48]

Done.

+void ExcludeChildren(syncable::BaseTransaction* trans,
+                     const std::vector<int64_t>& conflicted_items,
+                     std::vector<int64_t>* excluded_items,
+                     std::set<int64_t>* ready_unsynced_set) {
+  for (std::vector<int64_t>::const_iterator iter = conflicted_items.begin();
+       iter != conflicted_items.end(); ++iter) {
+    syncable::Entry entry(trans, syncable::GET_BY_HANDLE, *iter);
+
+    if (!entry.GetIsDir() || entry.GetIsDel())
+      continue;
+
+    std::vector<int64_t> children;
+    entry.GetChildHandles(&children);
+
+    for (std::vector<int64_t>::const_iterator child_iter = children.begin();
+         child_iter != children.end(); ++child_iter) {
+      // Collect all child handles that are in |ready_unsynced_set|.
+      int64_t child_handle = *child_iter;
+      std::set<int64_t>::iterator ready_iter =
+          ready_unsynced_set->find(child_handle);
+      if (ready_iter != ready_unsynced_set->end()) {
+        // Remove this entry from |ready_unsynced_set| and add it
+        // to |excluded_items|.
+        ready_unsynced_set->erase(ready_iter);
+        excluded_items->push_back(child_handle);
+      }
+    }
+  }
+}
+
 // Filters |unsynced_handles| to remove all entries that do not belong to the
 // specified |requested_types|, or are not eligible for a commit at this time.
 void FilterUnreadyEntries(
     syncable::BaseTransaction* trans,
     ModelTypeSet requested_types,
     ModelTypeSet encrypted_types,
     bool passphrase_missing,
     const syncable::Directory::Metahandles& unsynced_handles,
     std::set<int64_t>* ready_unsynced_set) {
+  std::vector<int64_t> deleted_conflicted_items;
+  std::vector<int64_t> conflicted_items;
+
+  // Go over all unsynced handles, filter the ones that might be committed based
+  // on type / encryption, then based on whether they are in conflict add them
+  // to either |ready_unsynced_set| or one of the conflicted lists.
   for (syncable::Directory::Metahandles::const_iterator iter =
        unsynced_handles.begin(); iter != unsynced_handles.end(); ++iter) {
     syncable::Entry entry(trans, syncable::GET_BY_HANDLE, *iter);
     // TODO(maniscalco): While we check if entry is ready to be committed, we
     // also need to check that all of its ancestors (parents, transitive) are
     // ready to be committed.  Once attachments can prevent an entry from being
     // committable, this method must ensure all ancestors are ready for commit
     // (bug 356273).
-    if (IsEntryReadyForCommit(requested_types,
-                              encrypted_types,
-                              passphrase_missing,
-                              entry)) {
-      ready_unsynced_set->insert(*iter);
+    if (MayEntryCommit(requested_types, encrypted_types, passphrase_missing,
+                       entry)) {
+      if (IsEntryInConflict(entry)) {
+        // Conflicting hierarchical entries might prevent their ancestors or
+        // descendants from being committed.
+        if (SupportsHierarchy(entry)) {
+          if (entry.GetIsDel()) {
+            deleted_conflicted_items.push_back(*iter);
+          } else if (entry.GetIsDir()) {
+            // Populate the initial version of |conflicted_items| with folder
+            // items that are in conflict.
+            conflicted_items.push_back(*iter);
+          }
+        }
+      } else {
+        ready_unsynced_set->insert(*iter);
+      }
     }
   }
+
+  // If there are any deleted conflicted entries, remote their deleted ancestors

[Nicolas Zea, 2015/12/31 02:12:03]

nit: remote -> remove

[stanisc, 2016/01/04 22:00:48]

Done.

+  // as well.
+  ExcludeDeletedAncestors(trans, deleted_conflicted_items, ready_unsynced_set);
+
+  // Starting with conflicted_items containing conflicted folders go down and
+  // exclude all descendants from |ready_unsynced_set|.
+  while (!conflicted_items.empty()) {
+    std::vector<int64_t> new_list;
+    ExcludeChildren(trans, conflicted_items, &new_list, ready_unsynced_set);
+    conflicted_items.swap(new_list);
+  }
 }
 
 // This class helps to implement OrderCommitIds().  Its members track the
 // progress of a traversal while its methods extend it.  It can return early if
 // the traversal reaches the desired size before the full traversal is complete.
 class Traversal {
  public:
   Traversal(syncable::BaseTransaction* trans,
             int64_t max_entries,
             syncable::Directory::Metahandles* out);
   ~Traversal();
 
   // First step of traversal building.  Adds non-deleted items in order.
   void AddCreatesAndMoves(const std::set<int64_t>& ready_unsynced_set);
 
   // Second step of traverals building.  Appends deleted items.
   void AddDeletes(const std::set<int64_t>& ready_unsynced_set);
 
  private:
   // The following functions do not modify the traversal directly.  They return
   // their results in the |result| vector instead.
-  bool AddUncommittedParents(const std::set<int64_t>& ready_unsynced_set,
+  void AddUncommittedParents(const std::set<int64_t>& ready_unsynced_set,
                              const syncable::Entry& item,
                              syncable::Directory::Metahandles* result) const;
 
-  void TryAddItem(const std::set<int64_t>& ready_unsynced_set,
+  bool TryAddItem(const std::set<int64_t>& ready_unsynced_set,
                   const syncable::Entry& item,
                   syncable::Directory::Metahandles* result) const;
 
-  bool AddDeletedParents(const std::set<int64_t>& ready_unsynced_set,
+  void AddDeletedParents(const std::set<int64_t>& ready_unsynced_set,
                          const syncable::Entry& item,
                          const syncable::Directory::Metahandles& traversed,
                          syncable::Directory::Metahandles* result) const;
 
-  bool SupportsHierarchy(const syncable::Entry& item) const;
-
   // Returns true if we've collected enough items.
   bool IsFull() const;
 
   // Returns true if the specified handle is already in the traversal.
   bool HaveItem(int64_t handle) const;
 
   // Adds the specified handles to the traversal.
   void AppendManyToTraversal(const syncable::Directory::Metahandles& handles);
 
   // Adds the specifed handle to the traversal.
   void AppendToTraversal(int64_t handle);
 
   syncable::Directory::Metahandles* out_;
   std::set<int64_t> added_handles_;
   const size_t max_entries_;
   syncable::BaseTransaction* trans_;
 
   DISALLOW_COPY_AND_ASSIGN(Traversal);
 };
 
 Traversal::Traversal(syncable::BaseTransaction* trans,
                      int64_t max_entries,
                      syncable::Directory::Metahandles* out)
     : out_(out), max_entries_(max_entries), trans_(trans) {}
 
 Traversal::~Traversal() {}
 
-bool Traversal::AddUncommittedParents(
+void Traversal::AddUncommittedParents(
     const std::set<int64_t>& ready_unsynced_set,
     const syncable::Entry& item,
     syncable::Directory::Metahandles* result) const {
   DCHECK(SupportsHierarchy(item));
   syncable::Directory::Metahandles dependencies;
   syncable::Id parent_id = item.GetParentId();
 
   // Climb the tree adding entries leaf -> root.
   while (!parent_id.ServerKnows()) {
     syncable::Entry parent(trans_, syncable::GET_BY_ID, parent_id);
     CHECK(parent.good()) << "Bad user-only parent in item path.";
     int64_t handle = parent.GetMetahandle();
     if (HaveItem(handle)) {
       // We've already added this parent (and therefore all of its parents).
       // We can return early.
       break;
     }
-    if (IsEntryInConflict(parent)) {
-      // We ignore all entries that are children of a conflicing item.  Return
-      // false immediately to forget the traversal we've built up so far.
-      DVLOG(1) << "Parent was in conflict, omitting " << item;
-      return false;
+
+    if (!TryAddItem(ready_unsynced_set, parent, &dependencies)) {
+      // The parent isn't in |ready_unsynced_set|.
+      break;
     }
-    TryAddItem(ready_unsynced_set, parent, &dependencies);
+
     parent_id = parent.GetParentId();
   }
 
   // Reverse what we added to get the correct order.
   result->insert(result->end(), dependencies.rbegin(), dependencies.rend());
-  return true;
 }
 
 // Adds the given item to the list if it is unsynced and ready for commit.
-void Traversal::TryAddItem(const std::set<int64_t>& ready_unsynced_set,
+bool Traversal::TryAddItem(const std::set<int64_t>& ready_unsynced_set,
                            const syncable::Entry& item,
                            syncable::Directory::Metahandles* result) const {
   DCHECK(item.GetIsUnsynced());
   int64_t item_handle = item.GetMetahandle();
   if (ready_unsynced_set.count(item_handle) != 0) {
     result->push_back(item_handle);
+    return true;
   }
+  return false;
 }
 
 // Traverses the tree from bottom to top, adding the deleted parents of the
 // given |item|.  Stops traversing if it encounters a non-deleted node, or
-// a node that was already listed in the |traversed| list.  Returns an error
-// (false) if a node along the traversal is in a conflict state.
+// a node that was already listed in the |traversed| list.
 //
 // The result list is reversed before it is returned, so the resulting
 // traversal is in top to bottom order.  Also note that this function appends
 // to the result list without clearing it.
-bool Traversal::AddDeletedParents(
+void Traversal::AddDeletedParents(
     const std::set<int64_t>& ready_unsynced_set,
     const syncable::Entry& item,
     const syncable::Directory::Metahandles& traversed,
     syncable::Directory::Metahandles* result) const {
   DCHECK(SupportsHierarchy(item));
   syncable::Directory::Metahandles dependencies;
   syncable::Id parent_id = item.GetParentId();
 
   // Climb the tree adding entries leaf -> root.
   while (!parent_id.IsRoot()) {
@@ skipping 17 matching lines @@
     if (!parent.GetIsDel()) {
       // We're not intersted in non-deleted parents.
       break;
     }
     if (std::find(traversed.begin(), traversed.end(), handle) !=
         traversed.end()) {
       // We've already added this parent (and therefore all of its parents).
       // We can return early.
       break;
     }
-    if (IsEntryInConflict(parent)) {
-      // We ignore all entries that are children of a conflicing item.  Return
-      // false immediately to forget the traversal we've built up so far.
-      DVLOG(1) << "Parent was in conflict, omitting " << item;
-      return false;
+
+    if (!TryAddItem(ready_unsynced_set, parent, &dependencies)) {
+      // The parent isn't in ready_unsynced_set.
+      break;
     }
-    TryAddItem(ready_unsynced_set, parent, &dependencies);
+
     parent_id = parent.GetParentId();
   }
 
   // Reverse what we added to get the correct order.
   result->insert(result->end(), dependencies.rbegin(), dependencies.rend());
-  return true;
 }
 
 bool Traversal::IsFull() const {
   return out_->size() >= max_entries_;
 }
 
 bool Traversal::HaveItem(int64_t handle) const {
   return added_handles_.find(handle) != added_handles_.end();
 }
 
-bool Traversal::SupportsHierarchy(const syncable::Entry& item) const {
-  // Types with explicit server supported hierarchy only.
-  return IsTypeWithServerGeneratedRoot(item.GetModelType());
-}
-
 void Traversal::AppendManyToTraversal(
     const syncable::Directory::Metahandles& handles) {
   out_->insert(out_->end(), handles.begin(), handles.end());
   added_handles_.insert(handles.begin(), handles.end());
 }
 
 void Traversal::AppendToTraversal(int64_t metahandle) {
   out_->push_back(metahandle);
   added_handles_.insert(metahandle);
 }
 
 void Traversal::AddCreatesAndMoves(
     const std::set<int64_t>& ready_unsynced_set) {
   // Add moves and creates, and prepend their uncommitted parents.
   for (std::set<int64_t>::const_iterator iter = ready_unsynced_set.begin();
        !IsFull() && iter != ready_unsynced_set.end(); ++iter) {
     int64_t metahandle = *iter;
     if (HaveItem(metahandle))
       continue;
 
     syncable::Entry entry(trans_,
                           syncable::GET_BY_HANDLE,
                           metahandle);
     if (!entry.GetIsDel()) {
       if (SupportsHierarchy(entry)) {
         // We only commit an item + its dependencies if it and all its
         // dependencies are not in conflict.
         syncable::Directory::Metahandles item_dependencies;
-        if (AddUncommittedParents(ready_unsynced_set, entry,
-                                  &item_dependencies)) {
-          TryAddItem(ready_unsynced_set, entry, &item_dependencies);
-          AppendManyToTraversal(item_dependencies);
-        }
+        AddUncommittedParents(ready_unsynced_set, entry, &item_dependencies);
+        TryAddItem(ready_unsynced_set, entry, &item_dependencies);
+        AppendManyToTraversal(item_dependencies);
       } else {
         // No hierarchy dependencies, just commit the item itself.
         AppendToTraversal(metahandle);
       }
     }
   }
 
   // It's possible that we overcommitted while trying to expand dependent
   // items.  If so, truncate the set down to the allowed size.
   if (out_->size() > max_entries_)
@@ skipping 17 matching lines @@
         deletion_list.end()) {
       continue;
     }
 
     syncable::Entry entry(trans_, syncable::GET_BY_HANDLE,
                           metahandle);
 
     if (entry.GetIsDel()) {
       if (SupportsHierarchy(entry)) {
         syncable::Directory::Metahandles parents;
-        if (AddDeletedParents(ready_unsynced_set, entry, deletion_list,
-                              &parents)) {
-          // Append parents and chilren in top to bottom order.
-          deletion_list.insert(deletion_list.end(), parents.begin(),
-                               parents.end());
-          deletion_list.push_back(metahandle);
-        }
-      } else {
-        deletion_list.push_back(metahandle);
+        AddDeletedParents(ready_unsynced_set, entry, deletion_list, &parents);
+        // Append parents and chilren in top to bottom order.
+        deletion_list.insert(deletion_list.end(), parents.begin(),
+                             parents.end());
       }
+      deletion_list.push_back(metahandle);
     }
   }
 
   // We've been gathering deletions in top to bottom order.  Now we reverse the
   // order as we prepare the list.
   std::reverse(deletion_list.begin(), deletion_list.end());
   AppendManyToTraversal(deletion_list);
 
   // It's possible that we overcommitted while trying to expand dependent
   // items.  If so, truncate the set down to the allowed size.
@@ skipping 18 matching lines @@
   // Add moves and creates, and prepend their uncommitted parents.
   traversal.AddCreatesAndMoves(ready_unsynced_set);
 
   // Add all deletes.
   traversal.AddDeletes(ready_unsynced_set);
 }
 
 }  // namespace
 
 }  // namespace syncer