Start refactoring extension reloading to avoid redundant reloads.

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 710 matching lines @@
     if (host && DevToolsAgentHost::HasFor(host->render_view_host())) {
       // Look for an open inspector for the background page.
       std::string devtools_cookie = DevToolsAgentHost::DisconnectRenderViewHost(
           host->render_view_host());
       if (devtools_cookie != std::string())
         orphaned_dev_tools_[extension_id] = devtools_cookie;
     }
 
     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];
   }
 
   on_load_events_[extension_id] = events;
   if (events & EVENT_RESTARTED) {
     extension_prefs_->GetSavedFileEntries(
         extension_id, &on_restart_file_entries_[extension_id]);
   }
 
@@ skipping 773 matching lines @@
   // Honor user choice and skip installation/enabling.
   if (IsExternalExtensionUninstalled(id)) {
     LOG(WARNING) << "Extension with id " << id
                  << " from sync was uninstalled as external extension";
     return true;
   }
 
   // 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);
   else
     DisableExtension(id, Extension::DISABLE_UNKNOWN_FROM_SYNC);
 
   // We need to cache some version information here because setting the
   // incognito flag invalidates the |extension| pointer (it reloads the
   // extension).
   bool extension_installed = (extension != NULL);
@@ skipping 363 matching lines @@
     // clean up its RequestContexts.
     system_->UnregisterExtensionWithRequestContexts(extension_id, reason);
     return;
   }
 
   // Keep information about the extension so that we can reload it later
   // even if it's not permanently installed.
   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);
 
   if (disabled_extensions_.Contains(extension->id())) {
     UnloadedExtensionInfo details(extension, reason);
     details.already_disabled = true;
     disabled_extensions_.Remove(extension->id());
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_EXTENSION_UNLOADED,
@@ skipping 107 matching lines @@
   // TODO(jstritar): We may be able to get rid of this branch by overriding the
   // default extension state to DISABLED when the --disable-extensions flag
   // is set (http://crbug.com/29067).
   if (!extensions_enabled() &&
       !extension->is_theme() &&
       extension->location() != Manifest::COMPONENT &&
       !Manifest::IsExternalLocation(extension->location())) {
     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());
 
   // If a terminated extension is loaded, remove it from the terminated list.
   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
     // blacklist before calling into here, e.g. CrxInstaller checks before
     // installation, we check when loading installed extensions.
     blacklisted_extensions_.Insert(extension);
   } else if (extension_prefs_->IsExtensionDisabled(extension->id())) {
     disabled_extensions_.Insert(extension);
     SyncExtensionChangeIfNeeded(*extension);
     content::NotificationService::current()->Notify(
@@ skipping 17 matching lines @@
       }
       extension_prefs_->extension_sorting()->EnsureValidOrdinals(
           extension->id(), syncer::StringOrdinal());
     }
 
     extensions_.Insert(extension);
     SyncExtensionChangeIfNeeded(*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) {
   const std::string old_version_string(
       extension_prefs_->GetVersionString(extension->id()));
   const Version old_version(old_version_string);
 
   if (!old_version.IsValid() || !old_version.Equals(*extension->version())) {
     VLOG(1) << "Component extension " << extension->name() << " ("
         << extension->id() << ") installing/upgrading from '"
         << old_version_string << "' to " << extension->version()->GetString();
 
     AddNewOrUpdatedExtension(extension,
                              Extension::ENABLED_COMPONENT,
                              syncer::StringOrdinal());
     return;
   }
 
   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.
   scoped_refptr<PermissionSet> active_permissions =
       extension_prefs()->GetActivePermissions(extension->id());
 
   if (active_permissions) {
     // We restrict the active permissions to be within the bounds defined in the
     // extension's manifest.
     //  a) active permissions must be a subset of optional + default permissions
     //  b) active permissions must contains all default permissions
     scoped_refptr<PermissionSet> total_permissions =
         PermissionSet::CreateUnion(
             extension->required_permission_set(),
             extension->optional_permission_set());
 
     // Make sure the active permissions contain no more than optional + default.
     scoped_refptr<PermissionSet> adjusted_active =
         PermissionSet::CreateIntersection(
             total_permissions.get(), active_permissions.get());
 
     // Make sure the active permissions contain the default permissions.
     adjusted_active = PermissionSet::CreateUnion(
             extension->required_permission_set(), adjusted_active.get());
 
     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
   // by including unknown permissions in their manifests. When the user
   // installs the extension, only the recognized permissions are recorded.
   // When the unknown permissions become recognized (e.g., through browser
   // upgrade), we can prompt the user to accept these new permissions.
   // Extensions can also silently upgrade to less permissions, and then
   // silently upgrade to a version that adds these permissions back.
   //
   // For example, pretend that Chrome 10 includes a permission "omnibox"
   // for an API that adds suggestions to the omnibox. An extension can
   // maintain backwards compatibility while still having "omnibox" in the
   // manifest. If a user installs the extension on Chrome 9, the browser
   // will record the permissions it recognized, not including "omnibox."
   // When upgrading to Chrome 10, "omnibox" will be recognized and Chrome
   // will disable the extension and prompt the user to approve the increase
   // in privileges. The extension could then release a new version that
   // removes the "omnibox" permission. When the user upgrades, Chrome will
   // 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());
 
+  bool is_default_app_install =
+      (!is_extension_upgrade && extension->was_installed_by_default());
+  // 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.
-  bool is_default_app_install =
-      (!is_extension_upgrade && extension->was_installed_by_default());
-  if (!(extension->CanSilentlyIncreasePermissions()
-        || is_default_app_install)) {
+  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 =
         extension_prefs_->GetGrantedPermissions(extension->id());
     CHECK(granted_permissions.get());
 
     // Here, we check if an extension's privileges have increased in a manner
     // that requires the user's approval. This could occur because the browser
     // upgraded and recognized additional privileges, or an extension upgrades
     // to a version that requires additional privileges.
     is_privilege_increase =
         granted_permissions->HasLessPrivilegesThan(
             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,
     // except based on the new version's permissions.
     if (previously_disabled &&
         disable_reasons == Extension::DISABLE_UNKNOWN_FROM_SYNC) {
       // Remove the DISABLE_UNKNOWN_FROM_SYNC reason.
       extension_prefs_->ClearDisableReasons(extension->id());

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

If there's no privilege increase, this will drop all the disable reasons from
the extension, even though the code looks like it wants to set
DISABLE_USER_ACTION. Thoughts on the right fix?

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

Seems harmless here, though, because disable_reasons ==
Extension::DISABLE_UNKNOWN_FROM_SYNC. We want to clear that anyway.

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

We want to clear that and add DISABLE_USER_ACTION. Without a privilege increase,
we lose the DISABLE_USER_ACTION.

[Matt Perry, 2013/04/25 19:22:43]

We only add DISABLE_USER_ACTION if it's *not* a privilege increase. That's
because if it was a privilege increase, we can't assume the user disabled it
explicitly. It may have been disabled due to an update.

[Yoyo Zhou, 2013/04/25 19:24:39]

I think we (I, specifically) forgot to actually add DISABLE_USER_ACTION.

[Matt Perry, 2013/04/25 19:29:55]

Oh right, because we only AddDisableReason if it's a privilege increase... oops.

       if (!is_privilege_increase)
         disable_reasons |= Extension::DISABLE_USER_ACTION;
     }
     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
   // notification should be sent by the caller.
   if (is_privilege_increase) {
     disable_reasons |= Extension::DISABLE_PERMISSIONS_INCREASE;
     if (!extension_prefs_->DidExtensionEscalatePermissions(extension->id())) {
       RecordPermissionMessagesHistogram(
           extension, "Extensions.Permissions_AutoDisable");
     }
@@ skipping 533 matching lines @@
 }
 
 bool ExtensionService::IsBeingUpgraded(const Extension* extension) const {
   ExtensionRuntimeDataMap::const_iterator it =
       extension_runtime_data_.find(extension->id());
   return it == extension_runtime_data_.end() ? false :
                                                it->second.being_upgraded;
 }
 
 void ExtensionService::SetBeingUpgraded(const Extension* extension,
-                                         bool value) {
+                                        bool value) {
   extension_runtime_data_[extension->id()].being_upgraded = value;
 }
 
 bool ExtensionService::HasUsedWebRequest(const Extension* extension) const {
   ExtensionRuntimeDataMap::const_iterator it =
       extension_runtime_data_.find(extension->id());
   return it == extension_runtime_data_.end() ? false :
                                                it->second.has_used_webrequest;
 }
 
@@ skipping 295 matching lines @@
 }
 
 void ExtensionService::AddUpdateObserver(extensions::UpdateObserver* observer) {
   update_observers_.AddObserver(observer);
 }
 
 void ExtensionService::RemoveUpdateObserver(
     extensions::UpdateObserver* observer) {
   update_observers_.RemoveObserver(observer);
 }