Don't try to reload blacklisted extensions.

BUG=373842

Review URL: https://codereview.chromium.org/291603002

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

chrome/browser/extensions/extension_service.cc

@@ -595,20 +595,33 @@ bool ExtensionService::UpdateExtension(const std::string& id,
   return true;
 }
 
-void ExtensionService::ReloadExtension(const std::string extension_id) {
+void ExtensionService::ReloadExtension(
+    // "transient" because the process of reloading may cause the reference
+    // to become invalid. Instead, use |extension_id|, a copy.
+    const std::string& transient_extension_id) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // If the extension is already reloading, don't reload again.
-  if (extension_prefs_->GetDisableReasons(extension_id) &
+  if (extension_prefs_->GetDisableReasons(transient_extension_id) &
       Extension::DISABLE_RELOAD) {
     return;
   }
 
+  // Ignore attempts to reload a blacklisted extension. Sometimes this can
+  // happen in a convoluted reload sequence triggered by the termination of a
+  // blacklisted extension and a naive attempt to reload it. For an example see
+  // http://crbug.com/373842.
+  if (registry_->blacklisted_extensions().Contains(transient_extension_id))
+    return;
+
   base::FilePath path;
-  const Extension* current_extension = GetExtensionById(extension_id, false);
+
+  std::string extension_id = transient_extension_id;
+  const Extension* transient_current_extension =
+      GetExtensionById(extension_id, false);
 
   // Disable the extension if it's loaded. It might not be loaded if it crashed.
-  if (current_extension) {
+  if (transient_current_extension) {
     // If the extension has an inspector open for its background page, detach
     // the inspector and hang onto a cookie for it, so that we can reattach
     // later.
@@ -624,15 +637,18 @@ void ExtensionService::ReloadExtension(const std::string extension_id) {
       orphaned_dev_tools_[extension_id] = agent_host;
     }
 
-    path = current_extension->path();
+    path = transient_current_extension->path();
     // BeingUpgraded is set back to false when the extension is added.
-    system_->runtime_data()->SetBeingUpgraded(current_extension, true);
+    system_->runtime_data()->SetBeingUpgraded(transient_current_extension,
+                                              true);
     DisableExtension(extension_id, Extension::DISABLE_RELOAD);
     reloading_extensions_.insert(extension_id);
   } else {
     path = unloaded_extension_paths_[extension_id];
   }
 
+  transient_current_extension = NULL;
+
   if (delayed_installs_.Contains(extension_id)) {
     FinishDelayedInstallation(extension_id);
     return;
@@ -666,12 +682,16 @@ void ExtensionService::ReloadExtension(const std::string extension_id) {
   SetBeingReloaded(extension_id, false);
 }
 
-bool ExtensionService::UninstallExtension(const std::string& extension_id,
-                                          bool external_uninstall,
-                                          base::string16* error) {
+bool ExtensionService::UninstallExtension(
+    // "transient" because the process of uninstalling may cause the reference
+    // to become invalid. Instead, use |extenson->id()|.
+    const std::string& transient_extension_id,
+    bool external_uninstall,
+    base::string16* error) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  scoped_refptr<const Extension> extension(GetInstalledExtension(extension_id));
+  scoped_refptr<const Extension> extension =
+      GetInstalledExtension(transient_extension_id);
 
   // Callers should not send us nonexistent extensions.
   CHECK(extension.get());
@@ -724,7 +744,7 @@ bool ExtensionService::UninstallExtension(const std::string& extension_id,
 
   // Unload before doing more cleanup to ensure that nothing is hanging on to
   // any of these resources.
-  UnloadExtension(extension_id, UnloadedExtensionInfo::REASON_UNINSTALL);
+  UnloadExtension(extension->id(), UnloadedExtensionInfo::REASON_UNINSTALL);
 
   // Tell the backend to start deleting installed extensions on the file thread.
   if (!Manifest::IsUnpackedLocation(extension->location())) {
@@ -732,16 +752,16 @@ bool ExtensionService::UninstallExtension(const std::string& extension_id,
             FROM_HERE,
             base::Bind(&extensions::file_util::UninstallExtension,
                        install_directory_,
-                       extension_id)))
+                       extension->id())))
       NOTREACHED();
   }
 
   // Do not remove the data of ephemeral apps. They will be garbage collected by
   // EphemeralAppService.
-  if (!extension_prefs_->IsEphemeralApp(extension_id))
+  if (!extension_prefs_->IsEphemeralApp(extension->id()))
     extensions::DataDeleter::StartDeleting(profile_, extension.get());
 
-  UntrackTerminatedExtension(extension_id);
+  UntrackTerminatedExtension(extension->id());
 
   // Notify interested parties that we've uninstalled this extension.
   content::NotificationService::current()->Notify(
@@ -750,14 +770,14 @@ bool ExtensionService::UninstallExtension(const std::string& extension_id,
       content::Details<const Extension>(extension.get()));
 
   if (extension_sync_service_) {
-    extension_sync_service_->ProcessSyncUninstallExtension(extension_id,
+    extension_sync_service_->ProcessSyncUninstallExtension(extension->id(),
                                                            sync_change);
   }
 
-  delayed_installs_.Remove(extension_id);
+  delayed_installs_.Remove(extension->id());
 
-  extension_prefs_->OnExtensionUninstalled(extension_id, extension->location(),
-                                           external_uninstall);
+  extension_prefs_->OnExtensionUninstalled(
+      extension->id(), extension->location(), external_uninstall);
 
   // Track the uninstallation.
   UMA_HISTOGRAM_ENUMERATION("Extensions.ExtensionUninstalled", 1, 2);

chrome/browser/extensions/extension_service.h

@@ -193,17 +193,13 @@ class ExtensionService
 
   // Reloads the specified extension, sending the onLaunched() event to it if it
   // currently has any window showing.
-  void ReloadExtension(const std::string extension_id);
+  void ReloadExtension(const std::string& extension_id);
 
   // Uninstalls the specified extension. Callers should only call this method
   // with extensions that exist. |external_uninstall| is a magical parameter
   // that is only used to send information to ExtensionPrefs, which external
   // callers should never set to true.
   //
-  // We pass the |extension_id| by value to avoid having it deleted from under
-  // us incase someone calls it with Extension::id() or another string that we
-  // are going to delete in this function.
-  //
   // TODO(aa): Remove |external_uninstall| -- this information should be passed
   // to ExtensionPrefs some other way.
   virtual bool UninstallExtension(const std::string& extension_id,
@@ -701,8 +697,8 @@ class ExtensionService
                            UnloadBlacklistedExtensionPolicy);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            WillNotLoadBlacklistedExtensionsFromDirectory);
-  FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
-                           BlacklistedInPrefsFromStartup);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest, ReloadBlacklistedExtension);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest, BlacklistedInPrefsFromStartup);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            GreylistedExtensionDisabled);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,

