Change extension icon load errors to warnings

extensions/common/manifest_handler_helpers.cc

Index: extensions/common/manifest_handler_helpers.cc
diff --git a/extensions/common/manifest_handler_helpers.cc b/extensions/common/manifest_handler_helpers.cc
index 4dc2f6b0837f204a39a30ca28eef5d691f556714..e5271f5e2ef9dea7b70c42a68fd81f5d06ce97f3 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,21 @@ 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
+    if (Manifest::IsComponentLocation(extension->location()) ||
+        Manifest::IsUnpackedLocation(extension->location()) ||
+        file_util::ValidateFilePath(

[Evan Stade, 2015/12/17 04:27:56]

to be honest, I'm not super happy with this because it seems like a violation of
the Parse/Validate contract (the former just looks at the manifest and the
latter checks that the manifest makes sense relative to the rest of the
extension pack). However, editing the icon set during validation would require
breaking a lot of const qualifiers (on Extension, ActionIcon, etc.).

I'd be more happy just telling extension developers that what was previously
undefined behavior but clearly broken and just happened to be ignored by Chrome
is now defined as an error, especially because in this case the fix is so dang
easy (delete a line from the manifest).

[Devlin, 2015/12/17 17:23:08]

I agree... :/

In my mind, this would be a little cleaner (though still far from ideal):
- If the path doesn't exist, add the size and an empty path.
- Skip empty paths in getting the icon.
- Validate() adds an install warning (or error) on any empty paths.

It results in adding a useless entry to the map, but doesn't break anything else
and keeps all the error/install warnings in one place, which I think is more
important.
My worry is just because extensions can be auto-updated, this risks a bunch of
weirdness with extensions that exist on the store already.  I'd say let's give
it a few milestones and then we can just fail.

[Evan Stade, 2015/12/17 20:15:58]

I tried this at first, but it's not correct that nothing else breaks. There were
more errors down the line and it seems as though many places assume that if it's
in the map and the extension has passed validation, it's valid. The best way to
fix this that I found was to actually remove it from the map.

[Devlin, 2015/12/17 22:05:57]

Well, shoot.  Okay.

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