Cleaning Up Extensions When Local Content Removed

This fixes an issue where removing an extension's local content, e.g. deleting
the user-data-dir/Default/Extensions/<id> directory, would result in a broken
extension. Chrome will now uninstall/remove any extensions where the path to
the extension cannot be resolved.

BUG=31910
TEST=ExtensionServiceTest.CleanupInternalExtensionsMissingLocalContent;
ExtensionServiceTest.CleanupUnpackedExtensionsMissingLocalContent;
test by hand by loading an extension, deleting the local directory, and
restarting chrome


Review URL: https://chromiumcodereview.appspot.com/10541126

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@142427 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 385 insertions, 25 deletions

diff --git a/chrome/browser/extensions/extension_garbage_collector.cc b/chrome/browser/extensions/extension_garbage_collector.cc
new file mode 100644
index 0000000..933f25c
--- /dev/null
+++ b/chrome/browser/extensions/extension_garbage_collector.cc
@@ -0,0 +1,192 @@
+// Copyright (c) 2012 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 "chrome/browser/extensions/extension_garbage_collector.h"
+
+#include <map>
+#include <string>
+
+#include "base/bind.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/common/extensions/extension_file_util.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace extensions {
+
+ExtensionGarbageCollector::ExtensionGarbageCollector(
+    ExtensionService* extension_service)
+    : extension_service_(extension_service) {
+}
+
+ExtensionGarbageCollector::~ExtensionGarbageCollector() {
+}
+
+void ExtensionGarbageCollector::GarbageCollectExtensions() {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  FilePath install_directory = extension_service_->install_directory();
+
+  // NOTE: It's important to use the file thread here because when the
+  // ExtensionService installs an extension, it begins on the file thread, and
+  // there are ordering dependencies between all of the extension-file-
+  // management tasks. If we do not follow the same thread pattern as the
+  // ExtensionService, then a race condition develops.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(
+           &ExtensionGarbageCollector::
+               CheckExtensionDirectoriesOnBackgroundThread,
+           base::Unretained(this),
+           install_directory));
+}
+
+void ExtensionGarbageCollector::CheckExtensionDirectoriesOnBackgroundThread(
+    const FilePath& install_directory) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+
+  DVLOG(1) << "Garbage collecting extensions...";
+
+  if (!file_util::DirectoryExists(install_directory))
+    return;
+
+  std::set<FilePath> extension_paths;
+
+  file_util::FileEnumerator enumerator(install_directory,
+                                       false,  // Not recursive.
+                                       file_util::FileEnumerator::DIRECTORIES);
+
+  for (FilePath extension_path = enumerator.Next(); !extension_path.empty();
+       extension_path = enumerator.Next()) {
+    std::string extension_id = extension_path.BaseName().MaybeAsASCII();
+    if (!Extension::IdIsValid(extension_id))
+      extension_id.clear();
+
+    // Delete directories that aren't valid IDs.
+    if (extension_id.empty()) {
+      DVLOG(1) << "Deleting invalid extension directory "
+               << extension_path.value() << ".";
+      file_util::Delete(extension_path, true);  // Recursive.
+      continue;
+    }
+
+    extension_paths.insert(extension_path);
+  }
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(
+           &ExtensionGarbageCollector::CheckExtensionPreferences,
+           base::Unretained(this),
+           extension_paths));
+}
+
+void ExtensionGarbageCollector::CheckExtensionPreferences(
+    const std::set<FilePath>& extension_paths) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  ExtensionPrefs* extension_prefs = extension_service_->extension_prefs();
+
+  if (extension_prefs->pref_service()->ReadOnly())
+    return;
+
+  scoped_ptr<ExtensionPrefs::ExtensionsInfo> info(
+      extension_prefs->GetInstalledExtensionsInfo());
+
+  std::map<std::string, FilePath> extension_pref_paths;
+  for (size_t i = 0; i < info->size(); ++i) {
+    extension_pref_paths[info->at(i)->extension_id] =
+        info->at(i)->extension_path;
+  }
+
+  std::set<std::string> extension_ids;
+
+  // Check each directory for a reference in the preferences.
+  for (std::set<FilePath>::const_iterator path = extension_paths.begin();
+       path != extension_paths.end();
+       ++path) {
+    std::string extension_id;
+    extension_id = path->BaseName().MaybeAsASCII();
+    if (extension_id.empty())
+      continue;
+
+    extension_ids.insert(extension_id);
+
+    std::map<std::string, FilePath>::const_iterator iter =
+        extension_pref_paths.find(extension_id);
+
+    if (iter == extension_pref_paths.end()) {
+      DVLOG(1) << "Deleting unreferenced install for directory "
+               << path->LossyDisplayName() << ".";
+      content::BrowserThread::PostTask(
+          content::BrowserThread::FILE, FROM_HERE,
+          base::Bind(
+              &extension_file_util::DeleteFile,
+              *path,
+              true));  // recursive.
+      continue;
+    }
+
+    // Clean up old version directories.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(
+             &ExtensionGarbageCollector::CleanupOldVersionsOnBackgroundThread,
+             base::Unretained(this), *path, iter->second.BaseName()));
+  }
+
+  // Check each entry in the preferences for an existing path.
+  for (std::map<std::string, FilePath>::const_iterator extension =
+           extension_pref_paths.begin();
+       extension != extension_pref_paths.end();
+       ++extension) {
+    std::set<std::string>::const_iterator iter = extension_ids.find(
+        extension->first);
+
+    if (iter != extension_ids.end())
+      continue;
+
+    std::string extension_id = extension->first;
+
+    DVLOG(1) << "Could not access local content for extension with id "
+             << extension_id << "; uninstalling extension.";
+
+    // If the extension failed to load fully (e.g. the user deleted an unpacked
+    // extension's manifest or the manifest points to the wrong path), we cannot
+    // use UninstallExtension, which relies on a valid Extension object.
+    if (!extension_service_->GetInstalledExtension(extension_id)) {
+      scoped_ptr<ExtensionInfo> info(extension_service_->extension_prefs()
+          ->GetInstalledExtensionInfo(extension_id));
+      extension_service_->extension_prefs()->OnExtensionUninstalled(
+          extension_id, info->extension_location, false);
+    } else {
+      extension_service_->UninstallExtension(extension_id, false, NULL);
+    }
+  }
+}
+
+void ExtensionGarbageCollector::CleanupOldVersionsOnBackgroundThread(
+    const FilePath& path,
+    const FilePath& current_version) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  file_util::FileEnumerator versions_enumerator(
+      path,
+      false,  // Not recursive.
+      file_util::FileEnumerator::DIRECTORIES);
+  for (FilePath version_dir = versions_enumerator.Next();
+       !version_dir.value().empty();
+       version_dir = versions_enumerator.Next()) {
+    if (version_dir.BaseName() != current_version) {
+      DVLOG(1) << "Deleting old version for directory "
+               << version_dir.LossyDisplayName() << ".";
+      if (!file_util::Delete(version_dir, true))  // Recursive.
+        NOTREACHED();
+    }
+  }
+}
+
+}  // namespace extensions
diff --git a/chrome/browser/extensions/extension_garbage_collector.h b/chrome/browser/extensions/extension_garbage_collector.h
new file mode 100644
index 0000000..bf75b1a
--- /dev/null
+++ b/chrome/browser/extensions/extension_garbage_collector.h
@@ -0,0 +1,53 @@
+// Copyright (c) 2012 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_GARBAGE_COLLECTOR_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_GARBAGE_COLLECTOR_H_
+#pragma once
+
+#include <set>
+
+#include "base/file_path.h"
+
+class ExtensionService;
+
+// Garbage collection for extensions. This will delete any directories in the
+// installation directory of |extension_service_| that aren't valid, as well
+// as removing any references in the preferences to extensions that are
+// missing local content.
+namespace extensions {
+
+class ExtensionGarbageCollector {
+ public:
+  explicit ExtensionGarbageCollector(ExtensionService* extension_service);
+  virtual ~ExtensionGarbageCollector();
+
+  // Begin garbage collection; fetch the installed extensions' ids.
+  void GarbageCollectExtensions();
+
+ private:
+  // Check the installation directory for directories with invalid extension
+  // ids, deleting them if found. Call CheckExtensionPreferences with a list of
+  // present directories.
+  void CheckExtensionDirectoriesOnBackgroundThread(
+      const FilePath& installation_directory);
+
+  // Check for any extension directories which:
+  //   - Are an old version of a current extension;
+  //   - Are present in the preferences but not on the file system;
+  //   - Are present on the file system, but not in the preferences
+  // and delete them if found.
+  void CheckExtensionPreferences(const std::set<FilePath>& extension_paths);
+
+  // Helper function to cleanup any old versions of an extension in the
+  // extension's base directory.
+  void CleanupOldVersionsOnBackgroundThread(
+      const FilePath& path, const FilePath& current_version);
+
+  ExtensionService* extension_service_;
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_GARBAGE_COLLECTOR_H_
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 62a32a0..61aedb6 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -119,7 +119,6 @@
 #include "chrome/browser/chromeos/extensions/input_method_event_router.h"
 #include "chrome/browser/chromeos/extensions/media_player_event_router.h"
 #include "chrome/browser/chromeos/input_method/input_method_manager.h"
-#include "chrome/browser/extensions/extension_input_ime_api.h"
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_mount_point_provider.h"
 #endif
@@ -325,6 +324,8 @@ ExtensionService::ExtensionService(Profile* profile,
     : profile_(profile),
       system_(ExtensionSystem::Get(profile)),
       extension_prefs_(extension_prefs),
+      extension_garbage_collector_(
+          new extensions::ExtensionGarbageCollector(this)),
       settings_frontend_(extensions::SettingsFrontend::Create(profile)),
       pending_extension_manager_(*ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       install_directory_(install_directory),
@@ -1803,7 +1804,7 @@ void ExtensionService::UnloadExtension(
   // Clean up runtime data.
   extension_runtime_data_.erase(extension_id);
 
-if (disabled_extensions_.Contains(extension->id())) {
+  if (disabled_extensions_.Contains(extension->id())) {
     UnloadedExtensionInfo details(extension, reason);
     details.already_disabled = true;
     disabled_extensions_.Remove(extension->id());
@@ -1845,23 +1846,7 @@ void ExtensionService::ReloadExtensions() {
 }
 
 void ExtensionService::GarbageCollectExtensions() {
-  if (extension_prefs_->pref_service()->ReadOnly())
-    return;
-
-  scoped_ptr<ExtensionPrefs::ExtensionsInfo> info(
-      extension_prefs_->GetInstalledExtensionsInfo());
-
-  std::map<std::string, FilePath> extension_paths;
-  for (size_t i = 0; i < info->size(); ++i)
-    extension_paths[info->at(i)->extension_id] = info->at(i)->extension_path;
-
-  if (!BrowserThread::PostTask(
-          BrowserThread::FILE, FROM_HERE,
-          base::Bind(
-              &extension_file_util::GarbageCollectExtensions,
-              install_directory_,
-              extension_paths)))
-    NOTREACHED();
+  extension_garbage_collector_->GarbageCollectExtensions();
 
   // Also garbage-collect themes.  We check |profile_| to be
   // defensive; in the future, we may call GarbageCollectExtensions()
@@ -2027,7 +2012,7 @@ void ExtensionService::InitializePermissions(const Extension* extension) {
     // Other than for unpacked extensions, CrxInstaller should have guaranteed
     // that we aren't downgrading.
     if (extension->location() != Extension::LOAD)
-      CHECK(extension->version()->CompareTo(*(old->version())) >= 0);
+      CHECK_GE(extension->version()->CompareTo(*(old->version())), 0);
 
     // Extensions get upgraded if the privileges are allowed to increase or
     // the privileges haven't increased.
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index b79ed33..f46ed87 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -27,6 +27,7 @@
 #include "chrome/browser/extensions/app_shortcut_manager.h"
 #include "chrome/browser/extensions/app_sync_bundle.h"
 #include "chrome/browser/extensions/apps_promo.h"
+#include "chrome/browser/extensions/extension_garbage_collector.h"
 #include "chrome/browser/extensions/extension_icon_manager.h"
 #include "chrome/browser/extensions/extension_menu_manager.h"
 #include "chrome/browser/extensions/extension_prefs.h"
@@ -711,6 +712,11 @@ class ExtensionService
   // Preferences for the owning profile (weak reference).
   ExtensionPrefs* extension_prefs_;
 
+  // The ExtensionGarbageCollector associated with this service; this is
+  // responsible for cleaning up old or partially deleted extensions.
+  scoped_ptr<extensions::ExtensionGarbageCollector>
+      extension_garbage_collector_;
+
   // Settings for the owning profile.
   scoped_ptr<extensions::SettingsFrontend> settings_frontend_;
 
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index c23230a..c657c10 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -235,7 +235,6 @@ class MockExtensionProvider : public ExternalExtensionProviderInterface {
 class MockProviderVisitor
     : public ExternalExtensionProviderInterface::VisitorInterface {
  public:
-
   // The provider will return |fake_base_path| from
   // GetBaseCrxFilePath().  User can test the behavior with
   // and without an empty path using this parameter.
@@ -1007,7 +1006,6 @@ void PackExtensionTestClient::OnPackFailure(const std::string& error_message,
      FAIL() << "Packing should not fail.";
   else
      FAIL() << "Existing CRX should have been overwritten.";
-
 }
 
 // Test loading good extensions from the profile directory.
@@ -1151,9 +1149,46 @@ TEST_F(ExtensionServiceTest, LoadAllExtensionsFromDirectoryFail) {
       UTF16ToUTF8(GetErrors()[3]);
 };
 
-// Test that partially deleted extensions are cleaned up during startup
+// Test that old versions of extensions are deleted during garbage collection.
+TEST_F(ExtensionServiceTest, GarbageCollectOldVersions) {
+  PluginService::GetInstance()->Init();
+
+  FilePath source_install_dir = data_dir_
+      .AppendASCII("garbage_collection")
+      .AppendASCII("Extensions");
+
+  FilePath pref_path = source_install_dir
+      .DirName()
+      .AppendASCII("Preferences");
+
+  InitializeInstalledExtensionService(pref_path, source_install_dir);
+
+  // Verify that there are two versions present initially.
+  ASSERT_TRUE(file_util::PathExists(
+      extensions_install_dir_.AppendASCII("fdoajpacpdeapbmhbblepcnilfkpmkff")
+                             .AppendASCII("1.0_0")));
+  ASSERT_TRUE(file_util::PathExists(
+      extensions_install_dir_.AppendASCII("fdoajpacpdeapbmhbblepcnilfkpmkff")
+                             .AppendASCII("1.1_0")));
+
+  ValidatePrefKeyCount(1u);
+
+  service_->Init();
+  loop_.RunAllPending();
+
+  // Garbage collection should have deleted the old version.
+  ASSERT_FALSE(file_util::PathExists(
+      extensions_install_dir_.AppendASCII("fdoajpacpdeapbmhbblepcnilfkpmkff")
+                             .AppendASCII("1.0_0")));
+  ASSERT_TRUE(file_util::PathExists(
+      extensions_install_dir_.AppendASCII("fdoajpacpdeapbmhbblepcnilfkpmkff")
+                             .AppendASCII("1.1_0")));
+}
+
+// Test that extensions which were deleted from the preferences are cleaned up
+// during startup.
 // Test loading bad extensions from the profile directory.
-TEST_F(ExtensionServiceTest, CleanupOnStartup) {
+TEST_F(ExtensionServiceTest, GarbageCollectOnStartup) {
   PluginService::GetInstance()->Init();
 
   FilePath source_install_dir = data_dir_
@@ -1165,11 +1200,13 @@ TEST_F(ExtensionServiceTest, CleanupOnStartup) {
 
   InitializeInstalledExtensionService(pref_path, source_install_dir);
 
+  ValidatePrefKeyCount(3u);
+
   // Simulate that one of them got partially deleted by clearing its pref.
   {
     DictionaryPrefUpdate update(profile_->GetPrefs(), "extensions.settings");
     DictionaryValue* dict = update.Get();
-    ASSERT_TRUE(dict != NULL);
+    ASSERT_TRUE(dict);
     dict->Remove("behllobkkfkfnphdnhnkndlbkcpglgmj", NULL);
   }
 
@@ -1192,6 +1229,93 @@ TEST_F(ExtensionServiceTest, CleanupOnStartup) {
   ASSERT_FALSE(file_util::PathExists(extension_dir));
 }
 
+// Test that internal extensions which are referenced in the preferences but
+// had their content deleted are cleaned up during startup.
+TEST_F(ExtensionServiceTest, GarbageCollectInternalExtensionsMissingContent) {
+  PluginService::GetInstance()->Init();
+
+  FilePath source_dir = data_dir_.AppendASCII("good").AppendASCII("Extensions");
+  FilePath pref_path = source_dir.DirName().AppendASCII("Preferences");
+
+  InitializeInstalledExtensionService(pref_path, source_dir);
+
+  ValidatePrefKeyCount(3u);
+
+  // Delete the extension's directory.
+  FilePath extension_dir = extensions_install_dir_
+      .AppendASCII("behllobkkfkfnphdnhnkndlbkcpglgmj");
+  ASSERT_TRUE(file_util::Delete(extension_dir, true));
+
+  // Run GarbageCollectExtensions.
+  service_->Init();
+  loop_.RunAllPending();
+
+  file_util::FileEnumerator dirs(extensions_install_dir_, false,
+                                 file_util::FileEnumerator::DIRECTORIES);
+  size_t count = 0;
+  while (!dirs.Next().empty())
+    count++;
+
+  // We should have only gotten two extensions now.
+  EXPECT_EQ(2u, count);
+
+  const DictionaryValue* dictionary =
+      profile_->GetPrefs()->GetDictionary("extensions.settings");
+  ASSERT_TRUE(dictionary);
+  ASSERT_FALSE(dictionary->HasKey("behllobkkfkfnphdnhnkndlbkcpglgmj"));
+}
+
+// Test that unpacked extensions which are referenced in the preferences but
+// had their content deleted are cleaned up during startup.
+TEST_F(ExtensionServiceTest, GarbageCollectUnpackedExtensionsMissingContent) {
+  InitializeEmptyExtensionService();
+
+  ScopedTempDir temp;
+  ASSERT_TRUE(temp.CreateUniqueTempDir());
+
+  // Write the manifest dynamically, since we have to delete the file anyway.
+  FilePath extension_path = temp.path();
+  FilePath manifest_path = extension_path.Append(Extension::kManifestFilename);
+  ASSERT_FALSE(file_util::PathExists(manifest_path));
+
+  // Construct a simple manifest and install it.
+  DictionaryValue manifest;
+  manifest.SetString("version", "1.0");
+  manifest.SetString("name", "Cleanup Unpacked Extensions Missing Content");
+  manifest.SetInteger("manifest_version", 2);
+
+  JSONFileValueSerializer serializer(manifest_path);
+  ASSERT_TRUE(serializer.Serialize(manifest));
+
+  extensions::UnpackedInstaller::Create(service_)->Load(extension_path);
+  loop_.RunAllPending();
+
+  // Make sure it installed correctly.
+  EXPECT_EQ(0u, GetErrors().size());
+  ASSERT_EQ(1u, loaded_.size());
+  EXPECT_EQ(Extension::LOAD, loaded_[0]->location());
+  EXPECT_EQ(1u, service_->extensions()->size());
+
+  std::string extension_id = loaded_[0]->id();
+
+  // Make sure it is in the preferences at this point.
+  const DictionaryValue* initial_dictionary =
+      profile_->GetPrefs()->GetDictionary("extensions.settings");
+  ASSERT_TRUE(initial_dictionary);
+  ASSERT_TRUE(initial_dictionary->HasKey(extension_id));
+
+  // Delete local content, GarbageCollectExtensions, and test whether the key
+  // is still in the preferences.
+  ASSERT_TRUE(file_util::Delete(extension_path, true));
+  service_->GarbageCollectExtensions();
+  loop_.RunAllPending();
+
+  const DictionaryValue* final_dictionary =
+      profile_->GetPrefs()->GetDictionary("extensions.settings");
+  ASSERT_TRUE(final_dictionary);
+  ASSERT_FALSE(final_dictionary->HasKey(extension_id));
+}
+
 // Test installing extensions. This test tries to install few extensions using
 // crx files. If you need to change those crx files, feel free to repackage
 // them, throw away the key used and change the id's above.