Extension syncing: Introduce a NeedsSync pref

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 "base/basictypes.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/extensions/bookmark_app_helper.h"
@@ skipping 17 matching lines @@
 #include "extensions/common/extension_set.h"
 #include "extensions/common/image_util.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
 
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionRegistry;
 using extensions::ExtensionSet;
 using extensions::ExtensionSyncData;
-using extensions::PendingEnables;
 using extensions::SyncBundle;
 
 namespace {
 
 void OnWebApplicationInfoLoaded(
     WebApplicationInfo synced_info,
     base::WeakPtr<ExtensionService> extension_service,
     const WebApplicationInfo& loaded_info) {
   DCHECK_EQ(synced_info.app_url, loaded_info.app_url);
 
@@ skipping 23 matching lines @@
   // Otherwise, unset.
   return ExtensionSyncData::BOOLEAN_UNSET;
 }
 
 // Returns true if the sync type of |extension| matches |type|.
 bool IsCorrectSyncType(const Extension& extension, syncer::ModelType type) {
   return (type == syncer::EXTENSIONS && extension.is_extension()) ||
          (type == syncer::APPS && extension.is_app());
 }
 
+syncer::SyncDataList ToSyncerSyncDataList(
+    const std::vector<ExtensionSyncData>& data) {
+  syncer::SyncDataList result;
+  result.reserve(data.size());
+  for (const ExtensionSyncData& item : data)
+    result.push_back(item.GetSyncData());
+  return result;
+}
+
 }  // namespace
 
 ExtensionSyncService::ExtensionSyncService(Profile* profile,
                                            ExtensionPrefs* extension_prefs,
                                            ExtensionService* extension_service)
     : profile_(profile),
       extension_prefs_(extension_prefs),
       extension_service_(extension_service),
       app_sync_bundle_(this),
