Remove race condition when installing default apps into a new profile.

chrome/browser/extensions/external_extension_provider_impl.cc

Index: chrome/browser/extensions/external_extension_provider_impl.cc
===================================================================
--- chrome/browser/extensions/external_extension_provider_impl.cc	(revision 105797)
+++ chrome/browser/extensions/external_extension_provider_impl.cc	(working copy)
@@ -14,16 +14,20 @@
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/default_apps_trial.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/external_extension_provider_interface.h"
 #include "chrome/browser/extensions/external_policy_extension_loader.h"
 #include "chrome/browser/extensions/external_pref_extension_loader.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "content/browser/browser_thread.h"
+#include "content/common/notification_details.h"
+#include "content/common/notification_source.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if defined(OS_WIN)
@@ -42,44 +46,123 @@
     "supported_locales";
 
 #if !defined(OS_CHROMEOS)
-class DefaultAppsProvider : public ExternalExtensionProviderImpl {
- public:
-  DefaultAppsProvider(VisitorInterface* service, Profile* profile)
-      : ExternalExtensionProviderImpl(service,
-            new ExternalPrefExtensionLoader(chrome::DIR_DEFAULT_APPS,
-                ExternalPrefExtensionLoader::NONE),
-            Extension::EXTERNAL_PREF, Extension::INVALID),
-        profile_(profile) {
-    DCHECK(profile_);
+DefaultAppsProvider::DefaultAppsProvider(VisitorInterface* service,
+                                         ExternalExtensionLoader* loader,
+                                         Profile* profile)
+    : ExternalExtensionProviderImpl(service, loader,
+          Extension::EXTERNAL_PREF, Extension::INVALID),
+      profile_(profile) {
+  DCHECK(profile_);
+}
+
+DefaultAppsProvider::~DefaultAppsProvider() {
+}
+
+void DefaultAppsProvider::SetPrefs(base::DictionaryValue* prefs) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (prefs->size() > 0) {
+    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
+                   Source<Profile>(profile_));
+    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
+                   NotificationService::AllSources());
   }
 
-  // ExternalExtensionProviderImpl overrides:
-  virtual void ServiceShutdown() OVERRIDE;
-  virtual void VisitRegisteredExtension() const OVERRIDE;
+  ExternalExtensionProviderImpl::SetPrefs(prefs);
 
- private:
-  Profile* profile_;
+  // If the number of invalid extension is the same as the total, then we are
+  // done.
+  if (prefs->size() == invalid_extensions().size()) {

[Finnur, 2011/10/19 10:01:29]

Is it safe to rely on the count? Don't you want to compare id's?

[Roger Tawa OOO till Jul 10th, 2011/10/19 20:48:25]

The invalid_extension set can only come from extensions listed in prefs.  So if
the counts are the same then all the ids must be same.

+    profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                     kInstallDone);
+    profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+  }
+}
 
-  DISALLOW_COPY_AND_ASSIGN(DefaultAppsProvider);
-};
-
 void DefaultAppsProvider::ServiceShutdown() {
   profile_ = NULL;
   ExternalExtensionProviderImpl::ServiceShutdown();
 }
 
 void DefaultAppsProvider::VisitRegisteredExtension() const {
-  // Don't install default apps if the profile already has apps installed.
   if (profile_) {
-    ExtensionService* extension_service = profile_->GetExtensionService();
-    if (extension_service && extension_service->HasApps()) {
-      service()->OnExternalProviderReady();
-      return;
+    int state = profile_->GetPrefs()->GetInteger(
+        prefs::kDefaultAppsInstallState);

[Finnur, 2011/10/19 10:01:29]

nit: Is there room in the line above or are my eyes deceiving me? 

[Roger Tawa OOO till Jul 10th, 2011/10/19 20:48:25]

yup, you need glasses :-)

[Finnur, 2011/10/20 10:10:43]

Awww... :)

On 2011/10/19 20:48:25, Roger Tawa wrote:

+    switch (state) {
+      case kInstallNotStarted: {
+        // We never even tried to install the default apps, so try now.
+        // Don't install default apps if the profile already has apps installed.
+        ExtensionService* extension_service = profile_->GetExtensionService();
+        if (extension_service && extension_service->HasApps()) {
+          service()->OnExternalProviderReady();

[Finnur, 2011/10/19 10:01:29]

nit: Probably doesn't matter, but shouldn't you set the kInstallDone pref and
then say that the provider is ready? (in other words, move this between line 102
and 103)?

[Roger Tawa OOO till Jul 10th, 2011/10/19 20:48:25]

Done.

+
+          // Keep track that we are done.

[Finnur, 2011/10/19 10:01:29]

nit: Keep track of the fact that we are done. 
Better?

[Roger Tawa OOO till Jul 10th, 2011/10/19 20:48:25]

Done.

+          profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                           kInstallDone);
+          profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+          return;
+        }
+
+        // Remember that we are now installing the default apps.
+        profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                         kInstalling);
+        profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+        // No break.

[Finnur, 2011/10/19 10:01:29]

This is a bit weird... skip the break to fall into a case statement that
contains only a break?

[Roger Tawa OOO till Jul 10th, 2011/10/19 20:48:25]

Done.

+      }
+      case kInstalling:
+        // The profile was closed while the extensions were being installed.
+        // Continue the install process.

