Change extension icon load errors to warnings

extensions/common/manifest_handler_helpers.cc

 // Copyright 2014 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 "extensions/common/manifest_handler_helpers.h"
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #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 {
 
 namespace errors = manifest_errors;
 
 namespace manifest_handler_helpers {
 
 bool NormalizeAndValidatePath(std::string* path) {
   size_t first_non_slash = path->find_first_not_of('/');
   if (first_non_slash == std::string::npos) {
     *path = "";
     return false;
   }
 
   *path = path->substr(first_non_slash);
   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);
   DCHECK(error);
   for (base::DictionaryValue::Iterator iterator(*icons_value);
        !iterator.IsAtEnd(); iterator.Advance()) {
     int size = 0;
     std::string icon_path;
     if (!base::StringToInt(iterator.key(), &size) ||
         !iterator.value().GetAsString(&icon_path) ||
         !NormalizeAndValidatePath(&icon_path)) {
       *error = ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidIconPath,
                                                    iterator.key());
       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;
 }
 
 }  // namespace manifest_handler_helpers
 
 }  // namespace extensions