[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..d433e47d001d86a515d8bc4cee2bd22857807c29 100644
--- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc
+++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc
@@ -89,24 +89,38 @@ class MockSyncFrontend : public SyncFrontend {
 
 class FakeSyncManagerFactory : public syncer::SyncManagerFactory {
  public:
-  FakeSyncManagerFactory() : manager_(NULL) {}
+  // Fills in |*manager_ptr| when the sync manager is created.
+  explicit FakeSyncManagerFactory(FakeSyncManager** manager_ptr)
+      : manager_ptr_(manager_ptr) {}

[tim (not reviewing), 2012/08/06 05:55:32]

Hm. manager_ptr_ptr?

[akalin, 2012/08/07 07:25:19]

moot, just exposed an accessor

   virtual ~FakeSyncManagerFactory() {}
 
-  // Takes ownership of |manager|.
-  void SetSyncManager(FakeSyncManager* manager) {
-    DCHECK(!manager_.get());
-    manager_.reset(manager);
-  }
-
   // Passes ownership of |manager_|.
   // SyncManagerFactory implementation.
-  virtual scoped_ptr<SyncManager> CreateSyncManager(std::string name) OVERRIDE {
-    DCHECK(manager_.get());
-    return manager_.Pass();
+  virtual scoped_ptr<SyncManager> CreateSyncManager(
+      std::string name) OVERRIDE {

[msw, 2012/08/03 23:30:46]

nit: fits on line above

[akalin, 2012/08/07 07:25:19]

not quite -- since it's exactly 80 chars, it overflows on emacs :/

+    *manager_ptr_ = new FakeSyncManager(initial_sync_ended_types_,
+                                        progress_marker_types_,
+                                        configure_fail_types_);
+    return scoped_ptr<SyncManager>(*manager_ptr_);
+  }
+
+  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** manager_ptr_;

[msw, 2012/08/03 23:30:46]

Why is this a double pointer? Also, I think our style guide discourages "_ptr".

[akalin, 2012/08/07 07:25:19]

moot

 };
 
 class SyncBackendHostTest : public testing::Test {
@@ -114,7 +128,8 @@ class SyncBackendHostTest : public testing::Test {
   SyncBackendHostTest()
       : ui_thread_(BrowserThread::UI, &ui_loop_),
         io_thread_(BrowserThread::IO),
-        fake_manager_(NULL) {}
+        fake_manager_(NULL),
+        fake_manager_factory_(&fake_manager_) {}
 
   virtual ~SyncBackendHostTest() {}
 
@@ -131,8 +146,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 +158,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 +183,15 @@ 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();
+    // Set on the sync thread, but we can rely on the message loop
+    // barriers to guarantee that we see the updated value.
+    DCHECK(fake_manager_);
   }
 
   // Synchronously configures the backend's datatypes.
@@ -206,15 +224,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 +275,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 +307,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 +494,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 +535,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 +627,31 @@ TEST_F(SyncBackendHostTest, DisableNotifications) {
   ui_loop_.Run();
 }
 
+// Call StopSyncingForShutdown() on the backend and fire some

[msw, 2012/08/03 23:30:46]

nit: line breaks :)

[akalin, 2012/08/07 07:25:19]

Done.

+// notifications before calling Shutdown().  Those notifications
+// shouldn't propagate to the frontend.

[tim (not reviewing), 2012/08/06 05:55:32]

Ever?  On restart? Do we have tests that cover that?

[akalin, 2012/08/07 07:25:19]

yeah.  added teardown and startup.

+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);
+
+  fake_manager_->WaitForSyncThread();

[tim (not reviewing), 2012/08/06 05:55:32]

Is this to flush all the tasks?  Does it need to be separate from stopping it?

[akalin, 2012/08/07 07:25:19]

yes.  this is so that the test would fail deterministically if it does fail.

+
+  backend_->Shutdown(false);
+  backend_.reset();
+}
+
 }  // namespace
 
 }  // namespace browser_sync