Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=590692

Review URL: https://codereview.chromium.org/1778923003

Cr-Commit-Position: refs/heads/master@{#380813}

6 files changed, 239 insertions, 18 deletions

diff --git a/chrome/browser/extensions/extension_disabled_ui_browsertest.cc b/chrome/browser/extensions/extension_disabled_ui_browsertest.cc
index 4466ade..823ea85 100644
--- a/chrome/browser/extensions/extension_disabled_ui_browsertest.cc
+++ b/chrome/browser/extensions/extension_disabled_ui_browsertest.cc
@@ -30,6 +30,8 @@
 #include "extensions/common/extension.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "net/url_request/test_url_request_interceptor.h"
+#include "sync/api/fake_sync_change_processor.h"
+#include "sync/api/sync_error_factory_mock.h"
 #include "sync/protocol/extension_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 
@@ -213,6 +215,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest,
       GURL("http://localhost/autoupdate/v2.crx"),
       scoped_temp_dir_.path().AppendASCII("permissions2.crx"));
 
+  sync_service->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS, syncer::SyncDataList(),
+      make_scoped_ptr(new syncer::FakeSyncChangeProcessor()),
+      make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
   extensions::TestExtensionRegistryObserver install_observer(registry_);
   sync_service->ProcessSyncChanges(
       FROM_HERE,
@@ -265,12 +271,16 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, RemoteInstall) {
                                          syncer::AttachmentIdList(),
                                          syncer::AttachmentServiceProxy());
 
+  ExtensionSyncService* sync_service = ExtensionSyncService::Get(profile());
+  sync_service->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS, syncer::SyncDataList(),
+      make_scoped_ptr(new syncer::FakeSyncChangeProcessor()),
+      make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
   extensions::TestExtensionRegistryObserver install_observer(registry_);
-  ExtensionSyncService::Get(profile())->ProcessSyncChanges(
+  sync_service->ProcessSyncChanges(
       FROM_HERE,
       syncer::SyncChangeList(
-          1, syncer::SyncChange(FROM_HERE,
-                                syncer::SyncChange::ACTION_ADD,
+          1, syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD,
                                 sync_data)));
 
   install_observer.WaitForExtensionWillBeInstalled();
diff --git a/chrome/browser/extensions/extension_service_sync_unittest.cc b/chrome/browser/extensions/extension_service_sync_unittest.cc
index 2eb884b..a283a8e 100644
--- a/chrome/browser/extensions/extension_service_sync_unittest.cc
+++ b/chrome/browser/extensions/extension_service_sync_unittest.cc
@@ -43,6 +43,7 @@
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/value_builder.h"
 #include "sync/api/fake_sync_change_processor.h"
+#include "sync/api/sync_change_processor_wrapper_for_test.h"
 #include "sync/api/sync_data.h"
 #include "sync/api/sync_error_factory_mock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -80,6 +81,81 @@ SyncChangeList MakeSyncChangeList(const std::string& id,
   return SyncChangeList(1, SyncChange(FROM_HERE, change_type, sync_data));
 }
 
