[Sync] Make GetFirstChildId return a flag indicating success

chrome/browser/sync/syncable/syncable.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/syncable/syncable.h"
 
 #include <algorithm>
 #include <cstring>
 #include <functional>
 #include <iomanip>
@@ skipping 1699 matching lines @@
       LOG(ERROR) << "Predecessor is not good : "
                  << predecessor_id.GetServerId();
       return false;
     }
     if (predecessor.Get(PARENT_ID) != Get(PARENT_ID))
       return false;
     successor_id = predecessor.Get(NEXT_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);
     if (!successor.good()) {
       LOG(ERROR) << "Successor is not good: "
                  << successor_id.GetServerId();
       return false;
     }
     if (successor.Get(PARENT_ID) != Get(PARENT_ID))
       return false;
@@ skipping 32 matching lines @@
   }
   DCHECK_LT(result, 0);
   return Id::CreateFromClientString(base::Int64ToString(result));
 }
 
 bool Directory::HasChildren(BaseTransaction* trans, const Id& id) {
   ScopedKernelLock lock(this);
   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);
+  EntryKernel* entry = GetPossibleLastChildForTest(lock, parent_id);
+  if (!entry) {
+    *last_child_id = Id();
+    return true;
+  }
 
-  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)) {
-      // 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(
     const EntryKernel* entry,
     const syncable::Id& parent_id) {
   ScopedKernelLock lock(this);
 
   // Find the natural insertion point in the parent_id_child_index, and
   // work back from there, filtering out ineligible candidates.
   ParentIdChildIndex::iterator sibling = LocateInParentChildIndex(lock,
@@ skipping 162 matching lines @@
     // ordering.
     if (entry->ref(PREV_ID).IsRoot() ||
         entry->ref(PREV_ID) != entry->ref(NEXT_ID)) {
       return entry;
     }
   }
   // There were no children in the linked list.
   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