Catch ManifestErrors with ErrorConsole; Add Browsertests

Add backend support for catching manifest install warnings with the ErrorConsole.  Forked from https://codereview.chromium.org/22938005 in order to reduce size and separate backend and frontend CLs.

BUG=21734

Review URL: https://chromiumcodereview.appspot.com/23684015

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

13 files changed, 227 insertions, 19 deletions

diff --git a/chrome/browser/extensions/error_console/error_console.cc b/chrome/browser/extensions/error_console/error_console.cc
index 2b0432f..b0193b3 100644
--- a/chrome/browser/extensions/error_console/error_console.cc
+++ b/chrome/browser/extensions/error_console/error_console.cc
@@ -5,16 +5,20 @@
 #include "chrome/browser/extensions/error_console/error_console.h"
 
 #include <list>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/lazy_instance.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_set.h"
 #include "chrome/common/extensions/feature_switch.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_details.h"
@@ -42,6 +46,21 @@ void DeleteIncognitoErrorsFromList(ErrorConsole::ErrorList* list) {
   }
 }
 
+// Iterate through an error list and remove and delete all errors of a given
+// |type|.
+void DeleteErrorsOfTypeFromList(ErrorConsole::ErrorList* list,
+                                ExtensionError::Type type) {
+  ErrorConsole::ErrorList::iterator iter = list->begin();
+  while (iter != list->end()) {
+    if ((*iter)->type() == type) {
+      delete *iter;
+      iter = list->erase(iter);
+    } else {
+      ++iter;
+    }
+  }
+}
+
 base::LazyInstance<ErrorConsole::ErrorList> g_empty_error_list =
     LAZY_INSTANCE_INITIALIZER;
 
@@ -50,8 +69,9 @@ base::LazyInstance<ErrorConsole::ErrorList> g_empty_error_list =
 void ErrorConsole::Observer::OnErrorConsoleDestroyed() {
 }
 
-ErrorConsole::ErrorConsole(Profile* profile) : enabled_(false),
-                                               profile_(profile) {
+ErrorConsole::ErrorConsole(Profile* profile,
+                           ExtensionService* extension_service)
+     : enabled_(false), profile_(profile) {
 // TODO(rdevlin.cronin): Remove once crbug.com/159265 is fixed.
 #if !defined(ENABLE_EXTENSIONS)
   return;
@@ -69,7 +89,7 @@ ErrorConsole::ErrorConsole(Profile* profile) : enabled_(false),
                                  base::Unretained(this)));
 
   if (profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode))
-    Enable();
+    Enable(extension_service);
 }
 
 ErrorConsole::~ErrorConsole() {
@@ -141,12 +161,12 @@ void ErrorConsole::OnPrefChanged() {
       profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode);
 
   if (developer_mode && !enabled_)
-    Enable();
+    Enable(ExtensionSystem::Get(profile_)->extension_service());
   else if (!developer_mode && enabled_)
     Disable();
 }
 
