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

Instead, ignore such invalid inputs from sync.

BUG=359210

Review URL: https://codereview.chromium.org/996213005

Cr-Commit-Position: refs/heads/master@{#320330}

13 files changed, 263 insertions, 153 deletions

diff --git a/chrome/browser/apps/ephemeral_app_browsertest.cc b/chrome/browser/apps/ephemeral_app_browsertest.cc
index fd1d298..348a125 100644
--- a/chrome/browser/apps/ephemeral_app_browsertest.cc
+++ b/chrome/browser/apps/ephemeral_app_browsertest.cc
@@ -533,8 +533,8 @@ class EphemeralAppBrowserTest : public EphemeralAppTestBase {
     for (syncer::SyncChangeList::iterator it =
              mock_sync_processor_.changes().begin();
          it != mock_sync_processor_.changes().end(); ++it) {
-      scoped_ptr<AppSyncData> data(new AppSyncData(*it));
-      if (data->id() == id)
+      scoped_ptr<AppSyncData> data(AppSyncData::CreateFromSyncChange(*it));
+      if (data.get() && data->id() == id)
         sync_data.reset(data.release());
     }
 
diff --git a/chrome/browser/extensions/app_sync_bundle.cc b/chrome/browser/extensions/app_sync_bundle.cc
index 69a5db6..c50f2c9 100644
--- a/chrome/browser/extensions/app_sync_bundle.cc
+++ b/chrome/browser/extensions/app_sync_bundle.cc
@@ -33,9 +33,11 @@ void AppSyncBundle::SetupSync(
   for (syncer::SyncDataList::const_iterator i = initial_sync_data.begin();
        i != initial_sync_data.end();
        ++i) {
-    AppSyncData app_sync_data(*i);
-    AddApp(app_sync_data.id());
-    extension_sync_service_->ProcessAppSyncData(app_sync_data);
+    scoped_ptr<AppSyncData> app_sync_data(AppSyncData::CreateFromSyncData(*i));
+    if (app_sync_data.get()) {
+      AddApp(app_sync_data->id());
+      extension_sync_service_->ProcessAppSyncData(*app_sync_data);
+    }
   }
 }
 
diff --git a/chrome/browser/extensions/app_sync_data.cc b/chrome/browser/extensions/app_sync_data.cc
index 6548ef5..ba3b232 100644
--- a/chrome/browser/extensions/app_sync_data.cc
+++ b/chrome/browser/extensions/app_sync_data.cc
@@ -15,16 +15,6 @@ namespace extensions {
 
 AppSyncData::AppSyncData() {}
 
-AppSyncData::AppSyncData(const syncer::SyncData& sync_data) {
-  PopulateFromSyncData(sync_data);
-}
-
-AppSyncData::AppSyncData(const syncer::SyncChange& sync_change) {
-  PopulateFromSyncData(sync_change.sync_data());
-  extension_sync_data_.set_uninstalled(
-      sync_change.change_type() == syncer::SyncChange::ACTION_DELETE);
-}
-
 AppSyncData::AppSyncData(const Extension& extension,
                          bool enabled,
                          bool incognito_enabled,
@@ -50,6 +40,27 @@ AppSyncData::AppSyncData(const Extension& extension,
 
 AppSyncData::~AppSyncData() {}
 
+// static
+scoped_ptr<AppSyncData> AppSyncData::CreateFromSyncData(
+    const syncer::SyncData& sync_data) {
+  scoped_ptr<AppSyncData> data(new AppSyncData);
+  if (data->PopulateFromSyncData(sync_data))
+    return data.Pass();
+  return scoped_ptr<AppSyncData>();
+}
+
+// static
+scoped_ptr<AppSyncData> AppSyncData::CreateFromSyncChange(
+    const syncer::SyncChange& sync_change) {
+  scoped_ptr<AppSyncData> data(CreateFromSyncData(sync_change.sync_data()));
+  if (!data.get())
+    return scoped_ptr<AppSyncData>();
+
+  data->extension_sync_data_.set_uninstalled(sync_change.change_type() ==
+                                             syncer::SyncChange::ACTION_DELETE);
+  return data.Pass();
+}
+
 syncer::SyncData AppSyncData::GetSyncData() const {
   sync_pb::EntitySpecifics specifics;
   PopulateAppSpecifics(specifics.mutable_app());
@@ -95,9 +106,11 @@ void AppSyncData::PopulateAppSpecifics(sync_pb::AppSpecifics* specifics) const {
       specifics->mutable_extension());
 }
 
-void AppSyncData::PopulateFromAppSpecifics(
+bool AppSyncData::PopulateFromAppSpecifics(
     const sync_pb::AppSpecifics& specifics) {
-  extension_sync_data_.PopulateFromExtensionSpecifics(specifics.extension());
+  if (!extension_sync_data_.PopulateFromExtensionSpecifics(
+          specifics.extension()))
+    return false;
 
   app_launch_ordinal_ = syncer::StringOrdinal(specifics.app_launch_ordinal());
   page_ordinal_ = syncer::StringOrdinal(specifics.page_ordinal());
@@ -109,10 +122,11 @@ void AppSyncData::PopulateFromAppSpecifics(
   bookmark_app_url_ = specifics.bookmark_app_url();
   bookmark_app_description_ = specifics.bookmark_app_description();
   bookmark_app_icon_color_ = specifics.bookmark_app_icon_color();
+  return true;
 }
 
-void AppSyncData::PopulateFromSyncData(const syncer::SyncData& sync_data) {
-  PopulateFromAppSpecifics(sync_data.GetSpecifics().app());
+bool AppSyncData::PopulateFromSyncData(const syncer::SyncData& sync_data) {
+  return PopulateFromAppSpecifics(sync_data.GetSpecifics().app());
 }
 
 }  // namespace extensions
diff --git a/chrome/browser/extensions/app_sync_data.h b/chrome/browser/extensions/app_sync_data.h
index bdbb164..b1ee77e 100644
--- a/chrome/browser/extensions/app_sync_data.h
+++ b/chrome/browser/extensions/app_sync_data.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_APP_SYNC_DATA_H_
 #define CHROME_BROWSER_EXTENSIONS_APP_SYNC_DATA_H_
 
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
 #include "extensions/common/constants.h"
 #include "sync/api/string_ordinal.h"
@@ -28,8 +29,6 @@ class ExtensionSyncData;
 class AppSyncData {
  public:
   AppSyncData();
-  explicit AppSyncData(const syncer::SyncData& sync_data);
-  explicit AppSyncData(const syncer::SyncChange& sync_change);
   AppSyncData(const Extension& extension,
               bool enabled,
               bool incognito_enabled,
@@ -40,6 +39,13 @@ class AppSyncData {
               extensions::LaunchType launch_type);
   ~AppSyncData();
 
+  // For constructing an AppSyncData from received sync data.
+  // May return null if the sync data was invalid.
+  static scoped_ptr<AppSyncData> CreateFromSyncData(
+      const syncer::SyncData& sync_data);
+  static scoped_ptr<AppSyncData> CreateFromSyncChange(
+      const syncer::SyncChange& sync_change);
+
   // Retrive sync data from this class.
   syncer::SyncData GetSyncData() const;
   syncer::SyncChange GetSyncChange(
@@ -80,10 +86,10 @@ class AppSyncData {
   // Convert an AppSyncData back out to a sync structure.
   void PopulateAppSpecifics(sync_pb::AppSpecifics* specifics) const;
 
-  // Populate this class from sync inputs.
-  void PopulateFromAppSpecifics(
-      const sync_pb::AppSpecifics& specifics);
-  void PopulateFromSyncData(const syncer::SyncData& sync_data);
+  // Populate this class from sync inputs. Return true if the input
+  // was valid.
+  bool PopulateFromAppSpecifics(const sync_pb::AppSpecifics& specifics);
+  bool PopulateFromSyncData(const syncer::SyncData& sync_data);
 
   ExtensionSyncData extension_sync_data_;
   syncer::StringOrdinal app_launch_ordinal_;
@@ -97,4 +103,3 @@ class AppSyncData {
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_APP_SYNC_DATA_H_
-
diff --git a/chrome/browser/extensions/app_sync_data_unittest.cc b/chrome/browser/extensions/app_sync_data_unittest.cc
index 97fafbd..5b90621 100644
--- a/chrome/browser/extensions/app_sync_data_unittest.cc
+++ b/chrome/browser/extensions/app_sync_data_unittest.cc
@@ -48,11 +48,13 @@ TEST_F(AppSyncDataTest, SyncDataToExtensionSyncDataForApp) {
   syncer::SyncData sync_data =
       syncer::SyncData::CreateLocalData("sync_tag", "non_unique_title", entity);
 
-  AppSyncData app_sync_data(sync_data);
+  scoped_ptr<AppSyncData> app_sync_data =
+      AppSyncData::CreateFromSyncData(sync_data);
+  ASSERT_TRUE(app_sync_data.get());
   EXPECT_EQ(app_specifics->app_launch_ordinal(),
-            app_sync_data.app_launch_ordinal().ToInternalValue());
+            app_sync_data->app_launch_ordinal().ToInternalValue());
   EXPECT_EQ(app_specifics->page_ordinal(),
-            app_sync_data.page_ordinal().ToInternalValue());
+            app_sync_data->page_ordinal().ToInternalValue());
 }
 
 
@@ -69,9 +71,11 @@ TEST_F(AppSyncDataTest, ExtensionSyncDataToSyncDataForApp) {
 
   syncer::SyncData sync_data =
       syncer::SyncData::CreateLocalData("sync_tag", "non_unique_title", entity);
-  AppSyncData app_sync_data(sync_data);
+  scoped_ptr<AppSyncData> app_sync_data =
+      AppSyncData::CreateFromSyncData(sync_data);
+  ASSERT_TRUE(app_sync_data.get());
 
-  syncer::SyncData output_sync_data = app_sync_data.GetSyncData();
+  syncer::SyncData output_sync_data = app_sync_data->GetSyncData();
   EXPECT_TRUE(sync_data.GetSpecifics().has_app());
   const sync_pb::AppSpecifics& output_specifics =
       output_sync_data.GetSpecifics().app();
@@ -93,8 +97,10 @@ TEST_F(AppSyncDataTest, ExtensionSyncDataInvalidOrdinal) {
       syncer::SyncData::CreateLocalData("sync_tag", "non_unique_title", entity);
 
   // There should be no issue loading the sync data.
-  AppSyncData app_sync_data(sync_data);
-  app_sync_data.GetSyncData();
+  scoped_ptr<AppSyncData> app_sync_data =
+      AppSyncData::CreateFromSyncData(sync_data);
+  ASSERT_TRUE(app_sync_data.get());
+  app_sync_data->GetSyncData();
 }
 
 }  // namespace extensions
diff --git a/chrome/browser/extensions/extension_disabled_ui_browsertest.cc b/chrome/browser/extensions/extension_disabled_ui_browsertest.cc
index c3cd109..ca09c23 100644
--- a/chrome/browser/extensions/extension_disabled_ui_browsertest.cc
+++ b/chrome/browser/extensions/extension_disabled_ui_browsertest.cc
@@ -238,7 +238,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, RemoteInstall) {
                                          syncer::AttachmentServiceProxy());
   // Sync is installing a new extension, so it pends.
   EXPECT_FALSE(sync_service->ProcessExtensionSyncData(
-      extensions::ExtensionSyncData(sync_data)));
+      *extensions::ExtensionSyncData::CreateFromSyncData(sync_data)));
 
   WaitForExtensionInstall();
   content::RunAllBlockingPoolTasksUntilIdle();
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 39d6514..7d1a0db 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -161,6 +161,7 @@ using content::PluginService;
 using extensions::APIPermission;
 using extensions::APIPermissionSet;
 using extensions::AppSorting;
+using extensions::AppSyncData;
 using extensions::Blacklist;
 using extensions::CrxInstaller;
 using extensions::Extension;
@@ -5872,8 +5873,8 @@ TEST_F(ExtensionServiceTest, DisableExtensionFromSync) {
   const Extension* extension = service()->GetExtensionById(good0, true);
   ASSERT_TRUE(extension);
   ASSERT_TRUE(service()->IsExtensionEnabled(good0));
-  extensions::ExtensionSyncData disable_good_crx(
-      *extension, false, false, false, ExtensionSyncData::BOOLEAN_UNSET);
+  ExtensionSyncData disable_good_crx(*extension, false, false, false,
+                                     ExtensionSyncData::BOOLEAN_UNSET);
 
   // Then sync data arrives telling us to disable |good0|.
   syncer::SyncDataList sync_data;
@@ -5919,8 +5920,8 @@ TEST_F(ExtensionServiceTest, DontDisableExtensionWithPendingEnableFromSync) {
 
   // Now sync data comes in that says to disable good0. This should be
   // ignored.
-  extensions::ExtensionSyncData disable_good_crx(
-      *extension, false, false, false, ExtensionSyncData::BOOLEAN_FALSE);
+  ExtensionSyncData disable_good_crx(*extension, false, false, false,
+                                     ExtensionSyncData::BOOLEAN_FALSE);
   syncer::SyncDataList sync_data;
   sync_data.push_back(disable_good_crx.GetSyncData());
   extension_sync_service()->MergeDataAndStartSyncing(
@@ -5952,17 +5953,19 @@ TEST_F(ExtensionServiceTest, GetSyncData) {
   syncer::SyncDataList list =
       extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS);
   ASSERT_EQ(list.size(), 1U);
-  extensions::ExtensionSyncData data(list[0]);
-  EXPECT_EQ(extension->id(), data.id());
-  EXPECT_FALSE(data.uninstalled());
-  EXPECT_EQ(service()->IsExtensionEnabled(good_crx), data.enabled());
+  scoped_ptr<ExtensionSyncData> data =
+      ExtensionSyncData::CreateFromSyncData(list[0]);
+  ASSERT_TRUE(data.get());
+  EXPECT_EQ(extension->id(), data->id());
+  EXPECT_FALSE(data->uninstalled());
+  EXPECT_EQ(service()->IsExtensionEnabled(good_crx), data->enabled());
   EXPECT_EQ(extensions::util::IsIncognitoEnabled(good_crx, profile()),
-            data.incognito_enabled());
-  EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data.all_urls_enabled());
-  EXPECT_TRUE(data.version().Equals(*extension->version()));
+            data->incognito_enabled());
+  EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data->all_urls_enabled());
+  EXPECT_TRUE(data->version().Equals(*extension->version()));
   EXPECT_EQ(extensions::ManifestURL::GetUpdateURL(extension),
-            data.update_url());
-  EXPECT_EQ(extension->name(), data.name());
+            data->update_url());
+  EXPECT_EQ(extension->name(), data->name());
 }
 
 TEST_F(ExtensionServiceTest, GetSyncDataTerminated) {
@@ -5984,17 +5987,19 @@ TEST_F(ExtensionServiceTest, GetSyncDataTerminated) {
   syncer::SyncDataList list =
       extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS);
   ASSERT_EQ(list.size(), 1U);
-  extensions::ExtensionSyncData data(list[0]);
-  EXPECT_EQ(extension->id(), data.id());
-  EXPECT_FALSE(data.uninstalled());
-  EXPECT_EQ(service()->IsExtensionEnabled(good_crx), data.enabled());
+  scoped_ptr<ExtensionSyncData> data =
+      ExtensionSyncData::CreateFromSyncData(list[0]);
+  ASSERT_TRUE(data.get());
+  EXPECT_EQ(extension->id(), data->id());
+  EXPECT_FALSE(data->uninstalled());
+  EXPECT_EQ(service()->IsExtensionEnabled(good_crx), data->enabled());
   EXPECT_EQ(extensions::util::IsIncognitoEnabled(good_crx, profile()),
-            data.incognito_enabled());
-  EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data.all_urls_enabled());
-  EXPECT_TRUE(data.version().Equals(*extension->version()));
+            data->incognito_enabled());
+  EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data->all_urls_enabled());
+  EXPECT_TRUE(data->version().Equals(*extension->version()));
   EXPECT_EQ(extensions::ManifestURL::GetUpdateURL(extension),
-            data.update_url());
-  EXPECT_EQ(extension->name(), data.name());
+            data->update_url());
+  EXPECT_EQ(extension->name(), data->name());
 }
 
 TEST_F(ExtensionServiceTest, GetSyncDataFilter) {
@@ -6036,10 +6041,12 @@ TEST_F(ExtensionServiceTest, GetSyncExtensionDataUserSettings) {
     syncer::SyncDataList list =
         extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS);
     ASSERT_EQ(list.size(), 1U);
-    extensions::ExtensionSyncData data(list[0]);
-    EXPECT_TRUE(data.enabled());
-    EXPECT_FALSE(data.incognito_enabled());
-    EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data.all_urls_enabled());
+    scoped_ptr<ExtensionSyncData> data =
+        ExtensionSyncData::CreateFromSyncData(list[0]);
+    ASSERT_TRUE(data.get());
+    EXPECT_TRUE(data->enabled());
+    EXPECT_FALSE(data->incognito_enabled());
+    EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data->all_urls_enabled());
   }
 
   service()->DisableExtension(good_crx, Extension::DISABLE_USER_ACTION);
