Fix extensions sync in cases where items change type

chrome/browser/extensions/extension_sync_service.cc

Index: chrome/browser/extensions/extension_sync_service.cc
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index 6d6b3af16598872e5a2b952252dff120ab01499e..ce8ad0eaa9401f9f8b5752e5b149b18752679273 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -319,24 +319,16 @@ void ExtensionSyncService::ApplySyncData(
                                                         : syncer::EXTENSIONS;
   const std::string& id = extension_sync_data.id();
   SyncBundle* bundle = GetSyncBundle(type);
+  DCHECK(bundle->IsSyncing());
   // Note: |extension| may be null if it hasn't been installed yet.
   const Extension* extension =
       ExtensionRegistry::Get(profile_)->GetInstalledExtension(id);
-  // TODO(bolms): we should really handle this better.  The particularly bad
-  // case is where an app becomes an extension or vice versa, and we end up with
-  // a zombie extension that won't go away.
-  // TODO(treib): Is this still true?
   if (extension && !IsCorrectSyncType(*extension, type)) {
-    // Special hack: There was a bug where themes incorrectly ended up in the
-    // syncer::EXTENSIONS type. If we get incoming sync data for a theme, clean
-    // it up. crbug.com/558299
-    // TODO(treib,devlin): Remove this after M52 or so.
-    if (extension->is_theme()) {
-      // First tell the bundle about the extension, so that it won't just ignore
-      // the deletion.
-      bundle->ApplySyncData(extension_sync_data);
-      bundle->PushSyncDeletion(id, extension_sync_data.GetSyncData());
-    }
+    // The installed item isn't the same type as the sync data item, so we need
+    // to remove the sync data item; otherwise it will be a zombie that will
+    // keep coming back even if the installed item with this id is uninstalled.
+    bundle->ApplySyncData(extension_sync_data);
+    bundle->PushSyncDeletion(id, extension_sync_data.GetSyncData());

[asargent_no_longer_on_chrome, 2016/03/09 23:28:07]

treib/devlin: I noticed that we originally added this code just for themes back
in codereview.chromium.org/1492453002 - was there any reason we didn't make that
more general, apart from conservatism? Can you think of scenarios where this
will cause problems?

[Marc Treib, 2016/03/10 10:36:34]

It was meant as a temporary hack at that point, and I was trying to keep its
scope as small as possible. Nice to see my temporary hack find a permanent home
:D  (for some value of "nice")
I don't see any problems this could cause.
One nit: Please keep a comment here on why the ApplySyncData call is necessary.

[asargent_no_longer_on_chrome, 2016/03/11 23:40:59]

added comment back in

     return;
   }
 
@@ -602,8 +594,13 @@ void ExtensionSyncService::OnExtensionInstalled(
   auto it = pending_updates_.find(extension->id());
   if (it != pending_updates_.end()) {
     int compare_result = extension->version()->CompareTo(it->second.version);
-    if (compare_result == 0 && it->second.grant_permissions_and_reenable)
+    if (compare_result == 0 && it->second.grant_permissions_and_reenable) {
+      // The call to SyncExtensionChangeIfNeeded below will take care of syncing
+      // changes to this extension, so we don't want to trigger sync activity
+      // from the call to GrantPermissionsAndEnableExtension.

[Marc Treib, 2016/03/10 10:36:34]

Ooh, nice catch!

+      base::AutoReset<bool> ignore_updates(&ignore_updates_, true);
       extension_service()->GrantPermissionsAndEnableExtension(extension);
+    }
     if (compare_result >= 0)
       pending_updates_.erase(it);
   }