Improve error messages when a manifest is not readable or not valid JSON.

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

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

7 files changed, 67 insertions, 5 deletions

diff --git a/chrome/browser/extensions/extension_file_util.cc b/chrome/browser/extensions/extension_file_util.cc
index 32c4819..1b6f474 100644
--- a/chrome/browser/extensions/extension_file_util.cc
+++ b/chrome/browser/extensions/extension_file_util.cc
@@ -101,14 +101,26 @@ Extension* LoadExtension(const FilePath& extension_path,
   FilePath manifest_path =
       extension_path.AppendASCII(Extension::kManifestFilename);
   if (!file_util::PathExists(manifest_path)) {
-    *error = extension_manifest_errors::kInvalidManifest;
+    *error = extension_manifest_errors::kManifestUnreadable;
     return NULL;
   }
 
   JSONFileValueSerializer serializer(manifest_path);
   scoped_ptr<Value> root(serializer.Deserialize(error));
-  if (!root.get())
+  if (!root.get()) {
+    if (error->empty()) {
+      // If |error| is empty, than the file could not be read.
+      // It would be cleaner to have the JSON reader give a specific error
+      // in this case, but other code tests for a file error with
+      // error->empty().  For now, be consistent.
+      *error = extension_manifest_errors::kManifestUnreadable;
+    } else {
+      *error = StringPrintf("%s  %s",
+                            extension_manifest_errors::kManifestParseError,
+                            error->c_str());
+    }
     return NULL;
+  }
 
   if (!root->IsType(Value::TYPE_DICTIONARY)) {
     *error = extension_manifest_errors::kInvalidManifest;
diff --git a/chrome/browser/extensions/extension_file_util_unittest.cc b/chrome/browser/extensions/extension_file_util_unittest.cc
index 7230ecd..c462032 100644
--- a/chrome/browser/extensions/extension_file_util_unittest.cc
+++ b/chrome/browser/extensions/extension_file_util_unittest.cc
@@ -185,6 +185,41 @@ TEST(ExtensionFileUtil, CheckIllegalFilenamesReservedAndIllegal) {
                                                              &error));
 }
 
+TEST(ExtensionFileUtil, LoadExtensionGivesHelpfullErrorOnMissingManifest) {
+  FilePath install_dir;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &install_dir));
+  install_dir = install_dir.AppendASCII("extensions")
+      .AppendASCII("bad")
+      .AppendASCII("Extensions")
+      .AppendASCII("dddddddddddddddddddddddddddddddd")
+      .AppendASCII("1.0");
+
+  std::string error;
+  scoped_ptr<Extension> extension(
+      extension_file_util::LoadExtension(install_dir, false, &error));
+  ASSERT_TRUE(extension == NULL);
+  ASSERT_FALSE(error.empty());
+  ASSERT_STREQ("Manifest file is missing or unreadable.", error.c_str());
+}
+
+TEST(ExtensionFileUtil, LoadExtensionGivesHelpfullErrorOnBadManifest) {
+  FilePath install_dir;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &install_dir));
+  install_dir = install_dir.AppendASCII("extensions")
+      .AppendASCII("bad")
+      .AppendASCII("Extensions")
+      .AppendASCII("eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee")
+      .AppendASCII("1.0");
+
+  std::string error;
+  scoped_ptr<Extension> extension(
+      extension_file_util::LoadExtension(install_dir, false, &error));
+  ASSERT_TRUE(extension == NULL);
+  ASSERT_FALSE(error.empty());
+  ASSERT_STREQ("Manifest is not valid JSON.  "
+               "Line: 2, column: 16, Syntax error.", error.c_str());
+}
+
 // TODO(aa): More tests as motivation allows. Maybe steal some from
 // ExtensionsService? Many of them could probably be tested here without the
 // MessageLoop shenanigans.
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 1eaa7159..f1a1ede 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -611,7 +611,7 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) {
 
   EXPECT_TRUE(MatchPattern(GetErrors()[1],
       std::string("Could not load extension from '*'. ") +
-      extension_manifest_errors::kInvalidManifest)) << GetErrors()[1];
+      extension_manifest_errors::kManifestUnreadable)) << GetErrors()[1];
 
   EXPECT_TRUE(MatchPattern(GetErrors()[2],
       std::string("Could not load extension from '*'. ") +
@@ -619,7 +619,7 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) {
 
   EXPECT_TRUE(MatchPattern(GetErrors()[3],
       std::string("Could not load extension from '*'. ") +
-      extension_manifest_errors::kInvalidManifest)) << GetErrors()[3];
+      extension_manifest_errors::kManifestUnreadable)) << GetErrors()[3];
 };
 
 // Test that partially deleted extensions are cleaned up during startup
diff --git a/chrome/common/extensions/extension_constants.cc b/chrome/common/extensions/extension_constants.cc
index 228197b..b269848 100644
--- a/chrome/common/extensions/extension_constants.cc
+++ b/chrome/common/extensions/extension_constants.cc
@@ -82,7 +82,7 @@ const char* kInvalidJsList =
 const char* kInvalidKey =
     "Value 'key' is missing or invalid.";
 const char* kInvalidManifest =
-    "Manifest is missing or invalid.";
+    "Manifest file is invalid.";
 const char* kInvalidMatchCount =
     "Invalid value for 'content_scripts[*].matches. There must be at least one "
     "match specified.";
@@ -142,6 +142,10 @@ const char* kInvalidVersion =
     "dot-separated integers.";
 const char* kInvalidZipHash =
     "Required key 'zip_hash' is missing or invalid.";
+const char* kManifestParseError =
+    "Manifest is not valid JSON.";
+const char* kManifestUnreadable =
+    "Manifest file is missing or unreadable.";
 const char* kMissingFile =
     "At least one js or css file is required for 'content_scripts[*]'.";
 const char* kInvalidTheme =
diff --git a/chrome/common/extensions/extension_constants.h b/chrome/common/extensions/extension_constants.h
index 125a47f..61f8092 100644
--- a/chrome/common/extensions/extension_constants.h
+++ b/chrome/common/extensions/extension_constants.h
@@ -104,6 +104,8 @@ namespace extension_manifest_errors {
   extern const char* kInvalidThemeColors;
   extern const char* kInvalidThemeTints;
   extern const char* kThemesCannotContainExtensions;
+  extern const char* kManifestParseError;
+  extern const char* kManifestUnreadable;
   extern const char* kMissingFile;
   extern const char* kInvalidUpdateURL;
   extern const char* kInvalidDefaultLocale;
diff --git a/chrome/test/data/extensions/bad/Extensions/dddddddddddddddddddddddddddddddd/1.0/git_hates_empty_dirs b/chrome/test/data/extensions/bad/Extensions/dddddddddddddddddddddddddddddddd/1.0/git_hates_empty_dirs
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/chrome/test/data/extensions/bad/Extensions/dddddddddddddddddddddddddddddddd/1.0/git_hates_empty_dirs
diff --git a/chrome/test/data/extensions/bad/Extensions/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/1.0/manifest.json b/chrome/test/data/extensions/bad/Extensions/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/1.0/manifest.json
new file mode 100644
index 0000000..65e7ab28
--- /dev/null
+++ b/chrome/test/data/extensions/bad/Extensions/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/1.0/manifest.json
@@ -0,0 +1,9 @@
+{
+  "version": ""1.0",
+  "name": "My extension 3",
+  "content_scripts": [
+    {
+      "matches": ["http://*/*"]
+    }
+  ]
+}