[Sync] Fix backup/rollback tests race conditions

chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc

 // Copyright 2014 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 "base/command_line.h"
 #include "base/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
+#include "base/test/test_timeouts.h"
+#include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
 #include "chrome/browser/sync/test/integration/preferences_helper.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "sync/internal_api/public/util/sync_db_util.h"
@@ skipping 45 matching lines @@
   }
 
  private:
   void CheckDbCallback(base::Time* time_out, base::Time time_in) {
     *time_out = syncer::ProtoTimeToTime(syncer::TimeToProtoTime(time_in));
   }
 
   DISALLOW_COPY_AND_ASSIGN(SingleClientBackupRollbackTest);
 };
 
-class SyncBackendStoppedChecker {
+// Waits until the ProfileSyncService's backend is in IDLE mode.
+class SyncBackendStoppedChecker : public ProfileSyncServiceBase::Observer {
  public:
-  explicit SyncBackendStoppedChecker(ProfileSyncService* service,
-                                     base::TimeDelta timeout)
+  explicit SyncBackendStoppedChecker(ProfileSyncService* service)
       : pss_(service),
-        timeout_(timeout) {}
+        timeout_(TestTimeouts::action_max_timeout()),
+        done_(false) {}
+
+  virtual void OnStateChanged() OVERRIDE {
+    if (ProfileSyncService::IDLE == pss_->backend_mode()) {
+      done_ = true;
+      run_loop_.Quit();
+    }
+  }
 
   bool Wait() {
-    expiration_ = base::TimeTicks::Now() + timeout_;
+    pss_->AddObserver(this);
+    if (ProfileSyncService::IDLE == pss_->backend_mode())
+      return true;
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
-        base::Bind(&SyncBackendStoppedChecker::PeriodicCheck,
-                   base::Unretained(this)),
-        base::TimeDelta::FromSeconds(1));
-    base::MessageLoop::current()->Run();
-    return ProfileSyncService::IDLE == pss_->backend_mode();
+        run_loop_.QuitClosure(),
+        timeout_);
+    run_loop_.Run();
+    pss_->RemoveObserver(this);
+    return done_;
   }
 
  private:
-  void PeriodicCheck() {
-    if (ProfileSyncService::IDLE == pss_->backend_mode() ||
-        base::TimeTicks::Now() > expiration_) {
-      base::MessageLoop::current()->Quit();
-    } else {
-      base::MessageLoop::current()->PostDelayedTask(
-          FROM_HERE,
-          base::Bind(&SyncBackendStoppedChecker::PeriodicCheck,
-                     base::Unretained(this)),
-          base::TimeDelta::FromSeconds(1));
+
+  ProfileSyncService* const pss_;
+  const base::TimeDelta timeout_;
+  base::RunLoop run_loop_;
+  bool done_;
+};
+
+// Waits until a rollback finishes.
+class SyncRollbackChecker : public ProfileSyncServiceBase::Observer,
+                            public BrowsingDataRemover::Observer {
+ public:
+  explicit SyncRollbackChecker(ProfileSyncService* service)
+      : pss_(service),
+        timeout_(TestTimeouts::action_max_timeout()),
+        rollback_started_(false),
+        clear_done_(false) {}
+
+  // ProfileSyncServiceBase::Observer implementation.
+  virtual void OnStateChanged() OVERRIDE {
+    if (ProfileSyncService::ROLLBACK == pss_->backend_mode()) {
+      rollback_started_ = true;
+      if (clear_done_)
+        run_loop_.Quit();
     }
   }
 
-  ProfileSyncService* pss_;
-  base::TimeDelta timeout_;
-  base::TimeTicks expiration_;
+  // BrowsingDataRemoverObserver::Observer implementation.
+  virtual void OnBrowsingDataRemoverDone() OVERRIDE {
+    clear_done_ = true;
+    if (rollback_started_) {
+      run_loop_.Quit();
+    }
+  }
+
+  bool Wait() {
+    pss_->AddObserver(this);
+    pss_->SetBrowsingDataRemoverObserverForTesting(this);
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        run_loop_.QuitClosure(),
+        timeout_);
+    run_loop_.Run();
+    pss_->RemoveObserver(this);
+    return rollback_started_ && clear_done_;
+  }
+
+  ProfileSyncService* const pss_;
+  const base::TimeDelta timeout_;
+  base::RunLoop run_loop_;
+  bool rollback_started_;
+  bool clear_done_;
 };
 
 #if defined(ENABLE_PRE_SYNC_BACKUP)
 #define MAYBE_TestBackup TestBackup
 #else
 #define MAYBE_TestBackup DISABLED_TestBackup
 #endif
 IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
                        MAYBE_TestBackup) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
