Draft: InvalidationService

chrome/browser/sync/glue/sync_backend_host.cc

Index: chrome/browser/sync/glue/sync_backend_host.cc
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 7d720b08ae9d2f4128bd9e0a9655911acb1650da..c02b1406dae9519d74ae07a452db73893947ce8b 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -21,17 +21,16 @@
 #include "base/tracked_objects.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/invalidation_service.h"
+#include "chrome/browser/invalidation_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/token_service.h"
 #include "chrome/browser/signin/token_service_factory.h"
-#include "chrome/browser/sync/glue/android_invalidator_bridge.h"
-#include "chrome/browser/sync/glue/android_invalidator_bridge_proxy.h"
 #include "chrome/browser/sync/glue/change_processor.h"
 #include "chrome/browser/sync/glue/chrome_encryptor.h"
 #include "chrome/browser/sync/glue/device_info.h"
 #include "chrome/browser/sync/glue/sync_backend_registrar.h"
 #include "chrome/browser/sync/glue/synced_device_tracker.h"
-#include "chrome/browser/sync/invalidations/invalidator_storage.h"
 #include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -55,7 +54,6 @@
 #include "sync/internal_api/public/sync_manager_factory.h"
 #include "sync/internal_api/public/util/experiments.h"
 #include "sync/internal_api/public/util/sync_string_conversions.h"
-#include "sync/notifier/invalidator.h"
 #include "sync/protocol/encryption.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/util/nigori.h"
@@ -86,8 +84,7 @@ using syncer::SyncCredentials;
 class SyncBackendHost::Core
     : public base::RefCountedThreadSafe<SyncBackendHost::Core>,
       public syncer::SyncEncryptionHandler::Observer,
