Fix browser crash in extension system.

When an external extension is removed from the json file
(for example by a 3rd party installer) after the user
has explicitly uninstalled it, Chrome would start crashing
and crashing.

This is now fixed. Also added a couple of unit tests for
this case (for each external provider) and cleaned up one
of those unit tests to make it match what the other was
doing.

BUG=http://crbug.com/30505
TEST=Covered by unit test.

Review URL: http://codereview.chromium.org/504016

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

3 files changed, 62 insertions, 118 deletions

diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index aef47ac..2b4abfa 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -76,6 +76,17 @@ void InstalledExtensions::VisitInstalledExtensions(
         continue;
       }
     }
+    int state_value;
+    if (!ext->GetInteger(kPrefState, &state_value)) {
+      LOG(WARNING) << "Missing state pref for extension " << *extension_id;
+      NOTREACHED();
+      continue;
+    }
+    if (state_value == Extension::KILLBIT) {
+      LOG(WARNING) << "External extension has been uninstalled by the user "
+                   << *extension_id;
+      continue;
+    }
     FilePath::StringType path;
     if (!ext->GetString(kPrefPath, &path)) {
       LOG(WARNING) << "Missing path pref for extension " << *extension_id;
diff --git a/chrome/browser/extensions/extension_prefs.h b/chrome/browser/extensions/extension_prefs.h
index 86ad883..e7046a3 100644
--- a/chrome/browser/extensions/extension_prefs.h
+++ b/chrome/browser/extensions/extension_prefs.h
@@ -129,7 +129,8 @@ class InstalledExtensions {
 
   // Runs |callback| for each installed extension with the path to the
   // version directory and the location. Blacklisted extensions won't trigger
-  // the callback. Ownership of |callback| is transferred to callee.
+  // the callback and neither will external extensions the user has explicitly
+  // uninstalled. Ownership of |callback| is transferred to callee.
   void VisitInstalledExtensions(Callback *callback);
 
  private:
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index dc8bdd6..74709ed 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -327,6 +327,9 @@ class ExtensionsServiceTest
   }
 
  protected:
+  void TestExternalProvider(MockExtensionProvider* provider,
+                            Extension::Location location);
+
   void InstallExtension(const FilePath& path,
                         bool should_succeed) {
     ASSERT_TRUE(file_util::PathExists(path));
@@ -1286,30 +1289,20 @@ TEST_F(ExtensionsServiceTest, GenerateID) {
   ASSERT_EQ(previous_id, loaded_[0]->id());
 }
 
-// Tests the external installation feature
-#if defined(OS_WIN)
-
-TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
-  // This should all work, even when normal extension installation is disabled.
-  InitializeEmptyExtensionsService();
-  set_extensions_enabled(false);
+void ExtensionsServiceTest::TestExternalProvider(
+    MockExtensionProvider* provider, Extension::Location location) {
   // Verify that starting with no providers loads no extensions.
   service_->Init();
   loop_.RunAllPending();
   ASSERT_EQ(0u, loaded_.size());
 
-  // Now add providers. Extension system takes ownership of the objects.
-  MockExtensionProvider* reg_provider =
-      new MockExtensionProvider(Extension::EXTERNAL_REGISTRY);
-  SetMockExternalProvider(Extension::EXTERNAL_REGISTRY, reg_provider);
-
   // Register a test extension externally using the mock registry provider.
   FilePath source_path;
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &source_path));
   source_path = source_path.AppendASCII("extensions").AppendASCII("good.crx");
 
   // Add the extension.
-  reg_provider->UpdateOrAddExtension(good_crx, "1.0.0.0", source_path);
+  provider->UpdateOrAddExtension(good_crx, "1.0.0.0", source_path);
 
   // Reloading extensions should find our externally registered extension
   // and install it.
@@ -1318,11 +1311,11 @@ TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
 
   ASSERT_EQ(0u, GetErrors().size());
   ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(Extension::EXTERNAL_REGISTRY, loaded_[0]->location());
+  ASSERT_EQ(location, loaded_[0]->location());
   ASSERT_EQ("1.0.0.0", loaded_[0]->version()->GetString());
   ValidatePrefKeyCount(1);
   ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_REGISTRY);
