ExtensionService: Delay non-critical IO until after startup has finished.

Delay the calls to CheckForExternalUpdates() and GarbageCollectExtensions() until onload fires for the first frame to reduce IO at startup.

For newly created profiles we don't delay CheckForExternalUpdates().

BUG=136623
TEST=All unit tests should pass, external extension updates should continue to work.


Review URL: https://chromiumcodereview.appspot.com/10914076

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@170960 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/extension_service.cc

@@ -369,6 +369,7 @@ ExtensionService::ExtensionService(Profile* profile,
       show_extensions_prompts_(true),
       install_updates_when_idle_(true),
       ready_(false),
+      external_update_check_has_run_(false),
       toolbar_model_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       menu_manager_(profile),
       app_notification_manager_(
@@ -611,12 +612,22 @@ void ExtensionService::Init() {
         g_browser_process->profile_manager()->will_import()) {
       RegisterForImportFinished();
     } else {
-      // TODO(erikkay) this should probably be deferred to a future point
-      // rather than running immediately at startup.
-      CheckForExternalUpdates();
-
-      // TODO(erikkay) this should probably be deferred as well.
-      GarbageCollectExtensions();
+      // For newly created profiles, check for external updates immediately.
+      // Otherwise delay the check until onload fires for the main frame, to
+      // speed up startup.
+      bool is_new_profile =
+          profile_->GetPrefs()->GetInitializationStatus() ==
+              PrefService::INITIALIZATION_STATUS_CREATED_NEW_PROFILE;
+      if (is_new_profile)
+        CheckForExternalUpdates();
+
+      // Hook onload for the first frame to perform delayed startup actions.
+      // TODO(jeremy):If a generic mechanism is introduced to perform actions
+      // delayed until after startup finishes, use that rather than listening
+      // on the event ourselves.
+      registrar_.Add(this,
+                     content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
+                     content::NotificationService::AllSources());
     }
   }
 }
@@ -1741,6 +1752,12 @@ void ExtensionService::SetAllowFileAccess(const Extension* extension,
 void ExtensionService::CheckForExternalUpdates() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // In the case of a new profile, this function is called at init time and then
+  // again when the first frame loads.  Short-circuit subsequent calls.
+  if (external_update_check_has_run_)
+    return;
+  external_update_check_has_run_ = true;
+
   // Note that this installation is intentionally silent (since it didn't
   // go through the front-end).  Extensions that are registered in this
   // way are effectively considered 'pre-bundled', and so implicitly
@@ -2698,6 +2715,16 @@ void ExtensionService::Observe(int type,
               host->extension()));
       break;
     }
+    case content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME:
+      registrar_.Remove(
+          this,
+          content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
+          content::NotificationService::AllSources());
+
+      CheckForExternalUpdates();
+      GarbageCollectExtensions();
+      break;
+
     case content::NOTIFICATION_RENDERER_PROCESS_CREATED: {
       content::RenderProcessHost* process =
           content::Source<content::RenderProcessHost>(source).ptr();

chrome/browser/extensions/extension_service.h

@@ -380,6 +380,13 @@ class ExtensionService
   // Check for updates (or potentially new extensions from external providers)
   void CheckForExternalUpdates();
 
+  // For testing: CheckForExternalUpdates() is idempotent.  Reset the flag
+  // that causes the function to perform an early return after it's first
+  // invocation.
+  void ResetExternalUpdateCheckGuardForTests() {
+    external_update_check_has_run_ = false;
+  }
+
   // Unload the specified extension.
   virtual void UnloadExtension(
       const std::string& extension_id,
@@ -894,6 +901,9 @@ class ExtensionService
   // has fired.
   bool ready_;
 
+  // Have we done an external update check yet?
+  bool external_update_check_has_run_;
+
   // Our extension updater, if updates are turned on.
   scoped_ptr<extensions::ExtensionUpdater> updater_;
 

chrome/browser/extensions/extension_service_unittest.cc

@@ -1230,7 +1230,9 @@ TEST_F(ExtensionServiceTest, CleanupOnStartup) {
   }
 
   service_->Init();
-  // Wait for GarbageCollectExtensions task to complete.
+  service_->GarbageCollectExtensions();
+
+  // Wait for GarbageCollectExtensions tasks to complete.
   loop_.RunUntilIdle();
 
   file_util::FileEnumerator dirs(extensions_install_dir_, false,
@@ -1268,7 +1270,8 @@ TEST_F(ExtensionServiceTest, GarbageCollectWithPendingUpdates) {
       "hpiknbiabeeppbpihjehijgoemciehgk/3")));
 
   service_->GarbageCollectExtensions();
-  // Wait for GarbageCollectExtensions task to complete.
+
+  // Wait for GarbageCollectExtensions tasks to complete.
   loop_.RunUntilIdle();
 
   // Verify that the pending update for the first extension didn't get
@@ -1302,7 +1305,10 @@ TEST_F(ExtensionServiceTest, UpdateOnStartup) {
       "hpiknbiabeeppbpihjehijgoemciehgk/3")));
 
   service_->Init();
