BackgroundSyncManager tracks client registrations

content/browser/background_sync/background_sync_manager_unittest.cc

Index: content/browser/background_sync/background_sync_manager_unittest.cc
diff --git a/content/browser/background_sync/background_sync_manager_unittest.cc b/content/browser/background_sync/background_sync_manager_unittest.cc
index e80881c12ee41f3cca8082c5e93781397412db4b..bb12d74ef4ae08ab692cb2e14505d0410ce14562 100644
--- a/content/browser/background_sync/background_sync_manager_unittest.cc
+++ b/content/browser/background_sync/background_sync_manager_unittest.cc
@@ -13,6 +13,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "content/browser/background_sync/background_sync_status.h"
+#include "content/browser/background_sync/client_background_sync_registration.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/service_worker/embedded_worker_test_helper.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
@@ -127,6 +128,7 @@ class TestBackgroundSyncManager : public BackgroundSyncManager {
   }
 
   void Continue() {
+    ASSERT_FALSE(continuation_.is_null());
     continuation_.Run();
     continuation_.Reset();
   }
@@ -184,15 +186,11 @@ class TestBackgroundSyncManager : public BackgroundSyncManager {
   }
 
   void FireOneShotSync(
-      const BackgroundSyncRegistration& registration,
+      BackgroundSyncManager::RefCountedRegistration* registration,
       const scoped_refptr<ServiceWorkerVersion>& active_version,
       const ServiceWorkerVersion::StatusCallback& callback) override {
-    if (one_shot_callback_.is_null()) {
-      BackgroundSyncManager::FireOneShotSync(registration, active_version,
-                                             callback);
-    } else {
-      one_shot_callback_.Run(active_version, callback);
-    }
+    ASSERT_FALSE(one_shot_callback_.is_null());
+    one_shot_callback_.Run(active_version, callback);
   }
 
  private:
