Reland "JsonSchemaCompiler: Raise error on parse failures of optional properties."

This reverts commit 898910982f.

Reason for revert: The original test failures were caused due to crbug.com/gn/215 which caused spurious failures in 
incremental rebuilds. The corresponding GN roll has now been reverted.

Original change's description:
> Revert "JsonSchemaCompiler: Raise error on parse failures of optional properties."
>
> This reverts commit eec7bff80b.
>
> Reason for revert:
> After this change, JsonSchemaCompilerErrorTest.* are failing on
> several linux bots:
> https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests/95122
> https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests%20%28dbg%29%281%29/92709
> https://ci.chromium.org/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/21165
>
> Original change's description:
> > 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}
>
> TBR=rdevlin.cronin@chromium.org,karandeepb@chromium.org
>
> Change-Id: I3b26905381996192b377f79abcde757efaf08f31
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1113513
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519330
> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
> Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#823902}

TBR=rdevlin.cronin@chromium.org,nhiroki@chromium.org,karandeepb@chromium.org

# Not skipping CQ checks because this is a reland.

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

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"')
@@ -255,24 +256,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);')
@@ -353,15 +345,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_):
@@ -921,8 +916,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);')
@@ -946,12 +940,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>();')
@@ -986,14 +977,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)
@@ -1010,10 +998,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:
@@ -1038,9 +1023,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)));
   }
 }