[Sync] (Merge to 1229) Avoid unregistering object IDs on shutdown

Add RegisterHandler() and UnregisterHandler(), which should be called before and after calls to UpdateRegisteredIds().  Use UnregisterHandler() on
shutdown instead of UpdateRegisteredIds(_, ObjectIdSet()).

Make SyncNotifierHelper non-thread-safe.  Fix test breakages that this revealed.  Also add GetAllRegisteredIds() instead of making it the return value of UpdateRegisteredIds().

Propagate UpdateRegisteredIds()/RegisterHandler()/UnregisterHandler() all
the way up to ProfileSyncService.

Make FakeSyncManager be created on the sync thread.

Clean up SyncBackendHost startup/shutdown behavior a bit.

BUG=140325
TBR=tim@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150990

Review URL: https://chromiumcodereview.appspot.com/10824161

Review URL: https://chromiumcodereview.appspot.com/10825379

git-svn-id: svn://svn.chromium.org/chrome/branches/1229/src@151820 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 339 insertions, 97 deletions

diff --git a/chrome/browser/sync/glue/bridged_sync_notifier.cc b/chrome/browser/sync/glue/bridged_sync_notifier.cc
index 5e3f669..f783418 100644
--- a/chrome/browser/sync/glue/bridged_sync_notifier.cc
+++ b/chrome/browser/sync/glue/bridged_sync_notifier.cc
@@ -18,14 +18,28 @@ BridgedSyncNotifier::BridgedSyncNotifier(
 BridgedSyncNotifier::~BridgedSyncNotifier() {
 }
 
+void BridgedSyncNotifier::RegisterHandler(
+    syncer::SyncNotifierObserver* handler) {
+  if (delegate_.get())
+    delegate_->RegisterHandler(handler);
+  bridge_->RegisterHandler(handler);
+}
+
 void BridgedSyncNotifier::UpdateRegisteredIds(
     syncer::SyncNotifierObserver* handler,
     const syncer::ObjectIdSet& ids) {
   if (delegate_.get())
-   delegate_->UpdateRegisteredIds(handler, ids);
+    delegate_->UpdateRegisteredIds(handler, ids);
   bridge_->UpdateRegisteredIds(handler, ids);
 }
 
+void BridgedSyncNotifier::UnregisterHandler(
+    syncer::SyncNotifierObserver* handler) {
+  if (delegate_.get())
+    delegate_->UnregisterHandler(handler);
+  bridge_->UnregisterHandler(handler);
+}
+
 void BridgedSyncNotifier::SetUniqueId(const std::string& unique_id) {
   if (delegate_.get())
     delegate_->SetUniqueId(unique_id);
diff --git a/chrome/browser/sync/glue/bridged_sync_notifier.h b/chrome/browser/sync/glue/bridged_sync_notifier.h
index f48d0f9..616e16d 100644
--- a/chrome/browser/sync/glue/bridged_sync_notifier.h
+++ b/chrome/browser/sync/glue/bridged_sync_notifier.h
@@ -28,9 +28,13 @@ class BridgedSyncNotifier : public syncer::SyncNotifier {
   virtual ~BridgedSyncNotifier();
 
   // SyncNotifier implementation. Passes through all calls to the delegate.
-  // UpdateRegisteredIds calls will also be forwarded to the bridge.
+  // RegisterHandler, UnregisterHandler, and UpdateRegisteredIds calls will
+  // also be forwarded to the bridge.
+  virtual void RegisterHandler(syncer::SyncNotifierObserver* handler) OVERRIDE;
   virtual void UpdateRegisteredIds(syncer::SyncNotifierObserver* handler,
                                    const syncer::ObjectIdSet& ids) OVERRIDE;
+  virtual void UnregisterHandler(
+      syncer::SyncNotifierObserver* handler) OVERRIDE;
   virtual void SetUniqueId(const std::string& unique_id) OVERRIDE;
   virtual void SetStateDeprecated(const std::string& state) OVERRIDE;
   virtual void UpdateCredentials(
diff --git a/chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc b/chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc
index 7347eac..8863be4 100644
--- a/chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc
+++ b/chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc
@@ -35,8 +35,11 @@ class MockChromeSyncNotificationBridge : public ChromeSyncNotificationBridge {
       : ChromeSyncNotificationBridge(profile, sync_task_runner) {}
   virtual ~MockChromeSyncNotificationBridge() {}
 
-  MOCK_METHOD2(UpdateRegisteredIds, void(syncer::SyncNotifierObserver*,
-                                         const syncer::ObjectIdSet&));
+  MOCK_METHOD1(RegisterHandler, void(syncer::SyncNotifierObserver*));
+  MOCK_METHOD2(UpdateRegisteredIds,
+               void(syncer::SyncNotifierObserver*,
+                    const syncer::ObjectIdSet&));
+  MOCK_METHOD1(UnregisterHandler, void(syncer::SyncNotifierObserver*));
 };
 
 class MockSyncNotifier : public syncer::SyncNotifier {
@@ -44,8 +47,11 @@ class MockSyncNotifier : public syncer::SyncNotifier {
   MockSyncNotifier() {}
   virtual ~MockSyncNotifier() {}
 
+  MOCK_METHOD1(RegisterHandler, void(syncer::SyncNotifierObserver*));
   MOCK_METHOD2(UpdateRegisteredIds,
-               void(syncer::SyncNotifierObserver*, const syncer::ObjectIdSet&));
+               void(syncer::SyncNotifierObserver*,
+                    const syncer::ObjectIdSet&));
+  MOCK_METHOD1(UnregisterHandler, void(syncer::SyncNotifierObserver*));
   MOCK_METHOD1(SetUniqueId, void(const std::string&));
   MOCK_METHOD1(SetStateDeprecated, void(const std::string&));
   MOCK_METHOD2(UpdateCredentials, void(const std::string&, const std::string&));
@@ -53,8 +59,9 @@ class MockSyncNotifier : public syncer::SyncNotifier {
 };
 
 // All tests just verify that each call is passed through to the delegate, with
-// the exception of UpdateRegisteredIds, which also verifies the call is
-// forwarded to the bridge.
+// the exception of RegisterHandler, UnregisterHandler, and
+// UpdateRegisteredIds, which also verifies the call is forwarded to the
+// bridge.
 class BridgedSyncNotifierTest : public testing::Test {
  public:
   BridgedSyncNotifierTest()
@@ -73,13 +80,26 @@ class BridgedSyncNotifierTest : public testing::Test {
   BridgedSyncNotifier bridged_notifier_;
 };
 
-TEST_F(BridgedSyncNotifierTest, UpdateRegisteredIds) {
+TEST_F(BridgedSyncNotifierTest, RegisterHandler) {
   syncer::MockSyncNotifierObserver observer;
+  EXPECT_CALL(mock_bridge_, RegisterHandler(&observer));
+  EXPECT_CALL(*mock_delegate_, RegisterHandler(&observer));
+  bridged_notifier_.RegisterHandler(&observer);
+}
+
+TEST_F(BridgedSyncNotifierTest, UpdateRegisteredIds) {
   EXPECT_CALL(mock_bridge_, UpdateRegisteredIds(
-      &observer, syncer::ObjectIdSet()));
+      NULL, syncer::ObjectIdSet()));
   EXPECT_CALL(*mock_delegate_, UpdateRegisteredIds(
-      &observer, syncer::ObjectIdSet()));
-  bridged_notifier_.UpdateRegisteredIds(&observer, syncer::ObjectIdSet());
+      NULL, syncer::ObjectIdSet()));
+  bridged_notifier_.UpdateRegisteredIds(NULL, syncer::ObjectIdSet());
+}
+
+TEST_F(BridgedSyncNotifierTest, UnregisterHandler) {
+  syncer::MockSyncNotifierObserver observer;
+  EXPECT_CALL(mock_bridge_, UnregisterHandler(&observer));
+  EXPECT_CALL(*mock_delegate_, UnregisterHandler(&observer));
+  bridged_notifier_.UnregisterHandler(&observer);
 }
 
 TEST_F(BridgedSyncNotifierTest, SetUniqueId) {
diff --git a/chrome/browser/sync/glue/chrome_sync_notification_bridge.cc b/chrome/browser/sync/glue/chrome_sync_notification_bridge.cc
index 31db8e2..038d4f2 100644
--- a/chrome/browser/sync/glue/chrome_sync_notification_bridge.cc
+++ b/chrome/browser/sync/glue/chrome_sync_notification_bridge.cc
@@ -6,11 +6,13 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
-#include "sync/notifier/sync_notifier_helper.h"
 #include "sync/notifier/sync_notifier_observer.h"
+#include "sync/notifier/sync_notifier_registrar.h"
 
 using content::BrowserThread;
 
@@ -25,9 +27,14 @@ class ChromeSyncNotificationBridge::Core
 
   // All member functions below must be called on the sync task runner.
 
+  void InitializeOnSyncThread();
+  void CleanupOnSyncThread();
+
   void UpdateEnabledTypes(syncer::ModelTypeSet enabled_types);
+  void RegisterHandler(syncer::SyncNotifierObserver* handler);
   void UpdateRegisteredIds(syncer::SyncNotifierObserver* handler,
                            const syncer::ObjectIdSet& ids);
+  void UnregisterHandler(syncer::SyncNotifierObserver* handler);
 
   void EmitNotification(
       const syncer::ModelTypePayloadMap& payload_map,
@@ -43,7 +50,7 @@ class ChromeSyncNotificationBridge::Core
 
   // Used only on |sync_task_runner_|.
   syncer::ModelTypeSet enabled_types_;
-  syncer::SyncNotifierHelper helper_;
+  scoped_ptr<syncer::SyncNotifierRegistrar> notifier_registrar_;
 };
 
 ChromeSyncNotificationBridge::Core::Core(
@@ -56,6 +63,15 @@ ChromeSyncNotificationBridge::Core::Core(
 ChromeSyncNotificationBridge::Core::~Core() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
          sync_task_runner_->RunsTasksOnCurrentThread());
+  DCHECK(!notifier_registrar_.get());
+}
+
+void ChromeSyncNotificationBridge::Core::InitializeOnSyncThread() {
+  notifier_registrar_.reset(new syncer::SyncNotifierRegistrar());
+}
+
+void ChromeSyncNotificationBridge::Core::CleanupOnSyncThread() {
+  notifier_registrar_.reset();
 }
 
 void ChromeSyncNotificationBridge::Core::UpdateEnabledTypes(
@@ -64,11 +80,23 @@ void ChromeSyncNotificationBridge::Core::UpdateEnabledTypes(
   enabled_types_ = types;
 }
 
+void ChromeSyncNotificationBridge::Core::RegisterHandler(
+    syncer::SyncNotifierObserver* handler) {
+  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
+  notifier_registrar_->RegisterHandler(handler);
+}
+
 void ChromeSyncNotificationBridge::Core::UpdateRegisteredIds(
     syncer::SyncNotifierObserver* handler,
     const syncer::ObjectIdSet& ids) {
   DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-  helper_.UpdateRegisteredIds(handler, ids);
+  notifier_registrar_->UpdateRegisteredIds(handler, ids);
+}
+
+void ChromeSyncNotificationBridge::Core::UnregisterHandler(
+    syncer::SyncNotifierObserver* handler) {
+  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
+  notifier_registrar_->UnregisterHandler(handler);
 }
 
 void ChromeSyncNotificationBridge::Core::EmitNotification(
@@ -80,7 +108,7 @@ void ChromeSyncNotificationBridge::Core::EmitNotification(
       syncer::ModelTypePayloadMapFromEnumSet(enabled_types_, std::string()) :
       payload_map;
 
-  helper_.DispatchInvalidationsToHandlers(
+  notifier_registrar_->DispatchInvalidationsToHandlers(
       ModelTypePayloadMapToObjectIdPayloadMap(effective_payload_map),
       notification_source);
 }
@@ -96,16 +124,34 @@ ChromeSyncNotificationBridge::ChromeSyncNotificationBridge(
                  content::Source<Profile>(profile));
   registrar_.Add(this, chrome::NOTIFICATION_SYNC_REFRESH_REMOTE,
                  content::Source<Profile>(profile));
+
+  if (!sync_task_runner_->PostTask(
+          FROM_HERE, base::Bind(&Core::InitializeOnSyncThread, core_))) {
+    NOTREACHED();
+  }
 }
 
 ChromeSyncNotificationBridge::~ChromeSyncNotificationBridge() {}
 
+void ChromeSyncNotificationBridge::StopForShutdown() {
+  if (!sync_task_runner_->PostTask(
+          FROM_HERE, base::Bind(&Core::CleanupOnSyncThread, core_))) {
+    NOTREACHED();
+  }
+}
+
 void ChromeSyncNotificationBridge::UpdateEnabledTypes(
     syncer::ModelTypeSet types) {
   DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
   core_->UpdateEnabledTypes(types);
 }
 
+void ChromeSyncNotificationBridge::RegisterHandler(
+    syncer::SyncNotifierObserver* handler) {
+  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
+  core_->RegisterHandler(handler);
+}
+
 void ChromeSyncNotificationBridge::UpdateRegisteredIds(
     syncer::SyncNotifierObserver* handler,
     const syncer::ObjectIdSet& ids) {
@@ -113,6 +159,12 @@ void ChromeSyncNotificationBridge::UpdateRegisteredIds(
   core_->UpdateRegisteredIds(handler, ids);
 }
 
+void ChromeSyncNotificationBridge::UnregisterHandler(
+    syncer::SyncNotifierObserver* handler) {
+  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
+  core_->UnregisterHandler(handler);
+}
+
 void ChromeSyncNotificationBridge::Observe(
     int type,
     const content::NotificationSource& source,
@@ -132,10 +184,12 @@ void ChromeSyncNotificationBridge::Observe(
   content::Details<const syncer::ModelTypePayloadMap>
       payload_details(details);
   const syncer::ModelTypePayloadMap& payload_map = *(payload_details.ptr());
-  sync_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&Core::EmitNotification,
-                 core_, payload_map, notification_source));
+  if (!sync_task_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(&Core::EmitNotification,
+                     core_, payload_map, notification_source))) {
+    NOTREACHED();
+  }
 }
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/chrome_sync_notification_bridge.h b/chrome/browser/sync/glue/chrome_sync_notification_bridge.h
index a878ff7..0216df9 100644
--- a/chrome/browser/sync/glue/chrome_sync_notification_bridge.h
+++ b/chrome/browser/sync/glue/chrome_sync_notification_bridge.h
@@ -36,11 +36,18 @@ class ChromeSyncNotificationBridge : public content::NotificationObserver {
       const scoped_refptr<base::SequencedTaskRunner>& sync_task_runner);
   virtual ~ChromeSyncNotificationBridge();
 
+  // Must be called on the UI thread while the sync task runner is still
+  // around.  No other member functions on the sync thread may be called after
+  // this is called.
+  void StopForShutdown();
+
   // Must be called on the sync task runner.
   void UpdateEnabledTypes(syncer::ModelTypeSet enabled_types);
   // Marked virtual for tests.
+  virtual void RegisterHandler(syncer::SyncNotifierObserver* handler);
   virtual void UpdateRegisteredIds(syncer::SyncNotifierObserver* handler,
                                    const syncer::ObjectIdSet& ids);
+  virtual void UnregisterHandler(syncer::SyncNotifierObserver* handler);
 
   // NotificationObserver implementation. Called on UI thread.
   virtual void Observe(int type,
diff --git a/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc b/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
index 991b774..ed50b9e 100644
--- a/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
+++ b/chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/sync/glue/chrome_sync_notification_bridge.h"
 
+#include <cstddef>
+
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -53,6 +55,7 @@ class FakeSyncNotifierObserver : public syncer::SyncNotifierObserver {
         expected_payloads_(expected_payloads),
         expected_source_(expected_source) {
     DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
+    bridge_->RegisterHandler(this);
     const syncer::ObjectIdSet& ids =
         syncer::ObjectIdPayloadMapToSet(expected_payloads);
     bridge_->UpdateRegisteredIds(this, ids);
@@ -60,7 +63,7 @@ class FakeSyncNotifierObserver : public syncer::SyncNotifierObserver {
 
   virtual ~FakeSyncNotifierObserver() {
     DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-    bridge_->UpdateRegisteredIds(this, syncer::ObjectIdSet());
+    bridge_->UnregisterHandler(this);
   }
 
   // SyncNotifierObserver implementation.
@@ -120,6 +123,7 @@ class ChromeSyncNotificationBridgeTest : public testing::Test {
   }
 
   virtual void TearDown() OVERRIDE {
+    bridge_->StopForShutdown();
     sync_thread_.Stop();
     // Must be reset only after the sync thread is stopped.
     bridge_.reset();
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 9b7d19d..e3dc7cf 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -170,6 +170,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(
@@ -245,6 +246,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);
 };
 
@@ -541,11 +549,15 @@ void SyncBackendHost::StopSyncManagerForShutdown(
 
 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
@@ -570,17 +582,23 @@ void SyncBackendHost::StopSyncingForShutdown() {
     // 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);
     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
@@ -605,7 +623,6 @@ void SyncBackendHost::Shutdown(bool sync_disabled) {
                       stop_sync_thread_time);
 
   registrar_.reset();
-  frontend_ = NULL;
   chrome_sync_notification_bridge_.reset();
   core_ = NULL;  // Releases reference to core_.
 }
@@ -846,7 +863,8 @@ SyncBackendHost::Core::Core(const std::string& name,
       host_(backend),
       sync_loop_(NULL),
       registrar_(NULL),
-      chrome_sync_notification_bridge_(NULL) {
+      chrome_sync_notification_bridge_(NULL),
+      registered_as_invalidation_handler_(false) {
   DCHECK(backend.get());
 }
 
@@ -892,11 +910,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 +1099,24 @@ 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_->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();
+    }
   }
 }
 
@@ -1110,7 +1133,7 @@ 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);
   }
@@ -1159,14 +1182,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 +1195,20 @@ 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();
+    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,
@@ -1269,7 +1299,7 @@ void SyncBackendHost::OnNigoriDownloadRetry() {
 
 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;
 
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index bafe140..dd703b4 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -179,8 +179,8 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
   // Called on |frontend_loop| to update SyncCredentials.
   void UpdateCredentials(const syncer::SyncCredentials& credentials);
 
-  // Registers the underlying frontend for the given IDs to the
-  // underlying notifier.
+  // Registers the underlying frontend for the given IDs to the underlying
+  // notifier.  This lasts until StopSyncingForShutdown() is called.
   void UpdateRegisteredInvalidationIds(const syncer::ObjectIdSet& ids);
 
   // This starts the SyncerThread running a Syncer object to communicate with
@@ -211,9 +211,10 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
   bool SetDecryptionPassphrase(const std::string& passphrase)
       WARN_UNUSED_RESULT;
 
-  // Called on |frontend_loop_| to kick off shutdown procedure. After this,
-  // no further sync activity will occur with the sync server and no further
+  // Called on |frontend_loop_| to kick off shutdown procedure. After this, no
+  // further sync activity will occur with the sync server and no further
   // change applications will occur from changes already downloaded.
+  // Furthermore, no notifications will be sent to any invalidation handler.
   virtual void StopSyncingForShutdown();
 
   // Called on |frontend_loop_| to kick off shutdown.
diff --git a/chrome/browser/sync/glue/sync_backend_host_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_unittest.cc
index 2145da1..16770e7d 100644
--- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc
+++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc
@@ -89,24 +89,44 @@ class MockSyncFrontend : public SyncFrontend {
 
 class FakeSyncManagerFactory : public syncer::SyncManagerFactory {
  public:
-  FakeSyncManagerFactory() : manager_(NULL) {}
+  FakeSyncManagerFactory() : fake_manager_(NULL) {}
   virtual ~FakeSyncManagerFactory() {}
 
-  // Takes ownership of |manager|.
-  void SetSyncManager(FakeSyncManager* manager) {
-    DCHECK(!manager_.get());
-    manager_.reset(manager);
+  // SyncManagerFactory implementation.  Called on the sync thread.
+  virtual scoped_ptr<SyncManager> CreateSyncManager(
+      std::string name) OVERRIDE {
+    DCHECK(!fake_manager_);
+    fake_manager_ = new FakeSyncManager(initial_sync_ended_types_,
+                                        progress_marker_types_,
+                                        configure_fail_types_);
+    return scoped_ptr<SyncManager>(fake_manager_);
   }
 
-  // Passes ownership of |manager_|.
-  // SyncManagerFactory implementation.
-  virtual scoped_ptr<SyncManager> CreateSyncManager(std::string name) OVERRIDE {
-    DCHECK(manager_.get());
-    return manager_.Pass();
+  // Returns NULL until CreateSyncManager() is called on the sync
+  // thread.  Called on the main thread, but only after
+  // OnBackendInitialized() is called (which is strictly after
+  // CreateSyncManager is called on the sync thread).
+  FakeSyncManager* fake_manager() {
+    return fake_manager_;
+  }
+
+  void set_initial_sync_ended_types(syncer::ModelTypeSet types) {
+    initial_sync_ended_types_ = types;
+  }
+
+  void set_progress_marker_types(syncer::ModelTypeSet types) {
+    progress_marker_types_ = types;
+  }
+
+  void set_configure_fail_types(syncer::ModelTypeSet types) {
+    configure_fail_types_ = types;
   }
 
  private:
-  scoped_ptr<SyncManager> manager_;
+  syncer::ModelTypeSet initial_sync_ended_types_;
+  syncer::ModelTypeSet progress_marker_types_;
+  syncer::ModelTypeSet configure_fail_types_;
+  FakeSyncManager* fake_manager_;
 };
 
 class SyncBackendHostTest : public testing::Test {
@@ -131,8 +151,6 @@ class SyncBackendHostTest : public testing::Test {
         invalidator_storage_->AsWeakPtr()));
     credentials_.email = "user@example.com";
     credentials_.sync_token = "sync_token";
-    fake_manager_ = new FakeSyncManager();
-    fake_sync_manager_factory_.SetSyncManager(fake_manager_);
 
     // NOTE: We can't include Passwords or Typed URLs due to the Sync Backend
     // Registrar removing them if it can't find their model workers.
@@ -145,8 +163,10 @@ class SyncBackendHostTest : public testing::Test {
   }
 
   virtual void TearDown() OVERRIDE {
-    backend_->StopSyncingForShutdown();
-    backend_->Shutdown(false);
+    if (backend_.get()) {
+      backend_->StopSyncingForShutdown();
+      backend_->Shutdown(false);
+    }
     backend_.reset();
     sync_prefs_.reset();
     invalidator_storage_.reset();
@@ -168,12 +188,17 @@ class SyncBackendHostTest : public testing::Test {
                          GURL(""),
                          credentials_,
                          true,
-                         &fake_sync_manager_factory_,
+                         &fake_manager_factory_,
                          &handler_,
                          NULL);
     ui_loop_.PostDelayedTask(FROM_HERE,
         ui_loop_.QuitClosure(), TestTimeouts::action_timeout());
     ui_loop_.Run();
+    // |fake_manager_factory_|'s fake_manager() is set on the sync
+    // thread, but we can rely on the message loop barriers to
+    // guarantee that we see the updated value.
+    fake_manager_ = fake_manager_factory_.fake_manager();
+    DCHECK(fake_manager_);
   }
 
   // Synchronously configures the backend's datatypes.
@@ -206,15 +231,15 @@ class SyncBackendHostTest : public testing::Test {
   MessageLoop ui_loop_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread io_thread_;
-  MockSyncFrontend mock_frontend_;
+  StrictMock<MockSyncFrontend> mock_frontend_;
   syncer::SyncCredentials credentials_;
   syncer::TestUnrecoverableErrorHandler handler_;
   scoped_ptr<TestingProfile> profile_;
   scoped_ptr<SyncPrefs> sync_prefs_;
   scoped_ptr<InvalidatorStorage> invalidator_storage_;
   scoped_ptr<SyncBackendHost> backend_;
-  FakeSyncManagerFactory fake_sync_manager_factory_;
   FakeSyncManager* fake_manager_;
+  FakeSyncManagerFactory fake_manager_factory_;
   syncer::ModelTypeSet enabled_types_;
 };
 
@@ -257,8 +282,8 @@ TEST_F(SyncBackendHostTest, FirstTimeSync) {
 TEST_F(SyncBackendHostTest, Restart) {
   sync_prefs_->SetSyncSetupCompleted();
   syncer::ModelTypeSet all_but_nigori = enabled_types_;
-  fake_manager_->set_progress_marker_types(enabled_types_);
-  fake_manager_->set_initial_sync_ended_types(enabled_types_);
+  fake_manager_factory_.set_progress_marker_types(enabled_types_);
+  fake_manager_factory_.set_initial_sync_ended_types(enabled_types_);
   InitializeBackend();
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Empty());
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
@@ -289,8 +314,8 @@ TEST_F(SyncBackendHostTest, PartialTypes) {
   syncer::ModelTypeSet partial_types(syncer::NIGORI, syncer::BOOKMARKS);
   syncer::ModelTypeSet full_types =
       Difference(enabled_types_, partial_types);
-  fake_manager_->set_progress_marker_types(enabled_types_);
-  fake_manager_->set_initial_sync_ended_types(full_types);
+  fake_manager_factory_.set_progress_marker_types(enabled_types_);
+  fake_manager_factory_.set_initial_sync_ended_types(full_types);
 
   // Bringing up the backend should purge all partial types, then proceed to
   // download the Nigori.
@@ -476,8 +501,8 @@ TEST_F(SyncBackendHostTest, NewlySupportedTypes) {
   // Set sync manager behavior before passing it down. All types have progress
   // markers and initial sync ended except the new types.
   syncer::ModelTypeSet old_types = enabled_types_;
-  fake_manager_->set_progress_marker_types(old_types);
-  fake_manager_->set_initial_sync_ended_types(old_types);
+  fake_manager_factory_.set_progress_marker_types(old_types);
+  fake_manager_factory_.set_initial_sync_ended_types(old_types);
   syncer::ModelTypeSet new_types(syncer::APP_SETTINGS,
                                  syncer::EXTENSION_SETTINGS);
   enabled_types_.PutAll(new_types);
@@ -517,8 +542,8 @@ TEST_F(SyncBackendHostTest, NewlySupportedTypesWithPartialTypes) {
   syncer::ModelTypeSet partial_types(syncer::NIGORI, syncer::BOOKMARKS);
   syncer::ModelTypeSet full_types =
       Difference(enabled_types_, partial_types);
-  fake_manager_->set_progress_marker_types(old_types);
-  fake_manager_->set_initial_sync_ended_types(full_types);
+  fake_manager_factory_.set_progress_marker_types(old_types);
+  fake_manager_factory_.set_initial_sync_ended_types(full_types);
   syncer::ModelTypeSet new_types(syncer::APP_SETTINGS,
                                  syncer::EXTENSION_SETTINGS);
   enabled_types_.PutAll(new_types);
@@ -609,6 +634,35 @@ TEST_F(SyncBackendHostTest, DisableNotifications) {
   ui_loop_.Run();
 }
 
+// Call StopSyncingForShutdown() on the backend and fire some notifications
+// before calling Shutdown().  Then start up and shut down the backend again.
+// Those notifications shouldn't propagate to the frontend.
+TEST_F(SyncBackendHostTest, NotificationsAfterStopSyncingForShutdown) {
+  InitializeBackend();
+
+  syncer::ObjectIdSet ids;
+  ids.insert(invalidation::ObjectId(5, "id5"));
+  backend_->UpdateRegisteredInvalidationIds(ids);
+
+  backend_->StopSyncingForShutdown();
+
+  // Should not trigger anything.
+  fake_manager_->DisableNotifications(syncer::TRANSIENT_NOTIFICATION_ERROR);
+  fake_manager_->EnableNotifications();
+  const syncer::ObjectIdPayloadMap& id_payloads =
+      syncer::ObjectIdSetToPayloadMap(ids, "payload");
+  fake_manager_->Invalidate(id_payloads, syncer::REMOTE_NOTIFICATION);
+
+  // Make sure the above calls take effect before we continue.
+  fake_manager_->WaitForSyncThread();
+
+  backend_->Shutdown(false);
+  backend_.reset();
+
+  TearDown();
+  SetUp();
+}
+
 }  // namespace
 
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index bc72dcc..5e83b20 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -428,7 +428,8 @@ void ProfileSyncService::StartUp() {
   // TODO(akalin): Fix this horribly non-intuitive behavior (see
   // http://crbug.com/140354).
   if (backend_.get()) {
-    backend_->UpdateRegisteredInvalidationIds(all_registered_ids_);
+    backend_->UpdateRegisteredInvalidationIds(
+        notifier_registrar_.GetAllRegisteredIds());
   }
 
   if (!sync_global_error_.get()) {
@@ -441,17 +442,29 @@ void ProfileSyncService::StartUp() {
   }
 }
 
+void ProfileSyncService::RegisterInvalidationHandler(
+    syncer::SyncNotifierObserver* handler) {
+  notifier_registrar_.RegisterHandler(handler);
+}
+
 void ProfileSyncService::UpdateRegisteredInvalidationIds(
     syncer::SyncNotifierObserver* handler,
     const syncer::ObjectIdSet& ids) {
-  all_registered_ids_ = notifier_helper_.UpdateRegisteredIds(handler, ids);
+  notifier_registrar_.UpdateRegisteredIds(handler, ids);
+
   // If |backend_| is NULL, its registered IDs will be updated when
   // it's created and initialized.
   if (backend_.get()) {
-    backend_->UpdateRegisteredInvalidationIds(all_registered_ids_);
+    backend_->UpdateRegisteredInvalidationIds(
+        notifier_registrar_.GetAllRegisteredIds());
   }
 }
 
+void ProfileSyncService::UnregisterInvalidationHandler(
+    syncer::SyncNotifierObserver* handler) {
+  notifier_registrar_.UnregisterHandler(handler);
+}
+
 void ProfileSyncService::Shutdown() {
   ShutdownImpl(false);
 }
@@ -464,7 +477,6 @@ void ProfileSyncService::ShutdownImpl(bool sync_disabled) {
   base::Time shutdown_start_time = base::Time::Now();
   if (backend_.get()) {
     backend_->StopSyncingForShutdown();
-    backend_->UpdateRegisteredInvalidationIds(syncer::ObjectIdSet());
   }
 
   // Stop all data type controllers, if needed.  Note that until Stop
@@ -659,18 +671,18 @@ void ProfileSyncService::DisableBrokenDatatype(
 }
 
 void ProfileSyncService::OnNotificationsEnabled() {
-  notifier_helper_.EmitOnNotificationsEnabled();
+  notifier_registrar_.EmitOnNotificationsEnabled();
 }
 
 void ProfileSyncService::OnNotificationsDisabled(
     syncer::NotificationsDisabledReason reason) {
-  notifier_helper_.EmitOnNotificationsDisabled(reason);
+  notifier_registrar_.EmitOnNotificationsDisabled(reason);
 }
 
 void ProfileSyncService::OnIncomingNotification(
     const syncer::ObjectIdPayloadMap& id_payloads,
     syncer::IncomingNotificationSource source) {
-  notifier_helper_.DispatchInvalidationsToHandlers(id_payloads, source);
+  notifier_registrar_.DispatchInvalidationsToHandlers(id_payloads, source);
 }
 
 void ProfileSyncService::OnBackendInitialized(
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index 3e3e14c..6e8ea6e 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -38,7 +38,7 @@
 #include "sync/internal_api/public/util/experiments.h"
 #include "sync/internal_api/public/util/unrecoverable_error_handler.h"
 #include "sync/js/sync_js_controller.h"
-#include "sync/notifier/sync_notifier_helper.h"
+#include "sync/notifier/sync_notifier_registrar.h"
 
 class Profile;
 class ProfileSyncComponentsFactory;
@@ -552,15 +552,56 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   // been cleared yet. Virtual for testing purposes.
   virtual bool waiting_for_auth() const;
 
-  // Updates the set of ObjectIds associated with a given |handler|.
-  // Passing an empty ObjectIdSet will unregister |handler|.
-  // There should be at most one handler registered per object id.
+  // Invalidation clients should follow the pattern below:
   //
-  // The handler -> registered ids map is persisted across restarts of
-  // sync.
+  // When starting the client:
+  //
+  //   pss->RegisterInvalidationHandler(client_handler);
+  //
+  // When the set of IDs to register changes for the client during its lifetime
+  // (i.e., between calls to RegisterInvalidationHandler(client_handler) and
+  // UnregisterInvalidationHandler(client_handler):
+  //
+  //   pss->UpdateRegisteredInvalidationIds(client_handler, client_ids);
+  //
+  // When shutting down the client for browser shutdown:
+  //
+  //   pss->UnregisterInvalidationHandler(client_handler);
+  //
+  // Note that there's no call to UpdateRegisteredIds() -- this is because the
+  // invalidation API persists registrations across browser restarts.
+  //
+  // When permanently shutting down the client, e.g. when disabling the related
+  // feature:
+  //
+  //   pss->UpdateRegisteredInvalidationIds(client_handler, ObjectIdSet());
+  //   pss->UnregisterInvalidationHandler(client_handler);
+
+  // NOTE(akalin): Invalidations that come in during browser shutdown may get
+  // dropped.  This won't matter once we have an Acknowledge API, though: see
+  // http://crbug.com/78462 and http://crbug.com/124149.
+
+  // Starts sending notifications to |handler|.  |handler| must not be NULL,
+  // and it must already be registered.
+  //
+  // Handler registrations are persisted across restarts of sync.
+  void RegisterInvalidationHandler(syncer::SyncNotifierObserver* handler);
+
+  // Updates the set of ObjectIds associated with |handler|.  |handler| must
+  // not be NULL, and must already be registered.  An ID must be registered for
+  // at most one handler.
+  //
+  // Registered IDs are persisted across restarts of sync.
   void UpdateRegisteredInvalidationIds(syncer::SyncNotifierObserver* handler,
                                        const syncer::ObjectIdSet& ids);
 
+  // Stops sending notifications to |handler|.  |handler| must not be NULL, and
+  // it must already be registered.  Note that this doesn't unregister the IDs
+  // associated with |handler|.
+  //
+  // Handler registrations are persisted across restarts of sync.
+  void UnregisterInvalidationHandler(syncer::SyncNotifierObserver* handler);
+
   // ProfileKeyedService implementation.
   virtual void Shutdown() OVERRIDE;
 
@@ -835,11 +876,8 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   // Factory the backend will use to build the SyncManager.
   syncer::SyncManagerFactory sync_manager_factory_;
 
-  // The set of all registered IDs.
-  syncer::ObjectIdSet all_registered_ids_;
-
   // Dispatches invalidations to handlers.
-  syncer::SyncNotifierHelper notifier_helper_;
+  syncer::SyncNotifierRegistrar notifier_registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileSyncService);
 };
diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc
index 78ac1f4..1a2fe90 100644
--- a/chrome/browser/sync/profile_sync_service_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_unittest.cc
@@ -389,6 +389,7 @@ TEST_F(ProfileSyncServiceTest, UpdateRegisteredInvalidationIds) {
   EXPECT_CALL(observer, OnNotificationsDisabled(
       syncer::TRANSIENT_NOTIFICATION_ERROR));
 
+  service_->RegisterInvalidationHandler(&observer);
   service_->UpdateRegisteredInvalidationIds(&observer, ids);
 
   SyncBackendHostForProfileSyncTest* const backend =
@@ -400,7 +401,7 @@ TEST_F(ProfileSyncServiceTest, UpdateRegisteredInvalidationIds) {
 
   Mock::VerifyAndClearExpectations(&observer);
 
-  service_->UpdateRegisteredInvalidationIds(&observer, syncer::ObjectIdSet());
+  service_->UnregisterInvalidationHandler(&observer);
 
   backend->EmitOnNotificationsEnabled();
   backend->EmitOnIncomingNotification(payloads, syncer::REMOTE_NOTIFICATION);
@@ -422,9 +423,12 @@ TEST_F(ProfileSyncServiceTest, UpdateRegisteredInvalidationIdsPersistence) {
   EXPECT_CALL(observer, OnNotificationsEnabled());
   EXPECT_CALL(observer, OnIncomingNotification(
       payloads, syncer::REMOTE_NOTIFICATION));
+  // This may get called more than once, as a real notifier is
+  // created.
   EXPECT_CALL(observer, OnNotificationsDisabled(
-      syncer::TRANSIENT_NOTIFICATION_ERROR));
+      syncer::TRANSIENT_NOTIFICATION_ERROR)).Times(AtLeast(1));
 
+  service_->RegisterInvalidationHandler(&observer);
   service_->UpdateRegisteredInvalidationIds(&observer, ids);
 
   service_->StopAndSuppress();