Reinstall extensions if they become policy-controlled

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 b5b7ae9f2abba597de3beb72a565ba886c4efee7..f717c608bc34529c918f8b957bbf4bd1f4332c45 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -410,9 +410,20 @@ void ExtensionService::OnExternalExtensionUpdateUrlFound(
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   CHECK(Extension::IdIsValid(id));
 
-  if (GetExtensionById(id, true)) {
-    // Already installed.  Do not change the update URL that the extension set.
-    return;
+  const Extension* extension = GetExtensionById(id, true);
+  if (extension) {
+    if (location != extension->location() &&

[Sam Kerner (Chrome), 2011/06/24 20:03:34]

Uninstalling wipes any extension state in localstorage.  It would be better to
install the new CRX without uninstalling the old one.  A quick read of the code
makes me think that if you let the install run without uninstalling, the new
location be used.  There may be some logic to stop the fetch or install if the
extension is already installed.  You could use a check that the pending
extension's location is different to override such a check.



Testing against Extension::EXTERNAL_POLICY_DOWNLOAD works, but it can be made
more general.  The function Extension::GetHigherPriorityLocation() takes two
Extension::Locations, and returns the one chrome should choose if the same
extension is installed with the two locations.  Policy is the highest priority. 
You could use that function to see if the new location outranks the current one,
and allow the install to continue if it does.

It is important not to allow the install if the locations are the same, because
that happens on every reading of the external sources.  Something like:

if (extension)
  Extension::Location current = extension->location();
  // If the current location outranks the new one, skip this install.
  if (current == Extension::GetHigherPriorityLocation(location, current))
    return;
}

This will do the right thing when a user-installed extension is added as
EXTERNAL_UPDATE_URL, which also outranks a user install.

[gfeher, 2011/06/27 12:59:49]

Your comment got me thinking. In the case you describe, consider:
1. user installs extension X
2. an application registers extension X as EXTERNAL_UPDATE_URL
3. the application gets uninstalled and removes extension X as
EXTERNAL_UPDATE_URL
4. extension X gets uninstalled
(I guess what we are missing is extensions having multiple locations, including
user install.)

This is actually also a problem for policy-controlled extensions, although it
may be acceptable in their special case. (I'll have to talk with gwilson about
it.)

WDYT?

[Sam Kerner (Chrome), 2011/06/27 14:26:30]

Yes, we can't do the right thing here without some way to represent the fact
that extensions are installed for more than one reason.  It was decided that the
complexity of supporting multiple locations was not worth the benefit of fixing
this.  At the time, we were not thinking about policy extensions.

If you feel that this is a significant issue for policy extensions, send an
email to Erik Kay and me, and we can reevaluate.

+        location == Extension::EXTERNAL_POLICY_DOWNLOAD) {
+      // Already installed, but not as a policy controlled extension.
+      // Trigger a reainstall to make its location right, and thus disable its
+      // manual uninstall.
+      std::string error;
+      UninstallExtension(extension->id(), false, &error);
+    } else {
+      // Already installed.  Do not change the update URL that the extension
+      // set.
+      return;
+    }
   }
   pending_extension_manager()->AddFromExternalUpdateUrl(
       id, update_url, location);