[Sync] Handle invalid specifics field numbers gracefully

Change GetModelTypeFromSpecificsFieldNumber() to not NOTREACHED() on an
unknown field number.  Instead, have callers compare the return value
to UNSPECIFIED and handle that case.

BUG=165171


Review URL: https://chromiumcodereview.appspot.com/11490018

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

5 files changed, 55 insertions, 14 deletions

diff --git a/sync/engine/store_timestamps_command.cc b/sync/engine/store_timestamps_command.cc
index bab995e..6f381b0 100644
--- a/sync/engine/store_timestamps_command.cc
+++ b/sync/engine/store_timestamps_command.cc
@@ -28,15 +28,14 @@ SyncerError StoreTimestampsCommand::ExecuteImpl(
   // state.
   ModelTypeSet forward_progress_types;
   for (int i = 0; i < updates.new_progress_marker_size(); ++i) {
-    ModelType model =
-        GetModelTypeFromSpecificsFieldNumber(
-            updates.new_progress_marker(i).data_type_id());
-    if (model == UNSPECIFIED || model == TOP_LEVEL_FOLDER) {
-      NOTREACHED() << "Unintelligible server response.";
+    int field_number = updates.new_progress_marker(i).data_type_id();
+    ModelType model_type = GetModelTypeFromSpecificsFieldNumber(field_number);
+    if (!IsRealDataType(model_type)) {
+      NOTREACHED() << "Unknown field number " << field_number;
       continue;
     }
-    forward_progress_types.Put(model);
-    dir->SetDownloadProgress(model, updates.new_progress_marker(i));
+    forward_progress_types.Put(model_type);
+    dir->SetDownloadProgress(model_type, updates.new_progress_marker(i));
   }
   DCHECK(!forward_progress_types.Empty() ||
          updates.changes_remaining() == 0);
diff --git a/sync/engine/syncer_proto_util.cc b/sync/engine/syncer_proto_util.cc
index 5ea54d4..6613c18 100644
--- a/sync/engine/syncer_proto_util.cc
+++ b/sync/engine/syncer_proto_util.cc
@@ -117,8 +117,13 @@ void SyncerProtoUtil::HandleMigrationDoneResponse(
       << "MIGRATION_DONE but no types specified.";
   ModelTypeSet to_migrate;
   for (int i = 0; i < response->migrated_data_type_id_size(); i++) {
-    to_migrate.Put(GetModelTypeFromSpecificsFieldNumber(
-        response->migrated_data_type_id(i)));
+    int field_number = response->migrated_data_type_id(i);
+    ModelType model_type = GetModelTypeFromSpecificsFieldNumber(field_number);
+    if (!IsRealDataType(model_type)) {
+      NOTREACHED() << "Unknown field number " << field_number;
+      continue;
+    }
+    to_migrate.Put(model_type);
   }
   // TODO(akalin): This should be a set union.
   session->mutable_status_controller()->
@@ -322,9 +327,14 @@ SyncProtocolError ConvertErrorPBToLocalType(
     // THROTTLED is currently the only error code that uses |error_data_types|.
     DCHECK_EQ(error.error_type(), sync_pb::SyncEnums::THROTTLED);
     for (int i = 0; i < error.error_data_type_ids_size(); ++i) {
-      sync_protocol_error.error_data_types.Put(
-          GetModelTypeFromSpecificsFieldNumber(
-              error.error_data_type_ids(i)));
+      int field_number = error.error_data_type_ids(i);
+      ModelType model_type =
+          GetModelTypeFromSpecificsFieldNumber(field_number);
+      if (!IsRealDataType(model_type)) {
+        NOTREACHED() << "Unknown field number " << field_number;
+        continue;
+      }
+      sync_protocol_error.error_data_types.Put(model_type);
     }
   }
 
diff --git a/sync/internal_api/public/base/model_type.h b/sync/internal_api/public/base/model_type.h
index 0585b7b..327d980 100644
--- a/sync/internal_api/public/base/model_type.h
+++ b/sync/internal_api/public/base/model_type.h
@@ -160,7 +160,25 @@ ModelTypeSet ControlTypes();
 bool IsControlType(ModelType model_type);
 
 // Determine a model type from the field number of its associated
-// EntitySpecifics field.
+// EntitySpecifics field.  Returns UNSPECIFIED if the field number is
+// not recognized.
+//
+// If you're putting the result in a ModelTypeSet, you should use the
+// following pattern:
+//
+//   ModelTypeSet model_types;
+//   // Say we're looping through a list of items, each of which has a
+//   // field number.
+//   for (...) {
+//     int field_number = ...;
+//     ModelType model_type =
+//         GetModelTypeFromSpecificsFieldNumber(field_number);
+//     if (!IsRealDataType(model_type)) {
+//       NOTREACHED() << "Unknown field number " << field_number;
+//       continue;
+//     }
+//     model_types.Put(model_type);
+//   }
 ModelType GetModelTypeFromSpecificsFieldNumber(int field_number);
 
 // Return the field number of the EntitySpecifics field associated with
diff --git a/sync/syncable/model_type.cc b/sync/syncable/model_type.cc
index 546ac0c8..ca9c3e7 100644
--- a/sync/syncable/model_type.cc
+++ b/sync/syncable/model_type.cc
@@ -93,7 +93,6 @@ ModelType GetModelTypeFromSpecificsFieldNumber(int field_number) {
     if (GetSpecificsFieldNumberFromModelType(model_type) == field_number)
       return model_type;
   }
-  NOTREACHED();
   return UNSPECIFIED;
 }
 
diff --git a/sync/syncable/model_type_unittest.cc b/sync/syncable/model_type_unittest.cc
index 559f0b2..b57378b 100644
--- a/sync/syncable/model_type_unittest.cc
+++ b/sync/syncable/model_type_unittest.cc
@@ -70,5 +70,20 @@ TEST_F(ModelTypeTest, IsRealDataType) {
   EXPECT_TRUE(IsRealDataType(APPS));
 }
 
+// Make sure we can convert ModelTypes to and from specifics field
+// numbers.
+TEST_F(ModelTypeTest, ModelTypeToFromSpecificsFieldNumber) {
+  for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
+    ModelType model_type = ModelTypeFromInt(i);
+    int field_number = GetSpecificsFieldNumberFromModelType(model_type);
+    EXPECT_EQ(model_type,
+              GetModelTypeFromSpecificsFieldNumber(field_number));
+  }
+}
+
+TEST_F(ModelTypeTest, ModelTypeOfInvalidSpecificsFieldNumber) {
+  EXPECT_EQ(UNSPECIFIED, GetModelTypeFromSpecificsFieldNumber(0));
+}
+
 }  // namespace
 }  // namespace syncer