Add minimum version to extension management

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 283f42a1fc3059b09f965b156b5455ac1dbfa62c..aa8b3653ecd20eb43366665b3eb16c093ce13b2f 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -853,6 +853,7 @@ void ExtensionService::DisableExtension(
   // preferences.
   if (extension &&
       disable_reason != Extension::DISABLE_RELOAD &&
+      disable_reason != Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY &&
       !system_->management_policy()->UserMayModifySettings(extension, NULL) &&
       extension->location() != Manifest::EXTERNAL_COMPONENT) {
     return;
@@ -1111,27 +1112,69 @@ base::SequencedTaskRunner* ExtensionService::GetFileTaskRunner() {
 void ExtensionService::CheckManagementPolicy() {
   std::vector<std::string> to_unload;
   std::map<std::string, Extension::DisableReason> to_disable;
+  std::vector<std::string> to_enable;
 
   // Loop through the extensions list, finding extensions we need to unload or
   // disable.
-  const ExtensionSet& extensions = registry_->enabled_extensions();
-  for (ExtensionSet::const_iterator iter = extensions.begin();
-       iter != extensions.end(); ++iter) {
-    const Extension* extension = (iter->get());
-    if (!system_->management_policy()->UserMayLoad(extension, NULL))
+  for (scoped_refptr<const Extension> extension :
+       registry_->enabled_extensions()) {
+    if (!system_->management_policy()->UserMayLoad(extension.get(), nullptr))
       to_unload.push_back(extension->id());
     Extension::DisableReason disable_reason = Extension::DISABLE_NONE;
     if (system_->management_policy()->MustRemainDisabled(
-            extension, &disable_reason, NULL))
+            extension.get(), &disable_reason, nullptr))
       to_disable[extension->id()] = disable_reason;
   }
 
-  for (size_t i = 0; i < to_unload.size(); ++i)
-    UnloadExtension(to_unload[i], UnloadedExtensionInfo::REASON_DISABLE);
+  extensions::ExtensionManagement* management =
+      extensions::ExtensionManagementFactory::GetForBrowserContext(profile());
+
+  // Loop through the disabled extension list, find extensions to re-enable
+  // automatically.
+  for (scoped_refptr<const Extension> extension :
+       registry_->disabled_extensions()) {
+    // Find all disabled extensions disabled due to minimum version requirement,
+    // but now satisfying it.
+    if (management->CheckMinimumVersionRequirement(extension.get(), nullptr) &&
+        extension_prefs_->HasDisableReason(
+            extension->id(), Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY)) {
+      // Is DISABLE_UPDATE_REQUIRED_BY_POLICY the *only* reason?
+      if (extension_prefs_->GetDisableReasons(extension->id()) ==
+          Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY) {
+        // We need to enable those disabled *only* due to minimum version
+        // requirement.
+        to_enable.push_back(extension->id());
+      }
+      extension_prefs_->RemoveDisableReason(
+          extension->id(), Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY);
+    }
+  }
+
+  for (const std::string& id : to_unload)
+    UnloadExtension(id, UnloadedExtensionInfo::REASON_DISABLE);
 
   for (std::map<std::string, Extension::DisableReason>::const_iterator i =
            to_disable.begin(); i != to_disable.end(); ++i)
     DisableExtension(i->first, i->second);
+
+  for (const std::string& id : to_enable)
+    EnableExtension(id);
+
+  if (updater_.get()) {
+    // Find all extensions (including the ones that got disabled just now)
+    // disabled due to minimum version requirement from policy, and check for
+    // update.
+    extensions::ExtensionUpdater::CheckParams params;
+    for (scoped_refptr<const Extension> extension :
+         registry_->disabled_extensions()) {
+      if (extension_prefs_->GetDisableReasons(extension->id()) ==
+          Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY) {
+        params.ids.push_back(extension->id());
+      }
+    }
+    if (!params.ids.empty())
+      updater_->CheckNow(params);
+  }
 }
 
 void ExtensionService::CheckForUpdatesSoon() {
@@ -1607,13 +1650,22 @@ void ExtensionService::OnExtensionInstalled(
   // Unsupported requirements overrides the management policy.
   if (install_flags & extensions::kInstallFlagHasRequirementErrors) {
     disable_reasons |= Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
-  // If the extension was disabled because of unsupported requirements but
-  // now supports all requirements after an update and there are not other
-  // disable reasons, enable it.
   } else if (extension_prefs_->GetDisableReasons(id) ==
              Extension::DISABLE_UNSUPPORTED_REQUIREMENT) {
-    disable_reasons = Extension::DISABLE_NONE;
-    extension_prefs_->ClearDisableReasons(id);
+    // Satisfies now, remove the corresponding disable reason instead.

[Finnur, 2014/11/19 15:47:39]

"Satisfies now" is weird.

But more importantly, I think there's a bug in the code now.  It is now removing
the disable reason _only_ if DISABLE_UNSUPPORTED_REQUIREMENT was the sole reason
it was disabled. Compare my else clause to your else if. I think you need to
convert this to a plain else, right?

[binjin, 2014/11/19 17:21:42]

Ooops, yes.

[Finnur, 2014/11/19 20:50:18]

I'm sorry, but I'm totally lost now.

How did we go from "yes, Finnur's right and we need to convert an 'else if' into
an 'else'" to this solution?

Can you walk me through what is happening? Specifically, why do we _only_ remove
DISABLE_UNSUPPORTED_REQUIREMENT (lines 1656-8) when it is the _sole_ disable
reason left (line 1653-4)? That doesn't seem right.

[binjin, 2014/11/19 21:31:40]

I'm confused as well at this point. I think my solution is exactly "convert an
'else if' into an 'else'".

The behavior is slightly different from the original code but I think it's still
correct.

For ExtensionPrefs, "ClearDisableReasons(id);" clears all bits, and
"RemoveDisableReason(id, DISABLE_REASON);"  clear a single bits, they are
logically same.

For 'disable_reasons', the original code "disable_reasons =
Extension::DISABLE_NONE;" is overriding the previous result, but the modified
code have slightly different behavior. "disable_reasons &=
~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;" just discard the reason we are
sure not holding now. Both behavior works I think.

[Finnur, 2014/11/20 12:25:34]

Again, I think the confusion is probably my fault. I notice now that I did
attribute some weirdness in the code to your CL, when in fact it was part of the
original code. I'll make another comment elsewhere, I think that would be easier
to comprehend, rather than continue this thread.

+    extension_prefs_->RemoveDisableReason(
+        id, Extension::DISABLE_UNSUPPORTED_REQUIREMENT);
+    disable_reasons &= ~Extension::DISABLE_UNSUPPORTED_REQUIREMENT;
+  }
+
+  // Check if the extension was disabled because of the minimum version
+  // requirements from enterprise policy, and satisfies it now.
+  if (extensions::ExtensionManagementFactory::GetForBrowserContext(profile())
+          ->CheckMinimumVersionRequirement(extension, nullptr)) {
+    // And remove the corresponding disable reason.
+    extension_prefs_->RemoveDisableReason(
+        id, Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY);
+    disable_reasons &= ~Extension::DISABLE_UPDATE_REQUIRED_BY_POLICY;
   }
 
   if (install_flags & extensions::kInstallFlagIsBlacklistedForMalware) {