[Sync] Avoid unregistering object IDs on shutdown

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 9b7d19d2c7a57677bb69b5fbf07f0e43af463a78..3e316cc1dce871718d296b0fc5196c6a43c64133 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -69,6 +69,8 @@ using syncer::InternalComponentsFactoryImpl;
 using syncer::sessions::SyncSessionSnapshot;
 using syncer::SyncCredentials;
 
+const char kHandlerName[] = "SyncBackendHost::Core";
+
 // Helper macros to log with the syncer thread name; useful when there
 // are multiple syncers involved.
 
@@ -170,6 +172,7 @@ class SyncBackendHost::Core
   //    (in step 2).
   void DoStopSyncManagerForShutdown(const base::Closure& closure);
   void DoShutdown(bool stopping_sync);
+  void DoDestroySyncManager();
 
   // Configuration methods that must execute on sync loop.
   void DoConfigureSyncer(
@@ -313,7 +316,8 @@ SyncBackendHost::SyncBackendHost(
                                profile_->GetRequestContext()),
           content::GetUserAgent(GURL()),
           invalidator_storage),
-      frontend_(NULL) {
+      frontend_(NULL),
+      propagate_invalidations_(true) {
 }
 
 SyncBackendHost::SyncBackendHost(Profile* profile)
@@ -541,11 +545,15 @@ void SyncBackendHost::StopSyncManagerForShutdown(
 
 void SyncBackendHost::StopSyncingForShutdown() {
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
+
+  // Stop propagating notifications to invalidation handlers.
+  propagate_invalidations_ = false;
+
   // Thread shutdown should occur in the following order:
   // - Sync Thread
   // - UI Thread (stops some time after we return from this call).
   //
-  // In order to acheive this, we first shutdown components from the UI thread
+  // In order to achieve this, we first shutdown components from the UI thread
   // and send signals to abort components that may be busy on the sync thread.
   // The callback (OnSyncerShutdownComplete) will happen on the sync thread,
   // after which we'll shutdown components on the sync thread, and then be
@@ -581,6 +589,10 @@ void SyncBackendHost::Shutdown(bool sync_disabled) {
     sync_thread_.message_loop()->PostTask(FROM_HERE,
         base::Bind(&SyncBackendHost::Core::DoShutdown, core_.get(),
                    sync_disabled));
+
+    if (chrome_sync_notification_bridge_.get()) {

[msw, 2012/08/03 23:30:46]

nit: remove unnecessary {}

[akalin, 2012/08/07 07:25:19]

Done.

+      chrome_sync_notification_bridge_->StopForShutdown();
+    }
   }
 
   // Stop will return once the thread exits, which will be after DoShutdown
@@ -892,11 +904,7 @@ void SyncBackendHost::Core::OnInitializationComplete(
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   if (!success) {
-    sync_manager_->RemoveObserver(this);
-    sync_manager_->UpdateRegisteredInvalidationIds(
-        this, syncer::ObjectIdSet());
-    sync_manager_->ShutdownOnSyncThread();
-    sync_manager_.reset();
+    DoDestroySyncManager();
   }
 
   host_.Call(
@@ -1085,15 +1093,23 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
       options.report_unrecoverable_error_function);
   LOG_IF(ERROR, !success) << "Sync manager initialization failed!";
 
-  // 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
-  // flag only when restarting the browser with an account already setup. If
-  // you use this before setting up the setup would not succeed as an error
-  // would be encountered.
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncThrowUnrecoverableError)) {
-    sync_manager_->ThrowUnrecoverableError();
+  // |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_->SetInvalidationHandler(kHandlerName, this);
+
+    // 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
+    // flag only when restarting the browser with an account already setup. If
+    // you use this before setting up the setup would not succeed as an error
+    // would be encountered.
+    if (CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kSyncThrowUnrecoverableError)) {
+      sync_manager_->ThrowUnrecoverableError();
+    }
   }
 }
 
@@ -1110,9 +1126,9 @@ void SyncBackendHost::Core::DoUpdateRegisteredInvalidationIds(
   // synchronous initialization mode) since this is called during
   // shutdown.
   //
-  // TODO(akalin): Fix this behavior.
+  // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
   if (sync_manager_.get()) {
-    sync_manager_->UpdateRegisteredInvalidationIds(this, ids);
+    sync_manager_->UpdateRegisteredInvalidationIds(kHandlerName, ids);
   }
 }
 
@@ -1159,14 +1175,7 @@ void SyncBackendHost::Core::DoStopSyncManagerForShutdown(
 
 void SyncBackendHost::Core::DoShutdown(bool sync_disabled) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (sync_manager_.get()) {
-    save_changes_timer_.reset();
-    sync_manager_->UpdateRegisteredInvalidationIds(
-        this, syncer::ObjectIdSet());
-    sync_manager_->ShutdownOnSyncThread();
-    sync_manager_->RemoveObserver(this);
-    sync_manager_.reset();
-  }
+  DoDestroySyncManager();
 
   chrome_sync_notification_bridge_ = NULL;
   registrar_ = NULL;
@@ -1179,6 +1188,17 @@ void SyncBackendHost::Core::DoShutdown(bool sync_disabled) {
   host_.Reset();
 }
 
+void SyncBackendHost::Core::DoDestroySyncManager() {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  if (sync_manager_.get()) {
+    save_changes_timer_.reset();
+    sync_manager_->SetInvalidationHandler(kHandlerName, NULL);
+    sync_manager_->RemoveObserver(this);
+    sync_manager_->ShutdownOnSyncThread();
+    sync_manager_.reset();
+  }
+}
+
 void SyncBackendHost::Core::DoConfigureSyncer(
     syncer::ConfigureReason reason,
     syncer::ModelTypeSet types_to_config,
@@ -1382,7 +1402,7 @@ void SyncBackendHost::HandleActionableErrorEventOnFrontendLoop(
 }
 
 void SyncBackendHost::HandleNotificationsEnabledOnFrontendLoop() {
-  if (!frontend_)
+  if (!frontend_ || !propagate_invalidations_)
     return;
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
   frontend_->OnNotificationsEnabled();
@@ -1390,7 +1410,7 @@ void SyncBackendHost::HandleNotificationsEnabledOnFrontendLoop() {
 
 void SyncBackendHost::HandleNotificationsDisabledOnFrontendLoop(
     syncer::NotificationsDisabledReason reason) {
-  if (!frontend_)
+  if (!frontend_ || !propagate_invalidations_)
     return;
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
   frontend_->OnNotificationsDisabled(reason);
@@ -1399,7 +1419,7 @@ void SyncBackendHost::HandleNotificationsDisabledOnFrontendLoop(
 void SyncBackendHost::HandleIncomingNotificationOnFrontendLoop(
     const syncer::ObjectIdPayloadMap& id_payloads,
     syncer::IncomingNotificationSource source) {
-  if (!frontend_)
+  if (!frontend_ || !propagate_invalidations_)
     return;
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
   frontend_->OnIncomingNotification(id_payloads, source);