-      extension_sync_bundle_(this),
-      pending_app_enables_(make_scoped_ptr(new sync_driver::SyncPrefs(
-                               extension_prefs_->pref_service())),
-                           &app_sync_bundle_,
-                           syncer::APPS),
-      pending_extension_enables_(make_scoped_ptr(new sync_driver::SyncPrefs(
-                                     extension_prefs_->pref_service())),
-                                 &extension_sync_bundle_,
-                                 syncer::EXTENSIONS) {
+      extension_sync_bundle_(this) {
   SetSyncStartFlare(sync_start_util::GetFlareForSyncableService(
       profile_->GetPath()));
 
   extension_service_->set_extension_sync_service(this);
   extension_prefs_->app_sorting()->SetExtensionSyncService(this);
 }
 
 ExtensionSyncService::~ExtensionSyncService() {}
 
 // static
 ExtensionSyncService* ExtensionSyncService::Get(
     content::BrowserContext* context) {
   return ExtensionSyncServiceFactory::GetForBrowserContext(context);
 }
 
 void ExtensionSyncService::SyncUninstallExtension(
     const extensions::Extension& extension) {
   if (!extensions::util::ShouldSync(&extension, profile_))
     return;
 
   // TODO(tim): If we get here and IsSyncing is false, this will cause
   // "back from the dead" style bugs, because sync will add-back the extension
   // that was uninstalled here when MergeDataAndStartSyncing is called.
   // See crbug.com/256795.

[Marc Treib, 2015/07/16 13:20:43]

I think we could handle this by setting NeedsSync on the uninstalled extension.
(Do extension prefs survive the uninstallation?)
Then in MergeDataAndStartSyncing, we can interpret "NeedsSync is set but the
extension isn't installed" as "we need to send a deletion to sync".
[BTW, I think this is what caused the problems with the KMOO extension.]

[not at google - send to devlin, 2015/07/16 18:25:46]

No, but IIRC there is a precedent for tombstone extension pref entries. I can't
remember what, worth looking into[*], but I agree that having a tombstone
extension pref entry with just the needs_sync bit set to true is close to the
right thing.

Another option to be to maintain a separate list, but I think a tombstone is
simpler. 
Yes that sounds likely. Ugh, good catch.


[*] we used to do this for blacklists but the tombstone entries actually caused
some funky bugs where they would get picked up as disabled, and then if you ever
tried to re-enable them the prefs for that extension would get corrupted and you
could never re-install that extension. Another place might be for external
extensions that get uninstalled - there needs to be a record that you
uninstalled it otherwise it's just going to get re-installed next chrome
startup. A similar problem really.

[Marc Treib, 2015/07/17 10:24:06]

I looked into ExtensionPrefs::IsExternalExtensionUninstalled, which is really
very similar to what we need here - basically a tombstone. As far as I can tell,
that one is just stored as an extension pref. So how does it survive the
uninstallation? If ExtensionPrefs are cleared on uninstallation, is this one
just set after the uninstall has happened, and so never gets cleared? That seems
kinda hacky and fragile, but still, the same approach might work here...

[not at google - send to devlin, 2015/07/17 15:28:50]

Ok maybe a separate pref would be better after all, and if the tombstone for
external extensions adds complexity we could use it there. Like,

"extensions": {
  "needs_sync_deletion": ["dafhsldjkfhaljksdlf", "kfhdlajksdf"],
  ...
}

we can discuss this later.

[Marc Treib, 2015/07/20 09:28:22]

Acknowledged.

   syncer::ModelType type =
       extension.is_app() ? syncer::APPS : syncer::EXTENSIONS;
   SyncBundle* bundle = GetSyncBundle(type);
   if (!bundle->IsSyncing()) {
     if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(type);  // Tell sync to start ASAP.
     return;
   }
-  const std::string& id = extension.id();
-  if (bundle->HasExtensionId(id))
-    bundle->PushSyncDeletion(id, CreateSyncData(extension).GetSyncData());
-}
-
-void ExtensionSyncService::SyncEnableExtension(const Extension& extension) {
-  // Syncing may not have started yet, so handle pending enables.
-  if (extensions::util::ShouldSync(&extension, profile_))
-    GetPendingEnables(extension.is_app())->Add(extension.id());
-
-  SyncExtensionChangeIfNeeded(extension);
-}
-
-void ExtensionSyncService::SyncDisableExtension(const Extension& extension) {
-  // Syncing may not have started yet, so handle pending enables.
-  if (extensions::util::ShouldSync(&extension, profile_))
-    GetPendingEnables(extension.is_app())->Remove(extension.id());
-
-  SyncExtensionChangeIfNeeded(extension);
+  bundle->PushSyncDeletion(extension.id(),
+                           CreateSyncData(extension).GetSyncData());
 }
 
 void ExtensionSyncService::SyncExtensionChangeIfNeeded(
     const Extension& extension) {
   if (!extensions::util::ShouldSync(&extension, profile_))
     return;
 
   syncer::ModelType type =
       extension.is_app() ? syncer::APPS : syncer::EXTENSIONS;
   SyncBundle* bundle = GetSyncBundle(type);
-  if (bundle->IsSyncing())
-    bundle->PushSyncAddOrUpdate(extension);
-  else if (extension_service_->is_ready() && !flare_.is_null())
-    flare_.Run(type);
+  if (bundle->IsSyncing()) {
+    bundle->PushSyncAddOrUpdate(extension.id(),
+                                CreateSyncData(extension).GetSyncData());
+    DCHECK(!ExtensionPrefs::Get(profile_)->NeedsSync(extension.id()));
+  } else {
+    ExtensionPrefs::Get(profile_)->SetNeedsSync(extension.id(), true);
+    if (extension_service_->is_ready() && !flare_.is_null())
+      flare_.Run(type);
+  }
 }
 
 syncer::SyncMergeResult ExtensionSyncService::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
     scoped_ptr<syncer::SyncErrorFactory> sync_error_factory) {
   CHECK(sync_processor.get());
   LOG_IF(FATAL, type != syncer::EXTENSIONS && type != syncer::APPS)
       << "Got " << type << " ModelType";
 
   SyncBundle* bundle = GetSyncBundle(type);
   bool is_apps = (type == syncer::APPS);
 
+  // Forward to the bundle which will apply the initial sync data, filtering
+  // out any items where we have more recent local changes.
   bundle->MergeDataAndStartSyncing(initial_sync_data, sync_processor.Pass());
-  GetPendingEnables(is_apps)->OnSyncStarted(extension_service_);
 
-  // Process local extensions.
-  // TODO(yoz): Determine whether pending extensions should be considered too.
-  //            See crbug.com/104399.
-  bundle->PushSyncDataList(GetAllSyncData(type));
+  // Now push those local changes to sync.
+  std::vector<ExtensionSyncData> data_list = GetLocalSyncDataList(type, true);
+  bundle->PushSyncDataList(ToSyncerSyncDataList(data_list));
+  for (const ExtensionSyncData& data : data_list)
+    ExtensionPrefs::Get(profile_)->SetNeedsSync(data.id(), false);
 
   if (is_apps)
     extension_prefs_->app_sorting()->FixNTPOrdinalCollisions();
 
   return syncer::SyncMergeResult(type);
 }
 
 void ExtensionSyncService::StopSyncing(syncer::ModelType type) {
   GetSyncBundle(type)->Reset();
 }
 
 syncer::SyncDataList ExtensionSyncService::GetAllSyncData(
     syncer::ModelType type) const {
-  std::vector<ExtensionSyncData> data = GetSyncDataList(type);
-  syncer::SyncDataList result;
-  result.reserve(data.size());
-  for (const ExtensionSyncData& item : data)
-    result.push_back(item.GetSyncData());
-  return result;
+  std::vector<extensions::ExtensionSyncData> sync_data_list =
+      GetLocalSyncDataList(type, false);
+
+  // Add pending data (where the local extension is either not installed yet or
+  // outdated).
+  std::vector<ExtensionSyncData> pending_extensions =
+      GetSyncBundle(type)->GetPendingData();
+  sync_data_list.insert(sync_data_list.begin(),
+                        pending_extensions.begin(),
+                        pending_extensions.end());
+
+  return ToSyncerSyncDataList(sync_data_list);
 }
 
 syncer::SyncError ExtensionSyncService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
   for (const syncer::SyncChange& sync_change : change_list) {
     syncer::ModelType type = sync_change.sync_data().GetDataType();
     GetSyncBundle(type)->ApplySyncChange(sync_change);
   }
 
