Change extension icon load errors to warnings

During the Extension parsing step, check if the icon file exists and if not, remove that entry from the dictionary.

Keep the same check during the validation phase and don't apply the workaround to unpacked extensions. This will more strongly discourage new extensions from making this mistake.

BUG=570249

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

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

25 files changed, 214 insertions, 40 deletions

diff --git a/chrome/browser/extensions/api/extension_action/browser_action_unittest.cc b/chrome/browser/extensions/api/extension_action/browser_action_unittest.cc
new file mode 100644
index 0000000..8060e9e
--- /dev/null
+++ b/chrome/browser/extensions/api/extension_action/browser_action_unittest.cc
@@ -0,0 +1,70 @@
+// Copyright 2015 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 "base/files/file_util.h"
+#include "chrome/browser/extensions/extension_service_test_with_install.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
+
+namespace extensions {
+namespace {
+
+class BrowserActionUnitTest : public ExtensionServiceTestWithInstall {
+};
+
+TEST_F(BrowserActionUnitTest, MultiIcons) {
+  InitializeEmptyExtensionService();
+  base::FilePath path =
+      data_dir().AppendASCII("api_test/browser_action/multi_icons");
+  ASSERT_TRUE(base::PathExists(path));
+
+  const Extension* extension = PackAndInstallCRX(path, INSTALL_NEW);
+
+  EXPECT_EQ(0U, extension->install_warnings().size());
+  const ActionInfo* browser_action_info =
+      ActionInfo::GetBrowserActionInfo(extension);
+  ASSERT_TRUE(browser_action_info);
+
+  const ExtensionIconSet& icons = browser_action_info->default_icon;
+
+  // Extension can provide arbitrary sizes.
+  EXPECT_EQ(4u, icons.map().size());
+  EXPECT_EQ("icon19.png", icons.Get(19, ExtensionIconSet::MATCH_EXACTLY));
+  EXPECT_EQ("icon24.png", icons.Get(24, ExtensionIconSet::MATCH_EXACTLY));
+  EXPECT_EQ("icon24.png", icons.Get(31, ExtensionIconSet::MATCH_EXACTLY));
+  EXPECT_EQ("icon38.png", icons.Get(38, ExtensionIconSet::MATCH_EXACTLY));
+}
+
+TEST_F(BrowserActionUnitTest, MissingIconInWebstoreCrx) {
+  InitializeEmptyExtensionService();
+  base::FilePath path =
+      data_dir().AppendASCII("api_test/browser_action/missing_icon");
+  ASSERT_TRUE(base::PathExists(path));
+
+  const Extension* extension = PackAndInstallCRX(path, INSTALL_NEW);
+
+  ASSERT_EQ(1U, extension->install_warnings().size());
+  EXPECT_EQ("Could not load extension icon 'icon19.png'.",
+            extension->install_warnings().front().message);
+  const ActionInfo* browser_action_info =
+      ActionInfo::GetBrowserActionInfo(extension);
+  ASSERT_TRUE(browser_action_info);
+
+  const ExtensionIconSet& icons = browser_action_info->default_icon;
+
+  EXPECT_EQ(2u, icons.map().size());
+  EXPECT_EQ("icon24.png", icons.Get(24, ExtensionIconSet::MATCH_EXACTLY));
+  EXPECT_EQ("icon38.png", icons.Get(38, ExtensionIconSet::MATCH_EXACTLY));
+}
+
+TEST_F(BrowserActionUnitTest, MissingIconInCommandLine) {
+  InitializeEmptyExtensionService();
+  base::FilePath path =
+      data_dir().AppendASCII("api_test/browser_action/missing_icon");
+  ASSERT_TRUE(base::PathExists(path));
+
+  PackAndInstallCRXWithLocation(path, Manifest::COMMAND_LINE, INSTALL_FAILED);
+}
+
+} // namespace
+} // namespace extensions
diff --git a/chrome/browser/extensions/extension_action_manager_unittest.cc b/chrome/browser/extensions/extension_action_manager_unittest.cc
index d040762..76c47f1 100644
--- a/chrome/browser/extensions/extension_action_manager_unittest.cc
+++ b/chrome/browser/extensions/extension_action_manager_unittest.cc
@@ -75,15 +75,18 @@ scoped_refptr<Extension> ExtensionActionManagerTest::BuildExtension(
     DictionaryBuilder& action,
     const char* action_type) {
   std::string id = base::IntToString(curr_id_++);
-  scoped_refptr<Extension> extension = ExtensionBuilder()
-      .SetManifest(DictionaryBuilder().Set("version", "1")
-          .Set("manifest_version", 2)
-          .Set("icons", extension_icons)
-          .Set(action_type, action)
-          .Set("name",
-              std::string("Test Extension").append(id)))
-      .SetID(id)
-      .Build();
+  scoped_refptr<Extension> extension =
+      ExtensionBuilder()
+          .SetManifest(
+              DictionaryBuilder()
+                  .Set("version", "1")
+                  .Set("manifest_version", 2)
+                  .Set("icons", extension_icons)
+                  .Set(action_type, action)
+                  .Set("name", std::string("Test Extension").append(id)))
+          .SetLocation(Manifest::UNPACKED)
+          .SetID(id)
+          .Build();
   registry_->AddEnabled(extension);
   return extension;
 }