@@ -6047,10 +6054,12 @@ TEST_F(ExtensionServiceTest, GetSyncExtensionDataUserSettings) {
     syncer::SyncDataList list =
         extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS);
     ASSERT_EQ(list.size(), 1U);
-    extensions::ExtensionSyncData data(list[0]);
-    EXPECT_FALSE(data.enabled());
-    EXPECT_FALSE(data.incognito_enabled());
-    EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data.all_urls_enabled());
+    scoped_ptr<ExtensionSyncData> data =
+        ExtensionSyncData::CreateFromSyncData(list[0]);
+    ASSERT_TRUE(data.get());
+    EXPECT_FALSE(data->enabled());
+    EXPECT_FALSE(data->incognito_enabled());
+    EXPECT_EQ(ExtensionSyncData::BOOLEAN_UNSET, data->all_urls_enabled());
   }
 
   extensions::util::SetIsIncognitoEnabled(good_crx, profile(), true);
@@ -6060,10 +6069,12 @@ TEST_F(ExtensionServiceTest, GetSyncExtensionDataUserSettings) {
     syncer::SyncDataList list =
         extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS);
     ASSERT_EQ(list.size(), 1U);
-    extensions::ExtensionSyncData data(list[0]);
-    EXPECT_FALSE(data.enabled());
-    EXPECT_TRUE(data.incognito_enabled());
-    EXPECT_EQ(ExtensionSyncData::BOOLEAN_FALSE, data.all_urls_enabled());
+    scoped_ptr<ExtensionSyncData> data =
+        ExtensionSyncData::CreateFromSyncData(list[0]);
+    ASSERT_TRUE(data.get());
+    EXPECT_FALSE(data->enabled());
+    EXPECT_TRUE(data->incognito_enabled());
+    EXPECT_EQ(ExtensionSyncData::BOOLEAN_FALSE, data->all_urls_enabled());
   }
 
   service()->EnableExtension(good_crx);
