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

This reverts commit eec7bff8.

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/+/2519330Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823902}

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

@@ -673,17 +673,12 @@ TEST_P(SingleRulesetTest, InvalidJSONRules_Parsed) {
   SetRules(base::JSONReader::ReadDeprecated(kRules));
 
   extension_loader()->set_ignore_manifest_warnings(true);
-
-  // 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);
+  LoadAndExpectSuccess(1u);
 
   // 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(2u, extension()->install_warnings().size());
+    ASSERT_EQ(3u, extension()->install_warnings().size());
     std::vector<InstallWarning> expected_warnings;
 
     expected_warnings.emplace_back(
@@ -692,6 +687,12 @@ 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)) {
-      DCHECK(parse_error.empty());
+    if (dnr_api::Rule::Populate(rules_list[i], &parsed_rule, &parse_error) &&
+        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,10 +125,16 @@ std::unique_ptr<OptionsPageInfo> OptionsPageInfo::Create(
 
     std::unique_ptr<OptionsUI> options_ui =
         OptionsUI::FromValue(*options_ui_value, &options_ui_error);
-    if (!options_ui) {
+    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.
       install_warnings->push_back(
           InstallWarning(base::UTF16ToASCII(options_ui_error)));
-    } else {
+    }
+
+    if (options_ui) {
       base::string16 options_parse_error;
       if (!ParseOptionsUrl(extension,
                            options_ui->page,

tools/json_schema_compiler/cc_generator.py

@@ -43,7 +43,6 @@ 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"')
@@ -256,15 +255,24 @@ class _Generator(object):
       if type_.properties or type_.additional_properties is not None:
         c.Append('const auto* dict = '
                      'static_cast<const base::DictionaryValue*>(&value);')
-
-      # 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).
+        if self._generate_error_messages:
+          c.Append('std::set<std::string> keys;')
       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);')
@@ -345,18 +353,15 @@ 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)
-    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('}')
+    (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('}')
+    )
     return c
 
   def _GenerateTypeToValue(self, cpp_namespace, type_):
@@ -916,7 +921,8 @@ class _Generator(object):
                 self._util_cc_helper.GetValueTypeString(
                     '%%(src_var)s', True)))))
         c.Append('%(dst_var)s.reset();')
-        c.Append('return %(failure_value)s;')
+        if not self._generate_error_messages:
+          c.Append('return %(failure_value)s;')
         (c.Eblock('}')
           .Append('else')
           .Append('  %(dst_var)s = std::make_unique<%(cpp_type)s>(temp);')
@@ -940,9 +946,12 @@ 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)))
-          .Append('return %(failure_value)s;')
-        )
+            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;')
         (c.Eblock('}')
           .Sblock('else {')
           .Append('auto temp = std::make_unique<%(cpp_type)s>();')
@@ -977,11 +986,14 @@ 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)))
-        .Append('return %(failure_value)s;')
+          .Concat(self._GenerateError(
+            '"\'%%(key)s\': expected list, got " + ' +
+            self._util_cc_helper.GetValueTypeString('%%(src_var)s', True)))
       )
+      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)
@@ -998,7 +1010,10 @@ class _Generator(object):
             self._GenerateArgs(('*list', '&%(dst_var)s'))))
         c.Concat(self._GenerateError(
             '"unable to populate array \'%%(parent_key)s\'"'))
-        c.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.Eblock('}')
     elif underlying_type.property_type == PropertyType.CHOICES:
@@ -1023,8 +1038,9 @@ 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_FALSE(errors::ObjectType::Populate(*value, &out, &error));
+    EXPECT_TRUE(errors::ObjectType::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'otherType': expected dictionary, got double",
         error));
     EXPECT_EQ(NULL, out.other_type.get());
@@ -226,7 +226,9 @@ TEST(JsonSchemaCompilerErrorTest, BadEnumValue) {
   }
 }
 
-TEST(JsonSchemaCompilerErrorTest, ErrorOnOptionalFailure) {
+// Warn but don't fail out errors
+
+TEST(JsonSchemaCompilerErrorTest, WarnOnOptionalFailure) {
   {
     std::unique_ptr<base::DictionaryValue> value =
         Dictionary("string", std::make_unique<Value>("bling"));
@@ -239,7 +241,7 @@ TEST(JsonSchemaCompilerErrorTest, ErrorOnOptionalFailure) {
 
     errors::OptionalTestType out;
     base::string16 error;
-    EXPECT_FALSE(errors::OptionalTestType::Populate(*value, &out, &error));
+    EXPECT_TRUE(errors::OptionalTestType::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'string': expected string, got integer",
         error));
     EXPECT_EQ(NULL, out.string.get());
@@ -260,7 +262,7 @@ TEST(JsonSchemaCompilerErrorTest, OptionalBinaryTypeFailure) {
 
     errors::OptionalBinaryData out;
     base::string16 error;
-    EXPECT_FALSE(errors::OptionalBinaryData::Populate(*value, &out, &error));
+    EXPECT_TRUE(errors::OptionalBinaryData::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'data': expected binary, got integer",
         error));
     EXPECT_EQ(NULL, out.data.get());
@@ -278,7 +280,7 @@ TEST(JsonSchemaCompilerErrorTest, OptionalArrayTypeFailure) {
         Dictionary("TheArray", std::make_unique<Value>(5));
     errors::ArrayObject out;
     base::string16 error;
-    EXPECT_FALSE(errors::ArrayObject::Populate(*value, &out, &error));
+    EXPECT_TRUE(errors::ArrayObject::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'TheArray': expected list, got integer",
         error));
     EXPECT_EQ(NULL, out.the_array.get());
@@ -298,8 +300,8 @@ TEST(JsonSchemaCompilerErrorTest, OptionalUnableToPopulateArray) {
         List(std::make_unique<Value>(5), std::make_unique<Value>(false));
     errors::OptionalChoiceType::Integers out;
     base::string16 error;
-    EXPECT_FALSE(errors::OptionalChoiceType::Integers::Populate(*params_value,
-                                                                &out, &error));
+    EXPECT_TRUE(errors::OptionalChoiceType::Integers::Populate(*params_value,
+                                                               &out, &error));
     EXPECT_TRUE(EqualsUtf16(
         "expected integer, got boolean; unable to populate array 'integers'",
         error));
@@ -313,12 +315,12 @@ TEST(JsonSchemaCompilerErrorTest, MultiplePopulationErrors) {
         Dictionary("TheArray", std::make_unique<Value>(5));
     errors::ArrayObject out;
     base::string16 error;
-    EXPECT_FALSE(errors::ArrayObject::Populate(*value, &out, &error));
+    EXPECT_TRUE(errors::ArrayObject::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'TheArray': expected list, got integer",
         error));
     EXPECT_EQ(NULL, out.the_array.get());
 
-    EXPECT_FALSE(errors::ArrayObject::Populate(*value, &out, &error));
+    EXPECT_TRUE(errors::ArrayObject::Populate(*value, &out, &error));
     EXPECT_TRUE(EqualsUtf16("'TheArray': expected list, got integer; "
         "'TheArray': expected list, got integer",
         error));
@@ -333,10 +335,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("", GetPopulateError<errors::TestType>(*value)));
+    EXPECT_TRUE(EqualsUtf16("found unexpected key 'ohno'",
+                            GetPopulateError<errors::TestType>(*value)));
   }
 }