diff options
| -rw-r--r-- | chrome/browser/extensions/extension.cc | 1 | ||||
| -rw-r--r-- | chrome/browser/extensions/extension.h | 43 | ||||
| -rw-r--r-- | chrome/browser/extensions/extension_unittest.cc | 17 | ||||
| -rw-r--r-- | chrome/browser/extensions/extensions_service.cc | 82 | ||||
| -rw-r--r-- | chrome/browser/extensions/extensions_service.h | 41 | ||||
| -rw-r--r-- | chrome/browser/extensions/extensions_service_unittest.cc | 59 | ||||
| -rw-r--r-- | chrome/common/notification_type.h | 9 | ||||
| -rw-r--r-- | chrome/test/data/extensions/good/extension3/1.0/EXTERNAL_INSTALL | 0 |
8 files changed, 193 insertions, 59 deletions
diff --git a/chrome/browser/extensions/extension.cc b/chrome/browser/extensions/extension.cc index 993deff..da1e841 100644 --- a/chrome/browser/extensions/extension.cc +++ b/chrome/browser/extensions/extension.cc @@ -213,6 +213,7 @@ FilePath Extension::GetResourcePath(const FilePath& extension_path, Extension::Extension(const FilePath& path) { DCHECK(path.IsAbsolute()); + location_ = INVALID; #if defined(OS_WIN) // Normalize any drive letter to upper-case. We do this for consistency with diff --git a/chrome/browser/extensions/extension.h b/chrome/browser/extensions/extension.h index 02cb13c..8f5ec9f 100644 --- a/chrome/browser/extensions/extension.h +++ b/chrome/browser/extensions/extension.h @@ -23,10 +23,14 @@ // Represents a Chromium extension. class Extension { public: - Extension() {} - explicit Extension(const FilePath& path); - explicit Extension(const Extension& path); - virtual ~Extension(); + // What an extension was loaded from. + enum Location { + INVALID, + INTERNAL, // A crx file from the internal Extensions directory + EXTERNAL, // A crx file from an external directory (via eg the registry + // on Windows) + LOAD // --load-extension + }; // The name of the manifest inside an extension. static const char kManifestFilename[]; @@ -102,6 +106,11 @@ class Extension { // The number of bytes in a legal id. static const size_t kIdSize; + Extension() : location_(INVALID) {} + explicit Extension(const FilePath& path); + explicit Extension(const Extension& other); + virtual ~Extension(); + // Returns an absolute url to a resource inside of an extension. The // |extension_url| argument should be the url() from an Extension object. The // |relative_path| can be untrusted user input. The returned URL will either @@ -124,22 +133,16 @@ class Extension { return GetResourcePath(path(), relative_path); } - DictionaryValue* GetThemeImages() const { return theme_images_.get(); } - DictionaryValue* GetThemeColors() const { return theme_colors_.get(); } - DictionaryValue* GetThemeTints() const { return theme_tints_.get(); } - bool IsTheme() { return is_theme_; } - // Initialize the extension from a parsed manifest. // If |require_id| is true, will return an error if the "id" key is missing // from the value. bool InitFromValue(const DictionaryValue& value, bool require_id, std::string* error); - // Retrieves a page action by |id|. - const PageAction* GetPageAction(std::string id) const; - const FilePath& path() const { return path_; } const GURL& url() const { return extension_url_; } + const Location location() const { return location_; } + void set_location(Location location) { location_ = location; } const std::string& id() const { return id_; } const Version* version() const { return version_.get(); } // String representation of the version number. @@ -154,6 +157,19 @@ class Extension { const std::vector<URLPattern>& permissions() const { return permissions_; } + // Retrieves a page action by |id|. + const PageAction* GetPageAction(std::string id) const; + + // Theme-related + DictionaryValue* GetThemeImages() const { return theme_images_.get(); } + DictionaryValue* GetThemeColors() const { return theme_colors_.get(); } + DictionaryValue* GetThemeTints() const { return theme_tints_.get(); } + bool IsTheme() { return is_theme_; } + + // It doesn't really make sense to 'uninstall' extensions loaded from + // --load-extension or external locations. + const bool is_uninstallable() const { return location_ == INTERNAL; } + private: // Helper method that loads a UserScript object from a // dictionary in the content_script list of the manifest. @@ -174,6 +190,9 @@ class Extension { // The base extension url for the extension. GURL extension_url_; + // The location the extension was loaded from. + Location location_; + // A human-readable ID for the extension. The convention is to use something // like 'com.example.myextension', but this is not currently enforced. An // extension's ID is used in things like directory structures and URLs, and diff --git a/chrome/browser/extensions/extension_unittest.cc b/chrome/browser/extensions/extension_unittest.cc index 0607186..df4c64f 100644 --- a/chrome/browser/extensions/extension_unittest.cc +++ b/chrome/browser/extensions/extension_unittest.cc @@ -254,3 +254,20 @@ TEST(ExtensionTest, GetResourceURLAndPath) { EXPECT_EQ(FilePath().value(), Extension::GetResourcePath(extension.path(), "../baz.js").value()); } + +TEST(ExtensionTest, Location) { + Extension extension; + EXPECT_EQ(Extension::INVALID, extension.location()); + + extension.set_location(Extension::INTERNAL); + EXPECT_EQ(Extension::INTERNAL, extension.location()); + EXPECT_TRUE(extension.is_uninstallable()); + + extension.set_location(Extension::EXTERNAL); + EXPECT_EQ(Extension::EXTERNAL, extension.location()); + EXPECT_FALSE(extension.is_uninstallable()); + + extension.set_location(Extension::LOAD); + EXPECT_EQ(Extension::LOAD, extension.location()); + EXPECT_FALSE(extension.is_uninstallable()); +} diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc index 2d1807e..05815a3 100644 --- a/chrome/browser/extensions/extensions_service.cc +++ b/chrome/browser/extensions/extensions_service.cc @@ -128,10 +128,39 @@ void ExtensionsService::InstallExtension(const FilePath& extension_path) { scoped_refptr<ExtensionsServiceFrontendInterface>(this))); } +void ExtensionsService::UninstallExtension(const std::string& extension_id) { + Extension* extension = NULL; + + ExtensionList::iterator iter; + for (iter = extensions_.begin(); iter != extensions_.end(); ++iter) { + if ((*iter)->id() == extension_id) { + extension = *iter; + break; + } + } + + // Remove the extension from our list. + extensions_.erase(iter); + + // Callers should check existence and that the extension is internal before + // calling us. + DCHECK(extension); + DCHECK(extension->is_uninstallable()); + + // Tell other services the extension is gone. + NotificationService::current()->Notify(NotificationType::EXTENSION_UNLOADED, + NotificationService::AllSources(), + Details<Extension>(extension)); + + // Tell the file thread to start deleting. + g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE, + NewRunnableMethod(backend_.get(), + &ExtensionsServiceBackend::UninstallExtension, extension_id)); + + delete extension; +} + void ExtensionsService::LoadExtension(const FilePath& extension_path) { - // TODO(aa): This message loop should probably come from a backend - // interface, similar to how the message loop for the frontend comes - // from the frontend interface. g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE, NewRunnableMethod(backend_.get(), &ExtensionsServiceBackend::LoadSingleExtension, @@ -261,7 +290,7 @@ void ExtensionsServiceBackend::LoadExtensionsFromInstallDirectory( if (CheckExternalUninstall(extension_path, extension_id)) { // TODO(erikkay): Possibly defer this operation to avoid slowing initial // load of extensions. - UninstallExtension(extension_path); + UninstallExtension(extension_id); // No error needs to be reported. The extension effectively doesn't // exist. @@ -295,6 +324,7 @@ void ExtensionsServiceBackend::LoadSingleExtension( Extension* extension = LoadExtension(extension_path, false); // don't require ID if (extension) { + extension->set_location(Extension::LOAD); ExtensionList* extensions = new ExtensionList; extensions->push_back(extension); ReportExtensionsLoaded(extensions); @@ -348,6 +378,12 @@ Extension* ExtensionsServiceBackend::LoadExtension( return NULL; } + FilePath external_marker = extension_path.AppendASCII(kExternalInstallFile); + if (file_util::PathExists(external_marker)) + extension->set_location(Extension::EXTERNAL); + else + extension->set_location(Extension::INTERNAL); + // TODO(glen): Add theme resource validation here. http://crbug.com/11678 // Validate that claimed script resources actually exist. for (size_t i = 0; i < extension->content_scripts().size(); ++i) { @@ -795,6 +831,8 @@ void ExtensionsServiceBackend::CheckForExternalUpdates( frontend_ = frontend; #if defined(OS_WIN) + // TODO(port): Pull this out into an interface. That will also allow us to + // test the behavior of external extensions. HKEY reg_root = HKEY_LOCAL_MACHINE; RegistryKeyIterator iterator(reg_root, kRegistryExtensions); while (iterator.Valid()) { @@ -861,18 +899,34 @@ bool ExtensionsServiceBackend::CheckExternalUninstall(const FilePath& path, } // Assumes that the extension isn't currently loaded or in use. -void ExtensionsServiceBackend::UninstallExtension(const FilePath& path) { - FilePath parent = path.DirName(); - FilePath version = - parent.AppendASCII(ExtensionsService::kCurrentVersionFileName); - bool version_exists = file_util::PathExists(version); - DCHECK(version_exists); - if (!version_exists) { - LOG(WARNING) << "Asked to uninstall bogus extension dir " << parent.value(); +void ExtensionsServiceBackend::UninstallExtension( + const std::string& extension_id) { + // First, delete the Current Version file. If the directory delete fails, then + // at least the extension won't be loaded again. + FilePath extension_directory = install_directory_.AppendASCII(extension_id); + + if (!file_util::PathExists(extension_directory)) { + LOG(WARNING) << "Asked to remove a non-existent extension " << extension_id; return; } - if (!file_util::Delete(parent, true)) { - LOG(WARNING) << "Failed to delete " << parent.value(); + + FilePath current_version_file = extension_directory.AppendASCII( + ExtensionsService::kCurrentVersionFileName); + if (!file_util::PathExists(current_version_file)) { + LOG(WARNING) << "Extension " << extension_id + << " does not have a Current Version file."; + } else { + if (!file_util::Delete(current_version_file, false)) { + LOG(WARNING) << "Could not delete Current Version file for extension " + << extension_id; + return; + } + } + + // Ok, now try and delete the entire rest of the directory. + if (!file_util::Delete(extension_directory, true)) { + LOG(WARNING) << "Could not delete directory for extension " + << extension_id; } } diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h index 31b8f90e..481cdd2 100644 --- a/chrome/browser/extensions/extensions_service.h +++ b/chrome/browser/extensions/extensions_service.h @@ -33,15 +33,6 @@ class ExtensionsServiceFrontendInterface // The message loop to invoke the frontend's methods on. virtual MessageLoop* GetMessageLoop() = 0; - // Install the extension file at |extension_path|. Will install as an - // update if an older version is already installed. - // For fresh installs, this method also causes the extension to be - // immediately loaded. - virtual void InstallExtension(const FilePath& extension_path) = 0; - - // Load the extension from the directory |extension_path|. - virtual void LoadExtension(const FilePath& extension_path) = 0; - // Called when extensions are loaded by the backend. The frontend takes // ownership of the list. virtual void OnExtensionsLoaded(ExtensionList* extensions) = 0; @@ -56,10 +47,6 @@ class ExtensionsServiceFrontendInterface // action using the fact that the user chose to reinstall the extension as a // signal (for example, setting the default theme to the extension). virtual void OnExtensionVersionReinstalled(const std::string& id) = 0; - - // Lookup an extension by |id|. - virtual Extension* GetExtensionByID(std::string id) = 0; - }; @@ -77,13 +64,26 @@ class ExtensionsService : public ExtensionsServiceFrontendInterface { // Initialize and start all installed extensions. bool Init(); + // Install the extension file at |extension_path|. Will install as an + // update if an older version is already installed. + // For fresh installs, this method also causes the extension to be + // immediately loaded. + void InstallExtension(const FilePath& extension_path); + + // Uninstalls the specified extension. Callers should only call this method + // with extensions that exist and are "internal". + void UninstallExtension(const std::string& extension_id); + + // Load the extension from the directory |extension_path|. + void LoadExtension(const FilePath& extension_path); + + // Lookup an extension by |id|. + virtual Extension* GetExtensionByID(std::string id); + // ExtensionsServiceFrontendInterface virtual MessageLoop* GetMessageLoop(); - virtual void InstallExtension(const FilePath& extension_path); - virtual void LoadExtension(const FilePath& extension_path); virtual void OnExtensionsLoaded(ExtensionList* extensions); virtual void OnExtensionInstalled(Extension* extension, bool is_update); - virtual Extension* GetExtensionByID(std::string id); virtual void OnExtensionVersionReinstalled(const std::string& id); // The name of the file that the current active version number is stored in. @@ -151,6 +151,11 @@ class ExtensionsServiceBackend void CheckForExternalUpdates( scoped_refptr<ExtensionsServiceFrontendInterface> frontend); + // Deletes all versions of the extension from the filesystem. Note that only + // extensions whose location() == INTERNAL can be uninstalled. Attempting to + // uninstall other extensions will silently fail. + void UninstallExtension(const std::string& extension_id); + private: // Load a single extension from |extension_path|, the top directory of // a specific extension where its manifest file lives. @@ -217,10 +222,6 @@ class ExtensionsServiceBackend // uninstalled. bool CheckExternalUninstall(const FilePath& path, const std::string& id); - // Deletes all versions of the extension from the filesystem. - // |path| points at a specific extension version dir. - void UninstallExtension(const FilePath& path); - // Should an extension of |id| and |version| be installed? // Returns true if no extension of type |id| is installed or if |version| // is greater than the current installed version. diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc index dc9be78..bc1a3e3 100644 --- a/chrome/browser/extensions/extensions_service_unittest.cc +++ b/chrome/browser/extensions/extensions_service_unittest.cc @@ -84,12 +84,6 @@ class ExtensionsServiceTestFrontend return &message_loop_; } - virtual void InstallExtension(const FilePath& extension_path) { - } - - virtual void LoadExtension(const FilePath& extension_path) { - } - virtual void OnExtensionsLoaded(ExtensionList* new_extensions) { extensions_.insert(extensions_.end(), new_extensions->begin(), new_extensions->end()); @@ -107,10 +101,6 @@ class ExtensionsServiceTestFrontend reinstalled_id_ = id; } - virtual Extension* GetExtensionByID(std::string id) { - return NULL; - } - void TestInstallExtension(const FilePath& path, ExtensionsServiceBackend* backend, bool should_succeed) { @@ -184,6 +174,7 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) { frontend->extensions()->at(0)->name()); EXPECT_EQ(std::string("The first extension that I made."), frontend->extensions()->at(0)->description()); + EXPECT_EQ(Extension::INTERNAL, frontend->extensions()->at(0)->location()); Extension* extension = frontend->extensions()->at(0); const UserScriptList& scripts = extension->content_scripts(); @@ -222,7 +213,8 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) { frontend->extensions()->at(1)->plugins_dir().value()); EXPECT_EQ(frontend->extensions()->at(1)->GetResourceURL("background.html"), frontend->extensions()->at(1)->background_url()); - ASSERT_EQ(0u, frontend->extensions()->at(1)->content_scripts().size()); + EXPECT_EQ(0u, frontend->extensions()->at(1)->content_scripts().size()); + EXPECT_EQ(Extension::INTERNAL, frontend->extensions()->at(1)->location()); EXPECT_EQ(std::string("20123456789abcdef0123456789abcdef0123456"), frontend->extensions()->at(2)->id()); @@ -230,7 +222,8 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) { frontend->extensions()->at(2)->name()); EXPECT_EQ(std::string(""), frontend->extensions()->at(2)->description()); - ASSERT_EQ(0u, frontend->extensions()->at(2)->content_scripts().size()); + EXPECT_EQ(0u, frontend->extensions()->at(2)->content_scripts().size()); + EXPECT_EQ(Extension::EXTERNAL, frontend->extensions()->at(2)->location()); }; // Test loading bad extensions from the profile directory. @@ -310,6 +303,47 @@ TEST_F(ExtensionsServiceTest, InstallExtension) { // TODO(erikkay): add tests for upgrade cases. } +// Tests uninstalling extensions +TEST_F(ExtensionsServiceTest, UninstallExtension) { + FilePath extensions_path; + ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path)); + extensions_path = extensions_path.AppendASCII("extensions"); + + FilePath install_path; + file_util::CreateNewTempDirectory(FILE_PATH_LITERAL("ext_test"), + &install_path); + scoped_refptr<ExtensionsServiceBackend> backend( + new ExtensionsServiceBackend(install_path)); + scoped_refptr<ExtensionsServiceTestFrontend> frontend( + new ExtensionsServiceTestFrontend); + + FilePath path = extensions_path.AppendASCII("good.crx"); + + // A simple extension that should install without error. + frontend->TestInstallExtension(path, backend, true); + + // The directory should be there now. + const char* extension_id = "00123456789abcdef0123456789abcdef0123456"; + FilePath extension_path = install_path.AppendASCII(extension_id); + EXPECT_TRUE(file_util::PathExists(extension_path)); + + // Uninstall it, directory should be gone. + backend->UninstallExtension(extension_id); + EXPECT_FALSE(file_util::PathExists(extension_path)); + + // Try uinstalling one that doesn't have a Current Version file for some + // reason. + frontend->TestInstallExtension(path, backend, true); + FilePath current_version_file = + extension_path.AppendASCII(ExtensionsService::kCurrentVersionFileName); + EXPECT_TRUE(file_util::Delete(current_version_file, true)); + backend->UninstallExtension(extension_id); + EXPECT_FALSE(file_util::PathExists(extension_path)); + + // Try uninstalling one that doesn't even exist. We shouldn't crash. + backend->UninstallExtension(extension_id); +} + TEST_F(ExtensionsServiceTest, ReinstallExtension) { // In this test, we install two extensions, verify that they both install // correctly, then install the first extension again and verify that it was @@ -394,6 +428,7 @@ TEST_F(ExtensionsServiceTest, LoadExtension) { frontend->GetMessageLoop()->RunAllPending(); EXPECT_EQ(1u, GetErrors().size()); ASSERT_EQ(1u, frontend->extensions()->size()); + ASSERT_EQ(Extension::LOAD, frontend->extensions()->at(0)->location()); } TEST_F(ExtensionsServiceTest, GenerateID) { diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h index 98f0259..340f5d1 100644 --- a/chrome/common/notification_type.h +++ b/chrome/common/notification_type.h @@ -550,12 +550,19 @@ class NotificationType { // Sent when new extensions are loaded. The details are an ExtensionList*. EXTENSIONS_LOADED, - // Sent when a new theme is installed. The details are an extension. + // Sent when a new theme is installed. The details are an Extension. THEME_INSTALLED, // Sent when new extensions are installed. The details are a FilePath. EXTENSION_INSTALLED, + // Sent when an extension is unloaded. This happens when an extension is + // uninstalled. When we add a disable feature, it will also happen then. + // The details are an Extension. Note that when this notification is sent, + // ExtensionsService has already removed the extension from its internal + // state. + EXTENSION_UNLOADED, + // Debugging --------------------------------------------------------------- // Sent from ~RenderViewHost. The source is the RenderViewHost. diff --git a/chrome/test/data/extensions/good/extension3/1.0/EXTERNAL_INSTALL b/chrome/test/data/extensions/good/extension3/1.0/EXTERNAL_INSTALL new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/chrome/test/data/extensions/good/extension3/1.0/EXTERNAL_INSTALL |