Add minimum version to extension management

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 835 matching lines @@
   }
 
   const Extension* extension = GetInstalledExtension(extension_id);
   // |extension| can be NULL if sync disables an extension that is not
   // installed yet.
   // EXTERNAL_COMPONENT extensions are not generally modifiable by users, but
   // can be uninstalled by the browser if the user sets extension-specific
   // 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;
   }
 
   extension_prefs_->SetExtensionState(extension_id, Extension::DISABLED);
   extension_prefs_->AddDisableReason(extension_id, disable_reason);
 
   int include_mask =
       ExtensionRegistry::EVERYTHING & ~ExtensionRegistry::DISABLED;
@@ skipping 238 matching lines @@
   file_task_runner_ = BrowserThread::GetBlockingPool()->
       GetSequencedTaskRunnerWithShutdownBehavior(
         BrowserThread::GetBlockingPool()->GetNamedSequenceToken(token),
         base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   return file_task_runner_.get();
 }
 
 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() {
   // This can legitimately happen in unit tests.
   if (!updater_.get())
     return;
 
   if (AreAllExternalProvidersReady()) {
     updater_->CheckSoon();
   } else {
@@ skipping 455 matching lines @@
     // extension; if we're here, that means the user is manually
     // installing the extension.
     if (extension_prefs_->IsExternalExtensionUninstalled(id)) {
       disable_reasons = Extension::DISABLE_NONE;
     }
   }
 
   // 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) {
     // Installation of a blacklisted extension can happen from sync, policy,
     // etc, where to maintain consistency we need to install it, just never
     // load it (see AddExtension). Usually it should be the job of callers to
     // incercept blacklisted extension earlier (e.g. CrxInstaller, before even
     // showing the install dialogue).
     extension_prefs_->AcknowledgeBlacklistedExtension(id);
     UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.SilentInstall",
@@ skipping 778 matching lines @@
 }
 
 void ExtensionService::OnProfileDestructionStarted() {
   ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs();
   for (ExtensionIdSet::iterator it = ids_to_unload.begin();
        it != ids_to_unload.end();
        ++it) {
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN);
   }
 }