diff --git a/chrome/browser/extensions/extension_service_test_with_install.cc b/chrome/browser/extensions/extension_service_test_with_install.cc
index f2cedef..8f93b2c 100644
--- a/chrome/browser/extensions/extension_service_test_with_install.cc
+++ b/chrome/browser/extensions/extension_service_test_with_install.cc
@@ -126,6 +126,20 @@ const Extension* ExtensionServiceTestWithInstall::PackAndInstallCRX(
                            Extension::NO_FLAGS);
 }
 
+const Extension* ExtensionServiceTestWithInstall::PackAndInstallCRXWithLocation(
+    const base::FilePath& dir_path,
+    Manifest::Location location,
+    InstallState install_state) {
+  // TODO(devlin): reuse another function instead of reimplementing here.
+  base::FilePath crx_path;
+  base::ScopedTempDir temp_dir;
+  EXPECT_TRUE(temp_dir.CreateUniqueTempDir());
+  crx_path = temp_dir.path().AppendASCII("temp.crx");
+
+  PackCRX(dir_path, base::FilePath(), crx_path);
+  return InstallCRXWithLocation(crx_path, location, install_state);
+}
+
 // Attempts to install an extension. Use INSTALL_FAILED if the installation
 // is expected to fail.
 // If |install_state| is INSTALL_UPDATED, and |expected_old_name| is
diff --git a/chrome/browser/extensions/extension_service_test_with_install.h b/chrome/browser/extensions/extension_service_test_with_install.h
index 0ad20eb..a6c3cad 100644
--- a/chrome/browser/extensions/extension_service_test_with_install.h
+++ b/chrome/browser/extensions/extension_service_test_with_install.h
@@ -56,6 +56,9 @@ class ExtensionServiceTestWithInstall : public ExtensionServiceTestBase,
                                      InstallState install_state);
   const Extension* PackAndInstallCRX(const base::FilePath& dir_path,
                                      InstallState install_state);
