sync: Improve handling of bad UniquePositions

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270308

[rlarocque, 2014-05-12 18:56:52]

Please review.

[maniscalco, 2014-05-12 19:21:49]

LGTM with some nits.

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
sync/engine/syncer_util.cc:282: LOG(ERROR) << "Update is missing requirements
for bookmark position."
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?

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
sync/engine/syncer_util.cc:284: return
base::RandBytesAsString(UniquePosition::kSuffixLength);
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?

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
File sync/engine/syncer_util_unittest.cc (right):

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
sync/engine/syncer_util_unittest.cc:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
nit: old copyright notice

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
sync/engine/syncer_util_unittest.cc:58: // Generate a suffix from originator
client GUID an original client-assigned
s/GUID an original/GUID and an original/ ?

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
sync/engine/syncer_util_unittest.cc:59: // ID.  These values should always be
present in updates sent down to the
s/down to/down from/ ?

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

You just saved us from a lot of flaky tests.  Thanks!

I've fixed the bug and a few nits.

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
sync/engine/syncer_util.cc:282: LOG(ERROR) << "Update is missing requirements
for bookmark position."
On 2014/05/12 19:21:50, maniscalco wrote:
> 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?

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.

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
sync/engine/syncer_util.cc:284: return
base::RandBytesAsString(UniquePosition::kSuffixLength);
On 2014/05/12 19:21:50, maniscalco wrote:
> 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?

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.

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
File sync/engine/syncer_util_unittest.cc (right):

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
sync/engine/syncer_util_unittest.cc:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
On 2014/05/12 19:21:50, maniscalco wrote:
> nit: old copyright notice

Done.

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
sync/engine/syncer_util_unittest.cc:58: // Generate a suffix from originator
client GUID an original client-assigned
On 2014/05/12 19:21:50, maniscalco wrote:
> s/GUID an original/GUID and an original/ ?

Should have been s/an/and/.  I've rewritten it slightly.

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util_...
sync/engine/syncer_util_unittest.cc:59: // ID.  These values should always be
present in updates sent down to the
On 2014/05/12 19:21:50, maniscalco wrote:
> s/down to/down from/ ?

s/server/client.

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

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
sync/engine/syncer_util.cc:282: LOG(ERROR) << "Update is missing requirements
for bookmark position."
On 2014/05/12 20:26:13, rlarocque wrote:
> On 2014/05/12 19:21:50, maniscalco wrote:
> > 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?
> 
> 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.  

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

https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
sync/engine/syncer_util.cc:284: return
base::RandBytesAsString(UniquePosition::kSuffixLength);
On 2014/05/12 20:26:13, rlarocque wrote:
> On 2014/05/12 19:21:50, maniscalco wrote:
> > 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?
> 
> 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.

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

[rlarocque, 2014-05-12 22:17:22]

On 2014/05/12 20:35:59, maniscalco wrote:
>
https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util.cc
> File sync/engine/syncer_util.cc (right):
> 
>
https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
> sync/engine/syncer_util.cc:282: LOG(ERROR) << "Update is missing requirements
> for bookmark position."
> On 2014/05/12 20:26:13, rlarocque wrote:
> > On 2014/05/12 19:21:50, maniscalco wrote:
> > > 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?
> > 
> > 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.  
> 
> SGTM, just wanted to make sure we explored our options here.
> 
>
https://codereview.chromium.org/278153002/diff/20001/sync/engine/syncer_util....
> sync/engine/syncer_util.cc:284: return
> base::RandBytesAsString(UniquePosition::kSuffixLength);
> On 2014/05/12 20:26:13, rlarocque wrote:
> > On 2014/05/12 19:21:50, maniscalco wrote:
> > > 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?
> > 
> > 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.
> 
> Cool, LMK when you've updated it and I'll take another look.

Oh, sorry, I thought I already had.  Fixed.

[maniscalco, 2014-05-12 23:37:59]

LGTM.  One comment nit.

https://codereview.chromium.org/278153002/diff/40001/sync/internal_api/public...
File sync/internal_api/public/base/unique_position.cc (right):

https://codereview.chromium.org/278153002/diff/40001/sync/internal_api/public...
sync/internal_api/public/base/unique_position.cc:42: // Users random data for
all but the last byte.  The last byte must not be
"must not be zero" instead of "must not be anything but zero" ?

[rlarocque, 2014-05-12 23:47:39]

https://codereview.chromium.org/278153002/diff/40001/sync/internal_api/public...
File sync/internal_api/public/base/unique_position.cc (right):

https://codereview.chromium.org/278153002/diff/40001/sync/internal_api/public...
sync/internal_api/public/base/unique_position.cc:42: // Users random data for
all but the last byte.  The last byte must not be
On 2014/05/12 23:37:59, maniscalco wrote:
> "must not be zero" instead of "must not be anything but zero" ?

Done.

[rlarocque, 2014-05-12 23:47:45]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-12 23:48:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/278153002/60001

[commit-bot: I haz the power, 2014-05-12 23:48:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-12 23:48:57]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rlarocque, 2014-05-12 23:55:53]

+Tim for full super star committer approval.

[tim (not reviewing), 2014-05-13 22:43:52]

lgtm

[rlarocque, 2014-05-13 22:45:35]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-13 22:56:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/278153002/80001

[commit-bot: I haz the power, 2014-05-14 02:58:25]

Change committed as 270308