Fix background sync tests to rely on correct behaviour

This is a pre-requisite for https://codereview.chromium.org/1195903002/.

The background sync flag was used to prevent the Sync event from firing. After removing the flag, the Sync event started firing in several unit tests.  This caused many failures as the tests were not expecting sync registrations to be cleaned up by the completion of the sync event.

The affected tests have been fixed to setup the correct conditions:
- EmbeddedWorkerTestHelper will handle the Sync event, so callbacks can fire (preventing memory leaks in some tests)
- Most unit tests do not expect the Sync event to be fired. The containing test fixtures will now simulate no network connection, to prevent event firing
- Any tests that require the event to be fired, will explicitly change the network connection

BUG=501839

Review URL: https://codereview.chromium.org/1219783002

Cr-Commit-Position: refs/heads/master@{#336947}

2 files changed, 61 insertions, 47 deletions

diff --git a/content/browser/background_sync/background_sync_manager_unittest.cc b/content/browser/background_sync/background_sync_manager_unittest.cc
index f49db2d..ac999b2 100644
--- a/content/browser/background_sync/background_sync_manager_unittest.cc
+++ b/content/browser/background_sync/background_sync_manager_unittest.cc
@@ -222,8 +222,6 @@ class BackgroundSyncManagerTest : public testing::Test {
   }
 
   void SetUp() override {
-    SetNetwork(net::NetworkChangeNotifier::CONNECTION_WIFI);
-
     helper_.reset(
         new EmbeddedWorkerTestHelper(base::FilePath(), kRenderProcessId));
 
@@ -234,8 +232,7 @@ class BackgroundSyncManagerTest : public testing::Test {
 
     SetOnBatteryPower(false);
 
-    background_sync_manager_ =
-        BackgroundSyncManager::Create(helper_->context_wrapper());
+    SetupBackgroundSyncManager();
 
     // Wait for storage to finish initializing before registering service
     // workers.
@@ -260,7 +257,7 @@ class BackgroundSyncManagerTest : public testing::Test {
     EXPECT_TRUE(called_2);
 
     // Hang onto the registrations as they need to be "live" when
-    // calling BackgroundSyncMasnager::Register.
+    // calling BackgroundSyncManager::Register.
     helper_->context_wrapper()->FindRegistrationForId(
         sw_registration_id_1_, GURL(kPattern1).GetOrigin(),
         base::Bind(FindServiceWorkerRegistrationCallback, &sw_registration_1_));
@@ -310,13 +307,40 @@ class BackgroundSyncManagerTest : public testing::Test {
   }
 
  protected:
-  void UseTestBackgroundSyncManager() {
+  void CreateBackgroundSyncManager() {
     test_background_sync_manager_ =
         new TestBackgroundSyncManager(helper_->context_wrapper());
-    test_background_sync_manager_->DoInit();
     background_sync_manager_.reset(test_background_sync_manager_);
   }
 
+  void InitBackgroundSyncManager() {
+    test_background_sync_manager_->DoInit();
+
+    // Many tests do not expect the sync event to fire immediately after
+    // register (and cleanup up the sync registrations).  Tests can control when
+    // the sync event fires by manipulating the network state as needed.
+    // NOTE: The setup of the network connection must happen after the
+    //       BackgroundSyncManager has been setup.
+    SetNetwork(net::NetworkChangeNotifier::CONNECTION_NONE);
+  }
+
+  void SetupBackgroundSyncManager() {
+    CreateBackgroundSyncManager();
+    InitBackgroundSyncManager();
+  }
+
+  void SetupCorruptBackgroundSyncManager() {
+    CreateBackgroundSyncManager();
+    test_background_sync_manager_->set_corrupt_backend(true);
+    InitBackgroundSyncManager();
+  }
+
+  void SetupDelayedBackgroundSyncManager() {
+    CreateBackgroundSyncManager();
+    test_background_sync_manager_->set_delay_backend(true);
+    InitBackgroundSyncManager();
+  }
+
   void DeleteBackgroundSyncManager() {
     background_sync_manager_.reset();
     test_background_sync_manager_ = nullptr;
@@ -427,25 +451,24 @@ class BackgroundSyncManagerTest : public testing::Test {
     return sw_id == sw_registration_id_1_ ? GURL(kPattern1) : GURL(kPattern2);
   }
 
+  void SetupForSyncEvent(
+      const TestBackgroundSyncManager::OneShotCallback& callback) {
+    test_background_sync_manager_->set_one_shot_callback(callback);
+    SetNetwork(net::NetworkChangeNotifier::CONNECTION_WIFI);
+  }
+
   void InitSyncEventTest() {
-    UseTestBackgroundSyncManager();
-    test_background_sync_manager_->set_one_shot_callback(
+    SetupForSyncEvent(
         base::Bind(OneShotSuccessfulCallback, &sync_events_called_));
-    base::RunLoop().RunUntilIdle();
   }
 
   void InitFailedSyncEventTest() {
-    UseTestBackgroundSyncManager();
-    test_background_sync_manager_->set_one_shot_callback(
-        base::Bind(OneShotFailedCallback, &sync_events_called_));
-    base::RunLoop().RunUntilIdle();
+    SetupForSyncEvent(base::Bind(OneShotFailedCallback, &sync_events_called_));
   }
 
   void InitDelayedSyncEventTest() {
-    UseTestBackgroundSyncManager();
-    test_background_sync_manager_->set_one_shot_callback(base::Bind(
-        OneShotDelayedCallback, &sync_events_called_, &sync_fired_callback_));
-    base::RunLoop().RunUntilIdle();
+    SetupForSyncEvent(base::Bind(OneShotDelayedCallback, &sync_events_called_,
+                                 &sync_fired_callback_));
   }
 
   void RegisterAndVerifySyncEventDelayed(
@@ -549,7 +572,6 @@ TEST_F(BackgroundSyncManagerTest, RegisterOverlappingPeriodicAndOneShotTags) {
 }
 
 TEST_F(BackgroundSyncManagerTest, RegisterBadBackend) {
-  UseTestBackgroundSyncManager();
   test_background_sync_manager_->set_corrupt_backend(true);
   EXPECT_FALSE(Register(sync_reg_1_));
   test_background_sync_manager_->set_corrupt_backend(false);
@@ -573,7 +595,6 @@ TEST_F(BackgroundSyncManagerTest, GetRegistrationExisting) {
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationBadBackend) {
-  UseTestBackgroundSyncManager();
   EXPECT_TRUE(Register(sync_reg_1_));
   test_background_sync_manager_->set_corrupt_backend(true);
   EXPECT_TRUE(GetRegistration(sync_reg_1_));
@@ -626,7 +647,6 @@ TEST_F(BackgroundSyncManagerTest, GetRegistrationsPeriodicity) {
 }
 
 TEST_F(BackgroundSyncManagerTest, GetRegistrationsBadBackend) {
-  UseTestBackgroundSyncManager();
   EXPECT_TRUE(Register(sync_reg_1_));
   test_background_sync_manager_->set_corrupt_backend(true);
   EXPECT_TRUE(GetRegistrations(sync_reg_1_.periodicity));
@@ -670,7 +690,6 @@ TEST_F(BackgroundSyncManagerTest, UnregisterSecond) {
 }
 
 TEST_F(BackgroundSyncManagerTest, UnregisterBadBackend) {
-  UseTestBackgroundSyncManager();
   sync_reg_1_.min_period += 1;
   EXPECT_TRUE(Register(sync_reg_1_));
   EXPECT_TRUE(Register(sync_reg_2_));
@@ -703,8 +722,7 @@ TEST_F(BackgroundSyncManagerTest, RegistrationIncreasesId) {
 TEST_F(BackgroundSyncManagerTest, RebootRecovery) {
   EXPECT_TRUE(Register(sync_reg_1_));
 
-  background_sync_manager_ =
-      BackgroundSyncManager::Create(helper_->context_wrapper());
+  SetupBackgroundSyncManager();
 
   EXPECT_TRUE(GetRegistration(sync_reg_1_));
   EXPECT_FALSE(GetRegistration(sync_reg_2_));
@@ -714,8 +732,7 @@ TEST_F(BackgroundSyncManagerTest, RebootRecoveryTwoServiceWorkers) {
   EXPECT_TRUE(RegisterWithServiceWorkerId(sw_registration_id_1_, sync_reg_1_));
   EXPECT_TRUE(RegisterWithServiceWorkerId(sw_registration_id_2_, sync_reg_2_));
 
-  background_sync_manager_ =
-      BackgroundSyncManager::Create(helper_->context_wrapper());
+  SetupBackgroundSyncManager();
 
   EXPECT_TRUE(
       GetRegistrationWithServiceWorkerId(sw_registration_id_1_, sync_reg_1_));
@@ -736,12 +753,7 @@ TEST_F(BackgroundSyncManagerTest, RebootRecoveryTwoServiceWorkers) {
 }
 
 TEST_F(BackgroundSyncManagerTest, InitWithBadBackend) {
-  DeleteBackgroundSyncManager();
-  test_background_sync_manager_ =
-      new TestBackgroundSyncManager(helper_->context_wrapper());
-  background_sync_manager_.reset(test_background_sync_manager_);
-  test_background_sync_manager_->set_corrupt_backend(true);
-  test_background_sync_manager_->DoInit();
+  SetupCorruptBackgroundSyncManager();
 
   EXPECT_FALSE(Register(sync_reg_1_));
   EXPECT_FALSE(GetRegistration(sync_reg_1_));
@@ -750,14 +762,7 @@ TEST_F(BackgroundSyncManagerTest, InitWithBadBackend) {
 TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
   // Schedule Init and all of the operations on a delayed backend. Verify that
   // the operations complete sequentially.
-  DeleteBackgroundSyncManager();
-
-  test_background_sync_manager_ =
-      new TestBackgroundSyncManager(helper_->context_wrapper());
-  background_sync_manager_.reset(test_background_sync_manager_);
-
-  test_background_sync_manager_->set_delay_backend(true);
-  test_background_sync_manager_->DoInit();
+  SetupDelayedBackgroundSyncManager();
 
   const int64 kExpectedInitialId =
       BackgroundSyncManager::BackgroundSyncRegistration::kInitialId;
@@ -818,8 +823,6 @@ TEST_F(BackgroundSyncManagerTest, UnregisterServiceWorker) {
 
 TEST_F(BackgroundSyncManagerTest,
        UnregisterServiceWorkerDuringSyncRegistration) {
-  UseTestBackgroundSyncManager();
-
   EXPECT_TRUE(Register(sync_reg_1_));
 
   test_background_sync_manager_->set_delay_backend(true);
@@ -850,7 +853,6 @@ TEST_F(BackgroundSyncManagerTest, DeleteAndStartOverServiceWorkerContext) {
 }
 
 TEST_F(BackgroundSyncManagerTest, DisabledManagerWorksAfterBrowserRestart) {
-  UseTestBackgroundSyncManager();
   EXPECT_TRUE(Register(sync_reg_1_));
   test_background_sync_manager_->set_corrupt_backend(true);
   EXPECT_FALSE(Register(sync_reg_2_));
@@ -862,13 +864,12 @@ TEST_F(BackgroundSyncManagerTest, DisabledManagerWorksAfterBrowserRestart) {
   EXPECT_FALSE(Register(sync_reg_2_));
 
   // Simulate restarting the browser by creating a new BackgroundSyncManager.
-  UseTestBackgroundSyncManager();
+  SetupBackgroundSyncManager();
   EXPECT_TRUE(GetRegistration(sync_reg_1_));
   EXPECT_TRUE(Register(sync_reg_2_));
 }
 
 TEST_F(BackgroundSyncManagerTest, DisabledManagerWorksAfterDeleteAndStartOver) {
-  UseTestBackgroundSyncManager();
   EXPECT_TRUE(Register(sync_reg_1_));
   test_background_sync_manager_->set_corrupt_backend(true);
   EXPECT_FALSE(Register(sync_reg_2_));
@@ -955,7 +956,7 @@ TEST_F(BackgroundSyncManagerTest, StoreAndRetrievePreservesValues) {
 
   // Simulate restarting the sync manager, forcing the next read to come from
   // disk.
-  UseTestBackgroundSyncManager();
+  SetupBackgroundSyncManager();
 
   EXPECT_TRUE(GetRegistration(reg_1));
   EXPECT_TRUE(reg_1.Equals(callback_registration_));
@@ -1085,6 +1086,7 @@ TEST_F(BackgroundSyncManagerTest, OneShotFiresOnManagerRestart) {
 
   // The next time the manager is started, the network is good.
   SetNetwork(net::NetworkChangeNotifier::CONNECTION_WIFI);
+  SetupBackgroundSyncManager();
   InitSyncEventTest();
 
   // The event should have fired.
@@ -1219,6 +1221,7 @@ TEST_F(BackgroundSyncManagerTest, KillManagerMidSync) {
   RegisterAndVerifySyncEventDelayed(sync_reg_1_);
 
   // Create a new manager which should fire the sync again on init.
+  SetupBackgroundSyncManager();
   InitSyncEventTest();
   EXPECT_FALSE(GetRegistration(sync_reg_1_));
   EXPECT_EQ(2, sync_events_called_);
diff --git a/content/browser/background_sync/background_sync_service_impl_unittest.cc b/content/browser/background_sync/background_sync_service_impl_unittest.cc
index 9c2ed5d..2ffac17 100644
--- a/content/browser/background_sync/background_sync_service_impl_unittest.cc
+++ b/content/browser/background_sync/background_sync_service_impl_unittest.cc
@@ -16,6 +16,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "mojo/public/cpp/bindings/interface_ptr.h"
+#include "net/base/network_change_notifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
@@ -89,7 +90,8 @@ class BackgroundSyncServiceImplTest : public testing::Test {
  public:
   BackgroundSyncServiceImplTest()
       : thread_bundle_(
-            new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)) {
+            new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)),
+        network_change_notifier_(net::NetworkChangeNotifier::CreateMock()) {
     default_sync_registration_ = SyncRegistration::New();
   }
 
@@ -119,6 +121,14 @@ class BackgroundSyncServiceImplTest : public testing::Test {
 
     background_sync_context_ = new BackgroundSyncContextImpl();
     background_sync_context_->Init(embedded_worker_helper_->context_wrapper());
+
+    // Tests do not expect the sync event to fire immediately after
+    // register (and cleanup up the sync registrations).  Prevent the sync
+    // event from firing by setting the network state to have no connection.
+    // NOTE: The setup of the network connection must happen after the
+    //       BackgroundSyncManager has been setup.
+    net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+        net::NetworkChangeNotifier::CONNECTION_NONE);
     base::RunLoop().RunUntilIdle();
   }
 
@@ -183,6 +193,7 @@ class BackgroundSyncServiceImplTest : public testing::Test {
   }
 
   scoped_ptr<TestBrowserThreadBundle> thread_bundle_;
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   scoped_ptr<EmbeddedWorkerTestHelper> embedded_worker_helper_;
   scoped_ptr<base::PowerMonitor> power_monitor_;
   scoped_refptr<BackgroundSyncContextImpl> background_sync_context_;