Cleaning Up Extensions When Local Content Removed

chrome/browser/extensions/extension_service_unittest.cc

Index: chrome/browser/extensions/extension_service_unittest.cc
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 3546a474a5f8d3e9634560e90e1c1e3fa2c819c3..6d0c22d847491a342a20d4a4770e3053dad9871f 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -249,7 +249,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.
@@ -1005,7 +1004,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.
@@ -1149,9 +1147,43 @@ 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")));
+  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_
@@ -1167,7 +1199,7 @@ TEST_F(ExtensionServiceTest, CleanupOnStartup) {
   {
     DictionaryPrefUpdate update(profile_->GetPrefs(), "extensions.settings");
     DictionaryValue* dict = update.Get();
-    ASSERT_TRUE(dict != NULL);
+    ASSERT_TRUE(dict);
     dict->Remove("behllobkkfkfnphdnhnkndlbkcpglgmj", NULL);
   }
 
@@ -1190,6 +1222,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();

[Aaron Boodman, 2012/04/07 18:01:03]

I don't think this is needed unless you're using plugin functionality somehow.
Try removing it.

[Devlin, 2012/04/14 18:13:21]

Tried removing; it broke.

+
+  FilePath source_dir = data_dir_.AppendASCII("good").AppendASCII("Extensions");
+  FilePath pref_path = source_dir.DirName().AppendASCII("Preferences");
+
+  InitializeInstalledExtensionService(pref_path, source_dir);

[Aaron Boodman, 2012/04/07 18:01:03]

ASSERT the number of extensions in the prefs you're starting with?

[Devlin, 2012/04/14 18:13:21]

Done.

+
+  // 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);
+
+  DictionaryPrefUpdate update(profile_->GetPrefs(), "extensions.settings");

[Aaron Boodman, 2012/04/07 18:01:03]

Nit: If you're not actually doing any updating, you can just access the
Preferences object from GetPrefs() directly.

[Devlin, 2012/04/14 18:13:21]

Done.

+  DictionaryValue* dictionary = update.Get();
+  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) {

[Aaron Boodman, 2012/04/07 18:01:03]

Can you look into moving all the garbage collection tests extension
extension_garbage_collector_unittest.cc?

[Devlin, 2012/04/14 18:13:21]

Unfortunately, these tests use a few things unique to ExtensionServiceTest
(versus ExtensionServiceTestBase, which is what's in the header file), e.g.
loaded_, ValidatePrefKeyCount(), etc. Moving the tests would mean moving the
definition of ExtensionServiceTest out of the cc file. Since other services use
this as a testing ground (e.g. installing, uninstalling, packing extensions), I
assumed it was alright for garbage collection to be here, too. If you want me to
refactor enough to move garbage collection out, lemme know and I'd be happy to
do it in a separate CL (this one's growing a little large, and it's a decent
size refactor).

+  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.
+  DictionaryPrefUpdate initial_update(
+      profile_->GetPrefs(), "extensions.settings");
+  DictionaryValue* initial_dictionary = initial_update.Get();
+  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();
+
+  DictionaryPrefUpdate final_update(
+      profile_->GetPrefs(), "extensions.settings");
+  DictionaryValue* final_dictionary = final_update.Get();
+  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.