+// This is a FakeSyncChangeProcessor specialization that maintains a store of
+// SyncData items in the superclass' data_ member variable, treating it like a
+// map keyed by the extension id from the SyncData. Each instance of this class
+// should only be used for one model type (which should be either extensions or
+// apps) to match how the real sync system handles things.
+class StatefulChangeProcessor : public syncer::FakeSyncChangeProcessor {
+ public:
+  explicit StatefulChangeProcessor(syncer::ModelType expected_type)
+      : expected_type_(expected_type) {
+    EXPECT_TRUE(expected_type == syncer::ModelType::EXTENSIONS ||
+                expected_type == syncer::ModelType::APPS);
+  }
+
+  ~StatefulChangeProcessor() override {}
+
+  // We let our parent class, FakeSyncChangeProcessor, handle saving the
+  // changes for us, but in addition we "apply" these changes by treating
+  // the FakeSyncChangeProcessor's SyncDataList as a map keyed by extension
+  // id.
+  syncer::SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const syncer::SyncChangeList& change_list) override {
+    syncer::FakeSyncChangeProcessor::ProcessSyncChanges(from_here, change_list);
+    for (const auto& change : change_list) {
+      syncer::SyncData sync_data = change.sync_data();
+      EXPECT_EQ(expected_type_, sync_data.GetDataType());
+
+      scoped_ptr<ExtensionSyncData> modified =
+          ExtensionSyncData::CreateFromSyncData(sync_data);
+
+      // Start by removing any existing entry for this extension id.
+      syncer::SyncDataList& data_list = data();
+      for (auto iter = data_list.begin(); iter != data_list.end(); ++iter) {
+        scoped_ptr<ExtensionSyncData> existing =
+            ExtensionSyncData::CreateFromSyncData(*iter);
+        if (existing->id() == modified->id()) {
+          data_list.erase(iter);
+          break;
+        }
+      }
+
+      // Now add in the new data for this id, if appropriate.
+      if (change.change_type() == SyncChange::ACTION_ADD ||
+          change.change_type() == SyncChange::ACTION_UPDATE) {
+        data_list.push_back(sync_data);
+      } else if (change.change_type() != SyncChange::ACTION_DELETE) {
+        ADD_FAILURE() << "Unexpected change type " << change.change_type();
+      }
+    }
+    return syncer::SyncError();
+  }
+
+  // We override this to help catch the error of trying to use a single
+  // StatefulChangeProcessor to process changes for both extensions and apps
+  // sync data.
+  syncer::SyncDataList GetAllSyncData(syncer::ModelType type) const override {
+    EXPECT_EQ(expected_type_, type);
+    return FakeSyncChangeProcessor::GetAllSyncData(type);
+  }
+
+  // This is a helper to vend a wrapped version of this object suitable for
+  // passing in to MergeDataAndStartSyncing, which takes a
+  // scoped_ptr<SyncChangeProcessor>, since in tests we typically don't want to
+  // give up ownership of a local change processor.
+  scoped_ptr<syncer::SyncChangeProcessor> GetWrapped() {
+    return make_scoped_ptr(new syncer::SyncChangeProcessorWrapperForTest(this));
+  }
+
+ protected:
+  // The expected ModelType of changes that this processor will see.
+  syncer::ModelType expected_type_;
+
+  DISALLOW_COPY_AND_ASSIGN(StatefulChangeProcessor);
+};
+
 }  // namespace
 
 class ExtensionServiceSyncTest
@@ -92,6 +168,16 @@ class ExtensionServiceSyncTest
     *model_type_passed_in = model_type;
   }
 
