[Sync] Avoid unregistering object IDs on shutdown

chrome/browser/sync/glue/sync_backend_host.cc

 // Copyright (c) 2012 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 "build/build_config.h"
 
 #include "chrome/browser/sync/glue/sync_backend_host.h"
 
 #include <algorithm>
 #include <map>
@@ skipping 51 matching lines @@
 typedef GoogleServiceAuthError AuthError;
 
 namespace browser_sync {
 
 using content::BrowserThread;
 using syncer::InternalComponentsFactory;
 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.
 
 #define SLOG(severity) LOG(severity) << name_ << ": "
 
 #define SDVLOG(verbose_level) DVLOG(verbose_level) << name_ << ": "
 
 class SyncBackendHost::Core
     : public base::RefCountedThreadSafe<SyncBackendHost::Core>,
       public syncer::SyncManager::Observer,
@@ skipping 81 matching lines @@
   //    a final SaveChanges, and close sqlite handles.
   // 2) Then, from |frontend_loop_|, halt the sync_thread_ (which is
   //    a blocking call). This causes syncapi thread-exit handlers
   //    to run and make use of cached pointers to various components
   //    owned implicitly by us.
   // 3) Destroy this Core. That will delete syncapi components in a
   //    safe order because the thread that was using them has exited
   //    (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(
       syncer::ConfigureReason reason,
       syncer::ModelTypeSet types_to_config,
       const syncer::ModelSafeRoutingInfo routing_info,
       const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
       const base::Closure& retry_callback);
   void DoFinishConfigureDataTypes(
       syncer::ModelTypeSet types_to_config,
@@ skipping 351 matching lines @@
             &SyncBackendHost::Core::DoStopSyncManagerForShutdown,
             core_.get(),
             closure));
   } else {
     core_->DoStopSyncManagerForShutdown(closure);
   }
 }
 
 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 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
   // able to stop the sync loop.
   if (sync_thread_.IsRunning()) {
     StopSyncManagerForShutdown(
         base::Bind(&SyncBackendRegistrar::OnSyncerShutdownComplete,
                    base::Unretained(registrar_.get())));
 
     // Before joining the sync_thread_, we wait for the UIModelWorker to
     // give us the green light that it is not depending on the frontend_loop_
     // to process any more tasks. Stop() blocks until this termination
     // condition is true.
     base::Time stop_registrar_start_time = base::Time::Now();
     if (registrar_.get())
       registrar_->StopOnUIThread();
     base::TimeDelta stop_registrar_time = base::Time::Now() -
         stop_registrar_start_time;
     UMA_HISTOGRAM_TIMES("Sync.Shutdown.StopRegistrarTime",
                         stop_registrar_time);
   } else {
     // 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(NOT_ATTEMPTED, initialization_state_);
+    DCHECK_EQ(initialization_state_, NOT_ATTEMPTED);

[tim (not reviewing), 2012/08/07 16:39:38]

hm, so I've always followed the pattern from ASSERT/EXPECT in tests, where the
first parameter to the macro is "expected" and the second is "actual".  

[akalin, 2012/08/07 17:19:23]

I think that's only because EXPECT makes a distinction between the two arguments
(expected is first).  Since DCHECK_EQ doesn't, there's no reason to not put the
arguments in natural order.  I think that's the accepted style.

     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
   // runs. DoShutdown needs to run from sync_thread_ because the sync backend
   // requires any thread that opened sqlite handles to relinquish them
   // personally. We need to join threads, because otherwise the main Chrome
   // thread (ui loop) can exit before DoShutdown finishes, at which point
   // virtually anything the sync backend does (or the post-back to
   // frontend_loop_ by our Core) will epically fail because the CRT won't be
   // initialized.
   // Since we are blocking the UI thread here, we need to turn ourselves in
   // with the ThreadRestriction police.  For sentencing and how we plan to fix
   // this, see bug 19757.
   base::Time stop_thread_start_time = base::Time::Now();
   {
     base::ThreadRestrictions::ScopedAllowIO allow_io;
     sync_thread_.Stop();
   }
   base::TimeDelta stop_sync_thread_time = base::Time::Now() -
       stop_thread_start_time;
   UMA_HISTOGRAM_TIMES("Sync.Shutdown.StopSyncThreadTime",
                       stop_sync_thread_time);
 
   registrar_.reset();
-  frontend_ = NULL;
   chrome_sync_notification_bridge_.reset();
   core_ = NULL;  // Releases reference to core_.
 }
 
 void SyncBackendHost::ConfigureDataTypes(
     syncer::ConfigureReason reason,
     syncer::ModelTypeSet types_to_add,
     syncer::ModelTypeSet types_to_remove,
     NigoriState nigori_state,
     const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
@@ skipping 266 matching lines @@
 }
 
 
 void SyncBackendHost::Core::OnInitializationComplete(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend,
     bool success,
     const syncer::ModelTypeSet restored_types) {
   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(
       FROM_HERE,
       &SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop,
       js_backend, success, restored_types);
 
   if (success) {
     // Initialization is complete, so we can schedule recurring SaveChanges.
     sync_loop_->PostTask(FROM_HERE,
@@ skipping 168 matching lines @@
       options.restored_keystore_key_for_bootstrapping,
       CommandLine::ForCurrentProcess()->HasSwitch(
               switches::kSyncKeystoreEncryption),
       scoped_ptr<InternalComponentsFactory>(
           options.internal_components_factory),
       &encryptor_,
       options.unrecoverable_error_handler,
       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();
+    }
   }
 }
 
 void SyncBackendHost::Core::DoUpdateCredentials(
     const SyncCredentials& credentials) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   sync_manager_->UpdateCredentials(credentials);
 }
 
 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.