chrome/browser/extensions/extension_service_unittest.cc

@@ -3018,6 +3018,19 @@ bool IsExtension(const Extension* extension) {
   return extension->GetType() == Manifest::TYPE_EXTENSION;
 }
 
+#if defined(ENABLE_BLACKLIST_TESTS)
+std::set<std::string> StringSet(const std::string& s) {
+  std::set<std::string> set;
+  set.insert(s);
+  return set;
+}
+std::set<std::string> StringSet(const std::string& s1, const std::string& s2) {
+  std::set<std::string> set = StringSet(s1);
+  set.insert(s2);
+  return set;
+}
+#endif  // defined(ENABLE_BLACKLIST_TESTS)
+
 }  // namespace
 
 // Test adding a pending extension.
@@ -3645,6 +3658,35 @@ TEST_F(ExtensionServiceTest, GreylistUnknownDontChange) {
   EXPECT_TRUE(enabled_extensions.Contains(good2));
   EXPECT_FALSE(disabled_extensions.Contains(good2));
 }
+
+// Tests that blacklisted extensions cannot be reloaded, both those loaded
+// before and after extension service startup.
+TEST_F(ExtensionServiceTest, ReloadBlacklistedExtension) {
+  extensions::TestBlacklist test_blacklist;
+
+  InitializeGoodInstalledExtensionService();
+  test_blacklist.Attach(service_->blacklist_);
+
+  test_blacklist.SetBlacklistState(
+      good1, extensions::BLACKLISTED_MALWARE, false);
+  service_->Init();
+  test_blacklist.SetBlacklistState(
+      good2, extensions::BLACKLISTED_MALWARE, false);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(StringSet(good0), registry_->enabled_extensions().GetIDs());
+  EXPECT_EQ(StringSet(good1, good2),
+            registry_->blacklisted_extensions().GetIDs());
+
+  service_->ReloadExtension(good1);
+  service_->ReloadExtension(good2);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(StringSet(good0), registry_->enabled_extensions().GetIDs());
+  EXPECT_EQ(StringSet(good1, good2),
+            registry_->blacklisted_extensions().GetIDs());
+}
+
 #endif  // defined(ENABLE_BLACKLIST_TESTS)
 
 // Will not install extension blacklisted by policy.