Change sync conflict resolution for extensions

sync/engine/conflict_resolver.cc

 // Copyright 2012 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/conflict_resolver.h"
 
 #include <list>
 #include <set>
 #include <string>
 
@@ skipping 98 matching lines @@
   // d) If the server_specifics match specifics, server_specifics are
   //    encrypted with the default key, and all other visible properties match,
   //    then we can safely ignore the local changes as redundant.
   // e) Otherwise if the base_server_specifics match the server_specifics, no
   //    functional change must have been made server-side (else
   //    base_server_specifics would have been cleared), and we can therefore
   //    safely ignore the server changes as redundant.
   // f) Otherwise, it's in general safer to ignore local changes, with the
   //    exception of deletion conflicts (choose to undelete) and conflicts
   //    where the non_unique_name or parent don't match.
-  if (!entry.GetServerIsDel()) {
+  // e) Except for the case of extensions and apps, where we want uninstalls to
+  //    win over local modifications to avoid "back from the dead" reinstalls.
+  ModelType type = entry.GetModelType();
+  if (!entry.GetServerIsDel() || (type == EXTENSIONS || type == APPS)) {

[Nicolas Zea, 2016/02/18 19:28:55]

The more I think about it, the more I think it would be safer to not have this
condition here, and instead special case the extension/apps case down below in
the else.

The rationale is that the logic within the body of this condition is based on
the assumption that the entry is not deleted by the server. I think this is why
you ran into that subtle case with the specifics matching not triggering the
right logic.

     // TODO(nick): The current logic is arbitrary; instead, it ought to be made
     // consistent with the ModelAssociator behavior for a datatype.  It would
     // be nice if we could route this back to ModelAssociator code to pick one
     // of three options: CLIENT, SERVER, or MERGE.  Some datatypes (autofill)
     // are easily mergeable.
     // See http://crbug.com/77339.
     bool name_matches = entry.GetNonUniqueName() ==
         entry.GetServerNonUniqueName();
     // The parent is implicit type root folder or the parent ID matches.
     bool parent_matches = entry.GetServerParentId().IsNull() ||
@@ skipping 102 matching lines @@
       Directory::Metahandles children;
       trans->directory()->GetChildHandlesById(trans,
                                               entry.GetId(),
                                               &children);
       // If a server deleted folder has local contents it should be a hierarchy
       // conflict.  Hierarchy conflicts should not be processed by this
       // function.
       DCHECK(children.empty());
     }
 
     // The entry is deleted on the server but still exists locally.

[Nicolas Zea, 2016/02/18 19:28:55]

Basically, this is where I would add a new condition. Something like:
if (type == Extensions or Apps) {
  Copy the code for ignoring local changes here
} else {
  Normal undeletion code here.
}

[Nicolas Zea, 2016/02/18 19:30:31]

Actually this logic should probably happen before the entry.GetIsDir check too.

if (is extension or app) {
   .. ignore local changes 
} else {
  if (is dir) {
    ...
  }
  .. undelete by overwriting server changes
}

[asargent_no_longer_on_chrome, 2016/02/19 23:59:52]

That seems to work great. Done.

     // We undelete it by overwriting the server's tombstone with the local
     // data.
     conflict_util::OverwriteServerChanges(&entry);
     status->increment_num_server_overwrites();
     counters->num_server_overwrites++;
     DVLOG(1) << "Resolving simple conflict, undeleting server entry: "
              << entry;
     UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                               UNDELETE,
                               CONFLICT_RESOLUTION_SIZE);
@@ skipping 18 matching lines @@
         GetModelTypeFromSpecifics(conflicting_node.GetSpecifics()))) {
       continue;
     }
 
     ProcessSimpleConflict(trans, *it, cryptographer, status, counters);
   }
   return;
 }
 
 }  // namespace syncer