[Sync] Make GetFirstChildId return a flag indicating success

[Sync] Make GetFirstChildId return a flag indicating success

Propagate that flag up a few levels and add CHECKs.  Since the old code
crashed, CHECKing is no worse.

Rename some functions to add *ForTest suffixes.

BUG=100907
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107686

[akalin, 2011-10-27 01:15:18]

+rlarocque and +zea both for review

[rlarocque, 2011-10-27 02:10:52]

On 2011/10/27 01:15:18, akalin wrote:
> +rlarocque and +zea both for review

LGTM.  

You did add CHECK()s to *all* the current uses of GetFirstChildId, right?

[rlarocque, 2011-10-27 02:11:32]

And here's the comment I intended to publish with that last message.

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.cc:1790: if (!entry) {
I'm not sure how I feel about this.  We should never get a NULL when we evaluate
GetEntryByID(entry->ref(PREV_ID)) on a valid entry.  We could end up corrupting
the database even further if we ignore the return value and accept the returned
id.  

Of course, you know that just as well as I do.  I suppose I'm OK with it if you
are.

[akalin, 2011-10-27 02:22:13]

> You did add CHECK()s to *all* the current uses of GetFirstChildId, right?

No, since some of the callers themselves can return errors, so I just propagate
them up when I can.

But if you're asking if I found all the callers and have them check the return
value, then I'm pretty confident I did, given that I changed the type signature
and so any calls I didn't touch would cause a compile failure.

[akalin, 2011-10-27 02:22:18]

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.cc:1790: if (!entry) {
On 2011/10/27 02:11:33, rlarocque wrote:
> I'm not sure how I feel about this.  We should never get a NULL when we
evaluate
> GetEntryByID(entry->ref(PREV_ID)) on a valid entry.  We could end up
corrupting
> the database even further if we ignore the return value and accept the
returned
> id.  

What returned ID?  If this function returns false, the caller should ignore
what's filled in first_child_id.

> 
> Of course, you know that just as well as I do.  I suppose I'm OK with it if
you
> are.  

The point of this patch is to propagate up the error.  Currently, it
CHECK-fails, but eventually, the error will end up causing an unrecoverable
error, so there should be no room to corrupt the DB even further.

[rlarocque, 2011-10-27 04:25:31]

On 2011/10/27 02:22:18, akalin wrote:
>
http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
> File chrome/browser/sync/syncable/syncable.cc (right):
> 
>
http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
> chrome/browser/sync/syncable/syncable.cc:1790: if (!entry) {
> On 2011/10/27 02:11:33, rlarocque wrote:
> > I'm not sure how I feel about this.  We should never get a NULL when we
> evaluate
> > GetEntryByID(entry->ref(PREV_ID)) on a valid entry.  We could end up
> corrupting
> > the database even further if we ignore the return value and accept the
> returned
> > id.  
> 
> What returned ID?  If this function returns false, the caller should ignore
> what's filled in first_child_id.
> 
> > 
> > Of course, you know that just as well as I do.  I suppose I'm OK with it if
> you
> > are.  
> 
> The point of this patch is to propagate up the error.  Currently, it
> CHECK-fails, but eventually, the error will end up causing an unrecoverable
> error, so there should be no room to corrupt the DB even further.

Doh.  I missed the part where the function arguments changed too, not just the
return value.  it looks like you have taken care of all the instances where it
is called, and had the compiler make sure of it.  

Definitely LGTM.

[Nicolas Zea, 2011-10-27 18:28:30]

LGTM with nits

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/change_reorder_buffer.h (right):

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/change_reorder_buffer.h:92:
ImmutableChangeRecordList* changes);
consider WARN_UNUSED_RESULT

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:932: bool
GetFirstChildId(BaseTransaction* trans, const Id& parent_id,
WARN_UNUSED_RESULT for new methods that return error bool?

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:939: bool GetLastChildIdForTest(
Prefer having first argument on this line and others lining up with it for
consistency

[akalin, 2011-10-27 21:11:42]

Addressed all comments, committing after tryjobs

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/change_reorder_buffer.h (right):

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/change_reorder_buffer.h:92:
ImmutableChangeRecordList* changes);
On 2011/10/27 18:28:30, nzea wrote:
> consider WARN_UNUSED_RESULT

Done.

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:932: bool
GetFirstChildId(BaseTransaction* trans, const Id& parent_id,
On 2011/10/27 18:28:30, nzea wrote:
> WARN_UNUSED_RESULT for new methods that return error bool?

Done.

http://codereview.chromium.org/8402014/diff/4001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:939: bool GetLastChildIdForTest(
On 2011/10/27 18:28:30, nzea wrote:
> Prefer having first argument on this line and others lining up with it for
> consistency

Done.

[commit-bot: I haz the power, 2011-10-28 00:12:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8402014/7002

[commit-bot: I haz the power, 2011-10-28 00:12:41]

Presubmit check for 8402014-7002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
You might be calling functions intended only for testing from
production code. Please verify that the following usages are OK,
and email joi@chromium.org if you are seeing false positives:
  chrome/browser/sync/engine/nigori_util.cc:83
    bool VerifyDataTypeEncryptionForTest( \
  chrome/browser/sync/syncable/syncable.cc:1801
    EntryKernel* entry = GetPossibleLastChildForTest(lock, parent_id);

Presubmit checks took 1.8s to calculate.