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..9fcf18ad20bb6e4dc05a2d56dff6d3eec8cc3a52 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -378,7 +378,8 @@ const Extension* ExtensionService::GetExtensionById(
     // callers of this function, and many might assume that this includes those
     // that have been disabled due to blacklisting.

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

Update comment?

[Mike Lerman, 2014/11/12 20:58:42]

Done.

     include_mask |= ExtensionRegistry::DISABLED |
-                    ExtensionRegistry::BLACKLISTED;
+                    ExtensionRegistry::BLACKLISTED |
+                    ExtensionRegistry::LOCKED;
   }
   return registry_->GetExtensionById(id, include_mask);
 }
@@ -579,12 +580,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 locked extension. Sometimes this

[not at google - send to devlin, 2014/11/12 18:22:59]

Also update this comment.

[Mike Lerman, 2014/11/12 20:58:41]

Ah, I'd only done half! Thanks. Done.

+  // 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) ||
+      registry_->locked_extensions().Contains(transient_extension_id)) {
     return;
+  }
 
   base::FilePath path;
 
@@ -789,7 +792,8 @@ bool ExtensionService::IsExtensionEnabled(
   }
 
   if (registry_->disabled_extensions().Contains(extension_id) ||
-      registry_->blacklisted_extensions().Contains(extension_id)) {
+      registry_->blacklisted_extensions().Contains(extension_id) ||
+      registry_->locked_extensions().Contains(extension_id)) {
     return false;
   }
 
@@ -797,6 +801,7 @@ bool ExtensionService::IsExtensionEnabled(
   // enabled unless otherwise noted.
   return !extension_prefs_->IsExtensionDisabled(extension_id) &&
          !extension_prefs_->IsExtensionBlacklisted(extension_id) &&
+         !extension_prefs_->IsExtensionLocked(extension_id) &&
          !extension_prefs_->IsExternalExtensionUninstalled(extension_id);
 }
 
@@ -920,6 +925,77 @@ void ExtensionService::DisableUserExtensions(
   }
 }
 
+

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

Remove redundant new line.

[Mike Lerman, 2014/11/12 20:58:42]

Done.

+// Extensions specified by |extension_ids| should become locked. Extensions
+// are no longer considered enabled or disabled. Blacklisted extensions are
+// now considered both blacklisted and locked.
+void ExtensionService::LockAllExtensions() {
+  scoped_ptr<ExtensionSet> extensions =
+      registry_->GenerateInstalledExtensionsSet();
+
+  ExtensionIdSet to_lock;
+  for (ExtensionSet::const_iterator extension = extensions->begin();

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

You could use the new : foreach syntax?

[Mike Lerman, 2014/11/12 20:58:42]

Done.

+       extension != extensions->end();
+       ++extension) {
+    const std::string& id = (*extension)->id();
+    if (!registry_->locked_extensions().Contains(id))
+      to_lock.insert(id);
+  }
+
+  for (ExtensionIdSet::iterator id = to_lock.begin();
+       id != to_lock.end(); ++id) {
+    scoped_refptr<const Extension> extension = GetInstalledExtension(*id);

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

Unless something is very wrong, this shouldn't be possible. I don't think it's
necessary to store IDs separately and loop over the extensions, though. The
installed extension set is a copy, and will hold on to references to the
Extensions, so you can just directly loop over that.

[Mike Lerman, 2014/11/12 20:58:42]

That saves a lot of code. Thanks!

+    if (!extension.get()) {
+      NOTREACHED() << "Extension " << *id << " needs to be "
+                   << "locked, but it's not installed.";
+      continue;
+    }
+
+    // Don't lock core components.
+    if (extension.get()->location() == Manifest::COMPONENT ||
+        extension.get()->location() == Manifest::EXTERNAL_COMPONENT) {

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

Hm, it's surprising that this is the first time we've needed to check this
(component + external component + policy) condition. Perhaps add a method like
CannotBeDisabled, and we can justify its addition by DCHECKing in methods like
DisableExtension that CannotBeDisabled is false.

[Mike Lerman, 2014/11/12 20:58:42]

It seems like this DCHECK in DisableExtension wouldn't be wise, according to
what I see here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext....

Created the refactored method anywho, and called it to ensure we handle
restoration of terminated extensions properly.

+      continue;
+    }
+
+    if (system_->management_policy()->MustRemainEnabled(extension.get(), NULL))
+      continue;
+
+    // Blacklisted extensions remain in the blacklisted list.

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

What about terminated extensions? Those need to go in the locked list as well.

[Mike Lerman, 2014/11/12 20:58:42]

I had assumed that a terminated extension was A) A separated bucket like
blacklist that things don't move out of very often and B) was something that
won't start up, since it's terminated... no?

Anywho - Done.

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

You can restart a terminated extension by going to chrome://extensions and
hitting "reload" on a terminated extension, and you can terminate an extension
by killing in the process manager.

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

Acknowledged.

+    if (registry_->enabled_extensions().Contains(*id)) {
+      registry_->RemoveEnabled(*id);
+    } else if (registry_->disabled_extensions().Contains(*id)) {
+      registry_->RemoveDisabled(*id);
+    }

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

I don't actually think the "if"s here are necessary, Remove just won't do
anything if the ID isn't there. Just:

registry_->RemoveEnabled(id);
registry_->RemoveDisabled(id);
registry_->RemoveTerminated(id);
// Important not to remove Blacklisted extensions!

[Mike Lerman, 2014/11/12 20:58:42]

Done.

+    registry_->AddLocked(extension.get());
+    extension_prefs_->SetExtensionState(*id, Extension::LOCKED);
+    UnloadExtension(
+        *id, extensions::UnloadedExtensionInfo::REASON_PROFILE_LOCK);
+  }
+}
+
+// All locked extensions should revert to being either enabled or disabled
+// as appropriate.
+void ExtensionService::UnlockAllLockedExtensions() {
+  ExtensionIdSet to_unlock = registry_->locked_extensions().GetIDs();
+
+  for (ExtensionIdSet::iterator it = to_unlock.begin();
+       it != to_unlock.end(); ++it) {
+    scoped_refptr<const Extension> extension = GetInstalledExtension(*it);
+    if (!extension.get()) {
+      NOTREACHED() << "Extension " << *it << " needs to be "

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

Again; not possible, and this code is perfectly safe (given you're taking a
reference to |extension|) to directly loop over the locked extensions and not
need to extract the IDs.

[Mike Lerman, 2014/11/12 20:58:42]

Really? I can't loop directory of registry->locked_extensions(), given I'm
calling RemoveLocked() within the loop.

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

Oh right, good point, I guess you need to take a copy of locked extensions.

+                   << "unlocked, but it's not installed.";
+      continue;
+    }
+
+    registry_->RemoveLocked(*it);
+    if (extension_prefs_->GetDisableReasons(*it))
+      extension_prefs_->SetExtensionState(*it, Extension::DISABLED);
+    else
+      extension_prefs_->SetExtensionState(*it, Extension::ENABLED);

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

Again: terminated extensions.

[Mike Lerman, 2014/11/12 20:58:42]

Done.

+    AddExtension(extension.get());
+  }
+}
+
 void ExtensionService::GrantPermissionsAndEnableExtension(
     const Extension* extension) {
   GrantPermissions(extension);
@@ -1373,6 +1449,8 @@ 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_->IsExtensionLocked(extension->id())) {
+    registry_->AddLocked(extension);
   } else if (!reloading &&
              extension_prefs_->IsExtensionDisabled(extension->id())) {
     registry_->AddDisabled(extension);