[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..25c3b682c416ebd8878dea0f47993d6f871d1708 100644
--- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc
+++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc
@@ -89,24 +89,42 @@ 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.
+  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.
+  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 +149,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 +161,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 +186,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 +229,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 +280,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 +312,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 +499,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 +540,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 +632,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();

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

Hmm.. wait, so how does the client know that it's out of date?

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

Do you mean in the interval that sync is shut down?  If sync is shutting down,
that means the browser is either shutting down or the user is stopping sync.  In
the former case, we may drop invalidations; this has always been true, since we
ack immediately.  Once we add the ack API, then it'll invalidate again when sync
is turned on (since the client won't ack).  In the latter case, the user is
stopping sync, which nukes invalidation state, so when he/she re-enables it,
it'll act like a new client (and send an initial invalidation).  I think even
without the initial invalidation, the ack api will take care of this case, too.

[tim (not reviewing), 2012/08/07 17:22:12]

Oh right.  This would be useful to have in a comment somewhere if it's not
already there (perhaps near the PSS invalidation related methods).

[akalin, 2012/08/07 18:11:30]

Done.

+}
+
 }  // namespace
 
 }  // namespace browser_sync