Revert "Extensions: Remove temporary cleanup code from crbug.com/558299"
This reverts commit fa48bec92e8b0856c65255469407f51db129dfbd.
Reason for revert: The new CHECK() is crashing, so it seems there is more to be cleaned up. We need to investigate further.
Original change's description:
> Extensions: Remove temporary cleanup code from crbug.com/558299
>
> In the past, there was a bug where some themes incorrectly got synced
> into the EXTENSIONS data type, so we added cleanup code to remove the
> bad data. That was long ago and all bad data should be long gone now,
> so let's get rid of the cleanup code.
>
> Bug: none
> Change-Id: I53fcc8ecb208e9fce6565cda70ec57cc960f9b6c
> Reviewed-on: https://chromium-review.googlesource.com/1124680
> Reviewed-by: Devlin <[email protected]>
> Commit-Queue: Marc Treib <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#572255}
[email protected],[email protected]
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: none
Change-Id: If10997927100f17c505eb83221770c47e4e04b7e
Reviewed-on: https://chromium-review.googlesource.com/1136614
Reviewed-by: Devlin <[email protected]>
Commit-Queue: Devlin <[email protected]>
Cr-Commit-Position: refs/heads/master@{#574979}diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 781da8e..ef74a66 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1398,6 +1398,10 @@
pending_extension_manager()->GetById(id);
if (pending_extension_info) {
if (!pending_extension_info->ShouldAllowInstall(extension)) {
+ // Hack for crbug.com/558299, see comment on DeleteThemeDoNotUse.
+ if (extension->is_theme() && pending_extension_info->is_from_sync())
+ ExtensionSyncService::Get(profile_)->DeleteThemeDoNotUse(*extension);
+
pending_extension_manager()->Remove(id);
LOG(WARNING) << "ShouldAllowInstall() returned false for " << id
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index ffb601e..c5f150e 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -57,11 +57,11 @@
}
// Predicate for PendingExtensionManager.
+// TODO(treib,devlin): The !is_theme check shouldn't be necessary anymore after
+// all the bad data from crbug.com/558299 has been cleaned up, after M52 or so.
bool ShouldAllowInstall(const Extension* extension) {
- // Note: In the past, there was a bug where some themes incorrectly ended up
- // here, see https://crbug.com/558299. Make sure that doesn't happen anymore.
- CHECK(!extension->is_theme());
- return extensions::sync_helper::IsSyncable(extension);
+ return !extension->is_theme() &&
+ extensions::sync_helper::IsSyncable(extension);
}
syncer::SyncDataList ToSyncerSyncDataList(
@@ -493,8 +493,11 @@
check_for_updates = true;
} else if (state == NOT_INSTALLED) {
if (!extension_service()->pending_extension_manager()->AddFromSync(
- id, extension_sync_data.update_url(), extension_sync_data.version(),
- ShouldAllowInstall, extension_sync_data.remote_install())) {
+ id,
+ extension_sync_data.update_url(),
+ extension_sync_data.version(),
+ ShouldAllowInstall,
+ extension_sync_data.remote_install())) {
LOG(WARNING) << "Could not add pending extension for " << id;
// This means that the extension is already pending installation, with a
// non-INTERNAL location. Add to pending_sync_data, even though it will
@@ -560,6 +563,12 @@
flare_ = flare;
}
+void ExtensionSyncService::DeleteThemeDoNotUse(const Extension& theme) {
+ DCHECK(theme.is_theme());
+ GetSyncBundle(syncer::EXTENSIONS)->PushSyncDeletion(
+ theme.id(), CreateSyncData(theme).GetSyncData());
+}
+
extensions::ExtensionService* ExtensionSyncService::extension_service() const {
return ExtensionSystem::Get(profile_)->extension_service();
}
diff --git a/chrome/browser/extensions/extension_sync_service.h b/chrome/browser/extensions/extension_sync_service.h
index 3a86afd..878b5ba 100644
--- a/chrome/browser/extensions/extension_sync_service.h
+++ b/chrome/browser/extensions/extension_sync_service.h
@@ -67,6 +67,12 @@
void SetSyncStartFlareForTesting(
const syncer::SyncableService::StartSyncFlare& flare);
+ // Special hack: There was a bug where themes incorrectly ended up in the
+ // syncer::EXTENSIONS type. This is for cleaning up the data. crbug.com/558299
+ // DO NOT USE FOR ANYTHING ELSE!
+ // TODO(treib,devlin): Remove this after M52 or so.
+ void DeleteThemeDoNotUse(const extensions::Extension& theme);
+
private:
FRIEND_TEST_ALL_PREFIXES(TwoClientAppsSyncTest, UnexpectedLaunchType);
FRIEND_TEST_ALL_PREFIXES(ExtensionDisabledGlobalErrorTest,
