Don't crash when trying to create an ExtensionSyncData from invalid SyncData.

chrome/browser/extensions/extension_sync_data.cc

Index: chrome/browser/extensions/extension_sync_data.cc
diff --git a/chrome/browser/extensions/extension_sync_data.cc b/chrome/browser/extensions/extension_sync_data.cc
index 31df101642edf351192b45e09a965aed685d0941..e499c7e9ee9e9fcbc60ad08b55e3f51468eec756 100644
--- a/chrome/browser/extensions/extension_sync_data.cc
+++ b/chrome/browser/extensions/extension_sync_data.cc
@@ -38,27 +38,6 @@ ExtensionSyncData::ExtensionSyncData()
       installed_by_custodian_(false) {
 }
 
-ExtensionSyncData::ExtensionSyncData(const syncer::SyncData& sync_data)
-    : uninstalled_(false),
-      enabled_(false),
-      incognito_enabled_(false),
-      remote_install_(false),
-      all_urls_enabled_(BOOLEAN_UNSET),
-      installed_by_custodian_(false) {
-  PopulateFromSyncData(sync_data);
-}
-
-ExtensionSyncData::ExtensionSyncData(const syncer::SyncChange& sync_change)
-    : uninstalled_(sync_change.change_type() ==
-                   syncer::SyncChange::ACTION_DELETE),
-      enabled_(false),
-      incognito_enabled_(false),
-      remote_install_(false),
-      all_urls_enabled_(BOOLEAN_UNSET),
-      installed_by_custodian_(false) {
-  PopulateFromSyncData(sync_change.sync_data());
-}
-
 ExtensionSyncData::ExtensionSyncData(const Extension& extension,
                                      bool enabled,
                                      bool incognito_enabled,
@@ -79,6 +58,28 @@ ExtensionSyncData::ExtensionSyncData(const Extension& extension,
 
 ExtensionSyncData::~ExtensionSyncData() {}
 
+// static
+scoped_ptr<ExtensionSyncData> ExtensionSyncData::CreateFromSyncData(
+    const syncer::SyncData& sync_data) {
+  scoped_ptr<ExtensionSyncData> data(new ExtensionSyncData);
+  if (data->PopulateFromSyncData(sync_data))
+    return data.Pass();
+  return NULL;
+}
+
+// static
+scoped_ptr<ExtensionSyncData> ExtensionSyncData::CreateFromSyncChange(
+    const syncer::SyncChange& sync_change) {
+  scoped_ptr<ExtensionSyncData> data(
+      CreateFromSyncData(sync_change.sync_data()));
+  if (!data.get())
+    return NULL;
+
+  data->set_uninstalled(sync_change.change_type() ==
+                        syncer::SyncChange::ACTION_DELETE);
+  return data.Pass();
+}
+
 syncer::SyncData ExtensionSyncData::GetSyncData() const {
   sync_pb::EntitySpecifics specifics;
   PopulateExtensionSpecifics(specifics.mutable_extension());
@@ -106,24 +107,27 @@ void ExtensionSyncData::PopulateExtensionSpecifics(
   specifics->set_name(name_);
 }
 
-void ExtensionSyncData::PopulateFromExtensionSpecifics(
+bool ExtensionSyncData::PopulateFromExtensionSpecifics(
     const sync_pb::ExtensionSpecifics& specifics) {
   if (!crx_file::id_util::IdIsValid(specifics.id())) {
-    LOG(FATAL) << "Attempt to sync bad ExtensionSpecifics (bad ID):\n"
+    LOG(ERROR) << "Attempt to sync bad ExtensionSpecifics (bad ID):\n"

[not at google - send to devlin, 2015/03/11 23:21:34]

Can we track this in UMA, with an enum for why (bad ID, bad version, etc)?

[Yoyo Zhou, 2015/03/12 00:25:48]

Done.

                << GetExtensionSpecificsLogMessage(specifics);
+    return false;
   }
 
   Version specifics_version(specifics.version());
   if (!specifics_version.IsValid()) {
-    LOG(FATAL) << "Attempt to sync bad ExtensionSpecifics (bad version):\n"
+    LOG(ERROR) << "Attempt to sync bad ExtensionSpecifics (bad version):\n"
                << GetExtensionSpecificsLogMessage(specifics);
+    return false;
   }
 
   // The update URL must be either empty or valid.
   GURL specifics_update_url(specifics.update_url());
   if (!specifics_update_url.is_empty() && !specifics_update_url.is_valid()) {
-    LOG(FATAL) << "Attempt to sync bad ExtensionSpecifics (bad update URL):\n"
+    LOG(ERROR) << "Attempt to sync bad ExtensionSpecifics (bad update URL):\n"
                << GetExtensionSpecificsLogMessage(specifics);
+    return false;
   }
 
   id_ = specifics.id();
@@ -142,20 +146,22 @@ void ExtensionSyncData::PopulateFromExtensionSpecifics(
   remote_install_ = specifics.remote_install();
   installed_by_custodian_ = specifics.installed_by_custodian();
   name_ = specifics.name();
+  return true;
 }
 
 void ExtensionSyncData::set_uninstalled(bool uninstalled) {
   uninstalled_ = uninstalled;
 }
 
-void ExtensionSyncData::PopulateFromSyncData(
+bool ExtensionSyncData::PopulateFromSyncData(
     const syncer::SyncData& sync_data) {
   const sync_pb::EntitySpecifics& entity_specifics = sync_data.GetSpecifics();
 
   if (entity_specifics.has_extension()) {
-    PopulateFromExtensionSpecifics(entity_specifics.extension());
+    return PopulateFromExtensionSpecifics(entity_specifics.extension());
   } else {

[not at google - send to devlin, 2015/03/11 23:21:34]

no else after return

[Yoyo Zhou, 2015/03/12 00:25:48]

Done.

-    LOG(FATAL) << "Attempt to sync bad EntitySpecifics.";
+    LOG(ERROR) << "Attempt to sync bad EntitySpecifics: no extension data.";
+    return false;
   }
 }