From 16ca320592ce7ef6cd36500da6e670cc645781cd Mon Sep 17 00:00:00 2001 From: jackhou Date: Thu, 9 Apr 2015 20:45:19 -0700 Subject: [PATCH] Cache --whitelisted-extension-id in SimpleFeature. During startup, CommandLine::HasSwitch(kWhitelistedExtensionID) is called ~1000 times by SimpleFeature. It is the most common switch lookup, the second being kAppsGalleryUpdateUrl at ~300 times. Caching this avoids allocating and copying the string each time. BUG=471976 Review URL: https://codereview.chromium.org/1047943002 Cr-Commit-Position: refs/heads/master@{#324583} --- .../permission_message_combinations_unittest.cc | 15 ++++--- .../extensions/test_extension_environment.cc | 49 +++++++++++++++------- .../extensions/test_extension_environment.h | 5 +++ .../extension_manifests_platformapp_unittest.cc | 6 +-- extensions/common/features/simple_feature.cc | 47 ++++++++++++++------- extensions/common/features/simple_feature.h | 12 ++++++ 6 files changed, 98 insertions(+), 36 deletions(-) diff --git a/chrome/browser/extensions/permission_message_combinations_unittest.cc b/chrome/browser/extensions/permission_message_combinations_unittest.cc index 42764bbb8d65..d4024e5ad11e 100644 --- a/chrome/browser/extensions/permission_message_combinations_unittest.cc +++ b/chrome/browser/extensions/permission_message_combinations_unittest.cc @@ -8,6 +8,7 @@ #include "chrome/browser/extensions/test_extension_environment.h" #include "chrome/common/extensions/permissions/chrome_permission_message_provider.h" #include "extensions/common/extension.h" +#include "extensions/common/features/simple_feature.h" #include "extensions/common/permissions/permission_message_test_util.h" #include "extensions/common/permissions/permissions_data.h" #include "extensions/common/switches.h" @@ -16,12 +17,15 @@ namespace extensions { +const char kWhitelistedExtensionID[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + // Tests that ChromePermissionMessageProvider produces the expected messages for // various combinations of app/extension permissions. class PermissionMessageCombinationsUnittest : public testing::Test { public: PermissionMessageCombinationsUnittest() - : message_provider_(new ChromePermissionMessageProvider()) {} + : message_provider_(new ChromePermissionMessageProvider()), + whitelisted_extension_id_(kWhitelistedExtensionID) {} ~PermissionMessageCombinationsUnittest() override {} // Overridden from testing::Test: @@ -39,10 +43,8 @@ class PermissionMessageCombinationsUnittest : public testing::Test { std::replace(json_manifest_with_double_quotes.begin(), json_manifest_with_double_quotes.end(), '\'', '"'); app_ = env_.MakeExtension( - *base::test::ParseJson(json_manifest_with_double_quotes)); - // Add the app to any whitelists so we can test all permissions. - base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( - switches::kWhitelistedExtensionID, app_->id()); + *base::test::ParseJson(json_manifest_with_double_quotes), + kWhitelistedExtensionID); } // Checks whether the currently installed app or extension produces the given @@ -197,6 +199,9 @@ class PermissionMessageCombinationsUnittest : public testing::Test { extensions::TestExtensionEnvironment env_; scoped_ptr message_provider_; scoped_refptr app_; + // Whitelist a known extension id so we can test all permissions. This ID + // will be used for each test app. + extensions::SimpleFeature::ScopedWhitelistForTest whitelisted_extension_id_; DISALLOW_COPY_AND_ASSIGN(PermissionMessageCombinationsUnittest); }; diff --git a/chrome/browser/extensions/test_extension_environment.cc b/chrome/browser/extensions/test_extension_environment.cc index 8b5c30e406ea..f8478fcbd9e6 100644 --- a/chrome/browser/extensions/test_extension_environment.cc +++ b/chrome/browser/extensions/test_extension_environment.cc @@ -27,6 +27,28 @@ namespace extensions { using content::BrowserThread; +namespace { + +scoped_ptr MakeExtensionManifest( + const base::Value& manifest_extra) { + scoped_ptr manifest = DictionaryBuilder() + .Set("name", "Extension") + .Set("version", "1.0") + .Set("manifest_version", 2) + .Build(); + const base::DictionaryValue* manifest_extra_dict; + if (manifest_extra.GetAsDictionary(&manifest_extra_dict)) { + manifest->MergeDictionary(manifest_extra_dict); + } else { + std::string manifest_json; + base::JSONWriter::Write(&manifest_extra, &manifest_json); + ADD_FAILURE() << "Expected dictionary; got \"" << manifest_json << "\""; + } + return manifest; +} + +} // namespace + TestExtensionEnvironment::TestExtensionEnvironment() : profile_(new TestingProfile), extension_service_(NULL), @@ -68,26 +90,25 @@ ExtensionPrefs* TestExtensionEnvironment::GetExtensionPrefs() { const Extension* TestExtensionEnvironment::MakeExtension( const base::Value& manifest_extra) { - scoped_ptr manifest = DictionaryBuilder() - .Set("name", "Extension") - .Set("version", "1.0") - .Set("manifest_version", 2) - .Build(); - const base::DictionaryValue* manifest_extra_dict; - if (manifest_extra.GetAsDictionary(&manifest_extra_dict)) { - manifest->MergeDictionary(manifest_extra_dict); - } else { - std::string manifest_json; - base::JSONWriter::Write(&manifest_extra, &manifest_json); - ADD_FAILURE() << "Expected dictionary; got \"" << manifest_json << "\""; - } - + scoped_ptr manifest = + MakeExtensionManifest(manifest_extra); scoped_refptr result = ExtensionBuilder().SetManifest(manifest.Pass()).Build(); GetExtensionService()->AddExtension(result.get()); return result.get(); } +const Extension* TestExtensionEnvironment::MakeExtension( + const base::Value& manifest_extra, + const std::string& id) { + scoped_ptr manifest = + MakeExtensionManifest(manifest_extra); + scoped_refptr result = + ExtensionBuilder().SetManifest(manifest.Pass()).SetID(id).Build(); + GetExtensionService()->AddExtension(result.get()); + return result.get(); +} + scoped_ptr TestExtensionEnvironment::MakeTab() const { scoped_ptr contents( content::WebContentsTester::CreateTestWebContents(profile(), NULL)); diff --git a/chrome/browser/extensions/test_extension_environment.h b/chrome/browser/extensions/test_extension_environment.h index 57136588c89a..bea77e1d4e96 100644 --- a/chrome/browser/extensions/test_extension_environment.h +++ b/chrome/browser/extensions/test_extension_environment.h @@ -62,6 +62,11 @@ class TestExtensionEnvironment { // manifest_extra override these defaults. const Extension* MakeExtension(const base::Value& manifest_extra); + // Use a specific extension ID instead of the default generated in + // Extension::Create. + const Extension* MakeExtension(const base::Value& manifest_extra, + const std::string& id); + // Returns a test web contents that has a tab id. scoped_ptr MakeTab() const; diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc index 1e5442ca024f..cdb355bd9031 100644 --- a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc +++ b/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc @@ -8,6 +8,7 @@ #include "chrome/common/extensions/manifest_handlers/app_isolation_info.h" #include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h" #include "extensions/common/error_utils.h" +#include "extensions/common/features/simple_feature.h" #include "extensions/common/manifest_constants.h" #include "extensions/common/manifest_handlers/csp_info.h" #include "extensions/common/manifest_handlers/incognito_info.h" @@ -81,9 +82,8 @@ TEST_F(PlatformAppsManifestTest, PlatformAppContentSecurityPolicy) { // Whitelisted ones can (this is the ID corresponding to the base 64 encoded // key in the init_platform_app_csp.json manifest.) - std::string test_id = "ahplfneplbnjcflhdgkkjeiglkkfeelb"; - base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( - switches::kWhitelistedExtensionID, test_id); + extensions::SimpleFeature::ScopedWhitelistForTest whitelist( + "ahplfneplbnjcflhdgkkjeiglkkfeelb"); scoped_refptr extension = LoadAndExpectSuccess("init_platform_app_csp.json"); EXPECT_EQ(0U, extension->install_warnings().size()) diff --git a/extensions/common/features/simple_feature.cc b/extensions/common/features/simple_feature.cc index 34258d4f0c60..3322facac6d9 100644 --- a/extensions/common/features/simple_feature.cc +++ b/extensions/common/features/simple_feature.cc @@ -11,6 +11,7 @@ #include "base/bind.h" #include "base/command_line.h" #include "base/debug/alias.h" +#include "base/macros.h" #include "base/sha1.h" #include "base/stl_util.h" #include "base/strings/string_number_conversions.h" @@ -27,6 +28,10 @@ namespace extensions { namespace { +// A singleton copy of the --whitelisted-extension-id so that we don't need to +// copy it from the CommandLine each time. +std::string* g_whitelisted_extension_id = NULL; + Feature::Availability IsAvailableToManifestForBind( const std::string& extension_id, Manifest::Type type, @@ -219,8 +224,33 @@ bool IsCommandLineSwitchEnabled(const std::string& switch_name) { return false; } +bool IsWhitelistedForTest(const std::string& extension_id) { + // TODO(jackhou): Delete the commandline whitelisting mechanism. + // Since it is only used it tests, ideally it should not be set via the + // commandline. At the moment the commandline is used as a mechanism to pass + // the id to the renderer process. + if (!g_whitelisted_extension_id) { + g_whitelisted_extension_id = new std::string( + base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( + switches::kWhitelistedExtensionID)); + } + return !g_whitelisted_extension_id->empty() && + *g_whitelisted_extension_id == extension_id; +} + } // namespace +SimpleFeature::ScopedWhitelistForTest::ScopedWhitelistForTest( + const std::string& id) + : previous_id_(g_whitelisted_extension_id) { + g_whitelisted_extension_id = new std::string(id); +} + +SimpleFeature::ScopedWhitelistForTest::~ScopedWhitelistForTest() { + delete g_whitelisted_extension_id; + g_whitelisted_extension_id = previous_id_; +} + struct SimpleFeature::Mappings { Mappings() { extension_types["extension"] = Manifest::TYPE_EXTENSION; @@ -360,20 +390,9 @@ Feature::Availability SimpleFeature::IsAvailableToManifest( if (component_extensions_auto_granted_ && location == Manifest::COMPONENT) return CreateAvailability(IS_AVAILABLE, type); - if (!whitelist_.empty()) { - if (!IsIdInWhitelist(extension_id)) { - // TODO(aa): This is gross. There should be a better way to test the - // whitelist. - base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); - if (!command_line->HasSwitch(switches::kWhitelistedExtensionID)) - return CreateAvailability(NOT_FOUND_IN_WHITELIST, type); - - std::string whitelist_switch_value = - base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( - switches::kWhitelistedExtensionID); - if (extension_id != whitelist_switch_value) - return CreateAvailability(NOT_FOUND_IN_WHITELIST, type); - } + if (!whitelist_.empty() && !IsIdInWhitelist(extension_id) && + !IsWhitelistedForTest(extension_id)) { + return CreateAvailability(NOT_FOUND_IN_WHITELIST, type); } if (!MatchesManifestLocation(location)) diff --git a/extensions/common/features/simple_feature.h b/extensions/common/features/simple_feature.h index 9d8c539414bb..5ec95cf91b4a 100644 --- a/extensions/common/features/simple_feature.h +++ b/extensions/common/features/simple_feature.h @@ -29,6 +29,18 @@ class SimpleFeatureTest; class SimpleFeature : public Feature { public: + // Used by tests to override the cached --whitelisted-extension-id. + class ScopedWhitelistForTest { + public: + explicit ScopedWhitelistForTest(const std::string& id); + ~ScopedWhitelistForTest(); + + private: + std::string* previous_id_; + + DISALLOW_COPY_AND_ASSIGN(ScopedWhitelistForTest); + }; + SimpleFeature(); ~SimpleFeature() override; -- 2.11.4.GIT