Revert 150990 - [Sync] Avoid unregistering object IDs on shutdown

chrome/browser/sync/glue/sync_backend_host.cc

Index: chrome/browser/sync/glue/sync_backend_host.cc
===================================================================
--- chrome/browser/sync/glue/sync_backend_host.cc	(revision 150991)
+++ chrome/browser/sync/glue/sync_backend_host.cc	(working copy)
@@ -170,7 +170,6 @@
   //    (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(
@@ -246,13 +245,6 @@
   // 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);
 };
 
@@ -549,15 +541,11 @@
 
 void SyncBackendHost::StopSyncingForShutdown() {
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
-
-  // Immediately stop sending messages to the frontend.
-  frontend_ = NULL;
-
   // Thread shutdown should occur in the following order:
   // - Sync Thread
   // - UI Thread (stops some time after we return from this call).
   //
-  // In order to achieve this, we first shutdown components from the UI thread
+  // In order to acheive 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
@@ -582,23 +570,17 @@
     // If the sync thread isn't running, then the syncer is effectively
     // stopped.  Moreover, it implies that we never attempted initialization,
     // so the registrar won't need stopping either.
-    DCHECK_EQ(initialization_state_, NOT_ATTEMPTED);
+    DCHECK_EQ(NOT_ATTEMPTED, initialization_state_);
     DCHECK(!registrar_.get());
   }
 }
 
 void SyncBackendHost::Shutdown(bool sync_disabled) {
-  // StopSyncingForShutdown() (which nulls out |frontend_|) should be
-  // called first.
-  DCHECK(!frontend_);
   // 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 (chrome_sync_notification_bridge_.get())
-      chrome_sync_notification_bridge_->StopForShutdown();
   }
 
   // Stop will return once the thread exits, which will be after DoShutdown
@@ -623,6 +605,7 @@
                       stop_sync_thread_time);
 
   registrar_.reset();
+  frontend_ = NULL;
   chrome_sync_notification_bridge_.reset();
   core_ = NULL;  // Releases reference to core_.
 }
@@ -863,8 +846,7 @@
       host_(backend),
       sync_loop_(NULL),
       registrar_(NULL),
-      chrome_sync_notification_bridge_(NULL),
-      registered_as_invalidation_handler_(false) {
+      chrome_sync_notification_bridge_(NULL) {
   DCHECK(backend.get());
 }
 
@@ -910,7 +892,11 @@
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   if (!success) {
-    DoDestroySyncManager();
+    sync_manager_->RemoveObserver(this);
+    sync_manager_->UpdateRegisteredInvalidationIds(
+        this, syncer::ObjectIdSet());
+    sync_manager_->ShutdownOnSyncThread();
+    sync_manager_.reset();
   }
 
   host_.Call(
@@ -1099,24 +1085,15 @@
       options.unrecoverable_error_handler,
       options.report_unrecoverable_error_function);
 
-  // |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_->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
-    // 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();
-    }
+  // 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();
   }
 }
 
@@ -1133,7 +1110,7 @@
   // synchronous initialization mode) since this is called during
   // shutdown.
   //
-  // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
+  // TODO(akalin): Fix this behavior.
   if (sync_manager_.get()) {
     sync_manager_->UpdateRegisteredInvalidationIds(this, ids);
   }
@@ -1182,7 +1159,14 @@
 
 void SyncBackendHost::Core::DoShutdown(bool sync_disabled) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  DoDestroySyncManager();
+  if (sync_manager_.get()) {
+    save_changes_timer_.reset();
+    sync_manager_->UpdateRegisteredInvalidationIds(
+        this, syncer::ObjectIdSet());
+    sync_manager_->ShutdownOnSyncThread();
+    sync_manager_->RemoveObserver(this);
+    sync_manager_.reset();
+  }
 
   chrome_sync_notification_bridge_ = NULL;
   registrar_ = NULL;
@@ -1195,20 +1179,6 @@
   host_.Reset();
 }
 
-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();
-  }
-}
-
 void SyncBackendHost::Core::DoConfigureSyncer(
     syncer::ConfigureReason reason,
     syncer::ModelTypeSet types_to_config,
@@ -1299,7 +1269,7 @@
 
 void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend, bool success) {
-  DCHECK_NE(initialization_state_, NOT_ATTEMPTED);
+  DCHECK_NE(NOT_ATTEMPTED, initialization_state_);
   if (!frontend_)
     return;