Cleaning Up Extensions When Local Content Removed

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 6340724de9cf9be8daeaf6880ad00255d9f87797..6cd9ff9cc447395cbc1f2874f2d28c2dc98c7554 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -110,7 +110,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
@@ -1885,7 +1884,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());
@@ -1934,8 +1933,27 @@ void ExtensionService::GarbageCollectExtensions() {
       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;
+  for (size_t i = 0; i < info->size(); ++i) {
+    // Uninstall the extension if the path does not exist. If the extension
+    // failed to load fully (e.g. the user deleted an external extension's
+    // manifest or the manifest points to the wrong path), we cannot use
+    // UninstallExtension, which relies on a valid Extension object.
+    if (!file_util::PathExists(info->at(i)->extension_path)) {

[Aaron Boodman, 2012/03/21 21:00:47]

You cannot use file_util::PathExists here (or indeed anything in file_util)
because it touches disk. In debug builds this will DCHECK.

You need to post a message to the file thread to collect a list of extension IDs
that are known there, and then compare it to the list that is known in
ExtensionService.

Stepping back slightly, I would like to pull GarbageCollection out into its own
little helper class (I hate how big ExtensionService is).

It would look a little similar to something like UnpackedInstaller. It would
start out on the UI thread, gather a list of known extension IDs, bounce to the
file thread, delete any unknown directories as well as collect a list of known
ids there, then bounce back to the UI thread where it would uninstall any
extensions with no directory.

Do you think this is something you could do?

[Devlin, 2012/03/21 21:48:38]

Not entirely sure why file_util works...but it does. (Is this something that
should be looked into separately?)
http://imgur.com/zJlhO

Otherwise, I see no problem with separating out GarbageCollection; I should be
able to do that.

+      LOG(WARNING) << "Could not access local content for extension with id " <<
+          info->at(i)->extension_id << "; uninstalling extension.";
+      LOG(WARNING) << "Path: " << info->at(i)->extension_path.value();
+      if (!GetInstalledExtension(info->at(i)->extension_id)) {
+        UnloadExtension(info->at(i)->extension_id,
+                        extension_misc::UNLOAD_REASON_UNINSTALL);
+        extension_prefs_->OnExtensionUninstalled(
+            info->at(i)->extension_id, info->at(i)->extension_location, false);
+      } else {
+        UninstallExtension(info->at(i)->extension_id, false, NULL);
+      }
+    } else {
+      extension_paths[info->at(i)->extension_id] = info->at(i)->extension_path;
+    }
+  }
 
   if (!BrowserThread::PostTask(
           BrowserThread::FILE, FROM_HERE,
@@ -2123,7 +2141,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.