ProfileSyncServiceHarness: Always call AwaitSyncSetupCompletion in SetupSync

Before this CL, SetupSync would early-out if skip_passphrase_verification
was set to true. I don't see a good reason for this, so now it always gets
called.
For reference, this logic was added in
https://codereview.chromium.org/2716413003/, but I couldn't find an
explanation for it there.

This is an attempt to fix Sync's E2E tests. Locally, it fixes some of them,
though others still fail.
The problem was that a ClearServerData task got scheduled in
SyncSchedulerImpl, but then before that task actually got a chance to run,
the scheduler was put back into CONFIGURATION mode via
SyncBackendHostImpl::ConfigureDataTypes (race condition). This change makes
sure that ConfigureDataTypes (and its async tasks) gets completed before
we try to clear server data.
More details on the failure can be found on the bug.

Bug: 835250
Change-Id: I392376d23930540ce9442911ec16e00a42abf043
Reviewed-on: https://chromium-review.googlesource.com/1028050Reviewed-by: vitaliii <vitaliii@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553588}

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

@@ -219,23 +219,23 @@ bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
         LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
   }
 
-  if (skip_passphrase_verification) {
-    return true;
-  }
-
-  // Set an implicit passphrase for encryption if an explicit one hasn't already
-  // been set. If an explicit passphrase has been set, immediately return false,
-  // since a decryption passphrase is required.
+  if (!skip_passphrase_verification) {
+    // Set an implicit passphrase for encryption if an explicit one hasn't
+    // already been set. If an explicit passphrase has been set, immediately
+    // return false, since a decryption passphrase is required.
     if (!service()->IsUsingSecondaryPassphrase()) {
-    service()->SetEncryptionPassphrase(password_, ProfileSyncService::IMPLICIT);
+      service()->SetEncryptionPassphrase(password_,
+                                         ProfileSyncService::IMPLICIT);
     } else {
-    LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
+      LOG(ERROR)
+          << "A passphrase is required for decryption. Sync cannot proceed"
              " until SetDecryptionPassphrase is called.";
       return false;
     }
+  }
 
   // Wait for initial sync cycle to be completed.
-  if (!AwaitSyncSetupCompletion()) {
+  if (!AwaitSyncSetupCompletion(skip_passphrase_verification)) {
     return false;
   }
 
@@ -251,7 +251,7 @@ bool ProfileSyncServiceHarness::RestartSyncService() {
   DVLOG(1) << "Requesting start for service";
   service()->RequestStart();
 
-  if (!AwaitEngineInitialization()) {
+  if (!AwaitEngineInitialization(/*skip_passphrase_verification=*/false)) {
     LOG(ERROR) << "AwaitEngineInitialization failed.";
     return false;
   }
@@ -271,7 +271,7 @@ bool ProfileSyncServiceHarness::RestartSyncService() {
   blocker.reset();
   service()->SetFirstSetupComplete();
 
-  if (!AwaitSyncSetupCompletion()) {
+  if (!AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false)) {
     LOG(FATAL) << "AwaitSyncSetupCompletion failed.";
     return false;
   }
@@ -340,14 +340,17 @@ bool ProfileSyncServiceHarness::AwaitEngineInitialization(
   return true;
 }
 
-bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() {
+bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion(
+    bool skip_passphrase_verification) {
   if (!SyncSetupChecker(service()).Wait()) {
     LOG(ERROR) << "SyncSetupChecker timed out.";
     return false;
   }
 
-  // Make sure that initial sync wasn't blocked by a missing passphrase.
-  if (service()->passphrase_required_reason() == syncer::REASON_DECRYPTION) {
+  // If passphrase verification is not skipped, make sure that initial sync
+  // wasn't blocked by a missing passphrase.
+  if (!skip_passphrase_verification &&
+      service()->passphrase_required_reason() == syncer::REASON_DECRYPTION) {
     LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
                   " until SetDecryptionPassphrase is called.";
     return false;
@@ -414,7 +417,7 @@ bool ProfileSyncServiceHarness::EnableSyncForDatatype(
   synced_datatypes.Put(syncer::ModelTypeFromInt(datatype));
   synced_datatypes.RetainAll(syncer::UserSelectableTypes());
   service()->OnUserChoseDatatypes(false, synced_datatypes);
-  if (AwaitSyncSetupCompletion()) {
+  if (AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false)) {
     DVLOG(1) << "EnableSyncForDatatype(): Enabled sync for datatype "
              << syncer::ModelTypeToString(datatype)
              << " on " << profile_debug_name_ << ".";
@@ -453,7 +456,7 @@ bool ProfileSyncServiceHarness::DisableSyncForDatatype(
   synced_datatypes.RetainAll(syncer::UserSelectableTypes());
   synced_datatypes.Remove(datatype);
   service()->OnUserChoseDatatypes(false, synced_datatypes);
-  if (AwaitSyncSetupCompletion()) {
+  if (AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false)) {
     DVLOG(1) << "DisableSyncForDatatype(): Disabled sync for datatype "
              << syncer::ModelTypeToString(datatype)
              << " on " << profile_debug_name_ << ".";
@@ -476,7 +479,7 @@ bool ProfileSyncServiceHarness::EnableSyncForAllDatatypes() {
   }
 
   service()->OnUserChoseDatatypes(true, syncer::UserSelectableTypes());
-  if (AwaitSyncSetupCompletion()) {
+  if (AwaitSyncSetupCompletion(/*skip_passphrase_verification=*/false)) {
     DVLOG(1) << "EnableSyncForAllDatatypes(): Enabled sync for all datatypes "
              << "on " << profile_debug_name_ << ".";
     return true;

chrome/browser/sync/test/integration/profile_sync_service_harness.h

@@ -99,12 +99,12 @@ class ProfileSyncServiceHarness {
   // (e.g., auth error) is reached. Returns true if and only if the engine
   // initialized successfully. See ProfileSyncService's IsEngineInitialized()
   // method for the definition of engine initialization.
-  bool AwaitEngineInitialization(bool skip_passphrase_verification = false);
+  bool AwaitEngineInitialization(bool skip_passphrase_verification);
 
   // Blocks the caller until sync setup is complete. Returns true if and only
   // if sync setup completed successfully. See syncer::SyncService's
   // IsSyncActive() method for the definition of what successful means here.
-  bool AwaitSyncSetupCompletion();
+  bool AwaitSyncSetupCompletion(bool skip_passphrase_verification);
 
   // Returns the ProfileSyncService member of the sync client.
   browser_sync::ProfileSyncService* service() const { return service_; }

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

@@ -235,7 +235,8 @@ IN_PROC_BROWSER_TEST_F(SyncErrorTest, ClientDataObsoleteTest) {
   // Make server return SUCCESS so that sync can initialize.
   EXPECT_TRUE(GetFakeServer()->TriggerError(sync_pb::SyncEnums::SUCCESS));
 
-  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization());
+  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization(
+      /*skip_passphrase_verification=*/false));
 
   // Ensure cache_guid changed.
   GetSyncService(0)->QueryDetailedSyncStatus(&status);