@@ skipping 58 matching lines @@
 
   if (!ApplyExtensionSyncDataHelper(extension_sync_data, type)) {
     GetSyncBundle(type)->AddPendingExtension(id, extension_sync_data);
     extension_service_->CheckForUpdatesSoon();
     return false;
   }
 
   return true;
 }
 
+bool ExtensionSyncService::ExtensionNeedsSync(
+    const std::string& extension_id) const {
+  return ExtensionPrefs::Get(profile_)->NeedsSync(extension_id);
+}
+
 void ExtensionSyncService::ApplyBookmarkAppSyncData(
     const extensions::ExtensionSyncData& extension_sync_data) {
   DCHECK(extension_sync_data.is_app());
 
   // Process bookmark app sync if necessary.
   GURL bookmark_app_url(extension_sync_data.bookmark_app_url());
   if (!bookmark_app_url.is_valid() ||
       extension_sync_data.uninstalled()) {
     return;
   }
@@ skipping 45 matching lines @@
 
   if (ext)
     SyncExtensionChangeIfNeeded(*ext);
 }
 
 void ExtensionSyncService::SetSyncStartFlare(
     const syncer::SyncableService::StartSyncFlare& flare) {
   flare_ = flare;
 }
 
-bool ExtensionSyncService::IsPendingEnable(
-    const std::string& extension_id) const {
-  return pending_app_enables_.Contains(extension_id) ||
-      pending_extension_enables_.Contains(extension_id);
-}
-
 SyncBundle* ExtensionSyncService::GetSyncBundle(syncer::ModelType type) {
   return const_cast<SyncBundle*>(
       const_cast<const ExtensionSyncService&>(*this).GetSyncBundle(type));
 }
 
 const SyncBundle* ExtensionSyncService::GetSyncBundle(
     syncer::ModelType type) const {
   return (type == syncer::APPS) ? &app_sync_bundle_ : &extension_sync_bundle_;
 }
 
