Skip to content

Commit

Permalink
Fix passphrase initialization flow in ProfileSyncServiceHarness
Browse files Browse the repository at this point in the history 
There was a race condition in ProfileSyncServiceHarness due to its multiple
inheritance of ProfileSyncServiceObserver and NotificationObserver,
causing several integration tests to fail.

This patch contains a fix that involves not inheriting from
NotificationObserver, and simply relying on the OnStateChanged()
mechanism of ProfileSyncServiceObserver, thereby eliminating the race.

It also contains 2 new test cases that further verify passphrase functionality:
SetPassphraseAndThenSetupSync and SetPassphraseTwice.

BUG=64109
TEST=sync_integration_tests

Review URL: http://codereview.chromium.org/5801003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69203 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rsimha@chromium.org committed Dec 15, 2010
1 parent 56b351d commit 6e759aa
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 50 deletions.
42 changes: 9 additions & 33 deletions chrome/browser/sync/profile_sync_service_harness.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ ProfileSyncServiceHarness::ProfileSyncServiceHarness(
min_timestamp_needed_(kMinTimestampNeededNone),
username_(username),
password_(password),
passphrase_acceptance_counter_(0),
id_(id) {
if (IsSyncAlreadySetup()) {
service_ = profile_->GetProfileSyncService();
Expand Down Expand Up @@ -132,16 +131,6 @@ bool ProfileSyncServiceHarness::SetupSync() {
return SetupSync(synced_datatypes);
}

void ProfileSyncServiceHarness::StartObservingPassphraseEvents() {
// Prime the counter to account for the implicit set passphrase due to
// gaia login.
passphrase_acceptance_counter_--;
registrar_.Add(this, NotificationType::SYNC_PASSPHRASE_ACCEPTED,
Source<browser_sync::SyncBackendHost>(service_->backend()));
registrar_.Add(this, NotificationType::SYNC_PASSPHRASE_REQUIRED,
Source<browser_sync::SyncBackendHost>(service_->backend()));
}

bool ProfileSyncServiceHarness::SetupSync(
const syncable::ModelTypeSet& synced_datatypes) {
// Initialize the sync client's profile sync service object.
Expand Down Expand Up @@ -184,10 +173,6 @@ bool ProfileSyncServiceHarness::SetupSync(
return false;
}

DCHECK(service()->observed_passphrase_required());
if (id_ == 0)
DCHECK(!service()->passphrase_required_for_decryption());

// Wait for initial gaia passphrase to be accepted.
DCHECK_EQ(wait_state_, WAITING_FOR_PASSPHRASE_ACCEPTED);
if (!AwaitStatusChangeWithTimeout(kLiveSyncOperationTimeoutMs,
Expand Down Expand Up @@ -230,21 +215,26 @@ bool ProfileSyncServiceHarness::RunStateChangeMachine() {
case WAITING_FOR_ON_BACKEND_INITIALIZED: {
LogClientInfo("WAITING_FOR_ON_BACKEND_INITIALIZED");
if (service()->sync_initialized()) {
// The sync backend is initialized. Watch for passphrase events.
StartObservingPassphraseEvents();
// The sync backend is initialized. We now wait for passphrase events.
SignalStateCompleteWithNextState(WAITING_FOR_PASSPHRASE_REQUIRED);
}
break;
}
case WAITING_FOR_PASSPHRASE_REQUIRED: {
LogClientInfo("WAITING_FOR_PASSPHRASE_REQUIRED");
if (service_->observed_passphrase_required())
if (service()->observed_passphrase_required()) {
// Special case when the first client signs in to sync.
if (id_ == 0)
DCHECK(!service()->passphrase_required_for_decryption());
// The SYNC_PASSPHRASE_REQUIRED notification has been seen.
SignalStateCompleteWithNextState(WAITING_FOR_PASSPHRASE_ACCEPTED);
}
break;
}
case WAITING_FOR_PASSPHRASE_ACCEPTED: {
LogClientInfo("WAITING_FOR_PASSPHRASE_ACCEPTED");
if (passphrase_acceptance_counter_ >= 0)
if (service()->ShouldPushChanges())
// The SYNC_PASSPHRASE_ACCEPTED notification has been seen.
SignalStateCompleteWithNextState(WAITING_FOR_INITIAL_SYNC);
break;
}
Expand Down Expand Up @@ -318,25 +308,11 @@ bool ProfileSyncServiceHarness::AwaitPassphraseAccepted() {
LOG(ERROR) << "Sync disabled for Client " << id_ << ".";
return false;
}
passphrase_acceptance_counter_--;
if (passphrase_acceptance_counter_ >= 0)
return true;
wait_state_ = WAITING_FOR_PASSPHRASE_ACCEPTED;
return AwaitStatusChangeWithTimeout(kLiveSyncOperationTimeoutMs,
"Waiting for passphrase accepted.");
}

void ProfileSyncServiceHarness::Observe(NotificationType type,
const NotificationSource& source,
const NotificationDetails& details) {
if (NotificationType::SYNC_PASSPHRASE_ACCEPTED == type.value) {
passphrase_acceptance_counter_++;
RunStateChangeMachine();
} else if (NotificationType::SYNC_PASSPHRASE_REQUIRED == type.value) {
RunStateChangeMachine();
}
}

bool ProfileSyncServiceHarness::AwaitSyncCycleCompletion(
const std::string& reason) {
LogClientInfo("AwaitSyncCycleCompletion");
Expand Down
18 changes: 1 addition & 17 deletions chrome/browser/sync/profile_sync_service_harness.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@

#include "base/time.h"
#include "chrome/browser/sync/profile_sync_service.h"
#include "chrome/common/notification_observer.h"
#include "chrome/common/notification_registrar.h"

using browser_sync::sessions::SyncSessionSnapshot;

Expand All @@ -23,8 +21,7 @@ class Profile;
// profile passed to it on construction and automates certain things like setup
// and authentication. It provides ways to "wait" adequate periods of time for
// several clients to get to the same state.
class ProfileSyncServiceHarness : public ProfileSyncServiceObserver,
public NotificationObserver {
class ProfileSyncServiceHarness : public ProfileSyncServiceObserver {
public:
ProfileSyncServiceHarness(Profile* profile,
const std::string& username,
Expand Down Expand Up @@ -57,11 +54,6 @@ class ProfileSyncServiceHarness : public ProfileSyncServiceObserver,
// ProfileSyncServiceObserver implementation.
virtual void OnStateChanged();

// NotificationObserver implementation.
virtual void Observe(NotificationType type,
const NotificationSource& source,
const NotificationDetails& details);

// Blocks the caller until this harness has completed a single sync cycle
// since the previous one. Returns true if a sync cycle has completed.
bool AwaitSyncCycleCompletion(const std::string& reason);
Expand Down Expand Up @@ -183,8 +175,6 @@ class ProfileSyncServiceHarness : public ProfileSyncServiceObserver,
// Returns the new value of |last_timestamp_|.
int64 GetUpdatedTimestamp();

void StartObservingPassphraseEvents();

WaitState wait_state_;

Profile* profile_;
Expand All @@ -204,12 +194,6 @@ class ProfileSyncServiceHarness : public ProfileSyncServiceObserver,
std::string username_;
std::string password_;

// A counter to track the number of await passphrase requests versus
// actual acceptances. Can go negative if #requests > #acceptances.
int passphrase_acceptance_counter_;

NotificationRegistrar registrar_;

// Client ID, used for logging purposes.
int id_;

Expand Down
46 changes: 46 additions & 0 deletions chrome/test/live_sync/two_client_live_passwords_sync_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,49 @@ IN_PROC_BROWSER_TEST_F(TwoClientLivePasswordsSyncTest,
ASSERT_EQ(0, GetClient(1)->GetLastSessionSnapshot()->
num_conflicting_updates);
}

// TODO(sync): Remove FAILS_ annotation after http://crbug.com/59867 is fixed.
#if defined(OS_MACOSX)
IN_PROC_BROWSER_TEST_F(TwoClientLivePasswordsSyncTest,
FAILS_SetPassphraseAndThenSetupSync) {
#else
IN_PROC_BROWSER_TEST_F(TwoClientLivePasswordsSyncTest,
SetPassphraseAndThenSetupSync) {
#endif
ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
ASSERT_TRUE(GetClient(0)->SetupSync());
GetClient(0)->service()->SetPassphrase(kValidPassphrase, true);
GetClient(0)->AwaitPassphraseAccepted();
GetClient(0)->AwaitSyncCycleCompletion("Initial sync.");

ASSERT_TRUE(GetClient(1)->SetupSync());
ASSERT_TRUE(AwaitQuiescence());
ASSERT_TRUE(GetClient(1)->service()->observed_passphrase_required());
GetClient(1)->service()->SetPassphrase(kValidPassphrase, true);
GetClient(1)->AwaitPassphraseAccepted();
ASSERT_FALSE(GetClient(1)->service()->observed_passphrase_required());
}

// TODO(sync): Remove FAILS_ annotation after http://crbug.com/59867 is fixed.
#if defined(OS_MACOSX)
IN_PROC_BROWSER_TEST_F(TwoClientLivePasswordsSyncTest,
FAILS_SetPassphraseTwice) {
#else
IN_PROC_BROWSER_TEST_F(TwoClientLivePasswordsSyncTest,
SetPassphraseTwice) {
#endif
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";

GetClient(0)->service()->SetPassphrase(kValidPassphrase, true);
GetClient(0)->AwaitPassphraseAccepted();
GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1));
ASSERT_TRUE(GetClient(1)->service()->observed_passphrase_required());

GetClient(1)->service()->SetPassphrase(kValidPassphrase, true);
GetClient(1)->AwaitPassphraseAccepted();
ASSERT_FALSE(GetClient(1)->service()->observed_passphrase_required());

GetClient(1)->service()->SetPassphrase(kValidPassphrase, true);
GetClient(1)->AwaitPassphraseAccepted();
ASSERT_FALSE(GetClient(1)->service()->observed_passphrase_required());
}

0 comments on commit 6e759aa

Please sign in to comment.