Load extensions service entirly on UI thread.
BUG=37548
TEST=SxtensionsServiceTest.LoadAllExtensionsFromDirectorySuccess,ExtensionsStartupTest.Test,manual testing of example extension from crbug/47286
Review URL: http://codereview.chromium.org/4752005
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@66267 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 17ea2e6..05c6768 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -15,6 +15,7 @@
#include "base/string_number_conversions.h"
#include "base/string_util.h"
#include "base/stringprintf.h"
+#include "base/thread_restrictions.h"
#include "base/time.h"
#include "base/utf_string_conversions.h"
#include "base/values.h"
@@ -55,6 +56,7 @@
#include "chrome/common/extensions/extension_error_utils.h"
#include "chrome/common/extensions/extension_file_util.h"
#include "chrome/common/extensions/extension_l10n_util.h"
+#include "chrome/common/extensions/extension_resource.h"
#include "chrome/common/notification_service.h"
#include "chrome/common/notification_type.h"
#include "chrome/common/json_value_serializer.h"
@@ -86,14 +88,27 @@
static const int kOmniboxIconPaddingRight = 0;
#endif
-bool ShouldReloadExtensionManifest(const ExtensionInfo& info) {
- // Always reload LOAD extension manifests, because they can change on disk
- // independent of the manifest in our prefs.
- if (info.extension_location == Extension::LOAD)
- return true;
+// The following enumeration is used in histograms matching
+// Extensions.ManifestReload* . Values may be added, as long
+// as existing values are not changed.
+enum ManifestReloadReason {
+ NOT_NEEDED = 0, // Reload not needed.
+ UNPACKED_DIR, // Unpacked directory
+ NEEDS_RELOCALIZATION, // The local has changed since we read this extension.
+ NUM_MANIFEST_RELOAD_REASONS
+};
- // Otherwise, reload the manifest it needs to be relocalized.
- return extension_l10n_util::ShouldRelocalizeManifest(info);
+ManifestReloadReason ShouldReloadExtensionManifest(const ExtensionInfo& info) {
+ // Always reload manifests of unpacked extensions, because they can change
+ // on disk independent of the manifest in our prefs.
+ if (info.extension_location == Extension::LOAD)
+ return UNPACKED_DIR;
+
+ // Reload the manifest if it needs to be relocalized.
+ if (extension_l10n_util::ShouldRelocalizeManifest(info))
+ return NEEDS_RELOCALIZATION;
+
+ return NOT_NEEDED;
}
void GetExplicitOriginsInExtent(const Extension* extension,
@@ -216,13 +231,6 @@
const GURL& update_url,
Extension::Location location);
- // Reloads the given extensions from their manifests on disk (instead of what
- // we have cached in the prefs).
- void ReloadExtensionManifests(
- ExtensionPrefs::ExtensionsInfo* extensions_to_reload,
- base::TimeTicks start_time,
- scoped_refptr<ExtensionsService> frontend);
-
private:
friend class base::RefCountedThreadSafe<ExtensionsServiceBackend>;
@@ -445,41 +453,6 @@
external_extension_added_ |= true;
}
-void ExtensionsServiceBackend::ReloadExtensionManifests(
- ExtensionPrefs::ExtensionsInfo* extensions_to_reload,
- base::TimeTicks start_time,
- scoped_refptr<ExtensionsService> frontend) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
- frontend_ = frontend;
-
- for (size_t i = 0; i < extensions_to_reload->size(); ++i) {
- ExtensionInfo* info = extensions_to_reload->at(i).get();
- if (!ShouldReloadExtensionManifest(*info))
- continue;
-
- // We need to reload original manifest in order to localize properly.
- std::string error;
- scoped_refptr<const Extension> extension(extension_file_util::LoadExtension(
- info->extension_path, info->extension_location, false, &error));
-
- if (extension.get())
- extensions_to_reload->at(i)->extension_manifest.reset(
- static_cast<DictionaryValue*>(
- extension->manifest_value()->DeepCopy()));
- }
-
- // Finish installing on UI thread.
- BrowserThread::PostTask(
- BrowserThread::UI, FROM_HERE,
- NewRunnableMethod(
- frontend_,
- &ExtensionsService::ContinueLoadAllExtensions,
- extensions_to_reload,
- start_time,
- true));
-}
-
bool ExtensionsService::IsDownloadFromGallery(const GURL& download_url,
const GURL& referrer_url) {
// Special-case the themes mini-gallery.
@@ -783,7 +756,7 @@
// type to be external. An external extension should not be
// rejected if it fails the safty checks for a syncable extension.
// TODO(skerner): Work out other potential overlapping conditions.
- // (crbug/61000)
+ // (crbug.com/61000)
PendingExtensionMap::iterator it = pending_extensions_.find(id);
if (it != pending_extensions_.end()) {
VLOG(1) << "Extension id " << id
@@ -989,47 +962,66 @@
}
void ExtensionsService::LoadAllExtensions() {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
base::TimeTicks start_time = base::TimeTicks::Now();
// Load any component extensions.
LoadComponentExtensions();
// Load the previously installed extensions.
- scoped_ptr<ExtensionPrefs::ExtensionsInfo> info(
+ scoped_ptr<ExtensionPrefs::ExtensionsInfo> extensions_info(
extension_prefs_->GetInstalledExtensionsInfo());
- // If any extensions need localization, we bounce them all to the file thread
- // for re-reading and localization.
- for (size_t i = 0; i < info->size(); ++i) {
- if (ShouldReloadExtensionManifest(*info->at(i))) {
- BrowserThread::PostTask(
- BrowserThread::FILE, FROM_HERE, NewRunnableMethod(
- backend_.get(),
- &ExtensionsServiceBackend::ReloadExtensionManifests,
- info.release(), // Callee takes ownership of the memory.
- start_time,
- scoped_refptr<ExtensionsService>(this)));
- return;
+ std::vector<int> reload_reason_counts(NUM_MANIFEST_RELOAD_REASONS, 0);
+ bool should_write_prefs = false;
+
+ for (size_t i = 0; i < extensions_info->size(); ++i) {
+ ExtensionInfo* info = extensions_info->at(i).get();
+
+ ManifestReloadReason reload_reason = ShouldReloadExtensionManifest(*info);
+ ++reload_reason_counts[reload_reason];
+ UMA_HISTOGRAM_ENUMERATION("Extensions.ManifestReloadEnumValue",
+ reload_reason, 100);
+
+ if (reload_reason != NOT_NEEDED) {
+ // Reloading and extension reads files from disk. We do this on the
+ // UI thread because reloads should be very rare, and the complexity
+ // added by delaying the time when the extensions service knows about
+ // all extensions is significant. See crbug.com/37548 for details.
+ // |allow_io| disables tests that file operations run on the file
+ // thread.
+ base::ThreadRestrictions::ScopedAllowIO allow_io;
+
+ std::string error;
+ scoped_refptr<const Extension> extension(
+ extension_file_util::LoadExtension(
+ info->extension_path, info->extension_location, false, &error));
+
+ if (extension.get()) {
+ extensions_info->at(i)->extension_manifest.reset(
+ static_cast<DictionaryValue*>(
+ extension->manifest_value()->DeepCopy()));
+ should_write_prefs = true;
+ }
}
}
- // Don't update prefs.
- // Callee takes ownership of the memory.
- ContinueLoadAllExtensions(info.release(), start_time, false);
-}
-
-void ExtensionsService::ContinueLoadAllExtensions(
- ExtensionPrefs::ExtensionsInfo* extensions_info,
- base::TimeTicks start_time,
- bool write_to_prefs) {
- scoped_ptr<ExtensionPrefs::ExtensionsInfo> info(extensions_info);
-
- for (size_t i = 0; i < info->size(); ++i) {
- LoadInstalledExtension(*info->at(i), write_to_prefs);
+ for (size_t i = 0; i < extensions_info->size(); ++i) {
+ LoadInstalledExtension(*extensions_info->at(i), should_write_prefs);
}
OnLoadedInstalledExtensions();
+ // The histograms Extensions.ManifestReload* allow us to validate
+ // the assumption that reloading manifest is a rare event.
+ UMA_HISTOGRAM_COUNTS_100("Extensions.ManifestReloadNotNeeded",
+ reload_reason_counts[NOT_NEEDED]);
+ UMA_HISTOGRAM_COUNTS_100("Extensions.ManifestReloadUnpackedDir",
+ reload_reason_counts[UNPACKED_DIR]);
+ UMA_HISTOGRAM_COUNTS_100("Extensions.ManifestReloadNeedsRelocalization",
+ reload_reason_counts[NEEDS_RELOCALIZATION]);
+
UMA_HISTOGRAM_COUNTS_100("Extensions.LoadAll", extensions_.size());
UMA_HISTOGRAM_COUNTS_100("Extensions.Disabled", disabled_extensions_.size());
@@ -1073,30 +1065,30 @@
UMA_HISTOGRAM_ENUMERATION("Extensions.LoadType", type, 100);
switch (type) {
case Extension::TYPE_THEME:
- theme_count++;
+ ++theme_count;
break;
case Extension::TYPE_USER_SCRIPT:
- user_script_count++;
+ ++user_script_count;
break;
case Extension::TYPE_HOSTED_APP:
- app_count++;
- hosted_app_count++;
+ ++app_count;
+ ++hosted_app_count;
break;
case Extension::TYPE_PACKAGED_APP:
- app_count++;
- packaged_app_count++;
+ ++app_count;
+ ++packaged_app_count;
break;
case Extension::TYPE_EXTENSION:
default:
- extension_count++;
+ ++extension_count;
break;
}
if (Extension::IsExternalLocation(location))
- external_count++;
+ ++external_count;
if ((*ex)->page_action() != NULL)
- page_action_count++;
+ ++page_action_count;
if ((*ex)->browser_action() != NULL)
- browser_action_count++;
+ ++browser_action_count;
}
UMA_HISTOGRAM_COUNTS_100("Extensions.LoadApp", app_count);
UMA_HISTOGRAM_COUNTS_100("Extensions.LoadHostedApp", hosted_app_count);
@@ -1110,6 +1102,7 @@
browser_action_count);
}
+
void ExtensionsService::LoadInstalledExtension(const ExtensionInfo& info,
bool write_to_prefs) {
std::string error;
@@ -1887,7 +1880,7 @@
// This is only here to help track down crbug.com/49114.
ExtensionHost::HostPointerList::iterator iter =
ExtensionHost::recently_deleted()->begin();
- for (; iter != ExtensionHost::recently_deleted()->end(); iter++) {
+ for (; iter != ExtensionHost::recently_deleted()->end(); ++iter) {
if (*iter == host) {
CHECK(host->GetURL().spec().size() + 2 != 0);
break;
