DNR: Improve install warnings.

This CL improves the install warnings for manifest rules which can't be parsed.
To do this enable error generation for the Declarative Net Request API. Another
side effect of this is that rules with unknown keys are now ignored (with an
install warning), instead of being parsed correctly.

Generate code diff- https://www.diffchecker.com/9d55eAZ5

BUG=926609

Change-Id: I695225f8dd8e96366d6462d31414402d0b14c4f4
Reviewed-on: https://chromium-review.googlesource.com/c/1454085
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629788}

chrome/browser/extensions/api/declarative_net_request/rule_indexing_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/json/json_reader.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -312,26 +313,30 @@ TEST_P(RuleIndexingTest, TooManyParseFailures) {
         extension()->install_warnings();
     ASSERT_EQ(1u + kMaxUnparsedRulesWarnings, expected_warnings.size());
 
-    InstallWarning warning(ErrorUtils::FormatErrorMessage(
-        kTooManyParseFailuresWarning,
-        std::to_string(kMaxUnparsedRulesWarnings)));
+    InstallWarning warning("");
     warning.key = manifest_keys::kDeclarativeNetRequestKey;
     warning.specific = manifest_keys::kDeclarativeRuleResourcesKey;
-    EXPECT_EQ(warning, expected_warnings[0]);
 
-    // The subsequent warnings should correspond to the first
+    // The initial warnings should correspond to the first
     // |kMaxUnparsedRulesWarnings| rules, which couldn't be parsed.
     for (size_t i = 0; i < kMaxUnparsedRulesWarnings; i++) {
-      warning.message = ErrorUtils::FormatErrorMessage(kRuleNotParsedWarning,
-                                                       std::to_string(i));
-      EXPECT_EQ(expected_warnings[i + 1], warning);
+      warning.message = ErrorUtils::FormatErrorMessage(
+          kRuleNotParsedWarning, std::to_string(i),
+          "'RuleActionType': expected \"block\" or \"redirect\" or \"allow\", "
+          "got \"invalid_action_type\"");
+      EXPECT_EQ(expected_warnings[i], warning);
     }
+
+    warning.message = ErrorUtils::FormatErrorMessage(
+        kTooManyParseFailuresWarning,
+        std::to_string(kMaxUnparsedRulesWarnings));
+    EXPECT_EQ(warning, expected_warnings[kMaxUnparsedRulesWarnings]);
   }
 }
 
 // Ensures that rules which can't be parsed are ignored and cause an install
 // warning.
