Error out if a version in external_extensions.json is invalid.

BUG=53915
TEST=ExtensionsServiceTest.ExternalPrefProvider

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

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

3 files changed, 54 insertions, 39 deletions

diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 4d1ce3b..06198387 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -1620,6 +1620,7 @@ void ExtensionsServiceBackend::SetProviderForTesting(
 void ExtensionsServiceBackend::OnExternalExtensionFileFound(
     const std::string& id, const Version* version, const FilePath& path,
     Extension::Location location) {
+  DCHECK(version);
   ChromeThread::PostTask(
       ChromeThread::UI, FROM_HERE,
       NewRunnableMethod(
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index bd92b0c..5e64b79 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -2309,17 +2309,17 @@ TEST_F(ExtensionsServiceTest, ExternalPrefProvider) {
   InitializeEmptyExtensionsService();
   std::string json_data =
       "{"
-        "\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
-          "\"external_crx\": \"RandomExtension.crx\","
-          "\"external_version\": \"1.0\""
-        "},"
-        "\"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\": {"
-          "\"external_crx\": \"RandomExtension2.crx\","
-          "\"external_version\": \"2.0\""
-        "},"
-        "\"cccccccccccccccccccccccccccccccc\": {"
-          "\"external_update_url\": \"http:\\\\foo.com/update\""
-        "}"
+      "  \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
+      "    \"external_crx\": \"RandomExtension.crx\","
+      "    \"external_version\": \"1.0\""
+      "  },"
+      "  \"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\": {"
+      "    \"external_crx\": \"RandomExtension2.crx\","
+      "    \"external_version\": \"2.0\""
+      "  },"
+      "  \"cccccccccccccccccccccccccccccccc\": {"
+      "    \"external_update_url\": \"http:\\\\foo.com/update\""
+      "  }"
       "}";
 
   MockProviderVisitor visitor;
@@ -2332,40 +2332,47 @@ TEST_F(ExtensionsServiceTest, ExternalPrefProvider) {
   ignore_list.insert("cccccccccccccccccccccccccccccccc");
   EXPECT_EQ(0, visitor.Visit(json_data, ignore_list));
 
-  // Use a json that contains six invalid extensions:
+  // Simulate an external_extensions.json file that contains seven invalid
+  // extensions:
   // - One that is missing the 'external_crx' key.
   // - One that is missing the 'external_version' key.
   // - One that is specifying .. in the path.
   // - One that specifies both a file and update URL.
   // - One that specifies no file or update URL.
   // - One that has an update URL that is not well formed.
-  // - Plus one valid extension to make sure the json file is parsed properly.
+  // - One that contains a malformed version.
+  // The final extension is valid, and we check that it is read to make sure
+  // failures don't stop valid records from being read.
   json_data =
       "{"
-        "\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
-          "\"external_version\": \"1.0\""
-        "},"
-        "\"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\": {"
-          "\"external_crx\": \"RandomExtension.crx\""
-        "},"
-        "\"cccccccccccccccccccccccccccccccc\": {"
-          "\"external_crx\": \"..\\\\foo\\\\RandomExtension2.crx\","
-          "\"external_version\": \"2.0\""
-        "},"
-        "\"dddddddddddddddddddddddddddddddd\": {"
-          "\"external_crx\": \"..\\\\foo\\\\RandomExtension2.crx\","
-          "\"external_version\": \"2.0\","
-          "\"external_update_url\": \"http:\\\\foo.com/update\""
-        "},"
-        "\"eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": {"
-        "},"
-        "\"ffffffffffffffffffffffffffffffff\": {"
-          "\"external_update_url\": \"This string is not avalid URL\""
-        "},"
-        "\"gggggggggggggggggggggggggggggggg\": {"
-          "\"external_crx\": \"RandomValidExtension.crx\","
-          "\"external_version\": \"1.0\""
-        "}"
+      "  \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
+      "    \"external_version\": \"1.0\""
+      "  },"
+      "  \"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\": {"
+      "    \"external_crx\": \"RandomExtension.crx\""
+      "  },"
+      "  \"cccccccccccccccccccccccccccccccc\": {"
+      "    \"external_crx\": \"..\\\\foo\\\\RandomExtension2.crx\","
+      "    \"external_version\": \"2.0\""
+      "  },"
+      "  \"dddddddddddddddddddddddddddddddd\": {"
+      "    \"external_crx\": \"RandomExtension2.crx\","
+      "    \"external_version\": \"2.0\","
+      "    \"external_update_url\": \"http:\\\\foo.com/update\""
+      "  },"
+      "  \"eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": {"
+      "  },"
+      "  \"ffffffffffffffffffffffffffffffff\": {"
+      "    \"external_update_url\": \"This string is not a valid URL\""
+      "  },"
+      "  \"gggggggggggggggggggggggggggggggg\": {"
+      "    \"external_crx\": \"RandomExtension3.crx\","
+      "    \"external_version\": \"This is not a valid version!\""
+      "  },"
+      "  \"hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh\": {"
+      "    \"external_crx\": \"RandomValidExtension.crx\","
+      "    \"external_version\": \"1.0\""
+      "  }"
       "}";
   ignore_list.clear();
   EXPECT_EQ(1, visitor.Visit(json_data, ignore_list));
diff --git a/chrome/browser/extensions/external_pref_extension_provider.cc b/chrome/browser/extensions/external_pref_extension_provider.cc
index 9020151..3eaafc9 100644
--- a/chrome/browser/extensions/external_pref_extension_provider.cc
+++ b/chrome/browser/extensions/external_pref_extension_provider.cc
@@ -86,7 +86,7 @@ void ExternalPrefExtensionProvider::VisitRegisteredExtension(
         continue;
       }
 
-      // See if it's an absolute path...
+      // If the path is relative, make it absolute.
       FilePath path(external_crx);
       if (!path.IsAbsolute()) {
         // Try path as relative path from external extension dir.
@@ -97,6 +97,12 @@ void ExternalPrefExtensionProvider::VisitRegisteredExtension(
 
       scoped_ptr<Version> version;
       version.reset(Version::GetVersionFromString(external_version));
+      if (!version.get()) {
+        LOG(ERROR) << "Malformed extension dictionary for extension: "
+                   << extension_id.c_str() << ".  Invalid version string \""
+                   << external_version << "\".";
+        continue;
+      }
       visitor->OnExternalExtensionFileFound(extension_id, version.get(), path,
                                             Extension::EXTERNAL_PREF);
       continue;
@@ -107,7 +113,8 @@ void ExternalPrefExtensionProvider::VisitRegisteredExtension(
     if (!update_url.is_valid()) {
       LOG(WARNING) << "Malformed extension dictionary for extension: "
                    << extension_id.c_str() << ".  " << kExternalUpdateUrl
-                   << " must be a valid URL: " << external_update_url;
+                   << " must be a valid URL.  Saw \"" << external_update_url
+                   << "\".";
       continue;
     }
     visitor->OnExternalExtensionUpdateUrlFound(extension_id, update_url);