@@ skipping 65 matching lines @@
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService(0)));
   ASSERT_TRUE(ModelMatchesVerifier(0));
 
   // Let server to return rollback command on next sync request.
   GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
 
   // Make another change to trigger downloading of rollback command.
   Remove(0, tier1_b, 0);
 
   // Wait for rollback to finish and sync backend is completely shut down.
-  SyncBackendStoppedChecker checker(GetSyncService(0),
-                                    base::TimeDelta::FromSeconds(5));
-  ASSERT_TRUE(checker.Wait());
+  SyncRollbackChecker rollback_checker(GetSyncService(0));
+  ASSERT_TRUE(rollback_checker.Wait());
+  SyncBackendStoppedChecker shutdown_checker(GetSyncService(0));
+  ASSERT_TRUE(shutdown_checker.Wait());
 
   // Verify bookmarks are restored.
   ASSERT_EQ(1, tier1_a->child_count());
   const BookmarkNode* url1 = tier1_a->GetChild(0);
   ASSERT_EQ(GURL("http://mail.google.com"), url1->url());
 
   ASSERT_EQ(1, tier1_b->child_count());
   const BookmarkNode* url2 = tier1_b->GetChild(0);
   ASSERT_EQ(GURL("http://www.nhl.com"), url2->url());
-
-  // Backup DB should be deleted after rollback is done.
-  ASSERT_FALSE(base::PathExists(
-      GetProfile(0)->GetPath().Append(FILE_PATH_LITERAL("Sync Data Backup"))));
 }
 
 #if defined(ENABLE_PRE_SYNC_BACKUP)
 #define MAYBE_TestRollbackDisabled TestRollbackDisabled
 #else
 #define MAYBE_TestRollbackDisabled DISABLED_TestRollbackDisabled
 #endif
 IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
                        MAYBE_TestRollbackDisabled) {
   DisableRollback();
@@ skipping 20 matching lines @@
                      GURL("http://www.yahoo.com")));
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
   ASSERT_TRUE(ModelMatchesVerifier(0));
 
   // Let server to return rollback command on next sync request.
   GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
 
   // Make another change to trigger downloading of rollback command.
   Remove(0, GetOtherNode(0), 0);
 
-  // Wait for rollback to finish and sync backend is completely shut down.
-  SyncBackendStoppedChecker checker(GetSyncService(0),
-                                    base::TimeDelta::FromSeconds(5));
-  ASSERT_TRUE(checker.Wait());
+  // Wait for sync backend is completely shut down.
+  SyncBackendStoppedChecker shutdown_checker(GetSyncService(0));
+  ASSERT_TRUE(shutdown_checker.Wait());
 
   // With rollback disabled, bookmarks in backup DB should not be restored.
   // Only bookmark added during sync is present.
   ASSERT_EQ(1, GetOtherNode(0)->child_count());
   ASSERT_EQ(GURL("http://www.yahoo.com"),
             GetOtherNode(0)->GetChild(0)->url());
-
-  // Backup DB should be deleted after.
-  ASSERT_FALSE(base::PathExists(
-      GetProfile(0)->GetPath().Append(FILE_PATH_LITERAL("Sync Data Backup"))));
 }
 
 #if defined(ENABLE_PRE_SYNC_BACKUP)
 #define MAYBE_TestSyncDisabled TestSyncDisabled
 #else
 #define MAYBE_TestSyncDisabled DISABLED_TestSyncDisabled
 #endif
 IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
                        MAYBE_TestSyncDisabled) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
@@ skipping 18 matching lines @@
                      GURL("http://www.yahoo.com")));
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
   ASSERT_TRUE(ModelMatchesVerifier(0));
 
   // Let server to return birthday error on next sync request.
   GetFakeServer()->TriggerError(sync_pb::SyncEnums::NOT_MY_BIRTHDAY);
 
   // Make another change to trigger downloading of rollback command.
   Remove(0, GetOtherNode(0), 0);
 
