DNR: Fix handling of install warnings for rule resources file.

This CL make the following changes:
- Fixes a potential null-dereference crash when multiple rules couldn't be
  parsed.
- Instead of showing only a single install warning for multiple unparsed rules,
  show individual warnings. Limit the count to 5 warnings.
- The rule count exceeded warning doesn't override the rule unparsed warning
  now.

BUG=894275

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

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

@@ -22,6 +22,7 @@
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/api/declarative_net_request.h"
 #include "extensions/common/api/declarative_net_request/test_utils.h"
+#include "extensions/common/error_utils.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/install_warning.h"
 #include "extensions/common/manifest_constants.h"
@@ -280,27 +281,100 @@ TEST_P(RuleIndexingTest, DuplicateIDS) {
                          .GetErrorDescription(kJSONRulesFilename));
 }
 
+// Ensure that we limit the number of parse failure warnings shown.
+TEST_P(RuleIndexingTest, TooManyParseFailures) {
+  const size_t kNumInvalidRules = 10;
+  const size_t kNumValidRules = 6;
+  const size_t kMaxUnparsedRulesWarnings = 5;
+
+  size_t rule_id = kMinValidID;
+  for (size_t i = 0; i < kNumInvalidRules; i++) {
+    TestRule rule = CreateGenericRule();
+    rule.id = rule_id++;
+    rule.action->type = std::string("invalid_action_type");
+    AddRule(rule);
+  }
+
+  for (size_t i = 0; i < kNumValidRules; i++) {
+    TestRule rule = CreateGenericRule();
+    rule.id = rule_id++;
+    AddRule(rule);
+  }
+
+  extension_loader()->set_ignore_manifest_warnings(true);
+  LoadAndExpectSuccess(kNumValidRules);
+
+  // 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) {
+    const std::vector<InstallWarning>& expected_warnings =
+        extension()->install_warnings();
+    ASSERT_EQ(1u + kMaxUnparsedRulesWarnings, expected_warnings.size());
+
+    InstallWarning warning(ErrorUtils::FormatErrorMessage(
+        kTooManyParseFailuresWarning,
+        std::to_string(kMaxUnparsedRulesWarnings)));
+    warning.key = manifest_keys::kDeclarativeNetRequestKey;
+    warning.specific = manifest_keys::kDeclarativeRuleResourcesKey;
+    EXPECT_EQ(warning, expected_warnings[0]);
+
+    // The subsequent 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);
+    }
+  }
+}
+
 // Ensures that rules which can't be parsed are ignored and cause an install
 // warning.
 TEST_P(RuleIndexingTest, InvalidJSONRule) {
+  {
     TestRule rule = CreateGenericRule();
+    rule.id = 1;
     AddRule(rule);
+  }
 
-  rule.id = kMinValidID + 1;
+  {
+    TestRule rule = CreateGenericRule();
+    rule.id = 2;
     rule.action->type = std::string("invalid action");
     AddRule(rule);
+  }
+
+  {
+    TestRule rule = CreateGenericRule();
+    rule.id = 3;
+    AddRule(rule);
+  }
+
+  {
+    TestRule rule = CreateGenericRule();
+    rule.id = 4;
+    rule.condition->domain_type = std::string("invalid_domain_type");
+    AddRule(rule);
+  }
 
   extension_loader()->set_ignore_manifest_warnings(true);
-  LoadAndExpectSuccess(1 /* rules count */);
+  LoadAndExpectSuccess(2 /* 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(1u, extension()->install_warnings().size());
-    EXPECT_EQ(InstallWarning(kRulesNotParsedWarning,
+    ASSERT_EQ(2u, extension()->install_warnings().size());
+    std::vector<InstallWarning> expected_warnings;
+
+    expected_warnings.emplace_back(
+        ErrorUtils::FormatErrorMessage(kRuleNotParsedWarning, "1"),
         manifest_keys::kDeclarativeNetRequestKey,
-                             manifest_keys::kDeclarativeRuleResourcesKey),
-              extension()->install_warnings()[0]);
+        manifest_keys::kDeclarativeRuleResourcesKey);
+    expected_warnings.emplace_back(
+        ErrorUtils::FormatErrorMessage(kRuleNotParsedWarning, "3"),
+        manifest_keys::kDeclarativeNetRequestKey,
+        manifest_keys::kDeclarativeRuleResourcesKey);
+    EXPECT_EQ(expected_warnings, extension()->install_warnings());
   }
 }
 

extensions/browser/api/declarative_net_request/constants.cc

@@ -34,9 +34,11 @@ const char kErrorNonAscii[] =
 
 const char kRuleCountExceeded[] =
     "Declarative Net Request: Rule count exceeded. Some rules were ignored.";
-const char kRulesNotParsedWarning[] =
-    "Declarative Net Request: Not all rules were successfully parsed.";
-
+const char kRuleNotParsedWarning[] =
+    "Declarative Net Request: Rule at index * couldn't be parsed.";
+const char kTooManyParseFailuresWarning[] =
+    "Declarative Net Request: Too many rule parse failures; Reporting the "
+    "first *.";
 const char kIndexAndPersistRulesTimeHistogram[] =
     "Extensions.DeclarativeNetRequest.IndexAndPersistRulesTime";
 const char kIndexRulesTimeHistogram[] =

extensions/browser/api/declarative_net_request/constants.h

@@ -49,7 +49,8 @@ extern const char kErrorNonAscii[];
 
 // Rule indexing install warnings.
 extern const char kRuleCountExceeded[];
-extern const char kRulesNotParsedWarning[];
+extern const char kRuleNotParsedWarning[];
+extern const char kTooManyParseFailuresWarning[];
 
 // Histogram names.
 extern const char kIndexAndPersistRulesTimeHistogram[];

extensions/browser/api/declarative_net_request/utils.cc

@@ -29,6 +29,7 @@
 #include "extensions/common/api/declarative_net_request.h"
 #include "extensions/common/api/declarative_net_request/dnr_manifest_data.h"
 #include "extensions/common/api/declarative_net_request/utils.h"
+#include "extensions/common/error_utils.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/install_warning.h"
 #include "extensions/common/manifest_constants.h"
@@ -138,6 +139,11 @@ std::string GetJSONRulesetFilename(const Extension& extension) {
   return GetRulesetResource(extension)->GetFilePath().BaseName().AsUTF8Unsafe();
 }
 
+InstallWarning CreateInstallWarning(const std::string& message) {
+  return InstallWarning(message, manifest_keys::kDeclarativeNetRequestKey,
+                        manifest_keys::kDeclarativeRuleResourcesKey);
+}
+
 // Helper function to index |rules| and persist them to the
 // |indexed_ruleset_path|.
 ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
@@ -151,8 +157,16 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
     return ParseInfo(ParseResult::ERROR_LIST_NOT_PASSED);
 
   FlatRulesetIndexer indexer;
-  base::Optional<InstallWarning> install_warning;
-  const size_t rule_count_limit = dnr_api::MAX_NUMBER_OF_RULES;
+
+  const size_t kRuleCountLimit = dnr_api::MAX_NUMBER_OF_RULES;
+  bool rule_count_exceeded = false;
+
+  // 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;
+
   base::ElapsedTimer timer;
   {
     std::set<int> id_set;  // Ensure all ids are distinct.
@@ -163,9 +177,13 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
       parsed_rule = dnr_api::Rule::FromValue(rules_list[i]);
 
       // Ignore rules which can't be successfully parsed and show an install
-      // warning for them.
-      if (!parsed_rule && !install_warning) {
-        install_warning = InstallWarning(kRulesNotParsedWarning);
+      // 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
+          unparsed_warnings_limit_exeeded = true;
         continue;
       }
 
@@ -179,8 +197,8 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
       if (parse_result != ParseResult::SUCCESS)
         return ParseInfo(parse_result, i);
 
-      if (indexer.indexed_rules_count() >= rule_count_limit) {
-        install_warning = InstallWarning(kRuleCountExceeded);
+      if (indexer.indexed_rules_count() >= kRuleCountLimit) {
+        rule_count_exceeded = true;
         break;
       }
 
@@ -193,10 +211,19 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
   if (!PersistRuleset(extension, indexer.GetData(), ruleset_checksum))
     return ParseInfo(ParseResult::ERROR_PERSISTING_RULESET);
 
-  if (install_warning) {
-    install_warning->key = manifest_keys::kDeclarativeNetRequestKey;
-    install_warning->specific = manifest_keys::kDeclarativeRuleResourcesKey;
-    warnings->push_back(std::move(*install_warning));
+  if (rule_count_exceeded)
+    warnings->push_back(CreateInstallWarning(kRuleCountExceeded));
+
+  if (unparsed_warnings_limit_exeeded) {
+    DCHECK_EQ(kMaxUnparsedRulesWarnings, unparsed_indices.size());
+    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());