From ecc63c8d3df04a1608af3501250d475336705031 Mon Sep 17 00:00:00 2001 From: treib Date: Mon, 7 Sep 2015 09:34:47 -0700 Subject: [PATCH] ExtensionSyncService: Properly differentiate between "pending install" and "pending update". For pending updates, store only the new version, as opposed to the whole ExtensionSyncData. Also includes some semi-related cleanup: Make a bunch of tests use the public ExtensionSyncService interface, rather than relying on private implementation methods. BUG=None Review URL: https://codereview.chromium.org/1321963002 Cr-Commit-Position: refs/heads/master@{#347636} --- chrome/browser/apps/ephemeral_app_browsertest.cc | 6 +- .../extension_disabled_ui_browsertest.cc | 29 +- .../extensions/extension_service_unittest.cc | 47 ++- chrome/browser/extensions/extension_sync_data.cc | 8 +- chrome/browser/extensions/extension_sync_data.h | 7 +- .../browser/extensions/extension_sync_service.cc | 315 +++++++++++---------- chrome/browser/extensions/extension_sync_service.h | 38 +-- chrome/browser/extensions/sync_bundle.cc | 8 +- chrome/browser/extensions/sync_bundle.h | 33 +-- 9 files changed, 258 insertions(+), 233 deletions(-) diff --git a/chrome/browser/apps/ephemeral_app_browsertest.cc b/chrome/browser/apps/ephemeral_app_browsertest.cc index 47dcb7fda7ec..61144b0700f4 100644 --- a/chrome/browser/apps/ephemeral_app_browsertest.cc +++ b/chrome/browser/apps/ephemeral_app_browsertest.cc @@ -495,8 +495,10 @@ class EphemeralAppBrowserTest : public EphemeralAppTestBase { std::string app_id = app->id(); app = NULL; - ExtensionSyncService* sync_service = ExtensionSyncService::Get(profile()); - sync_service->ApplySyncData(app_sync_data); + ExtensionSyncService::Get(profile())->ProcessSyncChanges( + FROM_HERE, + syncer::SyncChangeList( + 1, app_sync_data.GetSyncChange(syncer::SyncChange::ACTION_ADD))); // Verify the installation. VerifyPromotedApp(app_id, expected_set); diff --git a/chrome/browser/extensions/extension_disabled_ui_browsertest.cc b/chrome/browser/extensions/extension_disabled_ui_browsertest.cc index b8167cc45f5f..a6e0d8c84a1a 100644 --- a/chrome/browser/extensions/extension_disabled_ui_browsertest.cc +++ b/chrome/browser/extensions/extension_disabled_ui_browsertest.cc @@ -46,9 +46,8 @@ class ExtensionDisabledGlobalErrorTest : public ExtensionBrowserTest { void SetUpOnMainThread() override { ExtensionBrowserTest::SetUpOnMainThread(); EXPECT_TRUE(scoped_temp_dir_.CreateUniqueTempDir()); - service_ = extensions::ExtensionSystem::Get( - browser()->profile())->extension_service(); - registry_ = ExtensionRegistry::Get(browser()->profile()); + service_ = extensions::ExtensionSystem::Get(profile())->extension_service(); + registry_ = ExtensionRegistry::Get(profile()); const base::FilePath test_dir = test_data_dir_.AppendASCII("permissions_increase"); const base::FilePath pem_path = test_dir.AppendASCII("permissions.pem"); @@ -72,7 +71,7 @@ class ExtensionDisabledGlobalErrorTest : public ExtensionBrowserTest { // Returns the ExtensionDisabledGlobalError, if present. // Caution: currently only supports one error at a time. GlobalError* GetExtensionDisabledGlobalError() { - return GlobalErrorServiceFactory::GetForProfile(browser()->profile())-> + return GlobalErrorServiceFactory::GetForProfile(profile())-> GetGlobalErrorByMenuItemCommandID(IDC_EXTENSION_DISABLED_FIRST); } @@ -181,11 +180,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, UserDisabled) { // version with higher permissions was installed by sync. IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, HigherPermissionsFromSync) { - // Get data for extension v2 (disabled) into sync. + // Get sync data for extension v2 (disabled). const Extension* extension = InstallAndUpdateIncreasingPermissionsExtension(); std::string extension_id = extension->id(); - ExtensionSyncService* sync_service = ExtensionSyncService::Get( - browser()->profile()); + ExtensionSyncService* sync_service = ExtensionSyncService::Get(profile()); extensions::ExtensionSyncData sync_data = sync_service->CreateSyncData(*extension); UninstallExtension(extension_id); @@ -210,8 +208,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, extensions::ExtensionUpdater::CheckParams params; service_->updater()->set_default_check_params(params); - // Sync is replacing an older version, so it pends. - EXPECT_FALSE(sync_service->ApplySyncData(sync_data)); + sync_service->ProcessSyncChanges( + FROM_HERE, + syncer::SyncChangeList( + 1, sync_data.GetSyncChange(syncer::SyncChange::ACTION_ADD))); WaitForExtensionInstall(); content::RunAllBlockingPoolTasksUntilIdle(); @@ -229,8 +229,6 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, // Test that an error appears if an extension gets installed server side. IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, RemoteInstall) { static const char extension_id[] = "pgdpcfcocojkjfbgpiianjngphoopgmo"; - ExtensionSyncService* sync_service = - ExtensionSyncService::Get(browser()->profile()); // Note: This interceptor gets requests on the IO thread. net::LocalHostTestURLRequestInterceptor interceptor( @@ -261,9 +259,12 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, RemoteInstall) { base::Time::Now(), syncer::AttachmentIdList(), syncer::AttachmentServiceProxy()); - // Sync is installing a new extension, so it pends. - EXPECT_FALSE(sync_service->ApplySyncData( - *extensions::ExtensionSyncData::CreateFromSyncData(sync_data))); + ExtensionSyncService::Get(profile())->ProcessSyncChanges( + FROM_HERE, + syncer::SyncChangeList( + 1, syncer::SyncChange(FROM_HERE, + syncer::SyncChange::ACTION_ADD, + sync_data))); WaitForExtensionInstall(); content::RunAllBlockingPoolTasksUntilIdle(); diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc index 7984784f9022..25afcad5b550 100644 --- a/chrome/browser/extensions/extension_service_unittest.cc +++ b/chrome/browser/extensions/extension_service_unittest.cc @@ -6647,24 +6647,32 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) { ext_specifics->set_id(good_crx); ext_specifics->set_enabled(true); + const base::Version installed_version = + *service()->GetInstalledExtension(good_crx)->version(); + { - ext_specifics->set_version( - service()->GetInstalledExtension(good_crx)->version()->GetString()); + ext_specifics->set_version(installed_version.GetString()); syncer::SyncData sync_data = syncer::SyncData::CreateLocalData(good_crx, "Name", specifics); syncer::SyncChange sync_change(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, sync_data); - syncer::SyncChangeList list(1); - list[0] = sync_change; + syncer::SyncChangeList list(1, sync_change); // Should do nothing if extension version == sync version. extension_sync_service()->ProcessSyncChanges(FROM_HERE, list); EXPECT_FALSE(service()->updater()->WillCheckSoon()); + // Make sure the version we'll send back to sync didn't change. + syncer::SyncDataList data = + extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS); + ASSERT_EQ(1u, data.size()); + scoped_ptr extension_data = + ExtensionSyncData::CreateFromSyncData(data[0]); + ASSERT_TRUE(extension_data); + EXPECT_TRUE(installed_version.Equals(extension_data->version())); } - // Should do nothing if extension version > sync version (but see - // the TODO in ProcessExtensionSyncData). + // Should do nothing if extension version > sync version. { ext_specifics->set_version("0.0.0.0"); syncer::SyncData sync_data = @@ -6672,26 +6680,43 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) { syncer::SyncChange sync_change(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, sync_data); - syncer::SyncChangeList list(1); - list[0] = sync_change; + syncer::SyncChangeList list(1, sync_change); extension_sync_service()->ProcessSyncChanges(FROM_HERE, list); EXPECT_FALSE(service()->updater()->WillCheckSoon()); + // Make sure the version we'll send back to sync didn't change. + syncer::SyncDataList data = + extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS); + ASSERT_EQ(1u, data.size()); + scoped_ptr extension_data = + ExtensionSyncData::CreateFromSyncData(data[0]); + ASSERT_TRUE(extension_data); + EXPECT_TRUE(installed_version.Equals(extension_data->version())); } // Should kick off an update if extension version < sync version. { - ext_specifics->set_version("9.9.9.9"); + const base::Version new_version("9.9.9.9"); + ext_specifics->set_version(new_version.GetString()); syncer::SyncData sync_data = syncer::SyncData::CreateLocalData(good_crx, "Name", specifics); syncer::SyncChange sync_change(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, sync_data); - syncer::SyncChangeList list(1); - list[0] = sync_change; + syncer::SyncChangeList list(1, sync_change); extension_sync_service()->ProcessSyncChanges(FROM_HERE, list); EXPECT_TRUE(service()->updater()->WillCheckSoon()); + // Make sure that we'll send the NEW version back to sync, even though we + // haven't actually updated yet. This is to prevent the data in sync from + // flip-flopping back and forth until all clients are up to date. + syncer::SyncDataList data = + extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS); + ASSERT_EQ(1u, data.size()); + scoped_ptr extension_data = + ExtensionSyncData::CreateFromSyncData(data[0]); + ASSERT_TRUE(extension_data); + EXPECT_TRUE(new_version.Equals(extension_data->version())); } EXPECT_FALSE(service()->pending_extension_manager()->IsIdPending(good_crx)); diff --git a/chrome/browser/extensions/extension_sync_data.cc b/chrome/browser/extensions/extension_sync_data.cc index 6404e8b61ca6..c3667c0f5a8f 100644 --- a/chrome/browser/extensions/extension_sync_data.cc +++ b/chrome/browser/extensions/extension_sync_data.cc @@ -153,8 +153,8 @@ scoped_ptr ExtensionSyncData::CreateFromSyncChange( if (!data.get()) return nullptr; - data->set_uninstalled(sync_change.change_type() == - syncer::SyncChange::ACTION_DELETE); + if (sync_change.change_type() == syncer::SyncChange::ACTION_DELETE) + data->uninstalled_ = true; return data.Pass(); } @@ -317,10 +317,6 @@ bool ExtensionSyncData::PopulateFromAppSpecifics( return true; } -void ExtensionSyncData::set_uninstalled(bool uninstalled) { - uninstalled_ = uninstalled; -} - bool ExtensionSyncData::PopulateFromSyncData( const syncer::SyncData& sync_data) { const sync_pb::EntitySpecifics& entity_specifics = sync_data.GetSpecifics(); diff --git a/chrome/browser/extensions/extension_sync_data.h b/chrome/browser/extensions/extension_sync_data.h index 2c753f3955e4..2d2c03ac187b 100644 --- a/chrome/browser/extensions/extension_sync_data.h +++ b/chrome/browser/extensions/extension_sync_data.h @@ -77,8 +77,6 @@ class ExtensionSyncData { syncer::SyncChange GetSyncChange( syncer::SyncChange::SyncChangeType change_type) const; - void set_uninstalled(bool uninstalled); - bool is_app() const { return is_app_; } const std::string& id() const { return id_; } @@ -96,7 +94,8 @@ class ExtensionSyncData { // Version-dependent properties (i.e., should be used only when the // version of the currently-installed extension matches |version|). - const Version& version() const { return version_; } + const base::Version& version() const { return version_; } + void set_version(const base::Version& version) { version_ = version; } const GURL& update_url() const { return update_url_; } // Used only for debugging. const std::string& name() const { return name_; } @@ -154,7 +153,7 @@ class ExtensionSyncData { bool remote_install_; OptionalBoolean all_urls_enabled_; bool installed_by_custodian_; - Version version_; + base::Version version_; GURL update_url_; std::string name_; diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc index ae454e492b34..93f89ee6e712 100644 --- a/chrome/browser/extensions/extension_sync_service.cc +++ b/chrome/browser/extensions/extension_sync_service.cc @@ -17,7 +17,6 @@ #include "chrome/common/extensions/extension_constants.h" #include "chrome/common/extensions/sync_helper.h" #include "chrome/common/web_application_info.h" -#include "components/sync_driver/sync_prefs.h" #include "extensions/browser/app_sorting.h" #include "extensions/browser/extension_prefs.h" #include "extensions/browser/extension_registry.h" @@ -181,9 +180,9 @@ syncer::SyncDataList ExtensionSyncService::GetAllSyncData( std::vector sync_data_list = GetLocalSyncDataList(type, true); - // Add pending data (where the local extension is either not installed yet or - // outdated). - std::vector pending_extensions = bundle->GetPendingData(); + // Add pending data (where the local extension is not installed yet). + std::vector pending_extensions = + bundle->GetPendingExtensionData(); sync_data_list.insert(sync_data_list.begin(), pending_extensions.begin(), pending_extensions.end()); @@ -228,29 +227,137 @@ ExtensionSyncData ExtensionSyncService::CreateSyncData( Extension::DISABLE_REMOTE_INSTALL); ExtensionSyncData::OptionalBoolean allowed_on_all_url = GetAllowedOnAllUrlsOptionalBoolean(extension.id(), profile_); - if (extension.is_app()) { - AppSorting* app_sorting = ExtensionSystem::Get(profile_)->app_sorting(); - return ExtensionSyncData( - extension, enabled, disable_reasons, incognito_enabled, remote_install, - allowed_on_all_url, - app_sorting->GetAppLaunchOrdinal(extension.id()), - app_sorting->GetPageOrdinal(extension.id()), - extensions::GetLaunchTypePrefValue(extension_prefs, extension.id())); + AppSorting* app_sorting = ExtensionSystem::Get(profile_)->app_sorting(); + + ExtensionSyncData result = extension.is_app() + ? ExtensionSyncData( + extension, enabled, disable_reasons, incognito_enabled, + remote_install, allowed_on_all_url, + app_sorting->GetAppLaunchOrdinal(extension.id()), + app_sorting->GetPageOrdinal(extension.id()), + extensions::GetLaunchTypePrefValue(extension_prefs, + extension.id())) + : ExtensionSyncData( + extension, enabled, disable_reasons, incognito_enabled, + remote_install, allowed_on_all_url); + + // If there's a pending update, send the new version to sync instead of the + // installed one. + auto it = pending_update_versions_.find(extension.id()); + if (it != pending_update_versions_.end()) { + const base::Version& version = it->second; + // If we have a pending version, it should be newer than the installed one. + DCHECK_EQ(-1, extension.version()->CompareTo(version)); + result.set_version(version); } - return ExtensionSyncData( - extension, enabled, disable_reasons, incognito_enabled, remote_install, - allowed_on_all_url); + return result; } -bool ExtensionSyncService::ApplySyncData( +void ExtensionSyncService::ApplySyncData( const ExtensionSyncData& extension_sync_data) { syncer::ModelType type = extension_sync_data.is_app() ? syncer::APPS : syncer::EXTENSIONS; + const std::string& id = extension_sync_data.id(); + // 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)) + return; + SyncBundle* bundle = GetSyncBundle(type); + // Forward to the bundle. This will just update the list of synced extensions. bundle->ApplySyncData(extension_sync_data); - const std::string& id = extension_sync_data.id(); + // Handle uninstalls first. + if (extension_sync_data.uninstalled()) { + if (!ExtensionService::UninstallExtensionHelper( + extension_service(), id, extensions::UNINSTALL_REASON_SYNC)) { + LOG(WARNING) << "Could not uninstall extension " << id << " for sync"; + } + return; + } + + // Extension from sync was uninstalled by the user as an external extension. + // Honor user choice and skip installation/enabling. + // TODO(treib): Should we still apply pref changes? + if (ExtensionPrefs::Get(profile_)->IsExternalExtensionUninstalled(id)) { + LOG(WARNING) << "Extension with id " << id + << " from sync was uninstalled as external extension"; + return; + } + + int version_compare_result = extension ? + extension->version()->CompareTo(extension_sync_data.version()) : 0; + + // Enable/disable the extension. + if (extension_sync_data.enabled()) { + DCHECK(!extension_sync_data.disable_reasons()); + + // Only grant permissions if the sync data explicitly sets the disable + // reasons to Extension::DISABLE_NONE (as opposed to the legacy (GrantPermissionsAndEnableExtension(extension); + else + extension_service()->EnableExtension(id); + } else { + int disable_reasons = extension_sync_data.disable_reasons(); + if (extension_sync_data.remote_install()) { + if (!(disable_reasons & Extension::DISABLE_REMOTE_INSTALL)) { + // In the non-legacy case (>=M45) where disable reasons are synced at + // all, DISABLE_REMOTE_INSTALL should be among them already. + DCHECK(!extension_sync_data.supports_disable_reasons()); + disable_reasons |= Extension::DISABLE_REMOTE_INSTALL; + } + } else if (!extension_sync_data.supports_disable_reasons()) { + // Legacy case (=M45), clear any existing disable reasons first. + // Otherwise sync can't remove just some of them. + if (extension_sync_data.supports_disable_reasons()) + ExtensionPrefs::Get(profile_)->ClearDisableReasons(id); + + extension_service()->DisableExtension(id, disable_reasons); + } + // If the target extension has already been installed ephemerally, it can + // be promoted to a regular installed extension and downloading from the Web + // Store is not necessary. + if (extension && extensions::util::IsEphemeralApp(id, profile_)) + extension_service()->PromoteEphemeralApp(extension, true); + + // Cache whether the extension was already installed because setting the + // incognito flag invalidates the |extension| pointer (it reloads the + // extension). + bool extension_installed = (extension != nullptr); + + // Update the incognito flag. + extensions::util::SetIsIncognitoEnabled( + id, profile_, extension_sync_data.incognito_enabled()); + extension = nullptr; // No longer safe to use. + + // Update the all urls flag. + if (extension_sync_data.all_urls_enabled() != + ExtensionSyncData::BOOLEAN_UNSET) { + bool allowed = extension_sync_data.all_urls_enabled() == + ExtensionSyncData::BOOLEAN_TRUE; + extensions::util::SetAllowedScriptingOnAllUrls(id, profile_, allowed); + } + + // Set app-specific data. if (extension_sync_data.is_app()) { if (extension_sync_data.app_launch_ordinal().IsValid() && extension_sync_data.page_ordinal().IsValid()) { @@ -273,13 +380,34 @@ bool ExtensionSyncService::ApplySyncData( ApplyBookmarkAppSyncData(extension_sync_data); } - if (!ApplyExtensionSyncDataHelper(extension_sync_data, type)) { - bundle->AddPendingExtension(id, extension_sync_data); - extension_service()->CheckForUpdatesSoon(); - return false; + // Finally, trigger installation/update as required. + bool check_for_updates = false; + if (extension_installed) { + // If the extension is installed but outdated, store the new version. + if (version_compare_result < 0) { + pending_update_versions_[id] = extension_sync_data.version(); + check_for_updates = true; + } + } else { + if (!extension_service()->pending_extension_manager()->AddFromSync( + id, + extension_sync_data.update_url(), + extensions::sync_helper::IsSyncable, + extension_sync_data.remote_install(), + extension_sync_data.installed_by_custodian())) { + 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 + // never be removed (we'll never install a syncable version of the + // extension), so that GetAllSyncData() continues to send it. + } + // Track pending extensions so that we can return them in GetAllSyncData(). + bundle->AddPendingExtensionData(id, extension_sync_data); + check_for_updates = true; } - return true; + if (check_for_updates) + extension_service()->CheckForUpdatesSoon(); } void ExtensionSyncService::ApplyBookmarkAppSyncData( @@ -345,15 +473,22 @@ ExtensionService* ExtensionSyncService::extension_service() const { void ExtensionSyncService::OnExtensionInstalled( content::BrowserContext* browser_context, - const extensions::Extension* extension, + const Extension* extension, bool is_update) { DCHECK_EQ(profile_, browser_context); + // Clear pending version if the installed one has caught up. + auto it = pending_update_versions_.find(extension->id()); + if (it != pending_update_versions_.end()) { + const base::Version& pending_version = it->second; + if (extension->version()->CompareTo(pending_version) >= 0) + pending_update_versions_.erase(it); + } SyncExtensionChangeIfNeeded(*extension); } void ExtensionSyncService::OnExtensionUninstalled( content::BrowserContext* browser_context, - const extensions::Extension* extension, + const Extension* extension, extensions::UninstallReason reason) { DCHECK_EQ(profile_, browser_context); // Don't bother syncing if the extension will be re-installed momentarily. @@ -376,6 +511,8 @@ void ExtensionSyncService::OnExtensionUninstalled( } else if (extension_service()->is_ready() && !flare_.is_null()) { flare_.Run(type); // Tell sync to start ASAP. } + + pending_update_versions_.erase(extension->id()); } void ExtensionSyncService::OnExtensionStateChanged( @@ -415,8 +552,7 @@ const SyncBundle* ExtensionSyncService::GetSyncBundle( std::vector ExtensionSyncService::GetLocalSyncDataList( syncer::ModelType type, bool include_everything) const { - // Collect the local state. FillSyncDataList will filter out extensions for - // which we have pending data that should be used instead. + // Collect the local state. ExtensionRegistry* registry = ExtensionRegistry::Get(profile_); std::vector data; // TODO(treib, kalman): Should we be including blacklisted/blocked extensions @@ -439,137 +575,14 @@ void ExtensionSyncService::FillSyncDataList( syncer::ModelType type, bool include_everything, std::vector* sync_data_list) const { - const SyncBundle* bundle = GetSyncBundle(type); for (const scoped_refptr& extension : extensions) { if (IsCorrectSyncType(*extension, type) && extensions::util::ShouldSync(extension.get(), profile_) && - !bundle->HasPendingExtensionId(extension->id()) && (include_everything || ExtensionPrefs::Get(profile_)->NeedsSync(extension->id()))) { + // We should never have pending data for an installed extension. + DCHECK(!GetSyncBundle(type)->HasPendingExtensionData(extension->id())); sync_data_list->push_back(CreateSyncData(*extension)); } } } - -bool ExtensionSyncService::ApplyExtensionSyncDataHelper( - const ExtensionSyncData& extension_sync_data, - syncer::ModelType type) { - const std::string& id = extension_sync_data.id(); - ExtensionRegistry* registry = ExtensionRegistry::Get(profile_); - const Extension* extension = registry->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. - if (extension && !IsCorrectSyncType(*extension, type)) - return true; - - // Handle uninstalls first. - if (extension_sync_data.uninstalled()) { - if (!ExtensionService::UninstallExtensionHelper( - extension_service(), id, extensions::UNINSTALL_REASON_SYNC)) { - LOG(WARNING) << "Could not uninstall extension " << id << " for sync"; - } - return true; - } - - // Extension from sync was uninstalled by the user as external extensions. - // Honor user choice and skip installation/enabling. - if (ExtensionPrefs::Get(profile_)->IsExternalExtensionUninstalled(id)) { - LOG(WARNING) << "Extension with id " << id - << " from sync was uninstalled as external extension"; - return true; - } - - int version_compare_result = extension ? - extension->version()->CompareTo(extension_sync_data.version()) : 0; - - // Set user settings. - if (extension_sync_data.enabled()) { - DCHECK(!extension_sync_data.disable_reasons()); - - // Only grant permissions if the sync data explicitly sets the disable - // reasons to Extension::DISABLE_NONE (as opposed to the legacy (GrantPermissionsAndEnableExtension(extension); - else - extension_service()->EnableExtension(id); - } else { - int disable_reasons = extension_sync_data.disable_reasons(); - if (extension_sync_data.remote_install()) { - if (!(disable_reasons & Extension::DISABLE_REMOTE_INSTALL)) { - // In the non-legacy case (>=M45) where disable reasons are synced at - // all, DISABLE_REMOTE_INSTALL should be among them already. - DCHECK(!extension_sync_data.supports_disable_reasons()); - disable_reasons |= Extension::DISABLE_REMOTE_INSTALL; - } - } else if (!extension_sync_data.supports_disable_reasons()) { - // Legacy case (=M45), clear any existing disable reasons first. - // Otherwise sync can't remove just some of them. - if (extension_sync_data.supports_disable_reasons()) - ExtensionPrefs::Get(profile_)->ClearDisableReasons(id); - - extension_service()->DisableExtension(id, disable_reasons); - } - - // We need to cache some information here because setting the incognito flag - // invalidates the |extension| pointer (it reloads the extension). - bool extension_installed = (extension != NULL); - - // If the target extension has already been installed ephemerally, it can - // be promoted to a regular installed extension and downloading from the Web - // Store is not necessary. - if (extension && extensions::util::IsEphemeralApp(id, profile_)) - extension_service()->PromoteEphemeralApp(extension, true); - - // Update the incognito flag. - extensions::util::SetIsIncognitoEnabled( - id, profile_, extension_sync_data.incognito_enabled()); - extension = NULL; // No longer safe to use. - - // Update the all urls flag. - if (extension_sync_data.all_urls_enabled() != - ExtensionSyncData::BOOLEAN_UNSET) { - bool allowed = extension_sync_data.all_urls_enabled() == - ExtensionSyncData::BOOLEAN_TRUE; - extensions::util::SetAllowedScriptingOnAllUrls(id, profile_, allowed); - } - - if (extension_installed) { - // If the extension is already installed, check if it's outdated. - if (version_compare_result < 0) { - // Extension is outdated. - return false; - } - } else { - CHECK(type == syncer::EXTENSIONS || type == syncer::APPS); - if (!extension_service()->pending_extension_manager()->AddFromSync( - id, - extension_sync_data.update_url(), - extensions::sync_helper::IsSyncable, - extension_sync_data.remote_install(), - extension_sync_data.installed_by_custodian())) { - 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 - // never be removed (we'll never install a syncable version of the - // extension), so that GetAllSyncData() continues to send it. - } - // Track pending extensions so that we can return them in GetAllSyncData(). - return false; - } - - return true; -} diff --git a/chrome/browser/extensions/extension_sync_service.h b/chrome/browser/extensions/extension_sync_service.h index bd0e2e4f6fa7..4f878ced2886 100644 --- a/chrome/browser/extensions/extension_sync_service.h +++ b/chrome/browser/extensions/extension_sync_service.h @@ -9,6 +9,7 @@ #include #include "base/scoped_observer.h" +#include "base/version.h" #include "chrome/browser/extensions/sync_bundle.h" #include "components/keyed_service/core/keyed_service.h" #include "extensions/browser/extension_prefs_observer.h" @@ -55,21 +56,13 @@ class ExtensionSyncService : public syncer::SyncableService, const tracked_objects::Location& from_here, const syncer::SyncChangeList& change_list) override; + void SetSyncStartFlareForTesting( + const syncer::SyncableService::StartSyncFlare& flare); + private: FRIEND_TEST_ALL_PREFIXES(TwoClientAppsSyncTest, UnexpectedLaunchType); FRIEND_TEST_ALL_PREFIXES(ExtensionDisabledGlobalErrorTest, HigherPermissionsFromSync); - FRIEND_TEST_ALL_PREFIXES(ExtensionDisabledGlobalErrorTest, RemoteInstall); - FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest, - DeferredSyncStartupPreInstalledComponent); - FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest, - DeferredSyncStartupPreInstalledNormal); - FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest, - DeferredSyncStartupOnInstall); - friend class EphemeralAppBrowserTest; - - void SetSyncStartFlareForTesting( - const syncer::SyncableService::StartSyncFlare& flare); ExtensionService* extension_service() const; @@ -96,9 +89,11 @@ class ExtensionSyncService : public syncer::SyncableService, const extensions::Extension& extension) const; // Applies the given change coming in from the server to the local state. - // Returns false if the changes were not completely applied and were added - // to the pending list. - bool ApplySyncData(const extensions::ExtensionSyncData& extension_sync_data); + void ApplySyncData(const extensions::ExtensionSyncData& extension_sync_data); + + // Applies the bookmark app specific parts of |extension_sync_data|. + void ApplyBookmarkAppSyncData( + const extensions::ExtensionSyncData& extension_sync_data); // Collects the ExtensionSyncData for all installed apps or extensions. // If |include_everything| is true, includes all installed extensions, @@ -114,17 +109,6 @@ class ExtensionSyncService : public syncer::SyncableService, bool include_everything, std::vector* sync_data_list) const; - // Handles applying the extension specific values in |extension_sync_data| to - // the local state. - // Returns false if the changes were not completely applied. - bool ApplyExtensionSyncDataHelper( - const extensions::ExtensionSyncData& extension_sync_data, - syncer::ModelType type); - - // Processes the bookmark app specific parts of an AppSyncData. - void ApplyBookmarkAppSyncData( - const extensions::ExtensionSyncData& extension_sync_data); - // The normal profile associated with this ExtensionSyncService. Profile* profile_; @@ -136,6 +120,10 @@ class ExtensionSyncService : public syncer::SyncableService, extensions::SyncBundle app_sync_bundle_; extensions::SyncBundle extension_sync_bundle_; + // Map from extension id to new version. Used to send the new version back to + // the sync server while we're waiting for an extension to update. + std::map pending_update_versions_; + // Run()ning tells sync to try and start soon, because syncable changes // have started happening. It will cause sync to call us back // asynchronously via MergeDataAndStartSyncing as soon as possible. diff --git a/chrome/browser/extensions/sync_bundle.cc b/chrome/browser/extensions/sync_bundle.cc index 4e6a7756f1d7..42bcf9ebc8be 100644 --- a/chrome/browser/extensions/sync_bundle.cc +++ b/chrome/browser/extensions/sync_bundle.cc @@ -63,7 +63,7 @@ void SyncBundle::PushSyncAddOrUpdate(const std::string& extension_id, PushSyncChanges(syncer::SyncChangeList( 1, CreateSyncChange(extension_id, sync_data))); AddSyncedExtension(extension_id); - // Now sync and local state agree. If we had any pending change from sync, + // Now sync and local state agree. If we had a pending install from sync, // clear it now. pending_sync_data_.erase(extension_id); } @@ -75,17 +75,17 @@ void SyncBundle::ApplySyncData(const ExtensionSyncData& extension_sync_data) { AddSyncedExtension(extension_sync_data.id()); } -bool SyncBundle::HasPendingExtensionId(const std::string& id) const { +bool SyncBundle::HasPendingExtensionData(const std::string& id) const { return pending_sync_data_.find(id) != pending_sync_data_.end(); } -void SyncBundle::AddPendingExtension( +void SyncBundle::AddPendingExtensionData( const std::string& id, const ExtensionSyncData& extension_sync_data) { pending_sync_data_.insert(std::make_pair(id, extension_sync_data)); } -std::vector SyncBundle::GetPendingData() const { +std::vector SyncBundle::GetPendingExtensionData() const { std::vector pending_extensions; for (const auto& data : pending_sync_data_) pending_extensions.push_back(data.second); diff --git a/chrome/browser/extensions/sync_bundle.h b/chrome/browser/extensions/sync_bundle.h index adebbb10754c..2e34a1003e65 100644 --- a/chrome/browser/extensions/sync_bundle.h +++ b/chrome/browser/extensions/sync_bundle.h @@ -39,13 +39,14 @@ class SyncBundle { // to the server. void PushSyncDataList(const syncer::SyncDataList& sync_data_list); - // Handles the sync deletion of the given extension. This updates the set of - // synced extensions as appropriate, and then pushes a SyncChange to the - // server. + // Updates the set of synced extensions as appropriate, and then pushes a + // SyncChange to the server. void PushSyncDeletion(const std::string& extension_id, const syncer::SyncData& sync_data); - // Pushes any sync changes to |extension| to the server. + // Pushes any sync changes to an extension to the server and, if necessary, + // updates the set of synced extension. This also clears any pending data for + // the extension. void PushSyncAddOrUpdate(const std::string& extension_id, const syncer::SyncData& sync_data); @@ -53,16 +54,17 @@ class SyncBundle { // the list of synced extensions. void ApplySyncData(const ExtensionSyncData& extension_sync_data); - // Checks if there is pending sync data for the extension with the given |id| - // that should be sent to the server instead of the local state. - bool HasPendingExtensionId(const std::string& id) const; + // Checks if there is pending sync data for the extension with the given |id|, + // i.e. data to be sent to the sync server until the extension is installed + // locally. + bool HasPendingExtensionData(const std::string& id) const; - // Adds a pending extension to be synced. - void AddPendingExtension(const std::string& id, - const ExtensionSyncData& sync_data); + // Adds pending data for the extension with the given |id|. + void AddPendingExtensionData(const std::string& id, + const ExtensionSyncData& sync_data); - // Returns a vector of all the pending sync data. - std::vector GetPendingData() const; + // Returns a vector of all the pending extension data. + std::vector GetPendingExtensionData() const; private: // Creates a SyncChange to add or update an extension. @@ -82,10 +84,9 @@ class SyncBundle { // should be ACTION_ADD or ACTION_UPDATE. std::set synced_extensions_; - // This stores changes we got from sync that we couldn't apply immediately - // (such as installing a new extension, or an update). We'll send this back - // to the server instead of the local state, to prevent the sync state from - // flipping back and forth until all clients are on the same state. + // This stores pending installs we got from sync. We'll send this back to the + // server until we've installed the extension locally, to prevent the sync + // state from flipping back and forth until all clients are up to date. std::map pending_sync_data_; DISALLOW_COPY_AND_ASSIGN(SyncBundle); -- 2.11.4.GIT