From b6ab96da13cdfe15f38fbebcd22e0a1db5a50381 Mon Sep 17 00:00:00 2001 From: "mpcomplete@chromium.org" Date: Thu, 20 Aug 2009 18:58:19 +0000 Subject: Get rid of the extension's "Current Version" file. The entire manifest.json value is now stored in the prefs file. This will allow for quick extension checks on startup. BUG=18293 TEST=Make sure installing/upgrading/uninstalling extensions works as expected. Review URL: http://codereview.chromium.org/174036 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@23848 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/extensions/crx_installer.cc | 25 +-- chrome/browser/extensions/crx_installer.h | 4 + chrome/browser/extensions/extension_file_util.cc | 174 +++++++-------------- chrome/browser/extensions/extension_file_util.h | 44 +++--- .../extensions/extension_file_util_unittest.cc | 100 +++--------- chrome/browser/extensions/extension_prefs.cc | 44 +++++- chrome/browser/extensions/extension_prefs.h | 16 +- chrome/browser/extensions/extensions_service.cc | 62 ++++++-- chrome/browser/extensions/extensions_service.h | 4 +- .../extensions/extensions_service_unittest.cc | 48 ++---- chrome/common/extensions/extension.cc | 9 +- chrome/common/extensions/extension.h | 9 +- .../Current Version | 1 - .../Current Version | 1 - .../Current Version | 1 - 15 files changed, 254 insertions(+), 288 deletions(-) delete mode 100644 chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpglgmj/Current Version delete mode 100644 chrome/test/data/extensions/good/Extensions/bjafgdebaacbbbecmhlhpofkepfkgcpa/Current Version delete mode 100644 chrome/test/data/extensions/good/Extensions/hpiknbiabeeppbpihjehijgoemciehgk/Current Version diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc index 352cd8c..677aada 100644 --- a/chrome/browser/extensions/crx_installer.cc +++ b/chrome/browser/extensions/crx_installer.cc @@ -171,6 +171,9 @@ void CrxInstaller::ConfirmInstall() { return; } + current_version_ = + frontend_->extension_prefs()->GetVersionString(extension_->id()); + if (client_.get()) { AddRef(); // balanced in ContinueInstall() and AbortInstall(). client_->ConfirmInstall(this, extension_.get(), install_icon_.get()); @@ -199,17 +202,10 @@ void CrxInstaller::CompleteInstall() { DCHECK(MessageLoop::current() == file_loop_); FilePath version_dir; - Extension::InstallType install_type = Extension::INSTALL_ERROR; - std::string error_msg; - if (!extension_file_util::InstallExtension(unpacked_extension_root_, - install_directory_, - extension_->id(), - extension_->VersionString(), - &version_dir, - &install_type, &error_msg)) { - ReportFailureFromFileThread(error_msg); - return; - } + Extension::InstallType install_type = + extension_file_util::CompareToInstalledVersion( + install_directory_, extension_->id(), current_version_, + extension_->VersionString(), &version_dir); if (install_type == Extension::DOWNGRADE) { ReportFailureFromFileThread("Attempted to downgrade extension."); @@ -222,6 +218,13 @@ void CrxInstaller::CompleteInstall() { return; } + std::string error_msg; + if (!extension_file_util::InstallExtension(unpacked_extension_root_, + version_dir, &error_msg)) { + ReportFailureFromFileThread(error_msg); + return; + } + // This is lame, but we must reload the extension because absolute paths // inside the content scripts are established inside InitFromValue() and we // just moved the extension. diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h index d7ae504..c1a4d55 100644 --- a/chrome/browser/extensions/crx_installer.h +++ b/chrome/browser/extensions/crx_installer.h @@ -138,6 +138,10 @@ class CrxInstaller : // ExtensionsService on success, or delete it on failure. scoped_ptr extension_; + // If non-empty, contains the current version of the extension we're + // installing (for upgrades). + std::string current_version_; + // The icon we will display in the installation UI, if any. scoped_ptr install_icon_; diff --git a/chrome/browser/extensions/extension_file_util.cc b/chrome/browser/extensions/extension_file_util.cc index bf6cd9c..9c15da3 100644 --- a/chrome/browser/extensions/extension_file_util.cc +++ b/chrome/browser/extensions/extension_file_util.cc @@ -8,6 +8,7 @@ #include "base/logging.h" #include "base/scoped_temp_dir.h" #include "base/string_util.h" +#include "chrome/browser/extensions/extension_prefs.h" #include "chrome/browser/extensions/extension_l10n_util.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_constants.h" @@ -17,6 +18,8 @@ namespace extension_file_util { const char kInstallDirectoryName[] = "Extensions"; +// TODO(mpcomplete): obsolete. remove after migration period. +// http://code.google.com/p/chromium/issues/detail?id=19733 const char kCurrentVersionFileName[] = "Current Version"; bool MoveDirSafely(const FilePath& source_dir, const FilePath& dest_dir) { @@ -37,76 +40,34 @@ bool MoveDirSafely(const FilePath& source_dir, const FilePath& dest_dir) { return true; } -bool SetCurrentVersion(const FilePath& dest_dir, const std::string& version, - std::string* error) { - // Write out the new CurrentVersion file. - // /Extension//CurrentVersion - FilePath current_version = dest_dir.AppendASCII(kCurrentVersionFileName); - FilePath current_version_old = - current_version.InsertBeforeExtension(FILE_PATH_LITERAL("_old")); - if (file_util::PathExists(current_version_old)) { - if (!file_util::Delete(current_version_old, false)) { - *error = "Couldn't remove CurrentVersion_old file."; - return false; - } - } - - if (file_util::PathExists(current_version)) { - if (!file_util::Move(current_version, current_version_old)) { - *error = "Couldn't move CurrentVersion file."; - return false; - } - } - net::FileStream stream; - int flags = base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_WRITE; - if (stream.Open(current_version, flags) != 0) - return false; - if (stream.Write(version.c_str(), version.size(), NULL) < 0) { - // Restore the old CurrentVersion. - if (file_util::PathExists(current_version_old)) { - if (!file_util::Move(current_version_old, current_version)) { - LOG(WARNING) << "couldn't restore " << current_version_old.value() << - " to " << current_version.value(); - - // TODO(erikkay): This is an ugly state to be in. Try harder? - } - } - *error = "Couldn't create CurrentVersion file."; - return false; - } - return true; -} - -bool ReadCurrentVersion(const FilePath& dir, std::string* version_string) { - FilePath current_version = dir.AppendASCII(kCurrentVersionFileName); - if (file_util::PathExists(current_version)) { - if (file_util::ReadFileToString(current_version, version_string)) { - TrimWhitespaceASCII(*version_string, TRIM_ALL, version_string); - return true; - } - } - return false; -} - Extension::InstallType CompareToInstalledVersion( - const FilePath& install_directory, const std::string& id, - const std::string& new_version_str, std::string *current_version_str) { - CHECK(current_version_str); - FilePath dir(install_directory.AppendASCII(id.c_str())); - if (!ReadCurrentVersion(dir, current_version_str)) + const FilePath& extensions_dir, + const std::string& extension_id, + const std::string& current_version_str, + const std::string& new_version_str, + FilePath* version_dir) { + FilePath dest_dir = extensions_dir.AppendASCII(extension_id); + FilePath current_version_dir = dest_dir.AppendASCII(current_version_str); + *version_dir = dest_dir.AppendASCII(new_version_str); + + if (current_version_str.empty()) return Extension::NEW_INSTALL; scoped_ptr current_version( - Version::GetVersionFromString(*current_version_str)); + Version::GetVersionFromString(current_version_str)); scoped_ptr new_version( Version::GetVersionFromString(new_version_str)); int comp = new_version->CompareTo(*current_version); if (comp > 0) return Extension::UPGRADE; - else if (comp == 0) - return Extension::REINSTALL; - else + if (comp < 0) return Extension::DOWNGRADE; + + // Same version. Treat corrupted existing installation as new install case. + if (!SanityCheckExtension(current_version_dir)) + return Extension::NEW_INSTALL; + + return Extension::REINSTALL; } bool SanityCheckExtension(const FilePath& dir) { @@ -118,44 +79,17 @@ bool SanityCheckExtension(const FilePath& dir) { } bool InstallExtension(const FilePath& src_dir, - const FilePath& extensions_dir, - const std::string& extension_id, - const std::string& extension_version, - FilePath* version_dir, - Extension::InstallType* install_type, + const FilePath& version_dir, std::string* error) { - FilePath dest_dir = extensions_dir.AppendASCII(extension_id); - *version_dir = dest_dir.AppendASCII(extension_version); - - std::string current_version; - *install_type = CompareToInstalledVersion( - extensions_dir, extension_id, extension_version, ¤t_version); - - // Do not allow downgrade. - if (*install_type == Extension::DOWNGRADE) - return true; - - if (*install_type == Extension::REINSTALL) { - if (!SanityCheckExtension(*version_dir)) { - // Treat corrupted existing installation as new install case. - *install_type = Extension::NEW_INSTALL; - } else { - return true; - } - } - // If anything fails after this, we want to delete the extension dir. ScopedTempDir scoped_version_dir; - scoped_version_dir.Set(*version_dir); + scoped_version_dir.Set(version_dir); - if (!MoveDirSafely(src_dir, *version_dir)) { + if (!MoveDirSafely(src_dir, version_dir)) { *error = "Could not move extension directory into profile."; return false; } - if (!SetCurrentVersion(dest_dir, extension_version, error)) - return false; - scoped_version_dir.Take(); return true; } @@ -184,13 +118,20 @@ Extension* LoadExtension(const FilePath& extension_path, bool require_key, require_key, error)) return NULL; + if (!ValidateExtension(extension.get(), error)) + return NULL; + + return extension.release(); +} + +bool ValidateExtension(Extension* extension, std::string* error) { // Validate icons exist. for (std::map::const_iterator iter = extension->icons().begin(); iter != extension->icons().end(); ++iter) { if (!file_util::PathExists(extension->GetResourcePath(iter->second))) { *error = StringPrintf("Could not load extension icon '%s'.", iter->second.c_str()); - return NULL; + return false; } } @@ -207,14 +148,14 @@ Extension* LoadExtension(const FilePath& extension_path, bool require_key, *error = StringPrintf( "Could not load '%s' for theme.", WideToUTF8(image_path.ToWStringHack()).c_str()); - return NULL; + return false; } } } } // Themes cannot contain other extension types. - return extension.release(); + return true; } // Validate that claimed script resources actually exist. @@ -226,7 +167,7 @@ Extension* LoadExtension(const FilePath& extension_path, bool require_key, if (!file_util::PathExists(path)) { *error = StringPrintf("Could not load '%s' for content script.", WideToUTF8(path.ToWStringHack()).c_str()); - return NULL; + return false; } } @@ -235,7 +176,7 @@ Extension* LoadExtension(const FilePath& extension_path, bool require_key, if (!file_util::PathExists(path)) { *error = StringPrintf("Could not load '%s' for content script.", WideToUTF8(path.ToWStringHack()).c_str()); - return NULL; + return false; } } } @@ -246,7 +187,7 @@ Extension* LoadExtension(const FilePath& extension_path, bool require_key, if (!file_util::PathExists(plugin.path)) { *error = StringPrintf("Could not load '%s' for plugin.", WideToUTF8(plugin.path.ToWStringHack()).c_str()); - return NULL; + return false; } } @@ -257,7 +198,7 @@ Extension* LoadExtension(const FilePath& extension_path, bool require_key, if (!file_util::PathExists(blacklist.path)) { *error = StringPrintf("Could not load '%s' for privacy blacklist.", WideToUTF8(blacklist.path.ToWStringHack()).c_str()); - return NULL; + return false; } } @@ -272,33 +213,34 @@ Extension* LoadExtension(const FilePath& extension_path, bool require_key, if (!file_util::PathExists(extension->GetResourcePath(*iter))) { *error = StringPrintf("Could not load icon '%s' for page action.", iter->c_str()); - return NULL; + return false; } } } // Load locale information if available. - FilePath locale_path = extension_path.AppendASCII(Extension::kLocaleFolder); + FilePath locale_path = + extension->path().AppendASCII(Extension::kLocaleFolder); if (file_util::PathExists(locale_path)) { if (!extension_l10n_util::AddValidLocales(locale_path, - extension.get(), + extension, error)) { - return NULL; + return false; } - if (!extension_l10n_util::ValidateDefaultLocale(extension.get())) { + if (!extension_l10n_util::ValidateDefaultLocale(extension)) { *error = extension_manifest_errors::kLocalesNoDefaultLocaleSpecified; - return NULL; + return false; } } // Check children of extension root to see if any of them start with _ and is // not on the reserved list. - if (!CheckForIllegalFilenames(extension_path, error)) { - return NULL; + if (!CheckForIllegalFilenames(extension->path(), error)) { + return false; } - return extension.release(); + return true; } void UninstallExtension(const std::string& id, const FilePath& extensions_dir) { @@ -314,8 +256,7 @@ void UninstallExtension(const std::string& id, const FilePath& extensions_dir) { FilePath current_version_file = extension_root.AppendASCII( kCurrentVersionFileName); if (!file_util::PathExists(current_version_file)) { - LOG(WARNING) << "Extension " << id - << " does not have a Current Version file."; + // This is OK, since we're phasing out the current version file. } else { if (!file_util::Delete(current_version_file, false)) { LOG(WARNING) << "Could not delete Current Version file for extension " @@ -332,7 +273,8 @@ void UninstallExtension(const std::string& id, const FilePath& extensions_dir) { LOG(WARNING) << "Could not delete directory for extension " << id; } -void GarbageCollectExtensions(const FilePath& install_directory) { +void GarbageCollectExtensions(const FilePath& install_directory, + const std::set& installed_ids) { // Nothing to clean up if it doesn't exist. if (!file_util::DirectoryExists(install_directory)) return; @@ -347,13 +289,11 @@ void GarbageCollectExtensions(const FilePath& install_directory) { std::string extension_id = WideToASCII( extension_path.BaseName().ToWStringHack()); - // If there is no Current Version file, just delete the directory and move - // on. This can legitimately happen when an uninstall does not complete, for - // example, when a plugin is in use at uninstall time. - FilePath current_version_path = extension_path.AppendASCII( - kCurrentVersionFileName); - if (!file_util::PathExists(current_version_path)) { - LOG(INFO) << "Deleting incomplete install for directory " + // If there is no entry in the prefs file, just delete the directory and + // move on. This can legitimately happen when an uninstall does not + // complete, for example, when a plugin is in use at uninstall time. + if (installed_ids.count(extension_id) == 0) { + LOG(INFO) << "Deleting unreferenced install for directory " << WideToASCII(extension_path.ToWStringHack()) << "."; file_util::Delete(extension_path, true); // Recursive. continue; @@ -409,4 +349,4 @@ bool CheckForIllegalFilenames(const FilePath& extension_path, return true; } -} // extensionfile_util +} // extension_file_util diff --git a/chrome/browser/extensions/extension_file_util.h b/chrome/browser/extensions/extension_file_util.h index c2eb5c9..9c13ea2 100644 --- a/chrome/browser/extensions/extension_file_util.h +++ b/chrome/browser/extensions/extension_file_util.h @@ -5,6 +5,7 @@ #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FILE_UTIL_H_ #define CHROME_BROWSER_EXTENSIONS_EXTENSION_FILE_UTIL_H_ +#include #include #include "base/file_path.h" @@ -33,24 +34,26 @@ bool SetCurrentVersion(const FilePath& dest_dir, const std::string& version, // Reads the Current Version file. bool ReadCurrentVersion(const FilePath& dir, std::string* version_string); -// Determine what type of install (new, upgrade, overinstall, downgrade) a given -// id/version combination is. Also returns the current version, if any, in -// current_version_str. +// Determine what type of install it is (new, upgrade, overinstall, downgrade) +// given the current version and a newly installing version. |extensions_dir| is +// the root directory containing all extensions in the user profile. +// |extension_id| and |current_version_str| are the id and version of the +// extension contained in |src_dir|, if any. +// +// Returns the full path to the destination version directory and the type of +// install that was attempted. Extension::InstallType CompareToInstalledVersion( - const FilePath& install_directory, const std::string& id, - const std::string& new_version_str, std::string *current_version_str); + const FilePath& extensions_dir, + const std::string& extension_id, + const std::string& current_version_str, + const std::string& new_version_str, + FilePath* version_dir); // Sanity check that the directory has the minimum files to be a working // extension. bool SanityCheckExtension(const FilePath& extension_root); -// Installs an extension unpacked in |src_dir|. |extensions_dir| is the root -// directory containing all extensions in the user profile. |extension_id| and -// |extension_version| are the id and version of the extension contained in -// |src_dir|. -// -// Returns the full path to the destination version directory and the type of -// install that was attempted. +// Installs an extension unpacked in |src_dir|. // // On failure, also returns an error message. // @@ -58,25 +61,26 @@ bool SanityCheckExtension(const FilePath& extension_root); // overinstall of previous verisons of the extension. In that case, the function // returns true and install_type is populated. bool InstallExtension(const FilePath& src_dir, - const FilePath& extensions_dir, - const std::string& extension_id, - const std::string& extension_version, - FilePath* version_dir, - Extension::InstallType* install_type, + const FilePath& version_dir, std::string* error); -// Load an extension from the specified directory. Returns NULL on failure, with -// a description of the error in |error|. +// Loads and validates an extension from the specified directory. Returns NULL +// on failure, with a description of the error in |error|. Extension* LoadExtension(const FilePath& extension_root, bool require_key, std::string* error); +// Returns true if the given extension object is valid and consistent. +// Otherwise, a description of the error is returned in |error|. +bool ValidateExtension(Extension* extension, std::string* error); + // Uninstalls the extension |id| from the install directory |extensions_dir|. void UninstallExtension(const std::string& id, const FilePath& extensions_dir); // Clean up directories that aren't valid extensions from the install directory. // TODO(aa): Also consider passing in a list of known current extensions and // removing others? -void GarbageCollectExtensions(const FilePath& extensions_dir); +void GarbageCollectExtensions(const FilePath& extensions_dir, + const std::set& installed_ids); // We need to reserve the namespace of entries that start with "_" for future // use by Chrome. diff --git a/chrome/browser/extensions/extension_file_util_unittest.cc b/chrome/browser/extensions/extension_file_util_unittest.cc index 73dfb46..9f99606 100644 --- a/chrome/browser/extensions/extension_file_util_unittest.cc +++ b/chrome/browser/extensions/extension_file_util_unittest.cc @@ -61,110 +61,54 @@ TEST(ExtensionFileUtil, MoveDirSafely) { ASSERT_FALSE(file_util::PathExists(src_path)); } -TEST(ExtensionFileUtil, SetCurrentVersion) { - // Create an empty test directory. - ScopedTempDir temp; - ASSERT_TRUE(temp.CreateUniqueTempDir()); - - // Set its version. - std::string error; - std::string version = "1.0"; - ASSERT_TRUE(extension_file_util::SetCurrentVersion(temp.path(), version, - &error)); - - // Test the result. - std::string version_out; - ASSERT_TRUE(file_util::ReadFileToString( - temp.path().AppendASCII(extension_file_util::kCurrentVersionFileName), - &version_out)); - ASSERT_EQ(version, version_out); - - // Try again, overwriting the old value. - version = "2.0"; - version_out.clear(); - ASSERT_TRUE(extension_file_util::SetCurrentVersion(temp.path(), version, - &error)); - ASSERT_TRUE(file_util::ReadFileToString( - temp.path().AppendASCII(extension_file_util::kCurrentVersionFileName), - &version_out)); - ASSERT_EQ(version, version_out); -} - -TEST(ExtensionFileUtil, ReadCurrentVersion) { - // Read the version from a valid extension. - FilePath extension_path; - ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extension_path)); - extension_path = extension_path.AppendASCII("extensions") - .AppendASCII("good") - .AppendASCII("Extensions") - .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj"); - - std::string version; - ASSERT_TRUE(extension_file_util::ReadCurrentVersion(extension_path, - &version)); - ASSERT_EQ("1.0.0.0", version); - - // Create an invalid extension and read its current version. - ScopedTempDir temp; - ASSERT_TRUE(temp.CreateUniqueTempDir()); - ASSERT_FALSE(extension_file_util::ReadCurrentVersion(temp.path(), &version)); -} - TEST(ExtensionFileUtil, CompareToInstalledVersion) { // Compare to an existing extension. - FilePath install_directory; - ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &install_directory)); - install_directory = install_directory.AppendASCII("extensions") + FilePath install_dir; + ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &install_dir)); + install_dir = install_dir.AppendASCII("extensions") .AppendASCII("good") .AppendASCII("Extensions"); - std::string id = "behllobkkfkfnphdnhnkndlbkcpglgmj"; - std::string version = "1.0.0.0"; - std::string version_out; + const std::string kId = "behllobkkfkfnphdnhnkndlbkcpglgmj"; + const std::string kCurrentVersion = "1.0.0.0"; + + FilePath version_dir; - ASSERT_EQ(Extension::UPGRADE, extension_file_util::CompareToInstalledVersion( - install_directory, id, "1.0.0.1", &version_out)); - ASSERT_EQ(version, version_out); + ASSERT_EQ(Extension::UPGRADE, + extension_file_util::CompareToInstalledVersion( + install_dir, kId, kCurrentVersion, "1.0.0.1", &version_dir)); - version_out.clear(); ASSERT_EQ(Extension::REINSTALL, extension_file_util::CompareToInstalledVersion( - install_directory, id, "1.0.0.0", &version_out)); - ASSERT_EQ(version, version_out); + install_dir, kId, kCurrentVersion, "1.0.0.0", &version_dir)); - version_out.clear(); ASSERT_EQ(Extension::REINSTALL, extension_file_util::CompareToInstalledVersion( - install_directory, id, "1.0.0", &version_out)); - ASSERT_EQ(version, version_out); + install_dir, kId, kCurrentVersion, "1.0.0", &version_dir)); - version_out.clear(); ASSERT_EQ(Extension::DOWNGRADE, extension_file_util::CompareToInstalledVersion( - install_directory, id, "0.0.1.0", &version_out)); - ASSERT_EQ(version, version_out); + install_dir, kId, kCurrentVersion, "0.0.1.0", &version_dir)); - // Compare to an extension that is missing its Current Version file. + // Compare to an extension that is missing its manifest file. ScopedTempDir temp; ASSERT_TRUE(temp.CreateUniqueTempDir()); - FilePath src = install_directory.AppendASCII(id).AppendASCII(version); - FilePath dest = temp.path().AppendASCII(id).AppendASCII(version); + FilePath src = install_dir.AppendASCII(kId).AppendASCII(kCurrentVersion); + FilePath dest = temp.path().AppendASCII(kId).AppendASCII(kCurrentVersion); ASSERT_TRUE(file_util::CreateDirectory(dest.DirName())); ASSERT_TRUE(file_util::CopyDirectory(src, dest, true)); + ASSERT_TRUE(file_util::Delete(dest.AppendASCII("manifest.json"), false)); - version_out.clear(); ASSERT_EQ(Extension::NEW_INSTALL, extension_file_util::CompareToInstalledVersion( - temp.path(), id, "0.0.1.0", &version_out)); - ASSERT_EQ("", version_out); + temp.path(), kId, kCurrentVersion, "1.0.0", &version_dir)); - // Compare to a completely non-existent extension. - version_out.clear(); + // Compare to a non-existent extension. + const std::string kMissingVersion = ""; + const std::string kBadId = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; ASSERT_EQ(Extension::NEW_INSTALL, extension_file_util::CompareToInstalledVersion( - temp.path(), "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", "0.0.1.0", - &version_out)); - ASSERT_EQ("", version_out); + temp.path(), kBadId, kMissingVersion, "1.0.0", &version_dir)); } // Creates minimal manifest, with or without default_locale section. diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc index 84ee99d..4583944 100644 --- a/chrome/browser/extensions/extension_prefs.cc +++ b/chrome/browser/extensions/extension_prefs.cc @@ -24,6 +24,12 @@ const wchar_t kPrefState[] = L"state"; // The path to the current version's manifest file. const wchar_t kPrefPath[] = L"path"; +// The dictionary containing the extension's manifest. +const wchar_t kPrefManifest[] = L"manifest"; + +// The version number. +const wchar_t kPrefVersion[] = L"manifest.version"; + // Indicates if an extension is blacklisted: const wchar_t kPrefBlacklist[] = L"blacklist"; @@ -43,6 +49,7 @@ InstalledExtensions::~InstalledExtensions() { void InstalledExtensions::VisitInstalledExtensions( InstalledExtensions::Callback *callback) { + scoped_ptr cleanup(callback); DictionaryValue::key_iterator extension_id = extension_data_->begin_keys(); for (; extension_id != extension_data_->end_keys(); ++extension_id) { DictionaryValue* ext; @@ -74,9 +81,16 @@ void InstalledExtensions::VisitInstalledExtensions( NOTREACHED(); continue; } + DictionaryValue* manifest = NULL; + if (!ext->GetDictionary(kPrefManifest, &manifest)) { + LOG(WARNING) << "Missing manifest for extension " << *extension_id; + // Just a warning for now. + } + Extension::Location location = static_cast(location_value); - callback->Run(WideToASCII(*extension_id), FilePath(path), location); + callback->Run(manifest, WideToASCII(*extension_id), FilePath(path), + location); } } @@ -300,13 +314,18 @@ void ExtensionPrefs::SetShelfToolstripOrder(const URLList& urls) { void ExtensionPrefs::OnExtensionInstalled(Extension* extension) { const std::string& id = extension->id(); - UpdateExtensionPref(id, kPrefState, - Value::CreateIntegerValue(Extension::ENABLED)); + // Make sure we don't enable a disabled extension. + if (GetExtensionState(extension->id()) != Extension::DISABLED) { + UpdateExtensionPref(id, kPrefState, + Value::CreateIntegerValue(Extension::ENABLED)); + } UpdateExtensionPref(id, kPrefLocation, Value::CreateIntegerValue(extension->location())); FilePath::StringType path = MakePathRelative(install_directory_, extension->path(), NULL); UpdateExtensionPref(id, kPrefPath, Value::CreateStringValue(path)); + UpdateExtensionPref(id, kPrefManifest, + extension->manifest_value()->DeepCopy()); prefs_->SavePersistentPrefs(); } @@ -351,6 +370,25 @@ void ExtensionPrefs::SetExtensionState(Extension* extension, prefs_->SavePersistentPrefs(); } +std::string ExtensionPrefs::GetVersionString(const std::string& extension_id) { + DictionaryValue* extension = GetExtensionPref(extension_id); + if (!extension) + return std::string(); + + std::string version; + if (!extension->GetString(kPrefVersion, &version)) { + LOG(ERROR) << "Bad or missing pref 'version' for extension '" + << extension_id << "'"; + } + + return version; +} + +void ExtensionPrefs::MigrateToPrefs(Extension* extension) { + UpdateExtensionPref(extension->id(), kPrefManifest, + extension->manifest_value()->DeepCopy()); +} + bool ExtensionPrefs::UpdateExtensionPref(const std::string& extension_id, const std::wstring& key, Value* data_value) { diff --git a/chrome/browser/extensions/extension_prefs.h b/chrome/browser/extensions/extension_prefs.h index 291556c..59adfd8 100644 --- a/chrome/browser/extensions/extension_prefs.h +++ b/chrome/browser/extensions/extension_prefs.h @@ -49,6 +49,13 @@ class ExtensionPrefs { // Called to change the extension's state when it is enabled/disabled. void SetExtensionState(Extension* extension, Extension::State); + // Returns the version string for the currently installed extension, or + // the empty string if not found. + std::string GetVersionString(const std::string& extension_id); + + // Ensure old extensions have fully up-to-date prefs values. + void MigrateToPrefs(Extension* extension); + // Returns base extensions install directory. const FilePath& install_directory() const { return install_directory_; } @@ -106,19 +113,16 @@ class InstalledExtensions { explicit InstalledExtensions(ExtensionPrefs* prefs); ~InstalledExtensions(); - typedef Callback3::Type Callback; // Runs |callback| for each installed extension with the path to the // version directory and the location. Blacklisted extensions won't trigger - // the callback. + // the callback. Ownership of |callback| is transferred to callee. void VisitInstalledExtensions(Callback *callback); - // Same as above, but only for the given extension. - void VisitInstalledExtension(const std::string& extension_id, - Callback *callback); - private: // A copy of the extensions pref dictionary so that this can be passed // around without a dependency on prefs. diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc index f571c08..6dcc015 100644 --- a/chrome/browser/extensions/extensions_service.cc +++ b/chrome/browser/extensions/extensions_service.cc @@ -28,6 +28,31 @@ #include "chrome/browser/extensions/external_registry_extension_provider_win.h" #endif +namespace { + +// Helper class to collect the IDs of every extension listed in the prefs. +class InstalledExtensionSet { + public: + InstalledExtensionSet(InstalledExtensions* installed) { + scoped_ptr cleanup(installed); + installed->VisitInstalledExtensions( + NewCallback(this, &InstalledExtensionSet::ExtensionVisited)); + } + + const std::set& extensions() { return extensions_; } + + private: + void ExtensionVisited( + DictionaryValue* manifest, const std::string& id, + const FilePath& path, Extension::Location location) { + extensions_.insert(id); + } + + std::set extensions_; +}; + +} // namespace + // ExtensionsService. const char* ExtensionsService::kInstallDirectoryName = "Extensions"; @@ -283,8 +308,11 @@ void ExtensionsService::ReloadExtensions() { } void ExtensionsService::GarbageCollectExtensions() { + InstalledExtensionSet installed( + new InstalledExtensions(extension_prefs_.get())); backend_loop_->PostTask(FROM_HERE, NewRunnableFunction( - &extension_file_util::GarbageCollectExtensions, install_directory_)); + &extension_file_util::GarbageCollectExtensions, install_directory_, + installed.extensions())); } void ExtensionsService::OnLoadedInstalledExtensions() { @@ -347,6 +375,8 @@ void ExtensionsService::OnExtensionsLoaded(ExtensionList* new_extensions) { switch (extension_prefs_->GetExtensionState(extension->id())) { case Extension::ENABLED: + if (extension->location() != Extension::LOAD) + extension_prefs_->MigrateToPrefs(extension.get()); enabled_extensions.push_back(extension.get()); extensions_.push_back(extension.release()); break; @@ -378,11 +408,7 @@ void ExtensionsService::OnExtensionsLoaded(ExtensionList* new_extensions) { } void ExtensionsService::OnExtensionInstalled(Extension* extension) { - // Make sure we don't enable a disabled extension. - if (extension_prefs_->GetExtensionState(extension->id()) != - Extension::DISABLED) { - extension_prefs_->OnExtensionInstalled(extension); - } + extension_prefs_->OnExtensionInstalled(extension); // If the extension is a theme, tell the profile (and therefore ThemeProvider) // to apply it. @@ -516,9 +542,8 @@ void ExtensionsServiceBackend::LoadInstalledExtensions( alert_on_error_ = false; // Call LoadInstalledExtension for each extension |installed| knows about. - scoped_ptr callback( + installed->VisitInstalledExtensions( NewCallback(this, &ExtensionsServiceBackend::LoadInstalledExtension)); - installed->VisitInstalledExtensions(callback.get()); frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod( frontend_, &ExtensionsService::OnLoadedInstalledExtensions)); @@ -555,7 +580,8 @@ void ExtensionsServiceBackend::LoadSingleExtension( } void ExtensionsServiceBackend::LoadInstalledExtension( - const std::string& id, const FilePath& path, Extension::Location location) { + DictionaryValue* manifest, const std::string& id, + const FilePath& path, Extension::Location location) { if (CheckExternalUninstall(id, location)) { frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod( frontend_, @@ -567,10 +593,20 @@ void ExtensionsServiceBackend::LoadInstalledExtension( } std::string error; - Extension* extension = extension_file_util::LoadExtension( - path, - true, // Require id - &error); + Extension* extension = NULL; + if (manifest) { + scoped_ptr tmp(new Extension(path)); + if (tmp->InitFromValue(*manifest, true, &error) && + extension_file_util::ValidateExtension(tmp.get(), &error)) { + extension = tmp.release(); + } + } else { + // TODO(mpcomplete): obsolete. remove after migration period. + // http://code.google.com/p/chromium/issues/detail?id=19733 + extension = extension_file_util::LoadExtension(path, + true, // Require id + &error); + } if (!extension) { ReportExtensionLoadError(path, error); diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h index fafe97e..6a56749 100644 --- a/chrome/browser/extensions/extensions_service.h +++ b/chrome/browser/extensions/extensions_service.h @@ -303,7 +303,9 @@ class ExtensionsServiceBackend Extension::Location location); private: // Loads a single installed extension. - void LoadInstalledExtension(const std::string& id, const FilePath& path, + void LoadInstalledExtension(DictionaryValue* manifest, + const std::string& id, + const FilePath& path, Extension::Location location); // Finish installing the extension in |crx_path| after it has been unpacked to diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc index 3f464d5..d19a3d3 100644 --- a/chrome/browser/extensions/extensions_service_unittest.cc +++ b/chrome/browser/extensions/extensions_service_unittest.cc @@ -636,25 +636,21 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) { // Test that partially deleted extensions are cleaned up during startup // Test loading bad extensions from the profile directory. TEST_F(ExtensionsServiceTest, CleanupOnStartup) { - InitializeEmptyExtensionsService(); - - FilePath source_path; - ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &source_path)); - source_path = source_path.AppendASCII("extensions") - .AppendASCII("good") - .AppendASCII("Extensions"); - - file_util::Delete(extensions_install_dir_, true); + FilePath source_install_dir; + ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &source_install_dir)); + source_install_dir = source_install_dir + .AppendASCII("extensions") + .AppendASCII("good") + .AppendASCII("Extensions"); + FilePath pref_path = source_install_dir + .DirName() + .AppendASCII("Preferences"); - // Recursive. - file_util::CopyDirectory(source_path, extensions_install_dir_, true); + InitializeInstalledExtensionsService(pref_path, source_install_dir); - // Simulate that one of them got partially deleted by deling the - // Current Version file. - FilePath vers = extensions_install_dir_ - .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj") - .AppendASCII(ExtensionsService::kCurrentVersionFileName); - ASSERT_TRUE(file_util::Delete(vers, false)); // not recursive + // Simulate that one of them got partially deleted by clearing its pref. + prefs_->GetMutableDictionary(L"extensions.settings")-> + Remove(L"behllobkkfkfnphdnhnkndlbkcpglgmj", NULL); service_->Init(); loop_.RunAllPending(); @@ -669,8 +665,9 @@ TEST_F(ExtensionsServiceTest, CleanupOnStartup) { EXPECT_EQ(2u, count); // And extension1 dir should now be toast. - vers = vers.DirName(); - ASSERT_FALSE(file_util::PathExists(vers)); + FilePath extension_dir = extensions_install_dir_ + .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj"); + ASSERT_FALSE(file_util::PathExists(extension_dir)); } // Test installing extensions. This test tries to install few extensions using @@ -1148,19 +1145,6 @@ TEST_F(ExtensionsServiceTest, UninstallExtension) { // The directory should be gone. EXPECT_FALSE(file_util::PathExists(extension_path)); - - // Try uinstalling one that doesn't have a Current Version file for some - // reason. - unloaded_id_.clear(); - InstallExtension(path, true); - FilePath current_version_file = - extension_path.AppendASCII(ExtensionsService::kCurrentVersionFileName); - EXPECT_TRUE(file_util::Delete(current_version_file, true)); - service_->UninstallExtension(extension_id, false); - loop_.RunAllPending(); - EXPECT_FALSE(file_util::PathExists(extension_path)); - - ValidatePrefKeyCount(0); } // Tests loading single extensions (like --load-extension) diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc index 5ceee67..1eb43e3 100644 --- a/chrome/common/extensions/extension.cc +++ b/chrome/common/extensions/extension.cc @@ -459,7 +459,7 @@ Extension::Extension(const FilePath& path) // util class in base: // http://code.google.com/p/chromium/issues/detail?id=13572 bool Extension::ParsePEMKeyBytes(const std::string& input, - std::string* output) { + std::string* output) { DCHECK(output); if (!output) return false; @@ -498,8 +498,8 @@ bool Extension::ProducePEM(const std::string& input, } bool Extension::FormatPEMForFileOutput(const std::string input, - std::string* output, - bool is_public) { + std::string* output, + bool is_public) { CHECK(output); if (input.length() == 0) return false; @@ -550,6 +550,9 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id, ConvertHexadecimalToIDAlphabet(&id_); } + // Make a copy of the manifest so we can store it in prefs. + manifest_value_.reset(static_cast(source.DeepCopy())); + // Initialize the URL. extension_url_ = GURL(std::string(chrome::kExtensionScheme) + chrome::kStandardSchemeSeparator + id_ + "/"); diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h index f3acd1c..34685fc 100644 --- a/chrome/common/extensions/extension.h +++ b/chrome/common/extensions/extension.h @@ -237,6 +237,10 @@ class Extension { // an empty FilePath if the extension does not have that icon. FilePath GetIconPath(Icons icon); + const DictionaryValue* manifest_value() const { + return manifest_value_.get(); + } + // Returns a list of all locales supported by the extension. const std::set& supported_locales() const { return supported_locales_; @@ -363,7 +367,10 @@ class Extension { // URL for fetching an update manifest GURL update_url_; - // List of all locales extension supports. + // A copy of the manifest that this extension was created from. + scoped_ptr manifest_value_; + + // List of all locales extension supports. std::set supported_locales_; // Default locale, used for fallback. diff --git a/chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpglgmj/Current Version b/chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpglgmj/Current Version deleted file mode 100644 index 1921233..0000000 --- a/chrome/test/data/extensions/good/Extensions/behllobkkfkfnphdnhnkndlbkcpglgmj/Current Version +++ /dev/null @@ -1 +0,0 @@ -1.0.0.0 diff --git a/chrome/test/data/extensions/good/Extensions/bjafgdebaacbbbecmhlhpofkepfkgcpa/Current Version b/chrome/test/data/extensions/good/Extensions/bjafgdebaacbbbecmhlhpofkepfkgcpa/Current Version deleted file mode 100644 index d3827e7..0000000 --- a/chrome/test/data/extensions/good/Extensions/bjafgdebaacbbbecmhlhpofkepfkgcpa/Current Version +++ /dev/null @@ -1 +0,0 @@ -1.0 diff --git a/chrome/test/data/extensions/good/Extensions/hpiknbiabeeppbpihjehijgoemciehgk/Current Version b/chrome/test/data/extensions/good/Extensions/hpiknbiabeeppbpihjehijgoemciehgk/Current Version deleted file mode 100644 index 0cfbf08..0000000 --- a/chrome/test/data/extensions/good/Extensions/hpiknbiabeeppbpihjehijgoemciehgk/Current Version +++ /dev/null @@ -1 +0,0 @@ -2 -- cgit v1.1