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

chrome/browser/extensions/external_extension_provider_impl.h

 // 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_PROVIDER_IMPL_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_PROVIDER_IMPL_H_
 #pragma once
 
 #include "chrome/browser/extensions/external_extension_provider_interface.h"
 
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/extensions/external_extension_loader.h"
+#include "content/common/notification_observer.h"
+#include "content/common/notification_registrar.h"
 
 class ExternalExtensionLoader;
 class Profile;
 class Version;
 
 namespace base {
 class DictionaryValue;
 class ValueSerializer;
 }
 
@@ skipping 20 matching lines @@
   virtual ~ExternalExtensionProviderImpl();
 
   // Populates a list with providers for all known sources.
   static void CreateExternalProviders(
       VisitorInterface* service,
       Profile* profile,
       ProviderCollection* provider_list);
 
   // Sets underlying prefs and notifies provider. Only to be called by the
   // owned ExternalExtensionLoader instance.
-  void SetPrefs(base::DictionaryValue* prefs);
+  virtual void SetPrefs(base::DictionaryValue* prefs);

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

I'm getting uncomfortable with the increased overriding of methods from
ExternalExtensionProviderImpl so that you can implement DefaultAppsProvider,
since the two classes are becoming tightly coupled. 

The preferred design would be to either have a whole new
ExternalExtensionProviderInterface implementation, or to implement a new
ExternalExtensionLoader that you can plug into ExternalExtensionProviderImpl

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

I could create a separate implementation of ExternalExtensionProviderInterface,
but the implementation would have a lot of duplicate code with
ExternalExtensionProviderImpl, so I would likely create a base class anyway. 
Not sure that makes much of a difference then.

Also, the relationship between the loaders and the the provider implementation
is itself tightly coupled.  For example, ExternalExtensionLoader::Init() takes a
ExternalExtensionProviderImpl pointer directly, and not an
ExternalExtensionProviderInterface.  If this interface had a SetPrefs(), then
this could be cleaned up.  This would of course make SetPrefs() virtual in any
case.

I can make this change to the interface if you like and clean up the code if
this is the correct approach, let me know.

The default apps provider needs to do some things that cannot be done as an
extension loader only.  For example, it needs to know if the extensions are
valid and supported, even if they did load successfully.  Furthermore, the
loader could not simply say "there are no extensions to load" if the profile is
detected to already have apps, since then its provider could not implement
HasExtension() and GetExtensionDetails().

 
   // ExternalExtensionProvider implementation:
   virtual void ServiceShutdown() OVERRIDE;
   virtual void VisitRegisteredExtension() const OVERRIDE;
   virtual bool HasExtension(const std::string& id) const OVERRIDE;
   virtual bool GetExtensionDetails(const std::string& id,
                                    Extension::Location* location,
                                    scoped_ptr<Version>* version) const OVERRIDE;
 
   virtual bool IsReady();
 
   static const char kLocation[];
   static const char kState[];
   static const char kExternalCrx[];
   static const char kExternalVersion[];
   static const char kExternalUpdateUrl[];
   static const char kSupportedLocales[];
 
+  // Used only for testing.
+  const std::set<std::string>& invalid_extensions() const {
+    return invalid_extensions_;
+  }
+
  protected:
   VisitorInterface* service() const { return service_; }
 
+  base::DictionaryValue* prefs() const { return prefs_.get(); }
+
  private:
   // Location for external extensions that are provided by this provider from
   // local crx files.
   const Extension::Location crx_location_;
 
   // Location for external extensions that are provided by this provider from
   // update URLs.
   const Extension::Location download_location_;
 
   // Weak pointer to the object that consumes the external extensions.
   // This is zeroed out by: ServiceShutdown()
   VisitorInterface* service_;  // weak
 
   // Dictionary of the external extensions that are provided by this provider.
   scoped_ptr<base::DictionaryValue> prefs_;
 
   // Indicates that the extensions provided by this provider are loaded
   // entirely.
   bool ready_;
 
   // The loader that loads the list of external extensions and reports them
   // via |SetPrefs|.
   scoped_refptr<ExternalExtensionLoader> loader_;
 
+  // Extensions that were found to be invalid while processing the prefs_
+  // dictionary.  These are extensions that this provider did not even attempt
+  // to install because they failed one or more of the checks in SetPrefs().
+  //
+  // TODO(mihai): It would be nice to simply remove these extensions from
+  // prefs_, but for now, there is code that depends on these invalid extensions

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

What code are you referring to here?

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

If you look at patch set #1 of this CL, I was adding extensions that were deemed
invalid to the unsupported set.  In a private email you sent me, you worried
that because this would make those extensions unavailable to HasExtension(),
this might cause problems with existing code.  However, you said it would be
good to clean this up eventually.

+  // still being available to HasExtension().  See how to fix this.
+  std::set<std::string> invalid_extensions_;
+
   DISALLOW_COPY_AND_ASSIGN(ExternalExtensionProviderImpl);
 };
 
+#if !defined(OS_CHROMEOS)
+class DefaultAppsProvider : public ExternalExtensionProviderImpl,

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

Now that you're exposing this class in the header file and it's gotten pretty
big, you should probably move it to its own .h/.cc file. Also, then you can
exclude those files from the Chrome OS build via a Gyp condition instead of
needing preprocessor wrappers.

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

+1

On 2011/10/19 07:31:36, Mihai Parparita wrote:

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

Done.

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

Done.

+                            public NotificationObserver {
+ public:
+  DefaultAppsProvider(VisitorInterface* service,
+                      ExternalExtensionLoader* loader,
+                      Profile* profile);
+  virtual ~DefaultAppsProvider();
+
+  // ExternalExtensionProviderImpl overrides:
+  virtual void SetPrefs(base::DictionaryValue* prefs) OVERRIDE;
+  virtual void ServiceShutdown() OVERRIDE;
+  virtual void VisitRegisteredExtension() const OVERRIDE;
+
+  // Implementation of NotificationObserver:
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) OVERRIDE;
+
+  // Used only for testing.
+  const std::set<std::string>& install_error_extensions() const {
+    return install_error_extensions_;
+  }
+
+  enum InstallState {

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

Perhaps document that the values of these enums are written to the user's
profile, so they must not change?

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

Done.

+    kInstallNotStarted,
+    kInstalling,
+    kInstallDone
+  };

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

nit: enum should be at the top of the public section.

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

Done.

+
+ private:
+  Profile* profile_;
+  NotificationRegistrar registrar_;
+
+  // Set of extensions that failed to install for some reason.
+  std::set<std::string> install_error_extensions_;
+
+  DISALLOW_COPY_AND_ASSIGN(DefaultAppsProvider);
+};
+#endif
+
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_PROVIDER_IMPL_H_