Fix extensions sync in cases where items change type

chrome/browser/extensions/extension_sync_service.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 "chrome/browser/extensions/extension_sync_service.h"
 
 #include <utility>
 
 #include "base/auto_reset.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 301 matching lines @@
 void ExtensionSyncService::ApplySyncData(
     const ExtensionSyncData& extension_sync_data) {
   // Ignore any pref change notifications etc. while we're applying incoming
   // sync data, so that we don't end up notifying ourselves.
   base::AutoReset<bool> ignore_updates(&ignore_updates_, true);
 
   syncer::ModelType type = extension_sync_data.is_app() ? syncer::APPS
                                                         : 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.
+    // First tell the bundle about the extension, so that it won't just ignore
+    // the deletion, then push the deletion.
+    bundle->ApplySyncData(extension_sync_data);
+    bundle->PushSyncDeletion(id, extension_sync_data.GetSyncData());
     return;
   }
 
   // Forward to the bundle. This will just update the list of synced extensions.
   bundle->ApplySyncData(extension_sync_data);
 
   // Handle uninstalls first.
   if (extension_sync_data.uninstalled()) {
     if (!ExtensionService::UninstallExtensionHelper(
             extension_service(), id, extensions::UNINSTALL_REASON_SYNC)) {
@@ skipping 245 matching lines @@
 
 void ExtensionSyncService::OnExtensionInstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
     bool is_update) {
   DCHECK_EQ(profile_, browser_context);
   // Clear pending version if the installed one has caught up.
   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.
+      base::AutoReset<bool> ignore_updates(&ignore_updates_, true);
       extension_service()->GrantPermissionsAndEnableExtension(extension);
+    }
     if (compare_result >= 0)
       pending_updates_.erase(it);
   }
   SyncExtensionChangeIfNeeded(*extension);
 }
 
 void ExtensionSyncService::OnExtensionUninstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
     extensions::UninstallReason reason) {
@@ skipping 87 matching lines @@
       sync_data_list->push_back(CreateSyncData(*extension));
     }
   }
 }
 
 bool ExtensionSyncService::ShouldSync(const Extension& extension) const {
   // Themes are handled by the ThemeSyncableService.
   return extensions::util::ShouldSync(&extension, profile_) &&
          !extension.is_theme();
 }