+  const Extension* PackAndInstallCRXWithLocation(const base::FilePath& dir_path,
+                                                 Manifest::Location location,
+                                                 InstallState install_state);
 
   const Extension* InstallCRX(const base::FilePath& path,
                               InstallState install_state,
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index 0e99851..258cf2d 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -431,6 +431,7 @@
       'browser/extensions/api/dial/dial_service_unittest.cc',
       'browser/extensions/api/easy_unlock_private/easy_unlock_private_api_chromeos_unittest.cc',
       'browser/extensions/api/experience_sampling_private/experience_sampling_private_api_unittest.cc',
+      'browser/extensions/api/extension_action/browser_action_unittest.cc',
       'browser/extensions/api/extension_action/extension_action_prefs_unittest.cc',
       'browser/extensions/api/file_handlers/api_file_handler_util_unittest.cc',
       'browser/extensions/api/file_handlers/mime_util_unittest.cc',
diff --git a/chrome/common/extensions/api/extension_action/action_info.cc b/chrome/common/extensions/api/extension_action/action_info.cc
index 4059a9d..dcc6ce0 100644
--- a/chrome/common/extensions/api/extension_action/action_info.cc
+++ b/chrome/common/extensions/api/extension_action/action_info.cc
@@ -52,7 +52,7 @@ ActionInfo::~ActionInfo() {
 }
 
 // static
-scoped_ptr<ActionInfo> ActionInfo::Load(const Extension* extension,
+scoped_ptr<ActionInfo> ActionInfo::Load(Extension* extension,
                                         const base::DictionaryValue* dict,
                                         base::string16* error) {
   scoped_ptr<ActionInfo> result(new ActionInfo());
@@ -92,7 +92,7 @@ scoped_ptr<ActionInfo> ActionInfo::Load(const Extension* extension,
     std::string default_icon;
     if (dict->GetDictionary(keys::kPageActionDefaultIcon, &icons_value)) {
       if (!manifest_handler_helpers::LoadIconsFromDictionary(
-              icons_value, &result->default_icon, error)) {
+              extension, icons_value, &result->default_icon, error)) {
         return scoped_ptr<ActionInfo>();
       }
     } else if (dict->GetString(keys::kPageActionDefaultIcon, &default_icon) &&
diff --git a/chrome/common/extensions/api/extension_action/action_info.h b/chrome/common/extensions/api/extension_action/action_info.h
index 09e497c..3ad0a25 100644
--- a/chrome/common/extensions/api/extension_action/action_info.h
+++ b/chrome/common/extensions/api/extension_action/action_info.h
@@ -32,7 +32,7 @@ struct ActionInfo {
   };
 
   // Loads an ActionInfo from the given DictionaryValue.
-  static scoped_ptr<ActionInfo> Load(const Extension* extension,
+  static scoped_ptr<ActionInfo> Load(Extension* extension,
                                      const base::DictionaryValue* dict,
                                      base::string16* error);
 
diff --git a/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc b/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc
index 539477b..ea7b4be 100644
--- a/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc
+++ b/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc
@@ -81,6 +81,8 @@ TEST_F(BrowserActionManifestTest,
                                                    .Set("19", "icon19.png")
                                                    .Set("24", "icon24.png")
                                                    .Set("38", "icon38.png"))))
+          // Don't check for existence of icons during parsing.
+          .SetLocation(Manifest::UNPACKED)
           .Build();
 
   ASSERT_TRUE(extension.get());
diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc
index e73ba82..0a7a41f 100644
--- a/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc
+++ b/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc
@@ -15,9 +15,22 @@ namespace extensions {
 class IconsManifestTest : public ChromeManifestTest {
 };
 
+// Icons that don't exist on disk should be ignored (for most manifest
+// locations).
+TEST_F(IconsManifestTest, IgnoreNonExistentIcons) {
+  scoped_refptr<extensions::Extension> extension(
+      LoadAndExpectSuccess("normalize_icon_paths.json", Manifest::INTERNAL));
+  const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get());
+
+  EXPECT_EQ("", icons.Get(extension_misc::EXTENSION_ICON_BITTY,
+                          ExtensionIconSet::MATCH_EXACTLY));
+  EXPECT_EQ("", icons.Get(extension_misc::EXTENSION_ICON_MEDIUM,
+                          ExtensionIconSet::MATCH_EXACTLY));
+}
+
 TEST_F(IconsManifestTest, NormalizeIconPaths) {
   scoped_refptr<extensions::Extension> extension(
-      LoadAndExpectSuccess("normalize_icon_paths.json"));
+      LoadAndExpectSuccess("normalize_icon_paths.json", Manifest::UNPACKED));
   const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get());
 
   EXPECT_EQ("16.png", icons.Get(extension_misc::EXTENSION_ICON_BITTY,
@@ -28,7 +41,7 @@ TEST_F(IconsManifestTest, NormalizeIconPaths) {
 
 TEST_F(IconsManifestTest, IconSizes) {
   scoped_refptr<extensions::Extension> extension(
-      LoadAndExpectSuccess("init_icon_size.json"));
+      LoadAndExpectSuccess("init_icon_size.json", Manifest::UNPACKED));
   const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get());
 
   EXPECT_EQ("16.png", icons.Get(extension_misc::EXTENSION_ICON_BITTY,
diff --git a/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon24.png b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon24.png
new file mode 100644
index 0000000..84c4be3
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon24.png
diff --git a/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon38.png b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon38.png
new file mode 100644
index 0000000..84c4be3
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon38.png
diff --git a/chrome/test/data/extensions/api_test/browser_action/missing_icon/manifest.json b/chrome/test/data/extensions/api_test/browser_action/missing_icon/manifest.json
new file mode 100644
index 0000000..7f6df45
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/missing_icon/manifest.json
@@ -0,0 +1,12 @@
+{
+  "name": "A test extension that tests multiple browser action icons",
+  "version": "1.0",
+  "manifest_version": 2,
+  "browser_action": {
+    "default_icon": {
+      "19": "icon19.png",
+      "24": "icon24.png",
+      "38": "icon38.png"
+    }
+  }
+}
diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon19.png b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon19.png
new file mode 100644
index 0000000..84c4be3
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon19.png
diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon24.png b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon24.png
new file mode 100644
index 0000000..84c4be3
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon24.png
diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon38.png b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon38.png
new file mode 100644
index 0000000..84c4be3
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon38.png
diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json b/chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json
new file mode 100644
index 0000000..694d8102
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json
@@ -0,0 +1,13 @@
+{
+  "name": "A test extension that tests multiple browser action icons",
+  "version": "1.0",
+  "manifest_version": 2,
+  "browser_action": {
+    "default_icon": {
+      "19": "icon19.png",
+      "24": "icon24.png",
+      "31": "icon24.png",
+      "38": "icon38.png"
+    }
+  }
+}
diff --git a/extensions/common/file_util.cc b/extensions/common/file_util.cc
index a187378..a42bdae 100644
--- a/extensions/common/file_util.cc
+++ b/extensions/common/file_util.cc
@@ -52,18 +52,6 @@ enum SafeInstallationFlag {
 };
 SafeInstallationFlag g_use_safe_installation = DEFAULT;
 
-// Returns true if the given file path exists and is not zero-length.
-bool ValidateFilePath(const base::FilePath& path) {
-  int64 size = 0;
-  if (!base::PathExists(path) ||
-      !base::GetFileSize(path, &size) ||
-      size == 0) {
-    return false;
-  }
-
-  return true;
-}
-
 // Returns true if the extension installation should flush all files and the
 // directory.
 bool UseSafeInstallation() {
@@ -271,6 +259,11 @@ scoped_ptr<base::DictionaryValue> LoadManifest(
   return base::DictionaryValue::From(std::move(root));
 }
 
+bool ValidateFilePath(const base::FilePath& path) {
+  int64 size = 0;
+  return base::PathExists(path) && base::GetFileSize(path, &size) && size != 0;
+}
+
 bool ValidateExtension(const Extension* extension,
                        std::string* error,
                        std::vector<InstallWarning>* warnings) {
diff --git a/extensions/common/file_util.h b/extensions/common/file_util.h
index 3fda56a..609b27c 100644
--- a/extensions/common/file_util.h
+++ b/extensions/common/file_util.h
@@ -72,6 +72,9 @@ scoped_ptr<base::DictionaryValue> LoadManifest(
     const base::FilePath::CharType* manifest_filename,
     std::string* error);
 
+// Returns true if the given file path exists and is not zero-length.
+bool ValidateFilePath(const base::FilePath& path);
+
 // Returns true if the given extension object is valid and consistent.
 // May also append a series of warning messages to |warnings|, but they
 // should not prevent the extension from running.
diff --git a/extensions/common/file_util_unittest.cc b/extensions/common/file_util_unittest.cc
index 634b338..5148737 100644
--- a/extensions/common/file_util_unittest.cc
+++ b/extensions/common/file_util_unittest.cc
@@ -416,19 +416,39 @@ TEST_F(FileUtilTest, WarnOnPrivateKey) {
                   "extension includes the key file.*ext_root.a_key.pem"));
 }
 
-TEST_F(FileUtilTest, CheckZeroLengthIconFile) {
+// Try to install an extension with a zero-length icon file.
+TEST_F(FileUtilTest, CheckZeroLengthAndMissingIconFile) {
+  base::FilePath install_dir;
+  ASSERT_TRUE(PathService::Get(DIR_TEST_DATA, &install_dir));
+
+  base::FilePath ext_dir =
+      install_dir.AppendASCII("file_util").AppendASCII("bad_icon");
+
+  std::string error;
+  scoped_refptr<Extension> extension(file_util::LoadExtension(
+      ext_dir, Manifest::INTERNAL, Extension::NO_FLAGS, &error));
+  EXPECT_TRUE(extension);
+  ASSERT_EQ(2U, extension->install_warnings().size());
+
+  EXPECT_EQ("Could not load extension icon 'missing-icon.png'.",
+            extension->install_warnings()[0].message);
+  EXPECT_EQ("Could not load extension icon 'icon.png'.",
+            extension->install_warnings()[1].message);
+}
+
+// Try to install an unpacked extension with a zero-length icon file.
+TEST_F(FileUtilTest, CheckZeroLengthAndMissingIconFileUnpacked) {
   base::FilePath install_dir;
   ASSERT_TRUE(PathService::Get(DIR_TEST_DATA, &install_dir));
 
-  // Try to install an extension with a zero-length icon file.
   base::FilePath ext_dir =
       install_dir.AppendASCII("file_util").AppendASCII("bad_icon");
 
   std::string error;
   scoped_refptr<Extension> extension(file_util::LoadExtension(
       ext_dir, Manifest::UNPACKED, Extension::NO_FLAGS, &error));
-  EXPECT_TRUE(extension.get() == NULL);
-  EXPECT_STREQ("Could not load extension icon 'icon.png'.", error.c_str());
+  EXPECT_FALSE(extension);
+  EXPECT_EQ("Could not load extension icon 'missing-icon.png'.", error);
 }
 
 TEST_F(FileUtilTest, ExtensionURLToRelativeFilePath) {
diff --git a/extensions/common/manifest_handler_helpers.cc b/extensions/common/manifest_handler_helpers.cc
index 4dc2f6b..f04d0f8 100644
--- a/extensions/common/manifest_handler_helpers.cc
+++ b/extensions/common/manifest_handler_helpers.cc
@@ -11,8 +11,10 @@
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_icon_set.h"
+#include "extensions/common/file_util.h"
 #include "extensions/common/manifest_constants.h"
-
+#include "grit/extensions_strings.h"
+#include "ui/base/l10n/l10n_util.h"
 
 namespace extensions {
 
@@ -31,7 +33,8 @@ bool NormalizeAndValidatePath(std::string* path) {
   return true;
 }
 
-bool LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
+bool LoadIconsFromDictionary(Extension* extension,
+                             const base::DictionaryValue* icons_value,
                              ExtensionIconSet* icons,
                              base::string16* error) {
   DCHECK(icons);
@@ -48,7 +51,23 @@ bool LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
       return false;
     }
 
-    icons->Add(size, icon_path);
+    // For backwards compatibility, only warn (don't error out) if an icon is
+    // missing. Component extensions can skip this check as their icons are not
+    // located on disk. Unpacked extensions skip this check and fail later
+    // during validation if the file isn't present. See crbug.com/570249
+    // TODO(estade|devlin): remove this workaround and let install fail in the
+    // validate step a few releases after M49. See http://crbug.com/571193
+    if (Manifest::IsComponentLocation(extension->location()) ||
+        Manifest::IsUnpackedLocation(extension->location()) ||
+        file_util::ValidateFilePath(
+            extension->GetResource(icon_path).GetFilePath())) {
+      icons->Add(size, icon_path);
+    } else {
+      extension->AddInstallWarning(InstallWarning(
+          l10n_util::GetStringFUTF8(IDS_EXTENSION_LOAD_ICON_FAILED,
+                                    base::UTF8ToUTF16(icon_path)),
+          std::string()));
+    }
   }
   return true;
 }
diff --git a/extensions/common/manifest_handler_helpers.h b/extensions/common/manifest_handler_helpers.h
index b0249c3..e1f901a 100644
--- a/extensions/common/manifest_handler_helpers.h
+++ b/extensions/common/manifest_handler_helpers.h
@@ -6,9 +6,11 @@
 #define EXTENSIONS_COMMON_MANIFEST_HANDLER_HELPERS_H_
 
 #include <string>
+#include <vector>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
+#include "extensions/common/install_warning.h"
 
 class ExtensionIconSet;
 
@@ -17,6 +19,9 @@ class DictionaryValue;
 }
 
 namespace extensions {
+
+class Extension;
+
 namespace manifest_handler_helpers {
 
 // Strips leading slashes from the file path. Returns true iff the final path is
@@ -25,8 +30,10 @@ bool NormalizeAndValidatePath(std::string* path);
 
 // Loads icon paths defined in dictionary |icons_value| into ExtensionIconSet
 // |icons|. |icons_value| is a dictionary value {icon size -> icon path}.
-// Returns success. If load fails, |error| will be set.
-bool LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
+// Returns success. If load fails, |error| will be set. Non-failure warnings may
+// be added to |extension|.
+bool LoadIconsFromDictionary(Extension* extension,
+                             const base::DictionaryValue* icons_value,
                              ExtensionIconSet* icons,
                              base::string16* error);
 
diff --git a/extensions/common/manifest_handlers/icons_handler.cc b/extensions/common/manifest_handlers/icons_handler.cc
index 20ad025..52597e5 100644
--- a/extensions/common/manifest_handlers/icons_handler.cc
+++ b/extensions/common/manifest_handlers/icons_handler.cc
@@ -64,7 +64,7 @@ bool IconsHandler::Parse(Extension* extension, base::string16* error) {
   }
 
   if (!manifest_handler_helpers::LoadIconsFromDictionary(
-          icons_dict, &icons_info->icons, error)) {
+          extension, icons_dict, &icons_info->icons, error)) {
     return false;
   }
 
diff --git a/extensions/test/data/file_util/bad_icon/good-icon.png b/extensions/test/data/file_util/bad_icon/good-icon.png
new file mode 100644
index 0000000..d89ed60
--- /dev/null
+++ b/extensions/test/data/file_util/bad_icon/good-icon.png
diff --git a/extensions/test/data/file_util/bad_icon/manifest.json b/extensions/test/data/file_util/bad_icon/manifest.json
index e3846e4..27b7dac 100644
--- a/extensions/test/data/file_util/bad_icon/manifest.json
+++ b/extensions/test/data/file_util/bad_icon/manifest.json
@@ -2,7 +2,9 @@
   "name": "My extension with a zero-length icon",
   "version": "1.0",
   "icons": {
-    "16": "icon.png"
+    "16": "good-icon.png",
+    "32": "missing-icon.png",
+    "64": "icon.png"
   },
   "manifest_version": 2
 }
diff --git a/extensions/utility/unpacker.cc b/extensions/utility/unpacker.cc
index 36c0eba..4763268 100644
--- a/extensions/utility/unpacker.cc
+++ b/extensions/utility/unpacker.cc
@@ -185,9 +185,8 @@ bool Unpacker::Run() {
   // Decode any images that the browser needs to display.
   std::set<base::FilePath> image_paths =
       ExtensionsClient::Get()->GetBrowserImagePaths(extension.get());
-  for (std::set<base::FilePath>::iterator it = image_paths.begin();
-       it != image_paths.end(); ++it) {
-    if (!AddDecodedImage(*it))
+  for (const base::FilePath& path : image_paths) {
+    if (!AddDecodedImage(path))
       return false;  // Error was already reported.
   }