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

3 files changed, 4 insertions, 50 deletions

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