Re-enable extensions disabled due to permission increase if they have all permissions

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 271ca2e13b1c0fdf6e0cda588b16397b36590e55..dbee8232a626310111c01e9c250e4f100fb93da5 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1611,14 +1611,12 @@ void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
       GrantPermissions(extension);
   }
 
-  if (is_extension_installed) {
-    // If the extension was already disabled, suppress any alerts for becoming
-    // disabled on permissions increase.
-    bool previously_disabled =
-        extension_prefs_->IsExtensionDisabled(extension->id());
+  bool previously_disabled =
+      extension_prefs_->IsExtensionDisabled(extension->id());
+  if (is_extension_installed && previously_disabled) {
     // Legacy disabled extensions do not have a disable reason. Infer that it
     // was likely disabled by the user.
-    if (previously_disabled && disable_reasons == Extension::DISABLE_NONE)
+    if (disable_reasons == Extension::DISABLE_NONE)
       disable_reasons |= Extension::DISABLE_USER_ACTION;
 
     // Extensions that came to us disabled from sync need a similar inference,
@@ -1626,8 +1624,7 @@ void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
     // TODO(treib,devlin): Since M48, DISABLE_UNKNOWN_FROM_SYNC isn't used
     // anymore; this code is still here to migrate any existing old state.
     // Remove it after some grace period.
-    if (previously_disabled &&
-        (disable_reasons & Extension::DEPRECATED_DISABLE_UNKNOWN_FROM_SYNC)) {
+    if (disable_reasons & Extension::DEPRECATED_DISABLE_UNKNOWN_FROM_SYNC) {
       // Remove the DISABLE_UNKNOWN_FROM_SYNC reason.
       disable_reasons &= ~Extension::DEPRECATED_DISABLE_UNKNOWN_FROM_SYNC;
       extension_prefs_->RemoveDisableReason(
@@ -1638,6 +1635,15 @@ void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
     }
   }
 
+  // If the extension is disabled due to a permissions increase, but does in
+  // fact have all permissions, remove that disable reason.

[Marc Treib, 2016/06/10 14:12:17]

To make the "startup" case work, I had to pull this out of the "if
(is_extension_installed)" - turns out |is_extension_installed| is false during
startup, even if the extension was previously installed. Misleading names ftw
:-/
In fact, I can't see a reason why this check is even there at all. Removing it
doesn't seem to break anything, though from experience, that doesn't mean
much...

[Devlin, 2016/06/10 19:05:52]

Can you add a TODO to rename it on line 1442?  Something like:
// TODO([devlin|treib]): This is incorrect during startup, where the extension
isn't already loaded, but may have been installed.

(option b: rename it in this CL - up to you ;))

[Marc Treib, 2016/06/13 11:54:59]

Done.
I also added a TODO in CheckPermissionsIncrease, (a) to explain the same thing,
a (2) since I think we don't actually need it there.
Sure, I can do that - what would be a good name? is_extension_loaded?

[Devlin, 2016/06/13 13:51:50]

is_extension_loaded sgtm

[Marc Treib, 2016/06/14 15:24:26]

Done, and removed/updated the TODOs accordingly.

+  if (previously_disabled && !is_privilege_increase &&

[Devlin, 2016/06/10 19:05:52]

Add:
// TODO(devlin): This was added to fix crbug.com/616474, but it's unclear
// if this behavior should stay forever.

[Marc Treib, 2016/06/13 11:54:59]

Done.

+      (disable_reasons & Extension::DISABLE_PERMISSIONS_INCREASE)) {
+    disable_reasons &= ~Extension::DISABLE_PERMISSIONS_INCREASE;
+    extension_prefs_->RemoveDisableReason(
+        extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE);

[Devlin, 2016/06/10 19:05:52]

I wonder if we should add some UMA for this.  I'm expecting this to be
relatively rare, but if we see ourselves re-enabling bunches of extensions, it'd
at least be good to know.

[Marc Treib, 2016/06/13 11:54:59]

Added a binary histogram. PTAL.

+  }
+
   // Extension has changed permissions significantly. Disable it. A
   // notification should be sent by the caller. If the extension is already
   // disabled because it was installed remotely, don't add another disable
@@ -1664,7 +1670,10 @@ void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
     }
 #endif
   }
-  if (disable_reasons != Extension::DISABLE_NONE)
+
+  if (disable_reasons == Extension::DISABLE_NONE)
+    extension_prefs_->SetExtensionEnabled(extension->id());
+  else
     extension_prefs_->SetExtensionDisabled(extension->id(), disable_reasons);
 }