@@ -6073,10 +6084,12 @@ TEST_F(ExtensionServiceTest, GetSyncExtensionDataUserSettings) {
     syncer::SyncDataList list =
         extension_sync_service()->GetAllSyncData(syncer::EXTENSIONS);
     ASSERT_EQ(list.size(), 1U);
-    extensions::ExtensionSyncData data(list[0]);
-    EXPECT_TRUE(data.enabled());
-    EXPECT_TRUE(data.incognito_enabled());
-    EXPECT_EQ(ExtensionSyncData::BOOLEAN_TRUE, data.all_urls_enabled());
+    scoped_ptr<ExtensionSyncData> data =
+        ExtensionSyncData::CreateFromSyncData(list[0]);
+    ASSERT_TRUE(data.get());
+    EXPECT_TRUE(data->enabled());
+    EXPECT_TRUE(data->incognito_enabled());
+    EXPECT_EQ(ExtensionSyncData::BOOLEAN_TRUE, data->all_urls_enabled());
   }
 }
 
@@ -6144,9 +6157,10 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) {
         extension_sync_service()->GetAllSyncData(syncer::APPS);
     ASSERT_EQ(list.size(), 1U);
 
-    extensions::AppSyncData app_sync_data(list[0]);
-    EXPECT_TRUE(initial_ordinal.Equals(app_sync_data.app_launch_ordinal()));
-    EXPECT_TRUE(initial_ordinal.Equals(app_sync_data.page_ordinal()));
+    scoped_ptr<AppSyncData> app_sync_data =
+        AppSyncData::CreateFromSyncData(list[0]);
+    EXPECT_TRUE(initial_ordinal.Equals(app_sync_data->app_launch_ordinal()));
+    EXPECT_TRUE(initial_ordinal.Equals(app_sync_data->page_ordinal()));
   }
 
   AppSorting* sorting = ExtensionPrefs::Get(profile())->app_sorting();
