Initial clear server data impl

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

Index: chrome/browser/sync/test/integration/profile_sync_service_harness.cc
diff --git a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
index 97f45645a54a9bc05db265383cc7f7fa1849b5c1..81aff5cd34c1e1a7e9975ee455b08225561ef39d 100644
--- a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
+++ b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
@@ -130,7 +130,7 @@ void ProfileSyncServiceHarness::SetCredentials(const std::string& username,
 }
 
 bool ProfileSyncServiceHarness::SetupSync() {
-  bool result = SetupSync(syncer::UserSelectableTypes());
+  bool result = SetupSync(syncer::UserSelectableTypes(), false);
   if (result == false) {

[skym, 2017/03/07 21:32:24]

Ahh, you were copying from here. Can you fix this while you're in here? :)

[wylieb, 2017/03/07 23:04:40]

Done.

     std::string status = GetServiceStatus();
     LOG(ERROR) << profile_debug_name_
@@ -141,8 +141,21 @@ bool ProfileSyncServiceHarness::SetupSync() {
   return result;
 }
 
-bool ProfileSyncServiceHarness::SetupSync(
-    syncer::ModelTypeSet synced_datatypes) {
+bool ProfileSyncServiceHarness::SetupSyncForClear() {
+  bool result = SetupSync(syncer::UserSelectableTypes(), true);
+  if (result == false) {

[skym, 2017/03/07 21:32:24]

The result == false is a bit verbose. What do you think of

if (!result) { /*failure*/ } else { /*success*/ }

or even better

if (result) { /*success*/ } else { /*failure*/ }

[wylieb, 2017/03/07 23:04:39]

Done.

+    std::string status = GetServiceStatus();

[skym, 2017/03/07 21:32:24]

What do you think of in-lining this?

[wylieb, 2017/03/07 23:04:39]

Done.

+    LOG(ERROR) << profile_debug_name_
+               << ": SetupSyncForClear failed. Syncer status:\n"
+               << status;
+  } else {
+    DVLOG(1) << profile_debug_name_ << ": SetupSyncForClear successful.";
+  }
+  return result;
+}
+
+bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
+                                          bool forClear) {
   DCHECK(!profile_->IsLegacySupervised())
       << "SetupSync should not be used for legacy supervised users.";
 
@@ -179,7 +192,11 @@ bool ProfileSyncServiceHarness::SetupSync(
   // Now that auth is completed, request that sync actually start.
   service()->RequestStart();
 
-  if (!AwaitEngineInitialization()) {
+  if (forClear && !AwaitEngineInitializationForClear()) {
+    LOG(ERROR) << "AwaitEngineInitializationForClear failed.";

[skym, 2017/03/07 21:32:25]

These logs are kind of redundant, the await call already logs messages, right?
I'm okay if you think they're adding value and you want to keep them, I just
want us to think about it.

[wylieb, 2017/03/07 23:04:40]

Done.

+    return false;
+  } else if (!forClear && !AwaitEngineInitialization()) {
+    LOG(ERROR) << "AwaitEngineInitialization failed.";
     return false;
   }
 

[skym, 2017/03/07 21:32:24]

Why do you need to go any farther than waiting for the engine to init to perform
a server clear? I could believe a service()->RequestStart(); is required as
well. But do you really need things like AwaitSyncSetupCompletionForClear()?

[wylieb, 2017/03/07 23:04:40]

Good insight, thanks!

Done.

@@ -202,7 +219,10 @@ bool ProfileSyncServiceHarness::SetupSync(
   // 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()) {
+  if (forClear) {
+    // If we're clearing server data, don't check for decryption passphrase.
+    LOG(WARNING) << "Not checking passphrase decryption";

[skym, 2017/03/07 21:32:25]

Punctuation!

[wylieb, 2017/03/07 23:04:40]

Done.

+  } else if (!service()->IsUsingSecondaryPassphrase()) {
     service()->SetEncryptionPassphrase(password_, ProfileSyncService::IMPLICIT);
   } else {
     LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
@@ -211,11 +231,40 @@ bool ProfileSyncServiceHarness::SetupSync(
   }
 
   // Wait for initial sync cycle to be completed.
-  if (!AwaitSyncSetupCompletion()) {
-    LOG(ERROR) << "Initial sync cycle timed out.";
+  if (forClear && !AwaitSyncSetupCompletionForClear()) {
+    LOG(ERROR) << "AwaitSyncSetupCompletionForClear failed.";
+    return false;
+  } else if (!forClear && !AwaitSyncSetupCompletion()) {
+    LOG(ERROR) << "AwaitSyncSetupCompletion failed.";
+    return false;
+  }
+
+  return true;
+}
+
+bool ProfileSyncServiceHarness::RestartSyncService() {
+  sync_blocker_ = service()->GetSetupInProgressHandle();

[skym, 2017/03/07 21:32:25]

Sorry if it feels like I'm flip-flopping here, but I don't think you should use
the member blocker. I didn't understand what was going on here previously, and
now that I looked into it, I'm not convinced it's actually useful in the general
case. I'm running try bots against https://codereview.chromium.org/2735253002
right now which tries to remove it.

If you do still need it for this restart case, then perhaps it should be scoped
to this function. This is nicer anyways because then we don't have to rely on
FinishSyncSetup() releasing this blocker which just seems really fragile.

[wylieb, 2017/03/07 23:04:40]

Yeah that's why I had it that way to begin with. Could have some leaks here if
something calls SetupSync() while this happens.

+  DVLOG(1) << "Requesting stop for service.";
+  service()->RequestStop(ProfileSyncService::CLEAR_DATA);
+  DVLOG(1) << "Requesting start for service";
+
+  service()->RequestStart();
+  if (!AwaitEngineInitialization()) {
+    LOG(ERROR) << "AwaitEngineInitialization failed.";
+    return false;
+  }
+
+  // Check passphrase now, should be implicit

[skym, 2017/03/07 21:32:24]

This comment should end with a period as well.

[wylieb, 2017/03/07 23:04:40]

Done.

+  if (!service()->IsUsingSecondaryPassphrase()) {
+    service()->SetEncryptionPassphrase(password_, ProfileSyncService::IMPLICIT);
+  } else {
+    LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
+                  " until SetDecryptionPassphrase is called.";
     return false;
   }
 
+  FinishSyncSetup();
+

[skym, 2017/03/07 21:32:24]

Do you need to call AwaitSyncSetupCompletion() ?

[wylieb, 2017/03/07 23:04:40]

Good point.

Done.

[skym, 2017/03/08 19:15:02]

This never happened as far as I can tell.

[wylieb, 2017/03/09 18:42:26]

Sorry. I tried to implement this, but it times out when waiting. The condition
that fails is IsSyncActive(). Not sure what's going on there -- got stuck
debugging. The tests pass without this AwaitSyncSetup() call. Doing this wait in
the SetupSyncForClear() method seems to be enough. Thoughts?

[skym, 2017/03/10 17:36:31]

Was it possible this was while the blocker was still active? What if you
manually reset the blocker, call SetFirstSetupComplete(), and then await. I
think this needs to be figured out, otherwise we're setting ourselves up for
some flakiness.

   return true;
 }
 
@@ -272,7 +321,28 @@ bool ProfileSyncServiceHarness::AwaitEngineInitialization() {
   return true;
 }
 
+bool ProfileSyncServiceHarness::AwaitEngineInitializationForClear() {
+  LOG(WARNING) << "AwaitEngineInitializationForClear";
+  if (!EngineInitializeChecker(service()).Wait()) {
+    LOG(ERROR) << "EngineInitializeChecker timed out.";
+    return false;
+  }
+
+  if (!service()->IsEngineInitialized()) {
+    LOG(ERROR) << "Service engine not initialized.";
+    return false;
+  }
+
+  if (HasAuthError(service())) {
+    LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
+    return false;
+  }
+
+  return true;
+}
+
 bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() {
+  LOG(WARNING) << "AwaitSyncSetupCompletion";
   if (!SyncSetupChecker(service()).Wait()) {
     LOG(ERROR) << "SyncSetupChecker timed out.";
     return false;
@@ -293,6 +363,21 @@ bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() {
   return true;
 }
 
+bool ProfileSyncServiceHarness::AwaitSyncSetupCompletionForClear() {
+  LOG(WARNING) << "AwaitSyncSetupCompletionForClear";
+  if (!SyncSetupChecker(service()).Wait()) {
+    LOG(ERROR) << "SyncSetupChecker timed out.";
+    return false;
+  }
+
+  if (HasAuthError(service())) {
+    LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
+    return false;
+  }
+
+  return true;
+}
+
 std::string ProfileSyncServiceHarness::GenerateFakeOAuth2RefreshTokenString() {
   return base::StringPrintf("oauth2_refresh_token_%d",
                             ++oauth2_refesh_token_number_);