[Sync] Make GetFirstChildId return a flag indicating success

chrome/browser/sync/syncable/syncable.cc

Index: chrome/browser/sync/syncable/syncable.cc
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index 73ee43b26b7744fc2a6215ae5f5c53ea7d15b4b5..e6bf7209f144969fce1b5cbd8d5a7a18f93ad2db 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -1717,7 +1717,9 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
     predecessor.Put(NEXT_ID, Get(ID));
   } else {
     syncable::Directory* dir = trans()->directory();
-    successor_id = dir->GetFirstChildId(trans(), Get(PARENT_ID));
+    if (!dir->GetFirstChildId(trans(), Get(PARENT_ID), &successor_id)) {
+      return false;
+    }
   }
   if (!successor_id.IsRoot()) {
     MutableEntry successor(write_transaction(), GET_BY_ID, successor_id);
@@ -1770,55 +1772,52 @@ bool Directory::HasChildren(BaseTransaction* trans, const Id& id) {
   return (GetPossibleFirstChild(lock, id) != NULL);
 }
 
-Id Directory::GetFirstChildId(BaseTransaction* trans,
-                              const Id& parent_id) {
+bool Directory::GetFirstChildId(BaseTransaction* trans,
+                                const Id& parent_id,
+                                Id* first_child_id) {
   ScopedKernelLock lock(this);
   EntryKernel* entry = GetPossibleFirstChild(lock, parent_id);
-  if (!entry)
-    return Id();
+  if (!entry) {
+    *first_child_id = Id();
+    return true;
+  }
 
   // Walk to the front of the list; the server position ordering
   // is commonly identical to the linked-list ordering, but pending
   // unsynced or unapplied items may diverge.
   while (!entry->ref(PREV_ID).IsRoot()) {
-    // TODO(akalin): Gracefully handle GetEntryById returning NULL
-    // (http://crbug.com/100907).
     entry = GetEntryById(entry->ref(PREV_ID), &lock);
+    if (!entry) {

[rlarocque, 2011/10/27 02:11:33]

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:18]

What returned ID?  If this function returns false, the caller should ignore
what's filled in first_child_id.
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.

+      *first_child_id = Id();
+      return false;
+    }
   }
-  return entry->ref(ID);
+  *first_child_id = entry->ref(ID);
+  return true;
 }
 
-Id Directory::GetLastChildId(BaseTransaction* trans,
-                             const Id& parent_id) {
+bool Directory::GetLastChildIdForTest(
+    BaseTransaction* trans, const Id& parent_id, Id* last_child_id) {
   ScopedKernelLock lock(this);
-  // We can use the server positional ordering as a hint because it's generally
-  // in sync with the local (linked-list) positional ordering, and we have an
-  // index on it.
-  ParentIdChildIndex::iterator begin_range =
-      GetParentChildIndexLowerBound(lock, parent_id);
-  ParentIdChildIndex::iterator candidate =
-      GetParentChildIndexUpperBound(lock, parent_id);
-
-  while (begin_range != candidate) {
-    --candidate;
-    EntryKernel* entry = *candidate;
+  EntryKernel* entry = GetPossibleLastChildForTest(lock, parent_id);
+  if (!entry) {
+    *last_child_id = Id();
+    return true;
+  }
 
-    // Filter out self-looped items, which are temporarily not in the child
-    // ordering.
-    if (entry->ref(NEXT_ID).IsRoot() ||
-        entry->ref(NEXT_ID) != entry->ref(PREV_ID)) {
-      // Walk to the back of the list; the server position ordering
-      // is commonly identical to the linked-list ordering, but pending
-      // unsynced or unapplied items may diverge.
-      while (!entry->ref(NEXT_ID).IsRoot())
-        // TODO(akalin): Gracefully handle GetEntryById returning NULL
-        // (http://crbug.com/100907).
-        entry = GetEntryById(entry->ref(NEXT_ID), &lock);
-      return entry->ref(ID);
+  // Walk to the back of the list; the server position ordering
+  // is commonly identical to the linked-list ordering, but pending
+  // unsynced or unapplied items may diverge.
+  while (!entry->ref(NEXT_ID).IsRoot()) {
+    entry = GetEntryById(entry->ref(NEXT_ID), &lock);
+    if (!entry) {
+      *last_child_id = Id();
+      return false;
     }
   }
-  // There were no children in the linked list.
-  return Id();
+
+  *last_child_id = entry->ref(ID);
+  return true;
 }
 
 Id Directory::ComputePrevIdFromServerPosition(
@@ -2001,4 +2000,29 @@ EntryKernel* Directory::GetPossibleFirstChild(
   return NULL;
 }
 
+EntryKernel* Directory::GetPossibleLastChildForTest(
+    const ScopedKernelLock& lock, const Id& parent_id) {
+  // We can use the server positional ordering as a hint because it's generally
+  // in sync with the local (linked-list) positional ordering, and we have an
+  // index on it.
+  ParentIdChildIndex::iterator begin_range =
+      GetParentChildIndexLowerBound(lock, parent_id);
+  ParentIdChildIndex::iterator candidate =
+      GetParentChildIndexUpperBound(lock, parent_id);
+
+  while (begin_range != candidate) {
+    --candidate;
+    EntryKernel* entry = *candidate;
+
+    // Filter out self-looped items, which are temporarily not in the child
+    // ordering.
+    if (entry->ref(NEXT_ID).IsRoot() ||
+        entry->ref(NEXT_ID) != entry->ref(PREV_ID)) {
+      return entry;
+    }
+  }
+  // There were no children in the linked list.
+  return NULL;
+}
+
 }  // namespace syncable