@@ -6156,9 +6170,11 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) {
         extension_sync_service()->GetAllSyncData(syncer::APPS);
     ASSERT_EQ(list.size(), 1U);
 
-    extensions::AppSyncData app_sync_data(list[0]);
-    EXPECT_TRUE(initial_ordinal.LessThan(app_sync_data.app_launch_ordinal()));
-    EXPECT_TRUE(initial_ordinal.Equals(app_sync_data.page_ordinal()));
+    scoped_ptr<AppSyncData> app_sync_data =
+        AppSyncData::CreateFromSyncData(list[0]);
+    ASSERT_TRUE(app_sync_data.get());
+    EXPECT_TRUE(initial_ordinal.LessThan(app_sync_data->app_launch_ordinal()));
+    EXPECT_TRUE(initial_ordinal.Equals(app_sync_data->page_ordinal()));
   }
 
   sorting->SetPageOrdinal(app->id(), initial_ordinal.CreateAfter());
@@ -6167,9 +6183,11 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettings) {
         extension_sync_service()->GetAllSyncData(syncer::APPS);
     ASSERT_EQ(list.size(), 1U);
 
-    extensions::AppSyncData app_sync_data(list[0]);
-    EXPECT_TRUE(initial_ordinal.LessThan(app_sync_data.app_launch_ordinal()));
-    EXPECT_TRUE(initial_ordinal.LessThan(app_sync_data.page_ordinal()));
+    scoped_ptr<AppSyncData> app_sync_data =
+        AppSyncData::CreateFromSyncData(list[0]);
+    ASSERT_TRUE(app_sync_data.get());
+    EXPECT_TRUE(initial_ordinal.LessThan(app_sync_data->app_launch_ordinal()));
+    EXPECT_TRUE(initial_ordinal.LessThan(app_sync_data->page_ordinal()));
   }
 }
 
