diff options
25 files changed, 214 insertions, 40 deletions
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_unittest.cc b/chrome/browser/extensions/api/extension_action/browser_action_unittest.cc new file mode 100644 index 0000000..8060e9e --- /dev/null +++ b/chrome/browser/extensions/api/extension_action/browser_action_unittest.cc @@ -0,0 +1,70 @@ +// 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 diff --git a/chrome/browser/extensions/extension_action_manager_unittest.cc b/chrome/browser/extensions/extension_action_manager_unittest.cc index d040762..76c47f1 100644 --- a/chrome/browser/extensions/extension_action_manager_unittest.cc +++ b/chrome/browser/extensions/extension_action_manager_unittest.cc @@ -75,15 +75,18 @@ scoped_refptr<Extension> ExtensionActionManagerTest::BuildExtension( DictionaryBuilder& action, const char* action_type) { std::string id = base::IntToString(curr_id_++); - scoped_refptr<Extension> extension = ExtensionBuilder() - .SetManifest(DictionaryBuilder().Set("version", "1") - .Set("manifest_version", 2) - .Set("icons", extension_icons) - .Set(action_type, action) - .Set("name", - std::string("Test Extension").append(id))) - .SetID(id) - .Build(); + scoped_refptr<Extension> extension = + ExtensionBuilder() + .SetManifest( + DictionaryBuilder() + .Set("version", "1") + .Set("manifest_version", 2) + .Set("icons", extension_icons) + .Set(action_type, action) + .Set("name", std::string("Test Extension").append(id))) + .SetLocation(Manifest::UNPACKED) + .SetID(id) + .Build(); registry_->AddEnabled(extension); return extension; } diff --git a/chrome/browser/extensions/extension_service_test_with_install.cc b/chrome/browser/extensions/extension_service_test_with_install.cc index f2cedef..8f93b2c 100644 --- a/chrome/browser/extensions/extension_service_test_with_install.cc +++ b/chrome/browser/extensions/extension_service_test_with_install.cc @@ -126,6 +126,20 @@ 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 diff --git a/chrome/browser/extensions/extension_service_test_with_install.h b/chrome/browser/extensions/extension_service_test_with_install.h index 0ad20eb..a6c3cad 100644 --- a/chrome/browser/extensions/extension_service_test_with_install.h +++ b/chrome/browser/extensions/extension_service_test_with_install.h @@ -56,6 +56,9 @@ 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, diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 0e99851..258cf2d 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -431,6 +431,7 @@ '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', diff --git a/chrome/common/extensions/api/extension_action/action_info.cc b/chrome/common/extensions/api/extension_action/action_info.cc index 4059a9d..dcc6ce0 100644 --- a/chrome/common/extensions/api/extension_action/action_info.cc +++ b/chrome/common/extensions/api/extension_action/action_info.cc @@ -52,7 +52,7 @@ ActionInfo::~ActionInfo() { } // static -scoped_ptr<ActionInfo> ActionInfo::Load(const Extension* extension, +scoped_ptr<ActionInfo> ActionInfo::Load(Extension* extension, const base::DictionaryValue* dict, base::string16* error) { scoped_ptr<ActionInfo> result(new ActionInfo()); @@ -92,7 +92,7 @@ scoped_ptr<ActionInfo> ActionInfo::Load(const Extension* extension, std::string default_icon; if (dict->GetDictionary(keys::kPageActionDefaultIcon, &icons_value)) { if (!manifest_handler_helpers::LoadIconsFromDictionary( - icons_value, &result->default_icon, error)) { + extension, icons_value, &result->default_icon, error)) { return scoped_ptr<ActionInfo>(); } } else if (dict->GetString(keys::kPageActionDefaultIcon, &default_icon) && diff --git a/chrome/common/extensions/api/extension_action/action_info.h b/chrome/common/extensions/api/extension_action/action_info.h index 09e497c..3ad0a25 100644 --- a/chrome/common/extensions/api/extension_action/action_info.h +++ b/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(const Extension* extension, + static scoped_ptr<ActionInfo> Load(Extension* extension, const base::DictionaryValue* dict, base::string16* error); diff --git a/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc b/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc index 539477b..ea7b4be 100644 --- a/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc +++ b/chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc @@ -81,6 +81,8 @@ 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()); diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc index e73ba82..0a7a41f 100644 --- a/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc +++ b/chrome/common/extensions/manifest_tests/extension_manifests_icons_unittest.cc @@ -15,9 +15,22 @@ 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")); + LoadAndExpectSuccess("normalize_icon_paths.json", Manifest::UNPACKED)); const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get()); EXPECT_EQ("16.png", icons.Get(extension_misc::EXTENSION_ICON_BITTY, @@ -28,7 +41,7 @@ TEST_F(IconsManifestTest, NormalizeIconPaths) { TEST_F(IconsManifestTest, IconSizes) { scoped_refptr<extensions::Extension> extension( - LoadAndExpectSuccess("init_icon_size.json")); + LoadAndExpectSuccess("init_icon_size.json", Manifest::UNPACKED)); const ExtensionIconSet& icons = IconsInfo::GetIcons(extension.get()); EXPECT_EQ("16.png", icons.Get(extension_misc::EXTENSION_ICON_BITTY, diff --git a/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon24.png b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon24.png Binary files differnew file mode 100644 index 0000000..84c4be3 --- /dev/null +++ b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon24.png diff --git a/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon38.png b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon38.png Binary files differnew file mode 100644 index 0000000..84c4be3 --- /dev/null +++ b/chrome/test/data/extensions/api_test/browser_action/missing_icon/icon38.png diff --git a/chrome/test/data/extensions/api_test/browser_action/missing_icon/manifest.json b/chrome/test/data/extensions/api_test/browser_action/missing_icon/manifest.json new file mode 100644 index 0000000..7f6df45 --- /dev/null +++ b/chrome/test/data/extensions/api_test/browser_action/missing_icon/manifest.json @@ -0,0 +1,12 @@ +{ + "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" + } + } +} diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon19.png b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon19.png Binary files differnew file mode 100644 index 0000000..84c4be3 --- /dev/null +++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon19.png diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon24.png b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon24.png Binary files differnew file mode 100644 index 0000000..84c4be3 --- /dev/null +++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon24.png diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon38.png b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon38.png Binary files differnew file mode 100644 index 0000000..84c4be3 --- /dev/null +++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/icon38.png diff --git a/chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json b/chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json new file mode 100644 index 0000000..694d8102 --- /dev/null +++ b/chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json @@ -0,0 +1,13 @@ +{ + "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" + } + } +} 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; } diff --git a/extensions/test/data/file_util/bad_icon/good-icon.png b/extensions/test/data/file_util/bad_icon/good-icon.png Binary files differnew file mode 100644 index 0000000..d89ed60 --- /dev/null +++ b/extensions/test/data/file_util/bad_icon/good-icon.png 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 36c0eba..4763268 100644 --- a/extensions/utility/unpacker.cc +++ b/extensions/utility/unpacker.cc @@ -185,9 +185,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. } |
