[Extensions] Add unit tests for terminated extensions

Change enabled -> terminated state transition to be atomic.

Added unit tests for various functions' behavior with terminated
extensions.

Fix incorrect use of extension_id parameter in UninstallExtension (by renaming the parameter to extension_id_unsafe and the copy to extension_id).

Cleaned up unit tests a bit.

BUG=80756,80752
TEST=

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

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

3 files changed, 158 insertions, 113 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index cba28cc..60387c3 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -656,11 +656,17 @@ void ExtensionService::ReloadExtension(const std::string& extension_id) {
   }
 }
 
-bool ExtensionService::UninstallExtension(const std::string& extension_id,
-                                          bool external_uninstall,
-                                          std::string* error) {
+bool ExtensionService::UninstallExtension(
+    const std::string& extension_id_unsafe,
+    bool external_uninstall,
+    std::string* error) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Copy the extension identifier since the reference might have been
+  // obtained via Extension::id() and the extension may be deleted in
+  // this function.
+  std::string extension_id(extension_id_unsafe);
+
   const Extension* extension = GetInstalledExtension(extension_id);
 
   // Callers should not send us nonexistent extensions.
@@ -692,10 +698,6 @@ bool ExtensionService::UninstallExtension(const std::string& extension_id,
   RecordPermissionMessagesHistogram(
       extension, "Extensions.Permissions_Uninstall");
 
-  // Also copy the extension identifier since the reference might have been
-  // obtained via Extension::id().
-  std::string extension_id_copy(extension_id);
-
   if (profile_->GetTemplateURLModel())
     profile_->GetTemplateURLModel()->UnregisterExtensionKeyword(extension);
 
@@ -703,7 +705,7 @@ bool ExtensionService::UninstallExtension(const std::string& extension_id,
   // any of these resources.
   UnloadExtension(extension_id, UnloadedExtensionInfo::UNINSTALL);
 
-  extension_prefs_->OnExtensionUninstalled(extension_id_copy, location,
+  extension_prefs_->OnExtensionUninstalled(extension_id, location,
                                            external_uninstall);
 
   // Tell the backend to start deleting installed extensions on the file thread.
@@ -713,7 +715,7 @@ bool ExtensionService::UninstallExtension(const std::string& extension_id,
             NewRunnableFunction(
                 &extension_file_util::UninstallExtension,
                 install_directory_,
-                extension_id_copy)))
+                extension_id)))
       NOTREACHED();
   }
 
