JsonSchemaCompiler: Raise error on parse failures of optional properties.

Currently when an optional property fails to parse and error generation
is enabled for the schema, the auto-generated Populate function
continues parsing the type but still populates error. This CL changes
the code to ensure that we raise an error in these cases.

We also don't raise an error on unrecognized keys any longer since we
shouldn't be raising an error while returning a parse success: Most
consumers currently don't distinguish between the two cases and this can
lead to the returned error being a concatenation of unrelated errors,
which is not ideal.

We now enforce the constraint that Populate only returns an error iff
there is a parse failure. We DCHECK the same in the auto-generated
FromValue function.

This also brings the code more in-line with what we do for
auto-generated manifest parsing.

Currently only 3 schemas use error generation:
  - declarative_net_request.idl
  - extensions_manifest_types.json
  - manifest_types.json

The latter two may be affected by the change. In particular, parsing
manifest types declared by them may lead to a hard error when an
optional property can't be parsed now.

BUG=1113513

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

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

@@ -673,12 +673,17 @@ TEST_P(SingleRulesetTest, InvalidJSONRules_Parsed) {
   SetRules(base::JSONReader::ReadDeprecated(kRules));
 
   extension_loader()->set_ignore_manifest_warnings(true);
-  LoadAndExpectSuccess(1u);
+
+  // Rules with ids "2" and "3" will be successfully indexed. Note that for rule
+  // with id "3", the "invalidKey" will simply be ignored during parsing
+  // (without causing any install warning).
+  size_t expected_rule_count = 2;
+  LoadAndExpectSuccess(expected_rule_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());
+    ASSERT_EQ(2u, extension()->install_warnings().size());
     std::vector<InstallWarning> expected_warnings;
 
     expected_warnings.emplace_back(
@@ -687,12 +692,6 @@ TEST_P(SingleRulesetTest, InvalidJSONRules_Parsed) {
             "'condition': expected dictionary, got list"),
         dnr_api::ManifestKeys::kDeclarativeNetRequest,
         dnr_api::DNRInfo::kRuleResources);
-    expected_warnings.emplace_back(
-        ErrorUtils::FormatErrorMessage(
-            GetErrorWithFilename(kRuleNotParsedWarning), "id 3",
-            "found unexpected key 'invalidKey'"),
-        dnr_api::ManifestKeys::kDeclarativeNetRequest,
-        dnr_api::DNRInfo::kRuleResources);
     expected_warnings.emplace_back(
         ErrorUtils::FormatErrorMessage(
             GetErrorWithFilename(kRuleNotParsedWarning), "index 4",

extensions/browser/api/declarative_net_request/ruleset_source.cc

@@ -148,8 +148,8 @@ ReadJSONRulesResult ParseRulesFromJSON(const RulesetID& ruleset_id,
     dnr_api::Rule parsed_rule;
     base::string16 parse_error;
 
-    if (dnr_api::Rule::Populate(rules_list[i], &parsed_rule, &parse_error) &&
-        parse_error.empty()) {
+    if (dnr_api::Rule::Populate(rules_list[i], &parsed_rule, &parse_error)) {
+      DCHECK(parse_error.empty());
       if (result.rules.size() == rule_limit) {
         result.rule_parse_warnings.push_back(
             CreateInstallWarning(json_path, kRuleCountExceeded));

extensions/common/manifest_handlers/options_page_info.cc

@@ -125,16 +125,10 @@ std::unique_ptr<OptionsPageInfo> OptionsPageInfo::Create(
 
     std::unique_ptr<OptionsUI> options_ui =
         OptionsUI::FromValue(*options_ui_value, &options_ui_error);
-    if (!options_ui_error.empty()) {
-      // OptionsUI::FromValue populates |error| both when there are
-      // errors (in which case |options_ui| will be NULL) and warnings
-      // (in which case |options_ui| will be valid). Either way, show it
-      // as an install warning.
+    if (!options_ui) {
       install_warnings->push_back(
           InstallWarning(base::UTF16ToASCII(options_ui_error)));
-    }
-
-    if (options_ui) {
+    } else {
       base::string16 options_parse_error;
       if (!ParseOptionsUrl(extension,
                            options_ui->page,

tools/json_schema_compiler/cc_generator.py

@@ -43,6 +43,7 @@ class _Generator(object):
       .Append()
       .Append(self._util_cc_helper.GetIncludePath())
       .Append('#include "base/check.h"')
+      .Append('#include "base/check_op.h"')
       .Append('#include "base/notreached.h"')
       .Append('#include "base/strings/string_number_conversions.h"')
       .Append('#include "base/strings/utf_string_conversions.h"')
@@ -337,24 +338,15 @@ class _Generator(object):
       if type_.properties or type_.additional_properties is not None:
         c.Append('const auto* dict = '
                      'static_cast<const base::DictionaryValue*>(&value);')
-        if self._generate_error_messages:
-          c.Append('std::set<std::string> keys;')
+
+      # TODO(crbug.com/1145154): The generated code here will ignore
+      # unrecognized keys, but the parsing code for types passed to APIs in the
+      # renderer will hard-error on them. We should probably be consistent with
+      # the renderer here (at least for types also parsed in the renderer).
       for prop in type_.properties.values():
         c.Concat(self._InitializePropertyToDefault(prop, 'out'))
       for prop in type_.properties.values():
-        if self._generate_error_messages:
-          c.Append('keys.insert("%s");' % (prop.name))
         c.Concat(self._GenerateTypePopulateProperty(prop, 'dict', 'out'))
-      # Check for extra values.
-      if self._generate_error_messages:
-        (c.Sblock('for (base::DictionaryValue::Iterator it(*dict); '
-                       '!it.IsAtEnd(); it.Advance()) {')
-          .Sblock('if (!keys.count(it.key())) {')
-          .Concat(self._GenerateError('"found unexpected key \'" + '
-                                          'it.key() + "\'"'))
-          .Eblock('}')
-          .Eblock('}')
-        )
       if type_.additional_properties is not None:
         if type_.additional_properties.property_type == PropertyType.ANY:
           c.Append('out->additional_properties.MergeDictionary(dict);')
@@ -435,15 +427,18 @@ class _Generator(object):
       .Append('std::unique_ptr<%s> %s::FromValue(%s) {' % (classname,
         cpp_namespace, self._GenerateParams(('const base::Value& value',))))
     )
+    c.Sblock();
     if self._generate_error_messages:
       c.Append('DCHECK(error);')
-    (c.Append('  auto out = std::make_unique<%s>();' % classname)
-      .Append('  if (!Populate(%s))' % self._GenerateArgs(
-          ('value', 'out.get()')))
-      .Append('    return nullptr;')
-      .Append('  return out;')
-      .Append('}')
-    )
+    c.Append('auto out = std::make_unique<%s>();' % classname)
+    c.Append('bool result = Populate(%s);' %
+      self._GenerateArgs(('value', 'out.get()')))
+    if self._generate_error_messages:
+      c.Append('DCHECK_EQ(result, error->empty());')
+    c.Sblock('if (!result)')
+    c.Append('return nullptr;')
+    c.Eblock('return out;')
+    c.Eblock('}')
     return c
 
   def _GenerateTypeToValue(self, cpp_namespace, type_):
@@ -1003,8 +998,7 @@ class _Generator(object):
                 self._util_cc_helper.GetValueTypeString(
                     '%%(src_var)s', True)))))
         c.Append('%(dst_var)s.reset();')
-        if not self._generate_error_messages:
-          c.Append('return %(failure_value)s;')
+        c.Append('return %(failure_value)s;')
         (c.Eblock('}')
           .Append('else')
           .Append('  %(dst_var)s = std::make_unique<%(cpp_type)s>(temp);')
@@ -1028,12 +1022,9 @@ class _Generator(object):
           .Sblock('if (!%(src_var)s->GetAsDictionary(&dictionary)) {')
           .Concat(self._GenerateError(
             '"\'%%(key)s\': expected dictionary, got " + ' +
-            self._util_cc_helper.GetValueTypeString('%%(src_var)s', True))))
-        # If an optional property fails to populate, the population can still
-        # succeed with a warning. If no error messages are generated, this
-        # warning is not set and we fail out instead.
-        if not self._generate_error_messages:
-          c.Append('return %(failure_value)s;')
+            self._util_cc_helper.GetValueTypeString('%%(src_var)s', True)))
+          .Append('return %(failure_value)s;')
+        )
         (c.Eblock('}')
           .Sblock('else {')
           .Append('auto temp = std::make_unique<%(cpp_type)s>();')
@@ -1068,14 +1059,11 @@ class _Generator(object):
       # util_cc_helper deals with optional and required arrays
       (c.Append('const base::ListValue* list = nullptr;')
         .Sblock('if (!%(src_var)s->GetAsList(&list)) {')
-          .Concat(self._GenerateError(
-            '"\'%%(key)s\': expected list, got " + ' +
-            self._util_cc_helper.GetValueTypeString('%%(src_var)s', True)))
+        .Concat(self._GenerateError(
+          '"\'%%(key)s\': expected list, got " + ' +
+          self._util_cc_helper.GetValueTypeString('%%(src_var)s', True)))
+        .Append('return %(failure_value)s;')
       )
-      if is_ptr and self._generate_error_messages:
-        c.Append('%(dst_var)s.reset();')
-      else:
-        c.Append('return %(failure_value)s;')
       c.Eblock('}')
       c.Sblock('else {')
       item_type = self._type_helper.FollowRef(underlying_type.item_type)
@@ -1092,10 +1080,7 @@ class _Generator(object):
             self._GenerateArgs(('*list', '&%(dst_var)s'))))
         c.Concat(self._GenerateError(
             '"unable to populate array \'%%(parent_key)s\'"'))
-        if is_ptr and self._generate_error_messages:
-          c.Append('%(dst_var)s.reset();')
-        else:
-          c.Append('return %(failure_value)s;')
+        c.Append('return %(failure_value)s;')
         c.Eblock('}')
       c.Eblock('}')
     elif underlying_type.property_type == PropertyType.CHOICES:
@@ -1120,9 +1105,8 @@ class _Generator(object):
         .Concat(self._GenerateError(
           '"\'%%(key)s\': expected binary, got " + ' +
           self._util_cc_helper.GetValueTypeString('%%(src_var)s', True)))
+        .Append('return %(failure_value)s;')
       )
-      if not self._generate_error_messages:
-        c.Append('return %(failure_value)s;')
       (c.Eblock('}')
         .Sblock('else {')
       )

tools/json_schema_compiler/test/error_generation_unittest.cc

@@ -156,7 +156,7 @@ TEST(JsonSchemaCompilerErrorTest, WrongTypeValueType) {
         Dictionary("otherType", std::make_unique<Value>(1.1));
     errors::ObjectType out;
     base::string16 error;
-    EXPECT_TRUE(errors::ObjectType::Populate(*value, &out, &error));
+    EXPECT_FALSE(errors::ObjectType::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'otherType': expected dictionary, got double",
         error));
     EXPECT_EQ(NULL, out.other_type.get());
@@ -226,9 +226,7 @@ TEST(JsonSchemaCompilerErrorTest, BadEnumValue) {
   }
 }
 
-// Warn but don't fail out errors
-
-TEST(JsonSchemaCompilerErrorTest, WarnOnOptionalFailure) {
+TEST(JsonSchemaCompilerErrorTest, ErrorOnOptionalFailure) {
   {
     std::unique_ptr<base::DictionaryValue> value =
         Dictionary("string", std::make_unique<Value>("bling"));
@@ -241,7 +239,7 @@ TEST(JsonSchemaCompilerErrorTest, WarnOnOptionalFailure) {
 
     errors::OptionalTestType out;
     base::string16 error;
-    EXPECT_TRUE(errors::OptionalTestType::Populate(*value, &out, &error));
+    EXPECT_FALSE(errors::OptionalTestType::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'string': expected string, got integer",
         error));
     EXPECT_EQ(NULL, out.string.get());
@@ -262,7 +260,7 @@ TEST(JsonSchemaCompilerErrorTest, OptionalBinaryTypeFailure) {
 
     errors::OptionalBinaryData out;
     base::string16 error;
-    EXPECT_TRUE(errors::OptionalBinaryData::Populate(*value, &out, &error));
+    EXPECT_FALSE(errors::OptionalBinaryData::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'data': expected binary, got integer",
         error));
     EXPECT_EQ(NULL, out.data.get());
@@ -280,7 +278,7 @@ TEST(JsonSchemaCompilerErrorTest, OptionalArrayTypeFailure) {
         Dictionary("TheArray", std::make_unique<Value>(5));
     errors::ArrayObject out;
     base::string16 error;
-    EXPECT_TRUE(errors::ArrayObject::Populate(*value, &out, &error));
+    EXPECT_FALSE(errors::ArrayObject::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'TheArray': expected list, got integer",
         error));
     EXPECT_EQ(NULL, out.the_array.get());
@@ -300,8 +298,8 @@ TEST(JsonSchemaCompilerErrorTest, OptionalUnableToPopulateArray) {
         List(std::make_unique<Value>(5), std::make_unique<Value>(false));
     errors::OptionalChoiceType::Integers out;
     base::string16 error;
-    EXPECT_TRUE(errors::OptionalChoiceType::Integers::Populate(*params_value,
-                                                               &out, &error));
+    EXPECT_FALSE(errors::OptionalChoiceType::Integers::Populate(*params_value,
+                                                                &out, &error));
     EXPECT_TRUE(EqualsUtf16(
         "expected integer, got boolean; unable to populate array 'integers'",
         error));
@@ -315,12 +313,12 @@ TEST(JsonSchemaCompilerErrorTest, MultiplePopulationErrors) {
         Dictionary("TheArray", std::make_unique<Value>(5));
     errors::ArrayObject out;
     base::string16 error;
-    EXPECT_TRUE(errors::ArrayObject::Populate(*value, &out, &error));
+    EXPECT_FALSE(errors::ArrayObject::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'TheArray': expected list, got integer",
         error));
     EXPECT_EQ(NULL, out.the_array.get());
 
-    EXPECT_TRUE(errors::ArrayObject::Populate(*value, &out, &error));
+    EXPECT_FALSE(errors::ArrayObject::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'TheArray': expected list, got integer; "
         "'TheArray': expected list, got integer",
         error));
@@ -335,10 +333,10 @@ TEST(JsonSchemaCompilerErrorTest, TooManyKeys) {
     EXPECT_TRUE(EqualsUtf16("", GetPopulateError<errors::TestType>(*value)));
   }
   {
+    // We simply ignore extra keys.
     std::unique_ptr<base::DictionaryValue> value =
         Dictionary("string", std::make_unique<Value>("yes"), "ohno",
                    std::make_unique<Value>("many values"));
-    EXPECT_TRUE(EqualsUtf16("found unexpected key 'ohno'",
-                            GetPopulateError<errors::TestType>(*value)));
+    EXPECT_TRUE(EqualsUtf16("", GetPopulateError<errors::TestType>(*value)));
   }
 }