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}

6 files changed, 64 insertions, 22 deletions

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;
   }