From 31aa39ddb62a29bbff6b7073a71d2f513c6e06d7 Mon Sep 17 00:00:00 2001 From: "rlarocque@chromium.org" Date: Wed, 14 May 2014 02:58:23 +0000 Subject: sync: Improve handling of bad UniquePositions Makes the client assign a valid position to incoming bookmarks if the server has not populated the required fields. This code should never be triggered unless there is a bug in the server. This risks reordering users' bookmarks, but that's probably preferable to a crash. Detects bookmarks that do not have valid position information during database load. If these corrupted bookmarks are detected, the entire database is declared to be corrupt. Sync will then re-download all of the user's data, which should fix the problem. BUG=367247 Review URL: https://codereview.chromium.org/278153002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@270308 0039d316-1c4b-4281-b951-d872f2087c98 --- sync/engine/syncer_util.cc | 12 ++-- sync/engine/syncer_util.h | 9 ++- sync/engine/syncer_util_unittest.cc | 127 ++++++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 9 deletions(-) create mode 100644 sync/engine/syncer_util_unittest.cc (limited to 'sync/engine') diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc index 2235734..c7d93c1 100644 --- a/sync/engine/syncer_util.cc +++ b/sync/engine/syncer_util.cc @@ -12,6 +12,7 @@ #include "base/base64.h" #include "base/location.h" #include "base/metrics/histogram.h" +#include "base/rand_util.h" #include "base/strings/string_number_conversions.h" #include "sync/engine/conflict_resolver.h" #include "sync/engine/syncer_proto_util.h" @@ -278,7 +279,9 @@ UpdateAttemptResponse AttemptToUpdateEntry( std::string GetUniqueBookmarkTagFromUpdate(const sync_pb::SyncEntity& update) { if (!update.has_originator_cache_guid() || !update.has_originator_client_item_id()) { - return std::string(); + LOG(ERROR) << "Update is missing requirements for bookmark position." + << " This is a server bug."; + return UniquePosition::RandomSuffix(); } return syncable::GenerateSyncableBookmarkHash( @@ -295,7 +298,8 @@ UniquePosition GetUpdatePosition(const sync_pb::SyncEntity& update, } else if (update.has_position_in_parent()) { return UniquePosition::FromInt64(update.position_in_parent(), suffix); } else { - return UniquePosition::CreateInvalid(); + LOG(ERROR) << "No position information in update. This is a server bug."; + return UniquePosition::FromInt64(0, suffix); } } @@ -341,10 +345,6 @@ void UpdateBookmarkPositioning( GetUpdatePosition(update, local_entry->GetUniqueBookmarkTag()); if (update_pos.IsValid()) { local_entry->PutServerUniquePosition(update_pos); - } else { - // TODO(sync): This and other cases of unexpected input should be handled - // better. - NOTREACHED(); } } diff --git a/sync/engine/syncer_util.h b/sync/engine/syncer_util.h index 575ab11..f39ad4b 100644 --- a/sync/engine/syncer_util.h +++ b/sync/engine/syncer_util.h @@ -12,6 +12,7 @@ #include #include +#include "sync/base/sync_export.h" #include "sync/engine/syncer.h" #include "sync/engine/syncer_types.h" #include "sync/syncable/entry_kernel.h" @@ -58,12 +59,14 @@ UpdateAttemptResponse AttemptToUpdateEntry( // // Will return an invalid position if no valid position can be constructed, or // if this type does not support positioning. -UniquePosition GetUpdatePosition(const sync_pb::SyncEntity& update, - const std::string& suffix); +SYNC_EXPORT_PRIVATE UniquePosition GetUpdatePosition( + const sync_pb::SyncEntity& update, + const std::string& suffix); // Fetch the cache_guid and item_id-based unique bookmark tag from an update. // Will return an empty string if someting unexpected happens. -std::string GetUniqueBookmarkTagFromUpdate(const sync_pb::SyncEntity& update); +SYNC_EXPORT_PRIVATE std::string GetUniqueBookmarkTagFromUpdate( + const sync_pb::SyncEntity& update); // Pass in name to avoid redundant UTF8 conversion. void UpdateServerFieldsFromUpdate( diff --git a/sync/engine/syncer_util_unittest.cc b/sync/engine/syncer_util_unittest.cc new file mode 100644 index 0000000..eec72ea --- /dev/null +++ b/sync/engine/syncer_util_unittest.cc @@ -0,0 +1,127 @@ +// Copyright 2014 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_util.h" + +#include "base/rand_util.h" +#include "sync/internal_api/public/base/unique_position.h" +#include "sync/protocol/sync.pb.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace syncer { + +class GetUpdatePositionTest : public ::testing::Test { + public: + GetUpdatePositionTest() { + InitUpdate(); + + // Init test_position to some valid position value, but don't assign + // it to the update just yet. + std::string pos_suffix = UniquePosition::RandomSuffix(); + test_position = UniquePosition::InitialPosition(pos_suffix); + } + + void InitUpdate() { + update.set_id_string("I"); + update.set_parent_id_string("P"); + update.set_version(10); + update.set_mtime(100); + update.set_ctime(100); + update.set_deleted(false); + update.mutable_specifics()->mutable_bookmark()->set_title("Chrome"); + update.mutable_specifics()->mutable_bookmark()-> + set_url("https://www.chrome.com"); + } + + void InitSuffixIngredients() { + update.set_originator_cache_guid("CacheGUID"); + update.set_originator_client_item_id("OrigID"); + } + + void InitProtoPosition() { + test_position.ToProto(update.mutable_unique_position()); + } + + void InitInt64Position(int64 pos_value) { + update.set_position_in_parent(pos_value); + } + + sync_pb::SyncEntity update; + UniquePosition test_position; +}; + +// Generate a suffix from originator client GUID and client-assigned ID. These +// values should always be present in updates sent down to the client, and +// combine to create a globally unique value. +TEST_F(GetUpdatePositionTest, SuffixFromUpdate) { + InitSuffixIngredients(); + + // Expect suffix is valid and consistent. + std::string suffix1 = GetUniqueBookmarkTagFromUpdate(update); + std::string suffix2 = GetUniqueBookmarkTagFromUpdate(update); + + EXPECT_EQ(suffix1, suffix2); + EXPECT_TRUE(UniquePosition::IsValidSuffix(suffix1)); +} + +// Receive an update without the ingredients used to make a consistent suffix. +// +// The server should never send us an update like this. If it does, +// that's a bug and it needs to be fixed. Still, we'd like to not +// crash and have fairly reasonable results in this scenario. +TEST_F(GetUpdatePositionTest, SuffixFromRandom) { + // Intentonally do not call InitSuffixIngredients() + + // Expect suffix is valid but inconsistent. + std::string suffix1 = GetUniqueBookmarkTagFromUpdate(update); + std::string suffix2 = GetUniqueBookmarkTagFromUpdate(update); + + EXPECT_NE(suffix1, suffix2); + EXPECT_TRUE(UniquePosition::IsValidSuffix(suffix1)); + EXPECT_TRUE(UniquePosition::IsValidSuffix(suffix2)); +} + +TEST_F(GetUpdatePositionTest, FromInt64) { + InitSuffixIngredients(); + InitInt64Position(10); + + std::string suffix = GetUniqueBookmarkTagFromUpdate(update); + + // Expect the result is valid. + UniquePosition pos = GetUpdatePosition(update, suffix); + EXPECT_TRUE(pos.IsValid()); + + // Expect the position had some effect on ordering. + EXPECT_TRUE(pos.LessThan( + UniquePosition::FromInt64(11, UniquePosition::RandomSuffix()))); +} + +TEST_F(GetUpdatePositionTest, FromProto) { + InitSuffixIngredients(); + InitInt64Position(10); + + std::string suffix = GetUniqueBookmarkTagFromUpdate(update); + + // The proto position is not set, so we should get one based on the int64. + // It should not match the proto we defined in the test harness. + UniquePosition int64_pos = GetUpdatePosition(update, suffix); + EXPECT_FALSE(int64_pos.Equals(test_position)); + + // Move the test harness' position value into the update proto. + // Expect that it takes precedence over the int64-based position. + InitProtoPosition(); + UniquePosition pos = GetUpdatePosition(update, suffix); + EXPECT_TRUE(pos.Equals(test_position)); +} + +TEST_F(GetUpdatePositionTest, FromNothing) { + // Init none of the ingredients necessary to make a position. + // Verify we still generate a valid position locally. + + std::string suffix = GetUniqueBookmarkTagFromUpdate(update); + UniquePosition pos = GetUpdatePosition(update, suffix); + EXPECT_TRUE(pos.IsValid()); +} + +} // namespace syncer -- cgit v1.1