When receive a bookmark with an invalid unique position from sync

server, we create an unique position for this bookmark.

BUG=332371

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

Cr-Commit-Position: refs/heads/master@{#346158}

3 files changed, 42 insertions, 1 deletions

diff --git a/sync/engine/commit_util.cc b/sync/engine/commit_util.cc
index ed298b3..2515216 100644
--- a/sync/engine/commit_util.cc
+++ b/sync/engine/commit_util.cc
@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/debug/dump_without_crashing.h"
 #include "base/strings/string_util.h"
 #include "sync/engine/syncer_proto_util.h"
 #include "sync/internal_api/public/base/attachment_id_proto.h"
@@ -184,6 +185,10 @@ void BuildCommitItem(
           meta_entry.GetUniquePosition().ToInt64());
       meta_entry.GetUniquePosition().ToProto(
           sync_entry->mutable_unique_position());
+      if (!meta_entry.GetUniquePosition().IsValid()) {
+        // Should never upload invalid unique position for bookmark to server.
+        base::debug::DumpWithoutCrashing();
+      }
     }
     // Always send specifics for bookmarks.
     SetEntrySpecifics(meta_entry, sync_entry);
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index 505aebf..b4f1535 100644
--- a/sync/engine/syncer_util.cc
+++ b/sync/engine/syncer_util.cc
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/base64.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/location.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
@@ -290,7 +291,16 @@ UniquePosition GetUpdatePosition(const sync_pb::SyncEntity& update,
   if (!(SyncerProtoUtil::ShouldMaintainPosition(update))) {
     return UniquePosition::CreateInvalid();
   } else if (update.has_unique_position()) {
-    return UniquePosition::FromProto(update.unique_position());
+    UniquePosition proto_position =
+        UniquePosition::FromProto(update.unique_position());
+    if (!proto_position.IsValid()) {
+      // If download a bookmark with an invalid unique position, creating valid
+      // one for the bookmark. crbug.com/332371: This is a server bug, but for
+      // avoid crash, we made this fix.
+      base::debug::DumpWithoutCrashing();
+      return UniquePosition::FromInt64(0, suffix);
+    }
+    return proto_position;
   } else if (update.has_position_in_parent()) {
     return UniquePosition::FromInt64(update.position_in_parent(), suffix);
   } else {
diff --git a/sync/engine/syncer_util_unittest.cc b/sync/engine/syncer_util_unittest.cc
index 9fe3d4b..b4026d0 100644
--- a/sync/engine/syncer_util_unittest.cc
+++ b/sync/engine/syncer_util_unittest.cc
@@ -201,4 +201,30 @@ TEST_F(GetUpdatePositionTest, UpdateServerFieldsFromInvalidUpdateTest) {
   EXPECT_TRUE(target.GetServerUniquePosition().IsValid());
 }
 
+TEST_F(GetUpdatePositionTest, UpdateServerFieldsFromInvalidUniquePositionTest) {
+  InitSuffixIngredients();  // Initialize update with valid data.
+  sync_pb::SyncEntity invalid_update(update);
+
+  // Create and Setup an invalid position
+  sync_pb::UniquePosition* invalid_position = new sync_pb::UniquePosition();
+  invalid_position->set_value("");
+  invalid_update.set_allocated_unique_position(invalid_position);
+
+  std::string root_server_id = syncable::Id::GetRoot().GetServerId();
+  int64 handle = entry_factory()->CreateUnappliedNewBookmarkItemWithParent(
+      "I", DefaultBookmarkSpecifics(), root_server_id);
+
+  syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST, directory());
+  syncable::MutableEntry target(&trans, syncable::GET_BY_HANDLE, handle);
+
+  // Before update, target has invalid bookmark tag and unique position.
+  EXPECT_FALSE(UniquePosition::IsValidSuffix(target.GetUniqueBookmarkTag()));
+  EXPECT_FALSE(target.GetServerUniquePosition().IsValid());
+  UpdateServerFieldsFromUpdate(&target, invalid_update, "name");
+
+  // After update, target has valid bookmark tag and unique position.
+  EXPECT_TRUE(UniquePosition::IsValidSuffix(target.GetUniqueBookmarkTag()));
+  EXPECT_TRUE(target.GetServerUniquePosition().IsValid());
+}
+
 }  // namespace syncer