[Sync] Avoid unregistering object IDs on shutdown

chrome/browser/sync/glue/sync_backend_host_unittest.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 "chrome/browser/sync/glue/sync_backend_host.h"
 
 #include <cstddef>
 
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 71 matching lines @@
   MOCK_METHOD1(OnMigrationNeededForTypes, void(syncer::ModelTypeSet));
   MOCK_METHOD1(OnExperimentsChanged,
       void(const syncer::Experiments&));
   MOCK_METHOD1(OnActionableError,
       void(const syncer::SyncProtocolError& sync_error));
   MOCK_METHOD0(OnSyncConfigureRetry, void());
 };
 
 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 {
  protected:
   SyncBackendHostTest()
       : ui_thread_(BrowserThread::UI, &ui_loop_),
         io_thread_(BrowserThread::IO),
         fake_manager_(NULL) {}
 
   virtual ~SyncBackendHostTest() {}
 
   virtual void SetUp() OVERRIDE {
     io_thread_.StartIOThread();
     profile_.reset(new TestingProfile());
     profile_->CreateRequestContext();
     sync_prefs_.reset(new SyncPrefs(profile_->GetPrefs()));
     invalidator_storage_.reset(new InvalidatorStorage(profile_->GetPrefs()));
     backend_.reset(new SyncBackendHost(
         profile_->GetDebugName(),
         profile_.get(),
         sync_prefs_->AsWeakPtr(),
         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.
     enabled_types_.Put(syncer::BOOKMARKS);
     enabled_types_.Put(syncer::NIGORI);
     enabled_types_.Put(syncer::PREFERENCES);
     enabled_types_.Put(syncer::SESSIONS);
     enabled_types_.Put(syncer::SEARCH_ENGINES);
     enabled_types_.Put(syncer::AUTOFILL);
   }
 
   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();
     profile_.reset();
     // Pump messages posted by the sync thread (which may end up
     // posting on the IO thread).
     ui_loop_.RunAllPending();
     io_thread_.Stop();
     // Pump any messages posted by the IO thread.
     ui_loop_.RunAllPending();
   }
 
   // Synchronously initializes the backend.
   void InitializeBackend() {
     EXPECT_CALL(mock_frontend_, OnBackendInitialized(_, true)).
         WillOnce(InvokeWithoutArgs(QuitMessageLoop));
     backend_->Initialize(&mock_frontend_,
                          syncer::WeakHandle<syncer::JsEventHandler>(),
                          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

[msw, 2012/08/07 20:09:30]

Maybe make a similar comment with fake_manager_'s declaration?
It's lifetime management isn't obvious from the code there.

[akalin, 2012/08/07 23:38:05]

Done.

+    // 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.
   void ConfigureDataTypes(syncer::ModelTypeSet types_to_add,
                           syncer::ModelTypeSet types_to_remove,
                           BackendDataTypeConfigurer::NigoriState nigori_state) {
     backend_->ConfigureDataTypes(
         syncer::CONFIGURE_REASON_RECONFIGURATION,
         types_to_add,
         types_to_remove,
@@ skipping 12 matching lines @@
     MessageLoop::current()->Quit();
   }
 
   void OnDownloadRetry() {
     NOTIMPLEMENTED();
   }
 
   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_;
 };
 
 // Test basic initialization with no initial types (first time initialization).
 // Only the nigori should be configured.
 TEST_F(SyncBackendHostTest, InitShutdown) {
   InitializeBackend();
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       syncer::ModelTypeSet(syncer::NIGORI)));
   EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(
@@ skipping 22 matching lines @@
   EXPECT_TRUE(fake_manager_->GetAndResetEnabledTypes().Equals(enabled_types_));
   EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
       enabled_types_).Empty());
 }
 
 // Test the restart after setting up sync scenario. No enabled types should be
 // downloaded or cleaned.
 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(),
                            enabled_types_).Empty());
   EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(enabled_types_));
   EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
       enabled_types_).Empty());
 
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
@@ skipping 10 matching lines @@
 
 // Test a sync restart scenario where some types had never finished configuring.
 // The partial types should be purged, then reconfigured properly.
 TEST_F(SyncBackendHostTest, PartialTypes) {
   sync_prefs_->SetSyncSetupCompleted();
   // Set sync manager behavior before passing it down. All types have progress
   // markers, but nigori and bookmarks are missing initial sync ended.
   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.
   InitializeBackend();
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       syncer::ModelTypeSet(syncer::NIGORI)));
   EXPECT_TRUE(fake_manager_->GetAndResetCleanedTypes().HasAll(partial_types));
   EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(
       Union(full_types, syncer::ModelTypeSet(syncer::NIGORI))));
   EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
@@ skipping 165 matching lines @@
       old_types).Equals(disabled_types));
 }
 
 // Test restarting the browser to newly supported datatypes. The new datatypes
 // should be downloaded on the configuration after backend initialization.
 TEST_F(SyncBackendHostTest, NewlySupportedTypes) {
   sync_prefs_->SetSyncSetupCompleted();
   // 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);
 
   // Does nothing.
   InitializeBackend();
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Empty());
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
                            old_types).Empty());
   EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(old_types));
@@ skipping 19 matching lines @@
 // types as well. Both partial and newly supported types should be downloaded
 // the configuration.
 TEST_F(SyncBackendHostTest, NewlySupportedTypesWithPartialTypes) {
   sync_prefs_->SetSyncSetupCompleted();
   // 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_;
   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);
 
   // Purge the partial types.  The nigori will be among the purged types, but
   // the syncer will re-download it by the time the initialization is complete.
   InitializeBackend();
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       syncer::ModelTypeSet(syncer::NIGORI)));
   EXPECT_TRUE(fake_manager_->GetAndResetCleanedTypes().HasAll(partial_types));
@@ skipping 70 matching lines @@
 
   syncer::ObjectIdSet ids;
   ids.insert(invalidation::ObjectId(4, "id4"));
   backend_->UpdateRegisteredInvalidationIds(ids);
   fake_manager_->DisableNotifications(syncer::TRANSIENT_NOTIFICATION_ERROR);
   ui_loop_.PostDelayedTask(
       FROM_HERE, ui_loop_.QuitClosure(), TestTimeouts::action_timeout());
   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.

[msw, 2012/08/07 20:09:30]

Can you comment on how this is being ensured?

Is it the lack of an EXPECT_CALL(mock_frontend_,
OnIncomingNotification(id_payloads,
syncer::REMOTE_NOTIFICATION)).WillOnce(InvokeWithoutArgs(QuitMessageLoop)); ???

Can you call that with .Times(0) to make an explicit assertion?

[akalin, 2012/08/07 23:38:05]

mock_frontend_ is declared as a StrictMock.  Doesn't that suffice?  I could add
the .Times(0), but then logically I'd have to add that for every mocked
function. :/

[msw, 2012/08/08 00:55:23]

Ahh, I was unfamiliar with StrictMock and that behavior.
This is fine, if anything perhaps note that in the comment.

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

[msw, 2012/08/07 20:09:30]

Can you use MessageLoop::RunAllPending or similar instead?

[akalin, 2012/08/07 23:38:05]

No, because nothing is actually posted to the UI thread (since no messages are
triggered).

[msw, 2012/08/08 00:55:23]

I meant to RunAllPending on the sync thread, but as you noted, that's
insufficient here. I'll presume the other reviewers understand and vouch for
this :)

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