[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=

Review URL: http://codereview.chromium.org/8402014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107686 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 84 insertions, 58 deletions

diff --git a/chrome/browser/sync/internal_api/base_node.cc b/chrome/browser/sync/internal_api/base_node.cc
index 2aa212c..31c039d 100644
--- a/chrome/browser/sync/internal_api/base_node.cc
+++ b/chrome/browser/sync/internal_api/base_node.cc
@@ -224,8 +224,11 @@ int64 BaseNode::GetSuccessorId() const {
 int64 BaseNode::GetFirstChildId() const {
   syncable::Directory* dir = GetTransaction()->GetLookup();
   syncable::BaseTransaction* trans = GetTransaction()->GetWrappedTrans();
-  syncable::Id id_string =
-      dir->GetFirstChildId(trans, GetEntry()->Get(syncable::ID));
+  syncable::Id id_string;
+  // TODO(akalin): Propagate up the error further (see
+  // http://crbug.com/100907).
+  CHECK(dir->GetFirstChildId(trans,
+                             GetEntry()->Get(syncable::ID), &id_string));
   if (id_string.IsRoot())
     return kInvalidId;
   return IdToMetahandle(GetTransaction()->GetWrappedTrans(), id_string);
diff --git a/chrome/browser/sync/internal_api/change_reorder_buffer.cc b/chrome/browser/sync/internal_api/change_reorder_buffer.cc
index d030e5c..090f744 100644
--- a/chrome/browser/sync/internal_api/change_reorder_buffer.cc
+++ b/chrome/browser/sync/internal_api/change_reorder_buffer.cc
@@ -121,8 +121,9 @@ ChangeReorderBuffer::ChangeReorderBuffer() {
 ChangeReorderBuffer::~ChangeReorderBuffer() {
 }
 
-ImmutableChangeRecordList ChangeReorderBuffer::GetAllChangesInTreeOrder(
-    const BaseTransaction* sync_trans) {
+bool ChangeReorderBuffer::GetAllChangesInTreeOrder(
+    const BaseTransaction* sync_trans,
+    ImmutableChangeRecordList* changes) {
   syncable::BaseTransaction* trans = sync_trans->GetWrappedTrans();
 
   // Step 1: Iterate through the operations, doing three things:
@@ -200,8 +201,12 @@ ImmutableChangeRecordList ChangeReorderBuffer::GetAllChangesInTreeOrder(
       // There were ordering changes on the children of this parent, so
       // enumerate all the children in the sibling order.
       syncable::Entry parent(trans, syncable::GET_BY_HANDLE, next);
-      syncable::Id id = trans->directory()->
-          GetFirstChildId(trans, parent.Get(syncable::ID));
+      syncable::Id id;
+      if (!trans->directory()->GetFirstChildId(
+              trans, parent.Get(syncable::ID), &id)) {
+        *changes = ImmutableChangeRecordList();
+        return false;
+      }
       while (!id.IsRoot()) {
         syncable::Entry child(trans, syncable::GET_BY_ID, id);
         CHECK(child.good());
@@ -217,7 +222,8 @@ ImmutableChangeRecordList ChangeReorderBuffer::GetAllChangesInTreeOrder(
     }
   }
 
-  return ImmutableChangeRecordList(&changelist);
+  *changes = ImmutableChangeRecordList(&changelist);
+  return true;
 }
 
 }  // namespace sync_api
diff --git a/chrome/browser/sync/internal_api/change_reorder_buffer.h b/chrome/browser/sync/internal_api/change_reorder_buffer.h
index c78bcac..5bbdccf 100644
--- a/chrome/browser/sync/internal_api/change_reorder_buffer.h
+++ b/chrome/browser/sync/internal_api/change_reorder_buffer.h
@@ -13,6 +13,7 @@
 #include <map>
 #include <vector>
 
+#include "base/compiler_specific.h"
 #include "base/memory/linked_ptr.h"
 #include "chrome/browser/sync/internal_api/base_transaction.h"
 #include "chrome/browser/sync/internal_api/change_record.h"
@@ -84,11 +85,13 @@ class ChangeReorderBuffer {
     return operations_.empty();
   }
 
-  // Output a reordered list of changes to |changelist| using the items that
-  // were pushed into the reorder buffer. |sync_trans| is used to determine the
-  // ordering.
-  ImmutableChangeRecordList
-      GetAllChangesInTreeOrder(const BaseTransaction* sync_trans);
+  // Output a reordered list of changes to |changes| using the items
+  // that were pushed into the reorder buffer. |sync_trans| is used to
+  // determine the ordering.  Returns true if successful, or false if
+  // an error was encountered.
+  bool GetAllChangesInTreeOrder(
+      const BaseTransaction* sync_trans,
+      ImmutableChangeRecordList* changes) WARN_UNUSED_RESULT;
 
  private:
   class Traversal;
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index c76af38..9046a73 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -1401,8 +1401,11 @@ ModelTypeBitSet SyncManager::SyncInternal::HandleTransactionEndingChangeEvent(
     if (change_buffers_[type].IsEmpty())
       continue;
 
-    ImmutableChangeRecordList ordered_changes =
-        change_buffers_[type].GetAllChangesInTreeOrder(&read_trans);
+    ImmutableChangeRecordList ordered_changes;
+    // TODO(akalin): Propagate up the error further (see
+    // http://crbug.com/100907).
+    CHECK(change_buffers_[type].GetAllChangesInTreeOrder(&read_trans,
+                                                         &ordered_changes));
     if (!ordered_changes.Get().empty()) {
       change_delegate_->
           OnChangesApplied(type, &read_trans, ordered_changes);
diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc
index fd993c2..d34c4ac 100644
--- a/chrome/browser/sync/internal_api/syncapi_unittest.cc
+++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc
@@ -1252,18 +1252,21 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
   {
     ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     EXPECT_EQ(Cryptographer::SensitiveTypes(), GetEncryptedTypes(&trans));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::BOOKMARKS,
-                                                   false /* not encrypted */));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::SESSIONS,
-                                                   false /* not encrypted */));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::THEMES,
-                                                   false /* not encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::BOOKMARKS,
+        false /* not encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::SESSIONS,
+        false /* not encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::THEMES,
+        false /* not encrypted */));
   }
 
   EXPECT_CALL(observer_,
@@ -1274,18 +1277,21 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
   {
     ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     EXPECT_EQ(GetAllRealModelTypes(), GetEncryptedTypes(&trans));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::BOOKMARKS,
-                                                   true /* is encrypted */));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::SESSIONS,
-                                                   true /* is encrypted */));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::THEMES,
-                                                   true /* is encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::BOOKMARKS,
+        true /* is encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::SESSIONS,
+        true /* is encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::THEMES,
+        true /* is encrypted */));
   }
 
   // Trigger's a ReEncryptEverything with new passphrase.
