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 97 matching lines @@
   //    not received any further non-specifics-only or decryptable updates.
   // 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()) {
+  //    where the non_unique_name or parent don't match. Another exception is

[Nicolas Zea, 2016/02/11 23:41:50]

nit: may as well put this into its own e) bullet

[asargent_no_longer_on_chrome, 2016/02/18 00:53:45]

Done.

+  //    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)) {
     // 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 64 matching lines @@
                                 CONFLICT_RESOLUTION_SIZE);
     } else if (base_server_specifics_match) {
       DVLOG(1) << "Resolving simple conflict, ignoring server encryption "
                << " changes for: " << entry;
       status->increment_num_server_overwrites();
       counters->num_server_overwrites++;
       conflict_util::OverwriteServerChanges(&entry);
       UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                                 IGNORE_ENCRYPTION,
                                 CONFLICT_RESOLUTION_SIZE);
     } else if (entry_deleted || !name_matches || !parent_matches) {

[Nicolas Zea, 2016/02/11 23:41:50]

Have you manually tested that this works? I think that when we tombstone an
item, we also clear out the name, which might mean this logic gets triggered,
and we undelete the tombstone anyways.

It might be good to add an integration test for this to ensure that doesn't
happen. You can probably add it to single_client_extensions_sync_test.cc, and
borrow some of the logic from
SingleClientBookmarkSyncTest::DownloadModifiedBookmark (in particular, that test
calls into the fake server with GetFakeServer()->ModifyEntitySpecifics(..).
Basically, if you modify the specifics on the server first to delete it, then
modify the item locally, then wait, you should be able to force a conflict, and
verify that the items remains undeleted by waiting until it's undeleted at the
server too.

[asargent_no_longer_on_chrome, 2016/02/18 00:53:45]

I have manually tested that the patch works for the following scenario:

1) Get two separate profiles setup syncing to the same account, with one
installed extension that is disabled
2) Restart profile B with command-line flags for to use a proxy on a
non-listening local port, to simulate "no network connection"
3) On profile A (has network access), uninstall the extension
4) On profile B (still with no network access), enable the extension
5) Restart profile B without the command-line flag, so it has network access
again and can contact the sync server. 

Without my CL, what happens at the end of these steps is that the conflict logic
in this file ends up resurrecting the extension at profile A. With my CL, the
extension gets uninstalled at B as desired. 

 
Done. 

       // NOTE: The update application logic assumes that conflict resolution
       // will never result in changes to the local hierarchy.  The entry_deleted
       // and !parent_matches cases here are critical to maintaining that
       // assumption.
       conflict_util::OverwriteServerChanges(&entry);
       status->increment_num_server_overwrites();
       counters->num_server_overwrites++;
       DVLOG(1) << "Resolving simple conflict, overwriting server changes "
                << "for: " << entry;
       UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
@@ skipping 56 matching lines @@
         GetModelTypeFromSpecifics(conflicting_node.GetSpecifics()))) {
       continue;
     }
 
     ProcessSimpleConflict(trans, *it, cryptographer, status, counters);
   }
   return;
 }
 
 }  // namespace syncer