Temporarily disable extensions and sync while a profile is locked - Profiles Approach

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 d8e601beb50ac115f6528daf73bf231b02b4ae96..7cf147dec19c9f19e8cb0b786c2b9aba24dcda3e 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -270,6 +270,7 @@ ExtensionService::ExtensionService(Profile* profile,
       browser_terminating_(false),
       installs_delayed_for_gc_(false),
       is_first_run_(false),
+      block_extensions_(false),
       shared_module_service_(new extensions::SharedModuleService(profile_)) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -374,11 +375,12 @@ const Extension* ExtensionService::GetExtensionById(
     const std::string& id, bool include_disabled) const {
   int include_mask = ExtensionRegistry::ENABLED;
   if (include_disabled) {
-    // Include blacklisted extensions here because there are hundreds of
-    // callers of this function, and many might assume that this includes those
-    // that have been disabled due to blacklisting.
+    // Include blacklisted and blocked extensions here because there are
+    // hundreds of callers of this function, and many might assume that this
+    // includes those that have been disabled due to blacklisting or blocking.
     include_mask |= ExtensionRegistry::DISABLED |
-                    ExtensionRegistry::BLACKLISTED;
+                    ExtensionRegistry::BLACKLISTED |
+                    ExtensionRegistry::BLOCKED;
   }
   return registry_->GetExtensionById(id, include_mask);
 }
@@ -579,12 +581,14 @@ void ExtensionService::ReloadExtensionImpl(
     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))
+  // Ignore attempts to reload a blacklisted or blocked extension. Sometimes
+  // this can happen in a convoluted reload sequence triggered by the
+  // termination of a blacklisted or blocked extension and a naive attempt to
+  // reload it. For an example see http://crbug.com/373842.
+  if (registry_->blacklisted_extensions().Contains(transient_extension_id) ||
+      registry_->blocked_extensions().Contains(transient_extension_id)) {
     return;
+  }
 
   base::FilePath path;
 
@@ -789,7 +793,8 @@ bool ExtensionService::IsExtensionEnabled(
   }
 
   if (registry_->disabled_extensions().Contains(extension_id) ||
-      registry_->blacklisted_extensions().Contains(extension_id)) {
+      registry_->blacklisted_extensions().Contains(extension_id) ||
+      registry_->blocked_extensions().Contains(extension_id)) {
     return false;
   }
 
@@ -797,6 +802,7 @@ bool ExtensionService::IsExtensionEnabled(
   // enabled unless otherwise noted.
   return !extension_prefs_->IsExtensionDisabled(extension_id) &&
          !extension_prefs_->IsExtensionBlacklisted(extension_id) &&
+         !extension_prefs_->IsExtensionBlocked(extension_id) &&
          !extension_prefs_->IsExternalExtensionUninstalled(extension_id);
 }
 
@@ -920,6 +926,63 @@ void ExtensionService::DisableUserExtensions(
   }
 }
 