[Finnur, 2011/10/19 10:01:29]

This comment is a bit confusing. I think you want to say:
// The profile was closed last time the extensions were being installed.
// Resume the install process.

[Roger Tawa OOO till Jul 10th, 2011/10/19 20:48:25]

more verbose explanation.

+        break;
+      case kInstallDone:
+        // Defaults are installed, so don't need to try again.
+        service()->OnExternalProviderReady();
+        return;
     }
   }
 
   ExternalExtensionProviderImpl::VisitRegisteredExtension();
 }
+
+void DefaultAppsProvider::Observe(int type,
+                                  const NotificationSource& source,
+                                  const NotificationDetails& details) {
+  if (!profile_)
+    return;
+
+  // If the notification is for a failed extension, remember its id so that
+  // we don't wait forever for it to get installed.
+  if (type == chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR) {
+    CrxInstaller* crx_installer = Source<CrxInstaller>(source).ptr();
+    install_error_extensions_.insert(crx_installer->expected_id());
+  }
+
+  ExtensionService* extension_service = profile_->GetExtensionService();
+  if (!extension_service)
+    return;
+
+  for (DictionaryValue::key_iterator i = prefs()->begin_keys();
+       i != prefs()->end_keys(); ++i) {
+    const std::string& extension_id = *i;
+
+    // If the extension id is for one that is invalid or failed to install,
+    // then consider it "done".
+    if (invalid_extensions().count(extension_id) ||
+        install_error_extensions_.count(extension_id)) {
+      continue;
+    }
+
+    if (!extension_service->GetExtensionById(extension_id, true)) {
+      // We found an extension that is not yet known to the service.  We
+      // need to keep going.
+      return;
+    }
+  }
+
+  profile_->GetPrefs()->SetInteger(prefs::kDefaultAppsInstallState,
+                                   kInstallDone);
+  profile_->GetPrefs()->ScheduleSavePersistentPrefs();
+  registrar_.RemoveAll();
+}
 #endif
 
 ExternalExtensionProviderImpl::ExternalExtensionProviderImpl(
@@ -113,6 +196,7 @@
   // away while |loader_| was working on the FILE thread.
   if (!service_) return;
 
+  invalid_extensions_.clear();
   prefs_.reset(prefs);
   ready_ = true; // Queries for extensions are allowed from this point.
 
@@ -125,6 +209,9 @@
     const std::string& extension_id = *i;
     DictionaryValue* extension;
 
+    // Assume the extension is invalid until proven otherwise.
+    invalid_extensions_.insert(extension_id);
+
     if (!Extension::IdIsValid(extension_id)) {
       LOG(WARNING) << "Malformed extension dictionary: key "
                    << extension_id.c_str() << " is not a valid id.";
@@ -231,6 +318,7 @@
                      << external_version << "\".";
         continue;
       }
+      invalid_extensions_.erase(extension_id);
       service_->OnExternalExtensionFileFound(extension_id, version.get(), path,
                                              crx_location_);
     } else { // if (has_external_update_url)
@@ -248,6 +336,7 @@
                      << "\", which is not a valid URL.";
         continue;
       }
+      invalid_extensions_.erase(extension_id);
       service_->OnExternalExtensionUpdateUrlFound(
           extension_id, update_url, download_location_);
     }
@@ -420,7 +509,12 @@
     if (supported_locale) {
       provider_list->push_back(
           linked_ptr<ExternalExtensionProviderInterface>(
-              new DefaultAppsProvider(service, profile)));
+              new DefaultAppsProvider(
+                  service,
+                  new ExternalPrefExtensionLoader(
+                      chrome::DIR_DEFAULT_APPS,
+                      ExternalPrefExtensionLoader::NONE),
+                  profile)));
     }
   }
 #endif