diff options
34 files changed, 833 insertions, 1367 deletions
diff --git a/sync/engine/apply_updates_and_resolve_conflicts_command.cc b/sync/engine/apply_updates_and_resolve_conflicts_command.cc deleted file mode 100644 index 54b0b38..0000000 --- a/sync/engine/apply_updates_and_resolve_conflicts_command.cc +++ /dev/null @@ -1,134 +0,0 @@ -// Copyright 2012 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 "sync/engine/apply_updates_and_resolve_conflicts_command.h" - -#include "base/location.h" -#include "sync/engine/conflict_resolver.h" -#include "sync/engine/update_applicator.h" -#include "sync/sessions/sync_session.h" -#include "sync/syncable/directory.h" -#include "sync/syncable/syncable_read_transaction.h" -#include "sync/syncable/syncable_write_transaction.h" - -namespace syncer { - -using sessions::SyncSession; - -ApplyUpdatesAndResolveConflictsCommand:: - ApplyUpdatesAndResolveConflictsCommand() {} -ApplyUpdatesAndResolveConflictsCommand:: - ~ApplyUpdatesAndResolveConflictsCommand() {} - -std::set<ModelSafeGroup> -ApplyUpdatesAndResolveConflictsCommand::GetGroupsToChange( - const sessions::SyncSession& session) const { - std::set<ModelSafeGroup> groups_with_unapplied_updates; - - FullModelTypeSet server_types_with_unapplied_updates; - { - syncable::Directory* dir = session.context()->directory(); - syncable::ReadTransaction trans(FROM_HERE, dir); - server_types_with_unapplied_updates = - dir->GetServerTypesWithUnappliedUpdates(&trans); - } - - for (FullModelTypeSet::Iterator it = - server_types_with_unapplied_updates.First(); it.Good(); it.Inc()) { - groups_with_unapplied_updates.insert( - GetGroupForModelType(it.Get(), session.context()->routing_info())); - } - - return groups_with_unapplied_updates; -} - -SyncerError ApplyUpdatesAndResolveConflictsCommand::ModelChangingExecuteImpl( - SyncSession* session) { - sessions::StatusController* status = session->mutable_status_controller(); - syncable::Directory* dir = session->context()->directory(); - syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir); - - // Compute server types with unapplied updates that fall under our - // group restriction. - const FullModelTypeSet server_types_with_unapplied_updates = - dir->GetServerTypesWithUnappliedUpdates(&trans); - FullModelTypeSet server_type_restriction; - for (FullModelTypeSet::Iterator it = - server_types_with_unapplied_updates.First(); it.Good(); it.Inc()) { - if (GetGroupForModelType(it.Get(), session->context()->routing_info()) == - status->group_restriction()) { - server_type_restriction.Put(it.Get()); - } - } - - // Don't process control type updates here. They will be handled elsewhere. - FullModelTypeSet control_types = ToFullModelTypeSet(ControlTypes()); - server_type_restriction.RemoveAll(control_types); - - std::vector<int64> handles; - dir->GetUnappliedUpdateMetaHandles( - &trans, server_type_restriction, &handles); - - // First set of update application passes. - UpdateApplicator applicator( - dir->GetCryptographer(&trans), - session->context()->routing_info(), - status->group_restriction()); - applicator.AttemptApplications(&trans, handles); - status->increment_num_updates_applied_by(applicator.updates_applied()); - status->increment_num_hierarchy_conflicts_by( - applicator.hierarchy_conflicts()); - status->increment_num_encryption_conflicts_by( - applicator.encryption_conflicts()); - - if (applicator.simple_conflict_ids().size() != 0) { - // Resolve the simple conflicts we just detected. - ConflictResolver resolver; - resolver.ResolveConflicts(&trans, - dir->GetCryptographer(&trans), - applicator.simple_conflict_ids(), - status); - - // Conflict resolution sometimes results in more updates to apply. - handles.clear(); - dir->GetUnappliedUpdateMetaHandles( - &trans, server_type_restriction, &handles); - - UpdateApplicator conflict_applicator( - dir->GetCryptographer(&trans), - session->context()->routing_info(), - status->group_restriction()); - conflict_applicator.AttemptApplications(&trans, handles); - - // We count the number of updates from both applicator passes. - status->increment_num_updates_applied_by( - conflict_applicator.updates_applied()); - - // Encryption conflicts should remain unchanged by the resolution of simple - // conflicts. Those can only be solved by updating our nigori key bag. - DCHECK_EQ(conflict_applicator.encryption_conflicts(), - applicator.encryption_conflicts()); - - // Hierarchy conflicts should also remain unchanged, for reasons that are - // more subtle. Hierarchy conflicts exist when the application of a pending - // update from the server would make the local folder hierarchy - // inconsistent. The resolution of simple conflicts could never affect the - // hierarchy conflicting item directly, because hierarchy conflicts are not - // processed by the conflict resolver. It could, in theory, modify the - // local hierarchy on which hierarchy conflict detection depends. However, - // the conflict resolution algorithm currently in use does not allow this. - DCHECK_EQ(conflict_applicator.hierarchy_conflicts(), - applicator.hierarchy_conflicts()); - - // There should be no simple conflicts remaining. We know this because the - // resolver should have resolved all the conflicts we detected last time - // and, by the two previous assertions, that no conflicts have been - // downgraded from encryption or hierarchy down to simple. - DCHECK(conflict_applicator.simple_conflict_ids().empty()); - } - - return SYNCER_OK; -} - -} // namespace syncer diff --git a/sync/engine/apply_updates_and_resolve_conflicts_command.h b/sync/engine/apply_updates_and_resolve_conflicts_command.h deleted file mode 100644 index 5072f80..0000000 --- a/sync/engine/apply_updates_and_resolve_conflicts_command.h +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2012 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. - -#ifndef SYNC_ENGINE_APPLY_UPDATES_AND_RESOLVE_CONFLICTS_COMMAND_H_ -#define SYNC_ENGINE_APPLY_UPDATES_AND_RESOLVE_CONFLICTS_COMMAND_H_ - -#include "base/compiler_specific.h" -#include "sync/base/sync_export.h" -#include "sync/engine/model_changing_syncer_command.h" - -namespace syncer { - -class SYNC_EXPORT_PRIVATE ApplyUpdatesAndResolveConflictsCommand - : public ModelChangingSyncerCommand { - public: - ApplyUpdatesAndResolveConflictsCommand(); - virtual ~ApplyUpdatesAndResolveConflictsCommand(); - - protected: - // ModelChangingSyncerCommand implementation. - virtual std::set<ModelSafeGroup> GetGroupsToChange( - const sessions::SyncSession& session) const OVERRIDE; - virtual SyncerError ModelChangingExecuteImpl( - sessions::SyncSession* session) OVERRIDE; - - private: - DISALLOW_COPY_AND_ASSIGN(ApplyUpdatesAndResolveConflictsCommand); -}; - -} // namespace syncer - -#endif // SYNC_ENGINE_APPLY_UPDATES_AND_RESOLVE_CONFLICTS_COMMAND_H_ diff --git a/sync/engine/apply_updates_and_resolve_conflicts_command_unittest.cc b/sync/engine/apply_updates_and_resolve_conflicts_command_unittest.cc deleted file mode 100644 index d021789..0000000 --- a/sync/engine/apply_updates_and_resolve_conflicts_command_unittest.cc +++ /dev/null @@ -1,415 +0,0 @@ -// Copyright 2012 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 <string> - -#include "base/location.h" -#include "base/memory/scoped_ptr.h" -#include "base/strings/stringprintf.h" -#include "sync/engine/apply_updates_and_resolve_conflicts_command.h" -#include "sync/engine/syncer.h" -#include "sync/internal_api/public/test/test_entry_factory.h" -#include "sync/protocol/bookmark_specifics.pb.h" -#include "sync/protocol/password_specifics.pb.h" -#include "sync/syncable/mutable_entry.h" -#include "sync/syncable/syncable_id.h" -#include "sync/syncable/syncable_read_transaction.h" -#include "sync/syncable/syncable_util.h" -#include "sync/syncable/syncable_write_transaction.h" -#include "sync/test/engine/fake_model_worker.h" -#include "sync/test/engine/syncer_command_test.h" -#include "sync/test/engine/test_id_factory.h" -#include "sync/test/fake_sync_encryption_handler.h" -#include "sync/util/cryptographer.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace syncer { - -using std::string; -using syncable::Id; -using syncable::MutableEntry; -using syncable::UNITTEST; -using syncable::WriteTransaction; - -namespace { -sync_pb::EntitySpecifics DefaultBookmarkSpecifics() { - sync_pb::EntitySpecifics result; - AddDefaultFieldValue(BOOKMARKS, &result); - return result; -} -} // namespace - -// A test fixture for tests exercising ApplyUpdatesAndResolveConflictsCommand. -class ApplyUpdatesAndResolveConflictsCommandTest : public SyncerCommandTest { - public: - protected: - ApplyUpdatesAndResolveConflictsCommandTest() {} - virtual ~ApplyUpdatesAndResolveConflictsCommandTest() {} - - virtual void SetUp() { - workers()->clear(); - mutable_routing_info()->clear(); - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_UI))); - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD))); - (*mutable_routing_info())[BOOKMARKS] = GROUP_UI; - (*mutable_routing_info())[PASSWORDS] = GROUP_PASSWORD; - (*mutable_routing_info())[NIGORI] = GROUP_PASSIVE; - SyncerCommandTest::SetUp(); - entry_factory_.reset(new TestEntryFactory(directory())); - ExpectNoGroupsToChange(apply_updates_command_); - } - - protected: - DISALLOW_COPY_AND_ASSIGN(ApplyUpdatesAndResolveConflictsCommandTest); - - ApplyUpdatesAndResolveConflictsCommand apply_updates_command_; - scoped_ptr<TestEntryFactory> entry_factory_; -}; - -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, Simple) { - string root_server_id = syncable::GetNullId().GetServerId(); - entry_factory_->CreateUnappliedNewBookmarkItemWithParent( - "parent", DefaultBookmarkSpecifics(), root_server_id); - entry_factory_->CreateUnappliedNewBookmarkItemWithParent( - "child", DefaultBookmarkSpecifics(), "parent"); - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(0, status.num_encryption_conflicts()) - << "Simple update shouldn't result in conflicts"; - EXPECT_EQ(0, status.num_hierarchy_conflicts()) - << "Simple update shouldn't result in conflicts"; - EXPECT_EQ(2, status.num_updates_applied()) - << "All items should have been successfully applied"; -} - -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, - UpdateWithChildrenBeforeParents) { - // Set a bunch of updates which are difficult to apply in the order - // they're received due to dependencies on other unseen items. - string root_server_id = syncable::GetNullId().GetServerId(); - entry_factory_->CreateUnappliedNewBookmarkItemWithParent( - "a_child_created_first", DefaultBookmarkSpecifics(), "parent"); - entry_factory_->CreateUnappliedNewBookmarkItemWithParent( - "x_child_created_first", DefaultBookmarkSpecifics(), "parent"); - entry_factory_->CreateUnappliedNewBookmarkItemWithParent( - "parent", DefaultBookmarkSpecifics(), root_server_id); - entry_factory_->CreateUnappliedNewBookmarkItemWithParent( - "a_child_created_second", DefaultBookmarkSpecifics(), "parent"); - entry_factory_->CreateUnappliedNewBookmarkItemWithParent( - "x_child_created_second", DefaultBookmarkSpecifics(), "parent"); - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(5, status.num_updates_applied()) - << "All updates should have been successfully applied"; -} - -// Runs the ApplyUpdatesAndResolveConflictsCommand on an item that has both -// local and remote modifications (IS_UNSYNCED and IS_UNAPPLIED_UPDATE). We -// expect the command to detect that this update can't be applied because it is -// in a CONFLICT state. -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, SimpleConflict) { - entry_factory_->CreateUnappliedAndUnsyncedBookmarkItem("item"); - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(1, status.num_server_overwrites()) - << "Unsynced and unapplied item conflict should be resolved"; - EXPECT_EQ(0, status.num_updates_applied()) - << "Update should not be applied; we should override the server."; -} - -// Runs the ApplyUpdatesAndResolveConflictsCommand on an item that has both -// local and remote modifications *and* the remote modification cannot be -// applied without violating the tree constraints. We expect the command to -// detect that this update can't be applied and that this situation can't be -// resolved with the simple conflict processing logic; it is in a -// CONFLICT_HIERARCHY state. -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, HierarchyAndSimpleConflict) { - // Create a simply-conflicting item. It will start with valid parent ids. - int64 handle = entry_factory_->CreateUnappliedAndUnsyncedBookmarkItem( - "orphaned_by_server"); - { - // Manually set the SERVER_PARENT_ID to bad value. - // A bad parent indicates a hierarchy conflict. - WriteTransaction trans(FROM_HERE, UNITTEST, directory()); - MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); - ASSERT_TRUE(entry.good()); - - entry.PutServerParentId(TestIdFactory::MakeServer("bogus_parent")); - } - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(1, status.num_hierarchy_conflicts()); -} - - -// Runs the ApplyUpdatesAndResolveConflictsCommand on an item with remote -// modifications that would create a directory loop if the update were applied. -// We expect the command to detect that this update can't be applied because it -// is in a CONFLICT_HIERARCHY state. -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, - HierarchyConflictDirectoryLoop) { - // Item 'X' locally has parent of 'root'. Server is updating it to have - // parent of 'Y'. - { - // Create it as a child of root node. - int64 handle = entry_factory_->CreateSyncedItem("X", BOOKMARKS, true); - - WriteTransaction trans(FROM_HERE, UNITTEST, directory()); - MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); - ASSERT_TRUE(entry.good()); - - // Re-parent from root to "Y" - entry.PutServerVersion(entry_factory_->GetNextRevision()); - entry.PutIsUnappliedUpdate(true); - entry.PutServerParentId(TestIdFactory::MakeServer("Y")); - } - - // Item 'Y' is child of 'X'. - entry_factory_->CreateUnsyncedItem( - TestIdFactory::MakeServer("Y"), TestIdFactory::MakeServer("X"), "Y", true, - BOOKMARKS, NULL); - - // If the server's update were applied, we would have X be a child of Y, and Y - // as a child of X. That's a directory loop. The UpdateApplicator should - // prevent the update from being applied and note that this is a hierarchy - // conflict. - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - - // This should count as a hierarchy conflict. - EXPECT_EQ(1, status.num_hierarchy_conflicts()); -} - -// Runs the ApplyUpdatesAndResolveConflictsCommand on a directory where the -// server sent us an update to add a child to a locally deleted (and unsynced) -// parent. We expect the command to not apply the update and to indicate the -// update is in a CONFLICT_HIERARCHY state. -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, - HierarchyConflictDeletedParent) { - // Create a locally deleted parent item. - int64 parent_handle; - entry_factory_->CreateUnsyncedItem( - Id::CreateFromServerId("parent"), TestIdFactory::root(), - "parent", true, BOOKMARKS, &parent_handle); - { - WriteTransaction trans(FROM_HERE, UNITTEST, directory()); - MutableEntry entry(&trans, syncable::GET_BY_HANDLE, parent_handle); - entry.PutIsDel(true); - } - - // Create an incoming child from the server. - entry_factory_->CreateUnappliedNewItemWithParent( - "child", DefaultBookmarkSpecifics(), "parent"); - - // The server's update may seem valid to some other client, but on this client - // that new item's parent no longer exists. The update should not be applied - // and the update applicator should indicate this is a hierarchy conflict. - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(1, status.num_hierarchy_conflicts()); -} - -// Runs the ApplyUpdatesAndResolveConflictsCommand on a directory where the -// server is trying to delete a folder that has a recently added (and unsynced) -// child. We expect the command to not apply the update because it is in a -// CONFLICT_HIERARCHY state. -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, - HierarchyConflictDeleteNonEmptyDirectory) { - // Create a server-deleted directory. - { - // Create it as a child of root node. - int64 handle = entry_factory_->CreateSyncedItem("parent", BOOKMARKS, true); - - WriteTransaction trans(FROM_HERE, UNITTEST, directory()); - MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); - ASSERT_TRUE(entry.good()); - - // Delete it on the server. - entry.PutServerVersion(entry_factory_->GetNextRevision()); - entry.PutIsUnappliedUpdate(true); - entry.PutServerParentId(TestIdFactory::root()); - entry.PutServerIsDel(true); - } - - // Create a local child of the server-deleted directory. - entry_factory_->CreateUnsyncedItem( - TestIdFactory::MakeServer("child"), TestIdFactory::MakeServer("parent"), - "child", false, BOOKMARKS, NULL); - - // The server's request to delete the directory must be ignored, otherwise our - // unsynced new child would be orphaned. This is a hierarchy conflict. - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - // This should count as a hierarchy conflict. - EXPECT_EQ(1, status.num_hierarchy_conflicts()); -} - -// Runs the ApplyUpdatesAndResolveConflictsCommand on a server-created item that -// has a locally unknown parent. We expect the command to not apply the update -// because the item is in a CONFLICT_HIERARCHY state. -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, - HierarchyConflictUnknownParent) { - // We shouldn't be able to do anything with either of these items. - entry_factory_->CreateUnappliedNewItemWithParent( - "some_item", DefaultBookmarkSpecifics(), "unknown_parent"); - entry_factory_->CreateUnappliedNewItemWithParent( - "some_other_item", DefaultBookmarkSpecifics(), "some_item"); - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(2, status.num_hierarchy_conflicts()) - << "All updates with an unknown ancestors should be in conflict"; - EXPECT_EQ(0, status.num_updates_applied()) - << "No item with an unknown ancestor should be applied"; -} - -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, ItemsBothKnownAndUnknown) { - // See what happens when there's a mixture of good and bad updates. - string root_server_id = syncable::GetNullId().GetServerId(); - entry_factory_->CreateUnappliedNewItemWithParent( - "first_unknown_item", DefaultBookmarkSpecifics(), "unknown_parent"); - entry_factory_->CreateUnappliedNewItemWithParent( - "first_known_item", DefaultBookmarkSpecifics(), root_server_id); - entry_factory_->CreateUnappliedNewItemWithParent( - "second_unknown_item", DefaultBookmarkSpecifics(), "unknown_parent"); - entry_factory_->CreateUnappliedNewItemWithParent( - "second_known_item", DefaultBookmarkSpecifics(), "first_known_item"); - entry_factory_->CreateUnappliedNewItemWithParent( - "third_known_item", DefaultBookmarkSpecifics(), "fourth_known_item"); - entry_factory_->CreateUnappliedNewItemWithParent( - "fourth_known_item", DefaultBookmarkSpecifics(), root_server_id); - - ExpectGroupToChange(apply_updates_command_, GROUP_UI); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(2, status.num_hierarchy_conflicts()) - << "The updates with unknown ancestors should be in conflict"; - EXPECT_EQ(4, status.num_updates_applied()) - << "The updates with known ancestors should be successfully applied"; -} - -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, DecryptablePassword) { - // Decryptable password updates should be applied. - Cryptographer* cryptographer; - { - // Storing the cryptographer separately is bad, but for this test we - // know it's safe. - syncable::ReadTransaction trans(FROM_HERE, directory()); - cryptographer = directory()->GetCryptographer(&trans); - } - - KeyParams params = {"localhost", "dummy", "foobar"}; - cryptographer->AddKey(params); - - sync_pb::EntitySpecifics specifics; - sync_pb::PasswordSpecificsData data; - data.set_origin("http://example.com"); - - cryptographer->Encrypt(data, - specifics.mutable_password()->mutable_encrypted()); - entry_factory_->CreateUnappliedNewItem("item", specifics, false); - - ExpectGroupToChange(apply_updates_command_, GROUP_PASSWORD); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(1, status.num_updates_applied()) - << "The updates that can be decrypted should be applied"; -} - -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, UndecryptableData) { - // Undecryptable updates should not be applied. - sync_pb::EntitySpecifics encrypted_bookmark; - encrypted_bookmark.mutable_encrypted(); - AddDefaultFieldValue(BOOKMARKS, &encrypted_bookmark); - string root_server_id = syncable::GetNullId().GetServerId(); - entry_factory_->CreateUnappliedNewItemWithParent( - "folder", encrypted_bookmark, root_server_id); - entry_factory_->CreateUnappliedNewItem("item2", encrypted_bookmark, false); - sync_pb::EntitySpecifics encrypted_password; - encrypted_password.mutable_password(); - entry_factory_->CreateUnappliedNewItem("item3", encrypted_password, false); - - ExpectGroupsToChange(apply_updates_command_, GROUP_UI, GROUP_PASSWORD); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(3, status.num_encryption_conflicts()) - << "Updates that can't be decrypted should be in encryption conflict"; - EXPECT_EQ(0, status.num_updates_applied()) - << "No update that can't be decrypted should be applied"; -} - -TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, SomeUndecryptablePassword) { - Cryptographer* cryptographer; - // Only decryptable password updates should be applied. - { - sync_pb::EntitySpecifics specifics; - sync_pb::PasswordSpecificsData data; - data.set_origin("http://example.com/1"); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - cryptographer = directory()->GetCryptographer(&trans); - - KeyParams params = {"localhost", "dummy", "foobar"}; - cryptographer->AddKey(params); - - cryptographer->Encrypt(data, - specifics.mutable_password()->mutable_encrypted()); - } - entry_factory_->CreateUnappliedNewItem("item1", specifics, false); - } - { - // Create a new cryptographer, independent of the one in the session. - Cryptographer other_cryptographer(cryptographer->encryptor()); - KeyParams params = {"localhost", "dummy", "bazqux"}; - other_cryptographer.AddKey(params); - - sync_pb::EntitySpecifics specifics; - sync_pb::PasswordSpecificsData data; - data.set_origin("http://example.com/2"); - - other_cryptographer.Encrypt(data, - specifics.mutable_password()->mutable_encrypted()); - entry_factory_->CreateUnappliedNewItem("item2", specifics, false); - } - - ExpectGroupToChange(apply_updates_command_, GROUP_PASSWORD); - apply_updates_command_.ExecuteImpl(session()); - - const sessions::StatusController& status = session()->status_controller(); - EXPECT_EQ(1, status.num_encryption_conflicts()) - << "The updates that can't be decrypted should be in encryption " - << "conflict"; - EXPECT_EQ(1, status.num_updates_applied()) - << "The undecryptable password update shouldn't be applied"; -} - -} // namespace syncer diff --git a/sync/engine/download_unittest.cc b/sync/engine/download_unittest.cc index 9336893..8fd08d1 100644 --- a/sync/engine/download_unittest.cc +++ b/sync/engine/download_unittest.cc @@ -13,6 +13,7 @@ #include "sync/sessions/nudge_tracker.h" #include "sync/sessions/status_controller.h" #include "sync/syncable/directory.h" +#include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/test_directory_setter_upper.h" #include "sync/test/sessions/mock_debug_info_getter.h" #include "testing/gtest/include/gtest/gtest.h" @@ -31,9 +32,9 @@ class DownloadUpdatesTest : public ::testing::Test { virtual void SetUp() { dir_maker_.SetUp(); - AddUpdateHandler(AUTOFILL); - AddUpdateHandler(BOOKMARKS); - AddUpdateHandler(PREFERENCES); + AddUpdateHandler(AUTOFILL, GROUP_DB); + AddUpdateHandler(BOOKMARKS, GROUP_UI); + AddUpdateHandler(PREFERENCES, GROUP_UI); } virtual void TearDown() { @@ -58,11 +59,12 @@ class DownloadUpdatesTest : public ::testing::Test { } private: - void AddUpdateHandler(ModelType type) { + void AddUpdateHandler(ModelType type, ModelSafeGroup group) { DCHECK(directory()); - update_handler_map_.insert( - std::make_pair(type, - new SyncDirectoryUpdateHandler(directory(), type))); + scoped_refptr<ModelSafeWorker> worker = new FakeModelWorker(group); + SyncDirectoryUpdateHandler* handler = + new SyncDirectoryUpdateHandler(directory(), type, worker); + update_handler_map_.insert(std::make_pair(type, handler)); } base::MessageLoop loop_; // Needed for directory init. diff --git a/sync/engine/get_commit_ids.cc b/sync/engine/get_commit_ids.cc index 3a87383..42faf21 100644 --- a/sync/engine/get_commit_ids.cc +++ b/sync/engine/get_commit_ids.cc @@ -73,9 +73,8 @@ void GetCommitIdsForType( // We filter out all unready entries from the set of unsynced handles. This // new set of ready and unsynced items is then what we use to determine what - // is a candidate for commit. The caller of this SyncerCommand is responsible - // for ensuring that no throttled types are included among the - // requested_types. + // is a candidate for commit. The caller is responsible for ensuring that no + // throttled types are included among the requested_types. FilterUnreadyEntries(trans, ModelTypeSet(type), encrypted_types, diff --git a/sync/engine/model_changing_syncer_command.cc b/sync/engine/model_changing_syncer_command.cc deleted file mode 100644 index a793626..0000000 --- a/sync/engine/model_changing_syncer_command.cc +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright (c) 2012 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 "sync/engine/model_changing_syncer_command.h" - -#include "base/basictypes.h" -#include "base/bind.h" -#include "base/bind_helpers.h" -#include "sync/sessions/status_controller.h" -#include "sync/sessions/sync_session.h" - -namespace syncer { - -SyncerError ModelChangingSyncerCommand::ExecuteImpl( - sessions::SyncSession* session) { - work_session_ = session; - SyncerError result = SYNCER_OK; - - const std::set<ModelSafeGroup>& groups_to_change = - GetGroupsToChange(*work_session_); - for (size_t i = 0; i < session->context()->workers().size(); ++i) { - ModelSafeWorker* worker = session->context()->workers()[i].get(); - ModelSafeGroup group = worker->GetModelSafeGroup(); - // Skip workers whose group isn't active. - if (groups_to_change.count(group) == 0u) { - DVLOG(2) << "Skipping worker for group " - << ModelSafeGroupToString(group); - continue; - } - - sessions::StatusController* status = - work_session_->mutable_status_controller(); - sessions::ScopedModelSafeGroupRestriction r(status, group); - WorkCallback c = base::Bind( - &ModelChangingSyncerCommand::StartChangingModel, - // We wait until the callback is executed. So it is safe to use - // unretained. - base::Unretained(this)); - - SyncerError this_worker_result = worker->DoWorkAndWaitUntilDone(c); - // TODO(rlarocque): Figure out a better way to deal with errors from - // multiple models at once. See also: crbug.com/109422. - if (this_worker_result != SYNCER_OK) - result = this_worker_result; - } - - return result; -} - -} // namespace syncer diff --git a/sync/engine/model_changing_syncer_command.h b/sync/engine/model_changing_syncer_command.h deleted file mode 100644 index f9ffe37..0000000 --- a/sync/engine/model_changing_syncer_command.h +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright 2012 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. - -#ifndef SYNC_ENGINE_MODEL_CHANGING_SYNCER_COMMAND_H_ -#define SYNC_ENGINE_MODEL_CHANGING_SYNCER_COMMAND_H_ - -#include "base/compiler_specific.h" -#include "sync/base/sync_export.h" -#include "sync/engine/syncer_command.h" -#include "sync/internal_api/public/engine/model_safe_worker.h" - -namespace syncer { -namespace sessions { -class SyncSession; -} - -// An abstract SyncerCommand which dispatches its Execute step to the -// model-safe worker thread. Classes derived from ModelChangingSyncerCommand -// instead of SyncerCommand must implement ModelChangingExecuteImpl instead of -// ExecuteImpl, but otherwise, the contract is the same. -// -// A command should derive from ModelChangingSyncerCommand instead of -// SyncerCommand whenever the operation might change any client-visible -// fields on any syncable::Entry. If the operation involves creating a -// WriteTransaction, this is a sign that ModelChangingSyncerCommand is likely -// necessary. -class SYNC_EXPORT_PRIVATE ModelChangingSyncerCommand : public SyncerCommand { - public: - ModelChangingSyncerCommand() : work_session_(NULL) { } - virtual ~ModelChangingSyncerCommand() { } - - // SyncerCommand implementation. Sets work_session to session. - virtual SyncerError ExecuteImpl(sessions::SyncSession* session) OVERRIDE; - - // Wrapper so implementations don't worry about storing work_session. - SyncerError StartChangingModel() { - return ModelChangingExecuteImpl(work_session_); - } - - std::set<ModelSafeGroup> GetGroupsToChangeForTest( - const sessions::SyncSession& session) const { - return GetGroupsToChange(session); - } - - protected: - // This should return the set of groups in |session| that need to be - // changed. The returned set should be a subset of - // session.GetEnabledGroups(). Subclasses can guarantee this either - // by calling one of the session.GetEnabledGroups*() functions and - // filtering that, or using GetGroupForModelType() (which handles - // top-level/unspecified nodes) to project from model types to - // groups. - virtual std::set<ModelSafeGroup> GetGroupsToChange( - const sessions::SyncSession& session) const = 0; - - // Abstract method to be implemented by subclasses to handle logic that - // operates on the model. This is invoked with a SyncSession ModelSafeGroup - // restriction in place so that bits of state belonging to data types - // running on an unsafe thread are siloed away. - virtual SyncerError ModelChangingExecuteImpl( - sessions::SyncSession* session) = 0; - - private: - // ExecuteImpl is expected to be run by SyncerCommand to set work_session. - // StartChangingModel is called to start this command running. - // Implementations will implement ModelChangingExecuteImpl and not - // worry about storing the session or setting it. They are given work_session. - sessions::SyncSession* work_session_; - - DISALLOW_COPY_AND_ASSIGN(ModelChangingSyncerCommand); -}; - -} // namespace syncer - -#endif // SYNC_ENGINE_MODEL_CHANGING_SYNCER_COMMAND_H_ diff --git a/sync/engine/model_changing_syncer_command_unittest.cc b/sync/engine/model_changing_syncer_command_unittest.cc deleted file mode 100644 index 71345ef..0000000 --- a/sync/engine/model_changing_syncer_command_unittest.cc +++ /dev/null @@ -1,88 +0,0 @@ -// Copyright (c) 2012 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/basictypes.h" -#include "base/compiler_specific.h" -#include "base/memory/ref_counted.h" -#include "sync/engine/model_changing_syncer_command.h" -#include "sync/internal_api/public/base/model_type.h" -#include "sync/sessions/sync_session.h" -#include "sync/test/engine/fake_model_worker.h" -#include "sync/test/engine/syncer_command_test.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace syncer { - -namespace { - -class FakeModelChangingSyncerCommand : public ModelChangingSyncerCommand { - public: - FakeModelChangingSyncerCommand() {} - virtual ~FakeModelChangingSyncerCommand() {} - - const std::set<ModelSafeGroup>& changed_groups() const { - return changed_groups_; - } - - protected: - virtual std::set<ModelSafeGroup> GetGroupsToChange( - const sessions::SyncSession& session) const OVERRIDE { - // This command doesn't actually make changes, so the empty set might be an - // appropriate response. That would not be very interesting, so instead we - // return the set of groups with active types. - std::set<ModelSafeGroup> enabled_groups; - const ModelSafeRoutingInfo& routing_info = - session.context()->routing_info(); - for (ModelSafeRoutingInfo::const_iterator it = routing_info.begin(); - it != routing_info.end(); ++it) { - enabled_groups.insert(it->second); - } - enabled_groups.insert(GROUP_PASSIVE); - return enabled_groups; - } - - virtual SyncerError ModelChangingExecuteImpl( - sessions::SyncSession* session) OVERRIDE { - changed_groups_.insert(session->status_controller().group_restriction()); - return SYNCER_OK; - } - - private: - std::set<ModelSafeGroup> changed_groups_; - - DISALLOW_COPY_AND_ASSIGN(FakeModelChangingSyncerCommand); -}; - -class ModelChangingSyncerCommandTest : public SyncerCommandTest { - protected: - ModelChangingSyncerCommandTest() {} - virtual ~ModelChangingSyncerCommandTest() {} - - virtual void SetUp() { - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_UI))); - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD))); - (*mutable_routing_info())[BOOKMARKS] = GROUP_UI; - (*mutable_routing_info())[PASSWORDS] = GROUP_PASSWORD; - SyncerCommandTest::SetUp(); - } - - FakeModelChangingSyncerCommand command_; - - private: - DISALLOW_COPY_AND_ASSIGN(ModelChangingSyncerCommandTest); -}; - -TEST_F(ModelChangingSyncerCommandTest, Basic) { - ExpectGroupsToChange(command_, GROUP_UI, GROUP_PASSWORD, GROUP_PASSIVE); - EXPECT_TRUE(command_.changed_groups().empty()); - command_.ExecuteImpl(session()); - EXPECT_EQ(command_.GetGroupsToChangeForTest(*session()), - command_.changed_groups()); -} - -} // namespace - -} // namespace syncer diff --git a/sync/engine/sync_directory_update_handler.cc b/sync/engine/sync_directory_update_handler.cc index 0d7953b..1a9bd1e 100644 --- a/sync/engine/sync_directory_update_handler.cc +++ b/sync/engine/sync_directory_update_handler.cc @@ -4,19 +4,25 @@ #include "sync/engine/sync_directory_update_handler.h" +#include "sync/engine/conflict_resolver.h" #include "sync/engine/process_updates_util.h" +#include "sync/engine/update_applicator.h" #include "sync/sessions/status_controller.h" #include "sync/syncable/directory.h" #include "sync/syncable/syncable_model_neutral_write_transaction.h" +#include "sync/syncable/syncable_write_transaction.h" namespace syncer { using syncable::SYNCER; SyncDirectoryUpdateHandler::SyncDirectoryUpdateHandler( - syncable::Directory* dir, ModelType type) + syncable::Directory* dir, + ModelType type, + scoped_refptr<ModelSafeWorker> worker) : dir_(dir), - type_(type) {} + type_(type), + worker_(worker) {} SyncDirectoryUpdateHandler::~SyncDirectoryUpdateHandler() {} @@ -34,6 +40,91 @@ void SyncDirectoryUpdateHandler::ProcessGetUpdatesResponse( UpdateProgressMarker(progress_marker); } +void SyncDirectoryUpdateHandler::ApplyUpdates( + sessions::StatusController* status) { + if (IsControlType(type_)) { + return; // We don't process control types here. + } + + if (!dir_->TypeHasUnappliedUpdates(type_)) { + return; // No work to do. Skip this type. + } + + WorkCallback c = base::Bind( + &SyncDirectoryUpdateHandler::ApplyUpdatesImpl, + // We wait until the callback is executed. We can safely use Unretained. + base::Unretained(this), + base::Unretained(status)); + worker_->DoWorkAndWaitUntilDone(c); +} + +SyncerError SyncDirectoryUpdateHandler::ApplyUpdatesImpl( + sessions::StatusController* status) { + syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir_); + + std::vector<int64> handles; + dir_->GetUnappliedUpdateMetaHandles( + &trans, + FullModelTypeSet(type_), + &handles); + + // First set of update application passes. + UpdateApplicator applicator(dir_->GetCryptographer(&trans)); + applicator.AttemptApplications(&trans, handles); + status->increment_num_updates_applied_by(applicator.updates_applied()); + status->increment_num_hierarchy_conflicts_by( + applicator.hierarchy_conflicts()); + status->increment_num_encryption_conflicts_by( + applicator.encryption_conflicts()); + + if (applicator.simple_conflict_ids().size() != 0) { + // Resolve the simple conflicts we just detected. + ConflictResolver resolver; + resolver.ResolveConflicts(&trans, + dir_->GetCryptographer(&trans), + applicator.simple_conflict_ids(), + status); + + // Conflict resolution sometimes results in more updates to apply. + handles.clear(); + dir_->GetUnappliedUpdateMetaHandles( + &trans, + FullModelTypeSet(type_), + &handles); + + UpdateApplicator conflict_applicator(dir_->GetCryptographer(&trans)); + conflict_applicator.AttemptApplications(&trans, handles); + + // We count the number of updates from both applicator passes. + status->increment_num_updates_applied_by( + conflict_applicator.updates_applied()); + + // Encryption conflicts should remain unchanged by the resolution of simple + // conflicts. Those can only be solved by updating our nigori key bag. + DCHECK_EQ(conflict_applicator.encryption_conflicts(), + applicator.encryption_conflicts()); + + // Hierarchy conflicts should also remain unchanged, for reasons that are + // more subtle. Hierarchy conflicts exist when the application of a pending + // update from the server would make the local folder hierarchy + // inconsistent. The resolution of simple conflicts could never affect the + // hierarchy conflicting item directly, because hierarchy conflicts are not + // processed by the conflict resolver. It could, in theory, modify the + // local hierarchy on which hierarchy conflict detection depends. However, + // the conflict resolution algorithm currently in use does not allow this. + DCHECK_EQ(conflict_applicator.hierarchy_conflicts(), + applicator.hierarchy_conflicts()); + + // There should be no simple conflicts remaining. We know this because the + // resolver should have resolved all the conflicts we detected last time + // and, by the two previous assertions, that no conflicts have been + // downgraded from encryption or hierarchy down to simple. + DCHECK(conflict_applicator.simple_conflict_ids().empty()); + } + + return SYNCER_OK; +} + void SyncDirectoryUpdateHandler::UpdateSyncEntities( syncable::ModelNeutralWriteTransaction* trans, const SyncEntityList& applicable_updates, diff --git a/sync/engine/sync_directory_update_handler.h b/sync/engine/sync_directory_update_handler.h index 38256a4..ea4d7914 100644 --- a/sync/engine/sync_directory_update_handler.h +++ b/sync/engine/sync_directory_update_handler.h @@ -8,9 +8,11 @@ #include <map> #include "base/basictypes.h" +#include "base/memory/ref_counted.h" #include "sync/base/sync_export.h" #include "sync/engine/process_updates_util.h" #include "sync/internal_api/public/base/model_type.h" +#include "sync/internal_api/public/util/syncer_error.h" namespace sync_pb { class DataTypeProgressMarker; @@ -27,6 +29,8 @@ namespace syncable { class Directory; } +class ModelSafeWorker; + // This class represents the syncable::Directory's processes for requesting and // processing updates from the sync server. // @@ -35,7 +39,9 @@ class Directory; // It can also process a set of received SyncEntities and store their data. class SYNC_EXPORT_PRIVATE SyncDirectoryUpdateHandler { public: - SyncDirectoryUpdateHandler(syncable::Directory* dir, ModelType type); + SyncDirectoryUpdateHandler(syncable::Directory* dir, + ModelType type, + scoped_refptr<ModelSafeWorker> worker); ~SyncDirectoryUpdateHandler(); // Fills the given parameter with the stored progress marker for this type. @@ -53,8 +59,13 @@ class SYNC_EXPORT_PRIVATE SyncDirectoryUpdateHandler { const SyncEntityList& applicable_updates, sessions::StatusController* status); + // If there are updates to apply, apply them on the proper thread. + // Delegates to ApplyUpdatesImpl(). + void ApplyUpdates(sessions::StatusController* status); + private: - friend class SyncDirectoryUpdateHandlerTest; + friend class SyncDirectoryUpdateHandlerApplyUpdateTest; + friend class SyncDirectoryUpdateHandlerProcessUpdateTest; // Processes the given SyncEntities and stores their data in the directory. // Their types must match this update handler's type. @@ -68,8 +79,12 @@ class SYNC_EXPORT_PRIVATE SyncDirectoryUpdateHandler { void UpdateProgressMarker( const sync_pb::DataTypeProgressMarker& progress_marker); + // Skips all checks and goes straight to applying the updates. + SyncerError ApplyUpdatesImpl(sessions::StatusController* status); + syncable::Directory* dir_; ModelType type_; + scoped_refptr<ModelSafeWorker> worker_; DISALLOW_COPY_AND_ASSIGN(SyncDirectoryUpdateHandler); }; diff --git a/sync/engine/sync_directory_update_handler_unittest.cc b/sync/engine/sync_directory_update_handler_unittest.cc index 53589b8..86d447e 100644 --- a/sync/engine/sync_directory_update_handler_unittest.cc +++ b/sync/engine/sync_directory_update_handler_unittest.cc @@ -6,16 +6,22 @@ #include "base/compiler_specific.h" #include "base/message_loop/message_loop.h" +#include "base/stl_util.h" #include "sync/engine/syncer_proto_util.h" #include "sync/internal_api/public/base/model_type.h" +#include "sync/internal_api/public/test/test_entry_factory.h" #include "sync/protocol/sync.pb.h" #include "sync/sessions/status_controller.h" #include "sync/syncable/directory.h" #include "sync/syncable/entry.h" +#include "sync/syncable/mutable_entry.h" #include "sync/syncable/syncable_model_neutral_write_transaction.h" #include "sync/syncable/syncable_proto_util.h" #include "sync/syncable/syncable_read_transaction.h" +#include "sync/syncable/syncable_write_transaction.h" +#include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/test_directory_setter_upper.h" +#include "sync/test/engine/test_id_factory.h" #include "sync/test/engine/test_syncable_utils.h" #include "testing/gtest/include/gtest/gtest.h" @@ -23,8 +29,19 @@ namespace syncer { using syncable::UNITTEST; -class SyncDirectoryUpdateHandlerTest : public ::testing::Test { +// A test harness for tests that focus on processing updates. +// +// Update processing is what occurs when we first download updates. It converts +// the received protobuf message into information in the syncable::Directory. +// Any invalid or redundant updates will be dropped at this point. +class SyncDirectoryUpdateHandlerProcessUpdateTest : public ::testing::Test { public: + SyncDirectoryUpdateHandlerProcessUpdateTest() + : ui_worker_(new FakeModelWorker(GROUP_UI)) { + } + + virtual ~SyncDirectoryUpdateHandlerProcessUpdateTest() {} + virtual void SetUp() OVERRIDE { dir_maker_.SetUp(); } @@ -54,12 +71,18 @@ class SyncDirectoryUpdateHandlerTest : public ::testing::Test { SyncDirectoryUpdateHandler* handler, const sync_pb::DataTypeProgressMarker& progress); + scoped_refptr<FakeModelWorker> ui_worker() { + return ui_worker_; + } + private: base::MessageLoop loop_; // Needed to initialize the directory. TestDirectorySetterUpper dir_maker_; + scoped_refptr<FakeModelWorker> ui_worker_; }; -scoped_ptr<sync_pb::SyncEntity> SyncDirectoryUpdateHandlerTest::CreateUpdate( +scoped_ptr<sync_pb::SyncEntity> +SyncDirectoryUpdateHandlerProcessUpdateTest::CreateUpdate( const std::string& id, const std::string& parent, const ModelType& type) { @@ -73,7 +96,7 @@ scoped_ptr<sync_pb::SyncEntity> SyncDirectoryUpdateHandlerTest::CreateUpdate( return e.Pass(); } -void SyncDirectoryUpdateHandlerTest::UpdateSyncEntities( +void SyncDirectoryUpdateHandlerProcessUpdateTest::UpdateSyncEntities( SyncDirectoryUpdateHandler* handler, const SyncEntityList& applicable_updates, sessions::StatusController* status) { @@ -81,7 +104,7 @@ void SyncDirectoryUpdateHandlerTest::UpdateSyncEntities( handler->UpdateSyncEntities(&trans, applicable_updates, status); } -void SyncDirectoryUpdateHandlerTest::UpdateProgressMarkers( +void SyncDirectoryUpdateHandlerProcessUpdateTest::UpdateProgressMarkers( SyncDirectoryUpdateHandler* handler, const sync_pb::DataTypeProgressMarker& progress) { handler->UpdateProgressMarker(progress); @@ -90,8 +113,8 @@ void SyncDirectoryUpdateHandlerTest::UpdateProgressMarkers( static const char kCacheGuid[] = "IrcjZ2jyzHDV9Io4+zKcXQ=="; // Test that the bookmark tag is set on newly downloaded items. -TEST_F(SyncDirectoryUpdateHandlerTest, NewBookmarkTag) { - SyncDirectoryUpdateHandler handler(dir(), BOOKMARKS); +TEST_F(SyncDirectoryUpdateHandlerProcessUpdateTest, NewBookmarkTag) { + SyncDirectoryUpdateHandler handler(dir(), BOOKMARKS, ui_worker()); sync_pb::GetUpdatesResponse gu_response; sessions::StatusController status; @@ -127,8 +150,9 @@ TEST_F(SyncDirectoryUpdateHandlerTest, NewBookmarkTag) { } // Test the receipt of a type root node. -TEST_F(SyncDirectoryUpdateHandlerTest, ReceiveServerCreatedBookmarkFolders) { - SyncDirectoryUpdateHandler handler(dir(), BOOKMARKS); +TEST_F(SyncDirectoryUpdateHandlerProcessUpdateTest, + ReceiveServerCreatedBookmarkFolders) { + SyncDirectoryUpdateHandler handler(dir(), BOOKMARKS, ui_worker()); sync_pb::GetUpdatesResponse gu_response; sessions::StatusController status; @@ -161,15 +185,15 @@ TEST_F(SyncDirectoryUpdateHandlerTest, ReceiveServerCreatedBookmarkFolders) { } // Test the receipt of a non-bookmark item. -TEST_F(SyncDirectoryUpdateHandlerTest, ReceiveNonBookmarkItem) { - SyncDirectoryUpdateHandler handler(dir(), AUTOFILL); +TEST_F(SyncDirectoryUpdateHandlerProcessUpdateTest, ReceiveNonBookmarkItem) { + SyncDirectoryUpdateHandler handler(dir(), PREFERENCES, ui_worker()); sync_pb::GetUpdatesResponse gu_response; sessions::StatusController status; std::string root = syncable::GetNullId().GetServerId(); syncable::Id server_id = syncable::Id::CreateFromServerId("xyz"); scoped_ptr<sync_pb::SyncEntity> e = - CreateUpdate(SyncableIdToProto(server_id), root, AUTOFILL); + CreateUpdate(SyncableIdToProto(server_id), root, PREFERENCES); e->set_server_defined_unique_tag("9PGRuKdX5sHyGMB17CvYTXuC43I="); // Add it to the applicable updates list. @@ -192,8 +216,8 @@ TEST_F(SyncDirectoryUpdateHandlerTest, ReceiveNonBookmarkItem) { } // Tests the setting of progress markers. -TEST_F(SyncDirectoryUpdateHandlerTest, ProcessNewProgressMarkers) { - SyncDirectoryUpdateHandler handler(dir(), BOOKMARKS); +TEST_F(SyncDirectoryUpdateHandlerProcessUpdateTest, ProcessNewProgressMarkers) { + SyncDirectoryUpdateHandler handler(dir(), BOOKMARKS, ui_worker()); sync_pb::DataTypeProgressMarker progress; progress.set_data_type_id(GetSpecificsFieldNumberFromModelType(BOOKMARKS)); @@ -208,4 +232,595 @@ TEST_F(SyncDirectoryUpdateHandlerTest, ProcessNewProgressMarkers) { EXPECT_EQ(progress.data_type_id(), saved.data_type_id()); } +// A test harness for tests that focus on applying updates. +// +// Update application is performed when we want to take updates that were +// previously downloaded, processed, and stored in our syncable::Directory +// and use them to update our local state (both the Directory's local state +// and the model's local state, though these tests focus only on the Directory's +// local state). +// +// This is kept separate from the update processing test in part for historical +// reasons, and in part because these tests may require a bit more infrastrcture +// in the future. Update application should happen on a different thread a lot +// of the time so these tests may end up requiring more infrastructure than the +// update processing tests. Currently, we're bypassing most of those issues by +// using FakeModelWorkers, so there's not much difference between the two test +// harnesses. +class SyncDirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test { + public: + SyncDirectoryUpdateHandlerApplyUpdateTest() + : ui_worker_(new FakeModelWorker(GROUP_UI)), + password_worker_(new FakeModelWorker(GROUP_PASSWORD)), + passive_worker_(new FakeModelWorker(GROUP_PASSIVE)), + update_handler_map_deleter_(&update_handler_map_) {} + + virtual void SetUp() OVERRIDE { + dir_maker_.SetUp(); + entry_factory_.reset(new TestEntryFactory(directory())); + + update_handler_map_.insert(std::make_pair( + BOOKMARKS, + new SyncDirectoryUpdateHandler(directory(), BOOKMARKS, ui_worker_))); + update_handler_map_.insert(std::make_pair( + PASSWORDS, + new SyncDirectoryUpdateHandler(directory(), + PASSWORDS, + password_worker_))); + } + + virtual void TearDown() OVERRIDE { + dir_maker_.TearDown(); + } + + protected: + void ApplyBookmarkUpdates(sessions::StatusController* status) { + update_handler_map_[BOOKMARKS]->ApplyUpdates(status); + } + + void ApplyPasswordUpdates(sessions::StatusController* status) { + update_handler_map_[PASSWORDS]->ApplyUpdates(status); + } + + TestEntryFactory* entry_factory() { + return entry_factory_.get(); + } + + syncable::Directory* directory() { + return dir_maker_.directory(); + } + + private: + base::MessageLoop loop_; // Needed to initialize the directory. + TestDirectorySetterUpper dir_maker_; + scoped_ptr<TestEntryFactory> entry_factory_; + + scoped_refptr<FakeModelWorker> ui_worker_; + scoped_refptr<FakeModelWorker> password_worker_; + scoped_refptr<FakeModelWorker> passive_worker_; + + UpdateHandlerMap update_handler_map_; + STLValueDeleter<UpdateHandlerMap> update_handler_map_deleter_; +}; + +namespace { +sync_pb::EntitySpecifics DefaultBookmarkSpecifics() { + sync_pb::EntitySpecifics result; + AddDefaultFieldValue(BOOKMARKS, &result); + return result; +} +} // namespace + +// Test update application for a few bookmark items. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, SimpleBookmark) { + sessions::StatusController status; + + std::string root_server_id = syncable::GetNullId().GetServerId(); + int64 parent_handle = + entry_factory()->CreateUnappliedNewBookmarkItemWithParent( + "parent", DefaultBookmarkSpecifics(), root_server_id); + int64 child_handle = + entry_factory()->CreateUnappliedNewBookmarkItemWithParent( + "child", DefaultBookmarkSpecifics(), "parent"); + + ApplyBookmarkUpdates(&status); + + EXPECT_EQ(0, status.num_encryption_conflicts()) + << "Simple update shouldn't result in conflicts"; + EXPECT_EQ(0, status.num_hierarchy_conflicts()) + << "Simple update shouldn't result in conflicts"; + EXPECT_EQ(2, status.num_updates_applied()) + << "All items should have been successfully applied"; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + + syncable::Entry parent(&trans, syncable::GET_BY_HANDLE, parent_handle); + syncable::Entry child(&trans, syncable::GET_BY_HANDLE, child_handle); + + ASSERT_TRUE(parent.good()); + ASSERT_TRUE(child.good()); + + EXPECT_FALSE(parent.GetIsUnsynced()); + EXPECT_FALSE(parent.GetIsUnappliedUpdate()); + EXPECT_FALSE(child.GetIsUnsynced()); + EXPECT_FALSE(child.GetIsUnappliedUpdate()); + } +} + +// Test that the applicator can handle updates delivered out of order. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, + BookmarkChildrenBeforeParent) { + // Start with some bookmarks whose parents are unknown. + std::string root_server_id = syncable::GetNullId().GetServerId(); + int64 a_handle = entry_factory()->CreateUnappliedNewBookmarkItemWithParent( + "a_child_created_first", DefaultBookmarkSpecifics(), "parent"); + int64 x_handle = entry_factory()->CreateUnappliedNewBookmarkItemWithParent( + "x_child_created_first", DefaultBookmarkSpecifics(), "parent"); + + // Update application will fail. + sessions::StatusController status1; + ApplyBookmarkUpdates(&status1); + EXPECT_EQ(0, status1.num_updates_applied()); + EXPECT_EQ(2, status1.num_hierarchy_conflicts()); + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + + syncable::Entry a(&trans, syncable::GET_BY_HANDLE, a_handle); + syncable::Entry x(&trans, syncable::GET_BY_HANDLE, x_handle); + + ASSERT_TRUE(a.good()); + ASSERT_TRUE(x.good()); + + EXPECT_TRUE(a.GetIsUnappliedUpdate()); + EXPECT_TRUE(x.GetIsUnappliedUpdate()); + } + + // Now add their parent and a few siblings. + entry_factory()->CreateUnappliedNewBookmarkItemWithParent( + "parent", DefaultBookmarkSpecifics(), root_server_id); + entry_factory()->CreateUnappliedNewBookmarkItemWithParent( + "a_child_created_second", DefaultBookmarkSpecifics(), "parent"); + entry_factory()->CreateUnappliedNewBookmarkItemWithParent( + "x_child_created_second", DefaultBookmarkSpecifics(), "parent"); + + // Update application will succeed. + sessions::StatusController status2; + ApplyBookmarkUpdates(&status2); + EXPECT_EQ(5, status2.num_updates_applied()) + << "All updates should have been successfully applied"; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + + syncable::Entry a(&trans, syncable::GET_BY_HANDLE, a_handle); + syncable::Entry x(&trans, syncable::GET_BY_HANDLE, x_handle); + + ASSERT_TRUE(a.good()); + ASSERT_TRUE(x.good()); + + EXPECT_FALSE(a.GetIsUnappliedUpdate()); + EXPECT_FALSE(x.GetIsUnappliedUpdate()); + } +} + +// Try to apply changes on an item that is both IS_UNSYNCED and +// IS_UNAPPLIED_UPDATE. Conflict resolution should be performed. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, SimpleBookmarkConflict) { + int64 handle = entry_factory()->CreateUnappliedAndUnsyncedBookmarkItem("x"); + + int original_server_version = -10; + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry e(&trans, syncable::GET_BY_HANDLE, handle); + original_server_version = e.GetServerVersion(); + ASSERT_NE(original_server_version, e.GetBaseVersion()); + EXPECT_TRUE(e.GetIsUnsynced()); + } + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + EXPECT_EQ(1, status.num_server_overwrites()) + << "Unsynced and unapplied item conflict should be resolved"; + EXPECT_EQ(0, status.num_updates_applied()) + << "Update should not be applied; we should override the server."; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry e(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(e.good()); + EXPECT_EQ(original_server_version, e.GetServerVersion()); + EXPECT_EQ(original_server_version, e.GetBaseVersion()); + EXPECT_FALSE(e.GetIsUnappliedUpdate()); + + // The unsynced flag will remain set until we successfully commit the item. + EXPECT_TRUE(e.GetIsUnsynced()); + } +} + +// Create a simple conflict that is also a hierarchy conflict. If we were to +// follow the normal "server wins" logic, we'd end up violating hierarchy +// constraints. The hierarchy conflict must take precedence. We can not allow +// the update to be applied. The item must remain in the conflict state. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, HierarchyAndSimpleConflict) { + // Create a simply-conflicting item. It will start with valid parent ids. + int64 handle = entry_factory()->CreateUnappliedAndUnsyncedBookmarkItem( + "orphaned_by_server"); + { + // Manually set the SERVER_PARENT_ID to bad value. + // A bad parent indicates a hierarchy conflict. + syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + syncable::MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(entry.good()); + + entry.PutServerParentId(TestIdFactory::MakeServer("bogus_parent")); + } + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + EXPECT_EQ(0, status.num_updates_applied()); + EXPECT_EQ(0, status.num_server_overwrites()); + EXPECT_EQ(1, status.num_hierarchy_conflicts()); + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry e(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(e.good()); + EXPECT_TRUE(e.GetIsUnappliedUpdate()); + EXPECT_TRUE(e.GetIsUnsynced()); + } +} + +// Attempt to apply an udpate that would create a bookmark folder loop. This +// application should fail. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, BookmarkFolderLoop) { + // Item 'X' locally has parent of 'root'. Server is updating it to have + // parent of 'Y'. + + // Create it as a child of root node. + int64 handle = entry_factory()->CreateSyncedItem("X", BOOKMARKS, true); + + { + syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + syncable::MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(entry.good()); + + // Re-parent from root to "Y" + entry.PutServerVersion(entry_factory()->GetNextRevision()); + entry.PutIsUnappliedUpdate(true); + entry.PutServerParentId(TestIdFactory::MakeServer("Y")); + } + + // Item 'Y' is child of 'X'. + entry_factory()->CreateUnsyncedItem( + TestIdFactory::MakeServer("Y"), TestIdFactory::MakeServer("X"), "Y", true, + BOOKMARKS, NULL); + + // If the server's update were applied, we would have X be a child of Y, and Y + // as a child of X. That's a directory loop. The UpdateApplicator should + // prevent the update from being applied and note that this is a hierarchy + // conflict. + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + + // This should count as a hierarchy conflict. + EXPECT_EQ(1, status.num_hierarchy_conflicts()); + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry e(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(e.good()); + EXPECT_TRUE(e.GetIsUnappliedUpdate()); + EXPECT_FALSE(e.GetIsUnsynced()); + } +} + +// Test update application where the update has been orphaned by a local folder +// deletion. The update application attempt should fail. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, + HierarchyConflictDeletedParent) { + // Create a locally deleted parent item. + int64 parent_handle; + entry_factory()->CreateUnsyncedItem( + syncable::Id::CreateFromServerId("parent"), TestIdFactory::root(), + "parent", true, BOOKMARKS, &parent_handle); + { + syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + syncable::MutableEntry entry(&trans, + syncable::GET_BY_HANDLE, + parent_handle); + entry.PutIsDel(true); + } + + // Create an incoming child from the server. + int64 child_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "child", DefaultBookmarkSpecifics(), "parent"); + + // The server's update may seem valid to some other client, but on this client + // that new item's parent no longer exists. The update should not be applied + // and the update applicator should indicate this is a hierarchy conflict. + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + EXPECT_EQ(1, status.num_hierarchy_conflicts()); + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry child(&trans, syncable::GET_BY_HANDLE, child_handle); + ASSERT_TRUE(child.good()); + EXPECT_TRUE(child.GetIsUnappliedUpdate()); + EXPECT_FALSE(child.GetIsUnsynced()); + } +} + +// Attempt to apply an update that deletes a folder where the folder has +// locally-created children. The update application should fail. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, + HierarchyConflictDeleteNonEmptyDirectory) { + // Create a server-deleted folder as a child of root node. + int64 parent_handle = + entry_factory()->CreateSyncedItem("parent", BOOKMARKS, true); + { + syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + syncable::MutableEntry entry(&trans, + syncable::GET_BY_HANDLE, + parent_handle); + ASSERT_TRUE(entry.good()); + + // Delete it on the server. + entry.PutServerVersion(entry_factory()->GetNextRevision()); + entry.PutIsUnappliedUpdate(true); + entry.PutServerParentId(TestIdFactory::root()); + entry.PutServerIsDel(true); + } + + // Create a local child of the server-deleted directory. + entry_factory()->CreateUnsyncedItem( + TestIdFactory::MakeServer("child"), TestIdFactory::MakeServer("parent"), + "child", false, BOOKMARKS, NULL); + + // The server's request to delete the directory must be ignored, otherwise our + // unsynced new child would be orphaned. This is a hierarchy conflict. + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + + // This should count as a hierarchy conflict. + EXPECT_EQ(1, status.num_hierarchy_conflicts()); + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry parent(&trans, syncable::GET_BY_HANDLE, parent_handle); + ASSERT_TRUE(parent.good()); + EXPECT_TRUE(parent.GetIsUnappliedUpdate()); + EXPECT_FALSE(parent.GetIsUnsynced()); + } +} + +// Attempt to apply updates where the updated item's parent is not known to this +// client. The update application attempt should fail. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, + HierarchyConflictUnknownParent) { + // We shouldn't be able to do anything with either of these items. + int64 x_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "some_item", DefaultBookmarkSpecifics(), "unknown_parent"); + int64 y_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "some_other_item", DefaultBookmarkSpecifics(), "some_item"); + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + + EXPECT_EQ(2, status.num_hierarchy_conflicts()) + << "All updates with an unknown ancestors should be in conflict"; + EXPECT_EQ(0, status.num_updates_applied()) + << "No item with an unknown ancestor should be applied"; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry x(&trans, syncable::GET_BY_HANDLE, x_handle); + syncable::Entry y(&trans, syncable::GET_BY_HANDLE, y_handle); + ASSERT_TRUE(x.good()); + ASSERT_TRUE(y.good()); + EXPECT_TRUE(x.GetIsUnappliedUpdate()); + EXPECT_TRUE(y.GetIsUnappliedUpdate()); + EXPECT_FALSE(x.GetIsUnsynced()); + EXPECT_FALSE(y.GetIsUnsynced()); + } +} + +// Attempt application of a mix of items. Some update application attempts will +// fail due to hierarchy conflicts. Others should succeed. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, ItemsBothKnownAndUnknown) { + // See what happens when there's a mixture of good and bad updates. + std::string root_server_id = syncable::GetNullId().GetServerId(); + int64 u1_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "first_unknown_item", DefaultBookmarkSpecifics(), "unknown_parent"); + int64 k1_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "first_known_item", DefaultBookmarkSpecifics(), root_server_id); + int64 u2_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "second_unknown_item", DefaultBookmarkSpecifics(), "unknown_parent"); + int64 k2_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "second_known_item", DefaultBookmarkSpecifics(), "first_known_item"); + int64 k3_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "third_known_item", DefaultBookmarkSpecifics(), "fourth_known_item"); + int64 k4_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "fourth_known_item", DefaultBookmarkSpecifics(), root_server_id); + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + + EXPECT_EQ(2, status.num_hierarchy_conflicts()) + << "The updates with unknown ancestors should be in conflict"; + EXPECT_EQ(4, status.num_updates_applied()) + << "The updates with known ancestors should be successfully applied"; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry u1(&trans, syncable::GET_BY_HANDLE, u1_handle); + syncable::Entry u2(&trans, syncable::GET_BY_HANDLE, u2_handle); + syncable::Entry k1(&trans, syncable::GET_BY_HANDLE, k1_handle); + syncable::Entry k2(&trans, syncable::GET_BY_HANDLE, k2_handle); + syncable::Entry k3(&trans, syncable::GET_BY_HANDLE, k3_handle); + syncable::Entry k4(&trans, syncable::GET_BY_HANDLE, k4_handle); + ASSERT_TRUE(u1.good()); + ASSERT_TRUE(u2.good()); + ASSERT_TRUE(k1.good()); + ASSERT_TRUE(k2.good()); + ASSERT_TRUE(k3.good()); + ASSERT_TRUE(k4.good()); + EXPECT_TRUE(u1.GetIsUnappliedUpdate()); + EXPECT_TRUE(u2.GetIsUnappliedUpdate()); + EXPECT_FALSE(k1.GetIsUnappliedUpdate()); + EXPECT_FALSE(k2.GetIsUnappliedUpdate()); + EXPECT_FALSE(k3.GetIsUnappliedUpdate()); + EXPECT_FALSE(k4.GetIsUnappliedUpdate()); + } +} + +// Attempt application of password upates where the passphrase is known. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, DecryptablePassword) { + // Decryptable password updates should be applied. + Cryptographer* cryptographer; + { + // Storing the cryptographer separately is bad, but for this test we + // know it's safe. + syncable::ReadTransaction trans(FROM_HERE, directory()); + cryptographer = directory()->GetCryptographer(&trans); + } + + KeyParams params = {"localhost", "dummy", "foobar"}; + cryptographer->AddKey(params); + + sync_pb::EntitySpecifics specifics; + sync_pb::PasswordSpecificsData data; + data.set_origin("http://example.com"); + + cryptographer->Encrypt(data, + specifics.mutable_password()->mutable_encrypted()); + int64 handle = + entry_factory()->CreateUnappliedNewItem("item", specifics, false); + + sessions::StatusController status; + ApplyPasswordUpdates(&status); + + EXPECT_EQ(1, status.num_updates_applied()) + << "The updates that can be decrypted should be applied"; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry e(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(e.good()); + EXPECT_FALSE(e.GetIsUnappliedUpdate()); + EXPECT_FALSE(e.GetIsUnsynced()); + } +} + +// Attempt application of encrypted items when the passphrase is not known. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, UndecryptableData) { + // Undecryptable updates should not be applied. + sync_pb::EntitySpecifics encrypted_bookmark; + encrypted_bookmark.mutable_encrypted(); + AddDefaultFieldValue(BOOKMARKS, &encrypted_bookmark); + std::string root_server_id = syncable::GetNullId().GetServerId(); + int64 folder_handle = entry_factory()->CreateUnappliedNewItemWithParent( + "folder", + encrypted_bookmark, + root_server_id); + int64 bookmark_handle = entry_factory()->CreateUnappliedNewItem( + "item2", + encrypted_bookmark, + false); + sync_pb::EntitySpecifics encrypted_password; + encrypted_password.mutable_password(); + int64 password_handle = entry_factory()->CreateUnappliedNewItem( + "item3", + encrypted_password, + false); + + sessions::StatusController status; + ApplyBookmarkUpdates(&status); + ApplyPasswordUpdates(&status); + + EXPECT_EQ(3, status.num_encryption_conflicts()) + << "Updates that can't be decrypted should be in encryption conflict"; + EXPECT_EQ(0, status.num_updates_applied()) + << "No update that can't be decrypted should be applied"; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry folder(&trans, syncable::GET_BY_HANDLE, folder_handle); + syncable::Entry bm(&trans, syncable::GET_BY_HANDLE, bookmark_handle); + syncable::Entry pw(&trans, syncable::GET_BY_HANDLE, password_handle); + ASSERT_TRUE(folder.good()); + ASSERT_TRUE(bm.good()); + ASSERT_TRUE(pw.good()); + EXPECT_TRUE(folder.GetIsUnappliedUpdate()); + EXPECT_TRUE(bm.GetIsUnappliedUpdate()); + EXPECT_TRUE(pw.GetIsUnappliedUpdate()); + } +} + +// Test a mix of decryptable and undecryptable updates. +TEST_F(SyncDirectoryUpdateHandlerApplyUpdateTest, SomeUndecryptablePassword) { + Cryptographer* cryptographer; + + int64 decryptable_handle = -1; + int64 undecryptable_handle = -1; + + // Only decryptable password updates should be applied. + { + sync_pb::EntitySpecifics specifics; + sync_pb::PasswordSpecificsData data; + data.set_origin("http://example.com/1"); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + cryptographer = directory()->GetCryptographer(&trans); + + KeyParams params = {"localhost", "dummy", "foobar"}; + cryptographer->AddKey(params); + + cryptographer->Encrypt(data, + specifics.mutable_password()->mutable_encrypted()); + } + decryptable_handle = + entry_factory()->CreateUnappliedNewItem("item1", specifics, false); + } + { + // Create a new cryptographer, independent of the one in the session. + Cryptographer other_cryptographer(cryptographer->encryptor()); + KeyParams params = {"localhost", "dummy", "bazqux"}; + other_cryptographer.AddKey(params); + + sync_pb::EntitySpecifics specifics; + sync_pb::PasswordSpecificsData data; + data.set_origin("http://example.com/2"); + + other_cryptographer.Encrypt(data, + specifics.mutable_password()->mutable_encrypted()); + undecryptable_handle = + entry_factory()->CreateUnappliedNewItem("item2", specifics, false); + } + + sessions::StatusController status; + ApplyPasswordUpdates(&status); + + EXPECT_EQ(1, status.num_encryption_conflicts()) + << "The updates that can't be decrypted should be in encryption " + << "conflict"; + EXPECT_EQ(1, status.num_updates_applied()) + << "The undecryptable password update shouldn't be applied"; + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + syncable::Entry e1(&trans, syncable::GET_BY_HANDLE, decryptable_handle); + syncable::Entry e2(&trans, syncable::GET_BY_HANDLE, undecryptable_handle); + ASSERT_TRUE(e1.good()); + ASSERT_TRUE(e2.good()); + EXPECT_FALSE(e1.GetIsUnappliedUpdate()); + EXPECT_TRUE(e2.GetIsUnappliedUpdate()); + } +} + } // namespace syncer diff --git a/sync/engine/sync_engine_event.h b/sync/engine/sync_engine_event.h index 25f9692..026d329 100644 --- a/sync/engine/sync_engine_event.h +++ b/sync/engine/sync_engine_event.h @@ -23,7 +23,7 @@ struct SYNC_EXPORT_PRIVATE SyncEngineEvent { // Sent on entry of Syncer state machine SYNC_CYCLE_BEGIN, - // SyncerCommand generated events. + // Sent any time progress is made during a sync cycle. STATUS_CHANGED, // We have reached the SYNCER_END state in the main sync loop. diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc index 85e1b67..f3b96e4 100644 --- a/sync/engine/sync_scheduler_impl.cc +++ b/sync/engine/sync_scheduler_impl.cc @@ -242,10 +242,8 @@ void SyncSchedulerImpl::Start(Mode mode) { } ModelTypeSet SyncSchedulerImpl::GetEnabledAndUnthrottledTypes() { - ModelTypeSet enabled_types = - GetRoutingInfoTypes(session_context_->routing_info()); - ModelTypeSet throttled_types = - nudge_tracker_.GetThrottledTypes(); + ModelTypeSet enabled_types = session_context_->enabled_types(); + ModelTypeSet throttled_types = nudge_tracker_.GetThrottledTypes(); return Difference(enabled_types, throttled_types); } @@ -336,8 +334,7 @@ bool SyncSchedulerImpl::CanRunNudgeJobNow(JobPriority priority) { return false; } - const ModelTypeSet enabled_types = - GetRoutingInfoTypes(session_context_->routing_info()); + const ModelTypeSet enabled_types = session_context_->enabled_types(); if (nudge_tracker_.GetThrottledTypes().HasAll(enabled_types)) { SDVLOG(1) << "Not running a nudge because we're fully type throttled."; return false; @@ -459,8 +456,8 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) { DCHECK(CalledOnValidThread()); DCHECK(CanRunNudgeJobNow(priority)); - DVLOG(2) << "Will run normal mode sync cycle with routing info " - << ModelSafeRoutingInfoToString(session_context_->routing_info()); + DVLOG(2) << "Will run normal mode sync cycle with types " + << ModelTypeSetToString(session_context_->enabled_types()); scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this)); bool premature_exit = !syncer_->NormalSyncShare( GetEnabledAndUnthrottledTypes(), @@ -503,11 +500,11 @@ void SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) { return; } - SDVLOG(2) << "Will run configure SyncShare with routes " - << ModelSafeRoutingInfoToString(session_context_->routing_info()); + SDVLOG(2) << "Will run configure SyncShare with types " + << ModelTypeSetToString(session_context_->enabled_types()); scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this)); bool premature_exit = !syncer_->ConfigureSyncShare( - GetRoutingInfoTypes(session_context_->routing_info()), + session_context_->enabled_types(), pending_configure_params_->source, session.get()); AdjustPolling(FORCE_RESET); @@ -567,8 +564,8 @@ void SyncSchedulerImpl::DoPollSyncSessionJob() { return; } - SDVLOG(2) << "Polling with routes " - << ModelSafeRoutingInfoToString(session_context_->routing_info()); + SDVLOG(2) << "Polling with types " + << ModelTypeSetToString(session_context_->enabled_types()); scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this)); syncer_->PollSyncShare( GetEnabledAndUnthrottledTypes(), diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc index 0c451ee..39b068b 100644 --- a/sync/engine/syncer.cc +++ b/sync/engine/syncer.cc @@ -11,7 +11,6 @@ #include "base/time/time.h" #include "build/build_config.h" #include "sync/engine/apply_control_data_updates.h" -#include "sync/engine/apply_updates_and_resolve_conflicts_command.h" #include "sync/engine/commit.h" #include "sync/engine/conflict_resolver.h" #include "sync/engine/download.h" @@ -115,8 +114,11 @@ void Syncer::ApplyUpdates(SyncSession* session) { ApplyControlDataUpdates(session->context()->directory()); - ApplyUpdatesAndResolveConflictsCommand apply_updates; - apply_updates.Execute(session); + UpdateHandlerMap* handler_map = session->context()->update_handler_map(); + for (UpdateHandlerMap::iterator it = handler_map->begin(); + it != handler_map->end(); ++it) { + it->second->ApplyUpdates(session->mutable_status_controller()); + } session->context()->set_hierarchy_conflict_detected( session->status_controller().num_hierarchy_conflicts() > 0); diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h index 5661fff..6154f91 100644 --- a/sync/engine/syncer.h +++ b/sync/engine/syncer.h @@ -23,16 +23,15 @@ namespace syncer { class CancelationSignal; -// A Syncer provides a control interface for driving the individual steps -// of the sync cycle. Each cycle (hopefully) moves the client into closer -// synchronization with the server. The individual steps are modeled -// as SyncerCommands, and the ordering of the steps is expressed using -// the SyncerStep enum. +// A Syncer provides a control interface for driving the sync cycle. These +// cycles consist of downloading updates, parsing the response (aka. process +// updates), applying updates while resolving conflicts, and committing local +// changes. Some of these steps may be skipped if they're deemed to be +// unnecessary. // -// A Syncer instance expects to run on a dedicated thread. Calls -// to SyncShare() may take an unbounded amount of time, as SyncerCommands -// may block on network i/o, on lock contention, or on tasks posted to -// other threads. +// A Syncer instance expects to run on a dedicated thread. Calls to SyncShare() +// may take an unbounded amount of time because it may block on network I/O, on +// lock contention, or on tasks posted to other threads. class SYNC_EXPORT_PRIVATE Syncer { public: typedef std::vector<int64> UnsyncedMetaHandles; diff --git a/sync/engine/syncer_command.cc b/sync/engine/syncer_command.cc deleted file mode 100644 index 5dfd821..0000000 --- a/sync/engine/syncer_command.cc +++ /dev/null @@ -1,17 +0,0 @@ -// Copyright (c) 2012 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 "sync/engine/syncer_command.h" - -namespace syncer { - -SyncerCommand::SyncerCommand() {} -SyncerCommand::~SyncerCommand() {} - -SyncerError SyncerCommand::Execute(sessions::SyncSession* session) { - SyncerError result = ExecuteImpl(session); - return result; -} - -} // namespace syncer diff --git a/sync/engine/syncer_command.h b/sync/engine/syncer_command.h deleted file mode 100644 index 303aad5..0000000 --- a/sync/engine/syncer_command.h +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright 2012 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. - -#ifndef SYNC_ENGINE_SYNCER_COMMAND_H_ -#define SYNC_ENGINE_SYNCER_COMMAND_H_ - -#include "base/basictypes.h" - -#include "sync/base/sync_export.h" -#include "sync/internal_api/public/util/syncer_error.h" - -namespace syncer { - -namespace sessions { -class SyncSession; -} - -// Implementation of a simple command pattern intended to be driven by the -// Syncer. SyncerCommand is abstract and all subclasses must implement -// ExecuteImpl(). This is done so that chunks of syncer operation can be unit -// tested. -// -// Example Usage: -// -// SyncSession session = ...; -// SyncerCommand *cmd = SomeCommandFactory.createCommand(...); -// cmd->Execute(session); -// delete cmd; - -class SYNC_EXPORT_PRIVATE SyncerCommand { - public: - SyncerCommand(); - virtual ~SyncerCommand(); - - // Execute dispatches to a derived class's ExecuteImpl. - SyncerError Execute(sessions::SyncSession* session); - - // ExecuteImpl is where derived classes actually do work. - virtual SyncerError ExecuteImpl(sessions::SyncSession* session) = 0; - private: - DISALLOW_COPY_AND_ASSIGN(SyncerCommand); -}; - -} // namespace syncer - -#endif // SYNC_ENGINE_SYNCER_COMMAND_H_ diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 684b404..1678ba6 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -189,7 +189,7 @@ class SyncerTest : public testing::Test, EXPECT_TRUE( syncer_->NormalSyncShare( - GetRoutingInfoTypes(context_->routing_info()), + context_->enabled_types(), nudge_tracker_, session_.get())); } @@ -197,7 +197,7 @@ class SyncerTest : public testing::Test, void SyncShareConfigure() { session_.reset(SyncSession::Build(context_.get(), this)); EXPECT_TRUE(syncer_->ConfigureSyncShare( - GetRoutingInfoTypes(context_->routing_info()), + context_->enabled_types(), sync_pb::GetUpdatesCallerInfo::RECONFIGURATION, session_.get())); } @@ -544,8 +544,7 @@ TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) { // Now sync without enabling bookmarks. syncer_->NormalSyncShare( - Difference(GetRoutingInfoTypes(context_->routing_info()), - ModelTypeSet(BOOKMARKS)), + Difference(context_->enabled_types(), ModelTypeSet(BOOKMARKS)), nudge_tracker_, session_.get()); @@ -559,7 +558,7 @@ TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) { // Sync again with bookmarks enabled. syncer_->NormalSyncShare( - GetRoutingInfoTypes(context_->routing_info()), + context_->enabled_types(), nudge_tracker_, session_.get()); SyncShareNudge(); diff --git a/sync/engine/update_applicator.cc b/sync/engine/update_applicator.cc index 3edf1f8..e8731ce 100644 --- a/sync/engine/update_applicator.cc +++ b/sync/engine/update_applicator.cc @@ -19,12 +19,8 @@ namespace syncer { using syncable::ID; -UpdateApplicator::UpdateApplicator(Cryptographer* cryptographer, - const ModelSafeRoutingInfo& routes, - ModelSafeGroup group_filter) +UpdateApplicator::UpdateApplicator(Cryptographer* cryptographer) : cryptographer_(cryptographer), - group_filter_(group_filter), - routing_info_(routes), updates_applied_(0), encryption_conflicts_(0), hierarchy_conflicts_(0) { @@ -58,11 +54,6 @@ void UpdateApplicator::AttemptApplications( for (std::vector<int64>::iterator i = to_apply.begin(); i != to_apply.end(); ++i) { - syncable::Entry read_entry(trans, syncable::GET_BY_HANDLE, *i); - if (SkipUpdate(read_entry)) { - continue; - } - syncable::MutableEntry entry(trans, syncable::GET_BY_HANDLE, *i); UpdateAttemptResponse result = AttemptToUpdateEntry( trans, &entry, cryptographer_); @@ -103,23 +94,4 @@ void UpdateApplicator::AttemptApplications( } } -bool UpdateApplicator::SkipUpdate(const syncable::Entry& entry) { - ModelType type = entry.GetServerModelType(); - ModelSafeGroup g = GetGroupForModelType(type, routing_info_); - // The set of updates passed to the UpdateApplicator should already - // be group-filtered. - if (g != group_filter_) { - NOTREACHED(); - return true; - } - if (g == GROUP_PASSIVE && - !routing_info_.count(type) && - type != UNSPECIFIED && - type != TOP_LEVEL_FOLDER) { - DVLOG(1) << "Skipping update application, type not permitted."; - return true; - } - return false; -} - } // namespace syncer diff --git a/sync/engine/update_applicator.h b/sync/engine/update_applicator.h index b04325c..ff8fa15 100644 --- a/sync/engine/update_applicator.h +++ b/sync/engine/update_applicator.h @@ -35,9 +35,7 @@ class Cryptographer; class UpdateApplicator { public: - UpdateApplicator(Cryptographer* cryptographer, - const ModelSafeRoutingInfo& routes, - ModelSafeGroup group_filter); + UpdateApplicator(Cryptographer* cryptographer); ~UpdateApplicator(); // Attempt to apply the specified updates. @@ -67,10 +65,6 @@ class UpdateApplicator { // Used to decrypt sensitive sync nodes. Cryptographer* cryptographer_; - ModelSafeGroup group_filter_; - - const ModelSafeRoutingInfo routing_info_; - DISALLOW_COPY_AND_ASSIGN(UpdateApplicator); int updates_applied_; diff --git a/sync/internal_api/public/util/syncer_error.h b/sync/internal_api/public/util/syncer_error.h index 471bc7e..db59b7d 100644 --- a/sync/internal_api/public/util/syncer_error.h +++ b/sync/internal_api/public/util/syncer_error.h @@ -9,15 +9,7 @@ namespace syncer { -// This enum describes all the ways a SyncerCommand can fail. -// -// SyncerCommands do many different things, but they share a common function -// signature. This enum, the return value for all SyncerCommands, must be able -// to describe any possible failure for all SyncerComand. -// -// For convenience, functions which are invoked only by SyncerCommands may also -// return a SyncerError. It saves us having to write a conversion function, and -// it makes refactoring easier. +// This enum describes all the possible results of a sync cycle. enum SYNC_EXPORT_PRIVATE SyncerError { UNSET = 0, // Default value. CANNOT_DO_WORK, // A model worker could not process a work item. diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 2a1129b..91be3b8 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -66,6 +66,7 @@ #include "sync/syncable/syncable_util.h" #include "sync/syncable/syncable_write_transaction.h" #include "sync/test/callback_counter.h" +#include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/fake_sync_scheduler.h" #include "sync/test/engine/test_id_factory.h" #include "sync/test/fake_encryptor.h" @@ -816,6 +817,12 @@ class SyncManagerTest : public testing::Test, ModelSafeRoutingInfo routing_info; GetModelSafeRoutingInfo(&routing_info); + // This works only because all routing info types are GROUP_PASSIVE. + // If we had types in other groups, we would need additional workers + // to support them. + scoped_refptr<ModelSafeWorker> worker = new FakeModelWorker(GROUP_PASSIVE); + workers.push_back(worker.get()); + // Takes ownership of |fake_invalidator_|. sync_manager_.Init( temp_dir_.path(), diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc index f6fbdb5..314a232 100644 --- a/sync/sessions/status_controller.cc +++ b/sync/sessions/status_controller.cc @@ -13,9 +13,7 @@ namespace syncer { namespace sessions { -StatusController::StatusController() - : group_restriction_in_effect_(false), - group_restriction_(GROUP_PASSIVE) { +StatusController::StatusController() { } StatusController::~StatusController() {} @@ -129,20 +127,14 @@ int StatusController::num_encryption_conflicts() const { } int StatusController::num_hierarchy_conflicts() const { - DCHECK(!group_restriction_in_effect_) - << "num_hierarchy_conflicts applies to all ModelSafeGroups"; return model_neutral_.num_hierarchy_conflicts; } int StatusController::num_server_conflicts() const { - DCHECK(!group_restriction_in_effect_) - << "num_server_conflicts applies to all ModelSafeGroups"; return model_neutral_.num_server_conflicts; } int StatusController::TotalNumConflictingItems() const { - DCHECK(!group_restriction_in_effect_) - << "TotalNumConflictingItems applies to all ModelSafeGroups"; int sum = 0; sum += num_encryption_conflicts(); sum += num_hierarchy_conflicts(); diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h index 75b8b82..0e2c6f2 100644 --- a/sync/sessions/status_controller.h +++ b/sync/sessions/status_controller.h @@ -5,25 +5,14 @@ // StatusController handles all counter and status related number crunching and // state tracking on behalf of a SyncSession. // -// The most important feature of StatusController is the -// ScopedModelSafeGroupRestriction. Some of its functions expose per-thread -// state, and can be called only when the restriction is in effect. For -// example, if GROUP_UI is set then the value returned from -// commit_id_projection() will be useful for iterating over the commit IDs of -// items that live on the UI thread. +// This object may be accessed from many different threads. It will be accessed +// most often from the syncer thread. However, when update application is in +// progress it may also be accessed from the worker threads. This is safe +// because only one of them will run at a time, and the syncer thread will be +// blocked until update application completes. // -// Other parts of its state are global, and do not require the restriction. -// -// NOTE: There is no concurrent access protection provided by this class. It -// assumes one single thread is accessing this class for each unique -// ModelSafeGroup, and also only one single thread (in practice, the -// SyncerThread) responsible for all "shared" access when no restriction is in -// place. Thus, every bit of data is to be accessed mutually exclusively with -// respect to threads. -// -// StatusController can also track if changes occur to certain parts of state -// so that various parts of the sync engine can avoid broadcasting -// notifications if no changes occurred. +// This object contains only global state. None of its members are per model +// type counters. #ifndef SYNC_SESSIONS_STATUS_CONTROLLER_H_ #define SYNC_SESSIONS_STATUS_CONTROLLER_H_ @@ -96,10 +85,6 @@ class SYNC_EXPORT_PRIVATE StatusController { // download: in that case, this also returns false. bool ServerSaysNothingMoreToDownload() const; - ModelSafeGroup group_restriction() const { - return group_restriction_; - } - base::Time sync_start_time() const { // The time at which we sent the first GetUpdates command for this sync. return sync_start_time_; @@ -142,40 +127,13 @@ class SYNC_EXPORT_PRIVATE StatusController { void UpdateStartTime(); private: - friend class ScopedModelSafeGroupRestriction; - ModelNeutralState model_neutral_; - // Used to fail read/write operations on state that don't obey the current - // active ModelSafeWorker contract. - bool group_restriction_in_effect_; - ModelSafeGroup group_restriction_; - base::Time sync_start_time_; DISALLOW_COPY_AND_ASSIGN(StatusController); }; -// A utility to restrict access to only those parts of the given -// StatusController that pertain to the specified ModelSafeGroup. -class ScopedModelSafeGroupRestriction { - public: - ScopedModelSafeGroupRestriction(StatusController* to_restrict, - ModelSafeGroup restriction) - : status_(to_restrict) { - DCHECK(!status_->group_restriction_in_effect_); - status_->group_restriction_ = restriction; - status_->group_restriction_in_effect_ = true; - } - ~ScopedModelSafeGroupRestriction() { - DCHECK(status_->group_restriction_in_effect_); - status_->group_restriction_in_effect_ = false; - } - private: - StatusController* status_; - DISALLOW_COPY_AND_ASSIGN(ScopedModelSafeGroupRestriction); -}; - } // namespace sessions } // namespace syncer diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc index e6b59e8..32e7b64 100644 --- a/sync/sessions/status_controller_unittest.cc +++ b/sync/sessions/status_controller_unittest.cc @@ -52,14 +52,5 @@ TEST_F(StatusControllerTest, TotalNumConflictingItems) { EXPECT_EQ(6, status.TotalNumConflictingItems()); } -// Basic test that non group-restricted state accessors don't cause violations. -TEST_F(StatusControllerTest, Unrestricted) { - StatusController status; - status.model_neutral_state(); - status.download_updates_succeeded(); - status.ServerSaysNothingMoreToDownload(); - status.group_restriction(); -} - } // namespace sessions } // namespace syncer diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h index af846d4..f576720 100644 --- a/sync/sessions/sync_session.h +++ b/sync/sessions/sync_session.h @@ -4,12 +4,8 @@ // A class representing an attempt to synchronize the local syncable data // store with a sync server. A SyncSession instance is passed as a stateful -// bundle to and from various SyncerCommands with the goal of converging the -// client view of data with that of the server. The commands twiddle with -// session status in response to events and hiccups along the way, set and -// query session progress with regards to conflict resolution and applying -// server updates, and access the SyncSessionContext for the current session -// via SyncSession instances. +// bundle throughout the sync cycle. The SyncSession is not reused across +// sync cycles; each cycle starts with a new one. #ifndef SYNC_SESSIONS_SYNC_SESSION_H_ #define SYNC_SESSIONS_SYNC_SESSION_H_ diff --git a/sync/sessions/sync_session_context.cc b/sync/sessions/sync_session_context.cc index d44591d..aa5dfa5 100644 --- a/sync/sessions/sync_session_context.cc +++ b/sync/sessions/sync_session_context.cc @@ -35,8 +35,10 @@ SyncSessionContext::SyncSessionContext( server_enabled_pre_commit_update_avoidance_(false), client_enabled_pre_commit_update_avoidance_( client_enabled_pre_commit_update_avoidance) { - for (size_t i = 0u; i < workers.size(); ++i) - workers_.push_back(workers[i]); + for (size_t i = 0u; i < workers.size(); ++i) { + workers_.insert( + std::make_pair(workers[i]->GetModelSafeGroup(), workers[i])); + } std::vector<SyncEngineEventListener*>::const_iterator it; for (it = listeners.begin(); it != listeners.end(); ++it) @@ -48,24 +50,28 @@ SyncSessionContext::~SyncSessionContext() { void SyncSessionContext::set_routing_info( const ModelSafeRoutingInfo& routing_info) { - routing_info_ = routing_info; + enabled_types_ = GetRoutingInfoTypes(routing_info); // TODO(rlarocque): This is not a good long-term solution. We must find a // better way to initialize the set of CommitContributors and UpdateHandlers. - ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info); - + STLDeleteValues<UpdateHandlerMap>(&update_handler_map_); STLDeleteValues<CommitContributorMap>(&commit_contributor_map_); - for (ModelTypeSet::Iterator it = enabled_types.First(); it.Good(); it.Inc()) { - SyncDirectoryCommitContributor* contributor = - new SyncDirectoryCommitContributor(directory(), it.Get()); - commit_contributor_map_.insert(std::make_pair(it.Get(), contributor)); - } + for (ModelSafeRoutingInfo::const_iterator routing_iter = routing_info.begin(); + routing_iter != routing_info.end(); ++routing_iter) { + ModelType type = routing_iter->first; + ModelSafeGroup group = routing_iter->second; + std::map<ModelSafeGroup, scoped_refptr<ModelSafeWorker> >::iterator + worker_it = workers_.find(group); + DCHECK(worker_it != workers_.end()); + scoped_refptr<ModelSafeWorker> worker = worker_it->second; - STLDeleteValues<UpdateHandlerMap>(&update_handler_map_); - for (ModelTypeSet::Iterator it = enabled_types.First(); it.Good(); it.Inc()) { SyncDirectoryUpdateHandler* handler = - new SyncDirectoryUpdateHandler(directory(), it.Get()); - update_handler_map_.insert(std::make_pair(it.Get(), handler)); + new SyncDirectoryUpdateHandler(directory(), type, worker); + update_handler_map_.insert(std::make_pair(type, handler)); + + SyncDirectoryCommitContributor* contributor = + new SyncDirectoryCommitContributor(directory(), type); + commit_contributor_map_.insert(std::make_pair(type, contributor)); } } diff --git a/sync/sessions/sync_session_context.h b/sync/sessions/sync_session_context.h index d195943..5995ab1 100644 --- a/sync/sessions/sync_session_context.h +++ b/sync/sessions/sync_session_context.h @@ -3,15 +3,12 @@ // found in the LICENSE file. // SyncSessionContext encapsulates the contextual information and engine -// components specific to a SyncSession. A context is accessible via -// a SyncSession so that session SyncerCommands and parts of the engine have -// a convenient way to access other parts. In this way it can be thought of as -// the surrounding environment for the SyncSession. The components of this -// environment are either valid or not valid for the entire context lifetime, -// or they are valid for explicitly scoped periods of time by using Scoped -// installation utilities found below. This means that the context assumes no -// ownership whatsoever of any object that was not created by the context -// itself. +// components specific to a SyncSession. Unlike the SyncSession, the context +// can be reused across several sync cycles. +// +// The context does not take ownership of its pointer members. It's up to +// the surrounding classes to ensure those members remain valid while the +// context is in use. // // It can only be used from the SyncerThread. @@ -70,8 +67,8 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext { return directory_; } - const ModelSafeRoutingInfo& routing_info() const { - return routing_info_; + ModelTypeSet enabled_types() const { + return enabled_types_; } void set_routing_info(const ModelSafeRoutingInfo& routing_info); @@ -84,10 +81,6 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext { return &commit_contributor_map_; } - const std::vector<scoped_refptr<ModelSafeWorker> >& workers() const { - return workers_; - } - ExtensionsActivity* extensions_activity() { return extensions_activity_.get(); } @@ -159,9 +152,9 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext { ServerConnectionManager* const connection_manager_; syncable::Directory* const directory_; - // A cached copy of SyncBackendRegistrar's routing info. - // Must be updated manually when SBR's state is modified. - ModelSafeRoutingInfo routing_info_; + // The set of enabled types. Derrived from the routing info set with + // set_routing_info(). + ModelTypeSet enabled_types_; // A map of 'update handlers', one for each enabled type. // This must be kept in sync with the routing info. Our temporary solution to @@ -180,7 +173,7 @@ class SYNC_EXPORT_PRIVATE SyncSessionContext { STLValueDeleter<CommitContributorMap> commit_contributor_deleter_; // The set of ModelSafeWorkers. Used to execute tasks of various threads. - std::vector<scoped_refptr<ModelSafeWorker> > workers_; + std::map<ModelSafeGroup, scoped_refptr<ModelSafeWorker> > workers_; // We use this to stuff extensions activity into CommitMessages so the server // can correlate commit traffic with extension-related bookmark mutations. diff --git a/sync/sync_core.gypi b/sync/sync_core.gypi index 5d6d989..f4912fe 100644 --- a/sync/sync_core.gypi +++ b/sync/sync_core.gypi @@ -32,8 +32,6 @@ 'engine/all_status.h', 'engine/apply_control_data_updates.cc', 'engine/apply_control_data_updates.h', - 'engine/apply_updates_and_resolve_conflicts_command.cc', - 'engine/apply_updates_and_resolve_conflicts_command.h', 'engine/backoff_delay_provider.cc', 'engine/backoff_delay_provider.h', 'engine/commit_util.cc', @@ -54,8 +52,6 @@ 'engine/download.h', 'engine/get_commit_ids.cc', 'engine/get_commit_ids.h', - 'engine/model_changing_syncer_command.cc', - 'engine/model_changing_syncer_command.h', 'engine/net/server_connection_manager.cc', 'engine/net/server_connection_manager.h', 'engine/net/url_translator.cc', @@ -72,8 +68,6 @@ 'engine/sync_scheduler_impl.h', 'engine/syncer.cc', 'engine/syncer.h', - 'engine/syncer_command.cc', - 'engine/syncer_command.h', 'engine/syncer_proto_util.cc', 'engine/syncer_proto_util.h', 'engine/syncer_types.h', diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi index 010cb5b..683ca52 100644 --- a/sync/sync_tests.gypi +++ b/sync/sync_tests.gypi @@ -40,8 +40,6 @@ 'test/engine/fake_sync_scheduler.h', 'test/engine/mock_connection_manager.cc', 'test/engine/mock_connection_manager.h', - 'test/engine/syncer_command_test.cc', - 'test/engine/syncer_command_test.h', 'test/engine/test_directory_setter_upper.cc', 'test/engine/test_directory_setter_upper.h', 'test/engine/test_id_factory.h', @@ -243,10 +241,8 @@ 'internal_api/public/util/immutable_unittest.cc', 'internal_api/public/util/weak_handle_unittest.cc', 'engine/apply_control_data_updates_unittest.cc', - 'engine/apply_updates_and_resolve_conflicts_command_unittest.cc', 'engine/backoff_delay_provider_unittest.cc', 'engine/download_unittest.cc', - 'engine/model_changing_syncer_command_unittest.cc', 'engine/sync_scheduler_unittest.cc', 'engine/syncer_proto_util_unittest.cc', 'engine/syncer_unittest.cc', diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index 85e5107..33b7e15 100644 --- a/sync/syncable/directory.cc +++ b/sync/syncable/directory.cc @@ -910,17 +910,9 @@ int64 Directory::unsynced_entity_count() const { return kernel_->unsynced_metahandles.size(); } -FullModelTypeSet Directory::GetServerTypesWithUnappliedUpdates( - BaseTransaction* trans) const { - FullModelTypeSet server_types; +bool Directory::TypeHasUnappliedUpdates(ModelType type) { ScopedKernelLock lock(this); - for (int i = UNSPECIFIED; i < MODEL_TYPE_COUNT; ++i) { - const ModelType type = ModelTypeFromInt(i); - if (!kernel_->unapplied_update_metahandles[type].empty()) { - server_types.Put(type); - } - } - return server_types; + return !kernel_->unapplied_update_metahandles[type].empty(); } void Directory::GetUnappliedUpdateMetaHandles( diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h index 7a58dfc..0206dbb 100644 --- a/sync/syncable/directory.h +++ b/sync/syncable/directory.h @@ -315,11 +315,8 @@ class SYNC_EXPORT Directory { void GetUnsyncedMetaHandles(BaseTransaction* trans, Metahandles* result); - // Returns all server types with unapplied updates. A subset of - // those types can then be passed into - // GetUnappliedUpdateMetaHandles() below. - FullModelTypeSet GetServerTypesWithUnappliedUpdates( - BaseTransaction* trans) const; + // Returns whether or not this |type| has unapplied updates. + bool TypeHasUnappliedUpdates(ModelType type); // Get all the metahandles for unapplied updates for a given set of // server types. diff --git a/sync/test/engine/syncer_command_test.cc b/sync/test/engine/syncer_command_test.cc deleted file mode 100644 index 8cf8e13..0000000 --- a/sync/test/engine/syncer_command_test.cc +++ /dev/null @@ -1,83 +0,0 @@ -// Copyright (c) 2012 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 "sync/test/engine/syncer_command_test.h" - -namespace syncer { - -const unsigned int kMaxMessages = 10; -const unsigned int kMaxMessageSize = 5 * 1024; - -void SyncerCommandTestBase::OnThrottled( - const base::TimeDelta& throttle_duration) { - FAIL() << "Should not get silenced."; -} - -void SyncerCommandTestBase::OnTypesThrottled( - ModelTypeSet types, - const base::TimeDelta& throttle_duration) { - FAIL() << "Should not get silenced."; -} - -bool SyncerCommandTestBase::IsCurrentlyThrottled() { - return false; -} - -void SyncerCommandTestBase::OnReceivedLongPollIntervalUpdate( - const base::TimeDelta& new_interval) { - FAIL() << "Should not get poll interval update."; -} - -void SyncerCommandTestBase::OnReceivedShortPollIntervalUpdate( - const base::TimeDelta& new_interval) { - FAIL() << "Should not get poll interval update."; -} - -void SyncerCommandTestBase::OnReceivedSessionsCommitDelay( - const base::TimeDelta& new_delay) { - FAIL() << "Should not get sessions commit delay."; -} - -void SyncerCommandTestBase::OnReceivedClientInvalidationHintBufferSize( - int size) { - FAIL() << "Should not get hint buffer size."; -} - -void SyncerCommandTestBase::OnSyncProtocolError( - const sessions::SyncSessionSnapshot& session) { - return; -} -SyncerCommandTestBase::SyncerCommandTestBase() - : traffic_recorder_(kMaxMessages, kMaxMessageSize) { -} - -SyncerCommandTestBase::~SyncerCommandTestBase() { -} - -void SyncerCommandTestBase::SetUp() { - extensions_activity_ = new ExtensionsActivity(); - - // The session always expects there to be a passive worker. - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_PASSIVE))); - ResetContext(); -} - -void SyncerCommandTestBase::TearDown() { -} - -syncable::Directory* SyncerCommandTest::directory() { - return dir_maker_.directory(); -} - -void SyncerCommandTest::SetUp() { - dir_maker_.SetUp(); - SyncerCommandTestBase::SetUp(); -} - -void SyncerCommandTest::TearDown() { - dir_maker_.TearDown(); -} - -} // namespace syncer diff --git a/sync/test/engine/syncer_command_test.h b/sync/test/engine/syncer_command_test.h deleted file mode 100644 index 0f106c5..0000000 --- a/sync/test/engine/syncer_command_test.h +++ /dev/null @@ -1,189 +0,0 @@ -// Copyright (c) 2012 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. - -#ifndef SYNC_TEST_ENGINE_SYNCER_COMMAND_TEST_H_ -#define SYNC_TEST_ENGINE_SYNCER_COMMAND_TEST_H_ - -#include <algorithm> -#include <string> -#include <vector> - -#include "base/compiler_specific.h" -#include "base/memory/ref_counted.h" -#include "base/message_loop/message_loop.h" -#include "sync/engine/model_changing_syncer_command.h" -#include "sync/engine/traffic_recorder.h" -#include "sync/internal_api/debug_info_event_listener.h" -#include "sync/internal_api/public/base/cancelation_signal.h" -#include "sync/internal_api/public/engine/model_safe_worker.h" -#include "sync/sessions/debug_info_getter.h" -#include "sync/sessions/sync_session.h" -#include "sync/sessions/sync_session_context.h" -#include "sync/syncable/directory.h" -#include "sync/test/engine/fake_model_worker.h" -#include "sync/test/engine/mock_connection_manager.h" -#include "sync/test/engine/test_directory_setter_upper.h" -#include "sync/util/extensions_activity.h" -#include "testing/gmock/include/gmock/gmock.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace syncer { - -// A test fixture that simplifies writing unit tests for individual -// SyncerCommands, providing convenient access to a test directory -// and a syncer session. -class SyncerCommandTestBase : public testing::Test, - public sessions::SyncSession::Delegate { - public: - // SyncSession::Delegate implementation. - virtual void OnThrottled( - const base::TimeDelta& throttle_duration) OVERRIDE; - virtual void OnTypesThrottled( - ModelTypeSet types, - const base::TimeDelta& throttle_duration) OVERRIDE; - virtual bool IsCurrentlyThrottled() OVERRIDE; - virtual void OnReceivedLongPollIntervalUpdate( - const base::TimeDelta& new_interval) OVERRIDE; - virtual void OnReceivedShortPollIntervalUpdate( - const base::TimeDelta& new_interval) OVERRIDE; - virtual void OnReceivedSessionsCommitDelay( - const base::TimeDelta& new_delay) OVERRIDE; - virtual void OnReceivedClientInvalidationHintBufferSize(int size) OVERRIDE; - virtual void OnSyncProtocolError( - const sessions::SyncSessionSnapshot& session) OVERRIDE; - - std::vector<ModelSafeWorker*> GetWorkers() { - std::vector<ModelSafeWorker*> workers; - std::vector<scoped_refptr<ModelSafeWorker> >::iterator it; - for (it = workers_.begin(); it != workers_.end(); ++it) - workers.push_back(it->get()); - return workers; - } - void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) { - ModelSafeRoutingInfo copy(routing_info_); - out->swap(copy); - } - - protected: - SyncerCommandTestBase(); - - virtual ~SyncerCommandTestBase(); - virtual void SetUp(); - virtual void TearDown(); - - sessions::SyncSessionContext* context() const { return context_.get(); } - sessions::SyncSession::Delegate* delegate() { return this; } - - // Lazily create a session requesting all datatypes with no state. - sessions::SyncSession* session() { - if (!session_.get()) - session_.reset(sessions::SyncSession::Build(context(), delegate())); - return session_.get(); - } - - void ClearSession() { - session_.reset(); - } - - void ResetContext() { - context_.reset(new sessions::SyncSessionContext( - mock_server_.get(), directory(), - GetWorkers(), extensions_activity_.get(), - std::vector<SyncEngineEventListener*>(), - &debug_info_event_listener_, - &traffic_recorder_, - true, // enable keystore encryption - false, // force enable pre-commit GU avoidance experiment - "fake_invalidator_client_id")); - context_->set_routing_info(routing_info_); - context_->set_account_name(directory()->name()); - ClearSession(); - } - - // Install a MockServerConnection. Resets the context. By default, - // the context does not have a MockServerConnection attached. - void ConfigureMockServerConnection() { - mock_server_.reset(new MockConnectionManager(directory(), - &cancelation_signal_)); - ResetContext(); - } - - virtual syncable::Directory* directory() = 0; - - std::vector<scoped_refptr<ModelSafeWorker> >* workers() { - return &workers_; - } - - const ModelSafeRoutingInfo& routing_info() { return routing_info_; } - ModelSafeRoutingInfo* mutable_routing_info() { return &routing_info_; } - - MockConnectionManager* mock_server() { - return mock_server_.get(); - } - - DebugInfoEventListener* debug_info_event_listener() { - return &debug_info_event_listener_; - } - // Helper functions to check command.GetGroupsToChange(). - - void ExpectNoGroupsToChange(const ModelChangingSyncerCommand& command) { - EXPECT_TRUE(command.GetGroupsToChangeForTest(*session()).empty()); - } - - void ExpectGroupToChange( - const ModelChangingSyncerCommand& command, ModelSafeGroup group) { - std::set<ModelSafeGroup> expected_groups_to_change; - expected_groups_to_change.insert(group); - EXPECT_EQ(expected_groups_to_change, - command.GetGroupsToChangeForTest(*session())); - } - - void ExpectGroupsToChange( - const ModelChangingSyncerCommand& command, - ModelSafeGroup group1, ModelSafeGroup group2) { - std::set<ModelSafeGroup> expected_groups_to_change; - expected_groups_to_change.insert(group1); - expected_groups_to_change.insert(group2); - EXPECT_EQ(expected_groups_to_change, - command.GetGroupsToChangeForTest(*session())); - } - - void ExpectGroupsToChange( - const ModelChangingSyncerCommand& command, - ModelSafeGroup group1, ModelSafeGroup group2, ModelSafeGroup group3) { - std::set<ModelSafeGroup> expected_groups_to_change; - expected_groups_to_change.insert(group1); - expected_groups_to_change.insert(group2); - expected_groups_to_change.insert(group3); - EXPECT_EQ(expected_groups_to_change, - command.GetGroupsToChangeForTest(*session())); - } - - private: - base::MessageLoop message_loop_; - scoped_ptr<sessions::SyncSessionContext> context_; - scoped_ptr<MockConnectionManager> mock_server_; - scoped_ptr<sessions::SyncSession> session_; - std::vector<scoped_refptr<ModelSafeWorker> > workers_; - ModelSafeRoutingInfo routing_info_; - DebugInfoEventListener debug_info_event_listener_; - scoped_refptr<ExtensionsActivity> extensions_activity_; - TrafficRecorder traffic_recorder_; - CancelationSignal cancelation_signal_; - DISALLOW_COPY_AND_ASSIGN(SyncerCommandTestBase); -}; - -class SyncerCommandTest : public SyncerCommandTestBase { - public: - virtual void SetUp() OVERRIDE; - virtual void TearDown() OVERRIDE; - virtual syncable::Directory* directory() OVERRIDE; - - private: - TestDirectorySetterUpper dir_maker_; -}; - -} // namespace syncer - -#endif // SYNC_TEST_ENGINE_SYNCER_COMMAND_TEST_H_ |