diff options
-rw-r--r-- | sync/engine/syncer_util.cc | 12 | ||||
-rw-r--r-- | sync/engine/syncer_util.h | 9 | ||||
-rw-r--r-- | sync/engine/syncer_util_unittest.cc | 127 | ||||
-rw-r--r-- | sync/internal_api/public/base/unique_position.cc | 11 | ||||
-rw-r--r-- | sync/internal_api/public/base/unique_position.h | 4 | ||||
-rw-r--r-- | sync/sync_tests.gypi | 1 | ||||
-rw-r--r-- | sync/syncable/directory_backing_store.cc | 10 | ||||
-rw-r--r-- | sync/syncable/directory_unittest.cc | 37 | ||||
-rw-r--r-- | sync/syncable/syncable_unittest.cc | 7 |
9 files changed, 14 insertions, 204 deletions
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc index c7d93c1..2235734 100644 --- a/sync/engine/syncer_util.cc +++ b/sync/engine/syncer_util.cc @@ -12,7 +12,6 @@ #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" @@ -279,9 +278,7 @@ UpdateAttemptResponse AttemptToUpdateEntry( std::string GetUniqueBookmarkTagFromUpdate(const sync_pb::SyncEntity& update) { if (!update.has_originator_cache_guid() || !update.has_originator_client_item_id()) { - LOG(ERROR) << "Update is missing requirements for bookmark position." - << " This is a server bug."; - return UniquePosition::RandomSuffix(); + return std::string(); } return syncable::GenerateSyncableBookmarkHash( @@ -298,8 +295,7 @@ UniquePosition GetUpdatePosition(const sync_pb::SyncEntity& update, } else if (update.has_position_in_parent()) { return UniquePosition::FromInt64(update.position_in_parent(), suffix); } else { - LOG(ERROR) << "No position information in update. This is a server bug."; - return UniquePosition::FromInt64(0, suffix); + return UniquePosition::CreateInvalid(); } } @@ -345,6 +341,10 @@ 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 f39ad4b..575ab11 100644 --- a/sync/engine/syncer_util.h +++ b/sync/engine/syncer_util.h @@ -12,7 +12,6 @@ #include <string> #include <vector> -#include "sync/base/sync_export.h" #include "sync/engine/syncer.h" #include "sync/engine/syncer_types.h" #include "sync/syncable/entry_kernel.h" @@ -59,14 +58,12 @@ UpdateAttemptResponse AttemptToUpdateEntry( // // Will return an invalid position if no valid position can be constructed, or // if this type does not support positioning. -SYNC_EXPORT_PRIVATE UniquePosition GetUpdatePosition( - const sync_pb::SyncEntity& update, - const std::string& suffix); +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. -SYNC_EXPORT_PRIVATE std::string GetUniqueBookmarkTagFromUpdate( - const sync_pb::SyncEntity& update); +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 deleted file mode 100644 index eec72ea..0000000 --- a/sync/engine/syncer_util_unittest.cc +++ /dev/null @@ -1,127 +0,0 @@ -// 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 diff --git a/sync/internal_api/public/base/unique_position.cc b/sync/internal_api/public/base/unique_position.cc index 2d41614..40bab6e 100644 --- a/sync/internal_api/public/base/unique_position.cc +++ b/sync/internal_api/public/base/unique_position.cc @@ -6,7 +6,6 @@ #include "base/basictypes.h" #include "base/logging.h" -#include "base/rand_util.h" #include "base/stl_util.h" #include "base/strings/string_number_conversions.h" #include "sync/protocol/unique_position.pb.h" @@ -22,8 +21,7 @@ bool UniquePosition::IsValidSuffix(const std::string& suffix) { // The suffix must be exactly the specified length, otherwise unique suffixes // are not sufficient to guarantee unique positions (because prefix + suffix // == p + refixsuffix). - return suffix.length() == kSuffixLength - && suffix[kSuffixLength-1] != 0; + return suffix.length() == kSuffixLength; } // static. @@ -38,13 +36,6 @@ bool UniquePosition::IsValidBytes(const std::string& bytes) { } // static. -std::string UniquePosition::RandomSuffix() { - // Users random data for all but the last byte. The last byte must not be - // zero. We arbitrarily set it to 0x7f. - return base::RandBytesAsString(kSuffixLength - 1) + "\x7f"; -} - -// static. UniquePosition UniquePosition::CreateInvalid() { UniquePosition pos; DCHECK(!pos.IsValid()); diff --git a/sync/internal_api/public/base/unique_position.h b/sync/internal_api/public/base/unique_position.h index b844b82..eee5324 100644 --- a/sync/internal_api/public/base/unique_position.h +++ b/sync/internal_api/public/base/unique_position.h @@ -46,10 +46,6 @@ class SYNC_EXPORT_PRIVATE UniquePosition { static bool IsValidSuffix(const std::string& suffix); static bool IsValidBytes(const std::string& bytes); - // Returns a valid, but mostly random suffix. - // Avoid using this; it can lead to inconsistent sort orderings if misused. - static std::string RandomSuffix(); - // Returns an invalid position. static UniquePosition CreateInvalid(); diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi index 94cd52d..ea15c76 100644 --- a/sync/sync_tests.gypi +++ b/sync/sync_tests.gypi @@ -302,7 +302,6 @@ 'engine/sync_scheduler_unittest.cc', 'engine/syncer_proto_util_unittest.cc', 'engine/syncer_unittest.cc', - 'engine/syncer_util_unittest.cc', 'js/js_event_details_unittest.cc', 'js/sync_js_controller_unittest.cc', 'protocol/proto_enum_conversions_unittest.cc', diff --git a/sync/syncable/directory_backing_store.cc b/sync/syncable/directory_backing_store.cc index 55a01e6..ec28a53 100644 --- a/sync/syncable/directory_backing_store.cc +++ b/sync/syncable/directory_backing_store.cc @@ -123,16 +123,6 @@ scoped_ptr<EntryKernel> UnpackEntry(sql::Statement* statement) { kernel->mutable_ref(static_cast<AttachmentMetadataField>(i)).ParseFromArray( statement->ColumnBlob(i), statement->ColumnByteLength(i)); } - - // Sanity check on positions. We risk strange and rare crashes if our - // assumptions about unique position values are broken. - if (kernel->ShouldMaintainPosition() && - !kernel->ref(UNIQUE_POSITION).IsValid()) { - DVLOG(1) << "Unpacked invalid position on an entity that should have a " - << "valid position. Assuming the DB is corrupt."; - return scoped_ptr<EntryKernel>(); - } - return kernel.Pass(); } diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc index 6b6b8a2..f58f54f 100644 --- a/sync/syncable/directory_unittest.cc +++ b/sync/syncable/directory_unittest.cc @@ -81,11 +81,6 @@ DirOpenResult SyncableDirectoryTest::ReopenDirectory() { DirOpenResult open_result = dir_->Open(kDirectoryName, &delegate_, NullTransactionObserver()); - - if (open_result != OPENED) { - dir_.reset(); - } - return open_result; } @@ -1226,38 +1221,6 @@ TEST_F(SyncableDirectoryTest, PositionWithNullSurvivesSaveAndReload) { } } -// Any item with BOOKMARKS in their local specifics should have a valid local -// unique position. If there is an item in the loaded DB that does not match -// this criteria, we consider the whole DB to be corrupt. -TEST_F(SyncableDirectoryTest, BadPositionCountsAsCorruption) { - TestIdFactory id_factory; - - { - WriteTransaction trans(FROM_HERE, UNITTEST, dir().get()); - - MutableEntry parent(&trans, CREATE, BOOKMARKS, id_factory.root(), "parent"); - parent.PutIsDir(true); - parent.PutIsUnsynced(true); - - // The code is littered with DCHECKs that try to stop us from doing what - // we're about to do. Our work-around is to create a bookmark based on - // a server update, then update its local specifics without updating its - // local unique position. - - MutableEntry child( - &trans, CREATE_NEW_UPDATE_ITEM, id_factory.MakeServer("child")); - sync_pb::EntitySpecifics specifics; - AddDefaultFieldValue(BOOKMARKS, &specifics); - child.PutIsUnappliedUpdate(true); - child.PutSpecifics(specifics); - - EXPECT_TRUE(child.ShouldMaintainPosition()); - EXPECT_TRUE(!child.GetUniquePosition().IsValid()); - } - - EXPECT_EQ(FAILED_DATABASE_CORRUPT, SimulateSaveAndReloadDir()); -} - TEST_F(SyncableDirectoryTest, General) { int64 written_metahandle; const Id id = TestIdFactory::FromNumber(99); diff --git a/sync/syncable/syncable_unittest.cc b/sync/syncable/syncable_unittest.cc index 227b406..021e4bd 100644 --- a/sync/syncable/syncable_unittest.cc +++ b/sync/syncable/syncable_unittest.cc @@ -344,7 +344,7 @@ TEST_F(OnDiskSyncableDirectoryTest, specifics.mutable_bookmark()->set_favicon("PNG"); specifics.mutable_bookmark()->set_url("http://nowhere"); create.PutSpecifics(specifics); - update.PutServerSpecifics(specifics); + update.PutSpecifics(specifics); create_pre_save = create.GetKernelCopy(); update_pre_save = update.GetKernelCopy(); create_id = create.GetId(); @@ -379,9 +379,10 @@ TEST_F(OnDiskSyncableDirectoryTest, (i == TRANSACTION_VERSION ? 1 : 0), create_post_save.ref((Int64Field)i)) << "int64 field #" << i << " changed during save/load"; - EXPECT_EQ(update_pre_save.ref((Int64Field)i), + EXPECT_EQ(update_pre_save.ref((Int64Field)i) + + (i == TRANSACTION_VERSION ? 1 : 0), update_post_save.ref((Int64Field)i)) - << "int64 field #" << i << " changed during save/load"; + << "int64 field #" << i << " changed during save/load"; } for ( ; i < TIME_FIELDS_END ; ++i) { EXPECT_EQ(create_pre_save.ref((TimeField)i), |