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

chrome/browser/extensions/external_extension_provider_impl.cc

 // Copyright (c) 2011 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/external_extension_provider_impl.h"
 
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/memory/linked_ptr.h"
 #include "base/metrics/field_trial.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #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)
 #include "chrome/browser/extensions/external_registry_extension_loader_win.h"
 #endif
 
 // Constants for keeping track of extension preferences in a dictionary.
 const char ExternalExtensionProviderImpl::kLocation[] = "location";
 const char ExternalExtensionProviderImpl::kState[] = "state";
 const char ExternalExtensionProviderImpl::kExternalCrx[] = "external_crx";
 const char ExternalExtensionProviderImpl::kExternalVersion[] =
     "external_version";
 const char ExternalExtensionProviderImpl::kExternalUpdateUrl[] =
     "external_update_url";
 const char ExternalExtensionProviderImpl::kSupportedLocales[] =
     "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_;
-
-  DISALLOW_COPY_AND_ASSIGN(DefaultAppsProvider);
-};
+  // 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();
+  }
+}
 
 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(
     VisitorInterface* service,
     ExternalExtensionLoader* loader,
     Extension::Location crx_location,
     Extension::Location download_location)
   : crx_location_(crx_location),
     download_location_(download_location),
     service_(service),
@@ skipping 13 matching lines @@
   loader_->StartLoading();
 }
 
 void ExternalExtensionProviderImpl::SetPrefs(DictionaryValue* prefs) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Check if the service is still alive. It is possible that it had went
   // 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.
 
   // Set of unsupported extensions that need to be deleted from prefs_.
   std::set<std::string> unsupported_extensions;
 
   // Notify ExtensionService about all the extensions this provider has.
   for (DictionaryValue::key_iterator i = prefs_->begin_keys();
        i != prefs_->end_keys(); ++i) {
     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.";
       continue;
     }
 
     if (!prefs_->GetDictionaryWithoutPathExpansion(extension_id, &extension)) {
       LOG(WARNING) << "Malformed extension dictionary: key "
                    << extension_id.c_str()
                    << " has a value that is not a dictionary.";
@@ skipping 86 matching lines @@
       }
 
       scoped_ptr<Version> version;
       version.reset(Version::GetVersionFromString(external_version));
       if (!version.get()) {
         LOG(WARNING) << "Malformed extension dictionary for extension: "
                      << extension_id.c_str() << ".  Invalid version string \""
                      << external_version << "\".";
         continue;
       }
+      invalid_extensions_.erase(extension_id);
       service_->OnExternalExtensionFileFound(extension_id, version.get(), path,
                                              crx_location_);
     } else { // if (has_external_update_url)
       CHECK(has_external_update_url);  // Checking of keys above ensures this.
       if (download_location_ == Extension::INVALID) {
         LOG(WARNING) << "This provider does not support installing external "
                      << "extensions from update URLs.";
         continue;
       }
       GURL update_url(external_update_url);
       if (!update_url.is_valid()) {
         LOG(WARNING) << "Malformed extension dictionary for extension: "
                      << extension_id.c_str() << ".  Key " << kExternalUpdateUrl
                      << " has value \"" << external_update_url
                      << "\", which is not a valid URL.";
         continue;
       }
+      invalid_extensions_.erase(extension_id);
       service_->OnExternalExtensionUpdateUrlFound(
           extension_id, update_url, download_location_);
     }
   }
 
   for (std::set<std::string>::iterator it = unsupported_extensions.begin();
        it != unsupported_extensions.end(); ++it) {
     // Remove extension for the list of know external extensions. The extension
     // will be uninstalled later because provider doesn't provide it anymore.
     prefs_->Remove(*it, NULL);
@@ skipping 114 matching lines @@
 #endif
   provider_list->push_back(
       linked_ptr<ExternalExtensionProviderInterface>(
           new ExternalExtensionProviderImpl(
               service,
               new ExternalPolicyExtensionLoader(profile),
               Extension::INVALID,
               Extension::EXTERNAL_POLICY_DOWNLOAD)));
 
 #if !defined(OS_CHROMEOS)
   // We decide to install or not install default apps based on the following

[Mihai Parparita -not on Chrome, 2011/10/19 07:31:36]

If you're going to be moving DefaultAppsProvider to its own file, you should
also move the logic to check whether or not it needs to be registered to a
DefaultAppsProvider::ShouldRegister() static method.

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

Done.

   // criteria, from highest priority to lowest priority:
   //
   // - if this instance of chrome is participating in the default apps
   //   field trial, then install apps based on the group
   // - the command line option.  Tests use this option to disable installation
   //   of default apps in some cases
   // - the preferences value in the profile.  This value is usually set in
   //   the master_preferences file
   bool install_apps =
       profile->GetPrefs()->GetString(prefs::kDefaultApps) == "install";
@@ skipping 17 matching lines @@
     for (size_t i = 0; i < arraysize(unsupported_locales); ++i) {
       if (EndsWith(locale, unsupported_locales[i], false)) {
         supported_locale = false;
         break;
       }
     }
 
     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
 }