-  // Wait for rollback to finish and sync backend is completely shut down.
-  SyncBackendStoppedChecker checker(GetSyncService(0),
-                                    base::TimeDelta::FromSeconds(5));
-  ASSERT_TRUE(checker.Wait());
+  // Wait sync backend is completely shut down.
+  SyncBackendStoppedChecker shutdown_checker(GetSyncService(0));
+  ASSERT_TRUE(shutdown_checker.Wait());
 
   // Shouldn't restore bookmarks with sign-out only.
   ASSERT_EQ(1, GetOtherNode(0)->child_count());
   ASSERT_EQ(GURL("http://www.yahoo.com"),
             GetOtherNode(0)->GetChild(0)->url());
-
-  // Backup DB should be deleted after.
-  ASSERT_FALSE(base::PathExists(
-      GetProfile(0)->GetPath().Append(FILE_PATH_LITERAL("Sync Data Backup"))));
 }
 
 #if defined(ENABLE_PRE_SYNC_BACKUP)
 #define MAYBE_RollbackNoBackup RollbackNoBackup
 #else
 #define MAYBE_RollbackNoBackup DISABLED_RollbackNoBackup
 #endif
 IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
                        MAYBE_RollbackNoBackup) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
@@ skipping 17 matching lines @@
   base::DeleteFile(
       GetProfile(0)->GetPath().Append(FILE_PATH_LITERAL("Sync Data Backup")),
       true);
 
   // Let server to return rollback command on next sync request.
   GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
 
   // Make another change to trigger downloading of rollback command.
   Remove(0, GetOtherNode(0), 0);
 
-  // Wait for backend to completely shut down.
-  SyncBackendStoppedChecker checker(GetSyncService(0),
-                                    base::TimeDelta::FromSeconds(15));
+  // Wait for rollback to finish and sync backend is completely shut down.
+  SyncRollbackChecker rollback_checker(GetSyncService(0));
+  ASSERT_TRUE(rollback_checker.Wait());
+  SyncBackendStoppedChecker checker(GetSyncService(0));
   ASSERT_TRUE(checker.Wait());
 
   // Without backup DB, bookmarks remain at the state when sync stops.
   ASSERT_EQ(1, GetOtherNode(0)->child_count());
   ASSERT_EQ(GURL("http://www.nhl.com"),
             GetOtherNode(0)->GetChild(0)->url());
 }
 
 #if defined(ENABLE_PRE_SYNC_BACKUP)
 #define MAYBE_DontChangeBookmarkOrdering DontChangeBookmarkOrdering
@@ skipping 20 matching lines @@
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService(0)));
   ASSERT_TRUE(ModelMatchesVerifier(0));
 
   // Let server to return rollback command on next sync request.
   GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
 
   // Make another change to trigger downloading of rollback command.
   Remove(0, sub_folder, 0);
 
   // Wait for rollback to finish and sync backend is completely shut down.
-  SyncBackendStoppedChecker checker(GetSyncService(0),
-                                    base::TimeDelta::FromSeconds(5));
-  ASSERT_TRUE(checker.Wait());
+  SyncRollbackChecker rollback_checker(GetSyncService(0));

[haitaol1, 2014/08/21 20:50:32]

You always check rollback and backend shutdown back to back. So why do you need
rollback checker?

[Nicolas Zea, 2014/08/21 20:52:32]

Some of these tests don't require rollback (i.e. the ones with rollback
disabled, or just testing backup).  And it's not enough to check just for IDLE,
as that can happen before rollback completes. So for thoses tests where rollback
is expected, we verify it happens and completes, and only then wait for idle.

+  ASSERT_TRUE(rollback_checker.Wait());
+  SyncBackendStoppedChecker shutdown_checker(GetSyncService(0));
+  ASSERT_TRUE(shutdown_checker.Wait());
 
   // Verify bookmarks are unchanged.
   ASSERT_EQ(3, sub_folder->child_count());
   ASSERT_EQ(GURL(kUrl1), sub_folder->GetChild(0)->url());
   ASSERT_EQ(GURL(kUrl2), sub_folder->GetChild(1)->url());
   ASSERT_EQ(GURL(kUrl3), sub_folder->GetChild(2)->url());
 }