+  ValidateIntegerPref(good_crx, L"location", location);
 
   // Reload extensions without changing anything. The extension should be
   // loaded again.
@@ -1333,11 +1326,11 @@ TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
   ASSERT_EQ(1u, loaded_.size());
   ValidatePrefKeyCount(1);
   ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_REGISTRY);
+  ValidateIntegerPref(good_crx, L"location", location);
 
   // Now update the extension with a new version. We should get upgraded.
   source_path = source_path.DirName().AppendASCII("good2.crx");
-  reg_provider->UpdateOrAddExtension(good_crx, "1.0.0.1", source_path);
+  provider->UpdateOrAddExtension(good_crx, "1.0.0.1", source_path);
 
   loaded_.clear();
   service_->CheckForExternalUpdates();
@@ -1347,7 +1340,7 @@ TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
   ASSERT_EQ("1.0.0.1", loaded_[0]->version()->GetString());
   ValidatePrefKeyCount(1);
   ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_REGISTRY);
+  ValidateIntegerPref(good_crx, L"location", location);
 
   // Uninstall the extension and reload. Nothing should happen because the
   // preference should prevent us from reinstalling.
@@ -1365,10 +1358,9 @@ TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
   ASSERT_EQ(0u, loaded_.size());
   ValidatePrefKeyCount(1);
   ValidateIntegerPref(good_crx, L"state", Extension::KILLBIT);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_REGISTRY);
+  ValidateIntegerPref(good_crx, L"location", location);
 
-  // Now clear the preference, reinstall, then remove the reg key. The extension
-  // should be uninstalled.
+  // Now clear the preference and reinstall.
   SetPrefInteg(good_crx, L"state", Extension::ENABLED);
   prefs_->ScheduleSavePersistentPrefs();
 
@@ -1378,10 +1370,11 @@ TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
   ASSERT_EQ(1u, loaded_.size());
   ValidatePrefKeyCount(1);
   ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_REGISTRY);
+  ValidateIntegerPref(good_crx, L"location", location);
 
-  // Now test an externally triggered uninstall (deleting the registry key).
-  reg_provider->RemoveExtension(good_crx);
+  // Now test an externally triggered uninstall (deleting the registry key or
+  // the pref entry).
+  provider->RemoveExtension(good_crx);
 
   loaded_.clear();
   service_->LoadAllExtensions();
@@ -1391,120 +1384,59 @@ TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
 
   // The extension should also be gone from the install directory.
   ASSERT_FALSE(file_util::PathExists(install_path));
-}
-
-#endif
-
-TEST_F(ExtensionsServiceTest, ExternalInstallPref) {
-  InitializeEmptyExtensionsService();
-  // Verify that starting with no providers loads no extensions.
-  service_->Init();
-  loop_.RunAllPending();
-  ASSERT_EQ(0u, loaded_.size());
-
-  // Now add providers. Extension system takes ownership of the objects.
-  MockExtensionProvider* pref_provider =
-      new MockExtensionProvider(Extension::EXTERNAL_PREF);
-  SetMockExternalProvider(Extension::EXTERNAL_PREF, pref_provider);
 
-  // Register a external extension using preinstalled preferences.
-  FilePath source_path;
-  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &source_path));
-  source_path = source_path.AppendASCII("extensions").AppendASCII("good.crx");
-
-  // Add the extension.
-  pref_provider->UpdateOrAddExtension(good_crx, "1.0", source_path);
+  // Now test the case where user uninstalls and then the extension is removed
+  // from the external provider.
 
-  // Checking for updates should find our externally registered extension
-  // and install it.
+  provider->UpdateOrAddExtension(good_crx, "1.0", source_path);
   service_->CheckForExternalUpdates();
   loop_.RunAllPending();
 
-  ASSERT_EQ(0u, GetErrors().size());
   ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(Extension::EXTERNAL_PREF, loaded_[0]->location());
