Reland of Change extension icon load errors to warnings (patchset #2 id:300001 of https://codereview.chromium.org/1554583002/ )

Reason for revert:
this revert managed to break the tree

Original issue's description:
> Revert of Change extension icon load errors to warnings (patchset #8 id:140001 of https://codereview.chromium.org/1537473003/ )
>
> Reason for revert:
> file read from wrong thread
>
> Original issue's description:
> > 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
> >
> > Committed: https://crrev.com/6e8e7d1c49657e82d0e8f2518ad463794346321b
> > Cr-Commit-Position: refs/heads/master@{#366253}
>
> TBR=rdevlin.cronin@chromium.org
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=570249
>
> Committed: https://crrev.com/36dab10ffd04f3e38285bda1e4af07a7de081a31
> Cr-Commit-Position: refs/heads/master@{#367080}

TBR=rdevlin.cronin@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=570249

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

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

8 files changed, 64 insertions, 23 deletions

diff --git a/extensions/common/file_util.cc b/extensions/common/file_util.cc
index 90a7846..3ddf6d0 100644
--- a/extensions/common/file_util.cc
+++ b/extensions/common/file_util.cc
@@ -56,16 +56,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() {
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 0bfb8b9..afbcf3c 100644
--- a/extensions/common/file_util_unittest.cc
+++ b/extensions/common/file_util_unittest.cc
@@ -419,19 +419,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 bfc9e33..36af224d 100644
--- a/extensions/common/manifest_handler_helpers.cc
+++ b/extensions/common/manifest_handler_helpers.cc
@@ -13,7 +13,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 {
 
@@ -32,7 +35,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);
@@ -49,7 +53,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/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 c230f82..be0a4b5 100644
--- a/extensions/utility/unpacker.cc
+++ b/extensions/utility/unpacker.cc
@@ -187,9 +187,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.
   }