-      public syncer::SyncManager::Observer,
-      public syncer::InvalidationHandler {
+      public syncer::SyncManager::Observer {
  public:
   Core(const std::string& name,
        const base::FilePath& sync_data_folder_path,
@@ -128,11 +125,12 @@ class SyncBackendHost::Core
   virtual void OnPassphraseTypeChanged(syncer::PassphraseType type,
                                        base::Time passphrase_time) OVERRIDE;
 
-  // syncer::InvalidationHandler implementation.
-  virtual void OnInvalidatorStateChange(
-      syncer::InvalidatorState state) OVERRIDE;
-  virtual void OnIncomingInvalidation(
-      const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
+  // Forwards an invalidation state change to the sync manager.
+  void DoOnInvalidatorStateChange(syncer::InvalidatorState state);
+
+  // Forwards an invalidation to the sync manager.
+  void DoOnIncomingInvalidation(
+      syncer::ObjectIdInvalidationMap invalidation_map);
 
   // Note:
   //
@@ -149,15 +147,6 @@ class SyncBackendHost::Core
   // SyncBackendHost::UpdateCredentials.
   void DoUpdateCredentials(const syncer::SyncCredentials& credentials);
 
-  // Called to update the given registered ids on behalf of
-  // SyncBackendHost::UpdateRegisteredInvalidationIds.
-  void DoUpdateRegisteredInvalidationIds(const syncer::ObjectIdSet& ids);
-
-  // Called to acknowledge an invalidation on behalf of
-  // SyncBackendHost::AcknowledgeInvalidation.
-  void DoAcknowledgeInvalidation(const invalidation::ObjectId& id,
-                                 const syncer::AckHandle& ack_handle);
-
   // Called to tell the syncapi to start syncing (generally after
   // initialization and authentication).
   void DoStartSyncing(const syncer::ModelSafeRoutingInfo& routing_info);
@@ -287,67 +276,13 @@ class SyncBackendHost::Core
   // The top-level syncapi entry point.  Lives on the sync thread.
   scoped_ptr<syncer::SyncManager> sync_manager_;
 
-  // Whether or not we registered with |sync_manager_| as an invalidation
-  // handler.  Necessary since we may end up trying to unregister before we
-  // register in tests (in synchronous initialization mode).
-  //
-  // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
-  bool registered_as_invalidation_handler_;
-
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
-namespace {
-
-// Parses the given command line for notifier options.
-notifier::NotifierOptions ParseNotifierOptions(
-    const CommandLine& command_line,
-    const scoped_refptr<net::URLRequestContextGetter>&
-        request_context_getter) {
-  notifier::NotifierOptions notifier_options;
-  notifier_options.request_context_getter = request_context_getter;
-
-  if (command_line.HasSwitch(switches::kSyncNotificationHostPort)) {
-    notifier_options.xmpp_host_port =
-        net::HostPortPair::FromString(
-            command_line.GetSwitchValueASCII(
-                switches::kSyncNotificationHostPort));
-    DVLOG(1) << "Using " << notifier_options.xmpp_host_port.ToString()
-             << " for test sync notification server.";
-  }
-
-  notifier_options.try_ssltcp_first =
-      command_line.HasSwitch(switches::kSyncTrySsltcpFirstForXmpp);
-  DVLOG_IF(1, notifier_options.try_ssltcp_first)
-      << "Trying SSL/TCP port before XMPP port for notifications.";
-
-  notifier_options.invalidate_xmpp_login =
-      command_line.HasSwitch(switches::kSyncInvalidateXmppLogin);
-  DVLOG_IF(1, notifier_options.invalidate_xmpp_login)
-      << "Invalidating sync XMPP login.";
-
-  notifier_options.allow_insecure_connection =
-      command_line.HasSwitch(switches::kSyncAllowInsecureXmppConnection);
-  DVLOG_IF(1, notifier_options.allow_insecure_connection)
-      << "Allowing insecure XMPP connections.";
-
-  if (command_line.HasSwitch(switches::kSyncNotificationMethod)) {
-    const std::string notification_method_str(
-        command_line.GetSwitchValueASCII(switches::kSyncNotificationMethod));
-    notifier_options.notification_method =
-        notifier::StringToNotificationMethod(notification_method_str);
-  }
-
-  return notifier_options;
-}
-
-}  // namespace
-
 SyncBackendHost::SyncBackendHost(
     const std::string& name,
     Profile* profile,
-    const base::WeakPtr<SyncPrefs>& sync_prefs,
-    const base::WeakPtr<InvalidatorStorage>& invalidator_storage)
+    const base::WeakPtr<SyncPrefs>& sync_prefs)
     : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       sync_thread_("Chrome_SyncThread"),
       frontend_loop_(MessageLoop::current()),
@@ -357,13 +292,9 @@ SyncBackendHost::SyncBackendHost(
                      weak_ptr_factory_.GetWeakPtr())),
       initialization_state_(NOT_ATTEMPTED),
       sync_prefs_(sync_prefs),
-      invalidator_factory_(
-          ParseNotifierOptions(*CommandLine::ForCurrentProcess(),
-                               profile_->GetRequestContext()),
-          content::GetUserAgent(GURL()),
-          invalidator_storage),
       frontend_(NULL),
-      cached_passphrase_type_(syncer::IMPLICIT_PASSPHRASE) {
+      cached_passphrase_type_(syncer::IMPLICIT_PASSPHRASE),
+      invalidator_(InvalidationServiceFactory::GetForProfile(profile)) {
 }
 
 SyncBackendHost::SyncBackendHost(Profile* profile)
@@ -373,18 +304,12 @@ SyncBackendHost::SyncBackendHost(Profile* profile)
       profile_(profile),
       name_("Unknown"),
       initialization_state_(NOT_ATTEMPTED),
-      invalidator_factory_(
-          ParseNotifierOptions(*CommandLine::ForCurrentProcess(),
-                               profile_->GetRequestContext()),
-          content::GetUserAgent(GURL()),
-          base::WeakPtr<syncer::InvalidationStateTracker>()),
       frontend_(NULL),
       cached_passphrase_type_(syncer::IMPLICIT_PASSPHRASE) {
 }
 
 SyncBackendHost::~SyncBackendHost() {
   DCHECK(!core_ && !frontend_) << "Must call Shutdown before destructor.";
-  DCHECK(!android_invalidator_bridge_.get());
   DCHECK(!registrar_.get());
 }
 
@@ -413,10 +338,6 @@ void SyncBackendHost::Initialize(
   if (!sync_thread_.Start())
     return;
 
-  android_invalidator_bridge_.reset(
-      new AndroidInvalidatorBridge(
-          profile_, sync_thread_.message_loop_proxy()));
-
   frontend_ = frontend;
   DCHECK(frontend);
 
@@ -439,6 +360,8 @@ void SyncBackendHost::Initialize(
         InternalComponentsFactoryImpl::BACKOFF_SHORT_INITIAL_RETRY_OVERRIDE;
   }
 
+  invalidator_->RegisterInvalidationHandler(this);
+
   initialization_state_ = CREATING_SYNC_MANAGER;
   InitCore(DoInitializeOptions(
       sync_thread_.message_loop(),
@@ -451,8 +374,7 @@ void SyncBackendHost::Initialize(
       base::Bind(&MakeHttpBridgeFactory,
                  make_scoped_refptr(profile_->GetRequestContext())),
       credentials,
-      android_invalidator_bridge_.get(),
-      &invalidator_factory_,
+      invalidator_->GetInvalidatorClientId(),
       sync_manager_factory,
       delete_sync_data_folder,
       sync_prefs_->GetEncryptionBootstrapToken(),
@@ -469,24 +391,6 @@ void SyncBackendHost::UpdateCredentials(const SyncCredentials& credentials) {
                  credentials));
 }
 
-void SyncBackendHost::UpdateRegisteredInvalidationIds(
-    const syncer::ObjectIdSet& ids) {
-  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
-  DCHECK(sync_thread_.IsRunning());
-  sync_thread_.message_loop()->PostTask(FROM_HERE,
-      base::Bind(&SyncBackendHost::Core::DoUpdateRegisteredInvalidationIds,
-                 core_.get(), ids));
-}
-
-void SyncBackendHost::AcknowledgeInvalidation(
-    const invalidation::ObjectId& id, const syncer::AckHandle& ack_handle) {
-  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
-  DCHECK(sync_thread_.IsRunning());
-  sync_thread_.message_loop()->PostTask(FROM_HERE,
-      base::Bind(&SyncBackendHost::Core::DoAcknowledgeInvalidation,
-                 core_.get(), id, ack_handle));
-}
-
 void SyncBackendHost::StartSyncingWithServer() {
   SDVLOG(1) << "SyncBackendHost::StartSyncingWithServer called.";
 
@@ -630,14 +534,15 @@ void SyncBackendHost::Shutdown(bool sync_disabled) {
   // StopSyncingForShutdown() (which nulls out |frontend_|) should be
   // called first.
   DCHECK(!frontend_);
+
+  invalidator_->UnregisterInvalidationHandler(this);

[tim (not reviewing), 2013/04/15 16:48:24]

I'm trying to convince myself this is a safe change to shutdown ordering. We
were pretty careful in when we stopped the invalidator vs TerminateAllIO vs
scheduler vs InvalidateWeakPtrs vs SaveChanges and all that stuff in
SyncManager.

But I think this is the right thing to do now, since invalidations don't flow
from SyncManager.  Just calling it out to make sure you / Fred consider shutdown
ordering carefully.

[rlarocque, 2013/04/22 21:47:15]

It's actually pretty easy to verify that this is safe.  We only dereference the
invalidator_ pointer:
- When fetching the client ID, during init.
- When ACKing a received invalidation.
- When updating the list of enabled types following a configure. 
(FinishConfigureDataTypesOnFrontendLoop)

None of which should happen after Shutdown has been called.

+  invalidator_ = NULL;
+
   // TODO(tim): DCHECK(registrar_->StoppedOnUIThread()) would be nice.
   if (sync_thread_.IsRunning()) {
     sync_thread_.message_loop()->PostTask(FROM_HERE,
         base::Bind(&SyncBackendHost::Core::DoShutdown, core_.get(),
                    sync_disabled));
-
-    if (android_invalidator_bridge_.get())
-      android_invalidator_bridge_->StopForShutdown();
   }
 
   // Stop will return once the thread exits, which will be after DoShutdown
@@ -663,7 +568,6 @@ void SyncBackendHost::Shutdown(bool sync_disabled) {
 
   registrar_.reset();
   js_backend_.Reset();
-  android_invalidator_bridge_.reset();
   core_ = NULL;  // Releases reference to core_.
 }
 
@@ -833,11 +737,18 @@ void SyncBackendHost::RequestConfigureSyncer(
 }
 
 void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop(
+    syncer::ModelTypeSet enabled_types,
     syncer::ModelTypeSet failed_configuration_types,
     const base::Callback<void(syncer::ModelTypeSet)>& ready_task) {
+  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
+
   if (!frontend_)
     return;
-  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
+
+  invalidator_->UpdateRegisteredInvalidationIds(
+      this,
+      ModelTypeSetToObjectIdSet(enabled_types));
+
   if (!ready_task.is_null())
     ready_task.Run(failed_configuration_types);
 }
@@ -863,6 +774,10 @@ void SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop(
   notification_registrar_.Add(this, chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
                               content::Source<Profile>(profile_));
 
+  // Fake a state change to initialize the SyncManager's cached invalidator
+  // state.
+  OnInvalidatorStateChange(invalidator_->GetInvalidatorState());
+
   // Kick off the next step in SyncBackendHost initialization by downloading
   // any necessary control types.
   sync_thread_.message_loop()->PostTask(
@@ -898,8 +813,7 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
     const GURL& service_url,
     MakeHttpBridgeFactoryFn make_http_bridge_factory_fn,
     const syncer::SyncCredentials& credentials,
-    AndroidInvalidatorBridge* android_invalidator_bridge,
-    syncer::InvalidatorFactory* invalidator_factory,
+    const std::string& invalidator_client_id,
     syncer::SyncManagerFactory* sync_manager_factory,
     bool delete_sync_data_folder,
     const std::string& restored_key_for_bootstrapping,
@@ -917,8 +831,7 @@ SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
       service_url(service_url),
       make_http_bridge_factory_fn(make_http_bridge_factory_fn),
       credentials(credentials),
-      android_invalidator_bridge(android_invalidator_bridge),
-      invalidator_factory(invalidator_factory),
+      invalidator_client_id(invalidator_client_id),
       sync_manager_factory(sync_manager_factory),
       delete_sync_data_folder(delete_sync_data_folder),
       restored_key_for_bootstrapping(restored_key_for_bootstrapping),
@@ -939,8 +852,7 @@ SyncBackendHost::Core::Core(const std::string& name,
       sync_data_folder_path_(sync_data_folder_path),
       host_(backend),
       sync_loop_(NULL),
-      registrar_(NULL),
-      registered_as_invalidation_handler_(false) {
+      registrar_(NULL) {
   DCHECK(backend.get());
 }
 
@@ -1132,24 +1044,16 @@ void SyncBackendHost::Core::OnActionableError(
       sync_error);
 }
 
-void SyncBackendHost::Core::OnInvalidatorStateChange(
+void SyncBackendHost::Core::DoOnInvalidatorStateChange(
     syncer::InvalidatorState state) {
-  if (!sync_loop_)
-    return;
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  host_.Call(FROM_HERE,
-             &SyncBackendHost::HandleInvalidatorStateChangeOnFrontendLoop,
-             state);
+  sync_manager_->UpdateInvalidatorState(state);
 }
 
-void SyncBackendHost::Core::OnIncomingInvalidation(
-    const syncer::ObjectIdInvalidationMap& invalidation_map) {
-  if (!sync_loop_)
-    return;
+void SyncBackendHost::Core::DoOnIncomingInvalidation(
+    syncer::ObjectIdInvalidationMap invalidation_map) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  host_.Call(FROM_HERE,
-             &SyncBackendHost::HandleIncomingInvalidationOnFrontendLoop,
-             invalidation_map);
+  sync_manager_->Invalidate(invalidation_map);
 }
 
 void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
@@ -1186,15 +1090,7 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
       options.extensions_activity_monitor,
       options.registrar /* as SyncManager::ChangeDelegate */,
       options.credentials,
-#if defined(OS_ANDROID)
-      scoped_ptr<syncer::Invalidator>(
-          new AndroidInvalidatorBridgeProxy(
-              options.android_invalidator_bridge)),
-#else
-      scoped_ptr<syncer::Invalidator>(
-          options.invalidator_factory->CreateInvalidator()),
-#endif
-      options.invalidator_factory->GetInvalidatorClientId(),
+      options.invalidator_client_id,
       options.restored_key_for_bootstrapping,
       options.restored_keystore_key_for_bootstrapping,
       scoped_ptr<InternalComponentsFactory>(
@@ -1208,9 +1104,6 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
   //
   // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
   if (sync_manager_.get()) {
-    sync_manager_->RegisterInvalidationHandler(this);
-    registered_as_invalidation_handler_ = true;
-
     // Now check the command line to see if we need to simulate an
     // unrecoverable error for testing purpose. Note the error is thrown
     // only if the initialization succeeded. Also it makes sense to use this
@@ -1236,31 +1129,6 @@ void SyncBackendHost::Core::DoUpdateCredentials(
   }
 }
 
-void SyncBackendHost::Core::DoUpdateRegisteredInvalidationIds(
-    const syncer::ObjectIdSet& ids) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  // |sync_manager_| may end up being NULL here in tests (in
-  // synchronous initialization mode) since this is called during
-  // shutdown.
-  //
-  // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
-  if (sync_manager_.get()) {
-    sync_manager_->UpdateRegisteredInvalidationIds(this, ids);
-  }
-}
-
-void SyncBackendHost::Core::DoAcknowledgeInvalidation(
-    const invalidation::ObjectId& id, const syncer::AckHandle& ack_handle) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  // |sync_manager_| may end up being NULL here in tests (in
-  // synchronous initialization mode).
-  //
-  // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
-  if (sync_manager_.get()) {
-    sync_manager_->AcknowledgeInvalidation(id, ack_handle);
-  }
-}
-
 void SyncBackendHost::Core::DoStartSyncing(
     const syncer::ModelSafeRoutingInfo& routing_info) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
@@ -1370,10 +1238,6 @@ void SyncBackendHost::Core::DoDestroySyncManager() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (sync_manager_.get()) {
     save_changes_timer_.reset();
-    if (registered_as_invalidation_handler_) {
-      sync_manager_->UnregisterInvalidationHandler(this);
-      registered_as_invalidation_handler_ = false;
-    }
     sync_manager_->RemoveObserver(this);
     sync_manager_->ShutdownOnSyncThread();
     sync_manager_.reset();
@@ -1412,12 +1276,12 @@ void SyncBackendHost::Core::DoFinishConfigureDataTypes(
   registrar_->GetModelSafeRoutingInfo(&routing_info);
   syncer::ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
   enabled_types.RemoveAll(syncer::ProxyTypes());
-  sync_manager_->UpdateEnabledTypes(enabled_types);
 
   const syncer::ModelTypeSet failed_configuration_types =
       Difference(types_to_config, sync_manager_->InitialSyncEndedTypes());
   host_.Call(FROM_HERE,
              &SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop,
+             enabled_types,
              failed_configuration_types,
              ready_task);
 }
@@ -1548,20 +1412,30 @@ void SyncBackendHost::HandleActionableErrorEventOnFrontendLoop(
   frontend_->OnActionableError(sync_error);
 }
 
-void SyncBackendHost::HandleInvalidatorStateChangeOnFrontendLoop(
-    syncer::InvalidatorState state) {
-  if (!frontend_)
-    return;
-  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
-  frontend_->OnInvalidatorStateChange(state);
+void SyncBackendHost::OnInvalidatorStateChange(syncer::InvalidatorState state) {
+  sync_thread_.message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncBackendHost::Core::DoOnInvalidatorStateChange,
+                 core_.get(),
+                 state));
 }
 
-void SyncBackendHost::HandleIncomingInvalidationOnFrontendLoop(
+void SyncBackendHost::OnIncomingInvalidation(
     const syncer::ObjectIdInvalidationMap& invalidation_map) {
-  if (!frontend_)
-    return;
-  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
-  frontend_->OnIncomingInvalidation(invalidation_map);
+  // TODO(dcheng): Acknowledge immediately for now. Fix this once the
+  // invalidator doesn't repeatedly ping for unacknowledged invaliations, since
+  // it conflicts with the sync scheduler's internal backoff algorithm.
+  // See http://crbug.com/124149 for more information.
+  for (syncer::ObjectIdInvalidationMap::const_iterator it =
+       invalidation_map.begin(); it != invalidation_map.end(); ++it) {
+    invalidator_->AcknowledgeInvalidation(it->first, it->second.ack_handle);
+  }
+
+  sync_thread_.message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncBackendHost::Core::DoOnIncomingInvalidation,
+                 core_.get(),
+                 invalidation_map));
 }
 
 bool SyncBackendHost::CheckPassphraseAgainstCachedPendingKeys(