@@ -305,19 +303,20 @@ class BackgroundSyncManagerTest : public testing::Test {
   void StatusAndRegistrationCallback(
       bool* was_called,
       BackgroundSyncStatus status,
-      const BackgroundSyncRegistration& registration) {
+      scoped_ptr<ClientBackgroundSyncRegistration> registration) {
     *was_called = true;
     callback_status_ = status;
-    callback_registration_ = registration;
+    callback_registration_ = registration.Pass();
   }
 
   void StatusAndRegistrationsCallback(
       bool* was_called,
       BackgroundSyncStatus status,
-      const std::vector<BackgroundSyncRegistration>& registrations) {
+      scoped_ptr<ScopedVector<ClientBackgroundSyncRegistration>>
+          registrations) {
     *was_called = true;
     callback_status_ = status;
-    callback_registrations_ = registrations;
+    callback_registrations_ = registrations.Pass();
   }
 
   void StatusCallback(bool* was_called, BackgroundSyncStatus status) {
@@ -327,6 +326,8 @@ class BackgroundSyncManagerTest : public testing::Test {
 
  protected:
   void CreateBackgroundSyncManager() {
+    ClearRegistrations();
+
     test_background_sync_manager_ =
         new TestBackgroundSyncManager(helper_->context_wrapper());
     background_sync_manager_.reset(test_background_sync_manager_);
@@ -343,6 +344,12 @@ class BackgroundSyncManagerTest : public testing::Test {
     SetNetwork(net::NetworkChangeNotifier::CONNECTION_NONE);
   }
 
+  // Clear the registrations so that the BackgroundSyncManager can release them.
+  void ClearRegistrations() {
+    callback_registration_.reset();
+    callback_registrations_.reset();
+  }
+
   void SetupBackgroundSyncManager() {
     CreateBackgroundSyncManager();
     InitBackgroundSyncManager();
@@ -361,6 +368,7 @@ class BackgroundSyncManagerTest : public testing::Test {
   }
 
   void DeleteBackgroundSyncManager() {
+    ClearRegistrations();
     background_sync_manager_.reset();
     test_background_sync_manager_ = nullptr;
   }
@@ -382,18 +390,17 @@ class BackgroundSyncManagerTest : public testing::Test {
     return callback_status_ == BACKGROUND_SYNC_STATUS_OK;
   }
 
-  bool Unregister(const BackgroundSyncRegistration& sync_registration) {
+  bool Unregister(ClientBackgroundSyncRegistration* sync_registration) {
     return UnregisterWithServiceWorkerId(sw_registration_id_1_,
                                          sync_registration);
   }
 
   bool UnregisterWithServiceWorkerId(
       int64 sw_registration_id,
-      const BackgroundSyncRegistration& sync_registration) {
+      ClientBackgroundSyncRegistration* sync_registration) {
     bool was_called = false;
-    background_sync_manager_->Unregister(
-        sw_registration_id, sync_registration.options()->tag,
-        sync_registration.options()->periodicity, sync_registration.id(),
+    sync_registration->Unregister(
+        sw_registration_id,
         base::Bind(&BackgroundSyncManagerTest::StatusCallback,
                    base::Unretained(this), &was_called));
     base::RunLoop().RunUntilIdle();
@@ -419,7 +426,7 @@ class BackgroundSyncManagerTest : public testing::Test {
 
     if (callback_status_ == BACKGROUND_SYNC_STATUS_OK) {
       EXPECT_STREQ(sync_options.tag.c_str(),
-                   callback_registration_.options()->tag.c_str());
+                   callback_registration_->options()->tag.c_str());
     }
 
     return callback_status_ == BACKGROUND_SYNC_STATUS_OK;
@@ -514,8 +521,9 @@ class BackgroundSyncManagerTest : public testing::Test {
 
   // Callback values.
   BackgroundSyncStatus callback_status_;
-  BackgroundSyncRegistration callback_registration_;
-  std::vector<BackgroundSyncRegistration> callback_registrations_;
+  scoped_ptr<ClientBackgroundSyncRegistration> callback_registration_;
+  scoped_ptr<ScopedVector<ClientBackgroundSyncRegistration>>
+      callback_registrations_;
   ServiceWorkerStatusCode callback_sw_status_code_;
   int sync_events_called_;
   ServiceWorkerVersion::StatusCallback sync_fired_callback_;
@@ -528,8 +536,8 @@ TEST_F(BackgroundSyncManagerTest, Register) {
 TEST_F(BackgroundSyncManagerTest, RegistractionIntact) {
   EXPECT_TRUE(Register(sync_options_1_));
   EXPECT_STREQ(sync_options_1_.tag.c_str(),
-               callback_registration_.options()->tag.c_str());
-  EXPECT_TRUE(callback_registration_.IsValid());
+               callback_registration_->options()->tag.c_str());
+  EXPECT_TRUE(callback_registration_->IsValid());
 }
 
 TEST_F(BackgroundSyncManagerTest, RegisterWithoutLiveSWRegistration) {
@@ -544,22 +552,15 @@ TEST_F(BackgroundSyncManagerTest, RegisterWithoutActiveSWRegistration) {
   EXPECT_EQ(BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER, callback_status_);
 }
 
-TEST_F(BackgroundSyncManagerTest, RegisterExistingKeepsId) {

[iclelland, 2015/08/14 14:44:18]

Is this test no longer valid? Do we not care that the registration doesn't
change, or does that not happen anymore?

[jkarlin, 2015/08/17 17:14:49]

No longer valid. It used to be the same regitration returned the same id, but
with this change each ClientBackgroundSyncRegistration has as unique id.

-  EXPECT_TRUE(Register(sync_options_1_));
-  BackgroundSyncRegistration first_registration = callback_registration_;
-  EXPECT_TRUE(Register(sync_options_1_));
-  EXPECT_TRUE(callback_registration_.Equals(first_registration));
-  EXPECT_EQ(first_registration.id(), callback_registration_.id());
-}
-
 TEST_F(BackgroundSyncManagerTest, RegisterOverwrites) {
   EXPECT_TRUE(Register(sync_options_1_));
-  BackgroundSyncRegistration first_registration = callback_registration_;
+  scoped_ptr<ClientBackgroundSyncRegistration> first_registration =
+      callback_registration_.Pass();
 
   sync_options_1_.min_period = 100;
   EXPECT_TRUE(Register(sync_options_1_));
-  EXPECT_LT(first_registration.id(), callback_registration_.id());
-  EXPECT_FALSE(callback_registration_.Equals(first_registration));
+  EXPECT_LT(first_registration->id(), callback_registration_->id());
+  EXPECT_FALSE(callback_registration_->Equals(*first_registration));
 }
 
 TEST_F(BackgroundSyncManagerTest, RegisterOverlappingPeriodicAndOneShotTags) {
@@ -572,9 +573,9 @@ TEST_F(BackgroundSyncManagerTest, RegisterOverlappingPeriodicAndOneShotTags) {
   EXPECT_TRUE(Register(sync_options_1_));
   EXPECT_TRUE(Register(sync_options_2_));
   EXPECT_TRUE(GetRegistration(sync_options_1_));
-  EXPECT_EQ(SYNC_PERIODIC, callback_registration_.options()->periodicity);
+  EXPECT_EQ(SYNC_PERIODIC, callback_registration_->options()->periodicity);
   EXPECT_TRUE(GetRegistration(sync_options_2_));
-  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_.options()->periodicity);
+  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_->options()->periodicity);
 }
 
 TEST_F(BackgroundSyncManagerTest, RegisterBadBackend) {
@@ -614,15 +615,15 @@ TEST_F(BackgroundSyncManagerTest, GetRegistrationBadBackend) {
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationsZero) {
   EXPECT_TRUE(GetRegistrations(SYNC_ONE_SHOT));
-  EXPECT_EQ(0u, callback_registrations_.size());
+  EXPECT_EQ(0u, callback_registrations_->size());
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationsOne) {
   EXPECT_TRUE(Register(sync_options_1_));
   EXPECT_TRUE(GetRegistrations(sync_options_1_.periodicity));
 
-  EXPECT_EQ(1u, callback_registrations_.size());
-  sync_options_1_.Equals(*callback_registrations_[0].options());
+  EXPECT_EQ(1u, callback_registrations_->size());
+  sync_options_1_.Equals(*(*callback_registrations_)[0]->options());
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationsTwo) {
@@ -632,9 +633,9 @@ TEST_F(BackgroundSyncManagerTest, GetRegistrationsTwo) {
   EXPECT_TRUE(Register(sync_options_2_));
   EXPECT_TRUE(GetRegistrations(sync_options_1_.periodicity));
 
-  EXPECT_EQ(2u, callback_registrations_.size());
-  sync_options_1_.Equals(*callback_registrations_[0].options());
-  sync_options_2_.Equals(*callback_registrations_[1].options());
+  EXPECT_EQ(2u, callback_registrations_->size());
+  sync_options_1_.Equals(*(*callback_registrations_)[0]->options());
+  sync_options_2_.Equals(*(*callback_registrations_)[1]->options());
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationsPeriodicity) {
@@ -644,12 +645,12 @@ TEST_F(BackgroundSyncManagerTest, GetRegistrationsPeriodicity) {
   EXPECT_TRUE(Register(sync_options_2_));
 
   EXPECT_TRUE(GetRegistrations(SYNC_ONE_SHOT));
-  EXPECT_EQ(1u, callback_registrations_.size());
-  sync_options_1_.Equals(*callback_registrations_[0].options());
+  EXPECT_EQ(1u, callback_registrations_->size());
+  sync_options_1_.Equals(*(*callback_registrations_)[0]->options());
 
   EXPECT_TRUE(GetRegistrations(SYNC_PERIODIC));
-  EXPECT_EQ(1u, callback_registrations_.size());
-  sync_options_2_.Equals(*callback_registrations_[0].options());
+  EXPECT_EQ(1u, callback_registrations_->size());
+  sync_options_2_.Equals(*(*callback_registrations_)[0]->options());
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationsBadBackend) {
@@ -666,33 +667,30 @@ TEST_F(BackgroundSyncManagerTest, GetRegistrationsBadBackend) {
 
 TEST_F(BackgroundSyncManagerTest, Unregister) {
   EXPECT_TRUE(Register(sync_options_1_));
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_FALSE(GetRegistration(sync_options_1_));
 }
 
 TEST_F(BackgroundSyncManagerTest, UnregisterWrongId) {
   EXPECT_TRUE(Register(sync_options_1_));
-  callback_registration_.set_id(callback_registration_.id() + 1);
-  EXPECT_FALSE(Unregister(callback_registration_));
+  callback_registration_->set_id(callback_registration_->id() + 1);
+  EXPECT_FALSE(Unregister(callback_registration_.get()));
+  EXPECT_EQ(BACKGROUND_SYNC_STATUS_NOT_ALLOWED, callback_status_);
+
+  // Set the id back so that it's released before closing down
+  callback_registration_->set_id(callback_registration_->id() - 1);
 }
 
 TEST_F(BackgroundSyncManagerTest, Reregister) {
   EXPECT_TRUE(Register(sync_options_1_));
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_TRUE(Register(sync_options_1_));
 }
 
-TEST_F(BackgroundSyncManagerTest, UnregisterNonExisting) {

[iclelland, 2015/08/14 14:44:18]

There's still code in UnregisterImpl that does this -- why is the test gone?

[jkarlin, 2015/08/17 17:14:49]

Thanks for pointing this out. The BackgroundSyncManager now (as of this latest
patch) DCHECKS for the registration so the test needs to go.

-  BackgroundSyncRegistration nonexistant_registration;
-  nonexistant_registration.set_id(1);
-  EXPECT_FALSE(Unregister(nonexistant_registration));
-  EXPECT_EQ(BACKGROUND_SYNC_STATUS_NOT_FOUND, callback_status_);
-}
-
 TEST_F(BackgroundSyncManagerTest, UnregisterSecond) {
   EXPECT_TRUE(Register(sync_options_1_));
   EXPECT_TRUE(Register(sync_options_2_));
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_TRUE(GetRegistration(sync_options_1_));
   EXPECT_TRUE(Register(sync_options_2_));
 }
@@ -702,7 +700,7 @@ TEST_F(BackgroundSyncManagerTest, UnregisterBadBackend) {
   EXPECT_TRUE(Register(sync_options_1_));
   EXPECT_TRUE(Register(sync_options_2_));
   test_background_sync_manager_->set_corrupt_backend(true);
-  EXPECT_FALSE(Unregister(callback_registration_));
+  EXPECT_FALSE(Unregister(callback_registration_.get()));
   // Unregister should have discovered the bad backend and disabled the
   // BackgroundSyncManager.
   test_background_sync_manager_->set_corrupt_backend(false);
@@ -712,18 +710,18 @@ TEST_F(BackgroundSyncManagerTest, UnregisterBadBackend) {
 
 TEST_F(BackgroundSyncManagerTest, RegistrationIncreasesId) {
   EXPECT_TRUE(Register(sync_options_1_));
-  BackgroundSyncRegistration registered_sync = callback_registration_;
-  BackgroundSyncRegistration::RegistrationId cur_id =
-      callback_registration_.id();
+  scoped_ptr<ClientBackgroundSyncRegistration> registered_sync =
+      callback_registration_.Pass();
+  BackgroundSyncRegistration::RegistrationId cur_id = registered_sync->id();
 
   EXPECT_TRUE(GetRegistration(sync_options_1_));
   EXPECT_TRUE(Register(sync_options_2_));
-  EXPECT_LT(cur_id, callback_registration_.id());
-  cur_id = callback_registration_.id();
+  EXPECT_LT(cur_id, callback_registration_->id());
+  cur_id = callback_registration_->id();
 
-  EXPECT_TRUE(Unregister(registered_sync));
+  EXPECT_TRUE(Unregister(registered_sync.get()));
   EXPECT_TRUE(Register(sync_options_1_));
-  EXPECT_LT(cur_id, callback_registration_.id());
+  EXPECT_LT(cur_id, callback_registration_->id());
 }
 
 TEST_F(BackgroundSyncManagerTest, RebootRecovery) {
@@ -775,20 +773,12 @@ TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
   // the operations complete sequentially.
   SetupDelayedBackgroundSyncManager();
 
-  const int64 kExpectedInitialId = BackgroundSyncRegistration::kInitialId;
-
   bool register_called = false;
-  bool unregister_called = false;
   bool get_registration_called = false;
   test_background_sync_manager_->Register(
       sw_registration_id_1_, sync_options_1_,
       base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                  base::Unretained(this), &register_called));
-  test_background_sync_manager_->Unregister(
-      sw_registration_id_1_, sync_options_1_.tag, sync_options_1_.periodicity,
-      kExpectedInitialId,
-      base::Bind(&BackgroundSyncManagerTest::StatusCallback,
-                 base::Unretained(this), &unregister_called));
   test_background_sync_manager_->GetRegistration(
       sw_registration_id_1_, sync_options_1_.tag, sync_options_1_.periodicity,
       base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
@@ -797,31 +787,19 @@ TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
   base::RunLoop().RunUntilIdle();
   // Init should be blocked while loading from the backend.
   EXPECT_FALSE(register_called);
-  EXPECT_FALSE(unregister_called);
   EXPECT_FALSE(get_registration_called);
 
   test_background_sync_manager_->Continue();
   base::RunLoop().RunUntilIdle();
   // Register should be blocked while storing to the backend.
   EXPECT_FALSE(register_called);
-  EXPECT_FALSE(unregister_called);
   EXPECT_FALSE(get_registration_called);
 
   test_background_sync_manager_->Continue();
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(register_called);
-  EXPECT_EQ(kExpectedInitialId, callback_registration_.id());
   EXPECT_EQ(BACKGROUND_SYNC_STATUS_OK, callback_status_);
-  // Unregister should be blocked while storing to the backend.
-  EXPECT_FALSE(unregister_called);
-  EXPECT_FALSE(get_registration_called);
-
-  test_background_sync_manager_->Continue();
-  base::RunLoop().RunUntilIdle();
-  // Unregister should be done and since GetRegistration doesn't require the
-  // backend it should be done too.
-  EXPECT_EQ(BACKGROUND_SYNC_STATUS_NOT_FOUND, callback_status_);
-  EXPECT_TRUE(unregister_called);
+  // GetRegistration should run immediately as it doesn't write to disk.
   EXPECT_TRUE(get_registration_called);
 }
 
@@ -969,15 +947,15 @@ TEST_F(BackgroundSyncManagerTest, StoreAndRetrievePreservesValues) {
   SetupBackgroundSyncManager();
 
   EXPECT_TRUE(GetRegistration(options));
-  EXPECT_TRUE(options.Equals(*callback_registration_.options()));
+  EXPECT_TRUE(options.Equals(*callback_registration_->options()));
 }
 
 TEST_F(BackgroundSyncManagerTest, EmptyTagSupported) {
   sync_options_1_.tag = "a";
   EXPECT_TRUE(Register(sync_options_1_));
   EXPECT_TRUE(GetRegistration(sync_options_1_));
-  EXPECT_TRUE(sync_options_1_.Equals(*callback_registration_.options()));
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(sync_options_1_.Equals(*callback_registration_->options()));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_FALSE(GetRegistration(sync_options_1_));
 }
 
@@ -993,17 +971,17 @@ TEST_F(BackgroundSyncManagerTest, OverlappingPeriodicAndOneShotTags) {
   EXPECT_TRUE(Register(sync_options_2_));
 
   EXPECT_TRUE(GetRegistration(sync_options_1_));
-  EXPECT_EQ(SYNC_PERIODIC, callback_registration_.options()->periodicity);
+  EXPECT_EQ(SYNC_PERIODIC, callback_registration_->options()->periodicity);
   EXPECT_TRUE(GetRegistration(sync_options_2_));
-  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_.options()->periodicity);
+  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_->options()->periodicity);
 
   EXPECT_TRUE(GetRegistration(sync_options_1_));
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_FALSE(GetRegistration(sync_options_1_));
   EXPECT_TRUE(GetRegistration(sync_options_2_));
-  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_.options()->periodicity);
+  EXPECT_EQ(SYNC_ONE_SHOT, callback_registration_->options()->periodicity);
 
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_FALSE(GetRegistration(sync_options_2_));
 }
 
@@ -1126,14 +1104,12 @@ TEST_F(BackgroundSyncManagerTest, FailedOneShotReregisteredAndFires) {
   EXPECT_TRUE(Register(sync_options_1_));
   EXPECT_EQ(1, sync_events_called_);
   EXPECT_TRUE(GetRegistration(sync_options_1_));
-  BackgroundSyncRegistration first_registration = callback_registration_;
 
   InitSyncEventTest();
 
   // Reregistering should cause the sync event to fire again, this time
   // succeeding.
   EXPECT_TRUE(Register(sync_options_1_));
-  EXPECT_EQ(first_registration.id(), callback_registration_.id());
   EXPECT_EQ(2, sync_events_called_);
   EXPECT_FALSE(GetRegistration(sync_options_1_));
 }
@@ -1207,7 +1183,7 @@ TEST_F(BackgroundSyncManagerTest, UnregisterOneShotMidSync) {
 
   RegisterAndVerifySyncEventDelayed(sync_options_1_);
 
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_FALSE(GetRegistration(sync_options_1_));
 
   sync_fired_callback_.Run(SERVICE_WORKER_OK);
@@ -1272,7 +1248,7 @@ TEST_F(BackgroundSyncManagerTest, RegisterExistingFailsWithoutWindow) {
 TEST_F(BackgroundSyncManagerTest, UnregisterSucceedsWithoutWindow) {
   EXPECT_TRUE(Register(sync_options_1_));
   RemoveWindowClients();
-  EXPECT_TRUE(Unregister(callback_registration_));
+  EXPECT_TRUE(Unregister(callback_registration_.get()));
   EXPECT_FALSE(GetRegistration(sync_options_1_));
 }