Initial clear server data impl

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

 // Copyright 2013 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 "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 
 #include <cstddef>
 #include <iterator>
 #include <ostream>
 #include <sstream>
@@ skipping 112 matching lines @@
 
 ProfileSyncServiceHarness::~ProfileSyncServiceHarness() { }
 
 void ProfileSyncServiceHarness::SetCredentials(const std::string& username,
                                                const std::string& password) {
   username_ = username;
   password_ = password;
 }
 
 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_
                << ": SetupSync failed. Syncer status:\n" << status;
   } else {
     DVLOG(1) << profile_debug_name_ << ": SetupSync successful.";
   }
   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.";
 
   // Initialize the sync client's profile sync service object.
   if (service() == nullptr) {
     LOG(ERROR) << "SetupSync(): service() is null.";
     return false;
   }
 
   // Tell the sync service that setup is in progress so we don't start syncing
@@ skipping 16 matching lines @@
     service()->GoogleSigninSucceeded(account_id, username_, password_);
     ProfileOAuth2TokenServiceFactory::GetForProfile(profile_)->
       UpdateCredentials(account_id, GenerateFakeOAuth2RefreshTokenString());
   } else {
     LOG(ERROR) << "Unsupported profile signin type.";
   }
 
   // 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.

   // Choose the datatypes to be synced. If all datatypes are to be synced,
   // set sync_everything to true; otherwise, set it to false.
   bool sync_everything = (synced_datatypes == syncer::UserSelectableTypes());
   service()->OnUserChoseDatatypes(sync_everything, synced_datatypes);
 
   // Notify ProfileSyncService that we are done with configuration.
   FinishSyncSetup();
 
   if ((signin_type_ == SigninType::UI_SIGNIN) &&

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

What kind of sign in are E2E tests by the way? FAKE_SIGNIN?

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

UI_SIGNIN

       !login_ui_test_utils::DismissSyncConfirmationDialog(
           chrome::FindBrowserWithProfile(profile_),
           base::TimeDelta::FromSeconds(30))) {
     LOG(ERROR) << "Failed to dismiss sync confirmation dialog.";
     return false;
   }
 
   // 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"
                   " until SetDecryptionPassphrase is called.";
     return false;
   }
 
   // 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;
+}
+
 bool ProfileSyncServiceHarness::AwaitMutualSyncCycleCompletion(
     ProfileSyncServiceHarness* partner) {
   std::vector<ProfileSyncServiceHarness*> harnesses;
   harnesses.push_back(this);
   harnesses.push_back(partner);
   return AwaitQuiescence(harnesses);
 }
 
 bool ProfileSyncServiceHarness::AwaitGroupSyncCycleCompletion(
     const std::vector<ProfileSyncServiceHarness*>& partners) {
@@ skipping 33 matching lines @@
   }
 
   if (HasAuthError(service())) {
     LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
     return false;
   }
 
   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;
   }
 
   // Make sure that initial sync wasn't blocked by a missing passphrase.
   if (service()->passphrase_required_reason() == syncer::REASON_DECRYPTION) {
     LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
                   " until SetDecryptionPassphrase is called.";
     return false;
   }
 
   if (HasAuthError(service())) {
     LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
     return false;
   }
 
   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_);
 }
 
 bool ProfileSyncServiceHarness::IsSyncDisabled() const {
   return !service()->IsSetupInProgress() && !service()->IsFirstSetupComplete();
 }
 
 void ProfileSyncServiceHarness::FinishSyncSetup() {
@@ skipping 170 matching lines @@
 
 std::string ProfileSyncServiceHarness::GetServiceStatus() {
   std::unique_ptr<base::DictionaryValue> value(
       syncer::sync_ui_util::ConstructAboutInformation(
           service(), service()->signin(), chrome::GetChannel()));
   std::string service_status;
   base::JSONWriter::WriteWithOptions(
       *value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &service_status);
   return service_status;
 }