-TEST_P(RuleIndexingTest, InvalidJSONRule) {
+TEST_P(RuleIndexingTest, InvalidJSONRules_StrongTypes) {
   {
     TestRule rule = CreateGenericRule();
     rule.id = 1;
@@ -368,11 +373,76 @@ TEST_P(RuleIndexingTest, InvalidJSONRule) {
     std::vector<InstallWarning> expected_warnings;
 
     expected_warnings.emplace_back(
-        ErrorUtils::FormatErrorMessage(kRuleNotParsedWarning, "1"),
+        ErrorUtils::FormatErrorMessage(
+            kRuleNotParsedWarning, "1",
+            "'RuleActionType': expected \"block\" or \"redirect\" or \"allow\","
+            " got \"invalid action\""),
+        manifest_keys::kDeclarativeNetRequestKey,
+        manifest_keys::kDeclarativeRuleResourcesKey);
+    expected_warnings.emplace_back(
+        ErrorUtils::FormatErrorMessage(
+            kRuleNotParsedWarning, "3",
+            "'DomainType': expected \"firstParty\" or \"thirdParty\", got "
+            "\"invalid_domain_type\""),
+        manifest_keys::kDeclarativeNetRequestKey,
+        manifest_keys::kDeclarativeRuleResourcesKey);
+    EXPECT_EQ(expected_warnings, extension()->install_warnings());
+  }
+}
+
+// Ensures that rules which can't be parsed are ignored and cause an install
+// warning.
+TEST_P(RuleIndexingTest, InvalidJSONRules_Parsed) {
+  const char* kRules = R"(
+    [
+      {
+        "id" : 1,
+        "condition" : [],
+        "action" : {"type" : "block" }
+      },
+      {
+        "id" : 2,
+        "condition" : {"urlFilter" : "abc"},
+        "action" : {"type" : "block" }
+      },
+      {
+        "id" : 3,
+        "invalidKey" : "invalidKeyValue",
+        "condition" : {"urlFilter" : "example"},
+        "action" : {"type" : "block" }
+      },
+      {
+        "id" : "4",
+        "condition" : {"urlFilter" : "google"},
+        "action" : {"type" : "block" }
+      }
+    ]
+  )";
+  SetRules(base::JSONReader::Read(kRules));
+
+  extension_loader()->set_ignore_manifest_warnings(true);
+  LoadAndExpectSuccess(1 /* rules count */);
+
+  // TODO(crbug.com/879355): CrxInstaller reloads the extension after moving it,
+  // which causes it to lose the install warning. This should be fixed.
+  if (GetParam() != ExtensionLoadType::PACKED) {
+    ASSERT_EQ(3u, extension()->install_warnings().size());
+    std::vector<InstallWarning> expected_warnings;
+
+    expected_warnings.emplace_back(
+        ErrorUtils::FormatErrorMessage(
+            kRuleNotParsedWarning, "0",
+            "'condition': expected dictionary, got list"),
+        manifest_keys::kDeclarativeNetRequestKey,
+        manifest_keys::kDeclarativeRuleResourcesKey);
+    expected_warnings.emplace_back(
+        ErrorUtils::FormatErrorMessage(kRuleNotParsedWarning, "2",
+                                       "found unexpected key 'invalidKey'"),
         manifest_keys::kDeclarativeNetRequestKey,
         manifest_keys::kDeclarativeRuleResourcesKey);
     expected_warnings.emplace_back(
-        ErrorUtils::FormatErrorMessage(kRuleNotParsedWarning, "3"),
+        ErrorUtils::FormatErrorMessage(kRuleNotParsedWarning, "3",
+                                       "'id': expected id, got string"),
         manifest_keys::kDeclarativeNetRequestKey,
         manifest_keys::kDeclarativeRuleResourcesKey);
     EXPECT_EQ(expected_warnings, extension()->install_warnings());

extensions/browser/api/declarative_net_request/constants.cc

@@ -35,7 +35,8 @@ const char kErrorNonAscii[] =
 const char kRuleCountExceeded[] =
     "Declarative Net Request: Rule count exceeded. Some rules were ignored.";
 const char kRuleNotParsedWarning[] =
-    "Declarative Net Request: Rule at index * couldn't be parsed.";
+    "Declarative Net Request: Rule at index * couldn't be parsed. Parse error: "
+    "*.";
 const char kTooManyParseFailuresWarning[] =
     "Declarative Net Request: Too many rule parse failures; Reporting the "
     "first *.";

extensions/browser/api/declarative_net_request/declarative_net_request_api.cc

@@ -132,9 +132,13 @@ ExtensionFunction::ResponseAction
 DeclarativeNetRequestAddAllowedPagesFunction::Run() {
   using Params = api::declarative_net_request::AddAllowedPages::Params;
 
-  std::unique_ptr<Params> params(Params::Create(*args_));
+  base::string16 error;
+  std::unique_ptr<Params> params(Params::Create(*args_, &error));
   EXTENSION_FUNCTION_VALIDATE(params);
 
+  // EXTENSION_FUNCTION_VALIDATE should validate that the arguments are in the
+  // correct format. Ignore |error|.
+
   return UpdateAllowedPages(params->page_patterns, Action::ADD);
 }
 
@@ -147,9 +151,13 @@ ExtensionFunction::ResponseAction
 DeclarativeNetRequestRemoveAllowedPagesFunction::Run() {
   using Params = api::declarative_net_request::AddAllowedPages::Params;
 
-  std::unique_ptr<Params> params(Params::Create(*args_));
+  base::string16 error;
+  std::unique_ptr<Params> params(Params::Create(*args_, &error));
   EXTENSION_FUNCTION_VALIDATE(params);
 
+  // EXTENSION_FUNCTION_VALIDATE should validate that the arguments are in the
+  // correct format. Ignore |error|.
+
   return UpdateAllowedPages(params->page_patterns, Action::REMOVE);
 }
 

extensions/browser/api/declarative_net_request/utils.cc

@@ -19,6 +19,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/timer/elapsed_timer.h"
 #include "base/values.h"
 #include "components/url_pattern_index/url_pattern_index.h"
@@ -164,9 +165,8 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
 
   // Limit the maximum number of rule unparsed warnings to 5.
   const size_t kMaxUnparsedRulesWarnings = 5;
-  std::vector<int> unparsed_indices;
-  unparsed_indices.reserve(kMaxUnparsedRulesWarnings);
   bool unparsed_warnings_limit_exeeded = false;
+  size_t unparsed_warning_count = 0;
 
   base::ElapsedTimer timer;
   {
@@ -175,16 +175,22 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
 
     const auto& rules_list = rules.GetList();
     for (size_t i = 0; i < rules_list.size(); i++) {
-      parsed_rule = dnr_api::Rule::FromValue(rules_list[i]);
+      base::string16 parse_error;
+      parsed_rule = dnr_api::Rule::FromValue(rules_list[i], &parse_error);
 
       // Ignore rules which can't be successfully parsed and show an install
       // warning for them. A hard error is not thrown to maintain backwards
       // compatibility.
-      if (!parsed_rule) {
-        if (unparsed_indices.size() < kMaxUnparsedRulesWarnings)
-          unparsed_indices.push_back(i);
-        else
+      if (!parsed_rule || !parse_error.empty()) {
+        if (unparsed_warning_count < kMaxUnparsedRulesWarnings) {
+          ++unparsed_warning_count;
+          warnings->push_back(
+              CreateInstallWarning(ErrorUtils::FormatErrorMessage(
+                  kRuleNotParsedWarning, std::to_string(i),
+                  base::UTF16ToUTF8(parse_error))));
+        } else {
           unparsed_warnings_limit_exeeded = true;
+        }
         continue;
       }
 
@@ -216,17 +222,12 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
     warnings->push_back(CreateInstallWarning(kRuleCountExceeded));
 
   if (unparsed_warnings_limit_exeeded) {
-    DCHECK_EQ(kMaxUnparsedRulesWarnings, unparsed_indices.size());
+    DCHECK_EQ(kMaxUnparsedRulesWarnings, unparsed_warning_count);
     warnings->push_back(CreateInstallWarning(ErrorUtils::FormatErrorMessage(
         kTooManyParseFailuresWarning,
         std::to_string(kMaxUnparsedRulesWarnings))));
   }
 
-  for (int rule_index : unparsed_indices) {
-    warnings->push_back(CreateInstallWarning(ErrorUtils::FormatErrorMessage(
-        kRuleNotParsedWarning, std::to_string(rule_index))));
-  }
-
   UMA_HISTOGRAM_TIMES(kIndexAndPersistRulesTimeHistogram, timer.Elapsed());
   UMA_HISTOGRAM_COUNTS_100000(kManifestRulesCountHistogram,
                               indexer.indexed_rules_count());

extensions/common/api/declarative_net_request.idl

@@ -4,6 +4,7 @@
 
 // The <code>chrome.declarativeNetRequest</code> API is used to block or
 // redirect network requests by specifying declarative rules.
+[generate_error_messages]
 namespace declarativeNetRequest {
   // This describes the resource type of the network request.
   enum ResourceType {