@@ -1902,13 +1904,15 @@ const Extension* ExtensionService::GetExtensionByIdInternal(
 void ExtensionService::TrackTerminatedExtension(const Extension* extension) {
   if (terminated_extension_ids_.insert(extension->id()).second)
     terminated_extensions_.push_back(make_scoped_refptr(extension));
+
+  UnloadExtension(extension->id(), UnloadedExtensionInfo::DISABLE);
 }
 
 void ExtensionService::UntrackTerminatedExtension(const std::string& id) {
-  if (terminated_extension_ids_.erase(id) <= 0)
+  std::string lowercase_id = StringToLowerASCII(id);
+  if (terminated_extension_ids_.erase(lowercase_id) <= 0)
     return;
 
-  std::string lowercase_id = StringToLowerASCII(id);
   for (ExtensionList::iterator iter = terminated_extensions_.begin();
        iter != terminated_extensions_.end(); ++iter) {
     if ((*iter)->id() == lowercase_id) {
@@ -2046,26 +2050,25 @@ void ExtensionService::DidCreateRenderViewForBackgroundPage(
 }
 
 void ExtensionService::Observe(NotificationType type,
-                                const NotificationSource& source,
-                                const NotificationDetails& details) {
+                               const NotificationSource& source,
+                               const NotificationDetails& details) {
   switch (type.value) {
     case NotificationType::EXTENSION_PROCESS_TERMINATED: {
       if (profile_ != Source<Profile>(source).ptr()->GetOriginalProfile())
         break;
 
       ExtensionHost* host = Details<ExtensionHost>(details).ptr();
-      TrackTerminatedExtension(host->extension());
 
-      // Unload the entire extension. We want it to be in a consistent state:
-      // either fully working or not loaded at all, but never half-crashed.
-      // We do it in a PostTask so that other handlers of this notification will
-      // still have access to the Extension and ExtensionHost.
+      // Mark the extension as terminated and Unload it. We want it to
+      // be in a consistent state: either fully working or not loaded
+      // at all, but never half-crashed.  We do it in a PostTask so
+      // that other handlers of this notification will still have
+      // access to the Extension and ExtensionHost.
       MessageLoop::current()->PostTask(
           FROM_HERE,
           method_factory_.NewRunnableMethod(
-              &ExtensionService::UnloadExtension,
-              host->extension()->id(),
-              UnloadedExtensionInfo::DISABLE));
+              &ExtensionService::TrackTerminatedExtension,
+              host->extension()));
       break;
     }
     case NotificationType::RENDERER_PROCESS_CREATED: {
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index a18aab5..4801dfd 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -483,6 +483,12 @@ class ExtensionService
   // |client| can be NULL for a silent install.
   scoped_refptr<CrxInstaller> MakeCrxInstaller(ExtensionInstallUI* client);
 
+#if defined(UNIT_TEST)
+  void TrackTerminatedExtensionForTest(const Extension* extension) {
+    TrackTerminatedExtension(extension);
+  }
+#endif
+
  private:
   // Contains Extension data that can change during the life of the process,
   // but does not persist across restarts.
@@ -531,8 +537,12 @@ class ExtensionService
                                             bool include_terminated) const;
 
 
-  // Keep track of terminated extensions.
+  // Adds the given extension to the list of terminated extensions if
+  // it is not already there and unloads it.
   void TrackTerminatedExtension(const Extension* extension);
+
+  // Removes the extension with the given id from the list of
+  // terminated extensions if it is there.
   void UntrackTerminatedExtension(const std::string& id);
 
   // Handles sending notification that |extension| was loaded.
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 659971a..ce9c52a 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -693,11 +693,64 @@ class ExtensionServiceTest
     EXPECT_FALSE(file_util::PathExists(path));
   }
 
-  void ValidatePrefKeyCount(size_t count) {
+  void TerminateExtension(const std::string& id) {
+    const Extension* extension = service_->GetInstalledExtension(id);
+    if (!extension) {
+      ADD_FAILURE();
+      return;
+    }
+    service_->TrackTerminatedExtensionForTest(extension);
+  }
+
+  size_t GetPrefKeyCount() {
     const DictionaryValue* dict =
         profile_->GetPrefs()->GetDictionary("extensions.settings");
-    ASSERT_TRUE(dict != NULL);
-    EXPECT_EQ(count, dict->size());
+    if (!dict) {
+      ADD_FAILURE();
+      return 0;
+    }
+    return dict->size();
+  }
+
+  void UninstallExtension(const std::string& id, bool use_helper) {
+    // Verify that the extension is installed.
+    FilePath extension_path = extensions_install_dir_.AppendASCII(id);
+    EXPECT_TRUE(file_util::PathExists(extension_path));
+    size_t pref_key_count = GetPrefKeyCount();
+    EXPECT_GT(pref_key_count, 0u);
+    ValidateIntegerPref(id, "state", Extension::ENABLED);
+
+    // Uninstall it.
+    if (use_helper) {
+      EXPECT_TRUE(ExtensionService::UninstallExtensionHelper(service_, id));
+    } else {
+      EXPECT_TRUE(service_->UninstallExtension(id, false, NULL));
+    }
+    total_successes_ = 0;
+
+    // We should get an unload notification.
+    EXPECT_FALSE(unloaded_id_.empty());
+    EXPECT_EQ(id, unloaded_id_);
+
+    // Verify uninstalled state.
+    size_t new_pref_key_count = GetPrefKeyCount();
+    if (new_pref_key_count == pref_key_count) {
+      ValidateIntegerPref(id, "location",
+                          Extension::EXTERNAL_EXTENSION_UNINSTALLED);
+    } else {
+      EXPECT_EQ(new_pref_key_count, pref_key_count - 1);
+    }
+
+    // The extension should not be in the service anymore.
+    EXPECT_FALSE(service_->GetInstalledExtension(id));
+    loop_.RunAllPending();
+
+    // The directory should be gone.
+    EXPECT_FALSE(file_util::PathExists(extension_path));
+  }
+
+  void ValidatePrefKeyCount(size_t count) {
+    EXPECT_EQ(count, GetPrefKeyCount());
   }
 
   void ValidateBooleanPref(const std::string& extension_id,
@@ -1156,8 +1209,7 @@ TEST_F(ExtensionServiceTest, UninstallingExternalExtensions) {
   ASSERT_TRUE(service_->GetExtensionById(good_crx, false));
 
   // Uninstall it and check that its killbit gets set.
-  service_->UninstallExtension(good_crx, false, NULL);
-  loop_.RunAllPending();
+  UninstallExtension(good_crx, false);
   ValidateIntegerPref(good_crx, "location",
                       Extension::EXTERNAL_EXTENSION_UNINSTALLED);
 
@@ -1775,16 +1827,14 @@ TEST_F(ExtensionServiceTest, InstallAppsWithUnlimtedStorage) {
 
   // Uninstall one of them, unlimited storage should still be granted
   // to the origin.
-  service_->UninstallExtension(id1, false, NULL);
-  loop_.RunAllPending();
+  UninstallExtension(id1, false);
   EXPECT_EQ(1u, service_->extensions()->size());
   EXPECT_TRUE(profile_->GetExtensionSpecialStoragePolicy()->
       IsStorageUnlimited(origin1));
 
 
   // Uninstall the other, unlimited storage should be revoked.
-  service_->UninstallExtension(id2, false, NULL);
-  loop_.RunAllPending();
+  UninstallExtension(id2, false);
   EXPECT_EQ(0u, service_->extensions()->size());
   EXPECT_FALSE(profile_->GetExtensionSpecialStoragePolicy()->
       IsStorageUnlimited(origin2));
@@ -1817,12 +1867,10 @@ TEST_F(ExtensionServiceTest, InstallAppsAndCheckStorageProtection) {
   EXPECT_TRUE(profile_->GetExtensionSpecialStoragePolicy()->
       IsStorageProtected(origin2));
 
-  service_->UninstallExtension(id1, false, NULL);
-  loop_.RunAllPending();
+  UninstallExtension(id1, false);
   EXPECT_EQ(1u, service_->extensions()->size());
 
-  service_->UninstallExtension(id2, false, NULL);
-  loop_.RunAllPending();
+  UninstallExtension(id2, false);
 
   EXPECT_TRUE(service_->extensions()->empty());
   EXPECT_FALSE(profile_->GetExtensionSpecialStoragePolicy()->
@@ -2460,22 +2508,33 @@ TEST_F(ExtensionServiceTest, BlacklistedByPolicyRemovedIfRunning) {
 TEST_F(ExtensionServiceTest, DisableExtension) {
   InitializeEmptyExtensionService();
 
-  // A simple extension that should install without error.
-  FilePath path = data_dir_.AppendASCII("good.crx");
-  InstallCrx(path, true);
-
-  const char* extension_id = good_crx;
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
   EXPECT_FALSE(service_->extensions()->empty());
-  EXPECT_TRUE(service_->GetExtensionById(extension_id, true) != NULL);
-  EXPECT_TRUE(service_->GetExtensionById(extension_id, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, true) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, false) != NULL);
   EXPECT_TRUE(service_->disabled_extensions()->empty());
 
   // Disable it.
-  service_->DisableExtension(extension_id);
+  service_->DisableExtension(good_crx);
 
   EXPECT_TRUE(service_->extensions()->empty());
-  EXPECT_TRUE(service_->GetExtensionById(extension_id, true) != NULL);
-  EXPECT_FALSE(service_->GetExtensionById(extension_id, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, true) != NULL);
+  EXPECT_FALSE(service_->GetExtensionById(good_crx, false) != NULL);
+  EXPECT_FALSE(service_->disabled_extensions()->empty());
+}
+
+TEST_F(ExtensionServiceTest, DisableTerminatedExtension) {
+  InitializeEmptyExtensionService();
+
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
+  TerminateExtension(good_crx);
+  EXPECT_TRUE(service_->GetTerminatedExtension(good_crx));
+
+  // Disable it.
+  service_->DisableExtension(good_crx);
+
+  EXPECT_FALSE(service_->GetTerminatedExtension(good_crx));
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, true) != NULL);
   EXPECT_FALSE(service_->disabled_extensions()->empty());
 }
 
@@ -2552,74 +2611,29 @@ TEST_F(ExtensionServiceTest, ReloadExtensions) {
 // Tests uninstalling normal extensions
 TEST_F(ExtensionServiceTest, UninstallExtension) {
   InitializeEmptyExtensionService();
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
+  UninstallExtension(good_crx, false);
+}
 
-  // A simple extension that should install without error.
-  FilePath path = data_dir_.AppendASCII("good.crx");
-  InstallCrx(path, true);
-
-  // The directory should be there now.
-  const char* extension_id = good_crx;
-  FilePath extension_path = extensions_install_dir_.AppendASCII(extension_id);
-  EXPECT_TRUE(file_util::PathExists(extension_path));
-
-  ValidatePrefKeyCount(1);
-  ValidateIntegerPref(good_crx, "state", Extension::ENABLED);
-  ValidateIntegerPref(good_crx, "location", Extension::INTERNAL);
-
-  // Uninstall it.
-  service_->UninstallExtension(extension_id, false, NULL);
-  total_successes_ = 0;
-
-  // We should get an unload notification.
-  ASSERT_TRUE(unloaded_id_.length());
-  EXPECT_EQ(extension_id, unloaded_id_);
-
-  ValidatePrefKeyCount(0);
-
-  // The extension should not be in the service anymore.
-  ASSERT_FALSE(service_->GetExtensionById(extension_id, false));
-  loop_.RunAllPending();
-
-  // The directory should be gone.
-  EXPECT_FALSE(file_util::PathExists(extension_path));
+TEST_F(ExtensionServiceTest, UninstallTerminatedExtension) {
+  InitializeEmptyExtensionService();
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
+  TerminateExtension(good_crx);
+  UninstallExtension(good_crx, false);
 }
 
 // Tests the uninstaller helper.
 TEST_F(ExtensionServiceTest, UninstallExtensionHelper) {
   InitializeEmptyExtensionService();
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
+  UninstallExtension(good_crx, true);
+}
 
-  // A simple extension that should install without error.
-  FilePath path = data_dir_.AppendASCII("good.crx");
-  InstallCrx(path, true);
-
-  // The directory should be there now.
-  const char* extension_id = good_crx;
-  FilePath extension_path = extensions_install_dir_.AppendASCII(extension_id);
-  EXPECT_TRUE(file_util::PathExists(extension_path));
-
-  bool result = ExtensionService::UninstallExtensionHelper(service_,
-                                                           extension_id);
-  total_successes_ = 0;
-
-  EXPECT_TRUE(result);
-
-  // We should get an unload notification.
-  ASSERT_TRUE(unloaded_id_.length());
-  EXPECT_EQ(extension_id, unloaded_id_);
-
-  ValidatePrefKeyCount(0);
-
-  // The extension should not be in the service anymore.
-  ASSERT_FALSE(service_->GetExtensionById(extension_id, false));
-  loop_.RunAllPending();
-
-  // The directory should be gone.
-  EXPECT_FALSE(file_util::PathExists(extension_path));
-
-  // Attempt to uninstall again. This should fail as we just removed the
-  // extension.
-  result = ExtensionService::UninstallExtensionHelper(service_, extension_id);
-  EXPECT_FALSE(result);
+TEST_F(ExtensionServiceTest, UninstallExtensionHelperTerminated) {
+  InitializeEmptyExtensionService();
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
+  TerminateExtension(good_crx);
+  UninstallExtension(good_crx, true);
 }
 
 // Verifies extension state is removed upon uninstall
@@ -3512,15 +3526,14 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataUninstall) {
 TEST_F(ExtensionServiceTest, ProcessSyncDataSettings) {
   InitializeEmptyExtensionService();
 
-  FilePath extension_path = data_dir_.AppendASCII("good.crx");
-  InstallCrx(extension_path, true);
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
   EXPECT_TRUE(service_->IsExtensionEnabled(good_crx));
   EXPECT_FALSE(service_->IsIncognitoEnabled(good_crx));
 
   ExtensionSyncData extension_sync_data;
   extension_sync_data.id = good_crx;
   extension_sync_data.version =
-      *(service_->GetExtensionById(good_crx, true)->version());
+      *(service_->GetInstalledExtension(good_crx)->version());
 
   extension_sync_data.enabled = false;
   service_->ProcessSyncData(extension_sync_data, &AllExtensions);
@@ -3542,12 +3555,31 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataSettings) {
   EXPECT_FALSE(service_->pending_extension_manager()->IsIdPending(good_crx));
 }
 
+TEST_F(ExtensionServiceTest, ProcessSyncDataTerminatedExtension) {
+  InitializeExtensionServiceWithUpdater();
+
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
+  TerminateExtension(good_crx);
+  EXPECT_TRUE(service_->IsExtensionEnabled(good_crx));
+  EXPECT_FALSE(service_->IsIncognitoEnabled(good_crx));
+
+  ExtensionSyncData extension_sync_data;
+  extension_sync_data.id = good_crx;
+  extension_sync_data.version =
+      *(service_->GetInstalledExtension(good_crx)->version());
+  extension_sync_data.enabled = false;
+  extension_sync_data.incognito_enabled = true;
+  service_->ProcessSyncData(extension_sync_data, &AllExtensions);
+  EXPECT_FALSE(service_->IsExtensionEnabled(good_crx));
+  EXPECT_TRUE(service_->IsIncognitoEnabled(good_crx));
+
+  EXPECT_FALSE(service_->pending_extension_manager()->IsIdPending(good_crx));
+}
+
 TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) {
   InitializeExtensionServiceWithUpdater();
 
-  // Install the extension.
-  FilePath extension_path = data_dir_.AppendASCII("good.crx");
-  InstallCrx(extension_path, true);
+  InstallCrx(data_dir_.AppendASCII("good.crx"), true);
   EXPECT_TRUE(service_->IsExtensionEnabled(good_crx));
   EXPECT_FALSE(service_->IsIncognitoEnabled(good_crx));
 
@@ -3555,7 +3587,7 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataVersionCheck) {
   extension_sync_data.id = good_crx;
   extension_sync_data.enabled = true;
   extension_sync_data.version =
-      *(service_->GetExtensionById(good_crx, true)->version());
+      *(service_->GetInstalledExtension(good_crx)->version());
 
   // Should do nothing if extension version == sync version.
   service_->ProcessSyncData(extension_sync_data, &AllExtensions);