-void ErrorConsole::Enable() {
+void ErrorConsole::Enable(ExtensionService* extension_service) {
   enabled_ = true;
 
   notification_registrar_.Add(
@@ -157,6 +177,19 @@ void ErrorConsole::Enable() {
       this,
       chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
       content::Source<Profile>(profile_));
+  notification_registrar_.Add(
+      this,
+      chrome::NOTIFICATION_EXTENSION_INSTALLED,
+      content::Source<Profile>(profile_));
+
+  if (extension_service) {
+    // Get manifest errors for extensions already installed.
+    const ExtensionSet* extensions = extension_service->extensions();
+    for (ExtensionSet::const_iterator iter = extensions->begin();
+         iter != extensions->end(); ++iter) {
+      AddManifestErrorsForExtension(iter->get());
+    }
+  }
 }
 
 void ErrorConsole::Disable() {
@@ -165,6 +198,19 @@ void ErrorConsole::Disable() {
   enabled_ = false;
 }
 
+void ErrorConsole::AddManifestErrorsForExtension(const Extension* extension) {
+  const std::vector<InstallWarning>& warnings =
+      extension->install_warnings();
+  for (std::vector<InstallWarning>::const_iterator iter = warnings.begin();
+       iter != warnings.end(); ++iter) {
+    ReportError(scoped_ptr<ExtensionError>(new ManifestError(
+        extension->id(),
+        base::UTF8ToUTF16(iter->message),
+        base::UTF8ToUTF16(iter->key),
+        base::UTF8ToUTF16(iter->specific))));
+  }
+}
+
 void ErrorConsole::RemoveIncognitoErrors() {
   for (ErrorMap::iterator iter = errors_.begin();
        iter != errors_.end(); ++iter) {
@@ -204,6 +250,23 @@ void ErrorConsole::Observe(int type,
       RemoveErrorsForExtension(
           content::Details<Extension>(details).ptr()->id());
       break;
+    case chrome::NOTIFICATION_EXTENSION_INSTALLED: {
+      const InstalledExtensionInfo* info =
+          content::Details<InstalledExtensionInfo>(details).ptr();
+
+      // We don't want to have manifest errors from previous installs. We want
+      // to keep runtime errors, though, because extensions are reloaded on a
+      // refresh of chrome:extensions, and we don't want to wipe our history
+      // whenever that happens.
+      ErrorMap::iterator iter = errors_.find(info->extension->id());
+      if (iter != errors_.end()) {
+        DeleteErrorsOfTypeFromList(&(iter->second),
+                                   ExtensionError::MANIFEST_ERROR);
+      }
+
+      AddManifestErrorsForExtension(info->extension);
+      break;
+    }
     default:
       NOTREACHED();
   }
diff --git a/chrome/browser/extensions/error_console/error_console.h b/chrome/browser/extensions/error_console/error_console.h
index 6c8ebb5..45e9c00 100644
--- a/chrome/browser/extensions/error_console/error_console.h
+++ b/chrome/browser/extensions/error_console/error_console.h
@@ -24,10 +24,12 @@ class NotificationSource;
 class RenderViewHost;
 }
 
+class ExtensionService;
 class Profile;
 
 namespace extensions {
 class ErrorConsoleUnitTest;
+class Extension;
 
 // The ErrorConsole is a central object to which all extension errors are
 // reported. This includes errors detected in extensions core, as well as
@@ -49,7 +51,7 @@ class ErrorConsole : public content::NotificationObserver {
     virtual void OnErrorConsoleDestroyed();
   };
 
-  explicit ErrorConsole(Profile* profile);
+  explicit ErrorConsole(Profile* profile, ExtensionService* extension_service);
   virtual ~ErrorConsole();
 
   // Convenience method to return the ErrorConsole for a given profile.
@@ -72,8 +74,9 @@ class ErrorConsole : public content::NotificationObserver {
  private:
   FRIEND_TEST_ALL_PREFIXES(ErrorConsoleUnitTest, AddAndRemoveErrors);
 
-  // Enable the error console for error collection and retention.
-  void Enable();
+  // Enable the error console for error collection and retention. This involves
+  // subscribing to the appropriate notifications and fetching manifest errors.
+  void Enable(ExtensionService* extension_service);
   // Disable the error console, removing the subscriptions to notifications and
   // removing all current errors.
   void Disable();
@@ -83,6 +86,9 @@ class ErrorConsole : public content::NotificationObserver {
   // not.
   void OnPrefChanged();
 
+  // Add manifest errors from an extension's install warnings.
+  void AddManifestErrorsForExtension(const Extension* extension);
+
   // Remove all errors which happened while incognito; we have to do this once
   // the incognito profile is destroyed.
   void RemoveIncognitoErrors();
diff --git a/chrome/browser/extensions/error_console/error_console_browsertest.cc b/chrome/browser/extensions/error_console/error_console_browsertest.cc
index 1cdbf62..455eb5d 100644
--- a/chrome/browser/extensions/error_console/error_console_browsertest.cc
+++ b/chrome/browser/extensions/error_console/error_console_browsertest.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/error_console/error_console.h"
 
+#include "base/files/file_path.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
@@ -18,6 +19,8 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "extensions/browser/extension_error.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/error_utils.h"
+#include "extensions/common/manifest_constants.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -98,6 +101,26 @@ void CheckRuntimeError(const ExtensionError* error,
   EXPECT_EQ(expected_stack_size, runtime_error->stack_trace().size());
 }
 
+void CheckManifestError(const ExtensionError* error,
+                        const std::string& id,
+                        const std::string& message,
+                        const std::string& manifest_key,
+                        const std::string& manifest_specific) {
+  CheckError(error,
+             ExtensionError::MANIFEST_ERROR,
+             id,
+             // source is always the manifest for ManifestErrors.
+             base::FilePath(kManifestFilename).AsUTF8Unsafe(),
+             false,  // manifest errors are never from incognito.
+             message);
+
+  const ManifestError* manifest_error =
+      static_cast<const ManifestError*>(error);
+  EXPECT_EQ(UTF8ToUTF16(manifest_key), manifest_error->manifest_key());
+  EXPECT_EQ(UTF8ToUTF16(manifest_specific),
+            manifest_error->manifest_specific());
+}
+
 }  // namespace
 
 class ErrorConsoleBrowserTest : public ExtensionBrowserTest {
@@ -250,6 +273,83 @@ class ErrorConsoleBrowserTest : public ExtensionBrowserTest {
   ErrorConsole* error_console_;
 };
 
+// Test to ensure that we are successfully reporting manifest errors as an
+// extension is installed.
+IN_PROC_BROWSER_TEST_F(ErrorConsoleBrowserTest, ReportManifestErrors) {
+  const Extension* extension = NULL;
+  // We expect two errors - one for an invalid permission, and a second for
+  // an unknown key.
+  LoadExtensionAndCheckErrors("manifest_warnings",
+                              ExtensionBrowserTest::kFlagIgnoreManifestWarnings,
+                              2,
+                              ACTION_NONE,
+                              &extension);
+
+  const ErrorConsole::ErrorList& errors =
+      error_console()->GetErrorsForExtension(extension->id());
+
+  // Unfortunately, there's not always a hard guarantee of order in parsing the
+  // manifest, so there's not a definitive order in which these errors may
+  // occur. As such, we need to determine which error corresponds to which
+  // expected error.
+  const ExtensionError* permissions_error = NULL;
+  const ExtensionError* unknown_key_error = NULL;
+  const char kFakeKey[] = "not_a_real_key";
+  for (size_t i = 0; i < errors.size(); ++i) {
+    ASSERT_EQ(ExtensionError::MANIFEST_ERROR, errors[i]->type());
+    std::string utf8_key = UTF16ToUTF8(
+        (static_cast<const ManifestError*>(errors[i]))->manifest_key());
+    if (utf8_key == manifest_keys::kPermissions)
+      permissions_error = errors[i];
+    else if (utf8_key == kFakeKey)
+      unknown_key_error = errors[i];
+  }
+  ASSERT_TRUE(permissions_error);
+  ASSERT_TRUE(unknown_key_error);
+
+  const char kFakePermission[] = "not_a_real_permission";
+  CheckManifestError(permissions_error,
+                     extension->id(),
+                     ErrorUtils::FormatErrorMessage(
+                         manifest_errors::kPermissionUnknownOrMalformed,
+                         kFakePermission),
+                     manifest_keys::kPermissions,
+                     kFakePermission);
+
+  CheckManifestError(unknown_key_error,
+                     extension->id(),
+                     ErrorUtils::FormatErrorMessage(
+                         manifest_errors::kUnrecognizedManifestKey,
+                         kFakeKey),
+                     kFakeKey,
+                     EmptyString());
+}
+
+// Test that we do not store any errors unless the Developer Mode switch is
+// toggled on the profile.
+IN_PROC_BROWSER_TEST_F(ErrorConsoleBrowserTest,
+                       DontStoreErrorsWithoutDeveloperMode) {
+  profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, false);
+
+  const Extension* extension = NULL;
+  // Same test as ReportManifestErrors, except we don't expect any errors since
+  // we disable Developer Mode.
+  LoadExtensionAndCheckErrors("manifest_warnings",
+                              ExtensionBrowserTest::kFlagIgnoreManifestWarnings,
+                              0,
+                              ACTION_NONE,
+                              &extension);
+
+  // Now if we enable developer mode, the errors should be reported...
+  profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, true);
+  EXPECT_EQ(2u, error_console()->GetErrorsForExtension(extension->id()).size());
+
+  // ... and if we disable it again, all errors which we were holding should be
+  // removed.
+  profile()->GetPrefs()->SetBoolean(prefs::kExtensionsUIDeveloperMode, false);
+  EXPECT_EQ(0u, error_console()->GetErrorsForExtension(extension->id()).size());
+}
+
 // Load an extension which, upon visiting any page, first sends out a console
 // log, and then crashes with a JS TypeError.
 IN_PROC_BROWSER_TEST_F(ErrorConsoleBrowserTest,
diff --git a/chrome/browser/extensions/extension_system.cc b/chrome/browser/extensions/extension_system.cc
index 0b75800..801a4f7 100644
--- a/chrome/browser/extensions/extension_system.cc
+++ b/chrome/browser/extensions/extension_system.cc
@@ -221,7 +221,7 @@ void ExtensionSystemImpl::Shared::Init(bool extensions_enabled) {
       new ExtensionWarningBadgeService(profile_));
   extension_warning_service_->AddObserver(
       extension_warning_badge_service_.get());
-  error_console_.reset(new ErrorConsole(profile_));
+  error_console_.reset(new ErrorConsole(profile_, extension_service_.get()));
 }
 
 void ExtensionSystemImpl::Shared::Shutdown() {
diff --git a/chrome/browser/extensions/test_extension_system.cc b/chrome/browser/extensions/test_extension_system.cc
index 48078c2..3023808 100644
--- a/chrome/browser/extensions/test_extension_system.cc
+++ b/chrome/browser/extensions/test_extension_system.cc
@@ -34,7 +34,7 @@ TestExtensionSystem::TestExtensionSystem(Profile* profile)
     : profile_(profile),
       value_store_(NULL),
       info_map_(new ExtensionInfoMap()),
-      error_console_(new ErrorConsole(profile)) {
+      error_console_(new ErrorConsole(profile, NULL)) {
 }
 
 TestExtensionSystem::~TestExtensionSystem() {
diff --git a/chrome/common/extensions/extension_manifest_constants.h b/chrome/common/extensions/extension_manifest_constants.h
index bf38691..411c52d 100644
--- a/chrome/common/extensions/extension_manifest_constants.h
+++ b/chrome/common/extensions/extension_manifest_constants.h
@@ -253,10 +253,10 @@ namespace extension_manifest_errors {
   extern const char kMultipleOverrides[];
   extern const char kNoPermissionForMIMETypes[];
   extern const char kNoWildCardsInPaths[];
+  extern const char kOneUISurfaceOnly[];
   extern const char kPermissionMustBeOptional[];
   extern const char kPermissionNotAllowed[];
   extern const char kPermissionNotAllowedInManifest[];
-  extern const char kOneUISurfaceOnly[];
   extern const char kReservedMessageFound[];
   extern const char kScriptBadgeRequiresFlag[];
   extern const char kScriptBadgeIconIgnored[];
diff --git a/chrome/common/extensions/permissions/permissions_data.cc b/chrome/common/extensions/permissions/permissions_data.cc
index 139e4dc..139bb2e 100644
--- a/chrome/common/extensions/permissions/permissions_data.cc
+++ b/chrome/common/extensions/permissions/permissions_data.cc
@@ -23,6 +23,7 @@
 #include "extensions/common/error_utils.h"
 #include "extensions/common/features/feature.h"
 #include "extensions/common/manifest.h"
+#include "extensions/common/manifest_constants.h"
 #include "extensions/common/switches.h"
 #include "extensions/common/url_pattern_set.h"
 #include "extensions/common/user_script.h"
@@ -231,9 +232,9 @@ bool ParseHelper(Extension* extension,
     // It's probably an unknown API permission. Do not throw an error so
     // extensions can retain backwards compatability (http://crbug.com/42742).
     extension->AddInstallWarning(InstallWarning(
-       base::StringPrintf(
-            "Permission '%s' is unknown or URL pattern is malformed.",
-            permission_str.c_str()),
+        ErrorUtils::FormatErrorMessage(
+            manifest_errors::kPermissionUnknownOrMalformed,
+            permission_str),
         key,
         permission_str));
   }
diff --git a/chrome/test/data/extensions/error_console/manifest_warnings/manifest.json b/chrome/test/data/extensions/error_console/manifest_warnings/manifest.json
new file mode 100644
index 0000000..4dcb3ce
--- /dev/null
+++ b/chrome/test/data/extensions/error_console/manifest_warnings/manifest.json
@@ -0,0 +1,9 @@
+{
+  "name": "Error Console Manifest Warnings Test",
+  "manifest_version": 2,
+  "version": "1.1",
+  "not_a_real_key": "not_a_real_value",
+  "permissions": [
+    "tabs", "not_a_real_permission"
+  ]
+}
diff --git a/extensions/browser/extension_error.cc b/extensions/browser/extension_error.cc
index 9b1e95f..b372885 100644
--- a/extensions/browser/extension_error.cc
+++ b/extensions/browser/extension_error.cc
@@ -73,13 +73,17 @@ bool ExtensionError::IsEqual(const ExtensionError* rhs) const {
 }
 
 ManifestError::ManifestError(const std::string& extension_id,
-                             const string16& message)
+                             const string16& message,
+                             const string16& manifest_key,
+                             const string16& manifest_specific)
     : ExtensionError(ExtensionError::MANIFEST_ERROR,
                      extension_id,
                      false,  // extensions can't be installed while incognito.
                      logging::LOG_WARNING,  // All manifest errors are warnings.
                      base::FilePath(kManifestFilename).AsUTF16Unsafe(),
-                     message) {
+                     message),
+      manifest_key_(manifest_key),
+      manifest_specific_(manifest_specific) {
 }
 
 ManifestError::~ManifestError() {
diff --git a/extensions/browser/extension_error.h b/extensions/browser/extension_error.h
index 6ee7791..ceb1504 100644
--- a/extensions/browser/extension_error.h
+++ b/extensions/browser/extension_error.h
@@ -73,13 +73,25 @@ class ExtensionError {
 class ManifestError : public ExtensionError {
  public:
   ManifestError(const std::string& extension_id,
-                const base::string16& message);
+                const base::string16& message,
+                const base::string16& manifest_key,
+                const base::string16& manifest_specific);
   virtual ~ManifestError();
 
   virtual std::string PrintForTest() const OVERRIDE;
+
+  const base::string16& manifest_key() const { return manifest_key_; }
+  const base::string16& manifest_specific() const { return manifest_specific_; }
  private:
   virtual bool IsEqualImpl(const ExtensionError* rhs) const OVERRIDE;
 
+  // If present, this indicates the feature in the manifest which caused the
+  // error.
+  base::string16 manifest_key_;
+  // If present, this is a more-specific location of the error - for instance,
+  // a specific permission which is incorrect, rather than simply "permissions".
+  base::string16 manifest_specific_;
+
   DISALLOW_COPY_AND_ASSIGN(ManifestError);
 };
 
diff --git a/extensions/common/manifest.cc b/extensions/common/manifest.cc
index eaaaf76..85b5d98 100644
--- a/extensions/common/manifest.cc
+++ b/extensions/common/manifest.cc
@@ -160,8 +160,8 @@ bool Manifest::ValidateManifest(
        it.Advance()) {
     if (!provider->GetFeature(it.key())) {
       warnings->push_back(InstallWarning(
-          base::StringPrintf("Unrecognized manifest key '%s'.",
-                             it.key().c_str()),
+          ErrorUtils::FormatErrorMessage(
+              manifest_errors::kUnrecognizedManifestKey, it.key()),
           it.key()));
     }
   }
diff --git a/extensions/common/manifest_constants.cc b/extensions/common/manifest_constants.cc
index 3ba2a63..0402c40 100644
--- a/extensions/common/manifest_constants.cc
+++ b/extensions/common/manifest_constants.cc
@@ -149,4 +149,12 @@ const char kWebURLs[] = "app.urls";
 
 }  // namespace manifest_keys
 
+namespace manifest_errors {
+
+const char kPermissionUnknownOrMalformed[] =
+    "Permission '*' is unknown or URL pattern is malformed.";
+const char kUnrecognizedManifestKey[] = "Unrecognized manifest key '*'.";
+
+}  // namespace manifest_errors
+
 }  // namespace extensions
diff --git a/extensions/common/manifest_constants.h b/extensions/common/manifest_constants.h
index ff4ca85..e08348e 100644
--- a/extensions/common/manifest_constants.h
+++ b/extensions/common/manifest_constants.h
@@ -153,6 +153,11 @@ namespace manifest_keys {
   extern const char kWebURLs[];
 }  // namespace manifest_keys
 
+namespace manifest_errors {
+extern const char kPermissionUnknownOrMalformed[];
+extern const char kUnrecognizedManifestKey[];
+}  // namespace manifest_errors
+
 }  // namespace extensions
 
 #endif  // EXTENSIONS_COMMON_MANIFEST_CONSTANTS_H_