@@ -6206,9 +6224,10 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettingsOnExtensionMoved) {
         extension_sync_service()->GetAllSyncData(syncer::APPS);
     ASSERT_EQ(list.size(), 3U);
 
-    extensions::AppSyncData data[kAppCount];
+    scoped_ptr<AppSyncData> data[kAppCount];
     for (size_t i = 0; i < kAppCount; ++i) {
-      data[i] = extensions::AppSyncData(list[i]);
+      data[i] = AppSyncData::CreateFromSyncData(list[i]);
+      ASSERT_TRUE(data[i].get());
     }
 
     // The sync data is not always in the same order our apps were installed in,
@@ -6217,8 +6236,8 @@ TEST_F(ExtensionServiceTest, GetSyncAppDataUserSettingsOnExtensionMoved) {
     syncer::StringOrdinal app_launch_ordinals[kAppCount];
     for (size_t i = 0; i < kAppCount; ++i) {
       for (size_t j = 0; j < kAppCount; ++j) {
-        if (apps[i]->id() == data[j].id())
-          app_launch_ordinals[i] = data[j].app_launch_ordinal();
+        if (apps[i]->id() == data[j]->id())
+          app_launch_ordinals[i] = data[j]->app_launch_ordinal();
       }
     }
 
diff --git a/chrome/browser/extensions/extension_sync_bundle.cc b/chrome/browser/extensions/extension_sync_bundle.cc
index 2333ffa..61ec117 100644
--- a/chrome/browser/extensions/extension_sync_bundle.cc
+++ b/chrome/browser/extensions/extension_sync_bundle.cc
@@ -32,9 +32,12 @@ void ExtensionSyncBundle::SetupSync(
   for (syncer::SyncDataList::const_iterator i = initial_sync_data.begin();
        i != initial_sync_data.end();
        ++i) {
-    ExtensionSyncData extension_sync_data(*i);
-    AddExtension(extension_sync_data.id());
-    extension_sync_service_->ProcessExtensionSyncData(extension_sync_data);
+    scoped_ptr<ExtensionSyncData> extension_sync_data(
+        ExtensionSyncData::CreateFromSyncData(*i));
+    if (extension_sync_data.get()) {
+      AddExtension(extension_sync_data->id());
+      extension_sync_service_->ProcessExtensionSyncData(*extension_sync_data);
+    }
   }
 }
 
diff --git a/chrome/browser/extensions/extension_sync_data.cc b/chrome/browser/extensions/extension_sync_data.cc
index 31df101..aa56051 100644
--- a/chrome/browser/extensions/extension_sync_data.cc
+++ b/chrome/browser/extensions/extension_sync_data.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/extension_sync_data.h"
 
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/app_sync_data.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -27,36 +28,37 @@ std::string GetExtensionSpecificsLogMessage(
                             specifics.update_url().c_str());
 }
 
-}  // namespace
+enum BadSyncDataReason {
+  // Invalid extension ID.
+  BAD_EXTENSION_ID,
 
-ExtensionSyncData::ExtensionSyncData()
-    : uninstalled_(false),
-      enabled_(false),
-      incognito_enabled_(false),
-      remote_install_(false),
-      all_urls_enabled_(BOOLEAN_UNSET),
-      installed_by_custodian_(false) {
-}
+  // Invalid version.
+  BAD_VERSION,
 
-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);
+  // Invalid update URL.
+  BAD_UPDATE_URL,
+
+  // No ExtensionSpecifics in the EntitySpecifics.
+  NO_EXTENSION_SPECIFICS,
+
+  // Must be at the end.
+  NUM_BAD_SYNC_DATA_REASONS
+};
+
+void RecordBadSyncData(BadSyncDataReason reason) {
+  UMA_HISTOGRAM_ENUMERATION("Extensions.BadSyncDataReason", reason,
+                            NUM_BAD_SYNC_DATA_REASONS);
 }
 