+  // TODO(akalin): Fix this behavior (see http://crbug.com/140354).
   if (sync_manager_.get()) {
-    sync_manager_->UpdateRegisteredInvalidationIds(this, ids);
+    sync_manager_->UpdateRegisteredInvalidationIds(kHandlerName, ids);
   }
 }
 
 void SyncBackendHost::Core::DoStartSyncing(
     const syncer::ModelSafeRoutingInfo& routing_info) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   sync_manager_->StartSyncingNormally(routing_info);
 }
 
 void SyncBackendHost::Core::DoSetEncryptionPassphrase(
@@ skipping 26 matching lines @@
     const base::Closure& closure) {
   if (sync_manager_.get()) {
     sync_manager_->StopSyncingForShutdown(closure);
   } else {
     sync_loop_->PostTask(FROM_HERE, closure);
   }
 }
 
 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;
 
   if (sync_disabled)
     DeleteSyncDataFolder();
 
   sync_loop_ = NULL;
 
   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,
     const syncer::ModelSafeRoutingInfo routing_info,
     const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
     const base::Closure& retry_callback) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   sync_manager_->ConfigureSyncer(
       reason,
       types_to_config,
@@ skipping 70 matching lines @@
 void SyncBackendHost::OnNigoriDownloadRetry() {
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
   if (!frontend_)
     return;
 
   frontend_->OnSyncConfigureRetry();
 }
 
 void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend, bool success) {
-  DCHECK_NE(NOT_ATTEMPTED, initialization_state_);
+  DCHECK_NE(initialization_state_, NOT_ATTEMPTED);
   if (!frontend_)
     return;
 
   // We've at least created the sync manager at this point, but if that is all
   // we've done we're just beginning the initialization process.
   if (initialization_state_ == CREATING_SYNC_MANAGER)
     initialization_state_ = NOT_INITIALIZED;
 
   DCHECK_EQ(MessageLoop::current(), frontend_loop_);
   if (!success) {
@@ skipping 229 matching lines @@
       FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoRefreshNigori,
                  core_.get(), sync_thread_done_callback));
 }
 
 #undef SDVLOG
 
 #undef SLOG
 
 }  // namespace browser_sync