Start refactoring extension reloading to avoid redundant reloads.

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 796066f0502d8a40716f21b609d1043969ef73c8..8009ac102af02c731c4ad6dde9d5158f68b259a2 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -728,7 +728,7 @@ void ExtensionService::ReloadExtensionWithEvents(
 
     path = current_extension->path();
     DisableExtension(extension_id, Extension::DISABLE_RELOAD);
-    disabled_extension_paths_[extension_id] = path;
+    reloading_extensions_.insert(extension_id);
   } else {
     path = unloaded_extension_paths_[extension_id];
   }
@@ -1522,7 +1522,7 @@ bool ExtensionService::ProcessExtensionSyncDataHelper(
   // Set user settings.
   // If the extension has been disabled from sync, it may not have
   // been installed yet, so we don't know if the disable reason was a
-  // permissions increase.  That will be updated once InitializePermissions
+  // permissions increase.  That will be updated once CheckPermissionsIncrease
   // is called for it.
   if (extension_sync_data.enabled())
     EnableExtension(id);
@@ -1906,7 +1906,7 @@ void ExtensionService::UnloadExtension(
   unloaded_extension_paths_[extension->id()] = extension->path();
 
   // Clean up if the extension is meant to be enabled after a reload.
-  disabled_extension_paths_.erase(extension->id());
+  reloading_extensions_.erase(extension->id());
 
   // Clean up runtime data.
   extension_runtime_data_.erase(extension_id);
@@ -2034,7 +2034,16 @@ void ExtensionService::AddExtension(const Extension* extension) {
     return;
   }
 
-  SetBeingUpgraded(extension, false);
+  bool is_extension_upgrade = false;
+  if (const Extension* old = GetInstalledExtension(extension->id())) {
+    is_extension_upgrade = true;
+    DCHECK_NE(extension, old);
+    // Other than for unpacked extensions, CrxInstaller should have guaranteed
+    // that we aren't downgrading.
+    if (!Manifest::IsUnpackedLocation(extension->location()))
+      CHECK_GE(extension->version()->CompareTo(*(old->version())), 0);
+  }
+  SetBeingUpgraded(extension, is_extension_upgrade);
 
   // The extension is now loaded, remove its data from unloaded extension map.
   unloaded_extension_paths_.erase(extension->id());
@@ -2043,12 +2052,18 @@ void ExtensionService::AddExtension(const Extension* extension) {
   UntrackTerminatedExtension(extension->id());
 
   // If the extension was disabled for a reload, then enable it.
-  if (disabled_extension_paths_.erase(extension->id()) > 0)
+  if (reloading_extensions_.erase(extension->id()) > 0)
     EnableExtension(extension->id());

[Jeffrey Yasskin, 2013/04/25 01:53:37]

This Enable followed by Unload can cause an extra subprocess to be created and
destroyed. I'm planning to sanitize it in a subsequent change.

[Matt Perry, 2013/04/25 19:03:46]

Do you mean the Unload on line 2065? Can you just move this after that line?

[Jeffrey Yasskin, 2013/04/25 19:12:28]

Not immediately, that breaks tests. :) I'm figuring out how to move it in a
separate change.

 
-  // Check if the extension's privileges have changed and disable the
-  // extension if necessary.
-  InitializePermissions(extension);
+  // Check if the extension's privileges have changed and mark the
+  // extension disabled if necessary.
+  CheckPermissionsIncrease(extension, is_extension_upgrade);
+
+  if (is_extension_upgrade) {
+    // To upgrade an extension in place, unload the old one and
+    // then load the new one.
+    UnloadExtension(extension->id(), extension_misc::UNLOAD_REASON_UPDATE);
+  }
 
   if (extension_prefs_->IsExtensionBlacklisted(extension->id())) {
     // Only prefs is checked for the blacklist. We rely on callers to check the
@@ -2086,6 +2101,7 @@ void ExtensionService::AddExtension(const Extension* extension) {
     NotifyExtensionLoaded(extension);
     DoPostLoadTasks(extension);
   }
+  SetBeingUpgraded(extension, false);

[Jeffrey Yasskin, 2013/04/25 01:53:37]

This should be late enough to cover the uses inside BrowserActionsContainer.
BrowserActionAdded is called transitively through NotifyExtensionLoaded.

 }
 
 void ExtensionService::AddComponentExtension(const Extension* extension) {
@@ -2107,7 +2123,7 @@ void ExtensionService::AddComponentExtension(const Extension* extension) {
   AddExtension(extension);
 }
 
-void ExtensionService::InitializePermissions(const Extension* extension) {
+void ExtensionService::UpdateActivePermissions(const Extension* extension) {
   // If the extension has used the optional permissions API, it will have a
   // custom set of active permissions defined in the extension prefs. Here,
   // we update the extension's active permissions based on the prefs.
@@ -2136,6 +2152,11 @@ void ExtensionService::InitializePermissions(const Extension* extension) {
     extensions::PermissionsUpdater perms_updater(profile());
     perms_updater.UpdateActivePermissions(extension, adjusted_active);
   }
+}
+
+void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
+                                                bool is_extension_upgrade) {
+  UpdateActivePermissions(extension);
 
   // We keep track of all permissions the user has granted each extension.
   // This allows extensions to gracefully support backwards compatibility
@@ -2158,18 +2179,20 @@ void ExtensionService::InitializePermissions(const Extension* extension) {
   // still remember that "omnibox" had been granted, so that if the
   // extension once again includes "omnibox" in an upgrade, the extension
   // can upgrade without requiring this user's approval.
-  const Extension* old = GetInstalledExtension(extension->id());
-  bool is_extension_upgrade = old != NULL;
-  bool is_privilege_increase = false;
-  bool previously_disabled = false;
   int disable_reasons = extension_prefs_->GetDisableReasons(extension->id());
 
-  // We only need to compare the granted permissions to the current permissions
-  // if the extension is not allowed to silently increase its permissions.
   bool is_default_app_install =
       (!is_extension_upgrade && extension->was_installed_by_default());
-  if (!(extension->CanSilentlyIncreasePermissions()
-        || is_default_app_install)) {
+  // Silently grant all active permissions to default apps only on install.
+  // After install they should behave like other apps.
+  if (is_default_app_install)
+    GrantPermissions(extension);
+
+  bool is_privilege_increase = false;
+  // We only need to compare the granted permissions to the current permissions
+  // if the extension is not allowed to silently increase its permissions.
+  if (!(extension->CanSilentlyIncreasePermissions() ||
+        is_default_app_install)) {
     // Add all the recognized permissions if the granted permissions list

[Jeffrey Yasskin, 2013/04/25 01:53:37]

This isn't actually the effect of the next line, as far as I can tell. I'm
inclined just to remove the comment.

     // hasn't been initialized yet.
     scoped_refptr<PermissionSet> granted_permissions =
@@ -2185,31 +2208,15 @@ void ExtensionService::InitializePermissions(const Extension* extension) {
             extension->GetActivePermissions());
   }
 
-  // Silently grant all active permissions to default apps only on install.
-  // After install they should behave like other apps.
-  if (is_default_app_install)
-    GrantPermissions(extension);
-
   if (is_extension_upgrade) {
-    // Other than for unpacked extensions, CrxInstaller should have guaranteed
-    // that we aren't downgrading.
-    if (!Manifest::IsUnpackedLocation(extension->location()))
-      CHECK_GE(extension->version()->CompareTo(*(old->version())), 0);
-
-    // Extensions get upgraded if the privileges are allowed to increase or
-    // the privileges haven't increased.
-    if (!is_privilege_increase) {
-      SetBeingUpgraded(old, true);
-      SetBeingUpgraded(extension, true);
-    }
-
     // If the extension was already disabled, suppress any alerts for becoming
     // disabled on permissions increase.
-    previously_disabled = extension_prefs_->IsExtensionDisabled(old->id());
+    bool previously_disabled =
+        extension_prefs_->IsExtensionDisabled(extension->id());
     // Legacy disabled extensions do not have a disable reason. Infer that if
     // there was no permission increase, it was likely disabled by the user.
     if (previously_disabled && disable_reasons == Extension::DISABLE_NONE &&
-        !extension_prefs_->DidExtensionEscalatePermissions(old->id())) {
+        !extension_prefs_->DidExtensionEscalatePermissions(extension->id())) {
       disable_reasons |= Extension::DISABLE_USER_ACTION;
     }
     // Extensions that came to us disabled from sync need a similar inference,
@@ -2223,11 +2230,6 @@ void ExtensionService::InitializePermissions(const Extension* extension) {
     }
     if (disable_reasons != Extension::DISABLE_NONE)
       disable_reasons &= ~Extension::DISABLE_UNKNOWN_FROM_SYNC;

[Jeffrey Yasskin, 2013/04/25 01:53:37]

This has no effect unless the ClearDisableReasons line was executed.  The "if"
is also pointless since clearing a bit has no effect on a DISABLE_NONE reason.

[Yoyo Zhou, 2013/04/25 19:21:58]

This logic is quite complicated to compensate for not syncing disable reasons...

If we're UNKNOWN_FROM_SYNC and there is no privilege increase, it should be
USER_ACTION (2228). Otherwise it should be PERMISSIONS_INCREASE (2237). In any
case we want to clear the UNKNOWN_FROM_SYNC bit.

But you're right that these 2 lines seem to do nothing.

There also seems to be a bug here where we don't actually AddDisableReason for
the new disable reasons.

My vote for the fix is to pull 2243 and 2245 out of the if
(is_privilege_increase) and put them into an if (disable_reasons !=
Extension::DISABLE_NONE).

[Jeffrey Yasskin, 2013/04/26 00:52:44]

Moving SetExtensionState broke a ManagementPolicy test, apparently because
management policy resets the enabled state without removing any disable reason,
and runs before the permission check. We should try to figure that out in a
separate change, because I'm worried it means that management policy is
overridden by permission increases.

-
-    // To upgrade an extension in place, unload the old one and
-    // then load the new one.
-    UnloadExtension(old->id(), extension_misc::UNLOAD_REASON_UPDATE);
-    old = NULL;
   }
 
   // Extension has changed permissions significantly. Disable it. A
@@ -2781,7 +2783,7 @@ bool ExtensionService::IsBeingUpgraded(const Extension* extension) const {
 }
 
 void ExtensionService::SetBeingUpgraded(const Extension* extension,
-                                         bool value) {
+                                        bool value) {
   extension_runtime_data_[extension->id()].being_upgraded = value;
 }