Revert of Change extension icon load errors to warnings (patchset #8 id:140001...

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

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

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

chrome/browser/extensions/api/extension_action/browser_action_unittest.cc

-// 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

chrome/browser/extensions/extension_action_manager_unittest.cc

@@ -84,7 +84,6 @@ scoped_refptr<Extension> ExtensionActionManagerTest::BuildExtension(
                   .Set("icons", std::move(extension_icons))
                   .Set(action_type, std::move(action))
                   .Set("name", std::string("Test Extension").append(id))))
-          .SetLocation(Manifest::UNPACKED)
           .SetID(id)
           .Build();
   registry_->AddEnabled(extension);

chrome/browser/extensions/extension_service_test_with_install.cc

@@ -126,20 +126,6 @@ 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

chrome/browser/extensions/extension_service_test_with_install.h

@@ -58,9 +58,6 @@ 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,

chrome/chrome_tests_unit.gypi

@@ -433,7 +433,6 @@
       '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',

chrome/common/extensions/api/extension_action/action_info.cc

@@ -52,7 +52,7 @@ ActionInfo::~ActionInfo() {
 }
 
 // static
-scoped_ptr<ActionInfo> ActionInfo::Load(Extension* extension,
+scoped_ptr<ActionInfo> ActionInfo::Load(const Extension* extension,
                                         const base::DictionaryValue* dict,
                                         base::string16* error) {
   scoped_ptr<ActionInfo> result(new ActionInfo());
@@ -92,7 +92,7 @@ scoped_ptr<ActionInfo> ActionInfo::Load(Extension* extension,
     std::string default_icon;
     if (dict->GetDictionary(keys::kPageActionDefaultIcon, &icons_value)) {
       if (!manifest_handler_helpers::LoadIconsFromDictionary(
-              extension, icons_value, &result->default_icon, error)) {
+              icons_value, &result->default_icon, error)) {
         return scoped_ptr<ActionInfo>();
       }
     } else if (dict->GetString(keys::kPageActionDefaultIcon, &default_icon) &&

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(Extension* extension,
+  static scoped_ptr<ActionInfo> Load(const Extension* extension,
                                      const base::DictionaryValue* dict,
                                      base::string16* error);
 

chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc

@@ -86,8 +86,6 @@ 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());

chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc

@@ -15,22 +15,9 @@ 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", Manifest::UNPACKED));
+      LoadAndExpectSuccess("normalize_icon_paths.json"));
   const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get());
 
   EXPECT_EQ("16.png", icons.Get(extension_misc::EXTENSION_ICON_BITTY,
@@ -41,7 +28,7 @@ TEST_F(IconsManifestTest, NormalizeIconPaths) {
 
 TEST_F(IconsManifestTest, IconSizes) {
   scoped_refptr<extensions::Extension> extension(
-      LoadAndExpectSuccess("init_icon_size.json", Manifest::UNPACKED));
+      LoadAndExpectSuccess("init_icon_size.json"));
   const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get());
 
   EXPECT_EQ("16.png", icons.Get(extension_misc::EXTENSION_ICON_BITTY,

chrome/test/data/extensions/api_test/browser_action/missing_icon/manifest.json

-{
-  "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"
-    }
-  }
-}

chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json

-{
-  "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"
-    }
-  }
-}

extensions/common/file_util.cc

@@ -56,6 +56,16 @@ 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() {

extensions/common/file_util.h

@@ -72,9 +72,6 @@ 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.

extensions/common/file_util_unittest.cc

@@ -419,39 +419,19 @@ TEST_F(FileUtilTest, WarnOnPrivateKey) {
                   "extension includes the key file.*ext_root.a_key.pem"));
 }
 
-// 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) {
+TEST_F(FileUtilTest, CheckZeroLengthIconFile) {
   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_FALSE(extension);
-  EXPECT_EQ("Could not load extension icon 'missing-icon.png'.", error);
+  EXPECT_TRUE(extension.get() == NULL);
+  EXPECT_STREQ("Could not load extension icon 'icon.png'.", error.c_str());
 }
 
 TEST_F(FileUtilTest, ExtensionURLToRelativeFilePath) {

extensions/common/manifest_handler_helpers.cc

@@ -13,10 +13,7 @@
 #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 {
 
@@ -35,8 +32,7 @@ bool NormalizeAndValidatePath(std::string* path) {
   return true;
 }
 
-bool LoadIconsFromDictionary(Extension* extension,
-                             const base::DictionaryValue* icons_value,
+bool LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
                              ExtensionIconSet* icons,
                              base::string16* error) {
   DCHECK(icons);
@@ -53,23 +49,7 @@ bool LoadIconsFromDictionary(Extension* extension,
       return false;
     }
 
-    // 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()));
-    }
+    icons->Add(size, icon_path);
   }
   return true;
 }

extensions/common/manifest_handler_helpers.h

@@ -6,11 +6,9 @@
 #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;
 
@@ -19,9 +17,6 @@ class DictionaryValue;
 }
 
 namespace extensions {
-
-class Extension;
-
 namespace manifest_handler_helpers {
 
 // Strips leading slashes from the file path. Returns true iff the final path is
@@ -30,10 +25,8 @@ 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. Non-failure warnings may
-// be added to |extension|.
-bool LoadIconsFromDictionary(Extension* extension,
-                             const base::DictionaryValue* icons_value,
+// Returns success. If load fails, |error| will be set.
+bool LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
                              ExtensionIconSet* icons,
                              base::string16* error);
 

extensions/common/manifest_handlers/icons_handler.cc

@@ -64,7 +64,7 @@ bool IconsHandler::Parse(Extension* extension, base::string16* error) {
   }
 
   if (!manifest_handler_helpers::LoadIconsFromDictionary(
-          extension, icons_dict, &icons_info->icons, error)) {
+          icons_dict, &icons_info->icons, error)) {
     return false;
   }
 

extensions/test/data/file_util/bad_icon/manifest.json

@@ -2,9 +2,7 @@
   "name": "My extension with a zero-length icon",
   "version": "1.0",
   "icons": {
-    "16": "good-icon.png",
-    "32": "missing-icon.png",
-    "64": "icon.png"
+    "16": "icon.png"
   },
   "manifest_version": 2
 }

extensions/utility/unpacker.cc

@@ -187,8 +187,9 @@ bool Unpacker::Run() {
   // Decode any images that the browser needs to display.
   std::set<base::FilePath> image_paths =
       ExtensionsClient::Get()->GetBrowserImagePaths(extension.get());
-  for (const base::FilePath& path : image_paths) {
-    if (!AddDecodedImage(path))
+  for (std::set<base::FilePath>::iterator it = image_paths.begin();
+       it != image_paths.end(); ++it) {
+    if (!AddDecodedImage(*it))
       return false;  // Error was already reported.
   }