-  // Wait for GarbageCollectExtensions task to complete.
+
+  service_->GarbageCollectExtensions();
+
+  // Wait for GarbageCollectExtensions tasks to complete.
   loop_.RunUntilIdle();
 
   // Verify that the pending update for the first extension got installed.
@@ -3234,6 +3240,7 @@ TEST_F(ExtensionServiceTest, ComponentExtensionWhitelisted) {
   EXPECT_TRUE(service_->GetExtensionById(good0, false));
 
   // Poke external providers and make sure the extension is still present.
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   ASSERT_EQ(1u, service_->extensions()->size());
   EXPECT_TRUE(service_->GetExtensionById(good0, false));
@@ -3272,6 +3279,7 @@ TEST_F(ExtensionServiceTest, PolicyInstalledExtensionsWhitelisted) {
 
   // Reloading extensions should find our externally registered extension
   // and install it.
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
 
@@ -3450,6 +3458,7 @@ TEST_F(ExtensionServiceTest, ExternalExtensionAutoAcknowledgement) {
   }
 
   // Providers are set up. Let them run.
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
 
@@ -4059,6 +4068,7 @@ void ExtensionServiceTest::TestExternalProvider(
 
   // Reloading extensions should find our externally registered extension
   // and install it.
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
 
@@ -4086,6 +4096,7 @@ void ExtensionServiceTest::TestExternalProvider(
   provider->UpdateOrAddExtension(good_crx, "1.0.0.1", source_path);
 
   loaded_.clear();
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
   ASSERT_EQ(0u, GetErrors().size());
@@ -4109,6 +4120,7 @@ void ExtensionServiceTest::TestExternalProvider(
     // The extension should also be gone from the install directory.
     ASSERT_FALSE(file_util::PathExists(install_path));
     loaded_.clear();
+    service_->ResetExternalUpdateCheckGuardForTests();
     service_->CheckForExternalUpdates();
     loop_.RunUntilIdle();
     ASSERT_EQ(0u, loaded_.size());
@@ -4121,6 +4133,7 @@ void ExtensionServiceTest::TestExternalProvider(
     SetPrefInteg(good_crx, "state", Extension::ENABLED);
 
     loaded_.clear();
+    service_->ResetExternalUpdateCheckGuardForTests();
     service_->CheckForExternalUpdates();
     loop_.RunUntilIdle();
     ASSERT_EQ(1u, loaded_.size());
@@ -4148,6 +4161,7 @@ void ExtensionServiceTest::TestExternalProvider(
     // Now test the case where user uninstalls and then the extension is removed
     // from the external provider.
     provider->UpdateOrAddExtension(good_crx, "1.0.0.1", source_path);
+    service_->ResetExternalUpdateCheckGuardForTests();
     service_->CheckForExternalUpdates();
     loop_.RunUntilIdle();
 
@@ -4256,7 +4270,9 @@ TEST_F(ExtensionServiceTest, ExternalUninstall) {
   service_->Init();
 
   ASSERT_EQ(0u, GetErrors().size());
-  ASSERT_EQ(0u, loaded_.size());
+
+  service_->CheckForExternalUpdates();
+  service_->GarbageCollectExtensions();
 
   // Verify that it's not the disabled extensions flag causing it not to load.
   set_extensions_enabled(true);
@@ -4282,7 +4298,9 @@ TEST_F(ExtensionServiceTest, MultipleExternalUpdateCheck) {
 
   // Start two checks for updates.
   provider->set_visit_count(0);
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
 
@@ -4298,7 +4316,9 @@ TEST_F(ExtensionServiceTest, MultipleExternalUpdateCheck) {
   // Two checks for external updates should find the extension, and install it
   // once.
   provider->set_visit_count(0);
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
   EXPECT_EQ(2, provider->visit_count());
@@ -4312,7 +4332,9 @@ TEST_F(ExtensionServiceTest, MultipleExternalUpdateCheck) {
 
   provider->RemoveExtension(good_crx);
   provider->set_visit_count(0);
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
 
@@ -5750,7 +5772,9 @@ TEST_F(ExtensionServiceTest, ExternalInstallGlobalError) {
   provider->UpdateOrAddExtension(hosted_app, "1.0.0.0",
                                  data_dir_.AppendASCII("hosted_app.crx"));
 
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
+  service_->GarbageCollectExtensions();
   loop_.RunUntilIdle();
   EXPECT_TRUE(extensions::HasExternalInstallError(service_));
   service_->EnableExtension(hosted_app);
@@ -5761,6 +5785,7 @@ TEST_F(ExtensionServiceTest, ExternalInstallGlobalError) {
   provider->UpdateOrAddExtension(page_action, "1.0.0.0",
                                  data_dir_.AppendASCII("page_action.crx"));
 
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
   EXPECT_TRUE(extensions::HasExternalInstallError(service_));
@@ -5845,6 +5870,7 @@ TEST_F(ExtensionServiceTest, WipeOutExtension) {
   provider_pref->UpdateOrAddExtension(good_crx, "1.0.0.0",
       data_dir_.AppendASCII("good.crx"));
 
+  service_->ResetExternalUpdateCheckGuardForTests();
   service_->CheckForExternalUpdates();
   loop_.RunUntilIdle();
   EXPECT_FALSE(extensions::HasExternalInstallError(service_));