+// Extensions that are not locked, components or forced by policy should be
+// locked. Extensions are no longer considered enabled or disabled. Blacklisted
+// extensions are now considered both blacklisted and locked.
+void ExtensionService::BlockAllExtensions() {

[not at google - send to devlin, 2014/11/12 23:41:43]

Oh, you might want to check that it's not already blocked and/or there are no
extensions in the current block list?

[Mike Lerman, 2014/11/13 15:30:05]

There's no reason this method couldn't be called multiple times. It logically
won't the way the profiles code is implemented to call this, and shouldn't have
any effect with the safeguard that's added to AddExtension(), but I see no harm
in leaving it possible to be called multiple times.

If we add a DCHECK or if()-return , I see some future developer looking over
this code, needing to call it multiple times and saying, "But WHY was this
prevented? What edge-case was the author preventing?"

+  block_extensions_ = true;
+  scoped_ptr<ExtensionSet> extensions =
+      registry_->GenerateInstalledExtensionsSet();
+
+  ExtensionIdSet to_lock;

[not at google - send to devlin, 2014/11/12 23:41:43]

You can delete this now.

[Mike Lerman, 2014/11/13 15:30:05]

Bye bye!

+  for (const scoped_refptr<const Extension> extension : *extensions.get()) {
+    const std::string& id = extension.get()->id();

[not at google - send to devlin, 2014/11/12 23:41:43]

just extension->id();

[Mike Lerman, 2014/11/13 15:30:04]

Done.

+
+    if (!CanBlockExtension(extension.get()))
+      continue;
+
+    // Blacklisted extensions remain in the blacklisted list.
+    registry_->RemoveEnabled(id);
+    registry_->RemoveDisabled(id);
+    registry_->RemoveTerminated(id);
+

[not at google - send to devlin, 2014/11/12 23:41:43]

Come to think of it it's probably also wise/safer not to have an Extension in
both the blacklisted and blocked list, just to be safe.

Hey here's an idea: how about adding a 2nd version of
GenerateInstallExtensionsSet which takes an IncludeMask, then only include
non-blacklisted extensions in here in the first place. Then you don't have to
worry about that at all.

[Mike Lerman, 2014/11/13 15:30:05]

I'm concerned about removing things from the blacklist - it seems very important
that blacklisted things stay that way, and I'd hate for a bug here to somehow
result in something escaping the blacklist.

Also, if I remove it from the blacklist, I need a foolproof way to put it back
there in UnblockAllBlockedExtensions(), and I can't find a way to determine
that.

[not at google - send to devlin, 2014/11/13 18:09:47]

I'm not suggesting removing it from the blacklist. I'm suggesting that you
exclude the blacklisted extensions from ever actually entering the installed
extensions list by the method I mentioned above.

[Mike Lerman, 2014/11/13 21:42:32]

Done.

+    registry_->AddBlocked(extension.get());
+    extension_prefs_->SetExtensionState(id, Extension::BLOCKED);

[not at google - send to devlin, 2014/11/12 23:41:43]

Actually - why do you need to change prefs at all? Blocking is a global state,
you shouldn't need to change the extension *prefs*.

[Mike Lerman, 2014/11/13 15:30:04]

The goal was the keep the ExtensionState sync-ed with the ExtensionRegistry -
having a state of DISABLED but not being in the disabled_extensions() seems like
a problem waiting to happen. So I need to set the state to something, and made a
new one.

[not at google - send to devlin, 2014/11/13 18:09:47]

I don't think so. For example we keep terminated extensions in their
enabled/disabled prefs. It will simplify things considerably if you don't make
any changes to the prefs.

[Mike Lerman, 2014/11/13 21:42:32]

Done.

+    UnloadExtension(
+        id, extensions::UnloadedExtensionInfo::REASON_LOCK_ALL);
+  }
+}
+
+// All locked extensions should revert to being either enabled or disabled
+// as appropriate.
+void ExtensionService::UnblockAllBlockedExtensions() {
+  ExtensionIdSet to_unblock = registry_->blocked_extensions().GetIDs();

[not at google - send to devlin, 2014/11/12 23:41:43]

(see previous point about just copying the whole extension set)

[Mike Lerman, 2014/11/13 15:30:04]

Which one?

Is making a copy of the set better than just referring to the IDs? Also, using
the IDs is consistent with the implementations of similar loops in
UpdateBlockedExtensions and UpdateGreylistedExtensions.

[not at google - send to devlin, 2014/11/13 18:09:47]

Maybe they should copy the set as well. Needing 7 lines to deal with fetching
the extension back out of the list again is avoidable work (my point being
confusing to read rather than computationally efficient, though it probably is
also more efficient).

[Mike Lerman, 2014/11/13 21:42:32]

Copied as well. Done.

+
+  block_extensions_ = false;
+  for (ExtensionIdSet::iterator it = to_unblock.begin();

[not at google - send to devlin, 2014/11/12 23:41:43]

(and use : syntax here too)

[Mike Lerman, 2014/11/13 15:30:04]

Done.

+       it != to_unblock.end(); ++it) {
+    scoped_refptr<const Extension> extension = GetInstalledExtension(*it);
+    if (!extension.get()) {
+      NOTREACHED() << "Extension " << *it << " needs to be "
+                   << "unblocked, but it's not installed.";
+      continue;
+    }
+
+    registry_->RemoveBlocked(*it);
+    if (extensions_being_terminated_.find(extension->id()) !=

[not at google - send to devlin, 2014/11/12 23:41:43]

Mm, maybe this isn't actually useful. I doubt this set actually includes
anything in it anymore. Perhaps just re-enabling terminated extensions would be
fine, the important part is more just blocking them in the first place.

[Mike Lerman, 2014/11/13 15:30:05]

Done.

+               extensions_being_terminated_.end()) {
+      registry_->AddTerminated(extension.get());
+    } else {
+      if (extension_prefs_->GetDisableReasons(*it)) {

[not at google - send to devlin, 2014/11/12 23:41:43]

GetDisableReasons returns a bitmask, so it's a little confusing doing if
(GetDisableReasons). prefs_->IsExtensionDisabled(..) might be more appropriate.

However, see comment above.

[Mike Lerman, 2014/11/13 15:30:04]

I agree about using IsExtensionDisabled, if we have the state. Pending comment
above resolution :)

+        extension_prefs_->SetExtensionState(*it, Extension::DISABLED);
+      } else {
+        extension_prefs_->SetExtensionState(*it, Extension::ENABLED);
+      }
+      AddExtension(extension.get());
+    }
+  }
+}
+
 void ExtensionService::GrantPermissionsAndEnableExtension(
     const Extension* extension) {
   GrantPermissions(extension);
@@ -1373,6 +1436,9 @@ void ExtensionService::AddExtension(const Extension* extension) {
     // installation then threads through the install and pending install flow
     // of this class, and we check when loading installed extensions.
     registry_->AddBlacklisted(extension);
+  } else if (extension_prefs_->IsExtensionBlocked(extension->id()) ||
+             (block_extensions_ && CanBlockExtension(extension))) {
+    registry_->AddBlocked(extension);
   } else if (!reloading &&
              extension_prefs_->IsExtensionDisabled(extension->id())) {
     registry_->AddDisabled(extension);
@@ -2175,6 +2241,13 @@ bool ExtensionService::ShouldEnableOnInstall(const Extension* extension) {
   return true;
 }
 
+// Helper method to determine if an extension can be blocked.
+bool ExtensionService::CanBlockExtension(const Extension* extension) {
+  return (extension->location() != Manifest::COMPONENT &&

[not at google - send to devlin, 2014/11/12 23:41:43]

Outer parens are unnecessary.

[Mike Lerman, 2014/11/13 15:30:04]

Done.

+      extension->location() != Manifest::EXTERNAL_COMPONENT &&
+      !system_->management_policy()->MustRemainEnabled(extension, NULL));
+}
+
 bool ExtensionService::ShouldDelayExtensionUpdate(
     const std::string& extension_id,
     bool install_immediately) const {