@@ -1297,18 +1303,21 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
   {
     ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     EXPECT_EQ(GetAllRealModelTypes(), GetEncryptedTypes(&trans));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::BOOKMARKS,
-                                                   true /* is encrypted */));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::SESSIONS,
-                                                   true /* is encrypted */));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::THEMES,
-                                                   true /* is encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::BOOKMARKS,
+        true /* is encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::SESSIONS,
+        true /* is encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::THEMES,
+        true /* is encrypted */));
   }
   // Calling EncryptDataTypes with an empty encrypted types should not trigger
   // a reencryption and should just notify immediately.
@@ -1445,10 +1454,11 @@ TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) {
 
   {
     ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::BOOKMARKS,
-                                                   false /* not encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::BOOKMARKS,
+        false /* not encrypted */));
   }
 
   EXPECT_CALL(observer_,
@@ -1460,10 +1470,11 @@ TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) {
   {
     ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     EXPECT_EQ(GetAllRealModelTypes(), GetEncryptedTypes(&trans));
-    EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
-                                                   trans.GetCryptographer(),
-                                                   syncable::BOOKMARKS,
-                                                   true /* is encrypted */));
+    EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest(
+        trans.GetWrappedTrans(),
+        trans.GetCryptographer(),
+        syncable::BOOKMARKS,
+        true /* is encrypted */));
 
     ReadNode node(&trans);
     EXPECT_TRUE(node.InitByIdLookup(node_id1));