Commit 6cf2f700 authored by Devlin Cronin's avatar Devlin Cronin Committed by Commit Bot

[Extensions Bindings] Tweak signature parsing

Signature parsing failed in cases where an optional parameter of a
certain type was followed by a required parameter of the same type. The
parsing was greedy, and would always use a parameter if it matched. This
meant that passing a value like `1` to a signature that matched
(optional int, int) would fail, because the `1` would be applied to the
first optional int, and the second int would be missing.

Tweak signature parsing to find the signature that matches the supplied
arguments (if any possible signature matches) and resolve the arguments
before performing parsing.

Update unittests for the same.

Bug: 818555

Change-Id: I57c9909ff73505467fb9d333690113bea8eea433
Reviewed-on: https://chromium-review.googlesource.com/993532Reviewed-by: default avatarIstiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: default avatarJeremy Roman <jbroman@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549527}
parent 25fcb272
......@@ -379,10 +379,7 @@ TEST_F(APIBindingJSUtilUnittest, TestValidateCustomSignature) {
// Test a failing case (prop1 is supposed to be a string).
std::string expected_error =
"Uncaught TypeError: " +
api_errors::ArgumentError(
"", api_errors::InvalidType(api_errors::kTypeString,
api_errors::kTypeInteger));
"Uncaught TypeError: " + api_errors::NoMatchingSignature();
call_validate_signature(
R"((function(util) {
util.validateCustomSignature('custom_signature', [1, 2]);
......
......@@ -313,22 +313,17 @@ TEST_F(APIBindingUnittest, TestBasicAPICalls) {
ExpectPass(binding_object, "obj.oneString('foo');", "['foo']", false);
ExpectFailure(
binding_object, "obj.oneString(1);",
InvocationError(
"test.oneString", "string str",
ArgumentError("str", InvalidType(kTypeString, kTypeInteger))));
InvocationError("test.oneString", "string str", NoMatchingSignature()));
ExpectPass(binding_object, "obj.stringAndInt('foo', 1)", "['foo',1]", false);
ExpectFailure(
binding_object, "obj.stringAndInt(1)",
InvocationError(
"test.stringAndInt", "string str, integer int",
ArgumentError("str", InvalidType(kTypeString, kTypeInteger))));
ExpectFailure(binding_object, "obj.stringAndInt(1)",
InvocationError("test.stringAndInt", "string str, integer int",
NoMatchingSignature()));
ExpectPass(binding_object, "obj.intAndCallback(1, function() {})", "[1]",
true);
ExpectFailure(
binding_object, "obj.intAndCallback(function() {})",
InvocationError(
"test.intAndCallback", "integer int, function callback",
ArgumentError("int", InvalidType(kTypeInteger, kTypeFunction))));
InvocationError("test.intAndCallback", "integer int, function callback",
NoMatchingSignature()));
// ...And an interesting case (throwing an error during parsing).
ExpectThrow(binding_object,
......@@ -837,9 +832,7 @@ TEST_F(APIBindingUnittest, TestJSCustomHook) {
// the hook should never have been called, since the arguments didn't match.
ExpectFailure(
binding_object, "obj.oneString(1);",
InvocationError(
"test.oneString", "string str",
ArgumentError("str", InvalidType(kTypeString, kTypeInteger))));
InvocationError("test.oneString", "string str", NoMatchingSignature()));
v8::Local<v8::Value> property =
GetPropertyFromObject(context->Global(), context, "requestArguments");
ASSERT_FALSE(property.IsEmpty());
......@@ -892,9 +885,7 @@ TEST_F(APIBindingUnittest, TestUpdateArgumentsPreValidate) {
// have the hook called.
ExpectFailure(
binding_object, "obj.oneString(false);",
InvocationError(
"test.oneString", "string str",
ArgumentError("str", InvalidType(kTypeString, kTypeBoolean))));
InvocationError("test.oneString", "string str", NoMatchingSignature()));
EXPECT_EQ("[false]", GetStringPropertyFromObject(
context->Global(), context, "requestArguments"));
......@@ -1125,9 +1116,7 @@ TEST_F(APIBindingUnittest, TestUpdateArgumentsPostValidate) {
// should never enter the hook.
ExpectFailure(
binding_object, "obj.oneString(false);",
InvocationError(
"test.oneString", "string str",
ArgumentError("str", InvalidType(kTypeString, kTypeBoolean))));
InvocationError("test.oneString", "string str", NoMatchingSignature()));
EXPECT_EQ("undefined", GetStringPropertyFromObject(
context->Global(), context, "requestArguments"));
......
......@@ -92,6 +92,10 @@ std::string TooManyArguments() {
return "Too many arguments.";
}
std::string NoMatchingSignature() {
return "No matching signature.";
}
std::string MissingRequiredArgument(const char* argument_name) {
return base::StringPrintf("Missing required argument '%s'.", argument_name);
}
......
......@@ -46,6 +46,7 @@ std::string UnserializableValue();
std::string ScriptThrewError();
std::string TooManyArguments();
std::string MissingRequiredArgument(const char* argument_name);
std::string NoMatchingSignature();
// Returns an message indicating an error was found while parsing a given index
// in an array.
......
......@@ -39,7 +39,7 @@ class ArgumentParser {
std::string* error)
: context_(context),
signature_(signature),
arguments_(arguments),
provided_arguments_(arguments),
type_refs_(type_refs),
error_(error) {}
......@@ -50,24 +50,31 @@ class ArgumentParser {
v8::Isolate* GetIsolate() { return context_->GetIsolate(); }
private:
v8::Local<v8::Value> next_argument() {
return current_index_ < arguments_.size() ?
arguments_[current_index_] : v8::Local<v8::Value>();
}
void ConsumeArgument() {
current_index_ = std::min(arguments_.size(), current_index_ + 1);
}
// API methods can have multiple possible signatures. For instance, an API
// method that takes (optional int, string) could be invoked with either
// an int and string, or just a string. ResolveArguments() takes the
// |provided| arguments and the |expected| signature, and populates |result|
// with a normalized array of values such that each entry in |result| is
// positionally correct with the signature. Omitted arguments will be
// empty v8::Local<v8::Value> handles in the array.
// Returns true if the arguments were successfully resolved.
// Note: This only checks arguments against their basic types, not other
// values (like specific required properties or values).
bool ResolveArguments(
base::span<const v8::Local<v8::Value>> provided,
base::span<const std::unique_ptr<ArgumentSpec>> expected,
std::vector<v8::Local<v8::Value>>* result,
size_t index);
// Attempts to match the next argument to the given |spec|.
// If the next argument does not match and |spec| is optional, uses a null
// value.
// Returns true on success.
bool ParseArgument(const ArgumentSpec& spec);
bool ParseArgument(const ArgumentSpec& spec, v8::Local<v8::Value> value);
// Attempts to parse the callback from the given |spec|. Returns true on
// success.
bool ParseCallback(const ArgumentSpec& spec);
bool ParseCallback(const ArgumentSpec& spec, v8::Local<v8::Value> value);
// Adds a null value to the parsed arguments.
virtual void AddNull() = 0;
......@@ -83,10 +90,9 @@ class ArgumentParser {
v8::Local<v8::Context> context_;
const std::vector<std::unique_ptr<ArgumentSpec>>& signature_;
const std::vector<v8::Local<v8::Value>>& arguments_;
const std::vector<v8::Local<v8::Value>>& provided_arguments_;
const APITypeReferenceMap& type_refs_;
std::string* error_;
size_t current_index_ = 0;
// An error to pass while parsing arguments to avoid having to allocate a new
// std::string on the stack multiple times.
......@@ -171,80 +177,129 @@ class BaseValueArgumentParser : public ArgumentParser {
};
bool ArgumentParser::ParseArguments() {
if (arguments_.size() > signature_.size()) {
*error_ = api_errors::TooManyArguments();
if (provided_arguments_.size() > signature_.size()) {
*error_ = api_errors::NoMatchingSignature();
return false;
}
bool signature_has_callback = HasCallback(signature_);
std::vector<v8::Local<v8::Value>> resolved_arguments(signature_.size());
if (!ResolveArguments(provided_arguments_, signature_, &resolved_arguments,
0u)) {
*error_ = api_errors::NoMatchingSignature();
return false;
}
DCHECK_EQ(resolved_arguments.size(), signature_.size());
bool signature_has_callback = HasCallback(signature_);
size_t end_size =
signature_has_callback ? signature_.size() - 1 : signature_.size();
for (size_t i = 0; i < end_size; ++i) {
if (!ParseArgument(*signature_[i]))
if (!ParseArgument(*signature_[i], resolved_arguments[i]))
return false;
}
if (signature_has_callback && !ParseCallback(*signature_.back()))
if (signature_has_callback &&
!ParseCallback(*signature_.back(), resolved_arguments.back())) {
return false;
if (current_index_ != arguments_.size()) {
// This can potentially happen even if the check above for too many
// arguments succeeds when optional parameters are omitted. For instance,
// if the signature expects (optional int, function callback) and the caller
// provides (function callback, object random), the first size check and
// callback spec would succeed, but we wouldn't consume all the arguments.
*error_ = api_errors::TooManyArguments();
return false; // Extra arguments aren't allowed.
}
return true;
}
bool ArgumentParser::ParseArgument(const ArgumentSpec& spec) {
v8::Local<v8::Value> value = next_argument();
if (value.IsEmpty() || value->IsNull() || value->IsUndefined()) {
if (!spec.optional()) {
*error_ = api_errors::MissingRequiredArgument(spec.name().c_str());
return false;
bool ArgumentParser::ResolveArguments(
base::span<const v8::Local<v8::Value>> provided,
base::span<const std::unique_ptr<ArgumentSpec>> expected,
std::vector<v8::Local<v8::Value>>* result,
size_t index) {
// If the provided arguments and expected arguments are both empty, it means
// we've successfully matched all provided arguments to the expected
// signature.
if (provided.empty() && expected.empty())
return true;
// If there are more provided arguments than expected arguments, there's no
// possible signature that could match.
if (provided.size() > expected.size())
return false;
DCHECK(!expected.empty());
// If there are more provided arguments (and more expected arguments, as
// guaranteed above), check if the next argument could match the next expected
// argument.
if (!provided.empty()) {
// The argument could potentially match if it is either null or undefined
// and an optional argument, or if it's the correct expected type.
bool can_match = false;
if (expected[0]->optional() && provided[0]->IsNullOrUndefined()) {
can_match = true;
// For null/undefined, just use an empty handle. It'll be normalized to
// null in ParseArgument().
(*result)[index] = v8::Local<v8::Value>();
} else if (expected[0]->IsCorrectType(provided[0], type_refs_, error_)) {
can_match = true;
(*result)[index] = provided[0];
}
// This is safe to call even if |arguments| is at the end (which can happen
// if n optional arguments are omitted at the end of the signature).
ConsumeArgument();
AddNull();
return true;
// If the provided argument could potentially match the next expected
// argument, assume it does, and try to match the remaining arguments.
// This recursion is safe because it's bounded by the number of arguments
// present in the signature. Additionally, though this is 2^n complexity,
// <n> is bounded by the number of expected arguments, which is almost
// always small. Further, it is only when parameters are optional, which is
// also not the default.
if (can_match && ResolveArguments(provided.subspan(1), expected.subspan(1),
result, index + 1)) {
return true;
}
}
if (!spec.IsCorrectType(value, type_refs_, &parse_error_)) {
if (!spec.optional()) {
*error_ = api_errors::ArgumentError(spec.name(), parse_error_);
return false;
}
// One of three cases happened:
// - There are no more provided arguments.
// - The next provided argument could not match the expected argument.
// - The next provided argument could match the expected argument, but
// subsequent arguments did not.
// In all of these cases, if the expected argument was optional, assume it
// was omitted, and try matching subsequent arguments.
if (expected[0]->optional()) {
// Assume the expected argument was omitted.
(*result)[index] = v8::Local<v8::Value>();
// See comments above for recursion notes.
if (ResolveArguments(provided, expected.subspan(1), result, index + 1))
return true;
}
// A required argument was not matched.
return false;
}
bool ArgumentParser::ParseArgument(const ArgumentSpec& spec,
v8::Local<v8::Value> value) {
if (value.IsEmpty()) {
// ResolveArguments() should only allow empty values for optional arguments.
DCHECK(spec.optional());
AddNull();
return true;
}
// ResolveArguments() should verify that all arguments are at least the
// correct type.
DCHECK(spec.IsCorrectType(value, type_refs_, error_));
if (!spec.ParseArgument(context_, value, type_refs_, GetBaseBuffer(),
GetV8Buffer(), &parse_error_)) {
*error_ = api_errors::ArgumentError(spec.name(), parse_error_);
return false;
}
ConsumeArgument();
AddParsedArgument();
return true;
}
bool ArgumentParser::ParseCallback(const ArgumentSpec& spec) {
v8::Local<v8::Value> value = next_argument();
if (value.IsEmpty() || value->IsNull() || value->IsUndefined()) {
if (!spec.optional()) {
*error_ = api_errors::MissingRequiredArgument(spec.name().c_str());
return false;
}
ConsumeArgument();
bool ArgumentParser::ParseCallback(const ArgumentSpec& spec,
v8::Local<v8::Value> value) {
if (value.IsEmpty()) {
// ResolveArguments() should only allow empty values for optional arguments.
DCHECK(spec.optional());
AddNullCallback();
return true;
}
......@@ -257,7 +312,6 @@ bool ArgumentParser::ParseCallback(const ArgumentSpec& spec) {
return false;
}
ConsumeArgument();
SetCallback(value.As<v8::Function>());
return true;
}
......
......@@ -138,6 +138,22 @@ std::unique_ptr<APISignature> OptionalObjectAndCallback() {
return std::make_unique<APISignature>(std::move(specs));
}
std::unique_ptr<APISignature> OptionalIntAndNumber() {
SpecVector specs;
specs.push_back(
ArgumentSpecBuilder(ArgumentType::INTEGER, "int").MakeOptional().Build());
specs.push_back(ArgumentSpecBuilder(ArgumentType::DOUBLE, "num").Build());
return std::make_unique<APISignature>(std::move(specs));
}
std::unique_ptr<APISignature> OptionalIntAndInt() {
SpecVector specs;
specs.push_back(
ArgumentSpecBuilder(ArgumentType::INTEGER, "int").MakeOptional().Build());
specs.push_back(ArgumentSpecBuilder(ArgumentType::INTEGER, "int2").Build());
return std::make_unique<APISignature>(std::move(specs));
}
std::vector<v8::Local<v8::Value>> StringToV8Vector(
v8::Local<v8::Context> context,
const char* args) {
......@@ -234,48 +250,39 @@ TEST_F(APISignatureTest, BasicSignatureParsing) {
v8::HandleScope handle_scope(isolate());
{
SCOPED_TRACE("OneString");
auto signature = OneString();
ExpectPass(*signature, "['foo']", "['foo']", false);
ExpectPass(*signature, "['']", "['']", false);
ExpectFailure(
*signature, "[1]",
ArgumentError("string", InvalidType(kTypeString, kTypeInteger)));
ExpectFailure(*signature, "[]", MissingRequiredArgument("string"));
ExpectFailure(
*signature, "[{}]",
ArgumentError("string", InvalidType(kTypeString, kTypeObject)));
ExpectFailure(*signature, "['foo', 'bar']", TooManyArguments());
ExpectFailure(*signature, "[1]", NoMatchingSignature());
ExpectFailure(*signature, "[]", NoMatchingSignature());
ExpectFailure(*signature, "[{}]", NoMatchingSignature());
ExpectFailure(*signature, "['foo', 'bar']", NoMatchingSignature());
}
{
SCOPED_TRACE("StringAndInt");
auto signature = StringAndInt();
ExpectPass(*signature, "['foo', 42]", "['foo',42]", false);
ExpectPass(*signature, "['foo', -1]", "['foo',-1]", false);
ExpectFailure(
*signature, "[1]",
ArgumentError("string", InvalidType(kTypeString, kTypeInteger)));
ExpectFailure(*signature, "['foo'];", MissingRequiredArgument("int"));
ExpectFailure(
*signature, "[1, 'foo']",
ArgumentError("string", InvalidType(kTypeString, kTypeInteger)));
ExpectFailure(*signature, "['foo', 'foo']",
ArgumentError("int", InvalidType(kTypeInteger, kTypeString)));
ExpectFailure(*signature, "['foo', '1']",
ArgumentError("int", InvalidType(kTypeInteger, kTypeString)));
ExpectFailure(*signature, "['foo', 2.3]",
ArgumentError("int", InvalidType(kTypeInteger, kTypeDouble)));
ExpectFailure(*signature, "[1]", NoMatchingSignature());
ExpectFailure(*signature, "['foo'];", NoMatchingSignature());
ExpectFailure(*signature, "[1, 'foo']", NoMatchingSignature());
ExpectFailure(*signature, "['foo', 'foo']", NoMatchingSignature());
ExpectFailure(*signature, "['foo', '1']", NoMatchingSignature());
ExpectFailure(*signature, "['foo', 2.3]", NoMatchingSignature());
}
{
SCOPED_TRACE("StringOptionalIntAndBool");
auto signature = StringOptionalIntAndBool();
ExpectPass(*signature, "['foo', 42, true]", "['foo',42,true]", false);
ExpectPass(*signature, "['foo', true]", "['foo',null,true]", false);
ExpectFailure(
*signature, "['foo', 'bar', true]",
ArgumentError("bool", InvalidType(kTypeBoolean, kTypeString)));
ExpectFailure(*signature, "['foo', 'bar', true]", NoMatchingSignature());
}
{
SCOPED_TRACE("OneObject");
auto signature = OneObject();
ExpectPass(*signature, "[{prop1: 'foo'}]", "[{'prop1':'foo'}]", false);
ExpectFailure(*signature,
......@@ -284,41 +291,42 @@ TEST_F(APISignatureTest, BasicSignatureParsing) {
}
{
SCOPED_TRACE("NoArgs");
auto signature = NoArgs();
ExpectPass(*signature, "[]", "[]", false);
ExpectFailure(*signature, "[0]", TooManyArguments());
ExpectFailure(*signature, "['']", TooManyArguments());
ExpectFailure(*signature, "[null]", TooManyArguments());
ExpectFailure(*signature, "[undefined]", TooManyArguments());
ExpectFailure(*signature, "[0]", NoMatchingSignature());
ExpectFailure(*signature, "['']", NoMatchingSignature());
ExpectFailure(*signature, "[null]", NoMatchingSignature());
ExpectFailure(*signature, "[undefined]", NoMatchingSignature());
}
{
SCOPED_TRACE("IntAndCallback");
auto signature = IntAndCallback();
ExpectPass(*signature, "[1, function() {}]", "[1]", true);
ExpectFailure(
*signature, "[function() {}]",
ArgumentError("int", InvalidType(kTypeInteger, kTypeFunction)));
ExpectFailure(*signature, "[1]", MissingRequiredArgument("callback"));
ExpectFailure(*signature, "[function() {}]", NoMatchingSignature());
ExpectFailure(*signature, "[1]", NoMatchingSignature());
}
{
SCOPED_TRACE("OptionalIntAndCallback");
auto signature = OptionalIntAndCallback();
ExpectPass(*signature, "[1, function() {}]", "[1]", true);
ExpectPass(*signature, "[function() {}]", "[null]", true);
ExpectFailure(*signature, "[1]", MissingRequiredArgument("callback"));
ExpectFailure(*signature, "[1]", NoMatchingSignature());
}
{
SCOPED_TRACE("OptionalCallback");
auto signature = OptionalCallback();
ExpectPass(*signature, "[function() {}]", "[]", true);
ExpectPass(*signature, "[]", "[]", false);
ExpectPass(*signature, "[undefined]", "[]", false);
ExpectFailure(
*signature, "[0]",
ArgumentError("callback", InvalidType(kTypeFunction, kTypeInteger)));
ExpectFailure(*signature, "[0]", NoMatchingSignature());
}
{
SCOPED_TRACE("IntAnyOptionalObjectOptionalCallback");
auto signature = IntAnyOptionalObjectOptionalCallback();
ExpectPass(*signature, "[4, {foo: 'bar'}, function() {}]",
"[4,{'foo':'bar'},null]", true);
......@@ -328,10 +336,11 @@ TEST_F(APISignatureTest, BasicSignatureParsing) {
false);
ExpectFailure(*signature, "[4, function() {}]",
ArgumentError("any", UnserializableValue()));
ExpectFailure(*signature, "[4]", MissingRequiredArgument("any"));
ExpectFailure(*signature, "[4]", NoMatchingSignature());
}
{
SCOPED_TRACE("OptionalObjectAndCallback");
auto signature = OptionalObjectAndCallback();
ExpectPass(*signature, "[{prop1: 1}]", "[{'prop1':1}]", false);
ExpectPass(*signature, "[]", "[null]", false);
......@@ -345,6 +354,28 @@ TEST_F(APISignatureTest, BasicSignatureParsing) {
ArgumentError("obj", PropertyError("prop1", InvalidType(kTypeInteger,
kTypeString))));
}
{
SCOPED_TRACE("OptionalIntAndNumber");
auto signature = OptionalIntAndNumber();
ExpectPass(*signature, "[1.0, 1.0]", "[1,1.0]", false);
ExpectPass(*signature, "[1, 1]", "[1,1.0]", false);
ExpectPass(*signature, "[1.0]", "[null,1.0]", false);
ExpectPass(*signature, "[1]", "[null,1.0]", false);
ExpectFailure(*signature, "[1.0, null]", NoMatchingSignature());
ExpectFailure(*signature, "[1, null]", NoMatchingSignature());
}
{
SCOPED_TRACE("OptionalIntAndInt");
auto signature = OptionalIntAndInt();
ExpectPass(*signature, "[1.0, 1.0]", "[1,1]", false);
ExpectPass(*signature, "[1, 1]", "[1,1]", false);
ExpectPass(*signature, "[1.0]", "[null,1]", false);
ExpectPass(*signature, "[1]", "[null,1]", false);
ExpectFailure(*signature, "[1.0, null]", NoMatchingSignature());
ExpectFailure(*signature, "[1, null]", NoMatchingSignature());
}
}
TEST_F(APISignatureTest, TypeRefsTest) {
......
......@@ -88,10 +88,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, Basic) {
api_errors::InvocationError(
"idle.queryState",
"integer detectionIntervalInSeconds, function callback",
api_errors::ArgumentError(
"detectionIntervalInSeconds",
api_errors::InvalidType(api_errors::kTypeInteger,
api_errors::kTypeString))));
api_errors::NoMatchingSignature()));
}
{
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment