Fix double install and incorrect uninstall of external extensions.

BUG=chromium-os:7037
TEST=ExtensionsserviceTest.External*,ExtensionManagementTest.ExternalUrlUpdate

Review URL: http://codereview.chromium.org/3966001

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

1 files changed, 59 insertions, 14 deletions

diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index df29995..57d2cff 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -103,7 +103,7 @@ static std::vector<std::string> GetErrors() {
 class MockExtensionProvider : public ExternalExtensionProvider {
  public:
   explicit MockExtensionProvider(Extension::Location location)
-    : location_(location) {}
+    : location_(location), visit_count_(0) {}
   virtual ~MockExtensionProvider() {}
 
   void UpdateOrAddExtension(const std::string& id,
@@ -119,6 +119,7 @@ class MockExtensionProvider : public ExternalExtensionProvider {
   // ExternalExtensionProvider implementation:
   virtual void VisitRegisteredExtension(
       Visitor* visitor, const std::set<std::string>& ids_to_ignore) const {
+    visit_count_++;
     for (DataMap::const_iterator i = extension_map_.begin();
          i != extension_map_.end(); ++i) {
       if (ids_to_ignore.find(i->first) != ids_to_ignore.end())
@@ -131,15 +132,28 @@ class MockExtensionProvider : public ExternalExtensionProvider {
     }
   }
 
-  virtual Version* RegisteredVersion(const std::string& id,
-                                     Extension::Location* location) const {
+  virtual bool HasExtension(const std::string& id) const {
+    return extension_map_.find(id) != extension_map_.end();
+  }
+
+  virtual bool GetExtensionDetails(const std::string& id,
+                                   Extension::Location* location,
+                                   scoped_ptr<Version>* version) const {
     DataMap::const_iterator it = extension_map_.find(id);
     if (it == extension_map_.end())
-      return NULL;
+      return false;
+
+    if (version)
+      version->reset(Version::GetVersionFromString(it->second.first));
 
     if (location)
       *location = location_;
-    return Version::GetVersionFromString(it->second.first);
+
+    return true;
+  }
+  int visit_count() const { return visit_count_; }
+  void set_visit_count(int visit_count) {
+    visit_count_ = visit_count;
   }
 
  private:
@@ -147,6 +161,12 @@ class MockExtensionProvider : public ExternalExtensionProvider {
   DataMap extension_map_;
   Extension::Location location_;
 
+  // visit_count_ tracks the number of calls to VisitRegisteredExtension().
+  // Mutable because it must be incremented on each call to
+  // VisitRegisteredExtension(), which must be a const method to inherit
+  // from the class being mocked.
+  mutable int visit_count_;
+
   DISALLOW_COPY_AND_ASSIGN(MockExtensionProvider);
 };
 
@@ -197,10 +217,18 @@ class MockProviderVisitor : public ExternalExtensionProvider::Visitor {
        << "Got back ID (" << id.c_str() << ") we weren't expecting";
 
     if (pref) {
+      EXPECT_TRUE(provider_->HasExtension(id));
+
       // Ask provider if the extension we got back is registered.
       Extension::Location location = Extension::INVALID;
-      scoped_ptr<Version> v1(provider_->RegisteredVersion(id, NULL));
-      scoped_ptr<Version> v2(provider_->RegisteredVersion(id, &location));
+      scoped_ptr<Version> v1;
+      FilePath crx_path;
+
+      EXPECT_TRUE(provider_->GetExtensionDetails(id, NULL, &v1));
+      EXPECT_STREQ(version->GetString().c_str(), v1->GetString().c_str());
+
+      scoped_ptr<Version> v2;
+      EXPECT_TRUE(provider_->GetExtensionDetails(id, &location, &v2));
       EXPECT_STREQ(version->GetString().c_str(), v1->GetString().c_str());
       EXPECT_STREQ(version->GetString().c_str(), v2->GetString().c_str());
       EXPECT_EQ(Extension::EXTERNAL_PREF, location);
@@ -221,6 +249,13 @@ class MockProviderVisitor : public ExternalExtensionProvider::Visitor {
        << L"Got back ID (" << id.c_str() << ") we weren't expecting";
 
     if (pref) {
+      EXPECT_TRUE(provider_->HasExtension(id));
+
+      // External extensions with update URLs do not have versions.
+      scoped_ptr<Version> v1;
+      EXPECT_TRUE(provider_->GetExtensionDetails(id, NULL, &v1));
+      EXPECT_FALSE(v1.get());
+
       // Remove it so we won't count it again.
       prefs_->Remove(id, NULL);
     }
@@ -416,9 +451,8 @@ class ExtensionsServiceTest
     }
   }
 
-  void SetMockExternalProvider(Extension::Location location,
-                               ExternalExtensionProvider* provider) {
-    service_->SetProviderForTesting(location, provider);
+  void AddMockExternalProvider(ExternalExtensionProvider* provider) {
+    service_->AddProviderForTesting(provider);
   }
 
  protected:
@@ -2227,6 +2261,8 @@ void ExtensionsServiceTest::TestExternalProvider(
   loop_.RunAllPending();
   ASSERT_EQ(0u, loaded_.size());
 
+  provider->set_visit_count(0);
+
   // Register a test extension externally using the mock registry provider.
   FilePath source_path;
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &source_path));
@@ -2342,6 +2378,8 @@ void ExtensionsServiceTest::TestExternalProvider(
   loop_.RunAllPending();
   ASSERT_EQ(0u, loaded_.size());
   ValidatePrefKeyCount(1);
+
+  EXPECT_EQ(5, provider->visit_count());
 }
 
 // Tests the external installation feature
@@ -2354,7 +2392,7 @@ TEST_F(ExtensionsServiceTest, ExternalInstallRegistry) {
   // Now add providers. Extension system takes ownership of the objects.
   MockExtensionProvider* reg_provider =
       new MockExtensionProvider(Extension::EXTERNAL_REGISTRY);
-  SetMockExternalProvider(Extension::EXTERNAL_REGISTRY, reg_provider);
+  AddMockExternalProvider(reg_provider);
   TestExternalProvider(reg_provider, Extension::EXTERNAL_REGISTRY);
 }
 #endif
@@ -2367,7 +2405,8 @@ TEST_F(ExtensionsServiceTest, ExternalInstallPref) {
   // Now add providers. Extension system takes ownership of the objects.
   MockExtensionProvider* pref_provider =
       new MockExtensionProvider(Extension::EXTERNAL_PREF);
-  SetMockExternalProvider(Extension::EXTERNAL_PREF, pref_provider);
+
+  AddMockExternalProvider(pref_provider);
   TestExternalProvider(pref_provider, Extension::EXTERNAL_PREF);
 }
 
@@ -2376,10 +2415,16 @@ TEST_F(ExtensionsServiceTest, ExternalInstallPrefUpdateUrl) {
   InitializeEmptyExtensionsService();
   set_extensions_enabled(false);
 
-  // Now add providers. Extension system takes ownership of the objects.
+  // TODO(skerner): The mock provider is not a good model of a provider
+  // that works with update URLs, because it adds file and version info.
+  // Extend the mock to work with update URLs.  This test checks the
+  // behavior that is common to all external extension visitors.  The
+  // browser test ExtensionManagementTest.ExternalUrlUpdate tests that
+  // what the visitor does results in an extension being downloaded and
+  // installed.
   MockExtensionProvider* pref_provider =
       new MockExtensionProvider(Extension::EXTERNAL_PREF_DOWNLOAD);
-  SetMockExternalProvider(Extension::EXTERNAL_PREF_DOWNLOAD, pref_provider);
+  AddMockExternalProvider(pref_provider);
   TestExternalProvider(pref_provider, Extension::EXTERNAL_PREF_DOWNLOAD);
 }