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"
 
@@ skipping 32 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);
 
   // 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/20 02:30:25]

I didn't mean that there was code that relies on this, just that it's a behavior
change that should be documented. Maybe add it to the CL description?

[Roger Tawa OOO till Jul 10th, 2011/10/20 14:31:15]

OK.  I think it would be better to have the comment in the code though and not
in a CL description, since this way it will be more findable by others.  You had
said:

---start quote---
The change you have to by default add extension IDs unsupported_extensions will
mean that a lot more extensions will be uninstalled by default (since if
HasExtension returns false we uninstall the extension in
ExtensionService::CheckExternalUninstall). Arguably this might be a good thing
(if the path is no longer valid because the app that installed the extension was
moved/uninstalled, we should remove the extension too), but I want us to make
that change intentionally.
---end quote---

So it seems there is code in ExtensionService that depends on this behaviour.  I
thought my comment captured your sentiment pretty well but if you'd like to
suggest different text I would be more than happy to replace my comment with
yours.

+  // still being available to HasExtension().  See how to fix this.
+  std::set<std::string> invalid_extensions_;
+
   DISALLOW_COPY_AND_ASSIGN(ExternalExtensionProviderImpl);
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_PROVIDER_IMPL_H_