-extensions::PendingEnables* ExtensionSyncService::GetPendingEnables(
-    bool is_apps) {
-  return is_apps ? &pending_app_enables_ : &pending_extension_enables_;
-}
-
-std::vector<ExtensionSyncData> ExtensionSyncService::GetSyncDataList(
-    syncer::ModelType type) const {
+std::vector<ExtensionSyncData> ExtensionSyncService::GetLocalSyncDataList(
+    syncer::ModelType type,
+    bool only_with_needs_sync) const {

[not at google - send to devlin, 2015/07/16 18:25:47]

Sorry for nit but could we reverse this boolean? Make true mean to include all
sync data, false to mean only include things that have changed. Something about
that makes more sense to me. Maybe it's that true > false and all sync data >
only needs sync.

[Marc Treib, 2015/07/17 10:24:06]

Sure, done. Agreed that this makes it easier to read, somehow.

+  // Collect the local state. FillSyncDataList will filter out extensions for
+  // which we have pending data that should be used instead.
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
-  std::vector<ExtensionSyncData> extension_sync_list;
-  FillSyncDataList(registry->enabled_extensions(), type, &extension_sync_list);
-  FillSyncDataList(registry->disabled_extensions(), type, &extension_sync_list);
+  std::vector<ExtensionSyncData> data;
   FillSyncDataList(
-      registry->terminated_extensions(), type, &extension_sync_list);
-
-  std::vector<ExtensionSyncData> pending_extensions =
-      GetSyncBundle(type)->GetPendingData();
-  extension_sync_list.insert(extension_sync_list.begin(),
-                             pending_extensions.begin(),
-                             pending_extensions.end());
-
-  return extension_sync_list;
+      registry->enabled_extensions(), type, only_with_needs_sync, &data);
+  FillSyncDataList(
+      registry->disabled_extensions(), type, only_with_needs_sync, &data);
+  FillSyncDataList(
+      registry->terminated_extensions(), type, only_with_needs_sync, &data);
+  return data;
 }
 
 void ExtensionSyncService::FillSyncDataList(
     const ExtensionSet& extensions,
     syncer::ModelType type,
+    bool only_with_needs_sync,
     std::vector<ExtensionSyncData>* sync_data_list) const {
   const SyncBundle* bundle = GetSyncBundle(type);
+  // TODO(treib,kalman): This seems weird. Remove the IsSyncing check?
+  if (!bundle->IsSyncing())
+    return;
   for (const scoped_refptr<const Extension>& extension : extensions) {
     if (IsCorrectSyncType(*extension, type) &&
         extensions::util::ShouldSync(extension.get(), profile_) &&
-        bundle->ShouldIncludeInLocalSyncDataList(*extension)) {
+        !bundle->HasPendingExtensionId(extension->id()) &&
+        (!only_with_needs_sync || ExtensionNeedsSync(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_);
@@ skipping 35 matching lines @@
     // local one. Otherwise we just enable it without granting permissions. If
     // any permissions are missing, CheckPermissionsIncrease will soon disable
     // it again.
     bool grant_permissions =
         extension_sync_data.supports_disable_reasons() &&
         extension && (version_compare_result == 0);
     if (grant_permissions)
       extension_service_->GrantPermissionsAndEnableExtension(extension);
     else
       extension_service_->EnableExtension(id);
-  } else if (!IsPendingEnable(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), from before we synced disable reasons (see
@@ skipping 51 matching lines @@
       // 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;
 }