-  ASSERT_EQ("1.0.0.0", loaded_[0]->version()->GetString());
-  ValidatePrefKeyCount(1);
-  ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_PREF);
-
-  // Reload extensions without changing anything. The extension should be
-  // loaded again.
-  loaded_.clear();
-  service_->ReloadExtensions();
-  loop_.RunAllPending();
-  ASSERT_EQ(0u, GetErrors().size());
-  ASSERT_EQ(1u, loaded_.size());
-  ValidatePrefKeyCount(1);
-  ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_PREF);
-
-  // Now update the extension with a new version. We should get upgraded.
-  source_path = source_path.DirName().AppendASCII("good2.crx");
-  pref_provider->UpdateOrAddExtension(good_crx, "1.0.0.1", source_path);
+  ASSERT_EQ(1u, GetErrors().size());
 
+  // User uninstalls.
   loaded_.clear();
-  service_->CheckForExternalUpdates();
-  loop_.RunAllPending();
-  ASSERT_EQ(0u, GetErrors().size());
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ("1.0.0.1", loaded_[0]->version()->GetString());
-  ValidatePrefKeyCount(1);
-  ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_PREF);
-
-  // Uninstall the extension and reload. Nothing should happen because the
-  // preference should prevent us from reinstalling.
-  std::string id = loaded_[0]->id();
   service_->UninstallExtension(id, false);
   loop_.RunAllPending();
-
-  // The extension should also be gone from the install directory.
-  FilePath install_path = extensions_install_dir_.AppendASCII(id);
-  ASSERT_FALSE(file_util::PathExists(install_path));
-
-  loaded_.clear();
-  service_->CheckForExternalUpdates();
-  loop_.RunAllPending();
   ASSERT_EQ(0u, loaded_.size());
-  ValidatePrefKeyCount(1);
-  ValidateIntegerPref(good_crx, L"state", Extension::KILLBIT);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_PREF);
-
-  // Now clear the preference and reinstall.
-  SetPrefInteg(good_crx, L"state", Extension::ENABLED);
-  prefs_->ScheduleSavePersistentPrefs();
-
-  loaded_.clear();
-  service_->CheckForExternalUpdates();
-  loop_.RunAllPending();
-  ASSERT_EQ(1u, loaded_.size());
-  ValidatePrefKeyCount(1);
-  ValidateIntegerPref(good_crx, L"state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, L"location", Extension::EXTERNAL_PREF);
 
-  // Now test an externally triggered uninstall (deleting id from json file).
-  pref_provider->RemoveExtension(good_crx);
+  // Then remove the extension from the extension provider.
+  provider->RemoveExtension(good_crx);
 
+  // Should still be at 0.
   loaded_.clear();
   service_->LoadAllExtensions();
   loop_.RunAllPending();
   ASSERT_EQ(0u, loaded_.size());
-  ValidatePrefKeyCount(0);
-
-  // The extension should also be gone from the install directory.
-  ASSERT_FALSE(file_util::PathExists(install_path));
+  ValidatePrefKeyCount(1);
+}
 
-  // It should still work if extensions are disabled (disableness doesn't
-  // apply to externally registered extensions).
+// Tests the external installation feature
+#if defined(OS_WIN)
+TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
+  // This should all work, even when normal extension installation is disabled.
+  InitializeEmptyExtensionsService();
   set_extensions_enabled(false);
 
-  pref_provider->UpdateOrAddExtension(good_crx, "1.0", source_path);
-  service_->CheckForExternalUpdates();
-  loop_.RunAllPending();
+  // Now add providers. Extension system takes ownership of the objects.
+  MockExtensionProvider* reg_provider =
+      new MockExtensionProvider(Extension::EXTERNAL_REGISTRY);
+  SetMockExternalProvider(Extension::EXTERNAL_REGISTRY, reg_provider);
+  TestExternalProvider(reg_provider, Extension::EXTERNAL_REGISTRY);
+}
+#endif
 
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(1u, GetErrors().size());
+TEST_F(ExtensionsServiceTest, ExternalInstallPref) {
+  // This should all work, even when normal extension installation is disabled.
+  InitializeEmptyExtensionsService();
+  set_extensions_enabled(false);
+
+  // Now add providers. Extension system takes ownership of the objects.
+  MockExtensionProvider* pref_provider =
+      new MockExtensionProvider(Extension::EXTERNAL_PREF);
+  SetMockExternalProvider(Extension::EXTERNAL_PREF, pref_provider);
+  TestExternalProvider(pref_provider, Extension::EXTERNAL_PREF);
 }
 
 TEST_F(ExtensionsServiceTest, ExternalPrefProvider) {