+  // Helper to call MergeDataAndStartSyncing with no server data and dummy
+  // change processor / error factory.
+  void StartSyncing(syncer::ModelType type) {
+    ASSERT_TRUE(type == syncer::EXTENSIONS || type == syncer::APPS);
+    extension_sync_service()->MergeDataAndStartSyncing(
+        type, syncer::SyncDataList(),
+        make_scoped_ptr(new syncer::FakeSyncChangeProcessor()),
+        make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
+  }
+
  protected:
   // Paths to some of the fake extensions.
   base::FilePath good0_path() {
@@ -608,6 +694,7 @@ TEST_F(ExtensionServiceSyncTest, SyncForUninstalledExternalExtension) {
       syncer::EXTENSIONS, syncer::SyncDataList(),
       make_scoped_ptr(new syncer::FakeSyncChangeProcessor()),
       make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
+  StartSyncing(syncer::APPS);
 
   UninstallExtension(good_crx, false);
   EXPECT_TRUE(
@@ -792,6 +879,8 @@ TEST_F(ExtensionServiceSyncTest, ProcessSyncDataUninstall) {
 
 TEST_F(ExtensionServiceSyncTest, ProcessSyncDataWrongType) {
   InitializeEmptyExtensionService();
+  StartSyncing(syncer::EXTENSIONS);
+  StartSyncing(syncer::APPS);
 
   // Install the extension.
   base::FilePath extension_path = data_dir().AppendASCII("good.crx");
@@ -1486,6 +1575,7 @@ class ExtensionServiceTestSupervised : public ExtensionServiceSyncTest,
     ExtensionServiceInitParams params = CreateDefaultInitParams();
     params.profile_is_supervised = profile_is_supervised;
     InitializeExtensionService(params);
+    StartSyncing(syncer::EXTENSIONS);
 
     supervised_user_service()->SetDelegate(this);
     supervised_user_service()->Init();
@@ -1833,6 +1923,7 @@ TEST_F(ExtensionServiceSyncTest, SyncUninstallByCustodianSkipsPolicy) {
 
 TEST_F(ExtensionServiceSyncTest, SyncExtensionHasAllhostsWithheld) {
   InitializeEmptyExtensionService();
+  StartSyncing(syncer::EXTENSIONS);
 
   // Create an extension that needs all-hosts.
   const std::string kName("extension");
@@ -1879,3 +1970,96 @@ TEST_F(ExtensionServiceSyncTest, SyncExtensionHasAllhostsWithheld) {
 }
 
 #endif  // defined(ENABLE_SUPERVISED_USERS)
+
+// Tests sync behavior in the case of an item that starts out as an app and
+// gets updated to become an extension.
+TEST_F(ExtensionServiceSyncTest, AppToExtension) {
+  InitializeEmptyExtensionService();
+  service()->Init();
+  ASSERT_TRUE(service()->is_ready());
+
+  // Install v1, which is an app.
+  const Extension* v1 =
+      InstallCRX(data_dir().AppendASCII("sync_datatypes").AppendASCII("v1.crx"),
+                 INSTALL_NEW);
+  EXPECT_TRUE(v1->is_app());
+  EXPECT_FALSE(v1->is_extension());
+  std::string id = v1->id();
+
+  StatefulChangeProcessor extensions_processor(syncer::ModelType::EXTENSIONS);
+  StatefulChangeProcessor apps_processor(syncer::ModelType::APPS);
+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS, syncer::SyncDataList(),
+      extensions_processor.GetWrapped(),
+      make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::APPS, syncer::SyncDataList(), apps_processor.GetWrapped(),
+      make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
+
+  // Check the app/extension change processors to be sure the right data was
+  // added.
+  EXPECT_TRUE(extensions_processor.changes().empty());
+  EXPECT_TRUE(extensions_processor.data().empty());
+  EXPECT_EQ(1u, apps_processor.data().size());
+  ASSERT_EQ(1u, apps_processor.changes().size());
+  const SyncChange& app_change = apps_processor.changes()[0];
+  EXPECT_EQ(SyncChange::ACTION_ADD, app_change.change_type());
+  scoped_ptr<ExtensionSyncData> app_data =
+      ExtensionSyncData::CreateFromSyncData(app_change.sync_data());
+  EXPECT_TRUE(app_data->is_app());
+  EXPECT_EQ(id, app_data->id());
+  EXPECT_EQ(*v1->version(), app_data->version());
+
+  // Update the app to v2, which is an extension.
+  const Extension* v2 =
+      InstallCRX(data_dir().AppendASCII("sync_datatypes").AppendASCII("v2.crx"),
+                 INSTALL_UPDATED);
+  EXPECT_FALSE(v2->is_app());
+  EXPECT_TRUE(v2->is_extension());
+  EXPECT_EQ(id, v2->id());
+
+  // Make sure we saw an extension item added.
+  ASSERT_EQ(1u, extensions_processor.changes().size());
+  const SyncChange& extension_change = extensions_processor.changes()[0];
+  EXPECT_EQ(SyncChange::ACTION_ADD, extension_change.change_type());
+  scoped_ptr<ExtensionSyncData> extension_data =
+      ExtensionSyncData::CreateFromSyncData(extension_change.sync_data());
+  EXPECT_FALSE(extension_data->is_app());
+  EXPECT_EQ(id, extension_data->id());
+  EXPECT_EQ(*v2->version(), extension_data->version());
+
+  // Get the current data from the change processors to use as the input to
+  // the following call to MergeDataAndStartSyncing. This simulates what should
+  // happen with sync.
+  syncer::SyncDataList extensions_data =
+      extensions_processor.GetAllSyncData(syncer::EXTENSIONS);
+  syncer::SyncDataList apps_data = apps_processor.GetAllSyncData(syncer::APPS);
+
+  // Stop syncing, then start again.
+  extension_sync_service()->StopSyncing(syncer::EXTENSIONS);
+  extension_sync_service()->StopSyncing(syncer::APPS);
+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS, extensions_data, extensions_processor.GetWrapped(),
+      make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::APPS, apps_data, apps_processor.GetWrapped(),
+      make_scoped_ptr(new syncer::SyncErrorFactoryMock()));
+
+  // Make sure we saw an app item deleted.
+  bool found_delete = false;
+  for (const auto& change : apps_processor.changes()) {
+    if (change.change_type() == SyncChange::ACTION_DELETE) {
+      scoped_ptr<ExtensionSyncData> data =
+          ExtensionSyncData::CreateFromSyncChange(change);
+      if (data->id() == id) {
+        found_delete = true;
+        break;
+      }
+    }
+  }
+  EXPECT_TRUE(found_delete);
+
+  // Make sure there is one extension, and there are no more apps.
+  EXPECT_EQ(1u, extensions_processor.data().size());
+  EXPECT_TRUE(apps_processor.data().empty());
+}
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index 6d6b3af..a608f07 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -319,24 +319,18 @@ 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.
+    // 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;
   }
 
@@ -602,8 +596,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.
+      base::AutoReset<bool> ignore_updates(&ignore_updates_, true);
       extension_service()->GrantPermissionsAndEnableExtension(extension);
+    }
     if (compare_result >= 0)
       pending_updates_.erase(it);
   }
diff --git a/chrome/test/data/extensions/sync_datatypes/key.pem b/chrome/test/data/extensions/sync_datatypes/key.pem
new file mode 100644
index 0000000..5a29d65
--- /dev/null
+++ b/chrome/test/data/extensions/sync_datatypes/key.pem
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCxF/hN3FoCig1c
+YXyxHSweoZ8sbSPwipjhaSsQyeca4mXuf984W7LuMPvhMmwUaytnJXU294sT3nzj
+83O7pBdbH0jWKDKajJPMlcOnSQGs8dT9ABzt8winILAJOpJw7nSgxvRZiZgbKp2I
+VHydIJLCsBgvBCFPqCPAJirVeGm5D24cXjVChdcA6duoBJGWspDWiZ/LRkbM2ZuN
+8NxlXC2N+C0HpHkZqOO+BbM/GApJdW85Ce70JKUK/Nu/9EZB56kTwmo3MXjXdE9Y
+eK8qn2NqYFYMfzUe3uK1atRBiX8NobjrCnqu3LJrdPKxy/Mw+Y0YDaF/ZXXxSmHR
+GMvTofEjAgMBAAECggEAYAypHsmphAEOOBGjyIgS+tYb98OGH5t8SZ15vxRSpREv
+ychO8ElD5c5pfn2TgwuRMdNuHI7sPq2IPTY4igf4pvJz1btdntcp/mcoA94j64IK
+S+I4zpHnGoYvFAJRlLCwTtc5hiqLdgiKAnwYTjxxfOh3ZWCvFH4UTc4lozw40yZ0
+k9uHIVNZkE9PAPHdUX3Oiv7kPpuaZAqoFyfyAXdfUEbGch9hgosPqFrwB8mAbJeh
+DubTZkpMVlaMyIAFIgbYjvc8sXrvfHDu60qXNktFEDVQ7igSHHpwdHWtHtsIJ5vb
+AOY+TWR/ScDBgqvCXXlKiExRI2W0NSHD/ftgStWQ4QKBgQDmrXOnlWgfpkCGuCQH
+7R9iyBfogU9dQmjaVHhrxssHfPz2coZwDrHdESFxqYK9sHzLNfdz8bLyNdOZtNtf
+jS6GDVcsAM9miEi9nkYuu1pb7z8OXNXXV4LYIm2NXUFYzFIGCXK1n2JvlxziO1Mo
+9QhaWj0WmFkxfFjnDdaLk8VYcQKBgQDEiLEhgE0I+npSOFExEtN5IC/ohr6TwGB9
+zKcx7dS8Morpir2D0XdWghA8o2+wKML1XRBRVHbZzIFIGZi6Nfc45GIR6Cuo3Cuu
+kCXDwtUkNPTzKmoxxuUGxa1UOeveTRgM9csGo3fzk0lpy4by+ZdjM03e/VNE2M7u
+E/aa3YDM0wKBgQCMxk+tdv1rSy9Xx+qdN7WOuCP3DWscs9l/XEt9In1m3X0W/X9T
+xXQAQGMTlWonTxxpe06/YEJflD/FLt0t69/3iQ6o2Pm5TfRuW7fi7w1Oy6vEnR0X
+ZN2B/0iyG7Y0dcSc0IlDk7gj96l12tR+S0NEuItNTb4o+ATdRNGoro6h0QKBgFw1
+NcXeCEaaHiHNQmqfxpAhxdh2v5tauurKxfbq+tCBdiM0cM4TzMXNqAiLNa+UsEOm
+Mi22TzzIci99suZKw37xyAFWyIUlJ2lzQASkuJOQNQyRbdmE05dlz3ig5EUcLpiG
+CYdH0tN42wzD7MC60Yg9Xd+tQxAeGJgizaTDH9b5AoGBAMa5tbS4klwTQgHrDnu5
+vxqmTqE4Z5JT0Cj+aJvFAUtYtzrkH3DYvnBFDZvYXWuojqY3mA1+6wwhsyh3EfFO
+CyOUfcpqJTLblQdY8gO3rtK2fH2/TTOCu+1A1yW2XQLO1K4NORczIHF7CAYYPzxO
+S6UN7LQb/YUvm0czl0WbFDCf
+-----END PRIVATE KEY-----
diff --git a/chrome/test/data/extensions/sync_datatypes/v1.crx b/chrome/test/data/extensions/sync_datatypes/v1.crx
new file mode 100644
index 0000000..2412eb8
--- /dev/null
+++ b/chrome/test/data/extensions/sync_datatypes/v1.crx
diff --git a/chrome/test/data/extensions/sync_datatypes/v2.crx b/chrome/test/data/extensions/sync_datatypes/v2.crx
new file mode 100644
index 0000000..97375e7
--- /dev/null
+++ b/chrome/test/data/extensions/sync_datatypes/v2.crx