-ExtensionSyncData::ExtensionSyncData(const syncer::SyncChange& sync_change)
-    : uninstalled_(sync_change.change_type() ==
-                   syncer::SyncChange::ACTION_DELETE),
+}  // namespace
+
+ExtensionSyncData::ExtensionSyncData()
+    : uninstalled_(false),
       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,
@@ -79,6 +81,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 scoped_ptr<ExtensionSyncData>();
+}
+
+// static
+scoped_ptr<ExtensionSyncData> ExtensionSyncData::CreateFromSyncChange(
+    const syncer::SyncChange& sync_change) {
+  scoped_ptr<ExtensionSyncData> data(
+      CreateFromSyncData(sync_change.sync_data()));
+  if (!data.get())
+    return scoped_ptr<ExtensionSyncData>();
+
+  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 +130,30 @@ 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"
                << GetExtensionSpecificsLogMessage(specifics);
+    RecordBadSyncData(BAD_EXTENSION_ID);
+    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);
+    RecordBadSyncData(BAD_VERSION);
+    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);
+    RecordBadSyncData(BAD_UPDATE_URL);
+    return false;
   }
 
   id_ = specifics.id();
@@ -142,21 +172,23 @@ 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());
-  } else {
-    LOG(FATAL) << "Attempt to sync bad EntitySpecifics.";
-  }
+  if (entity_specifics.has_extension())
+    return PopulateFromExtensionSpecifics(entity_specifics.extension());
+
+  LOG(ERROR) << "Attempt to sync bad EntitySpecifics: no extension data.";
+  RecordBadSyncData(NO_EXTENSION_SPECIFICS);
+  return false;
 }
 
 }  // namespace extensions
diff --git a/chrome/browser/extensions/extension_sync_data.h b/chrome/browser/extensions/extension_sync_data.h
index 9e67043..5808b6b 100644
--- a/chrome/browser/extensions/extension_sync_data.h
+++ b/chrome/browser/extensions/extension_sync_data.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/memory/scoped_ptr.h"
 #include "base/version.h"
 #include "sync/api/sync_change.h"
 #include "url/gurl.h"
@@ -33,8 +34,6 @@ class ExtensionSyncData {
   };
 
   ExtensionSyncData();
