Revert 270308 "sync: Improve handling of bad UniquePositions"

Reverting due to possible build breakage: http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%20LSan%20Tests%20%282%29&number=2633

> 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

TBR=rlarocque@chromium.org

Review URL: https://codereview.chromium.org/270543005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@270324 0039d316-1c4b-4281-b951-d872f2087c98

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),