[Sync] Avoid unregistering object IDs on shutdown

chrome/browser/sync/glue/sync_backend_host_unittest.cc

Index: chrome/browser/sync/glue/sync_backend_host_unittest.cc
diff --git a/chrome/browser/sync/glue/sync_backend_host_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_unittest.cc
index 2145da1fc64ed736fe8d323682d95700cc5f3d89..16770e7d24457f71e23406ce139d85cb65f6a66d 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