-  explicit ExtensionSyncData(const syncer::SyncData& sync_data);
-  explicit ExtensionSyncData(const syncer::SyncChange& sync_change);
   ExtensionSyncData(const Extension& extension,
                     bool enabled,
                     bool incognito_enabled,
@@ -42,6 +41,13 @@ class ExtensionSyncData {
                     OptionalBoolean all_urls_enabled);
   ~ExtensionSyncData();
 
+  // For constructing an ExtensionSyncData from received sync data.
+  // May return null if the sync data was invalid.
+  static scoped_ptr<ExtensionSyncData> CreateFromSyncData(
+      const syncer::SyncData& sync_data);
+  static scoped_ptr<ExtensionSyncData> CreateFromSyncChange(
+      const syncer::SyncChange& sync_change);
+
   // Retrieve sync data from this class.
   syncer::SyncData GetSyncData() const;
   syncer::SyncChange GetSyncChange(
@@ -50,8 +56,8 @@ class ExtensionSyncData {
   // Convert an ExtensionSyncData back out to a sync structure.
   void PopulateExtensionSpecifics(sync_pb::ExtensionSpecifics* specifics) const;
 
-  // Populate this class from sync inputs.
-  void PopulateFromExtensionSpecifics(
+  // Populate this class from sync inputs. Returns true if the input was valid.
+  bool PopulateFromExtensionSpecifics(
       const sync_pb::ExtensionSpecifics& specifics);
 
   void set_uninstalled(bool uninstalled);
@@ -77,7 +83,7 @@ class ExtensionSyncData {
 
  private:
   // Populate this class from sync inputs.
-  void PopulateFromSyncData(const syncer::SyncData& sync_data);
+  bool PopulateFromSyncData(const syncer::SyncData& sync_data);
 
   std::string id_;
   bool uninstalled_;
diff --git a/chrome/browser/extensions/extension_sync_data_unittest.cc b/chrome/browser/extensions/extension_sync_data_unittest.cc
index 4236502..b61f4d1 100644
--- a/chrome/browser/extensions/extension_sync_data_unittest.cc
+++ b/chrome/browser/extensions/extension_sync_data_unittest.cc
@@ -27,8 +27,10 @@ const char kName[] = "MyExtension";
 void ProtobufToSyncDataEqual(const sync_pb::EntitySpecifics& entity) {
   syncer::SyncData sync_data =
       syncer::SyncData::CreateLocalData("sync_tag", "non_unique_title", entity);
-  ExtensionSyncData extension_sync_data(sync_data);
-  syncer::SyncData output_sync_data = extension_sync_data.GetSyncData();
+  scoped_ptr<ExtensionSyncData> extension_sync_data =
+      ExtensionSyncData::CreateFromSyncData(sync_data);
+  ASSERT_TRUE(extension_sync_data.get());
+  syncer::SyncData output_sync_data = extension_sync_data->GetSyncData();
   const sync_pb::ExtensionSpecifics& output =
       output_sync_data.GetSpecifics().extension();
   const sync_pb::ExtensionSpecifics& input = entity.extension();
@@ -53,18 +55,20 @@ void ProtobufToSyncDataEqual(const sync_pb::EntitySpecifics& entity) {
 // confirms that the input is the same as the output.
 void SyncDataToProtobufEqual(const ExtensionSyncData& input) {
   syncer::SyncData sync_data = input.GetSyncData();
-  ExtensionSyncData output(sync_data);
-
-  EXPECT_EQ(input.id(), output.id());
-  EXPECT_EQ(input.uninstalled(), output.uninstalled());
-  EXPECT_EQ(input.enabled(), output.enabled());
-  EXPECT_EQ(input.incognito_enabled(), output.incognito_enabled());
-  EXPECT_EQ(input.remote_install(), output.remote_install());
-  EXPECT_EQ(input.installed_by_custodian(), output.installed_by_custodian());
-  EXPECT_EQ(input.all_urls_enabled(), output.all_urls_enabled());
-  EXPECT_TRUE(input.version().Equals(output.version()));
-  EXPECT_EQ(input.update_url(), output.update_url());
-  EXPECT_EQ(input.name(), output.name());
+  scoped_ptr<ExtensionSyncData> output =
+      ExtensionSyncData::CreateFromSyncData(sync_data);
+  ASSERT_TRUE(output.get());
+
+  EXPECT_EQ(input.id(), output->id());
+  EXPECT_EQ(input.uninstalled(), output->uninstalled());
+  EXPECT_EQ(input.enabled(), output->enabled());
+  EXPECT_EQ(input.incognito_enabled(), output->incognito_enabled());
+  EXPECT_EQ(input.remote_install(), output->remote_install());
+  EXPECT_EQ(input.installed_by_custodian(), output->installed_by_custodian());
+  EXPECT_EQ(input.all_urls_enabled(), output->all_urls_enabled());
+  EXPECT_TRUE(input.version().Equals(output->version()));
+  EXPECT_EQ(input.update_url(), output->update_url());
+  EXPECT_EQ(input.name(), output->name());
 }
 
 }  // namespace
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index da191a6..56f4990 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -39,6 +39,7 @@
 #include "sync/api/sync_error_factory.h"
 #include "ui/gfx/image/image_family.h"
 
+using extensions::AppSyncData;
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionRegistry;
@@ -260,10 +261,16 @@ syncer::SyncError ExtensionSyncService::ProcessSyncChanges(
       i != change_list.end();
       ++i) {
     syncer::ModelType type = i->sync_data().GetDataType();
-    if (type == syncer::EXTENSIONS)
-      extension_sync_bundle_.ProcessSyncChange(ExtensionSyncData(*i));
-    else if (type == syncer::APPS)
-      app_sync_bundle_.ProcessSyncChange(extensions::AppSyncData(*i));
+    if (type == syncer::EXTENSIONS) {
+      scoped_ptr<ExtensionSyncData> extension_data(
+          ExtensionSyncData::CreateFromSyncChange(*i));
+      if (extension_data.get())
+        extension_sync_bundle_.ProcessSyncChange(*extension_data);
+    } else if (type == syncer::APPS) {
+      scoped_ptr<AppSyncData> app_data(AppSyncData::CreateFromSyncChange(*i));
+      if (app_data.get())
+        app_sync_bundle_.ProcessSyncChange(*app_data);
+    }
   }
 
   extension_prefs_->app_sorting()->FixNTPOrdinalCollisions();
@@ -282,11 +289,10 @@ ExtensionSyncData ExtensionSyncService::GetExtensionSyncData(
       GetAllowedOnAllUrlsOptionalBoolean(extension.id(), profile_));
 }
 
-extensions::AppSyncData ExtensionSyncService::GetAppSyncData(
+AppSyncData ExtensionSyncService::GetAppSyncData(
     const Extension& extension) const {
-  return extensions::AppSyncData(
-      extension,
-      extension_service_->IsExtensionEnabled(extension.id()),
+  return AppSyncData(
+      extension, extension_service_->IsExtensionEnabled(extension.id()),
       extensions::util::IsIncognitoEnabled(extension.id(), profile_),
       extension_prefs_->HasDisableReason(extension.id(),
                                          Extension::DISABLE_REMOTE_INSTALL),
@@ -316,10 +322,9 @@ std::vector<ExtensionSyncData>
   return extension_sync_list;
 }
 
-std::vector<extensions::AppSyncData> ExtensionSyncService::GetAppSyncDataList()
-    const {
+std::vector<AppSyncData> ExtensionSyncService::GetAppSyncDataList() const {
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
-  std::vector<extensions::AppSyncData> app_sync_list;
+  std::vector<AppSyncData> app_sync_list;
   app_sync_bundle_.GetAppSyncDataListHelper(
       registry->enabled_extensions(), &app_sync_list);
   app_sync_bundle_.GetAppSyncDataListHelper(
@@ -327,8 +332,7 @@ std::vector<extensions::AppSyncData> ExtensionSyncService::GetAppSyncDataList()
   app_sync_bundle_.GetAppSyncDataListHelper(
       registry->terminated_extensions(), &app_sync_list);
 
-  std::vector<extensions::AppSyncData> pending_apps =
-      app_sync_bundle_.GetPendingData();
+  std::vector<AppSyncData> pending_apps = app_sync_bundle_.GetPendingData();
   app_sync_list.insert(app_sync_list.begin(),
                        pending_apps.begin(),
                        pending_apps.end());
@@ -350,7 +354,7 @@ bool ExtensionSyncService::ProcessExtensionSyncData(
 }
 
 bool ExtensionSyncService::ProcessAppSyncData(
-    const extensions::AppSyncData& app_sync_data) {
+    const AppSyncData& app_sync_data) {
   const std::string& id = app_sync_data.id();
 
   if (app_sync_data.app_launch_ordinal().IsValid() &&
@@ -384,7 +388,7 @@ bool ExtensionSyncService::ProcessAppSyncData(
 }
 
 void ExtensionSyncService::ProcessBookmarkAppSyncData(
-    const extensions::AppSyncData& app_sync_data) {
+    const AppSyncData& app_sync_data) {
   // Process bookmark app sync if necessary.
   GURL bookmark_app_url(app_sync_data.bookmark_app_url());
   if (!bookmark_app_url.is_valid() ||
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 341b540..00f6835 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -8472,6 +8472,14 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   </summary>
 </histogram>
 
+<histogram name="Extensions.BadSyncDataReason" enum="BadSyncDataReason">
+  <owner>yoz@chromium.org</owner>
+  <summary>
+    The reason a valid ExtensionSyncData could not be parsed from a SyncData
+    received from sync.
+  </summary>
+</histogram>
+
 <histogram name="Extensions.CheckForExternalUpdatesTime">
   <owner>rkaplow@chromium.org</owner>
   <summary>
@@ -44815,6 +44823,13 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <int value="2" label="Failure"/>
 </enum>
 
+<enum name="BadSyncDataReason" type="int">
+  <int value="0" label="Bad extension ID"/>
+  <int value="1" label="Bad version"/>
+  <int value="2" label="Bad update URL"/>
+  <int value="3" label="No ExtensionSpecifics"/>
+</enum>
+
 <enum name="BaseRelocationType" type="int">
   <int value="0" label="IMAGE_REL_BASED_ABSOLUTE"/>
   <int value="1" label="IMAGE_REL_BASED_HIGH"/>