sync: Improve handling of bad UniquePositions

sync/engine/syncer_util.cc

Index: sync/engine/syncer_util.cc
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index 2235734aedbacbfb0a36c00090646ae62133fdb8..96ce1047ad765dd69a8709f30845d24487648381 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."

[maniscalco, 2014/05/12 19:21:50]

It may not be worth it, but I'm wondering if there's a way we can find out how
frequently we reach this case.  I assume it's very rare.

WDYT about using DumpWithoutCrashing (or a modified
ChromeReportUnrecoverableError) here?  Is there another tool we could use to get
back channel that reports on how frequently this happens?  Does a UMA counter
make sense?

[rlarocque, 2014/05/12 20:26:13]

I wouldn't want to DumpWithoutCrashing() here, since it could happen fairly
often if the server is actually broken.  Doing that once per received bookmark
is probably a bad idea.

The alternative would be an UnrecoverableError, but I don't trust that it's
sufficiently plumbed through here that it will actually work.  It's also
impossible to test.

UMA counters might be a better idea, but I don't think that anyone looks at them
often enough that we would notice if something was wrong.  

[maniscalco, 2014/05/12 20:35:59]

SGTM, just wanted to make sure we explored our options here.

+               << " This is a server bug.";
+    return base::RandBytesAsString(UniquePosition::kSuffixLength);

[maniscalco, 2014/05/12 19:21:50]

In the non-error case, the return value of this function is a base64 string.  Do
you think it's worth ensuring that the result in the error case is also a base64
string?  In other words, do you think returning a string containing something
other than the base64 char set might break some existing code?

[rlarocque, 2014/05/12 20:26:13]

That's a very good point.

It's not necessary that this be a base64 string.  The only requirements are that
it be the right length, and that it not end in a 'NULL' value.  

This implementation has a 1 in 256 chance of violating the second constraint. 
Thanks for catching it here.  I'm glad this didn't make it to the build bots.

[maniscalco, 2014/05/12 20:35:59]

Cool, LMK when you've updated it and I'll take another look.

   }
 
   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();
   }
 }